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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5DB6E69EBB for ; Fri, 12 Mar 2021 14:55:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4F03133557 for ; Fri, 12 Mar 2021 14:55: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D86923354B for ; Fri, 12 Mar 2021 14:55: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 9B42D463DD for ; Fri, 12 Mar 2021 14:55:17 +0100 (CET) Date: Fri, 12 Mar 2021 14:55:16 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pmg-devel@lists.proxmox.com, f.gruenbichler@proxmox.com Message-ID: <20210312135516.jxmbeuerjry6252v@wobu-vie.proxmox.com> References: <20210309141401.19237-1-w.bumiller@proxmox.com> <20210309141401.19237-3-w.bumiller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.033 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 T_FILL_THIS_FORM_SHORT 0.01 Fill in a short form with personal information URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [htmlmail.pm, config.pm, clusterconfig.pm, cluster.pm, modgroup.pm, certhelpers.pm, smtpprinter.pm] 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: Fri, 12 Mar 2021 13:55:19 -0000 On Thu, Mar 11, 2021 at 11:05:21AM +0100, Dominik Csapak wrote: > 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? Yeah I suppose that works > or only using the enum as variable defined somewhere? (or at least that will) > feels weird to have a format + option that do basically > the same thing I know > > > + > > +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 -alias was specifically introduced to allow using underscore prefixes. (See pve-manager b3d421707) > > > + > > +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 That's just lock api madness... will fix.