[PATCH proxmox-acme 0/3] acme: fix missing/diverging shim helpers, tighten missing-function check

[PATCH proxmox-acme 0/3] acme: fix missing/diverging shim helpers, tighten missing-function check

[Mathias Bartmann]

dns_oci.sh (Oracle Cloud DNS) cannot complete a DNS-01 challenge,
see bug #7851 for the full analysis. Patch 1 fixes the two shim helpers
behind it and adds a test running them through the shim.

While looking at why this was not caught at build time, it turned out
that check-missing-functions misses calls at the end of a command
substitution or pipe, and drops any line that also has an assignment.
With that fixed (patch 3) it finds a third missing helper, _url_replace
used by dns_yc.sh, added in patch 2 so the series passes at every
commit.

A minimal fix within the existing grep pipeline is possible, but it
still drops any line that also calls a known helper, so it would not
catch _url_replace, and it needs ten more false-positive entries for
variables after export, unset and read. The rewrite checks each called
name individually, so the expected list shrinks to the one real
upstream issue (dns_aws.sh: _error). Happy to send the minimal variant
instead if you prefer.

Patch 1 was verified end to end on PVE 9.2 with an OCI-hosted zone:
certificate ordered and installed on pveproxy. Patch 2 is a verbatim
copy of the upstream helper, but I could not test it against Yandex
Cloud.

Mathias Bartmann (3):
  fix #7851: acme: add missing _mktemp and restore _dbase64 default
  acme: add missing _url_replace function
  tests: rewrite check-missing-functions to catch calls in substitutions

 src/proxmox-acme                    | 15 +++++++-
 src/test/Makefile                   |  2 +-
 src/test/check-missing-functions    | 59 ++++++++++++++++++++++++-----
 src/test/missing-functions.expected |  8 +---
 src/test/test-shim-helpers          | 37 ++++++++++++++++++
 5 files changed, 102 insertions(+), 19 deletions(-)
 create mode 100755 src/test/test-shim-helpers

-- 
2.55.0

[PATCH proxmox-acme 1/3] fix #7851: acme: add missing _mktemp and restore _dbase64 default

[Mathias Bartmann]

The dns_oci.sh plugin cannot complete a DNS-01 challenge, because of two
helpers in the proxmox-acme shim that differ from upstream acme.sh:

_dbase64() only implements upstream's multiline branch and ignores its
argument. Upstream decodes with '-A' by default and only drops it when
called as '_dbase64 multiline'. dns_oci.sh calls it without argument to
decode the single-line base64 private key, so the key decodes to an
empty string ("Usage: _fingerprint privkey"). Restore the upstream
conditional; dns_cyon.sh, dns_nic.sh and dns_yc.sh pass 'multiline' and
keep their current behaviour.

_mktemp() is not defined at all. dns_oci.sh writes the key to a
temporary file to sign its requests, so every request goes out
unsigned, and OCI's 404 then surfaces as the misleading
"DNS Zone not found". dns_transip.sh uses _mktemp the same way.

Add a test that runs these helpers through the shim and compares them
with upstream semantics.

Signed-off-by: Mathias Bartmann <mathias.bartmann@gmail.com>
---
 src/proxmox-acme           | 11 ++++++++++-
 src/test/Makefile          |  2 +-
 src/test/test-shim-helpers | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100755 src/test/test-shim-helpers

diff --git a/src/proxmox-acme b/src/proxmox-acme
index 705f7df..00f4c06 100644
--- a/src/proxmox-acme
+++ b/src/proxmox-acme
@@ -15,8 +15,17 @@ _base64() {
   openssl base64 -e | tr -d '\r\n'
 }
 
+#Usage: multiline
 _dbase64() {
-  openssl base64 -d
+  if [ "$1" ]; then
+    openssl base64 -d
+  else
+    openssl base64 -d -A
+  fi
+}
+
+_mktemp() {
+  mktemp
 }
 
 # Usage: hashalg  [outputhex]
diff --git a/src/test/Makefile b/src/test/Makefile
index bfa8991..b6044c6 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,6 +1,6 @@
 
 .PHONY: test test-missing-functions
-test: verify-dnsapi-plugins-in-schema.pl.t verify-acme-sources-in-makefile.pl.t test-missing-functions
+test: verify-dnsapi-plugins-in-schema.pl.t verify-acme-sources-in-makefile.pl.t test-shim-helpers.t test-missing-functions
 
 %.t: %
 	./$<
