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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

mostly stylistic nits inline, but also a comment w.r.t. FWICT needles ABI
breakage.

Am 23/10/2023 um 15:18 schrieb Folke Gleumes:
> implementation acording to rfc855 section 7.3.4

s/acording/according/

> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
>  src/PVE/ACME.pm | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 

> @@ -251,6 +251,28 @@ sub jws {
>      };
>  }
>  
> +# EAB signing using the HS256 alg (HMAC/SHA256).

At least write out the acronym "external account binding" out once here,
or maybe expand the method name "external_account_binding", for clarity.

> +sub eab {
> +    my ($self, $eab_kid, $eab_hmac_key, $url) = @_;
> +

> @@ -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.
>  # 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) = @_;

This needlessly breaks older call sites though? Why not keep the plain hash
here, you do not really win anything by switching to a hash ref, or?

Let's avoid churn w.r.t. breaking older package dependencies, if not really
required..

>      my $url = $self->_method('newAccount');
>  
> +    if ($info->{'eab'}) {

nit: eab doesn't need quoting

And if you keep the method signature to accept a hash (not a hash reference)
here you could do something like:

if (my $eab = $info{eab}) {
    my $eab_hmac_key = decode_base64($eab->{hmac_key});
    # ..
}

> +	my $eab_hmac_key = decode_base64($info->{'eab'}->{hmac_key});

nit: inconsistent hash key quoting in the same statement

> +	$info->{externalAccountBinding} = $self->eab($info->{'eab'}->{kid}, $eab_hmac_key, $url);
> +    }
> +
>      if ($tos_url) {
>  	$self->{tos} = $tos_url;
> -	$info{termsOfServiceAgreed} = JSON::true;> +	$info->{termsOfServiceAgreed} = JSON::true;

can be avoided by keeping the hash

>      }
>  
> -    return $self->__new_account(201, $url, 1, %info);
> +    return $self->__new_account(201, $url, 1, %{$info});

like above, can be avoided

>  }
>  
>  # Update existing account with new %info