[pve-devel] [RFC access-control 1/2] fix #2079: add periodic auth key rotation

[Thomas Lamprecht]

On 3/6/19 8:21 AM, Fabian Grünbichler wrote:
> 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
>     
>     it would also be possible to extend pve-common's Ticket.pm to support a list of
>     keys + age ranges, but that makes the interface rather non-straight-forward..
>     
>     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 ;)
>     
>     a similar approach could also work for CSRF tokens if desired..
>     
>     the auth key lifetime can of course also be set to a bigger value (e.g., 24h)

just for the record from off-line discussion: looks OK in general, but I'd like that
a user still can login if the node/cluster wasn't quorate for a time span where a
rotation would've needed to happen (plus grace period). One shouldn't be able to
modify things (i.e., do harm) in a non-quorate situation anyway and for checking the
state of a node it's convenient, else a user could be confused/annoyed if login
just fails.


> 
>  PVE/AccessControl.pm | 211 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 191 insertions(+), 20 deletions(-)
> 
> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
> index bdadfd2..ff6413c 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*3; # 3 hours
>  
>  Crypt::OpenSSL::RSA->import_random_seed();
>  
> @@ -62,16 +72,131 @@ 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');
> +
> +    if (!defined($res) || !check_authkey(1)) {
> +	rotate_authkey();
> +	$res = $cache_read_key->('priv');
> +    }
> +
> +    return wantarray ? ($res->{key}, $res->{mtime}) : $res->{key};
> +}
> +
> +sub check_authkey {
> +    my ($quiet) = @_;
> +
> +    my ($pub_key, $mtime) = get_pubkey();
> +    if (!$pub_key) {
> +	warn('auth key pair missing, generating new one..') if !$quiet;
> +	return 0;
> +    } else {
> +	if (time() - $mtime >= $authkey_lifetime) {
> +	    warn('auth key pair too old, rotating..') if !$quiet;;
> +	    return 0;
> +	} else {
> +	    warn('auth key new enough, skipping rotation') if !$quiet;;
> +	    return 1;
> +	}
> +    }
> +}
>  
> -    return $pve_auth_pub_key if $pve_auth_pub_key;
> +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 $input = PVE::Tools::file_get_contents($authpubkeyfn);
> +	my $old = get_pubkey();
>  
> -    $pve_auth_pub_key = Crypt::OpenSSL::RSA->new_public_key($input);
> +	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";
> +	    }
> +	}
>  
> -    return $pve_auth_pub_key;
> +	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 +225,33 @@ 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, $max);
>  
> -    $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) {
> +	    # key should have been rotated, clamp range accordingly
> +	    $min = $key_age - $authkey_lifetime;
> +	} else {
> +	    $min = -300;
> +	}
> +    }
>  
> -    return $pve_auth_priv_key;
> -}
> +    return undef if $min > $ticket_lifetime;
> +
> +    $max = $ticket_lifetime;
> +    $max = $key_age + 300 if (!$rotated && $key_age < $ticket_lifetime);
> +
> +    return ($min, $max);
> +};
>  
>  sub assemble_ticket {
>      my ($username) = @_;
> @@ -123,12 +264,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 +317,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);
>  }
>