diff --git a/src/test/test-shim-helpers b/src/test/test-shim-helpers
new file mode 100755
index 0000000..b625a53
--- /dev/null
+++ b/src/test/test-shim-helpers
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+# Check that helpers in src/proxmox-acme behave like their acme.sh upstream
+# counterparts, as the dnsapi plugins are synced unmodified from there.
+
+SHIM="bash ../proxmox-acme"
+FAILED=0
+
+fail() {
+    echo "FAIL: $1" >&2
+    FAILED=1
+}
+
+# a single base64 line longer than openssl's 64 character line limit, like
+# the base64-encoded private key dns_oci.sh decodes with a plain _dbase64
+PLAIN=$(head -c 1500 /dev/zero | tr '\0' 'x')
+SINGLE_LINE=$(printf '%s' "$PLAIN" | openssl base64 -e | tr -d '\n')
+WRAPPED=$(printf '%s' "$PLAIN" | openssl base64 -e)
+
+[ "$(printf '%s' "$SINGLE_LINE" | $SHIM _dbase64)" = "$PLAIN" ] ||
+    fail "_dbase64 without argument does not decode a single long line"
+
+[ "$(printf '%s\n' "$WRAPPED" | $SHIM _dbase64 multiline)" = "$PLAIN" ] ||
+    fail "_dbase64 multiline does not decode line-wrapped input"
+
+TMP_FILE=$($SHIM _mktemp)
+if [ -f "$TMP_FILE" ] && [ -w "$TMP_FILE" ]; then
+    rm -f "$TMP_FILE"
+else
+    fail "_mktemp does not create a writable temporary file"
+fi
+
+[ "$FAILED" -eq 0 ] || exit 1
+echo "OK: proxmox-acme helpers behave like upstream acme.sh."
-- 
2.55.0

[PATCH proxmox-acme 2/3] acme: add missing _url_replace function

[Mathias Bartmann]

dns_yc.sh (Yandex Cloud) pipes the header and payload of the JWT it
signs through _url_replace to turn base64 into base64url, but the
proxmox-acme shim does not define that helper. Add it as in upstream
acme.sh.

Signed-off-by: Mathias Bartmann <mathias.bartmann@gmail.com>
---
 src/proxmox-acme           | 4 ++++
 src/test/test-shim-helpers | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/src/proxmox-acme b/src/proxmox-acme
index 00f4c06..7d776ce 100644
--- a/src/proxmox-acme
+++ b/src/proxmox-acme
@@ -28,6 +28,10 @@ _mktemp() {
   mktemp
 }
 
