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

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

[Folke Gleumes]

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

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

 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] [PATCH acme 1/5] fix #4497: add support for external account bindings

[Folke Gleumes]

implementation acording to rfc855 section 7.3.4

Signed-off-by: Folke Gleumes <f.gleumes@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.
 # 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);
+    }
+
     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] [PATCH manager 2/5] fix #4497: acme: add support for external account bindings

[Folke Gleumes]

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
 PVE/API2/ACMEAccount.pm | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
index b790843a..daae18d8 100644
--- a/PVE/API2/ACMEAccount.pm
+++ b/PVE/API2/ACMEAccount.pm
@@ -115,6 +115,16 @@ __PACKAGE__->register_method ({
 		default => $acme_default_directory_url,
 		optional => 1,
 	    }),
+	    eab_kid => {
+		type => 'string',
+		description => 'Key Identifier for External Account Binding.',
+		optional => 1,
+	    },
+	    eab_hmac_key => {
+		type => 'string',
+		description => 'HMAC key for External Account Binding.',
+		optional => 1,
+	    },
 	},
     },
     returns => {
@@ -130,8 +140,15 @@ __PACKAGE__->register_method ({
 	my $account_file = "${acme_account_dir}/${account_name}";
 	mkdir $acme_account_dir if ! -e $acme_account_dir;
 
+	my $eab_kid = extract_param($param, 'eab_kid');
+	my $eab_hmac_key = extract_param($param, 'eab_hmac_key');
+
 	raise_param_exc({'name' => "ACME account config file '${account_name}' already exists."})
 	    if -e $account_file;
+	raise_param_exc({'eab_kid' => "'eab_hmac_key' must be defined if 'eab_kid' is set."})
+	    if defined($eab_kid) and not defined($eab_hmac_key);
+	raise_param_exc({'eab_hmac_key' => "'eab_kid' must be defined if 'eab_hmac_key' is set."})
+	    if defined($eab_hmac_key) and not defined($eab_kid);
 
 	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
 	my $contact = $account_contact_from_param->($param);
@@ -145,7 +162,15 @@ __PACKAGE__->register_method ({
 		print "Generating ACME account key..\n";
 		$acme->init(4096);
 		print "Registering ACME account..\n";
-		eval { $acme->new_account($param->{tos_url}, contact => $contact); };
+		my $info = {contact => $contact};
+		if (defined($eab_kid) and defined($eab_hmac_key)) {
+		    $info->{eab} = {
+			kid => $eab_kid,
+			hmac_key => $eab_hmac_key
+		    };
+		}
+
+		eval { $acme->new_account($param->{tos_url}, $info); };
 		if (my $err = $@) {
 		    unlink $account_file;
 		    die "Registration failed: $err\n";
-- 
2.39.2

[pve-devel] [PATCH manager 3/5] fix #4497: api/acme: deprecate tos endpoint in favor of meta

[Folke Gleumes]

The ToS endpoint ignored data that is needed to detect if EAB needs to
be used. Instead of adding a new endpoint that does the same request,
the tos endpoint is deprecated and replaced by the meta endpoint,
that returns all information returned by the directory.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
 PVE/API2/ACMEAccount.pm | 46 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
index daae18d8..bfe76734 100644
--- a/PVE/API2/ACMEAccount.pm
+++ b/PVE/API2/ACMEAccount.pm
@@ -62,6 +62,7 @@ __PACKAGE__->register_method ({
 	return [
 	    { name => 'account' },
 	    { name => 'tos' },
+	    { name => 'meta' },
 	    { name => 'directories' },
 	    { name => 'plugins' },
 	    { name => 'challenge-schema' },
@@ -333,11 +334,12 @@ __PACKAGE__->register_method ({
 	return $update_account->($param, 'deactivate', status => 'deactivated');
     }});
 
+# TODO: deprecated, remove with pve 9
 __PACKAGE__->register_method ({
     name => 'get_tos',
     path => 'tos',
     method => 'GET',
-    description => "Retrieve ACME TermsOfService URL from CA.",
+    description => "Retrieve ACME TermsOfService URL from CA. Deprecated, please use /cluster/acme/meta.",
     permissions => { user => 'all' },
     parameters => {
 	additionalProperties => 0,
@@ -364,6 +366,48 @@ __PACKAGE__->register_method ({
 	return $meta ? $meta->{termsOfService} : undef;
     }});
 
+__PACKAGE__->register_method ({
+    name => 'get_meta',
+    path => 'meta',
+    method => 'GET',
+    description => "Retrieve ACME Directory Meta Information",
+    permissions => { user => 'all' },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    directory => get_standard_option('pve-acme-directory-url', {
+		default => $acme_default_directory_url,
+		optional => 1,
+	    }),
+	},
+    },
+    returns => {
+	type => 'object',
+	additionalProperties => 0,
+	properties => {
+	    termsOfService => {
+		type => 'string',
+		optional => 1,
+		description => 'ACME TermsOfService URL.',
+	    },
+	    externalAccountRequired => {
+		type => 'boolean',
+		optional => 1,
+		description => 'EAB Required'
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
+
+	my $acme = PVE::ACME->new(undef, $directory);
+	my $meta = $acme->get_meta();
+
+	return $meta;
+    }});
+
 __PACKAGE__->register_method ({
     name => 'get_directories',
     path => 'directories',
-- 
2.39.2

[pve-devel] [PATCH manager 4/5] fix #4497: cli/acme: detect eab and ask for credentials

[Folke Gleumes]

Since external account binding is advertised the same way as the ToS,
it can be detected when creating an account and asked for if needed.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
 PVE/CLI/pvenode.pm | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
index acef6c3b..e3d6b15a 100644
--- a/PVE/CLI/pvenode.pm
+++ b/PVE/CLI/pvenode.pm
@@ -117,8 +117,9 @@ __PACKAGE__->register_method({
 	    }
 	}
 	print "\nAttempting to fetch Terms of Service from '$param->{directory}'..\n";
-	my $tos = PVE::API2::ACMEAccount->get_tos({ directory => $param->{directory} });
-	if ($tos) {
+	my $meta = PVE::API2::ACMEAccount->get_meta({ directory => $param->{directory} });
+	if ($meta->{termsOfService}) {
+	    my $tos = $meta->{termsOfService};
 	    print "Terms of Service: $tos\n";
 	    my $term = Term::ReadLine->new('pvenode');
 	    my $agreed = $term->readline('Do you agree to the above terms? [y|N]: ');
@@ -129,6 +130,17 @@ __PACKAGE__->register_method({
 	} else {
 	    print "No Terms of Service found, proceeding.\n";
 	}
+	if ($meta->{externalAccountRequired}) {
+	    print "The ACME Directory uses External Account Binding\n";
+	    my $term = Term::ReadLine->new('pvenode');
+	    my $eab_kid = $term->readline('Enter EAB kid: ');
+	    my $eab_hmac_key = $term->readline('Enter EAB HMAC key: ');
+
+	    $param->{eab_kid} = $eab_kid;
+	    $param->{eab_hmac_key} = $eab_hmac_key;
+	} else {
+	    print "No EAB required, proceeding.\n";
+	}
 	print "\nAttempting to register account with '$param->{directory}'..\n";
 
 	$upid_exit->(PVE::API2::ACMEAccount->register_account($param));
-- 
2.39.2

[pve-devel] [PATCH manager 5/5] fix #4497: ui/acme: switch to new meta endpoint

[Folke Gleumes]

Besides the switch from tos to meta endpoint, this fixes a visual bug,
where the 'Accept TOS' button would show up, even if no ToS was needed.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
 www/manager6/node/ACME.js | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/www/manager6/node/ACME.js b/www/manager6/node/ACME.js
index 9f1dabce..d64ac36f 100644
--- a/www/manager6/node/ACME.js
+++ b/www/manager6/node/ACME.js
@@ -79,15 +79,19 @@ Ext.define('PVE.node.ACMEAccountCreate', {
 		    checkbox.setHidden(true);
 
 		    Proxmox.Utils.API2Request({
-			url: '/cluster/acme/tos',
+			url: '/cluster/acme/meta',
 			method: 'GET',
 			params: {
 			    directory: value,
 			},
 			success: function(response, opt) {
-			    field.setValue(response.result.data);
-			    disp.setValue(response.result.data);
-			    checkbox.setHidden(false);
+			    if (response.result.data.termsOfService) {
+			        field.setValue(response.result.data.termsOfService);
+			        disp.setValue(response.result.data.termsOfService);
+			        checkbox.setHidden(false);
+			    } else {
+				disp.setValue(undefined);
+			    }
 			},
 			failure: function(response, opt) {
 			    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
-- 
2.39.2

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

[Fabian Grünbichler]

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

Re: [pve-devel] [PATCH manager 4/5] fix #4497: cli/acme: detect eab and ask for credentials

[Fabian Grünbichler]

On October 23, 2023 3:18 pm, Folke Gleumes wrote:
> Since external account binding is advertised the same way as the ToS,
> it can be detected when creating an account and asked for if needed.
> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
>  PVE/CLI/pvenode.pm | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
> index acef6c3b..e3d6b15a 100644
> --- a/PVE/CLI/pvenode.pm
> +++ b/PVE/CLI/pvenode.pm
> @@ -117,8 +117,9 @@ __PACKAGE__->register_method({
>  	    }
>  	}
>  	print "\nAttempting to fetch Terms of Service from '$param->{directory}'..\n";
> -	my $tos = PVE::API2::ACMEAccount->get_tos({ directory => $param->{directory} });
> -	if ($tos) {
> +	my $meta = PVE::API2::ACMEAccount->get_meta({ directory => $param->{directory} });
> +	if ($meta->{termsOfService}) {
> +	    my $tos = $meta->{termsOfService};
>  	    print "Terms of Service: $tos\n";
>  	    my $term = Term::ReadLine->new('pvenode');
>  	    my $agreed = $term->readline('Do you agree to the above terms? [y|N]: ');
> @@ -129,6 +130,17 @@ __PACKAGE__->register_method({
>  	} else {
>  	    print "No Terms of Service found, proceeding.\n";
>  	}
> +	if ($meta->{externalAccountRequired}) {
> +	    print "The ACME Directory uses External Account Binding\n";

s/uses/requires

and maybe s/Directory/CA/

since "directory" is just the name for the entrypoint of the API :)

> +	    my $term = Term::ReadLine->new('pvenode');

since this is the "interactive" user friendly mode, we might want to add
another line here to indicate that the requested values should have been
given to the user by the CA?

> +	    my $eab_kid = $term->readline('Enter EAB kid: ');

might be worth to s/kid/key identifer ("kid")/ to make it more
understandable for users who haven't already learned the ACME spec by
heart ;)

> +	    my $eab_hmac_key = $term->readline('Enter EAB HMAC key: ');
> +
> +	    $param->{eab_kid} = $eab_kid;
> +	    $param->{eab_hmac_key} = $eab_hmac_key;

maybe:

} elsif ($directory_is_custom) {
# ask for optional EAB parameters
}

> +	} else {
> +	    print "No EAB required, proceeding.\n";
> +	}
>  	print "\nAttempting to register account with '$param->{directory}'..\n";
>  
>  	$upid_exit->(PVE::API2::ACMEAccount->register_account($param));
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH manager 3/5] fix #4497: api/acme: deprecate tos endpoint in favor of meta

[Fabian Grünbichler]

On October 23, 2023 3:18 pm, Folke Gleumes wrote:
> The ToS endpoint ignored data that is needed to detect if EAB needs to
> be used. Instead of adding a new endpoint that does the same request,
> the tos endpoint is deprecated and replaced by the meta endpoint,
> that returns all information returned by the directory.

not opposed to this, but we could also get away for the time being
without this patch and just make users that want to use EAB to
explicitly opt-in. any CA that requires EABs has to error out if an
attempt to register is made without a binding. the spec also allows CAs
to optionally allow bindings without requiring them (e.g., to support
both short-lived auto-validated, and longer-lived manually pre-validated
certs), or to ignore the EAB part of account registrations with
(unwanted or invalid) EABs by just discarding the binding/not persisting
it in the created account.

so basically every combination of CA indicating EAB requirements and EAB
being provided upon registration *might* work, except for EAB being
required and not provided.

> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
>  PVE/API2/ACMEAccount.pm | 46 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
> index daae18d8..bfe76734 100644
> --- a/PVE/API2/ACMEAccount.pm
> +++ b/PVE/API2/ACMEAccount.pm
> @@ -62,6 +62,7 @@ __PACKAGE__->register_method ({
>  	return [
>  	    { name => 'account' },
>  	    { name => 'tos' },
> +	    { name => 'meta' },
>  	    { name => 'directories' },
>  	    { name => 'plugins' },
>  	    { name => 'challenge-schema' },
> @@ -333,11 +334,12 @@ __PACKAGE__->register_method ({
>  	return $update_account->($param, 'deactivate', status => 'deactivated');
>      }});
>  
> +# TODO: deprecated, remove with pve 9
>  __PACKAGE__->register_method ({
>      name => 'get_tos',
>      path => 'tos',
>      method => 'GET',
> -    description => "Retrieve ACME TermsOfService URL from CA.",
> +    description => "Retrieve ACME TermsOfService URL from CA. Deprecated, please use /cluster/acme/meta.",
>      permissions => { user => 'all' },
>      parameters => {
>  	additionalProperties => 0,
> @@ -364,6 +366,48 @@ __PACKAGE__->register_method ({
>  	return $meta ? $meta->{termsOfService} : undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'get_meta',
> +    path => 'meta',
> +    method => 'GET',
> +    description => "Retrieve ACME Directory Meta Information",
> +    permissions => { user => 'all' },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    directory => get_standard_option('pve-acme-directory-url', {
> +		default => $acme_default_directory_url,
> +		optional => 1,
> +	    }),
> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +	additionalProperties => 0,
> +	properties => {
> +	    termsOfService => {
> +		type => 'string',
> +		optional => 1,
> +		description => 'ACME TermsOfService URL.',
> +	    },
> +	    externalAccountRequired => {
> +		type => 'boolean',
> +		optional => 1,
> +		description => 'EAB Required'
> +	    },

the RFC has a few more, so we should maybe add the known ones here as
well, and for sure set additionalProperties to 1.

> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
> +
> +	my $acme = PVE::ACME->new(undef, $directory);
> +	my $meta = $acme->get_meta();
> +
> +	return $meta;
> +    }});
> +
>  __PACKAGE__->register_method ({
>      name => 'get_directories',
>      path => 'directories',
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Fabian Grünbichler]

On October 23, 2023 3:18 pm, Folke Gleumes wrote:
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
>  PVE/API2/ACMEAccount.pm | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
> index b790843a..daae18d8 100644
> --- a/PVE/API2/ACMEAccount.pm
> +++ b/PVE/API2/ACMEAccount.pm
> @@ -115,6 +115,16 @@ __PACKAGE__->register_method ({
>  		default => $acme_default_directory_url,
>  		optional => 1,
>  	    }),
> +	    eab_kid => {
> +		type => 'string',
> +		description => 'Key Identifier for External Account Binding.',
> +		optional => 1,
> +	    },
> +	    eab_hmac_key => {
> +		type => 'string',
> +		description => 'HMAC key for External Account Binding.',
> +		optional => 1,
> +	    },

Nit: s/_/-/ for new parameters :)
>  	},
>      },
>      returns => {
> @@ -130,8 +140,15 @@ __PACKAGE__->register_method ({
>  	my $account_file = "${acme_account_dir}/${account_name}";
>  	mkdir $acme_account_dir if ! -e $acme_account_dir;
>  
> +	my $eab_kid = extract_param($param, 'eab_kid');
> +	my $eab_hmac_key = extract_param($param, 'eab_hmac_key');
> +
>  	raise_param_exc({'name' => "ACME account config file '${account_name}' already exists."})
>  	    if -e $account_file;
> +	raise_param_exc({'eab_kid' => "'eab_hmac_key' must be defined if 'eab_kid' is set."})
> +	    if defined($eab_kid) and not defined($eab_hmac_key);
> +	raise_param_exc({'eab_hmac_key' => "'eab_kid' must be defined if 'eab_hmac_key' is set."})
> +	    if defined($eab_hmac_key) and not defined($eab_kid);

these two checks can be encoded directly in the schema by adding

requires => "name-of-require-parameter"

to both definitions, pointing at the other one. if a caller only
provides either of them and not both (or none), the schema check will
error:

eab_hmac_key: missing property - 'eab_kid' requires this property

without needing any manual handling in the API endpoint handler sub.

>  
>  	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
>  	my $contact = $account_contact_from_param->($param);
> @@ -145,7 +162,15 @@ __PACKAGE__->register_method ({
>  		print "Generating ACME account key..\n";
>  		$acme->init(4096);
>  		print "Registering ACME account..\n";
> -		eval { $acme->new_account($param->{tos_url}, contact => $contact); };
> +		my $info = {contact => $contact};
> +		if (defined($eab_kid) and defined($eab_hmac_key)) {
> +		    $info->{eab} = {
> +			kid => $eab_kid,
> +			hmac_key => $eab_hmac_key
> +		    };
> +		}
> +
> +		eval { $acme->new_account($param->{tos_url}, $info); };

if you switch this line to %$info or $info->%*, the new_account sub can
still take the hash directly instead of a reference, but see comments on
the proxmox-acme patch for possibly nicer signatures.

>  		if (my $err = $@) {
>  		    unlink $account_file;
>  		    die "Registration failed: $err\n";
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [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@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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Thomas Lamprecht]

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

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

[Thomas Lamprecht]

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.