challenge-directory now needs to be set to an *existing* directory.

Since lacme(8) spawns a builtin webserver by default the change doesn't
affect default configurations.

See https://bugs.debian.org/970800 for the rationale.

5 files changed, 66 insertions, 54 deletions

diff --git a/Changelog b/Changelog
index 34ace54..39249b4 100644
--- a/Changelog
+++ b/Changelog
@@ -1,5 +1,12 @@
 lacme (0.7.1) upstream;
 
+ * Breaking change: 'challenge-directory' now needs to be set to an
+   *existing* directory (writable by the lacme client user).  Since
+   lacme(8) spawns a builtin webserver by default the change doesn't
+   affect default configurations.
+   Thanks to Benjamin Tietz for the idea and initial patch.
+ * Breaking change: the 'iptables' option is now ignored unless the
+   builtin webserver is used.
  * Unprivileged user/group for the internal client resp. webserver are
    now configurable at install time.
  * lacme: new flag `--force`, which aliases to `--min-days=-1`, i.e.,
diff --git a/config/lacme.conf b/config/lacme.conf
index 2955984..4c7dc86 100644
--- a/config/lacme.conf
+++ b/config/lacme.conf
@@ -64,17 +64,14 @@
 #
 #listen = @@runstatedir@@/lacme-www.socket
 
-# Non-existent directory under which an external HTTP daemon is
-# configured to serve GET requests for challenge files under
-# "/.well-known/acme-challenge/" (for each virtual host requiring
-# authorization) as static files.
+# Directory under which an external HTTP daemon is configured to serve
+# GET requests for challenge files under "/.well-known/acme-challenge/"
+# (for each virtual host requiring authorization) as static files.
+# NOTE: the directory must exist and be writable by the lacme client
+# user.
 #
 #challenge-directory =
 
-# Do not symlink the challenge-directory, but copy the challenge-files
-# explictly.
-#hard-copy-challenge-directory = No
-
 # username to drop privileges to (setting both effective and real uid).
 # Skip privilege drop if the value is empty (not recommended).
 #
diff --git a/lacme b/lacme
index d7ae8ce..7ad7aa8 100755
--- a/lacme
+++ b/lacme
@@ -26,9 +26,8 @@ our $VERSION = '0.3';
 my $NAME = 'lacme';
 
 use Errno 'EINTR';
-use Fcntl qw/F_GETFD F_SETFD FD_CLOEXEC SEEK_SET/;
+use Fcntl qw/F_GETFD F_SETFD FD_CLOEXEC O_CREAT O_EXCL O_WRONLY SEEK_SET/;
 use File::Temp ();
-use File::Path 'remove_tree';
 use Getopt::Long qw/:config posix_default no_ignore_case gnu_getopt auto_version/;
 use List::Util 'first';
 use POSIX ();