+_url_replace() {
+  tr '/+' '_-' | tr -d '= '
+}
+
 # Usage: hashalg  [outputhex]
 # Output Base64-encoded digest
 _digest() {
diff --git a/src/test/test-shim-helpers b/src/test/test-shim-helpers
index b625a53..d4ec5a3 100755
--- a/src/test/test-shim-helpers
+++ b/src/test/test-shim-helpers
@@ -30,5 +30,8 @@ else
     fail "_mktemp does not create a writable temporary file"
 fi
 
+[ "$(printf '%s' 'ab/c+d==' | $SHIM _url_replace)" = 'ab_c-d' ] ||
+    fail "_url_replace does not convert base64 to base64url"
+
 [ "$FAILED" -eq 0 ] || exit 1
 echo "OK: proxmox-acme helpers behave like upstream acme.sh."
-- 
2.55.0

[PATCH proxmox-acme 3/3] tests: rewrite check-missing-functions to catch calls in substitutions

[Mathias Bartmann]

The check did not notice the missing _json_decode, _mktemp and
_url_replace helpers, for two reasons:

- it only matched a _name followed by a space, so calls at the end of
  a command substitution or pipe like "$(_mktemp)" or "| _json_decode)"
  were never seen
- it filtered whole lines, so any line that also contains an
  assignment, like "_tmp_file=$(_mktemp)", was dropped

Rewrite it in Perl to extract each _name in command position (line
start, after ( ` | ; & ! or a shell keyword, not followed by '='),
skipping arithmetic like "$((_cnt - 1))", and report the ones defined
neither in proxmox-acme nor in the calling plugin. Checked against the
tree before the respective fixes, this reports dns_active24.sh:
_json_decode, dns_oci.sh and dns_transip.sh: _mktemp, and dns_yc.sh:
_url_replace.

The false positives of the old check are gone, so the expected list
shrinks to dns_aws.sh calling _error, which upstream acme.sh does not
define either.

Signed-off-by: Mathias Bartmann <mathias.bartmann@gmail.com>
---
 src/test/check-missing-functions    | 59 ++++++++++++++++++++++++-----
 src/test/missing-functions.expected |  8 +---
 2 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/src/test/check-missing-functions b/src/test/check-missing-functions
index dfc32d3..adb93cd 100755
--- a/src/test/check-missing-functions
+++ b/src/test/check-missing-functions
@@ -1,14 +1,53 @@
-#!/bin/sh
+#!/usr/bin/perl
 
-set -e
+use strict;
+use warnings;
 
-# functions already in src/proxmox-acme
-PRESENT=$(awk 'BEGIN{ORS="\\W|";} /^_/{ gsub(/\(\) {/, ""); print $0}' \
-	../proxmox-acme | sed -r 's/\|$//')
+# Print every _helper a dnsapi plugin calls that is defined neither in
+# src/proxmox-acme nor in the plugin itself.
 
-# functions defined in all plugins
-LOCAL=$(awk 'BEGIN{ORS="\\W|";} /^_/{ gsub(/\(\) {/, ""); print $0}' \
-	../acme.sh/dnsapi/dns*.sh | sed -r 's/\|$//')
+my $shim_path = '../proxmox-acme';
+my $dnsapi_path = '../acme.sh/dnsapi';
 
-grep -P '(?<!["$])\b_[a-zA-Z0-9_-]+ ' ../acme.sh/dnsapi/dns_*sh | \
-	grep -Ev "$PRESENT|$LOCAL|\b_[a-zA-Z0-9_-]+=|^../acme.sh/dnsapi/.*sh: *#"
+die "cannot find '$shim_path'!\n" if !-f $shim_path;
+die "cannot find dnsapi path '$dnsapi_path'!\n" if !-d $dnsapi_path;
+
+sub defined_functions {
+    my ($path) = @_;
+
+    open(my $fh, '<', $path) or die "cannot open '$path' - $!\n";
+    my $functions = {};
+    while (my $line = <$fh>) {
+        $functions->{$1} = 1 if $line =~ /^\s*(_[A-Za-z0-9_]+)\s*\(\)/;
+    }
+    close($fh);
+
+    return $functions;
+}
+
+# a _word in command position: at line start, after ( ` | ; & ! or a shell
+# keyword, and not followed by '=' (an assignment) - this also covers calls
+# inside command substitutions like "$(_mktemp)" and pipes like "| _dbase64",
+# but not variables in arithmetic like "$((_cnt - 1))"
+my $call_re = qr/
+    (?: ^ | (?<!\()\((?!\() | [`|;&!] | \b(?:if|then|else|elif|do|while|until)\s )
+    \s* (_[A-Za-z0-9_]+) (?= [\s)`|;&] | $ )
+/x;
+
+my $shim_functions = defined_functions($shim_path);
+
+for my $plugin (sort glob("$dnsapi_path/dns_*.sh")) {
+    my $plugin_functions = defined_functions($plugin);
+
+    open(my $fh, '<', $plugin) or die "cannot open '$plugin' - $!\n";
+    my $reported = {};
+    while (my $line = <$fh>) {
+        next if $line =~ /^\s*#/;
+        while ($line =~ /$call_re/g) {
+            my $function = $1;
+            next if $shim_functions->{$function} || $plugin_functions->{$function};
+            print "$plugin: $function\n" if !$reported->{$function}++;
+        }
+    }
+    close($fh);
+}
diff --git a/src/test/missing-functions.expected b/src/test/missing-functions.expected
index b338a58..c8f8240 100644
--- a/src/test/missing-functions.expected
+++ b/src/test/missing-functions.expected
@@ -1,7 +1 @@
-../acme.sh/dnsapi/dns_artfiles.sh:  response="$(printf -- '%s' "$response" | sed '/_acme-challenge "'"$txtValue"'"/d')"
-../acme.sh/dnsapi/dns_aws.sh:      _error "invalid domain"
-../acme.sh/dnsapi/dns_cpanel.sh:  for _domain in $_domains; do
-../acme.sh/dnsapi/dns_cyon.sh:  printf "%s" "${_dns_entries}" | while read -r _hash _identifier; do
-../acme.sh/dnsapi/dns_mythic_beasts.sh:  export _H1 _H2
-../acme.sh/dnsapi/dns_openstack.sh:    for _rec in $_records; do
-../acme.sh/dnsapi/dns_selectel.sh:    for _one_id in $_record_id; do
+../acme.sh/dnsapi/dns_aws.sh: _error
-- 
2.55.0