[pve-devel] [PATCH access-control 0/3] improve tfa config locking

[pve-devel] [PATCH access-control 0/3] improve tfa config locking

[Dominik Csapak]

intended as a replacement for my previous patch: [0]

while we may not want users to login into a non-quorate cluster,
preventing it as a side-effect of locking the tfa config is wrong.

currently there is only one situation where we actually need to lock
the tfa config, namely when using recovery keys, since they have to be
removed from it. so this series changes the tfa code in pve so that
we only lock when the tfa response is a recovery key

an alternative approach to this would be to implement a 'needs save'
check in rust and call that with the tfa-response, but we can still do
that later

patches 2 and 3 are debatable, but i found it makes the code a bit clearer

my suggestion for the 'let users not login in non-quorate cluster' would
be to maybe add a flag to the users that must be explicitely enabled
for them to login, so that e.g. some admin users can always login, but
normal users cannot (i got no real feedback on that idea in the
conversation of the last version of this sadly..)

0: https://lists.proxmox.com/pipermail/pve-devel/2022-September/053939.html

Dominik Csapak (3):
  authenticate_2nd_new: only lock tfa config for recovery keys
  authenticate_2nd_new: rename $otp to $tfa_response
  authenticate_user: don't give empty $tfa_challenge to
    authenticate_2nd_new

 src/PVE/AccessControl.pm | 131 +++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 60 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH access-control 1/3] authenticate_2nd_new: only lock tfa config for recovery keys

[Dominik Csapak]

we currently only need to lock the tfa config when we got a recovery key
as a tfa challenge response, since that's the only thing that can
actually change the tfa config (every other method only reads from
there).

so to do that, factor out the code that was inside the lock, and call it
with/without lock depending on the tfa challenge response

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/AccessControl.pm | 127 +++++++++++++++++++++------------------
 1 file changed, 69 insertions(+), 58 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index c32dcc3..c45188c 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -786,79 +786,90 @@ sub authenticate_2nd_old : prototype($$$) {
     return wantarray ? ($username, $tfa_data) : $username;
 }
 
