Re: [pve-devel] [PATCH acme/manager 0/5] fix #4497: add external account binding support

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On October 23, 2023 3:18 pm, Folke Gleumes wrote:
> This patch series adds functionality to use acme directiories
> that require the use of external account binding, as specified
> in rfc 8555 section 7.3.4.
> 
> To avoid code duplication and redundant calls to the CA,
> the `/cluster/acme/tos` endpoint has been deprecated and
> it's function will be covered by the new `/cluster/acme/meta`
> endpoint, which exposes all meta information provided by the CA,
> including the flag indicating that EAB needs to be used.
> The underlying call to the CA remains the same.
> 
> The CLI interface will only ask for the EAB credentials if needed,
> similar to how it works for the ToS.
> 
> The patches have been tested to work with and without EAB
> by using pebble [0] as the CA.
> 
> [0] https://github.com/letsencrypt/pebble

this already looks quite good, some comments on the individual patches,
mainly about the interface change in proxmox-acme and the meta endpoint.

there might be some additional follow-up work needed once a subsequent
version has been applied - in my experience pebble and production CAs
often don't handle all corner cases identical (even pebble and boulder,
where the divergence is often intentional to force client devs to
implement those corner cases right ;)), or commercial CAs requiring some
special attention.

> 
> acme: Folke Gleumes (1):
>   fix #4497: add support for external account bindings
> 
>  src/PVE/ACME.pm | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> manager: Folke Gleumes (4):
>   fix #4497: acme: add support for external account bindings
>   fix #4497: api/acme: deprecate tos endpoint in favor of meta
>   fix #4497: cli/acme: detect eab and ask for credentials
>   fix #4497: ui/acme: switch to new meta endpoint

nit: we usually only add the fixes tag to the patch/commit the makes the
fix user-visible. not always clear cut, and having more than one can be
okay. in this case, the tos/meta patches are only indirectly related to
the fix, and not needed to use EAB, so they likely can drop the prefix.

> 
>  PVE/API2/ACMEAccount.pm   | 73 +++++++++++++++++++++++++++++++++++++--
>  PVE/CLI/pvenode.pm        | 16 +++++++--
>  www/manager6/node/ACME.js | 12 ++++---
>  3 files changed, 93 insertions(+), 8 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>