Re: [pmg-devel] [PATCH api 6/8] add certificates api endpoint

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Mar 11, 2021 at 12:06:23PM +0100, Dominik Csapak wrote:
> 
> 
> On 3/9/21 3:13 PM, Wolfgang Bumiller wrote:
> > This adds /nodes/{nodename}/certificates endpoint
> > containing:
> > 
> >    /custom/{type} - update smtp or api certificates manually
> >    /acme/{type} - update via acme
> > 
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> > ---
> >   src/Makefile                 |   1 +
> >   src/PMG/API2/Certificates.pm | 690 +++++++++++++++++++++++++++++++++++
> >   src/PMG/API2/Nodes.pm        |   7 +
> >   3 files changed, 698 insertions(+)
> >   create mode 100644 src/PMG/API2/Certificates.pm
> > 
> > diff --git a/src/Makefile b/src/Makefile
> > index ebc6bd8..e0629b2 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -155,6 +155,7 @@ LIBSOURCES =				\
> >   	PMG/API2/When.pm		\
> >   	PMG/API2/What.pm		\
> >   	PMG/API2/Action.pm		\
> > +	PMG/API2/Certificates.pm	\
> >   	PMG/API2/ACME.pm		\
> >   	PMG/API2/ACMEPlugin.pm		\
> >   	PMG/API2.pm			\
> > diff --git a/src/PMG/API2/Certificates.pm b/src/PMG/API2/Certificates.pm
> > new file mode 100644
> > index 0000000..d196af6
> > --- /dev/null
> > +++ b/src/PMG/API2/Certificates.pm
> > @@ -0,0 +1,690 @@
> > +package PMG::API2::Certificates;
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use PVE::Certificate;
> > +use PVE::Exception qw(raise raise_param_exc);
> > +use PVE::JSONSchema qw(get_standard_option);
> > +use PVE::Tools qw(extract_param file_get_contents file_set_contents);
> > +
> > +use PMG::CertHelpers;
> > +use PMG::NodeConfig;
> > +use PMG::RS::CSR;
> > +
> > +use PMG::API2::ACMEPlugin;
> > +
> > +use base qw(PVE::RESTHandler);
> > +
> > +my $acme_account_dir = PMG::CertHelpers::acme_account_dir();
> > +
> > +sub first_typed_pem_entry : prototype($$) {
> > +    my ($label, $data) = @_;
> > +
> > +    if ($data =~ /^(-----BEGIN (?<label>\Q$label\E)-----\n.*?\n-----END \g{label}-----)$/ms) {
> 
> nit: isn't
> $data =~ /^(-----BEGIN \Q$label\E-----\n.*?\n-----END \Q$label\E-----)$/ms
> 
> shorter and does the same?
> 
> 
> > +	chomp(my $content = $1);
> 
> nit: why chomp? the regex does not allow trailing/whitespace newlines in $1
> ?
> 

Yeah those are leftovers from previous versions.

> > +__PACKAGE__->register_method ({
> > +    name => 'upload_custom_cert',
> > +    path => 'custom/{type}',
> > +    method => 'POST',
> > +    permissions => { check => [ 'admin' ] },
> > +    description => 'Upload or update custom certificate chain and key.',
> > +    protected => 1,
> > +    proxyto => 'node',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    certificates => {
> > +		type => 'string',
> > +		format => 'pem-certificate-chain',
> > +		description => 'PEM encoded certificate (chain).',
> > +	    },
> > +	    key => {
> > +		type => 'string',
> > +		description => 'PEM encoded private key.',
> > +		format => 'pem-string',
> > +		optional => 0,
> > +	    },
> > +	    type => get_standard_option('pmg-certificate-type'),
> > +	    force => {
> > +		type => 'boolean',
> > +		description => 'Overwrite existing custom or ACME certificate files.',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> > +	    restart => {
> > +		type => 'boolean',
> > +		description => 'Restart services.',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> > +	},
> > +    },
> > +    returns => get_standard_option('pve-certificate-info'),
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $type = extract_param($param, 'type'); # also used to know which service to restart
> > +	my $cert_path = PMG::CertHelpers::cert_path($type);
> > +
> > +	my $certs = extract_param($param, 'certificates');
> > +	$certs = PVE::Certificate::strip_leading_text($certs);
> > +
> > +	my $key = extract_param($param, 'key');
> > +	if ($key) {
> > +	    $key = PVE::Certificate::strip_leading_text($key);
> > +	    $certs = "$key\n$certs";
> > +	} else {
> > +	    my $private_key = pem_private_key($certs);
> > +	    if (!defined($private_key)) {
> > +		my $old = file_get_contents($cert_path);
> > +		$private_key = pem_private_key($old);
> > +		if (!defined($private_key)) {
> > +		    raise_param_exc({
> > +			'key' => "Attempted to upload custom certificate without (existing) key."
> > +		    })
> > +		}
> > +
> > +		# copy the old certificate's key:
> > +		$certs = "$key\n$certs";
> > +	    }
> > +	}
> > +
> > +	my $info;
> > +
> > +	my $code = sub {
> > +	    print "Setting custom certificate file $cert_path\n";
> > +	    $info = PMG::CertHelpers::set_cert_file($certs, $cert_path, $param->{force});
> > +
> > +	    if ($type eq 'api' && $param->{restart}) {
> > +		print "Restarting pmgproxy\n";
> > +		PVE::Tools::run_command(['systemctl', 'reload-or-restart', 'pmgproxy']);
> 
> you could reuse the 'restart_after_cert_update' here, no?

Yeah, actually I'll pass $param->{restart} along to update_cert()

> 
> > +	    }
> > +	};
> > +
> > +	PMG::CertHelpers::cert_lock(10, $code);
> > +	die "$@\n" if $@;
> > +
> > +	if ($type eq 'smtp') {
> > +	    $code = sub {
> > +		my $cfg = PMG::Config->new();
> > +
> > +		print "Rewriting postfix config\n";
> > +		$cfg->set('mail', 'tls', 1);
> > +		$cfg->rewrite_config_postfix();
> > +
> > +		if ($param->{restart}) {
> > +		    print "Reloading postfix\n";
> > +		    PMG::Utils::service_cmd('postfix', 'reload');
> 
> also, why couldn't that be handled there too? then we could
> combine the two restart/reload calls?

These run with 2 different locks.

(And failure to reload update the config or reloading postfix should not
undo the certificate update.)

(Technically neither should reloading pmgproxy though that's just the
same as in PVE and can really only fail if the `systemctl` command
fails, so that's rather rare.)

> 
> > +		}
> > +	    };
> > +	    PMG::Config::lock_config($code, "failed to reload postfix");
> > +	}
> > +
> > +	return $info;
> > +    }});
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'remove_custom_cert',
> > +    path => 'custom/{type}',
> > +    method => 'DELETE',
> > +    permissions => { check => [ 'admin' ] },
> > +    description => 'DELETE custom certificate chain and key.',
> > +    protected => 1,
> > +    proxyto => 'node',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    type => get_standard_option('pmg-certificate-type'),
> > +	    restart => {
> > +		type => 'boolean',
> > +		description => 'Restart pmgproxy.',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'null',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $type = extract_param($param, 'type');
> > +	my $cert_path = PMG::CertHelpers::cert_path($type);
> > +
> > +	my $code = sub {
> > +	    print "Deleting custom certificate files\n";
> > +	    unlink $cert_path;
> > +
> > +	    if ($param->{restart}) {
> > +		restart_after_cert_update($type);
> > +	    }
> > +	};
> > +
> > +	PMG::CertHelpers::cert_lock(10, $code);
> > +	die "$@\n" if $@;
> > +
> > +	return undef;
> 
> don't we need to update the postfix config and reload if the type is smtp?
> or at least error out?

Yeah, adding that...

> > +    }});
> > +

