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

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

[Dominik Csapak]

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

changes from v1:
* fix wrong regex
* pass undef explicitely instead of omitting the parameter
* this time tested in both directions:
  user without tfa can login in quorate & non-quorate cluster
  user with non-recovery tfa can login in quorate & non-quorate cluster
  user with recovery tfa can only login in quorate cluser

Dominik Csapak (3):
  authenticate_2nd_new: only lock tfa config for recovery keys
  authenticate_2nd_new: rename $otp to $tfa_response
  authenticate_user: pass undef instead of 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 v2 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..f4ea358 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 v2 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 f4ea358..3d77bed 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 v2 3/3] authenticate_user: pass undef instead of 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 pass it as such.

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 3d77bed..5addcbf 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, undef);
 	return wantarray ? ($username, $tfa_challenge) : $username;
     } else {
 	return authenticate_2nd_old($username, $realm, $otp);
-- 
2.30.2

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

[Wolfgang Bumiller]

applied series, thanks

On Fri, Oct 21, 2022 at 10:31:14AM +0200, Dominik Csapak wrote:
> 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
> 
> changes from v1:
> * fix wrong regex
> * pass undef explicitely instead of omitting the parameter
> * this time tested in both directions:
>   user without tfa can login in quorate & non-quorate cluster
>   user with non-recovery tfa can login in quorate & non-quorate cluster
>   user with recovery tfa can only login in quorate cluser
> 
> Dominik Csapak (3):
>   authenticate_2nd_new: only lock tfa config for recovery keys
>   authenticate_2nd_new: rename $otp to $tfa_response
>   authenticate_user: pass undef instead of empty $tfa_challenge to
>     authenticate_2nd_new
> 
>  src/PVE/AccessControl.pm | 131 +++++++++++++++++++++------------------
>  1 file changed, 71 insertions(+), 60 deletions(-)
> 
> -- 
> 2.30.2

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

[Thomas Lamprecht]

Can we *please* get sane commit subject for *human* consumption?!

The worst one, that really triggers me:
> authenticate_user: pass undef instead of empty $tfa_challenge to authenticate_2nd_new
Instead of a high level human overview it gives basically another almost
machine readable diff of the lower level changes. So there's close to zero
of useful higher level and/or _new_ info in there, but sure makes one stop
to parse on seeing this in some log.

Copying the method name (nor file module name!) is basically never a good
idea, especially as tag and especially for (admin) user facing changes - it
can naturally be OK for library stuff (i.e., dev-only facing changes), but
even then seldom as start `<tag>:`...

following would allow for a quicker to read, and (granted, slightly
subjective) better high level understanding of the commits, thus better
categorizing for when skimming through the log, e.g., in search of relevant
changes due to some new/questionable/.. behavior; besides that it makes
d/changelog writing easier.

* tfa: only lock config for recovery keys on authentication
* tfa: rename outdated $otp variable to $tfa_response
  (this is internal and won't ever make it in the d/changelog so only dev
  readability matters)
* auth: drop passing bogus challenge variable when checking first factor
  (also internally, dev oriented)

This should be also verified and either amened or commented on by the
maintainer (planning) pushing a commit/series - while its obviously 1) hard
and 2) a lot overhead to have a very strict rule book that's fine for a
partially subjective thing like this, it should be that hard to detect the
obvious cases, at least for user facing changes.

May look like a small thing to "rant" on, but I spend a lot of time in git
log et al. and copy pasted method/file names without simple concise tagging
and for-human info can make it much harder with no benefit for anyone.

Naturally applies to all devs/maintainers not just those in To/Cc here.

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

[Dominik Csapak]

On 10/21/22 13:29, Thomas Lamprecht wrote:
> Can we *please* get sane commit subject for *human* consumption?!
> 
> The worst one, that really triggers me:
>> authenticate_user: pass undef instead of empty $tfa_challenge to authenticate_2nd_new
> Instead of a high level human overview it gives basically another almost
> machine readable diff of the lower level changes. So there's close to zero
> of useful higher level and/or _new_ info in there, but sure makes one stop
> to parse on seeing this in some log.
> 
> Copying the method name (nor file module name!) is basically never a good
> idea, especially as tag and especially for (admin) user facing changes - it
> can naturally be OK for library stuff (i.e., dev-only facing changes), but
> even then seldom as start `<tag>:`...
> 
> following would allow for a quicker to read, and (granted, slightly
> subjective) better high level understanding of the commits, thus better
> categorizing for when skimming through the log, e.g., in search of relevant
> changes due to some new/questionable/.. behavior; besides that it makes
> d/changelog writing easier.
> 
> * tfa: only lock config for recovery keys on authentication
> * tfa: rename outdated $otp variable to $tfa_response
>    (this is internal and won't ever make it in the d/changelog so only dev
>    readability matters)
> * auth: drop passing bogus challenge variable when checking first factor
>    (also internally, dev oriented)
> 
> This should be also verified and either amened or commented on by the
> maintainer (planning) pushing a commit/series - while its obviously 1) hard
> and 2) a lot overhead to have a very strict rule book that's fine for a
> partially subjective thing like this, it should be that hard to detect the
> obvious cases, at least for user facing changes.
> 
> May look like a small thing to "rant" on, but I spend a lot of time in git
> log et al. and copy pasted method/file names without simple concise tagging
> and for-human info can make it much harder with no benefit for anyone.
> 
> Naturally applies to all devs/maintainers not just those in To/Cc here.

yes, you're completely right of course.

i don't think that it's a 'small thing to rant on' at all, i often
looked for commits where a better subject would have made it much more
obvious what is happening and would have been easier to identify

i'll use better commit subjects in the future.