[pmg-devel] [PATCH pmg-api 0/4] cosmetic and minor improvements to certificate integration

[pmg-devel] [PATCH pmg-api 0/4] cosmetic and minor improvements to certificate integration

[Stoiko Ivanov]

while trying the new acme integration on our internal system, we noticed 3
small glitches:
* /etc/pmg/acme/accounts could not get created (since it's parent dir did
  not exist) (patches 1,2)
* cluster fingerprint update failed when running on master-node (patch 3)
* postfix did not load the new pmg-tls.pem after the successful task (patch
  4)

all tested on my test-installs

Stoiko Ivanov (4):
  package: ship /etc/pmg/acme/accounts in deb
  acme: recursively create account directory
  cluster: use old and new fingerprint on master
  certs: reload postfix to activate new certificate

 debian/dirs                  |  1 +
 src/PMG/API2/ACME.pm         |  4 +++-
 src/PMG/API2/Certificates.pm | 14 ++++++--------
 src/PMG/Cluster.pm           | 12 ++++++------
 4 files changed, 16 insertions(+), 15 deletions(-)

-- 
2.20.1

[pmg-devel] [PATCH pmg-api 1/4] package: ship /etc/pmg/acme/accounts in deb

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 debian/dirs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/debian/dirs b/debian/dirs
index f138bb4..ec10482 100644
--- a/debian/dirs
+++ b/debian/dirs
@@ -1,4 +1,5 @@
 /etc/pmg
+/etc/pmg/acme/accounts
 /etc/pmg/dkim
 /etc/pmg/pbs
 /var/lib/pmg
-- 
2.20.1

[pmg-devel] [PATCH pmg-api 2/4] acme: recursively create account directory

[Stoiko Ivanov]

to account for the new layout

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/ACME.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PMG/API2/ACME.pm b/src/PMG/API2/ACME.pm
index 1b35511..7aee17a 100644
--- a/src/PMG/API2/ACME.pm
+++ b/src/PMG/API2/ACME.pm
@@ -3,6 +3,8 @@ package PMG::API2::ACME;
 use strict;
 use warnings;
 
+use File::Path;
+
 use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(extract_param);
@@ -142,7 +144,7 @@ __PACKAGE__->register_method ({
 	my $authuser = $rpcenv->get_user();
 
 	my ($account_name, $account_file) = extract_account_name($param);
-	mkdir $acme_account_dir if ! -e $acme_account_dir;
+	File::Path::make_path($acme_account_dir) if ! -e $acme_account_dir;
 
 	raise_param_exc({'name' => "ACME account config file '${account_name}' already exists."})
 	    if -e $account_file;
-- 
2.20.1

[pmg-devel] [PATCH pmg-api 3/4] cluster: use old and new fingerprint on master

[Stoiko Ivanov]

when triggering a fingerprint update on master right after reloading
pmgproxy as we do for ACME certificates it can happen that the
connection is made against the old pmgproxy process (with the old
fingerprint). Simply trusting both fingerprints in that case seems
acceptable from a security perspective and makes the fingerprint
update more robust

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Cluster.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
index e7bf266..acaea8d 100644
--- a/src/PMG/Cluster.pm
+++ b/src/PMG/Cluster.pm
@@ -316,11 +316,13 @@ sub trigger_update_fingerprints {
     my ($cinfo) = @_;
 
     my $master = $cinfo->{master} || die "unable to lookup master node\n";
-    my $master_fp = $master->{fingerprint};
+    my $cached_fp = { $master->{fingerprint} => 1 };
 
     # if running on master the current fingerprint for the API-connection is needed
+    # in addition (to prevent races with restarting pmgproxy
     if ($cinfo->{local}->{type} eq 'master') {
-	$master_fp = PMG::Cluster::read_local_ssl_cert_fingerprint();
+	my $new_fp = PMG::Cluster::read_local_ssl_cert_fingerprint();
+	$cached_fp->{$new_fp} = 1;
     }
 
     my $ticket = PMG::Ticket::assemble_ticket('root@pam');
@@ -330,10 +332,8 @@ sub trigger_update_fingerprints {
 	csrftoken => $csrftoken,
 	cookie_name => 'PMGAuthCookie',
 	host => $master->{ip},
-	cached_fingerprints => {
-	    $master_fp => 1,
-	},
-    );
+	cached_fingerprints => $cached_fp,
+	);
 
     $conn->post("/config/cluster/update-fingerprints", {});
     return undef;
-- 
2.20.1

[pmg-devel] [PATCH pmg-api 4/4] certs: reload postfix to activate new certificate

[Stoiko Ivanov]

the current logic for reloading postfix only does so if the tls config
parameter changes (after rewriting the config files).
this does not cover the case where a certificate is replaced in a
setup, which already has tls enabled (config stays the same, so
postfix does not get reloaded)

the issue is mostly cosmetic, since postfix does eventually fork off
new smtpd instances, which read the files from disk, but it's
inconvenient, when trying out the new acme integration, and then
running a ssl-check on your PMG from external just to see that the
certificate was not updated.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
best viewed with `git show -w`
 src/PMG/API2/Certificates.pm | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/PMG/API2/Certificates.pm b/src/PMG/API2/Certificates.pm
index 1a6c434..1a7ded6 100644
--- a/src/PMG/API2/Certificates.pm
+++ b/src/PMG/API2/Certificates.pm
@@ -69,16 +69,14 @@ my sub set_smtp : prototype($$) {
 
     my $code = sub {
 	my $cfg = PMG::Config->new();
-	if (!$cfg->get('mail', 'tls') == !$on) {
-	    return;
+	if (!$cfg->get('mail', 'tls') != !$on) {
+	    print "Rewriting postfix config\n";
+	    $cfg->set('mail', 'tls', $on);
+	    $cfg->write();
+	    my $changed = $cfg->rewrite_config_postfix();
 	}
 
-	print "Rewriting postfix config\n";
-	$cfg->set('mail', 'tls', $on);
-	$cfg->write();
-	my $changed = $cfg->rewrite_config_postfix();
-
-	if ($changed && $reload) {
+	if ($reload) {
 	    print "Reloading postfix\n";
 	    PMG::Utils::service_cmd('postfix', 'reload');
 	}
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-api 0/4] cosmetic and minor improvements to certificate integration

[Thomas Lamprecht]

On 18.03.21 16:14, Stoiko Ivanov wrote:
> while trying the new acme integration on our internal system, we noticed 3
> small glitches:
> * /etc/pmg/acme/accounts could not get created (since it's parent dir did
>   not exist) (patches 1,2)
> * cluster fingerprint update failed when running on master-node (patch 3)
> * postfix did not load the new pmg-tls.pem after the successful task (patch
>   4)
> 
> all tested on my test-installs
> 
> Stoiko Ivanov (4):
>   package: ship /etc/pmg/acme/accounts in deb
>   acme: recursively create account directory
>   cluster: use old and new fingerprint on master
>   certs: reload postfix to activate new certificate
> 
>  debian/dirs                  |  1 +
>  src/PMG/API2/ACME.pm         |  4 +++-
>  src/PMG/API2/Certificates.pm | 14 ++++++--------
>  src/PMG/Cluster.pm           | 12 ++++++------
>  4 files changed, 16 insertions(+), 15 deletions(-)
> 


applied, thanks!

Dropped a now useless variable from set_smtp, also unified make_path, mkpath and
remove_tree, rmtree usages tree-wide