From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 C3CAF696BA for ; Thu, 11 Mar 2021 11:05:23 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B754F2696C for ; Thu, 11 Mar 2021 11:05:23 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id EA8C62695E for ; Thu, 11 Mar 2021 11:05:22 +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 AFE9841BDF for ; Thu, 11 Mar 2021 11:05:22 +0100 (CET) To: pmg-devel@lists.proxmox.com References: <20210309141401.19237-1-w.bumiller@proxmox.com> <20210309141401.19237-3-w.bumiller@proxmox.com> From: Dominik Csapak Message-ID: Date: Thu, 11 Mar 2021 11:05:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210309141401.19237-3-w.bumiller@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.187 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 T_FILL_THIS_FORM_SHORT 0.01 Fill in a short form with personal information Subject: Re: [pmg-devel] [PATCH api 2/8] add PMG::CertHelpers module X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Mar 2021 10:05:23 -0000 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 > --- > 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; > +} >