From af5e3d794fc2f83f6cc3b5ddff386dad5463707d Mon Sep 17 00:00:00 2001 From: Guilhem Moulin Date: Tue, 23 Feb 2021 00:20:32 +0100 Subject: Consolidate error messages. --- client | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'client') diff --git a/client b/client index fdef865..6438f6a 100755 --- a/client +++ b/client @@ -87,13 +87,13 @@ do { if (defined (my $extra = $2)) { my $h = eval { JSON::->new->decode($extra) }; if ($@ or !defined $h) { - print STDERR "WARN: Ignoring extra greeting data from accountd \"$extra\"\n"; + print STDERR "Warning: Ignoring extra greeting data from accountd \"$extra\"\n"; } else { print STDERR "Received extra greeting data from accountd: $extra\n" if $ENV{DEBUG}; ($JWK_thumbprint, $ALG, $KID) = @$h{qw/jwk-thumbprint alg kid/}; } } - my $jwk_str = $S->getline() // die "ERROR: No JWK from lacme-accountd\n"; + my $jwk_str = $S->getline() // die "Error: No JWK from lacme-accountd\n"; $JWK = JSON::->new->decode($jwk_str); $JWK_thumbprint //= encode_base64url(sha256(json()->encode($JWK))); # SHA-256 is hardcoded, see RFC 8555 sec. 8.1 $ALG //= "RS256"; @@ -210,7 +210,7 @@ sub acme2($$;$) { $payload = defined $payload ? encode_base64url(json()->encode($payload)) : ""; $S->printflush($protected, ".", $payload, "\r\n"); - my $sig = $S->getline() // die "ERROR: No response from lacme-accountd\n"; + my $sig = $S->getline() // die "Error: lost connection with lacme-accountd\n"; $sig =~ s/\r\n\z// or die; undef $NONCE; # consume the nonce @@ -249,7 +249,7 @@ sub acme_resource($%) { if ($r eq "newAccount" or ($r eq "revokeCert" and !defined $KID)) { # per RFC 8555 sec. 6.2 these requests MUST have a JWK - print STDERR "WARNING: lacme-accountd supplied an empty JWK; try removing 'keyid' ", + print STDERR "Warning: lacme-accountd supplied an empty JWK; try removing 'keyid' ", "setting from lacme-accountd.conf if the ACME resource request fails.\n" unless %$JWK; return acme2($uri, {jwk => $JWK}, \%payload); @@ -342,7 +342,7 @@ elsif ($COMMAND eq 'newOrder') { $fh->print($keyAuthorization); $fh->close() or die "close: $!"; } elsif ($! == EEXIST) { - print STDERR "WARNING: File exists: $challenge->{token}\n"; + print STDERR "Warning: File exists: $challenge->{token}\n"; } else { die "open($challenge->{token}): $!"; } -- cgit v1.2.3 From c612a7ff44995f4f9c39fa0fb68470d90c88decf Mon Sep 17 00:00:00 2001 From: Guilhem Moulin Date: Wed, 24 Feb 2021 21:01:12 +0100 Subject: lacme: Default mode for certificate(-chain) creation is 0644 minus umask restrictions. Also, always spawn the client with umask 0022 so a starting lacme(8) with a restrictive umask doesn't impede serving challenge response files. --- Changelog | 4 ++++ client | 3 ++- lacme | 1 + lacme.8.md | 3 ++- tests/cert-install | 45 +++++++++++++++++++++++++++++++-------------- 5 files changed, 40 insertions(+), 16 deletions(-) (limited to 'client') diff --git a/Changelog b/Changelog index 2a027f1..f7f11f6 100644 --- a/Changelog +++ b/Changelog @@ -3,6 +3,10 @@ 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: default mode for certificate(-chain) creation is 0644 minus + umask restrictions. Also, always spawn the client with umask 0022 so + a starting lacme(8) with a restrictive umask doesn't impede serving + challenge files. - 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/client b/client index 6438f6a..33189d3 100755 --- a/client +++ b/client @@ -338,7 +338,8 @@ elsif ($COMMAND eq 'newOrder') { my $keyAuthorization = $challenge->{token}.'.'.$JWK_thumbprint; # serve $keyAuthorization at http://$domain/.well-known/acme-challenge/$challenge->{token} - if (sysopen(my $fh, $challenge->{token}, O_CREAT|O_EXCL|O_WRONLY, 0644)) { + if (sysopen(my $fh, $challenge->{token}, O_CREAT|O_EXCL|O_WRONLY)) { + # note: the file is created mode 0666 minus umask restrictions $fh->print($keyAuthorization); $fh->close() or die "close: $!"; } elsif ($! == EEXIST) { diff --git a/lacme b/lacme index 2366830..9012890 100755 --- a/lacme +++ b/lacme @@ -581,6 +581,7 @@ sub acme_client($@) { set_FD_CLOEXEC($client, 1); my $rv = spawn({in => $args->{in}, out => $args->{out}, child => sub() { drop_privileges($conf->{user}, $conf->{group}, $args->{chdir} // '/'); + umask(0022) // die; set_FD_CLOEXEC($_, 0) foreach ($CONFFILE, $client); seek($CONFFILE, SEEK_SET, 0) or die "seek: $!"; $ENV{DEBUG} = $OPTS{debug} // 0; diff --git a/lacme.8.md b/lacme.8.md index c39f51c..7f6558e 100644 --- a/lacme.8.md +++ b/lacme.8.md @@ -429,7 +429,8 @@ Valid settings are: *chmod* : An optional octal mode to chmod the issued *certificate* and - *certificate-chain* to. + *certificate-chain* to. By default the files are created with mode + 0644 minus umask restrictions. *notify* diff --git a/tests/cert-install b/tests/cert-install index 5d8a239..c49a294 100644 --- a/tests/cert-install +++ b/tests/cert-install @@ -149,40 +149,57 @@ st="$(stat -c "%U:%G %#a" /etc/lacme/test5.pem)" st="$(stat -c "%U:%G %#a" /etc/lacme/test5.crt)" [ "$st" = "nobody:nogroup 0644" ] -# chmod +# umask restrictions (also test empty values) openssl genpkey -algorithm RSA -out /etc/lacme/test6.key cat >"/etc/lacme/lacme-certs.conf.d/test6.conf" <<- EOF [test6] certificate-key = /etc/lacme/test6.key - certificate = /etc/lacme/test6.pem certificate-chain = /etc/lacme/test6.crt - chmod = 0400 + certificate = + chmod = + chown = subject = $subject EOF -lacme newOrder test6 2>"$STDERR" || fail newOrder test6 -st="$(stat -c "%U:%G %#a" /etc/lacme/test6.pem)" -[ "$st" = "root:root 0400" ] +( umask 0077 && lacme newOrder test6 2>"$STDERR" || fail newOrder test6 ) +! test -e /etc/lacme/test6.pem st="$(stat -c "%U:%G %#a" /etc/lacme/test6.crt)" -[ "$st" = "root:root 0400" ] +[ "$st" = "root:root 0600" ] -# post-issuance notification +# chmod openssl genpkey -algorithm RSA -out /etc/lacme/test7.key cat >"/etc/lacme/lacme-certs.conf.d/test7.conf" <<- EOF [test7] certificate-key = /etc/lacme/test7.key + certificate = /etc/lacme/test7.pem certificate-chain = /etc/lacme/test7.crt + chmod = 0400 subject = $subject - notify = touch /tmp/test7.notify EOF lacme newOrder test7 2>"$STDERR" || fail newOrder test7 -grepstderr -Fxq "Running notification command \`touch /tmp/test7.notify\`" -test -e /tmp/test7.notify +st="$(stat -c "%U:%G %#a" /etc/lacme/test7.pem)" +[ "$st" = "root:root 0400" ] +st="$(stat -c "%U:%G %#a" /etc/lacme/test7.crt)" +[ "$st" = "root:root 0400" ] -rm -f /tmp/test7.notify -lacme newOrder test7 2>"$STDERR" || fail newOrder test7 +# post-issuance notification +openssl genpkey -algorithm RSA -out /etc/lacme/test8.key +cat >"/etc/lacme/lacme-certs.conf.d/test8.conf" <<- EOF + [test8] + certificate-key = /etc/lacme/test8.key + certificate-chain = /etc/lacme/test8.crt + subject = $subject + notify = touch /tmp/test8.notify +EOF + +lacme newOrder test8 2>"$STDERR" || fail newOrder test8 +grepstderr -Fxq "Running notification command \`touch /tmp/test8.notify\`" +test -e /tmp/test8.notify + +rm -f /tmp/test8.notify +lacme newOrder test8 2>"$STDERR" || fail newOrder test8 ngrepstderr -Fq "Running notification command" -! test -e /tmp/test7.notify +! test -e /tmp/test8.notify # vim: set filetype=sh : -- cgit v1.2.3 From 9a8f705eddd18ccc9a24fe0e7efe6b5a87b2be09 Mon Sep 17 00:00:00 2001 From: Guilhem Moulin Date: Thu, 25 Feb 2021 01:41:59 +0100 Subject: lacme: pass a temporary JSON file with the client configuration to the internal client. So it doesn't have to parse the INI file again. Also, while lacme.conf is world-readable by default, one might restrict permissions and add private information in there, not realizing that everything, including comments, will be readable by the client. --- Changelog | 2 ++ client | 8 +------- lacme | 26 ++++++++++++++++---------- tests/drop-privileges | 4 ++-- 4 files changed, 21 insertions(+), 19 deletions(-) (limited to 'client') diff --git a/Changelog b/Changelog index d63c754..8bf0721 100644 --- a/Changelog +++ b/Changelog @@ -11,6 +11,8 @@ lacme (0.8.1) upstream; 'chmod'. + lacme: split certificates using Net::SSLeay::PEM_* instead of calling openssl. + + lacme: pass a temporary JSON file with the client configuration to + the internal client, so it doesn't have to parse the INI file again. - 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/client b/client index 33189d3..8439ddb 100755 --- a/client +++ b/client @@ -56,8 +56,6 @@ use Date::Parse (); use LWP::UserAgent (); use JSON (); -use Config::Tiny (); - # Clean up PATH $ENV{PATH} = join ':', qw{/usr/bin /bin}; delete @ENV{qw/IFS CDPATH ENV BASH_ENV/}; @@ -107,11 +105,7 @@ do { my $CONFIG = do { my $conf = do { local $/ = undef; <$CONFFILE> }; - close $CONFFILE or die "close: $!"; - my $h = Config::Tiny::->read_string($conf) or die Config::Tiny::->errstr()."\n"; - $h->{_} //= {}; - $h->{client}->{$_} //= $h->{_}->{$_} foreach keys %{$h->{_}}; # add defaults - $h->{client}; + JSON::->new->decode($conf); }; my $UA = do { my %args = %$CONFIG; diff --git a/lacme b/lacme index 13c2ef5..d7dac54 100755 --- a/lacme +++ b/lacme @@ -37,13 +37,14 @@ use Socket 1.95 qw/AF_UNIX AF_INET AF_INET6 PF_UNIX PF_INET PF_INET6 PF_UNSPEC use Config::Tiny (); use Date::Parse (); +use JSON (); use Net::SSLeay 1.46 (); # Clean up PATH $ENV{PATH} = join ':', qw{/usr/bin /bin}; delete @ENV{qw/IFS CDPATH ENV BASH_ENV/}; -my ($COMMAND, %OPTS, $CONFFILE, $CONFIG, @CLEANUP); +my ($COMMAND, %OPTS, $CONFIG, @CLEANUP); $SIG{$_} = sub() { exit 1 } foreach qw/INT TERM/; # run the END block upon SIGINT/SIGTERM @@ -99,14 +100,12 @@ sub spec_expand($) { return $str; } -sub set_FD_CLOEXEC($$); my $CONFFILENAME = spec_expand($OPTS{config} // "%E/lacme/$NAME.conf"); do { print STDERR "Using configuration file: $CONFFILENAME\n" if $OPTS{debug}; - open $CONFFILE, '<', $CONFFILENAME or die "Can't open $CONFFILENAME: $!\n"; - my $conf = do { local $/ = undef; <$CONFFILE> }; - # don't close $CONFFILE so we can pass it to the client - set_FD_CLOEXEC($CONFFILE, 1); + open my $fh, '<', $CONFFILENAME or die "Can't open $CONFFILENAME: $!\n"; + my $conf = do { local $/ = undef; <$fh> }; + close $fh or die "close: $!"; my $h = Config::Tiny::->read_string($conf) or die Config::Tiny::->errstr()."\n"; my $defaults = delete $h->{_} // {}; @@ -573,19 +572,26 @@ sub acme_client($@) { die "connect: $!"; } } + set_FD_CLOEXEC($client, 1); + + my $client_config; + do { + my $tmp = File::Temp::->new(TMPDIR => 1, TEMPLATE => "lacme-client.conf.json-XXXXXXXXXX", UNLINK => 1) // die; + print $tmp JSON::->new->encode($conf); + open $client_config, "<", $tmp->filename() or die "open: $!"; + }; # use execve(2) rather than a Perl pseudo-process to ensure that the # child doesn't have access to the parent's memory my ($cmd, @args2) = split(/\s+/, $conf->{command}) or die "Empty client command\n"; - my @fileno = map { fileno($_) =~ /^(\d+)$/ ? $1 : die } ($CONFFILE, $client); # untaint fileno - set_FD_CLOEXEC($client, 1); + my @fileno = map { fileno($_) =~ /^(\d+)$/ ? $1 : die } ($client_config, $client); # untaint fileno my $rv = spawn({in => $args->{in}, out => $args->{out}, child => sub() { drop_privileges($conf->{user}, $conf->{group}, $args->{chdir} // '/'); umask(0022) // die; - set_FD_CLOEXEC($_, 0) foreach ($CONFFILE, $client); - seek($CONFFILE, SEEK_SET, 0) or die "seek: $!"; + set_FD_CLOEXEC($_, 0) for ($client_config, $client); $ENV{DEBUG} = $OPTS{debug} // 0; }}, $cmd, @args2, $COMMAND, @fileno, @args); + close $client_config or die "close: $!\n"; if (defined $cleanup) { @CLEANUP = grep { $_ ne $cleanup } @CLEANUP; diff --git a/tests/drop-privileges b/tests/drop-privileges index fd432d9..8deb8f1 100644 --- a/tests/drop-privileges +++ b/tests/drop-privileges @@ -123,8 +123,8 @@ check_client() { grep -Exq "[0-9]+ 0700 $UID:$GID socket:\[[0-9]+\]" "$prefix/fd" || return 1 sed -ri '0,\#^[0-9]+ .* socket:\[[0-9]+\]$# {//d}' "$prefix/fd" - grep -Exq "[0-9]+ 0500 $UID:$GID /etc/lacme/lacme\.conf" "$prefix/fd" || return 1 - sed -ri '0,\#^[0-9]+ .* /etc/lacme/lacme\.conf$# {//d}' "$prefix/fd" + grep -Eq "^[0-9]+ 0500 $UID:$GID /tmp/lacme-client.conf\.json-" "$prefix/fd" || return 1 + sed -ri '0,\#^[0-9]+ .* /tmp/lacme-client.conf\.json-# {//d}' "$prefix/fd" ! test -s "$prefix/fd" || return 1 } check_webserver() { -- cgit v1.2.3 From b3af3526b293f396da02a6276ea86ca17dcd2d03 Mon Sep 17 00:00:00 2001 From: Guilhem Moulin Date: Wed, 25 Jan 2023 03:23:51 +0100 Subject: Prepare new release v0.8.1. --- Changelog | 2 +- client | 2 +- lacme | 2 +- lacme-accountd | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) (limited to 'client') diff --git a/Changelog b/Changelog index 4aa9f4f..fc658bf 100644 --- a/Changelog +++ b/Changelog @@ -28,7 +28,7 @@ lacme (0.8.1) upstream; - Makefile: replace '$(dir $@)' with '$(@D)'. - Test suite: Adjust against current Let's Encrypt staging environment. - -- Guilhem Moulin Mon, 22 Feb 2021 12:04:28 +0100 + -- Guilhem Moulin Wed, 25 Jan 2023 03:23:51 +0100 lacme (0.8.0) upstream; diff --git a/client b/client index 8439ddb..3cda821 100755 --- a/client +++ b/client @@ -43,7 +43,7 @@ use warnings; # instance own by another user and created with umask 0177) is not a # problem since SOCKET_FD can be bound as root prior to the execve(2). -our $VERSION = '0.8.0'; +our $VERSION = '0.8.1'; my $PROTOCOL_VERSION = 1; my $NAME = 'lacme-client'; diff --git a/lacme b/lacme index d7dac54..21a184c 100755 --- a/lacme +++ b/lacme @@ -22,7 +22,7 @@ use v5.14.2; use strict; use warnings; -our $VERSION = '0.8.0'; +our $VERSION = '0.8.1'; my $NAME = 'lacme'; use Errno 'EINTR'; diff --git a/lacme-accountd b/lacme-accountd index 98c11ad..a9f5469 100755 --- a/lacme-accountd +++ b/lacme-accountd @@ -23,7 +23,7 @@ use v5.14.2; use strict; use warnings; -our $VERSION = '0.8.0'; +our $VERSION = '0.8.1'; my $PROTOCOL_VERSION = 1; my $NAME = 'lacme-accountd'; -- cgit v1.2.3