refactor: acme.json extraction (#2274)

Split into scoped commits with messages if further details are needed, view those via the associated PR :)

**Commit Summary:**

**`check-for-changes.sh`**

- Prevent `SSL_DOMAIN` silently skipping when value has wildcard prefix `*.` (_at least this was known as a bugfix when originally committed in linked PR_).
- Improved inlined docs for maintainers.
- Additional logging for debugging.

**`helper-functions.sh:_extract_certs_from_acme`**:

- Fail if the input arg (_`$CERT_DOMAIN`, aka the FQDN_) provided for extraction is empty.
- Use `$CERT_DOMAIN` in place of `$HOSTNAME` and `$1` for a consistent value (_previously could mismatch, eg with `SSL_DOMAIN` defined_).
- The conditional is now only for handling extraction failure (_key or cert value is missing from extraction_).
- Log an actual warning or success (debug) based on outcome.
- Don't use `SSL_DOMAIN` with wildcard value for the `mkdir` letsencrypt directory name (_wildcard prefix `*.` is first stripped instead_).

**`acme_extract`** (_new python utility for `acme.json` handling_):

- Extracted out into a python script that can be treated as a utility in the `$PATH` like other helper scripts. It can now be used and optionally tested directly instead of via `helper-functions.sh`.
-Made compatible with Python 3, as Python 2 is EOL and no longer in newer versions of Debian.

target/bin/acme_extract

@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
+import argparse,json
+
+parser = argparse.ArgumentParser(description='Traefik acme.json key and cert extractor utility.')
+parser.add_argument('filepath', metavar='<filepath>', help='Path to acme.json')
+parser.add_argument('fqdn',     metavar='<FQDN>',     help="FQDN to match in a certificates 'main' or 'sans' field")
+
+# Only one of these options can be used at a time, `const` is the key value that will be queried:
+key_or_cert = parser.add_mutually_exclusive_group(required=True)
+key_or_cert.add_argument('--key',  dest='requested', action='store_const', const='key',         help='Output the key data to stdout')
+key_or_cert.add_argument('--cert', dest='requested', action='store_const', const='certificate', help='Output the cert data to stdout')
+
+args = parser.parse_args()
+
+def has_fqdn(domains, fqdn):
+    main = domains.get('main', '')
+    sans = domains.get('sans', [])
+    return main == fqdn or fqdn in sans
+
+# Searches the acme.json data for the target FQDN,
+# upon a match returns the requested key or cert:
+def retrieve_data():
+    with open(args.filepath) as json_file:
+        acme_data = json.load(json_file)
+        for key, value in acme_data.items():
+            try:
+                certs = value['Certificates'] or []
+                for cert in certs:
+                    if has_fqdn(cert['domain'], args.fqdn):
+                        return cert[args.requested]
+            # One of the expected keys is missing.. return an empty result
+            # Certificates: [{domain: [main, sans], key, certificate}]
+            except KeyError:
+                return None
+
+# No match == 'None', we convert to empty string for
+# existing error handling by `helper-functions.sh`:
+result = retrieve_data() or ''
+print(result)

target/scripts/check-for-changes.sh

@@ -3,7 +3,12 @@
 # shellcheck source=./helper-functions.sh
 . /usr/local/bin/helper-functions.sh
 
-LOG_DATE=$(date +"%Y-%m-%d %H:%M:%S ")
+function _log_date
+{
+  date +"%Y-%m-%d %H:%M:%S"
+}
+
+LOG_DATE=$(_log_date)
 _notify 'task' "${LOG_DATE} Start check-for-changes script."
 
 # ? --------------------------------------------- Checks
@@ -32,12 +37,14 @@ _obtain_hostname_and_domainname
 
 PM_ADDRESS="${POSTMASTER_ADDRESS:=postmaster@${DOMAINNAME}}"
 _notify 'inf' "${LOG_DATE} Using postmaster address ${PM_ADDRESS}"
+
+# Change detection delayed during startup to avoid conflicting writes
 sleep 10
 
+_notify 'inf' "$(_log_date) check-for-changes is ready"
+
 while true
 do
