[pve-devel] [PATCH acme 1/5] fix #4497: add support for external account bindings

[Fabian Grünbichler]

On October 23, 2023 3:18 pm, Folke Gleumes wrote:
> implementation acording to rfc855 section 7.3.4
> 
> Signed-off-by: Folke Gleumes <f.gleumes at proxmox.com>
> ---
>  src/PVE/ACME.pm | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
> index 3f66182..f65729a 100644
> --- a/src/PVE/ACME.pm
> +++ b/src/PVE/ACME.pm
> @@ -7,10 +7,10 @@ use POSIX;
>  
>  use Data::Dumper;
>  use Date::Parse;
> -use MIME::Base64 qw(encode_base64url);
> +use MIME::Base64 qw(encode_base64url decode_base64);
>  use File::Path qw(make_path);
>  use JSON;
> -use Digest::SHA qw(sha256 sha256_hex);
> +use Digest::SHA qw(sha256 sha256_hex hmac_sha256);
>  
>  use HTTP::Request;
>  use LWP::UserAgent;
> @@ -251,6 +251,28 @@ sub jws {
>      };
>  }
>  
> +# EAB signing using the HS256 alg (HMAC/SHA256).
> +sub eab {
> +    my ($self, $eab_kid, $eab_hmac_key, $url) = @_;
> +
> +    my $protected = {
> +	alg => 'HS256',
> +	kid => $eab_kid,
> +	url => $url,
> +    };
> +    $protected = encode(tojs($protected));
> +
> +    my $payload = encode(tojs($self->jwk()));
> +    my $signdata = "$protected.$payload";
> +    my $signature = encode(hmac_sha256($signdata, $eab_hmac_key));
> +
> +    return {
> +	protected => $protected,
> +	payload => $payload,
> +	signature => $signature,
> +    };
> +}
> +
>  sub __get_result {
>      my ($resp, $code, $plain) = @_;
>  
> @@ -300,8 +322,8 @@ sub list_methods {
>  }
>  
>  # return (optional) meta directory entry.
> -# this is public because it might contain the ToS, which should be displayed
> -# and agreed to before creating an account
> +# this is public because it might contain the ToS and EAB requirements,
> +# which have to be considered before creating an account
>  sub get_meta {
>      my ($self) = @_;
>      my $methods = $self->__get_methods();
> @@ -329,21 +351,26 @@ sub __new_account {
>      return $self->{account};
>  }
>  
> -# Create a new account using data in %info.
> +# Create a new account using data in $info.

this change (and the related ones below) are actually not required (and
would be inconsistent with the signature of the other account methods
here) - see comment in the patch for pve-manager.

>  # Optionally pass $tos_url to agree to the given Terms of Service
>  # POST to newAccount endpoint
>  # Expects a '201 Created' reply
>  # Saves and returns the account data
>  sub new_account {
> -    my ($self, $tos_url, %info) = @_;
> +    my ($self, $tos_url, $info) = @_;
>      my $url = $self->_method('newAccount');
>  
> +    if ($info->{'eab'}) {
> +	my $eab_hmac_key = decode_base64($info->{'eab'}->{hmac_key});
> +	$info->{externalAccountBinding} = $self->eab($info->{'eab'}->{kid}, $eab_hmac_key, $url);

this means that `info` now contains both the binding, but also the input
including the KID (okay, this is contained in the binding as well, so
just duplicate info) and the HMAC key, which is supposed to be secret.
granted, it is a secret given to the user by the CA over some channel,
and we only send it back to the CA, but some ACME implementations might
still reject the request because of the unexpected contents. and if the
user ever mixes up the CAs they are talking to, they might accidentally
leak the secret to the wrong entitiy.

since `info` is directly translated to the new_account request contents,
it might be better to pass in the EAB parameters on their own, and make
this sub take

my ($self, $tos_url, $eab, %info) = @_;

if $eab is undef -> no EAB. if it is set, generate the binding and put
it into %info for further passing to the ACME provider.

alternatively, it would also work to combine $tos_url and $eab into a
new $account_params or $params hash, if we think that more parameters
might be added in the future, or if we plan on merging new_account and
init, like we do in PMG/PBS.

or, as third option, we could switch the public sub new_account to take

my ($self, $tos_url, $contact, $eab) = @_;

or

my ($self, $tos_url, $contact, $eab, $rsa_bits) = @_;

which would be more aligned with how PMG and PBS look like.

almost all of the variants are a breaking change (except if we keep the
signature as is, and properly extract the eab member if it is set) that
require a double check for any reverse dependencies. there's at least
one internal tool that uses this as well that would need to be updated,
for example.

> +    }
> +
>      if ($tos_url) {
>  	$self->{tos} = $tos_url;
> -	$info{termsOfServiceAgreed} = JSON::true;
> +	$info->{termsOfServiceAgreed} = JSON::true;
>      }
>  
> -    return $self->__new_account(201, $url, 1, %info);
> +    return $self->__new_account(201, $url, 1, %{$info});
>  }
>  
>  # Update existing account with new %info
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>