[pve-devel] [PATCH v2 access-control] fix #2079: add periodic auth key rotation

Fabian Gr├╝nbichler f.gruenbichler at proxmox.com
Wed Mar 6 11:30:54 CET 2019


and modify checks to accept still valid tickets generated using the
previous auth key.

the slightly complicated caching mechanism is needed for reading the key and
its modification timestamp in one go while only reading and parsing it again if
it has changed.

the +- 300 seconds fuzzing is kept for slightly out-of-sync clusters, since the
time encoded in the tickets is the result of time() on whichever node the
ticket API call got forwarded to.

Signed-off-by: Fabian Gr├╝nbichler <f.gruenbichler at proxmox.com>
---
Notes:
    VERSIONED DEPENDENCY: on pve-cluster with cfs_lock_authkey needed
    
    changes from v1:
    - only clamp current key if cluster is quorate, warn otherwise but accept tickets.
    - bump auth key lifetime to 24h
    - add some newlines to warn statements
    
    if there are no objections to this approach in general, extending it for VNC
    tickets should be straight-forward - although they are only valid for a minute
    anyway ;)

 PVE/AccessControl.pm | 215 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 195 insertions(+), 20 deletions(-)

diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
index bdadfd2..66e1b4b 100644
--- a/PVE/AccessControl.pm
+++ b/PVE/AccessControl.pm
@@ -9,6 +9,8 @@ use Net::SSLeay;
 use Net::IP;
 use MIME::Base64;
 use Digest::SHA;
+use IO::File;
+use File::stat;
 
 use PVE::OTP;
 use PVE::Ticket;
@@ -33,11 +35,19 @@ PVE::Auth::Plugin->init();
 # $authdir must be writable by root only!
 my $confdir = "/etc/pve";
 my $authdir = "$confdir/priv";
-my $authprivkeyfn = "$authdir/authkey.key";
-my $authpubkeyfn = "$confdir/authkey.pub";
+
 my $pve_www_key_fn = "$confdir/pve-www.key";
 
+my $pve_auth_key_files = {
+    priv => "$authdir/authkey.key",
+    pub =>  "$confdir/authkey.pub",
+    pubold => "$confdir/authkey.pub.old",
+};
+
+my $pve_auth_key_cache = {};
+
 my $ticket_lifetime = 3600*2; # 2 hours
+my $authkey_lifetime = 3600*24; # 24 hours
 
 Crypt::OpenSSL::RSA->import_random_seed();
 
@@ -62,16 +72,134 @@ sub lock_user_config {
     }
 }
 
-my $pve_auth_pub_key;
+my $cache_read_key = sub {
+    my ($type) = @_;
+
+    my $path = $pve_auth_key_files->{$type};
+
+    my $read_key_and_mtime = sub {
+	my $fh = IO::File->new($path, "r");
+
+	return undef if !defined($fh);
+
+	my $st = stat($fh);
+	my $pem = PVE::Tools::safe_read_from($fh, 0, 0, $path);
+
+	close $fh;
+
+	my $key;
+	if ($type eq 'pub' || $type eq 'pubold') {
+	    $key = eval { Crypt::OpenSSL::RSA->new_public_key($pem); };
+	} elsif ($type eq 'priv') {
+	    $key = eval { Crypt::OpenSSL::RSA->new_private_key($pem); };
+	} else {
+	    die "Invalid authkey type '$type'\n";
+	}
+
+	return { key => $key, mtime => $st->mtime };
+    };
+
+    if (!defined($pve_auth_key_cache->{$type})) {
+	$pve_auth_key_cache->{$type} = $read_key_and_mtime->();
+    } else {
+	my $st = stat($path);
+	if (!$st || $st->mtime != $pve_auth_key_cache->{$type}->{mtime}) {
+	    $pve_auth_key_cache->{$type} = $read_key_and_mtime->();
+	}
+    }
+
+    return $pve_auth_key_cache->{$type};
+};
+
 sub get_pubkey {
+    my ($old) = @_;
+
+    my $type = $old ? 'pubold' : 'pub';
+
+    my $res = $cache_read_key->($type);
+    return undef if !defined($res);
+
+    return wantarray ? ($res->{key}, $res->{mtime}) : $res->{key};
+}
+
+sub get_privkey {
+    my $res = $cache_read_key->('priv');
 
-    return $pve_auth_pub_key if $pve_auth_pub_key;
+    if (!defined($res) || !check_authkey(1)) {
+	rotate_authkey();
+	$res = $cache_read_key->('priv');
+    }
+
+    return wantarray ? ($res->{key}, $res->{mtime}) : $res->{key};
+}
 
-    my $input = PVE::Tools::file_get_contents($authpubkeyfn);
+sub check_authkey {
+    my ($quiet) = @_;
 
-    $pve_auth_pub_key = Crypt::OpenSSL::RSA->new_public_key($input);
+    # skip check if non-quorate, as rotation is not possible anyway
+    return 1 if !PVE::Cluster::check_cfs_quorum(1);
 
-    return $pve_auth_pub_key;
+    my ($pub_key, $mtime) = get_pubkey();
+    if (!$pub_key) {
+	warn "auth key pair missing, generating new one..\n"  if !$quiet;
+	return 0;
+    } else {
+	if (time() - $mtime >= $authkey_lifetime) {
+	    warn "auth key pair too old, rotating..\n" if !$quiet;;
+	    return 0;
+	} else {
+	    warn "auth key new enough, skipping rotation\n" if !$quiet;;
+	    return 1;
+	}
+    }
+}
+
+sub rotate_authkey {
+    my $res = PVE::Cluster::cfs_lock_authkey(undef, sub {
+	# re-check with lock to avoid double rotation in clusters
+	return if check_authkey();
+
+	my $old = get_pubkey();
+
+	if ($old) {
+	    eval {
+		my $pem = $old->get_public_key_x509_string();
+		PVE::Tools::file_set_contents($pve_auth_key_files->{pubold}, $pem);
+	    };
+	    die "Failed to store old auth key: $@\n" if $@;
+	}
+
+	my $new = Crypt::OpenSSL::RSA->generate_key(2048);
+	eval {
+	    my $pem = $new->get_public_key_x509_string();
+	    PVE::Tools::file_set_contents($pve_auth_key_files->{pub}, $pem);
+	};
+	if ($@) {
+	    if ($old) {
+		warn "Failed to store new auth key - $@\n";
+		warn "Reverting to previous auth key\n";
+		eval {
+		    my $pem = $old->get_public_key_x509_string();
+		    PVE::Tools::file_set_contents($pve_auth_key_files->{pub}, $pem);
+		};
+		die "Failed to restore old auth key: $@\n" if $@;
+	    } else {
+		die "Failed to store new auth key - $@\n";
+	    }
+	}
+
+	eval {
+	    my $pem = $new->get_private_key_string();
+	    PVE::Tools::file_set_contents($pve_auth_key_files->{priv}, $pem);
+	};
+	if ($@) {
+	    warn "Failed to store new auth key - $@\n";
+	    warn "Deleting auth key to force regeneration\n";
+	    unlink $pve_auth_key_files->{pub};
+	    unlink $pve_auth_key_files->{priv};
+	}
+    });
+    die $@ if (!defined($res) && $@);
 }
 
 my $csrf_prevention_secret;
@@ -100,17 +228,34 @@ sub verify_csrf_prevention_token {
 	$secret, $username, $token, -300, $ticket_lifetime, $noerr);
 }
 
-my $pve_auth_priv_key;
-sub get_privkey {
+my $get_ticket_age_range = sub {
+    my ($now, $mtime, $rotated) = @_;
 
-    return $pve_auth_priv_key if $pve_auth_priv_key;
+    my $key_age = $now - $mtime;
+    $key_age = 0 if $key_age < 0;
 
-    my $input = PVE::Tools::file_get_contents($authprivkeyfn);
+    my $min = -300;
+    my $max = $ticket_lifetime;
 
-    $pve_auth_priv_key = Crypt::OpenSSL::RSA->new_private_key($input);
+    if ($rotated) {
+	# ticket creation after rotation is not allowed
+	$min = $key_age - 300;
+    } else {
+	if ($key_age > $authkey_lifetime) {
+	    if (PVE::Cluster::check_cfs_quorum(1)) {
+		# key should have been rotated, clamp range accordingly
+		$min = $key_age - $authkey_lifetime;
+	    } else {
+		warn "Cluster not quorate - extending auth key lifetime!\n";
+	    }
+	}
 
-    return $pve_auth_priv_key;
-}
+	$max = $key_age + 300 if $key_age < $ticket_lifetime;
+    }
+
+    return undef if $min > $ticket_lifetime;
+    return ($min, $max);
+};
 
 sub assemble_ticket {
     my ($username) = @_;
@@ -123,12 +268,34 @@ sub assemble_ticket {
 sub verify_ticket {
     my ($ticket, $noerr) = @_;
 
-    my $rsa_pub = get_pubkey();
+    my $now = time();
 
-    my ($username, $age) = PVE::Ticket::verify_rsa_ticket(
-	$rsa_pub, 'PVE', $ticket, undef, -300, $ticket_lifetime, $noerr);
+    my $check = sub {
+	my ($old) = @_;
 
-    return undef if $noerr && !defined($username);
+	my ($rsa_pub, $rsa_mtime) = get_pubkey($old);
+	return undef if !$rsa_pub;
+
+	my ($min, $max) = $get_ticket_age_range->($now, $rsa_mtime, $old);
+	return undef if !$min;
+
+	return PVE::Ticket::verify_rsa_ticket(
+	    $rsa_pub, 'PVE', $ticket, undef, $min, $max, 1);
+    };
+
+    my ($username, $age) = $check->();
+
+    # check with old, rotated key if current key failed
+    ($username, $age) = $check->(1) if !defined($username);
+
+    if (!defined($username)) {
+	if ($noerr) {
+	    return undef;
+	} else {
+	    # raise error via undef ticket
+	    PVE::Ticket::verify_rsa_ticket(undef, 'PVE');
+	}
+    }
 
     return undef if !PVE::Auth::Plugin::verify_username($username, $noerr);
 
@@ -154,10 +321,18 @@ sub assemble_vnc_ticket {
 sub verify_vnc_ticket {
     my ($ticket, $username, $path, $noerr) = @_;
 
-    my $rsa_pub = get_pubkey();
-
     my $secret_data = "$username:$path";
 
+    my ($rsa_pub, $rsa_mtime) = get_pubkey();
+    if (!$rsa_pub || (time() - $rsa_mtime > $authkey_lifetime)) {
+	if ($noerr) {
+	    return undef;
+	} else {
+	    # raise error via undef ticket
+	    PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVEVNC');
+	}
+    }
+
     return PVE::Ticket::verify_rsa_ticket(
 	$rsa_pub, 'PVEVNC', $ticket, $secret_data, -20, 40, $noerr);
 }
-- 
2.20.1




More information about the pve-devel mailing list