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

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

[Folke Gleumes]

Changes since v2:
 * reverted the new_account abi to be non breaking

Changes since v1:
 * fixed nit's
 * expanded meta endpoint by all return values defined in the rfc
 * expanded new_account signature by field for eab credentials
 * allow for eab even if not required

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 | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)


manager: Folke Gleumes (4):
  fix #4497: acme: add support for external account bindings
  api/acme: deprecate tos endpoint in favor of meta
  fix #4497: cli/acme: detect eab and ask for credentials
  ui/acme: switch to new meta endpoint

 PVE/API2/ACMEAccount.pm   | 83 ++++++++++++++++++++++++++++++++++++++-
 PVE/CLI/pvenode.pm        | 26 +++++++++++-
 www/manager6/node/ACME.js | 12 ++++--
 3 files changed, 113 insertions(+), 8 deletions(-)

-- 
2.39.2

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

[Folke Gleumes]

implementation acording to rfc8555 section 7.3.4

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---

Changes since v2:
Transport eab credentials in the info hash, but don't reuse it as
payload. Instead, needed values are extracted and, if needed, transformed
into a new hash.
While this limits how the info hash can be used, AFAIK there are no
fields that could be set when creating a new account [0], that can't be
produced with this abi.

[0] https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.2

 src/PVE/ACME.pm | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
index 3f66182..bf5410d 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).
+my sub external_account_binding_jws {
+    my ($eab_kid, $eab_hmac_key, $jwk, $url) = @_;
+
+    my $protected = {
+	alg => 'HS256',
+	kid => $eab_kid,
+	url => $url,
+    };
+    $protected = encode(tojs($protected));
+
+    my $payload = encode(tojs($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();
@@ -331,6 +353,8 @@ sub __new_account {
 
 # Create a new account using data in %info.
 # Optionally pass $tos_url to agree to the given Terms of Service
+# %info must have at least a 'contact' field and may have a 'eab' field
+# containing a hash with 'kid' and 'hmac_key' set.
 # POST to newAccount endpoint
 # Expects a '201 Created' reply
 # Saves and returns the account data
@@ -338,12 +362,24 @@ sub new_account {
     my ($self, $tos_url, %info) = @_;
     my $url = $self->_method('newAccount');
 
+    my %payload = ( contact => $info{contact} );
+
+    if (defined($info{eab})) {
+	my $eab_hmac_key = decode_base64($info{eab}->{hmac_key});
+	$payload{externalAccountBinding} = external_account_binding_jws(
+	    $info{eab}->{kid},
+	    $eab_hmac_key,
+	    $self->jwk(),
+	    $url
+	);
+    }
+
     if ($tos_url) {
 	$self->{tos} = $tos_url;
-	$info{termsOfServiceAgreed} = JSON::true;
+	$payload{termsOfServiceAgreed} = JSON::true;
     }
 
-    return $self->__new_account(201, $url, 1, %info);
+    return $self->__new_account(201, $url, 1, %payload);
 }
 
 # Update existing account with new %info
-- 
2.39.2

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

[Folke Gleumes]

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
No changes in v3

 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..ec4eba24 100644
--- a/PVE/API2/ACMEAccount.pm
+++ b/PVE/API2/ACMEAccount.pm
@@ -115,6 +115,18 @@ __PACKAGE__->register_method ({
 		default => $acme_default_directory_url,
 		optional => 1,
 	    }),
+	    'eab-kid' => {
+		type => 'string',
+		description => 'Key Identifier for External Account Binding.',
+		requires => 'eab-hmac-key',
+		optional => 1,
+	    },
+	    'eab-hmac-key' => {
+		type => 'string',
+		description => 'HMAC key for External Account Binding.',
+		requires => 'eab-kid',
+		optional => 1,
+	    },
 	},
     },
     returns => {
@@ -130,6 +142,9 @@ __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;
 
@@ -145,7 +160,17 @@ __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)) {
+		    $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 v3 3/5] 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>
---
No changes in v3

 PVE/API2/ACMEAccount.pm | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
index ec4eba24..bc45d5ab 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,58 @@ __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 => 1,
+	properties => {
+	    termsOfService => {
+		type => 'string',
+		optional => 1,
+		description => 'ACME TermsOfService URL.',
+	    },
+	    externalAccountRequired => {
+		type => 'boolean',
+		optional => 1,
+		description => 'EAB Required'
+	    },
+	    website => {
+		type => 'string',
+		optional => 1,
+		description => 'URL to more information about the ACME server.'
+	    },
+	    caaIdentities => {
+		type => 'string',
+		optional => 1,
+		description => 'Hostnames referring to the ACME servers.'
+	    },
+	},
+    },
+    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 v3 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>
---
No changes in v3

 PVE/CLI/pvenode.pm | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
index acef6c3b..a6fbb34e 100644
--- a/PVE/CLI/pvenode.pm
+++ b/PVE/CLI/pvenode.pm
@@ -83,6 +83,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
+	my $custom_directory = 0;
 	if (!$param->{directory}) {
 	    my $directories = PVE::API2::ACMEAccount->get_directories({});
 	    print "Directory endpoints:\n";
@@ -100,6 +101,7 @@ __PACKAGE__->register_method({
 		    $selection = $1;
 		    if ($selection == $i) {
 			$param->{directory} = $term->readline("Enter custom URL: ");
+			$custom_directory = 1;
 			return;
 		    } elsif ($selection < $i && $selection >= 0) {
 			$param->{directory} = $directories->[$selection]->{url};
@@ -117,8 +119,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 +132,25 @@ __PACKAGE__->register_method({
 	} else {
 	    print "No Terms of Service found, proceeding.\n";
 	}
+
+	my $eab_enabled = $meta->{externalAccountRequired};
+	if (!$eab_enabled && $custom_directory) {
+	    my $term = Term::ReadLine->new('pvenode');
+	    my $agreed = $term->readline('Do you want to use external account binding? [y|N]: ');
+	    $eab_enabled = ($agreed =~ /^y$/i);
+	} elsif ($eab_enabled) {
+	    print "The CA requires external account binding.\n";
+	}
+	if ($eab_enabled) {
+	    print "You should have received a key id and a key from your CA.\n";
+	    my $term = Term::ReadLine->new('pvenode');
+	    my $eab_kid = $term->readline('Enter EAB key id: ');
+	    my $eab_hmac_key = $term->readline('Enter EAB key: ');
+
+	    $param->{'eab-kid'} = $eab_kid;
+	    $param->{'eab-hmac-key'} = $eab_hmac_key;
+	}
+
 	print "\nAttempting to register account with '$param->{directory}'..\n";
 
 	$upid_exit->(PVE::API2::ACMEAccount->register_account($param));
-- 
2.39.2

[pve-devel] [PATCH manager v3 5/5] 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>
---
Changes since v2:
fixed tabs/spaces

 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..21137b1a 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 v3 0/5] fix #4497: add support for external account bindings

[Fabian Grünbichler]

On October 31, 2023 10:05 am, Folke Gleumes wrote:
> Changes since v2:
>  * reverted the new_account abi to be non breaking
> 
> Changes since v1:
>  * fixed nit's
>  * expanded meta endpoint by all return values defined in the rfc
>  * expanded new_account signature by field for eab credentials
>  * allow for eab even if not required
> 
> 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

Reviewed-by: Fabian.Grünbichler <f.gruenbichler@proxmox.com>
Tested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

Held off applying for now, since this requires a bump of proxmox-acme
(and a versioned depends, else the EAB part will be silently ignored by
the old version ;)), and we might want to update the plugins as well
while doing that..

> acme: Folke Gleumes (1):
>   fix #4497: add support for external account bindings
> 
>  src/PVE/ACME.pm | 48 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> 
> manager: Folke Gleumes (4):
>   fix #4497: acme: add support for external account bindings
>   api/acme: deprecate tos endpoint in favor of meta
>   fix #4497: cli/acme: detect eab and ask for credentials
>   ui/acme: switch to new meta endpoint
> 
>  PVE/API2/ACMEAccount.pm   | 83 ++++++++++++++++++++++++++++++++++++++-
>  PVE/CLI/pvenode.pm        | 26 +++++++++++-
>  www/manager6/node/ACME.js | 12 ++++--
>  3 files changed, 113 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 v3 3/5] api/acme: deprecate tos endpoint in favor of meta

[Thomas Lamprecht]

Am 31/10/2023 um 10:05 schrieb 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>
> ---
> No changes in v3
> 
>  PVE/API2/ACMEAccount.pm | 56 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
> index ec4eba24..bc45d5ab 100644
> --- a/PVE/API2/ACMEAccount.pm
> +++ b/PVE/API2/ACMEAccount.pm

> +    returns => {
> +	type => 'object',
> +	additionalProperties => 1,
> +	properties => {
> +	    termsOfService => {
> +		type => 'string',
> +		optional => 1,
> +		description => 'ACME TermsOfService URL.',

nit: we normally place the description at the top, and sometimes as
second item after "type", but at the bottom is rather unusual.

> +	    },
> +	    externalAccountRequired => {
> +		type => 'boolean',
> +		optional => 1,
> +		description => 'EAB Required'
> +	    },
> +	    website => {
> +		type => 'string',
> +		optional => 1,
> +		description => 'URL to more information about the ACME server.'
> +	    },
> +	    caaIdentities => {
> +		type => 'string',

but the RFC say's that this is an array, and we do not actually mangle this into
a plain string anywhere FWICT, checking the API response would agree on that too.

So, can you re-check and mark this as array in a follow-up?

> +		optional => 1,
> +		description => 'Hostnames referring to the ACME servers.'



> +	    },

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

[Thomas Lamprecht]

Am 31/10/2023 um 10:05 schrieb Folke Gleumes:
> Changes since v2:
>  * reverted the new_account abi to be non breaking
> 
> Changes since v1:
>  * fixed nit's
>  * expanded meta endpoint by all return values defined in the rfc
>  * expanded new_account signature by field for eab credentials
>  * allow for eab even if not required
> 
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> 
> manager: Folke Gleumes (4):
>   fix #4497: acme: add support for external account bindings
>   api/acme: deprecate tos endpoint in favor of meta
>   fix #4497: cli/acme: detect eab and ask for credentials
>   ui/acme: switch to new meta endpoint
> 
>  PVE/API2/ACMEAccount.pm   | 83 ++++++++++++++++++++++++++++++++++++++-
>  PVE/CLI/pvenode.pm        | 26 +++++++++++-
>  www/manager6/node/ACME.js | 12 ++++--
>  3 files changed, 113 insertions(+), 8 deletions(-)
> 


applied series with Fabian's R-b and T-b trailers, thanks!

But I commented w.r.t. return schema on patch 3/5, please check that out.

And some commits I'd have slightly favored a split the rework and addition
of new feature into two separate commits, like the proxmox-acme one, but no
biggie.