@@ -105,7 +104,6 @@ do {
         webserver => {
             listen                => '@@runstatedir@@/lacme-www.socket',
             'challenge-directory' => undef,
-            'hard-copy-challenge-directory' => 'No',
             user                  => '@@lacme_www_user@@',
             group                 => '@@lacme_www_group@@',
             command               => '@@libexecdir@@/lacme/webserver',
@@ -258,15 +256,6 @@ sub set_FD_CLOEXEC($$) {
 # The temporary challenge directory is returned.
 #
 sub spawn_webserver() {
-    # create a temporary directory; give write access to the ACME client
-    # and read access to the webserver
-    my $tmpdir = File::Temp::->newdir(CLEANUP => 1, TMPDIR => 1) // die;
-    chmod 0755, $tmpdir or die "Can't chmod: $!";
-    if ((my $username = $CONFIG->{client}->{user}) ne '') {
-        my $uid = getpwnam($username) // die "Can't getpwnam($username): $!";
-        chown($uid, -1, $tmpdir) or die "Can't chown: $!";
-    }
-
     my $conf = $CONFIG->{webserver};
 
     # parse and pack addresses to listen to
@@ -286,35 +275,56 @@ sub spawn_webserver() {
         push @sockaddr, $sockaddr;
     }
 
-    # symlink the 'challenge-directory' configuration option to the
-    # temporary challenge directory (so an existing httpd can directly
-    # serve ACME challenge reponses).
+    # Use existing HTTPd to serve challenge files using 'challenge-directory'
+    # as document root
     if (defined (my $dir = $conf->{'challenge-directory'})) {
         print STDERR "[$$] Using existing webserver on $dir\n" if $OPTS{debug};
-        if (lc ($conf->{'hard-copy-challenge-directory'} // 'No') eq 'yes') {
-            mkdir $dir or die "Can't create directory $dir: $!";
-            $tmpdir = $dir;
-            push @CLEANUP, sub() {
-                my $error = undef;
-                remove_tree($dir, { safe => 1, error => \$error });
-                if ($error && @$error) {
-                    foreach my $e (@$error) {
-                        my ($file, $message) = %$e;
-                        my $msghead = $file?"Error removing $file in":"Error while removing";
-                        warn "$msghead challenge dir $dir: $message\n";
-                    }
+        # lacme(8) doesn't have the list of challenge files to delete on
+        # cleanup -- instead, we unlink all files and fails at
+        # initialization stage when the challenge directory is not empty
+
+        opendir my $dh, $dir or die "opendir($dir): $!\n";
+        while (readdir $dh) {
+            die "Error: Refusing to use non-empty challenge directory $dir\n"
+                unless $_ eq '.' or $_ eq '..';
+        }
+        closedir $dh or die "close: $!";
+        undef $dh;
+
+        # use a "lock file" (NFS-friendly) to avoid concurrent usages
+        my $lockfile = ".$NAME.lock";
+        sysopen(my $fh, "$dir/$lockfile", O_CREAT|O_EXCL|O_WRONLY, 0600)
+            or die "Can't create lockfile in challenge directory: $!";
+        print $fh $$, "\n";
+        close $fh or die "close: $!";
+        undef $fh;
+
+        push @CLEANUP, sub() {
+            if (opendir(my $dh, $dir)) {
+                my @files = grep { $_ ne '.' and $_ ne '..' and $_ ne $lockfile } readdir $dh;
+                closedir $dh or warn "close: $!";
+                push @files, $lockfile; # unlink $lockfile last
+                foreach (@files) {
+                    die unless /\A(.+)\z/; # untaint
+                    unlink "$dir/$1" or warn "unlink($dir/$1): $!";
                 }
+            } else {
+                warn "opendir($dir): $!\n";
             }
-        } else {
-            symlink $tmpdir, $dir or die "Can't symlink $dir -> $tmpdir: $!";
-            push @CLEANUP, sub() {
-                print STDERR "Unlinking $dir\n" if $OPTS{debug};
-                unlink $dir or warn "Warning: Can't unlink $dir: $!";
-            }
-        }
+        };
+        return $dir; # ignore 'listen' and 'iptables'
     }
-    elsif (!@sockaddr) {
-        die "'challenge-directory' option of section [webserver] is required when 'listen' is empty\n";
+
+    die "'challenge-directory' option is required in section [webserver] when 'listen' is empty\n"
+        unless @sockaddr;
+
+    # create a temporary directory; give write access to the ACME client
+    # and read access to the webserver
+    my $tmpdir = File::Temp::->newdir(CLEANUP => 1, TMPDIR => 1) // die;
+    chmod 0755, $tmpdir or die "Can't chmod: $!";
+    if ((my $username = $CONFIG->{client}->{user}) ne '') {
+        my $uid = getpwnam($username) // die "Can't getpwnam($username): $!";
+        chown($uid, -1, $tmpdir) or die "Can't chown: $!";
     }
 
     # create socket(s) and spawn webserver(s)
diff --git a/lacme.8.md b/lacme.8.md
index 404180c..76cdd0d 100644
--- a/lacme.8.md
+++ b/lacme.8.md
@@ -238,16 +238,13 @@ served during certificate issuance.
 
 *challenge-directory*
 
-:   Specify a non-existent directory under which an external HTTP daemon
-    is configured to serve `GET` requests for challenge files under
-    `/.well-known/acme-challenge/` (for each virtual host requiring
-    authorization) as static files.
-    This option is required when *listen* is empty.
-
-*hard-copy-challenge-directory*
+:   Directory under which an external HTTP daemon is configured to serve `GET`
+    requests for challenge files under `/.well-known/acme-challenge/` (for
+    each virtual host requiring authorization) as static files.
+    `lacme` _must_ exist beforehand, _must_ be empty, and be writable by the
+    lacme client user (by default @@lacme_client_user@@).
 
-:   Do not symlink the challenge-directory, but copy the challenge-files
-    explictly.
+    This option is required when *listen* is empty.
 
 *user*
 
@@ -275,6 +272,7 @@ served during certificate issuance.
 :   Whether to automatically install temporary [`iptables`(8)] rules to
     open the `ADDRESS[:PORT]` specified with *listen*.  The rules are
     automatically removed once `lacme` exits.
+    This option is ignored when *challenge-directory* is set.
     Default: `No`.
 
 `[accountd]` section
diff --git a/snippets/nginx.conf b/snippets/nginx.conf
index 6775489..af2e92e 100644
--- a/snippets/nginx.conf
+++ b/snippets/nginx.conf
@@ -13,6 +13,6 @@ location ^~ /.well-known/acme-challenge/ {
     ## lacme's configuration file
     # alias /var/www/acme-challenge/;
     # default_type application/jose+json;
-    # disable_symlinks on from=$document_root;
+    # disable_symlinks on;
     # autoindex off;
 }