Recent cleanups and test suite

.github/workflows/docker-publish.yml

@@ -20,16 +20,32 @@ env:
   IMAGE_NAME: ${{ github.repository }}
 
 jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      - name: Install Perl dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y --no-install-recommends \
+            libmojolicious-perl \
+            libcrypt-bcrypt-perl \
+            libtest-without-module-perl \
+            curl
+
+      - name: Run test harness
+        run: make -C t test
+
   build:
+    needs: test
     strategy:
       fail-fast: false
       matrix:
         platform:
           - docker: linux/amd64
             s6arch: x86_64
-          - docker: linux/arm/v7
-            qemu: arm
-            s6arch: arm
           - docker: linux/arm64/v8
             qemu: arm64
             s6arch: aarch64

acmeproxy.pl

@@ -36,13 +36,10 @@
 use Cwd;
 use strict;
 
-my $has_bcrypt = eval {
-	require Crypt::Bcrypt;
-	Crypt::Bcrypt->import();
-	1;
-};
+my $has_bcrypt = eval { require Crypt::Bcrypt; 1 };
 
-die("$0: please install curl.\n") unless (-x `/usr/bin/which curl` =~ s/[\r\n]//r);
+chomp(my $curl_path = qx{command -v curl 2>/dev/null});
+die("$0: please install curl.\n") unless -x $curl_path;
 
 # acme.sh uses this log format so we're sort of stuck with it
 sub logg ($in) { say strftime("[%a %b %e %I:%M:%S %p %Z %Y] ", localtime()) . $in };
@@ -51,33 +48,22 @@
 logg('WARNING: acmeproxy.pl.conf is world-readable. Please chmod 0600 acmeproxy.pl.conf') if ((stat('acmeproxy.pl.conf'))[2] & 04);
 my $config = plugin 'Config' => {file => cwd().'/acmeproxy.pl.conf', format => 'perl'};
 
-# Backwards compatibility checks/updates
-if (!exists($config->{acmesh_extra_params_install})) {
-  $config->{acmesh_extra_params_install} = [];
-}
-if (!exists($config->{acmesh_extra_params_install_cert})) {
-  $config->{acmesh_extra_params_install_cert} = [];
-}
-if (!exists($config->{acmesh_extra_params_issue})) {
-  $config->{acmesh_extra_params_issue} = [];
-}
-if ($has_bcrypt) {
-  foreach my $auth (@{$config->{auth}}) {
-    if (exists($auth->{pass})) {
-      logg "One or more users are defined with plaintext passwords. You should convert them to bcrypt hashes!";
-      last;
-    }
-  }
-} else {
-  foreach my $auth (@{$config->{auth}}) {
-    if (exists($auth->{hash})) {
-      die("One or more users are defined with bcrypt hashes, but Crypt::Bcrypt is not available. Either install Crypt::Bcrypt, or change these users to have a plaintext password!");
-    }
-  }
-}
-if (!exists($config->{keypair_directory})) {
-  $config->{keypair_directory} = $acme_home;
+# Backwards compatibility defaults
+$config->{acmesh_extra_params_install}      = [] unless exists $config->{acmesh_extra_params_install};
+$config->{acmesh_extra_params_install_cert} = [] unless exists $config->{acmesh_extra_params_install_cert};
+$config->{acmesh_extra_params_issue}        = [] unless exists $config->{acmesh_extra_params_issue};
+$config->{keypair_directory}                = $acme_home unless exists $config->{keypair_directory};
+
+# Validate auth entries against bcrypt availability
+my ($has_plaintext, $has_hash) = (0, 0);
+foreach my $auth (@{$config->{auth}}) {
+  $has_plaintext ||= exists $auth->{pass};
+  $has_hash      ||= exists $auth->{hash};
 }
+die("One or more users are defined with bcrypt hashes, but Crypt::Bcrypt is not available. Either install Crypt::Bcrypt, or change these users to have a plaintext password!\n")
+  if ($has_hash && !$has_bcrypt);
+logg "One or more users are defined with plaintext passwords. You should convert them to bcrypt hashes!"
+  if ($has_plaintext && $has_bcrypt);
 
 # Set environment variables from config
 foreach (keys %{$config->{env}}) { $ENV{$_} = $config->{env}->{$_}; }
@@ -128,21 +114,19 @@ sub handle_request {
 
 # We used acme.sh to generate our TLS certificate so its cron job should update our cert regularly
 # Check the TLS certificate file for changes every second and reload our app if it's been modified
-{
-  my $watcher;
-  my $cert_mtime = (stat("$acmeproxy_crt_file"))[9]; 
-  $watcher = Mojo::IOLoop->recurring(1 => sub {
-    if ((stat($acmeproxy_crt_file))[9] != $cert_mtime) {
-      $cert_mtime = (stat($acmeproxy_crt_file))[9];
-      logg "$acmeproxy_crt_file modified. Reloading";
-      exec($^X, $0, @ARGV) or logg "reload failed!"; # Just re-exec ourselves
-    }
-  });
-}
+my $cert_mtime = (stat($acmeproxy_crt_file))[9];
+Mojo::IOLoop->recurring(1 => sub {
+  my $mtime = (stat($acmeproxy_crt_file))[9];
+  return if $mtime == $cert_mtime;
+  $cert_mtime = $mtime;
+  logg "$acmeproxy_crt_file modified. Reloading";
+  exec($^X, $0, @ARGV) or logg "reload failed!"; # Just re-exec ourselves
+});
 
 # Anchors aweigh!
 app->mode('production');
-app->start('daemon', '-l', "https://$config->{bind}?cert=$acmeproxy_crt_file&key=$acmeproxy_key_file");
+app->start('daemon', '-l', "https://$config->{bind}?cert=$acmeproxy_crt_file&key=$acmeproxy_key_file")
+  unless caller;
 
 # Add or remove a DNS record using the configured acme.sh DNS provider
 # Hijacks acme.sh to use it's dnsapi library.
@@ -153,15 +137,19 @@ ($action, $fqdn, $value)
   return { text => "invalid characters in value", status => 400} unless ($value =~ /^[\w_\.-]+$/);
   $fqdn =~ s/\.+$//; # Some acme.sh plugins add an additional . to the end of the hostname
 
-  my $shellcmd = '/usr/bin/env bash -c "' .
-    "source $acme_home/acme.sh >/dev/null 2>&1; " .     		                  # Load all bash functions from acme.sh
-    "source $acme_home/dnsapi/$config->{dns_provider}.sh; " .			            # source ~/.acme.sh/dns_cf.sh
-    $config->{dns_provider}.'_'.$action.' \"'.$fqdn.'\" '.'\"'.$value.'\";"';	# dns_cf_add "sub.domain.com" "value123456"
-  logg "executing: $shellcmd";
+  # Source acme.sh and the dnsapi provider, then call the provider's add/rm function.
+  # fqdn and value are passed as positional args ($1, $2) rather than interpolated into
+  # the shell string, so they can never be parsed as shell syntax.
+  my $func = $config->{dns_provider}.'_'.$action;
+  my $script = "source $acme_home/acme.sh >/dev/null 2>&1; " .
+               "source $acme_home/dnsapi/$config->{dns_provider}.sh; " .
+               '"$0" "$1" "$2"';
+  logg "executing: $func \"$fqdn\" \"$value\"";
 
   # acme.sh/dnslib/dns_acmeproxy.sh explicitly looks for the quotes around $value to determine success
   # other clients expect full JSON and fqdn needs to end with "."
-  return { text => "{\"fqdn\": \"$fqdn.\", \"value\": \"$value\"}", status => 200} unless (system("$shellcmd"));
+  return { text => "{\"fqdn\": \"$fqdn.\", \"value\": \"$value\"}", status => 200}
+    unless (system('/usr/bin/env', 'bash', '-c', $script, $func, $fqdn, $value));
   return { text => "failed. check acmeproxy.pl logs", status => 500};
 }
 
@@ -176,17 +164,15 @@ ($userpass, $fqdn)
   my ($user, $pass) = split(/:/, $userpass, 2);
 
   foreach my $auth (@{$config->{auth}}) {
-    my $auth_check = 0;
-    if (secure_compare($user, $auth->{user}) && $fqdn =~ /\.$auth->{host}\.?$/) {
-      if ($has_bcrypt && exists($auth->{hash})) {
-        $auth_check = Crypt::Bcrypt::bcrypt_check($pass, $auth->{hash});
-      } else {
-        $auth_check = secure_compare($pass, $auth->{pass});
-      }
-      if ($auth_check) {
-        logg "auth: $user successfully authenticated for $fqdn";
-        return 1;
-      }
+    next unless secure_compare($user, $auth->{user}) && $fqdn =~ /\.$auth->{host}\.?$/;
+
+    my $ok = ($has_bcrypt && exists $auth->{hash})
+      ? Crypt::Bcrypt::bcrypt_check($pass, $auth->{hash})
+      : secure_compare($pass, $auth->{pass});
+
+    if ($ok) {
+      logg "auth: $user successfully authenticated for $fqdn";
+      return 1;
     }
   }
 
@@ -198,7 +184,7 @@ ($userpass, $fqdn)
 sub acme_install {
   say "Installing acme.sh";
   my $extra_params_install = join(' ', @{$config->{acmesh_extra_params_install}});
-  system("curl https://raw.githubusercontent.com/acmesh-official/acme.sh/master/acme.sh | sh -s -- --install-online -m $config->{email} $extra_params_install") && die("ouldn't install acme.sh\n");
+  system("curl https://raw.githubusercontent.com/acmesh-official/acme.sh/master/acme.sh | sh -s -- --install-online -m $config->{email} $extra_params_install") && die("Couldn't install acme.sh\n");
   say "Completed";
 }
 
@@ -214,8 +200,8 @@ ($hn)
   die("Could not create TLS certificate for $hn") if ($ret != 0 && $ret >> 8 != 2);
 
   my $extra_params_install_cert = join(' ', @{$config->{acmesh_extra_params_install_cert}});
-	$ret = system("$acme_home/acme.sh --log --install-cert $extra_params_install_cert $domain_list " .
-                   "--key-file $acmeproxy_key_file --fullchain-file $acmeproxy_crt_file");
+  $ret = system("$acme_home/acme.sh --log --install-cert $extra_params_install_cert $domain_list " .
+                "--key-file $acmeproxy_key_file --fullchain-file $acmeproxy_crt_file");
   die("Could not install TLS certificate for $hn") if ($ret);
 }
 

t/00-compile.t

@@ -0,0 +1,20 @@
+use strict;
+use warnings;
+use Test::More;
+use FindBin;
+use lib "$FindBin::Bin/lib";
+use AcmeProxyTest qw(setup_testenv);
+
+setup_testenv();
+require "$FindBin::Bin/../acmeproxy.pl";
+AcmeProxyTest::silence_logg();
+
+ok(defined &main::check_auth, 'check_auth defined in main::');
+ok(defined &main::acme_cmd,   'acme_cmd defined in main::');
+ok(main::app()->isa('Mojolicious'), 'app is a Mojolicious instance');
+
+my @paths = sort map { $_->pattern->unparsed } @{main::app()->routes->children};
+is_deeply(\@paths, [sort '/*', '/cleanup', '/present'], 'expected routes registered')
+    or diag explain \@paths;
+
+done_testing();

t/10-auth.t

@@ -0,0 +1,92 @@
+use strict;
+use warnings;
+use Test::More;
+use FindBin;
+use lib "$FindBin::Bin/lib";
+use AcmeProxyTest qw(setup_testenv clear_log @LOG_LINES);
+
+setup_testenv();
+require "$FindBin::Bin/../acmeproxy.pl";
+AcmeProxyTest::silence_logg();
+
+# --- negative paths --------------------------------------------------------
+ok(!main::check_auth(undef, 'foo.bob.example.com'), 'undef userinfo rejected');
+ok(!main::check_auth('',    'foo.bob.example.com'), 'empty userinfo rejected');
+ok(!main::check_auth('mallory:x',  'foo.bob.example.com'), 'unknown user rejected');
+ok(!main::check_auth('bob:wrong',  'foo.bob.example.com'), 'wrong password rejected');
+
+# --- positive paths --------------------------------------------------------
+# acme.sh sends FQDNs like "_acme-challenge.<host>" — any subdomain of the
+# configured host satisfies the /\.HOST\.?$/ regex
+ok( main::check_auth('bob:dobbs', 'foo.bob.example.com'),
+    'subdomain match for bob');
+ok( main::check_auth('bob:dobbs', '_acme-challenge.bob.example.com'),
+    'acme-challenge prefix matches');
+ok( main::check_auth('bob:dobbs', '_acme-challenge.bob.example.com.'),
+    'trailing-dot FQDN still matches');
+
+# --- host scoping ----------------------------------------------------------
+ok(!main::check_auth('bob:dobbs',     'foo.alice.example.com'),
+    'bob cannot issue for alice host');
+ok(!main::check_auth('bob:dobbs',     'evil.com'),
+    'bob cannot issue for unrelated domain');
+ok(!main::check_auth('bob:dobbs',     'foobob.example.com'),
+    'host match requires preceding dot (no substring sneak)');
+ok( main::check_auth('alice:rabbit', 'foo.alice.example.com'),
+    'alice can issue for alice subdomain');
+ok(!main::check_auth('alice:rabbit', 'foo.bob.example.com'),
+    'alice cannot issue for bob subdomain');
+
+# --- multi-host single user -----------------------------------------------
+push @{ main::app()->config->{auth} },
+    { user => 'bob', pass => 'dobbs', host => 'subgenius.example.com' };
+ok( main::check_auth('bob:dobbs', 'foo.subgenius.example.com'),
+    'bob authorized for second host entry');
+ok( main::check_auth('bob:dobbs', 'foo.bob.example.com'),
+    'bob still authorized for first host entry');
+
+# --- audit log -------------------------------------------------------------
+clear_log();
+main::check_auth('bob:wrong', 'foo.bob.example.com');
+ok( (grep { /auth: Invalid credentials for user bob/ } @LOG_LINES),
+    'invalid creds logged for audit')
+    or diag explain \@LOG_LINES;
+
+clear_log();
+main::check_auth('bob:dobbs', 'foo.bob.example.com');
+ok( (grep { /auth: bob successfully authenticated/ } @LOG_LINES),
+    'successful auth logged for audit')
+    or diag explain \@LOG_LINES;
+
+clear_log();
+main::check_auth(undef, 'foo.bob.example.com');
+ok( (grep { /credentials not supplied/ } @LOG_LINES),
+    'missing creds logged for audit')
+    or diag explain \@LOG_LINES;
+
+# --- bcrypt (skipped when Crypt::Bcrypt unavailable) ----------------------
+SKIP: {
+    skip 'Crypt::Bcrypt not installed', 3
+        unless eval { require Crypt::Bcrypt; Crypt::Bcrypt->import('bcrypt'); 1 };
+
+    # Generate a real hash for password "carrot" at cost 4 (fast)
+    my $hash = Crypt::Bcrypt::bcrypt('carrot', '2b', 4, '0123456789012345');
+
+    # Add a charlie entry that has BOTH pass and hash. With Crypt::Bcrypt
+    # available, the hash path should win (per script's ternary at line 168).
+    push @{ main::app()->config->{auth} }, {
+        user => 'charlie',
+        pass => 'ignored-because-hash-wins',
+        hash => $hash,
+        host => 'charlie.example.com',
+    };
+
+    ok( main::check_auth('charlie:carrot', 'foo.charlie.example.com'),
+        'bcrypt: correct password authorizes');
+    ok(!main::check_auth('charlie:wrong',  'foo.charlie.example.com'),
+        'bcrypt: wrong password rejected');
+    ok(!main::check_auth('charlie:ignored-because-hash-wins', 'foo.charlie.example.com'),
+        'bcrypt: plaintext field ignored when hash is present');
+}
+
+done_testing();

t/20-acme_cmd.t

@@ -0,0 +1,73 @@
+use strict;
+use warnings;
+use Test::More;
+use FindBin;
+use lib "$FindBin::Bin/lib";
+use AcmeProxyTest qw(setup_testenv $TMPDIR);
+
+my $tmp = setup_testenv();
+require "$FindBin::Bin/../acmeproxy.pl";
+AcmeProxyTest::silence_logg();
+
+# --- fqdn validation -------------------------------------------------------
+my @metachars = (';', '$', '`', ' ', '|', '&', '<', '>', "\n", '"', "'", '(', ')');
+for my $c (@metachars) {
+    my $label = $c eq "\n" ? 'newline' : "'$c'";
+    my $res = main::acme_cmd('add', "foo${c}bar.example.com", 'tok');
+    is($res->{status}, 400, "fqdn with $label rejected");
+    like($res->{text}, qr/invalid characters in fqdn/, "fqdn $label error text");
+}
+
+# --- value validation ------------------------------------------------------
+for my $c (@metachars) {
+    my $label = $c eq "\n" ? 'newline' : "'$c'";
+    my $res = main::acme_cmd('add', 'foo.example.com', "tok${c}bad");
+    is($res->{status}, 400, "value with $label rejected");
+    like($res->{text}, qr/invalid characters in value/, "value $label error text");
+}
+
+# --- happy path ------------------------------------------------------------
+my $res = main::acme_cmd('add', '_acme-challenge.bob.example.com', 'token123');
+is($res->{status}, 200, 'valid add returns 200');
+like($res->{text}, qr/"fqdn":\s*"_acme-challenge\.bob\.example\.com\."/,
+    'response includes fqdn with trailing dot');
+like($res->{text}, qr/"value":\s*"token123"/,
+    'response includes value');
+
+# --- trailing-dot normalization -------------------------------------------
+$res = main::acme_cmd('add', '_acme-challenge.bob.example.com.', 'tok');
+like($res->{text}, qr/"fqdn":\s*"_acme-challenge\.bob\.example\.com\."/,
+    'trailing dot stripped then re-added');
+
+$res = main::acme_cmd('add', '_acme-challenge.bob.example.com...', 'tok');
+like($res->{text}, qr/"fqdn":\s*"_acme-challenge\.bob\.example\.com\."/,
+    'multiple trailing dots collapsed to one');
+
+# --- rm action -------------------------------------------------------------
+$res = main::acme_cmd('rm', '_acme-challenge.bob.example.com', 'token123');
+is($res->{status}, 200, 'rm returns 200');
+
+# --- failure path ----------------------------------------------------------
+{
+    local $ENV{ACMEPROXY_TEST_FAIL} = 1;
+    $res = main::acme_cmd('add', '_acme-challenge.bob.example.com', 'tok');
+    is($res->{status}, 500, 'dnsapi failure returns 500');
+    like($res->{text}, qr/check acmeproxy\.pl logs/, '500 error message text');
+}
+
+# --- call log inspection ---------------------------------------------------
+# The stub dns_test.sh appends "add|rm <fqdn> <value>" lines to calls.log.
+# After the happy-path runs above, the log should contain at least one add
+# and one rm.
+my $log_path = "$tmp/.acme.sh/calls.log";
+ok(-f $log_path, 'calls.log exists');
+my $content = do { open my $fh, '<', $log_path or die $!; local $/; <$fh> };
+like($content, qr/^add /m, 'add invocation recorded');
+like($content, qr/^rm /m,  'rm invocation recorded');
+
+# Arguments passed to the stub are positional ($1, $2) so they cannot
+# be parsed as shell syntax. Confirm the FQDN ended up as a literal arg.
+like($content, qr/_acme-challenge\.bob\.example\.com token123/,
+    'fqdn and value reach dnsapi as positional args');
+
+done_testing();