From cdd025133a306cd8d3e81aa832ac056119d65f3a Mon Sep 17 00:00:00 2001 From: Guilhem Moulin Date: Wed, 24 Feb 2021 20:03:44 +0100 Subject: 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. --- Changelog | 1 + lacme | 54 +++++++++++++++++++++++++++++------------------------- tests/cert-install | 34 ++++++++++++++-------------------- 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 -- cgit v1.2.3