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

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

Am 24/10/2023 um 10:32 schrieb Fabian Grünbichler:
>>  # 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.

passing %info direct around seems like a bad ABI anyway, so why not
stop doing that and construct a new hash here that only takes the
properties from info out that we actually care about?

> 
> 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.

IMO this is not really a better general API (the unconstrained passing
around of %info, while requiring further ABI breakage on any future
parameter addition, is still there).

> 
> 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.

Aligning more towards PBS/PMG has it's merits, FWIW, we could also do
so with a new method, and then transform the existing one to just transform
the parameters as needed and call into that.

> 
> 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.
> 

Yeah, that or adding a new method would be preferred from my side.