From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <w.bumiller@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 8BF9369EE0
 for <pmg-devel@lists.proxmox.com>; Fri, 12 Mar 2021 15:51:49 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 744EB342B8
 for <pmg-devel@lists.proxmox.com>; Fri, 12 Mar 2021 15:51:19 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 6A4B4342AC
 for <pmg-devel@lists.proxmox.com>; Fri, 12 Mar 2021 15:51:17 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 368FF463E1
 for <pmg-devel@lists.proxmox.com>; Fri, 12 Mar 2021 15:51:17 +0100 (CET)
Date: Fri, 12 Mar 2021 15:51:14 +0100
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>
Cc: pmg-devel@lists.proxmox.com
Message-ID: <20210312145114.h5s65pewiwmsajfl@wobu-vie.proxmox.com>
References: <20210309141401.19237-1-w.bumiller@proxmox.com>
 <20210309141401.19237-7-w.bumiller@proxmox.com>
 <b99b2e0a-0cd5-9441-05e0-49b563e11426@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b99b2e0a-0cd5-9441-05e0-49b563e11426@proxmox.com>
User-Agent: NeoMutt/20180716
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.038 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [when.pm, action.pm, nodes.pm, certificates.pm, acmeplugin.pm,
 api2.pm, acme.pm, what.pm]
Subject: Re: [pmg-devel] [PATCH api 6/8] add certificates api endpoint
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
 <pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 12 Mar 2021 14:51:49 -0000

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