Re: [pmg-devel] [PATCH api 2/8] add PMG::CertHelpers module

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 3/9/21 3:13 PM, Wolfgang Bumiller wrote:
> Contains helpers to update certificates and provide locking
> for certificates and when accessing acme accounts.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>   src/Makefile           |   1 +
>   src/PMG/CertHelpers.pm | 180 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 181 insertions(+)
>   create mode 100644 src/PMG/CertHelpers.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index 8891a3c..c1d4812 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -55,6 +55,7 @@ LIBSOURCES =				\
>   	PMG/HTMLMail.pm			\
>   	PMG/ModGroup.pm			\
>   	PMG/SMTPPrinter.pm		\
> +	PMG/CertHelpers.pm		\
>   	PMG/Config.pm			\
>   	PMG/Cluster.pm			\
>   	PMG/ClusterConfig.pm		\
> diff --git a/src/PMG/CertHelpers.pm b/src/PMG/CertHelpers.pm
> new file mode 100644
> index 0000000..2cf8a4e
> --- /dev/null
> +++ b/src/PMG/CertHelpers.pm
> @@ -0,0 +1,180 @@
> +package PMG::CertHelpers;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Certificate;
> +use PVE::JSONSchema;
> +use PVE::Tools;
> +
> +use constant {
> +    API_CERT => '/etc/pmg/pmg-api.pem',
> +    SMTP_CERT => '/etc/pmg/pmg-tls.pem',
> +};
> +
> +my $account_prefix = '/etc/pmg/acme';
> +
> +# TODO: Move `pve-acme-account-name` to common and reuse instead of this.
> +PVE::JSONSchema::register_standard_option('pmg-acme-account-name', {
> +    description => 'ACME account config file name.',
> +    type => 'string',
> +    format => 'pve-configid',
> +    format_description => 'name',
> +    optional => 1,
> +    default => 'default',
> +});
> +
> +PVE::JSONSchema::register_standard_option('pmg-acme-account-contact', {
> +    type => 'string',
> +    format => 'email-list',
> +    description => 'Contact email addresses.',
> +});
> +
> +PVE::JSONSchema::register_standard_option('pmg-acme-directory-url', {
> +    type => 'string',
> +    description => 'URL of ACME CA directory endpoint.',
> +    pattern => '^https?://.*',
> +});
> +
> +PVE::JSONSchema::register_format('pmg-certificate-type', sub {
> +    my ($type, $noerr) = @_;
> +
> +    if ($type =~ /^(?: api | smtp )$/x) {
> +	return $type;
> +    }
> +    return undef if $noerr;
> +    die "value '$type' does not look like a valid certificate type\n";
> +});
> +
> +PVE::JSONSchema::register_standard_option('pmg-certificate-type', {
> +    type => 'string',
> +    description => 'The TLS certificate type (API or SMTP certificate).',
> +    enum => ['api', 'smtp'],
> +});

i get why you did the format and the option (you need it once as a 
'-list') but would it not have been possible to reuse the format instead
of redefining the enum?

or only using the enum as variable defined somewhere?

feels weird to have a format + option that do basically
the same thing

> +
> +PVE::JSONSchema::register_format('pmg-acme-domain', sub {
> +    my ($domain, $noerr) = @_;
> +
> +    my $label = qr/[a-z0-9][a-z0-9_-]*/i;
> +
> +    return $domain if $domain =~ /^$label(?:\.$label)+$/;
> +    return undef if $noerr;
> +    die "value '$domain' does not look like a valid domain name!\n";
> +});
> +
> +PVE::JSONSchema::register_format('pmg-acme-alias', sub {
> +    my ($alias, $noerr) = @_;
> +
> +    my $label = qr/[a-z0-9_][a-z0-9_-]*/i;
> +
> +    return $alias if $alias =~ /^$label(?:\.$label)+$/;
> +    return undef if $noerr;
> +    die "value '$alias' does not look like a valid alias name!\n";
> +});