-  LOG_DATE=$(date +"%Y-%m-%d %H:%M:%S ")
-
   # get chksum and check it, no need to lock config yet
   _monitored_files_checksums >"${CHKSUM_FILE}.new"
   cmp --silent -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new"
@@ -47,7 +54,7 @@ do
   # 2 – inaccessible or missing argument
   if [ $? -eq 1 ]
   then
-    _notify 'inf' "${LOG_DATE} Change detected"
+    _notify 'inf' "$(_log_date) Change detected"
     create_lock # Shared config safety lock
     CHANGED=$(grep -Fxvf "${CHKSUM_FILE}" "${CHKSUM_FILE}.new" | sed 's/^[^ ]\+  //')
 
@@ -61,23 +68,39 @@ do
     # Also note that changes are performed in place and are not atomic
     # We should fix that and write to temporary files, stop, swap and start
 
+    # TODO: Consider refactoring this:
     for FILE in ${CHANGED}
     do
       case "${FILE}" in
+        # This file is only relevant to Traefik, and is where it stores the certificates
+        # it manages. When a change is detected it's assumed to be a possible cert renewal
+        # that needs to be extracted for `docker-mailserver` to switch to.
         "/etc/letsencrypt/acme.json" )
-          for CERTDOMAIN in ${SSL_DOMAIN} ${HOSTNAME} ${DOMAINNAME}
+          _notify 'inf' "'/etc/letsencrypt/acme.json' has changed, extracting certs.."
+          # This breaks early as we only need the first successful extraction. For more details see `setup-stack.sh` `SSL_TYPE=letsencrypt` case handling.
+          # NOTE: HOSTNAME is set via `helper-functions.sh`, it is not the original system HOSTNAME ENV anymore.
+          # TODO: SSL_DOMAIN is Traefik specific, it no longer seems relevant and should be considered for removal.
+          FQDN_LIST=("${SSL_DOMAIN}" "${HOSTNAME}" "${DOMAINNAME}")
+          for CERT_DOMAIN in "${FQDN_LIST[@]}"
           do
-            _extract_certs_from_acme "${CERTDOMAIN}" && break
+            _notify 'inf' "Attempting to extract for '${CERT_DOMAIN}'"
+            _extract_certs_from_acme "${CERT_DOMAIN}" && break
           done
           ;;
 
+        # This seems like an invalid warning, as if the whole loop and case statement
+        # are only intended for the `acme.json` file..?
         * )
-          _notify 'warn' 'File not found for certificate in check_for_changes.sh'
+          _notify 'warn' "No certificate found in '${FILE}'"
           ;;
 
       esac
     done
 
+    # WARNING: This block of duplicate code is already out of sync
+    # It appears to unneccesarily run, even if the related entry in the CHKSUM_FILE
+    # has not changed?
+    #
     # regenerate postix aliases
     echo "root: ${PM_ADDRESS}" >/etc/aliases
     if [[ -f /tmp/docker-mailserver/postfix-aliases.cf ]]
@@ -219,12 +242,15 @@ s/$/ regexp:\/etc\/postfix\/regexp/
       chown -R 5000:5000 /var/mail
     fi
 
+    _notify 'inf' "Restarting services due to detected changes.."
+
     supervisorctl restart postfix
 
     # prevent restart of dovecot when smtp_only=1
     [[ ${SMTP_ONLY} -ne 1 ]] && supervisorctl restart dovecot
 
     remove_lock
+    _notify 'inf' "$(_log_date) Completed handling of detected change"
   fi
 
   # mark changes as applied

target/scripts/helper-functions.sh