> > +# Filter domains and raise an error if the list becomes empty.
> > +my $filter_domains = sub {
> > +    my ($acme_config, $type) = @_;
> > +
> > +    my $domains = $acme_config->{domains};
> > +    foreach my $domain (keys %$domains) {
> > +	my $entry = $domains->{$domain};
> > +	if (!(grep { $_ eq $type } PVE::Tools::split_list($entry->{usage}))) {
> > +	    delete $domains->{$domain};
> > +	}
> > +    }
> > +
> > +    if (!%$domains) {
> > +	raise("No domains configured for type '$type'\n", 400);
> > +    }
> > +};
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'new_acme_cert',
> > +    path => 'acme/{type}',
> > +    method => 'POST',
> > +    permissions => { check => [ 'admin' ] },
> > +    description => 'Order a new certificate from ACME-compatible CA.',
> > +    protected => 1,
> > +    proxyto => 'node',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    type => get_standard_option('pmg-certificate-type'),
> > +	    force => {
> > +		type => 'boolean',
> > +		description => 'Overwrite existing custom certificate.',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $type = extract_param($param, 'type'); # also used to know which service to restart
> > +	my $cert_path = PMG::CertHelpers::cert_path($type);
> > +	raise_param_exc({'force' => "Custom certificate exists but 'force' is not set."})
> > +	    if !$param->{force} && -e $cert_path;
> > +
> > +	my $node_config = PMG::NodeConfig::load_config();
> > +	my $acme_config = PMG::NodeConfig::get_acme_conf($node_config);
> > +	raise("ACME domain list in configuration is missing!", 400)
> > +	    if !$acme_config || !$acme_config->{domains}->%*;
> > +
> > +	$filter_domains->($acme_config, $type);
> > +
> > +	my $rpcenv = PMG::RESTEnvironment->get();
> > +	my $authuser = $rpcenv->get_user();
> > +
> > +	my $realcmd = sub {
> > +	    STDOUT->autoflush(1);
> > +	    my $account = $acme_config->{account};
> > +	    my $account_file = "${acme_account_dir}/${account}";
> > +	    die "ACME account config file '$account' does not exist.\n"
> > +		if ! -e $account_file;
> > +
> > +	    print "Loading ACME account details\n";
> > +	    my $acme = PMG::RS::Acme->load($account_file);
> > +
> > +	    my ($cert, $key) = $order_certificate->($acme, $acme_config);
> > +	    my $certificate = "$key\n$cert";
> > +
> > +	    update_cert($type, $cert_path, $certificate, $param->{force});
> > +
> > +	    if ($type eq 'smtp') {
> > +		my $code = sub {
> > +		    my $cfg = PMG::Config->new();
> > +
> > +		    print "Rewriting postfix config\n";
> > +		    $cfg->set('mail', 'tls', 1);
> > +		    if ($cfg->rewrite_config_postfix()) {
> > +			print "Reloading postfix\n";
> > +			PMG::Utils::service_cmd('postfix', 'reload');
> > +		    }
> > +		};
> > +		PMG::Config::lock_config($code, "failed to reload postfix");
> > +	    }
> > +
> > +	    die "$@\n" if $@;
> > +	};
> > +
> > +	return $rpcenv->fork_worker("acmenewcert", undef, $authuser, $realcmd);
> > +    }});
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'renew_acme_cert',
> > +    path => 'acme/{type}',
> > +    method => 'PUT',
> > +    permissions => { check => [ 'admin' ] },
> > +    description => "Renew existing certificate from CA.",
> > +    protected => 1,
> > +    proxyto => 'node',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    type => get_standard_option('pmg-certificate-type'),
> > +	    force => {
> > +		type => 'boolean',
> > +		description => 'Force renewal even if expiry is more than 30 days away.',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $type = extract_param($param, 'type'); # also used to know which service to restart
> > +	my $cert_path = PMG::CertHelpers::cert_path($type);
> > +
> > +	raise("No current (custom) certificate found, please order a new certificate!\n")
> > +	    if ! -e $cert_path;
> > +
> > +	my $expires_soon = PVE::Certificate::check_expiry($cert_path, time() + 30*24*60*60);
> > +	raise_param_exc({'force' => "Certificate does not expire within the next 30 days, and 'force' is not set."})
> > +	    if !$expires_soon && !$param->{force};
> > +
> > +	my $node_config = PMG::NodeConfig::load_config();
> > +	my $acme_config = PMG::NodeConfig::get_acme_conf($node_config);
> > +	raise("ACME domain list in configuration is missing!", 400)
> > +	    if !$acme_config || !$acme_config->{domains}->%*;
> > +
> > +	$filter_domains->($acme_config, $type);
> > +
> > +	my $rpcenv = PMG::RESTEnvironment->get();
> > +	my $authuser = $rpcenv->get_user();
> > +
> > +	my $old_cert = PVE::Tools::file_get_contents($cert_path);
> > +
> > +	my $realcmd = sub {
> > +	    STDOUT->autoflush(1);
> > +	    my $account = $acme_config->{account};
> > +	    my $account_file = "${acme_account_dir}/${account}";
> > +	    die "ACME account config file '$account' does not exist.\n"
> > +		if ! -e $account_file;
> > +
> > +	    print "Loading ACME account details\n";
> > +	    my $acme = PMG::RS::Acme->load($account_file);
> > +
> > +	    my ($cert, $key) = $order_certificate->($acme, $acme_config);
> > +	    my $certificate = "$key\n$cert";
> > +
> > +	    update_cert($type, $cert_path, $certificate, 1);
> > +
> > +	    if (defined($old_cert)) {
> > +		print "Revoking old certificate\n";
> > +		eval { $acme->revoke_certificate($old_cert, undef) };
> > +		warn "Revoke request to CA failed: $@" if $@;
> > +	    }
> > +	};
> > +
> > +	return $rpcenv->fork_worker("acmerenew", undef, $authuser, $realcmd);
> > +    }});
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'revoke_acme_cert',
> > +    path => 'acme/{type}',
> > +    method => 'DELETE',
> > +    permissions => { check => [ 'admin' ] },
> > +    description => "Revoke existing certificate from CA.",
> > +    protected => 1,
> > +    proxyto => 'node',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    type => get_standard_option('pmg-certificate-type'),
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $type = extract_param($param, 'type'); # also used to know which service to restart
> > +	my $cert_path = PMG::CertHelpers::cert_path($type);
> > +
> > +	my $node_config = PMG::NodeConfig::load_config();
> > +	my $acme_config = PMG::NodeConfig::get_acme_conf($node_config);
> > +	raise("ACME domain list in configuration is missing!", 400)
> > +	    if !$acme_config || !$acme_config->{domains}->%*;
> > +
> > +	$filter_domains->($acme_config, $type);
> > +
> > +	my $rpcenv = PMG::RESTEnvironment->get();
> > +	my $authuser = $rpcenv->get_user();
> > +
> > +	my $cert = PVE::Tools::file_get_contents($cert_path);
> > +	$cert = pem_certificate($cert)
> > +	    or die "no certificate section found in '$cert_path'\n";
> > +
> > +	my $realcmd = sub {
> > +	    STDOUT->autoflush(1);
> > +	    my $account = $acme_config->{account};
> > +	    my $account_file = "${acme_account_dir}/${account}";
> > +	    die "ACME account config file '$account' does not exist.\n"
> > +		if ! -e $account_file;
> > +
> > +	    print "Loading ACME account details\n";
> > +	    my $acme = PMG::RS::Acme->load($account_file);
> > +
> > +	    print "Revoking old certificate\n";
> > +	    eval { $acme->revoke_certificate($cert, undef) };
> > +	    if (my $err = $@) {
> > +		# is there a better check?
> > +		die "Revoke request to CA failed: $err" if $err !~ /"Certificate is expired"/;
> > +	    }
> > +
> > +	    my $code = sub {
> > +		print "Deleting certificate files\n";
> > +		unlink $cert_path;
> > +
> > +		# FIXME: Regenerate self-signed `api` certificate.
> > +		restart_after_cert_update($type);
> > +	    };
> > +
> > +	    PMG::CertHelpers::cert_lock(10, $code);
> > +	    die "$@\n" if $@;
> > +	};
> > +
> > +	return $rpcenv->fork_worker("acmerevoke", undef, $authuser, $realcmd);
> > +    }});
> 
> what happens here with postfix?

will update & reload here too