assert tfa/user config lock order

[pve-access-control.git] / src / PVE / AccessControl.pm

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 29d22accd9a929c4d0960002a15b454533379ea5..49cef946ab29b33c4bc0f83525b07cce0261f48d 100644 (file)
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -12,6 +12,7 @@ use Digest::SHA;
 use IO::File;
 use File::stat;
 use JSON;
+use Scalar::Util 'weaken';
 
 use PVE::OTP;
 use PVE::Ticket;
@@ -68,13 +69,37 @@ sub pve_verify_realm {
     PVE::Auth::Plugin::pve_verify_realm(@_);
 }
 
+# Locking both config files together is only ever allowed in one order:
+#  1) tfa config
+#  2) user config
+# If we permit the other way round, too, we might end up deadlocking!
+my $user_config_locked;
 sub lock_user_config {
     my ($code, $errmsg) = @_;
 
+    my $locked = 1;
+    $user_config_locked = \$locked;
+    weaken $user_config_locked; # make this scope guard signal safe...
+
     cfs_lock_file("user.cfg", undef, $code);
+    $user_config_locked = undef;
+    if (my $err = $@) {
+       $errmsg ? die "$errmsg: $err" : die $err;
+    }
+}
+
+sub lock_tfa_config {
+    my ($code, $errmsg) = @_;
+
+    die "tfa config lock cannot be acquired while holding user config lock\n"
+       if ($user_config_locked && $$user_config_locked);
+
+    my $res = cfs_lock_file("priv/tfa.cfg", undef, $code);
     if (my $err = $@) {
        $errmsg ? die "$errmsg: $err" : die $err;
     }
+
+    return $res;
 }
 
 my $cache_read_key = sub {
@@ -795,16 +820,21 @@ sub authenticate_yubico_new : prototype($$$) {
     my $keys = $tfa_cfg->get_yubico_keys($username);
     die "no keys configured\n" if !defined($keys) || !length($keys);
 
-    # Defer to after unlocking the TFA config:
-
-    # fixme: proxy support?
-    my $proxy;
-    PVE::OTP::yubico_verify_otp($otp, $keys, $realm->{url}, $realm->{id}, $realm->{key}, $proxy);
+    authenticate_yubico_do($otp, $keys, $realm);
 
     # return `undef` to clear the tfa challenge.
     return undef;
 }
 
+sub authenticate_yubico_do : prototype($$$) {
+    my ($value, $keys, $realm) = @_;
+
+    # fixme: proxy support?
+    my $proxy = undef;
+
+    PVE::OTP::yubico_verify_otp($value, $keys, $realm->{url}, $realm->{id}, $realm->{key}, $proxy);
+}
+
 sub configure_u2f_and_wa : prototype($) {
     my ($tfa_cfg) = @_;
 
@@ -1720,73 +1750,18 @@ my $USER_CONTROLLED_TFA_TYPES = {
     oath => 1,
 };
 
-# Delete an entry by setting $data=undef in which case $type is ignored.
-# Otherwise both must be valid.
-sub user_set_tfa {
-    my ($userid, $realm, $type, $data, $cached_usercfg, $cached_domaincfg) = @_;
-
-    if (defined($data) && !defined($type)) {
-       # This is an internal usage error and should not happen
-       die "cannot set tfa data without a type\n";
-    }
-
-    my $user_cfg = $cached_usercfg || cfs_read_file('user.cfg');
-    my $user = $user_cfg->{users}->{$userid};
-
-    my $domain_cfg = $cached_domaincfg || cfs_read_file('domains.cfg');
-    my $realm_cfg = $domain_cfg->{ids}->{$realm};
-    die "auth domain '$realm' does not exist\n" if !$realm_cfg;
-
-    my $realm_tfa = $realm_cfg->{tfa};
-    if (defined($realm_tfa)) {
-       $realm_tfa = PVE::Auth::Plugin::parse_tfa_config($realm_tfa);
-       # If the realm has a TFA setting, we're only allowed to use that.
-       if (defined($data)) {
-           die "user '$userid' not found\n" if !defined($user);
-           my $required_type = $realm_tfa->{type};
-           if ($required_type ne $type) {
-               die "realm '$realm' only allows TFA of type '$required_type\n";
-           }
-
-           if (defined($data->{config})) {
-               # XXX: Is it enough if the type matches? Or should the configuration also match?
-           }
+sub assert_new_tfa_config_available() {
+    # FIXME: Assert cluster-wide new-tfa-config support!
+}
 
-           # realm-configured tfa always uses a simple key list, so use the user.cfg
-           $user->{keys} = $data->{keys};
-       } else {
-           # TFA is enforce by realm, only allow deletion if the whole user gets delete
-           die "realm '$realm' does not allow removing the 2nd factor\n" if defined($user);
-       }
-    } else {
-       die "user '$userid' not found\n" if !defined($user) && defined($data);
-       # Without a realm-enforced TFA setting the user can add a u2f or totp entry by themselves.
-       # The 'yubico' type requires yubico server settings, which have to be configured on the
-       # realm, so this is not supported here:
-       die "domain '$realm' does not support TFA type '$type'\n"
-           if defined($data) && !$USER_CONTROLLED_TFA_TYPES->{$type};
-    }
+sub user_remove_tfa : prototype($) {
+    my ($userid) = @_;
 
-    # Custom TFA entries are stored in priv/tfa.cfg as they can be more complet: u2f uses a
-    # public key and a key handle, TOTP requires the usual totp settings...
+    assert_new_tfa_config_available();
 
     my $tfa_cfg = cfs_read_file('priv/tfa.cfg');
-    my $tfa = ($tfa_cfg->{users}->{$userid} //= {});
-
-    if (defined($data)) {
-       $tfa->{type} = $type;
-       $tfa->{data} = $data;
-       cfs_write_file('priv/tfa.cfg', $tfa_cfg);
-
-       $user->{keys} = "x!$type";
-    } else {
-       delete $tfa_cfg->{users}->{$userid};
-       cfs_write_file('priv/tfa.cfg', $tfa_cfg);
-
-       delete $user->{keys} if defined($user);
-    }
-
-    cfs_write_file('user.cfg', $user_cfg) if defined($user);
+    $tfa_cfg->remove_user($userid);
+    cfs_write_file('priv/tfa.cfg', $tfa_cfg);
 }
 
 sub user_get_tfa : prototype($$$) {