@@ -150,51 +150,43 @@ export -f _sanitize_ipv4_to_subnet_cidr
 
 function _extract_certs_from_acme
 {
-  local KEY
+  local CERT_DOMAIN=${1}
-  # shellcheck disable=SC2002
+  if [[ -z ${CERT_DOMAIN} ]]
-  KEY=$(cat /etc/letsencrypt/acme.json | python -c "
-import sys,json
-acme = json.load(sys.stdin)
-for key, value in acme.items():
-    certs = value['Certificates']
-    if certs is not None:
-        for cert in certs:
-            if 'domain' in cert and 'key' in cert:
-                if 'main' in cert['domain'] and cert['domain']['main'] == '${1}' or 'sans' in cert['domain'] and '${1}' in cert['domain']['sans']:
-                    print cert['key']
-                    break
-")
-
-  local CERT
-  # shellcheck disable=SC2002
-  CERT=$(cat /etc/letsencrypt/acme.json | python -c "
-import sys,json
-acme = json.load(sys.stdin)
-for key, value in acme.items():
-    certs = value['Certificates']
-    if certs is not None:
-        for cert in certs:
-            if 'domain' in cert and 'certificate' in cert:
-                if 'main' in cert['domain'] and cert['domain']['main'] == '${1}' or 'sans' in cert['domain'] and '${1}' in cert['domain']['sans']:
-                    print cert['certificate']
-                    break
-")
-
-  if [[ -n "${KEY}${CERT}" ]]
   then
-    mkdir -p "/etc/letsencrypt/live/${HOSTNAME}/"
+    _notify 'err' "_extract_certs_from_acme | CERT_DOMAIN is empty"
-
-    echo "${KEY}" | base64 -d >/etc/letsencrypt/live/"${HOSTNAME}"/key.pem || exit 1
-    echo "${CERT}" | base64 -d >/etc/letsencrypt/live/"${HOSTNAME}"/fullchain.pem || exit 1
-    _notify 'inf' "Cert found in /etc/letsencrypt/acme.json for ${1}"
-
-    return 0
-  else
     return 1
   fi
+
+  local KEY CERT
+  KEY=$(acme_extract /etc/letsencrypt/acme.json "${CERT_DOMAIN}" --key)
+  CERT=$(acme_extract /etc/letsencrypt/acme.json "${CERT_DOMAIN}" --cert)
+
+  if [[ -z ${KEY} ]] || [[ -z ${CERT} ]]
+  then
+    _notify 'warn' "_extract_certs_from_acme | Unable to find key & cert for '${CERT_DOMAIN}' in '/etc/letsencrypt/acme.json'"
+    return 1
+  fi
+
+  # Currently we advise SSL_DOMAIN for wildcard support using a `*.example.com` value,
+  # The filepath however should be `example.com`, avoiding the wildcard part:
+  if [[ ${SSL_DOMAIN} == "${CERT_DOMAIN}" ]]
+  then
+    CERT_DOMAIN=$(_strip_wildcard_prefix "${SSL_DOMAIN}")
+  fi
+
+  mkdir -p "/etc/letsencrypt/live/${CERT_DOMAIN}/"
+  echo "${KEY}" | base64 -d > "/etc/letsencrypt/live/${CERT_DOMAIN}/key.pem" || exit 1
+  echo "${CERT}" | base64 -d > "/etc/letsencrypt/live/${CERT_DOMAIN}/fullchain.pem" || exit 1
+
+  _notify 'inf' "_extract_certs_from_acme | Certificate successfully extracted for '${CERT_DOMAIN}'"
 }
 export -f _extract_certs_from_acme
 
+# Remove the `*.` prefix if it exists
+function _strip_wildcard_prefix {
+  [[ "${1}" == "*."* ]] && echo "${1:2}"
+}
+
 # ? --------------------------------------------- Notifications
 
 function _notify

test/mail_ssl_letsencrypt.bats

@@ -117,11 +117,11 @@ function teardown_file() {
   assert_output --partial "postfix: started"
   assert_output --partial "Change detected"
 
-  run docker exec mail_lets_acme_json /bin/bash -c "cat /etc/letsencrypt/live/mail.my-domain.com/key.pem"
+  run docker exec mail_lets_acme_json /bin/bash -c "cat /etc/letsencrypt/live/example.com/key.pem"
   assert_output "$(cat "$(private_config_path mail_lets_acme_json)/letsencrypt/changed/key.pem")"
   assert_success
 
-  run docker exec mail_lets_acme_json /bin/bash -c "cat /etc/letsencrypt/live/mail.my-domain.com/fullchain.pem"
+  run docker exec mail_lets_acme_json /bin/bash -c "cat /etc/letsencrypt/live/example.com/fullchain.pem"
   assert_output "$(cat "$(private_config_path mail_lets_acme_json)/letsencrypt/changed/fullchain.pem")"
   assert_success
 }