lacme: Don't write certificate(-chain) file on chown/chmod failure.

Otherwise we end up with files with mode 0644 owned by root:root, and
subsequent lacme(8) invocations will likely not renew them for a while.

This change also saves a chown(2) call.  And the new logic (chown resp.
chmod from root:root resp. 0600) is safe if we ever include private key
material in there too.

3 files changed, 44 insertions, 45 deletions

diff --git a/Changelog b/Changelog
index e047ac5..2a027f1 100644
--- a/Changelog
+++ b/Changelog
@@ -2,6 +2,7 @@ lacme (0.8.1) upstream;
 
  + lacme-accountd: improve log messages and refactor logging logic.
  + lacme-accountd: refuse to sign JWS with an invalid Protected Header.
+ + lacme: don't write certificate(-chain) file on chown/chmod failure.
  - lacme: in the [accountd] config, let lacme-accountd(1) do the
    %-expansion for 'config', not lacme(8) when building the command.
  - lacme-accountd: don't log debug messages unless --debug is set.
diff --git a/lacme b/lacme
index fb19646..2366830 100755
--- a/lacme
+++ b/lacme
@@ -657,8 +657,9 @@ sub spawn($@) {
 #############################################################################
 # Install the certificate (optionally excluding the chain of trust)
 #
-sub install_cert($$;$) {
-    my ($filename, $chain, $leafonly) = @_;
+sub install_cert(%) {
+    my %args = @_;
+    my $filename = $args{path} // die;
 
     my ($dirname, $basename) =
         $filename =~ /\A(.*)\/([^\/]+)\z/ ? ($1, $2) : ('.', $filename);
@@ -666,10 +667,8 @@ sub install_cert($$;$) {
                                TEMPLATE => "$basename.XXXXXX") // die;
 
     eval {
-        my $umask = umask() // die "umask: $!";
-        chmod(0644 &~ $umask, $fh) or die "chmod: $!";
-        if ($leafonly) {
-            # keep only the leaf certificate
+        if ($args{nochain}) {
+            # keep only the first certificate in the file
             pipe my $rd, my $wd or die "pipe: $!";
             my $pid = fork // die "fork: $!";
             unless ($pid) {
@@ -678,16 +677,34 @@ sub install_cert($$;$) {
                 exec qw/openssl x509 -outform PEM/ or die;
             }
             $rd->close() or die "close: $!";
-            $wd->print($chain);
+            $wd->print($args{content});
             $wd->close() or die "close: $!";
 
             waitpid $pid => 0;
             die $? if $? > 0;
         } else {
-            $fh->print($chain) or die "print: $!";
+            $fh->print($args{content}) or die "print: $!";
         }
+
+        my $mode;
+        if ((my $m = $args{mode}) ne "") {
+            $mode = oct($m) // die;
+        } else {
+            my $umask = umask() // die;
+            $mode = 0644 &~ $umask;
+        }
+        chmod($mode, $fh) or die "chown: $!";
+
+        if ((my $owner = $args{owner}) ne "") {
+            my ($user, $group) = split /:/, $owner, 2;
+            my $uid = getpwnam($user) // die "getpwnam($user)", ($! ? ": $!" : "\n");
+            my $gid = getgrnam($group) // die "getgrnam($group)", ($! ? ": $!" : "\n") if defined $group;
+            chown($uid, $gid // -1, $fh) or die "chown: $!";
+        }
+
         $fh->close() or die "close: $!";
     };
+
     my $path = $fh->filename();
     if ($@) {
         print STDERR "Unlinking $path\n" if $OPTS{debug};
@@ -837,29 +854,16 @@ elsif ($COMMAND eq 'newOrder' or $COMMAND eq 'new-cert') {
             }
         }
 
+        my %install = ( content => $x509, mode => $conf->{chmod} // "", owner => $conf->{chown} // "" );
+
         # install certificate
         if ((my $path = $conf->{'certificate'} // "") ne "") {
             print STDERR "Installing X.509 certificate $path\n";
-            install_cert($path, $x509, 1);
+            install_cert(%install, path => $path, nochain => 1);
         }
         if ((my $path = $conf->{'certificate-chain'} // "") ne "") {
             print STDERR "Installing X.509 certificate chain $path\n";
-            install_cert($path, $x509);
-        }
-
-        if ((my $own = $conf->{chown} // "") ne "") {
-            my ($user, $group) = split /:/, $own, 2;
-            my $uid = getpwnam($user) // die "getpwnam($user)", ($! ? ": $!" : "\n");
-            my $gid = getgrnam($group) // die "getgrnam($group)", ($! ? ": $!" : "\n") if defined $group;
-            foreach (@certs) {
-                chown($uid, $gid // -1, $_) or die "chown: $!";
-            }
-        }
-        if ((my $mode = $conf->{chmod} // "") ne "") {
-            my $mode = oct($mode) // die;
-            foreach (@certs) {
-                chmod($mode, $_) or die "chown: $!";
-            }
+            install_cert(%install, path => $path);
         }
 
         my @certopts = join ',', qw/no_header no_version no_pubkey no_sigdump/;
diff --git a/tests/cert-install b/tests/cert-install
index 39110f4..5d8a239 100644
--- a/tests/cert-install
+++ b/tests/cert-install
@@ -110,25 +110,22 @@ cat >"/etc/lacme/lacme-certs.conf.d/test4.conf" <<- EOF
 	certificate-key = /etc/lacme/test4.key
 	certificate = /etc/lacme/test4.pem
 	certificate-chain = /etc/lacme/test4.crt
-	chown = nobody
+	chown = nonexistent-user
 	subject = $subject
 EOF
 
+! lacme newOrder test4 2>"$STDERR" || fail newOrder test4
+grepstderr -Fxq "getpwnam(nonexistent-user)"
+! test -e /etc/lacme/test4.pem
+! test -e /etc/lacme/test4.crt
+
+sed -ri "s/^chown\\s*=.*/chown = nobody/" /etc/lacme/lacme-certs.conf.d/test4.conf
 lacme newOrder test4 2>"$STDERR" || fail newOrder test4
 st="$(stat -c "%U:%G %#a" /etc/lacme/test4.pem)"
 [ "$st" = "nobody:root 0644" ]
 st="$(stat -c "%U:%G %#a" /etc/lacme/test4.crt)"
 [ "$st" = "nobody:root 0644" ]
 
-rm -f /etc/lacme/test4.pem /etc/lacme/test4.crt
-sed -ri "s/^chown\\s*=.*/chown = nonexistent-user/" /etc/lacme/lacme-certs.conf.d/test4.conf
-! lacme newOrder test4 2>"$STDERR" || fail newOrder test4
-grepstderr -Fxq "getpwnam(nonexistent-user)"
-st="$(stat -c "%U:%G %#a" /etc/lacme/test4.pem)"
-[ "$st" = "root:root 0644" ]
-st="$(stat -c "%U:%G %#a" /etc/lacme/test4.crt)"
-[ "$st" = "root:root 0644" ]
-
 # chown user:group
 openssl genpkey -algorithm RSA -out /etc/lacme/test5.key
 cat >"/etc/lacme/lacme-certs.conf.d/test5.conf" <<- EOF
@@ -136,25 +133,22 @@ cat >"/etc/lacme/lacme-certs.conf.d/test5.conf" <<- EOF
 	certificate-key = /etc/lacme/test5.key
 	certificate = /etc/lacme/test5.pem
 	certificate-chain = /etc/lacme/test5.crt
-	chown = nobody:nogroup
+	chown = nobody:nonexistent-group
 	subject = $subject
 EOF
 
+! lacme newOrder test5 2>"$STDERR" || fail newOrder test5
+grepstderr -Fxq "getgrnam(nonexistent-group)"
+! test -e /etc/lacme/test5.pem
+! test -e /etc/lacme/test5.crt
+
+sed -ri "s/^chown\\s*=.*/chown = nobody:nogroup/" /etc/lacme/lacme-certs.conf.d/test5.conf
 lacme newOrder test5 2>"$STDERR" || fail newOrder test5
 st="$(stat -c "%U:%G %#a" /etc/lacme/test5.pem)"
 [ "$st" = "nobody:nogroup 0644" ]
 st="$(stat -c "%U:%G %#a" /etc/lacme/test5.crt)"
 [ "$st" = "nobody:nogroup 0644" ]
 
-rm -f /etc/lacme/test5.pem /etc/lacme/test5.crt
-sed -ri "s/^chown\\s*=.*/chown = nobody:nonexistent-group/" /etc/lacme/lacme-certs.conf.d/test5.conf
-! lacme newOrder test5 2>"$STDERR" || fail newOrder test5
-grepstderr -Fxq "getgrnam(nonexistent-group)"
-st="$(stat -c "%U:%G %#a" /etc/lacme/test5.pem)"
-[ "$st" = "root:root 0644" ]
-st="$(stat -c "%U:%G %#a" /etc/lacme/test5.crt)"
-[ "$st" = "root:root 0644" ]
-
 # chmod
 openssl genpkey -algorithm RSA -out /etc/lacme/test6.key
 cat >"/etc/lacme/lacme-certs.conf.d/test6.conf" <<- EOF