could we not reuse the '-domain' format here ?
i know the error message would be different then, but it is still a domain?

if not, we could refactor the regexes though

> +
> +my $local_cert_lock = '/var/lock/pmg-certs.lock';
> +my $local_acme_lock = '/var/lock/pmg-acme.lock';
> +
> +sub cert_path : prototype($) {
> +    my ($type) = @_;
> +    if ($type eq 'api') {
> +	return API_CERT;
> +    } elsif ($type eq 'smtp') {
> +	return SMTP_CERT;
> +    } else {
> +	die "unknown certificate type '$type'\n";
> +    }
> +}
> +
> +sub cert_lock {
> +    my ($timeout, $code, @param) = @_;
> +
> +    my $res = PVE::Tools::lock_file($local_cert_lock, $timeout, $code, @param);
> +    die $@ if $@;
> +    return $res;
> +}
> +
> +sub set_cert_file {
> +    my ($cert, $cert_path, $force) = @_;
> +
> +    my ($old_cert, $info);
> +
> +    my $cert_path_old = "${cert_path}.old";
> +
> +    die "Custom certificate file exists but force flag is not set.\n"
> +	if !$force && -e $cert_path;
> +
> +    PVE::Tools::file_copy($cert_path, $cert_path_old) if -e $cert_path;
> +
> +    eval {
> +	my $gid = undef;
> +	if ($cert_path eq &API_CERT) {
> +	    $gid = getgrnam('www-data') ||
> +		die "user www-data not in group file\n";
> +	}
> +
> +	if (defined($gid)) {
> +	    my $cert_path_tmp = "${cert_path}.tmp";
> +	    PVE::Tools::file_set_contents($cert_path_tmp, $cert, 0640);
> +	    if (!chown(-1, $gid, $cert_path_tmp)) {
> +		my $msg =
> +		    "failed to change group ownership of '$cert_path_tmp' to www-data ($gid): $!\n";
> +		unlink($cert_path_tmp);
> +		die $msg;
> +	    }
> +	    if (!rename($cert_path_tmp, $cert_path)) {
> +		my $msg =
> +		    "failed to rename '$cert_path_tmp' to '$cert_path': $!\n";
> +		unlink($cert_path_tmp);
> +		die $msg;
> +	    }
> +	} else {
> +	    PVE::Tools::file_set_contents($cert_path, $cert, 0600);
> +	}
> +
> +	$info = PVE::Certificate::get_certificate_info($cert_path);
> +    };
> +    my $err = $@;
> +
> +    if ($err) {
> +	if (-e $cert_path_old) {
> +	    eval {
> +		warn "Attempting to restore old certificate file..\n";
> +		PVE::Tools::file_copy($cert_path_old, $cert_path);
> +	    };
> +	    warn "$@\n" if $@;
> +	}
> +	die "Setting certificate files failed - $err\n"
> +    }
> +
> +    unlink $cert_path_old;
> +
> +    return $info;
> +}
> +
> +sub lock_acme {
> +    my ($account_name, $timeout, $code, @param) = @_;
> +
> +    my $file = "$local_acme_lock.$account_name";
> +
> +    return PVE::Tools::lock_file($file, $timeout, $code, @param);
> +}
> +

is there a special reason why you die $@ if $@ above in cert_lock
but not here?

afaics, you do it manually in the later patches always anyway

> +sub acme_account_dir {
> +    return $account_prefix;
> +}
> +
> +sub list_acme_accounts {
> +    my $accounts = [];
> +
> +    return $accounts if ! -d $account_prefix;
> +
> +    PVE::Tools::dir_glob_foreach($account_prefix, qr/[^.]+.*/, sub {
> +	my ($name) = @_;
> +
> +	push @$accounts, $name
> +	    if PVE::JSONSchema::pve_verify_configid($name, 1);
> +    });
> +
> +    return $accounts;
> +}
>