[pve-devel] [PATCH manager 2/2] enable certificate pinning for proxied requests

[Dietmar Maurer]

some comments inline:

> diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
> index 1c7d033..5ddbd07 100755
> --- a/PVE/HTTPServer.pm
> +++ b/PVE/HTTPServer.pm
> @@ -105,6 +105,53 @@ sub get_login_formatter {
>      return $info->{func};
>  }
>  
> +my $cert_cache_nodes = {};
> +my $cert_cache_fingerprints = {};
> +sub update_cert_cache {
> +    my ($node) = @_;
> +
> +    if (!defined($node)) {
> +	for $node (keys %$cert_cache_nodes) {
> +	    update_cert_cache($node);

please can we avoid the recursive call here?

my $list = defined($node) ? [ $node ] : [ keys %$cert_cache_nodes ];
foreach my $node (@$list) {
   # do your work here
}


> +	}
> +	return;
> +    }
> +
> +    if (my $old_fp = $cert_cache_nodes->{$node}) {
> +	delete $cert_cache_fingerprints->{$old_fp};

I would do that later, after we successfully read the new data.

> +    }
> +
> +    my $cert_path = "/etc/pve/nodes/$node/pve-ssl.pem";
> +    my $custom_cert_path = "/etc/pve/nodes/$node/pveproxy-ssl.pem";
> +    $cert_path = $custom_cert_path if -f $custom_cert_path;
> +    my $bio = Net::SSLeay::BIO_new_file($cert_path, 'r');
> +    my $cert = Net::SSLeay::PEM_read_bio_X509($bio);
> +    Net::SSLeay::BIO_free($bio);

What if any of these methods raise exceptions?

> +
> +    my $fp = Net::SSLeay::X509_get_fingerprint($cert, 'sha256');
> +    return if !defined($fp);
> +
> +    $cert_cache_fingerprints->{$fp} = 1;
> +    $cert_cache_nodes->{$node} = $fp;
> +}
> +
> +sub check_cert_fp {

Please use a better name: check_cert_fingerprints()

> +    my ($fp) = @_;
> +
> +    my $check = sub {
> +	for my $expected (keys %$cert_cache_fingerprints) {
> +	    return 1 if $fp eq $expected;
> +	}
> +	return 0;
> +    };
> +
> +    return 1 if &$check();
> +
> +    # refresh cache and retry once
> +    update_cert_cache();
> +    return &$check();
> +}
> +
>  # server implementation
>  
>  sub log_request {
> @@ -601,6 +648,28 @@ sub proxy_request {
>  	    }
>  	}
>  
> +	my $tls = {
> +	    # TLS 1.x only, with certificate pinning
> +	    method => 'any',
> +	    sslv2 => 0,
> +	    sslv3 => 0,
> +	    verify => 1,
> +	    verify_cb => sub {
> +		my (undef, undef, undef, $depth, undef, undef, $cert) = @_;
> +		# we don't care about intermediate or root certificates
> +		return 0 if $depth != 0;
> +
> +		# get fingerprint of server certificate
> +		my $fp = Net::SSLeay::X509_get_fingerprint($cert, 'sha256');
> +		return 0 if !defined($fp); # error
> +		return check_cert_fp($fp); # check against cache of pinned FPs

I would merge this code with check_cert_fp, so that we have a
single method (maybe call it 'tls_verify_callback'?

> +	    },
> +	};
> +
> +	# load and cache cert fingerprint if first time we proxy to this node
> +	update_cert_cache($node)
> +	    if defined($node) && !defined($cert_cache_nodes->{$node});
> +
>  	my $w; $w = http_request(
>  	    $method => $target,
>  	    headers => $headers,
> @@ -609,7 +678,7 @@ sub proxy_request {
>  	    proxy => undef, # avoid use of $ENV{HTTP_PROXY}
>  	    keepalive => $keep_alive,
>  	    body => $content,
> -	    tls_ctx => $self->{tls_ctx},
> +	    tls_ctx => AnyEvent::TLS->new(%{$tls}),
>  	    sub {
>  		my ($body, $hdr) = @_;
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel