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

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

[Folke Gleumes]

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 | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 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   | 79 ++++++++++++++++++++++++++++++++++++++-
 PVE/CLI/pvenode.pm        | 26 ++++++++++++-
 www/manager6/node/ACME.js | 12 ++++--
 3 files changed, 109 insertions(+), 8 deletions(-)


-- 
2.39.2

[pve-devel] [PATCH acme v2 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>
---

Changes v1 -> v2:
Switched from including the eab credentials in the info hash,
to passing them in their own variable. This still unfortunately still 
breaks the api, but doesn't potentially expose secrets and is
cleaner then purging the values from the hash afterwards.

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

diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
index 3f66182..7b3840b 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 external_account_binding_jws {
+    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();
@@ -331,13 +353,23 @@ sub __new_account {
 
 # Create a new account using data in %info.
 # Optionally pass $tos_url to agree to the given Terms of Service
+# Optionally pass $eab to use External Account Binding
 # 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, $eab, %info) = @_;
     my $url = $self->_method('newAccount');
 
+    if ($eab) {
+	my $eab_hmac_key = decode_base64($eab->{hmac_key});
+	$info{externalAccountBinding} = $self->external_account_binding_jws(
+	    $eab->{kid},
+	    $eab_hmac_key,
+	    $url
+	);
+    }
+
     if ($tos_url) {
 	$self->{tos} = $tos_url;
 	$info{termsOfServiceAgreed} = JSON::true;
-- 
2.39.2

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

[Thomas Lamprecht]

Am 25/10/2023 um 15:07 schrieb Folke Gleumes:
> Changes v1 -> v2:
> Switched from including the eab credentials in the info hash,
> to passing them in their own variable. This still unfortunately still 
> breaks the api, but doesn't potentially expose secrets and is
> cleaner then purging the values from the hash afterwards.

yeah, IMO the signature of that method is still not really ideal, i.e.,
adding that as explicit param is a lateral move and still breaks ABI, so
meh, and I'd prefer

>  src/PVE/ACME.pm | 42 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
> index 3f66182..7b3840b 100644
> --- a/src/PVE/ACME.pm
> +++ b/src/PVE/ACME.pm

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

should this be actually a private method? I.e., some

my sub external_account_binding_jws { }

or as code ref in a variable:

my $external_account_binding_jws = sub { }

FYI: For above examples you'd need to pass $self then explicitly
either way though.

As this is probably not intended to be called from the "outside"?
Albeit, that would be yet another way to avoid API breakage: let the
caller call this first and place the result into the %info hash, but
that would still keep that unchecked passing along of the %info hash.

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

[Thomas Lamprecht]

Am 27/10/2023 um 08:58 schrieb Thomas Lamprecht:
> Am 25/10/2023 um 15:07 schrieb Folke Gleumes:
>> Changes v1 -> v2:
>> Switched from including the eab credentials in the info hash,
>> to passing them in their own variable. This still unfortunately still 
>> breaks the api, but doesn't potentially expose secrets and is
>> cleaner then purging the values from the hash afterwards.
> 
> yeah, IMO the signature of that method is still not really ideal, i.e.,
> adding that as explicit param is a lateral move and still breaks ABI, so
> meh, and I'd prefer

argh, sent to early, but bascially what I write in my reply to v1:

https://lists.proxmox.com/pipermail/pve-devel/2023-October/059670.html

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

[Folke Gleumes]

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

Changes v1 -> v2:
 * renamed api methods so they use '-' instead of '_'
 * use 'requires' in api to declare dependency instead of manual check

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

diff --git a/PVE/API2/ACMEAccount.pm b/PVE/API2/ACMEAccount.pm
index b790843a..994d2b49 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,13 @@ __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 $eab = (defined($eab_kid) and defined($eab_hmac_key)) ? {
+		    kid => $eab_kid,
+		    hmac_key => $eab_hmac_key
+		} : undef;
+
+		eval { $acme->new_account($param->{tos_url}, $eab, contact => $contact); };
+
 		if (my $err = $@) {
 		    unlink $account_file;
 		    die "Registration failed: $err\n";
-- 
2.39.2

[pve-devel] [PATCH manager v2 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>
---

Changes v1 -> v2:
 * Include additional possible return values in api definition

 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 994d2b49..aac3f527 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' },
@@ -329,11 +330,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,
@@ -360,6 +362,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 v2 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>
---

Changes v1 -> v2:
 * If a custom directory is used, ask if EAB should be used, even when not required by the CA.

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

No changes in v2

 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