-# Returns a tfa challenge or undef.
-sub authenticate_2nd_new : prototype($$$$) {
+sub authenticate_2nd_new_do : prototype($$$$) {
     my ($username, $realm, $otp, $tfa_challenge) = @_;
+    my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1);
 
-    my $result = lock_tfa_config(sub {
-	my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1);
-
-	if (!defined($tfa_cfg)) {
-	    return undef;
-	}
-
-	my $realm_type = $realm_tfa && $realm_tfa->{type};
-	# verify realm type unless using recovery keys:
-	if (defined($realm_type)) {
-	    $realm_type = 'totp' if $realm_type eq 'oath'; # we used to call it that
-	    if ($realm_type eq 'yubico') {
-		# Yubico auth will not be supported in rust for now...
-		if (!defined($tfa_challenge)) {
-		    my $challenge = { yubico => JSON::true };
-		    # Even with yubico auth we do allow recovery keys to be used:
-		    if (my $recovery = $tfa_cfg->recovery_state($username)) {
-			$challenge->{recovery} = $recovery;
-		    }
-		    return to_json($challenge);
-		}
+    if (!defined($tfa_cfg)) {
+	return undef;
+    }
 
-		if ($otp =~ /^yubico:(.*)$/) {
-		    $otp = $1;
-		    # Defer to after unlocking the TFA config:
-		    return sub {
-			authenticate_yubico_new(
-			    $tfa_cfg, $username, $realm_tfa, $tfa_challenge, $otp,
-			);
-		    };
+    my $realm_type = $realm_tfa && $realm_tfa->{type};
+    # verify realm type unless using recovery keys:
+    if (defined($realm_type)) {
+	$realm_type = 'totp' if $realm_type eq 'oath'; # we used to call it that
+	if ($realm_type eq 'yubico') {
+	    # Yubico auth will not be supported in rust for now...
+	    if (!defined($tfa_challenge)) {
+		my $challenge = { yubico => JSON::true };
+		# Even with yubico auth we do allow recovery keys to be used:
+		if (my $recovery = $tfa_cfg->recovery_state($username)) {
+		    $challenge->{recovery} = $recovery;
 		}
+		return to_json($challenge);
 	    }
 
-	    my $response_type;
-	    if (defined($otp)) {
-		if ($otp !~ /^([^:]+):/) {
-		    die "bad otp response\n";
-		}
-		$response_type = $1;
+	    if ($otp =~ /^yubico:(.*)$/) {
+		$otp = $1;
+		# Defer to after unlocking the TFA config:
+		return sub {
+		    authenticate_yubico_new(
+			$tfa_cfg, $username, $realm_tfa, $tfa_challenge, $otp,
+		    );
+		};
 	    }
+	}
 
-	    die "realm requires $realm_type authentication\n"
-		if $response_type && $response_type ne 'recovery' && $response_type ne $realm_type;
+	my $response_type;
+	if (defined($otp)) {
+	    if ($otp !~ /^([^:]+):/) {
+		die "bad otp response\n";
+	    }
+	    $response_type = $1;
 	}
 
-	configure_u2f_and_wa($tfa_cfg);
+	die "realm requires $realm_type authentication\n"
+	    if $response_type && $response_type ne 'recovery' && $response_type ne $realm_type;
+    }
 
-	my $must_save = 0;
-	if (defined($tfa_challenge)) {
-	    $tfa_challenge = verify_ticket($tfa_challenge, 0, $username);
-	    $must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
-	    $tfa_challenge = undef;
-	} else {
-	    $tfa_challenge = $tfa_cfg->authentication_challenge($username);
-	    if (defined($otp)) {
-		if (defined($tfa_challenge)) {
-		    $must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
-		} else {
-		    die "no such challenge\n";
-		}
+    configure_u2f_and_wa($tfa_cfg);
+
+    my $must_save = 0;
+    if (defined($tfa_challenge)) {
+	$tfa_challenge = verify_ticket($tfa_challenge, 0, $username);
+	$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
+	$tfa_challenge = undef;
+    } else {
+	$tfa_challenge = $tfa_cfg->authentication_challenge($username);
+	if (defined($otp)) {
+	    if (defined($tfa_challenge)) {
+		$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
+	    } else {
+		die "no such challenge\n";
 	    }
 	}
+    }
 
-	if ($must_save) {
-	    cfs_write_file('priv/tfa.cfg', $tfa_cfg);
-	}
+    if ($must_save) {
+	cfs_write_file('priv/tfa.cfg', $tfa_cfg);
+    }
 
-	return $tfa_challenge;
-    });
+    return $tfa_challenge;
+}
+
+# Returns a tfa challenge or undef.
+sub authenticate_2nd_new : prototype($$$$) {
+    my ($username, $realm, $otp, $tfa_challenge) = @_;
+
+    my $result;
+
+    if (defined($otp) && $otp =~ m/^recovery:$/) {
+	$result = lock_tfa_config(sub {
+	    authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
+	});
+    } else {
+	$result = authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
+    }
 
     # Yubico auth returns the authentication sub:
     if (ref($result) eq 'CODE') {
-- 
2.30.2

[pve-devel] [PATCH access-control 2/3] authenticate_2nd_new: rename $otp to $tfa_response

[Dominik Csapak]

since that is what it really is, not only a otp

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/AccessControl.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index c45188c..d83dee2 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -787,7 +787,7 @@ sub authenticate_2nd_old : prototype($$$) {
 }
 
 sub authenticate_2nd_new_do : prototype($$$$) {
-    my ($username, $realm, $otp, $tfa_challenge) = @_;
+    my ($username, $realm, $tfa_response, $tfa_challenge) = @_;
     my ($tfa_cfg, $realm_tfa) = user_get_tfa($username, $realm, 1);
 
     if (!defined($tfa_cfg)) {
@@ -809,20 +809,20 @@ sub authenticate_2nd_new_do : prototype($$$$) {
 		return to_json($challenge);
 	    }
 
-	    if ($otp =~ /^yubico:(.*)$/) {
-		$otp = $1;
+	    if ($tfa_response =~ /^yubico:(.*)$/) {
+		$tfa_response = $1;
 		# Defer to after unlocking the TFA config:
 		return sub {
 		    authenticate_yubico_new(
-			$tfa_cfg, $username, $realm_tfa, $tfa_challenge, $otp,
+			$tfa_cfg, $username, $realm_tfa, $tfa_challenge, $tfa_response,
 		    );
 		};
 	    }
 	}
 
 	my $response_type;
-	if (defined($otp)) {
-	    if ($otp !~ /^([^:]+):/) {
+	if (defined($tfa_response)) {
+	    if ($tfa_response !~ /^([^:]+):/) {
 		die "bad otp response\n";
 	    }
 	    $response_type = $1;
@@ -837,13 +837,13 @@ sub authenticate_2nd_new_do : prototype($$$$) {
     my $must_save = 0;
     if (defined($tfa_challenge)) {
 	$tfa_challenge = verify_ticket($tfa_challenge, 0, $username);
-	$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
+	$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $tfa_response);
 	$tfa_challenge = undef;
     } else {
 	$tfa_challenge = $tfa_cfg->authentication_challenge($username);
-	if (defined($otp)) {
+	if (defined($tfa_response)) {
 	    if (defined($tfa_challenge)) {
-		$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $otp);
+		$must_save = $tfa_cfg->authentication_verify($username, $tfa_challenge, $tfa_response);
 	    } else {
 		die "no such challenge\n";
 	    }
@@ -859,16 +859,16 @@ sub authenticate_2nd_new_do : prototype($$$$) {
 
 # Returns a tfa challenge or undef.
 sub authenticate_2nd_new : prototype($$$$) {
-    my ($username, $realm, $otp, $tfa_challenge) = @_;
+    my ($username, $realm, $tfa_response, $tfa_challenge) = @_;
 
     my $result;
 
-    if (defined($otp) && $otp =~ m/^recovery:$/) {
+    if (defined($tfa_response) && $tfa_response =~ m/^recovery:$/) {
 	$result = lock_tfa_config(sub {
-	    authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
+	    authenticate_2nd_new_do($username, $realm, $tfa_response, $tfa_challenge);
 	});
     } else {
-	$result = authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
+	$result = authenticate_2nd_new_do($username, $realm, $tfa_response, $tfa_challenge);
     }
 
     # Yubico auth returns the authentication sub:
-- 
2.30.2

[pve-devel] [PATCH access-control 3/3] authenticate_user: don't give empty $tfa_challenge to authenticate_2nd_new

[Dominik Csapak]

just above, we check & return if $tfa_challenge is set, so there is no
way that it would be set here. To make it clearer that it must be undef
here, just omit it in the call.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/AccessControl.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index d83dee2..ca36db9 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -746,7 +746,7 @@ sub authenticate_user : prototype($$$$;$) {
 
     if ($new_format) {
 	# This is the first factor with an optional immediate 2nd factor for TOTP:
-	my $tfa_challenge = authenticate_2nd_new($username, $realm, $otp, $tfa_challenge);
+	my $tfa_challenge = authenticate_2nd_new($username, $realm, $otp);
 	return wantarray ? ($username, $tfa_challenge) : $username;
     } else {
 	return authenticate_2nd_old($username, $realm, $otp);
-- 
2.30.2

Re: [pve-devel] [PATCH access-control 1/3] authenticate_2nd_new: only lock tfa config for recovery keys

[Wolfgang Bumiller]

On Thu, Oct 20, 2022 at 03:14:10PM +0200, Dominik Csapak wrote:
>(...)
> +# Returns a tfa challenge or undef.
> +sub authenticate_2nd_new : prototype($$$$) {
> +    my ($username, $realm, $otp, $tfa_challenge) = @_;
> +
> +    my $result;
> +
> +    if (defined($otp) && $otp =~ m/^recovery:$/) {

That regex should never trigger ;-)
(The '$' at the end is wrong)

> +	$result = lock_tfa_config(sub {
> +	    authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
> +	});
> +    } else {
> +	$result = authenticate_2nd_new_do($username, $realm, $otp, $tfa_challenge);
> +    }
>  
>      # Yubico auth returns the authentication sub:
>      if (ref($result) eq 'CODE') {
> -- 
> 2.30.2

Re: [pve-devel] [PATCH access-control 3/3] authenticate_user: don't give empty $tfa_challenge to authenticate_2nd_new

[Wolfgang Bumiller]

On Thu, Oct 20, 2022 at 03:14:12PM +0200, Dominik Csapak wrote:
> just above, we check & return if $tfa_challenge is set, so there is no
> way that it would be set here. To make it clearer that it must be undef
> here, just omit it in the call.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/AccessControl.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index d83dee2..ca36db9 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -746,7 +746,7 @@ sub authenticate_user : prototype($$$$;$) {
>  
>      if ($new_format) {
>  	# This is the first factor with an optional immediate 2nd factor for TOTP:
> -	my $tfa_challenge = authenticate_2nd_new($username, $realm, $otp, $tfa_challenge);
> +	my $tfa_challenge = authenticate_2nd_new($username, $realm, $otp);

I'd prefer to explicitly pass `undef`, as I also prefer to have
prototypes on subs which would not allow this ;-)

>  	return wantarray ? ($username, $tfa_challenge) : $username;
>      } else {
>  	return authenticate_2nd_old($username, $realm, $otp);
> -- 
> 2.30.2

Re: [pve-devel] [PATCH access-control 0/3] improve tfa config locking

[Wolfgang Bumiller]

On Thu, Oct 20, 2022 at 03:14:09PM +0200, Dominik Csapak wrote:
> intended as a replacement for my previous patch: [0]
> 
> while we may not want users to login into a non-quorate cluster,
> preventing it as a side-effect of locking the tfa config is wrong.
> 
> currently there is only one situation where we actually need to lock
> the tfa config, namely when using recovery keys, since they have to be
> removed from it. so this series changes the tfa code in pve so that
> we only lock when the tfa response is a recovery key
> 
> an alternative approach to this would be to implement a 'needs save'
> check in rust and call that with the tfa-response, but we can still do
> that later
> 
> patches 2 and 3 are debatable, but i found it makes the code a bit clearer
> 
> my suggestion for the 'let users not login in non-quorate cluster' would
> be to maybe add a flag to the users that must be explicitely enabled
> for them to login, so that e.g. some admin users can always login, but
> normal users cannot (i got no real feedback on that idea in the
> conversation of the last version of this sadly..)

I think it makes sense. Eg. you may not want to expose ssh access
publicly but need the UI - then at least root could access the shell
over the UI to fix stuff, while for other users we can never be sure
they're actually still valid. Although we could argue @pam users should
be allowed to login as well, since those are machine-local after all?
But as far as I'm concerned, even root@pam-only for non-quorate nodes
would make enough sense.

Re: [pve-devel] [PATCH access-control 0/3] improve tfa config locking

[Thomas Lamprecht]

Am 21/10/2022 um 10:06 schrieb Wolfgang Bumiller:
>> my suggestion for the 'let users not login in non-quorate cluster' would
>> be to maybe add a flag to the users that must be explicitely enabled
>> for them to login, so that e.g. some admin users can always login, but
>> normal users cannot (i got no real feedback on that idea in the
>> conversation of the last version of this sadly..)
>
> I think it makes sense. Eg. you may not want to expose ssh access
> publicly but need the UI - then at least root could access the shell
> over the UI to fix stuff, while for other users we can never be sure
> they're actually still valid. Although we could argue @pam users should
> be allowed to login as well, since those are machine-local after all?
> But as far as I'm concerned, even root@pam-only for non-quorate nodes
> would make enough sense.

That's something else than the flag Dominik proposed though, would special
case @pam yet another time, but at least it has some arguments and make more
sense than we do for the host shell... Biggest benefit, no config required
at all.

So yeah that in form of an implementation and docs patch would be nice.