From e807631a76c7cd7283f5120a02582234f6ca1e32 Mon Sep 17 00:00:00 2001 From: Brennan Kinney <5098581+polarathene@users.noreply.github.com> Date: Thu, 4 Nov 2021 09:28:40 +1300 Subject: [PATCH] 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 | 39 ++++++++++++++++ target/scripts/check-for-changes.sh | 40 ++++++++++++++--- target/scripts/helper-functions.sh | 70 +++++++++++++---------------- test/mail_ssl_letsencrypt.bats | 4 +- 4 files changed, 105 insertions(+), 48 deletions(-) create mode 100644 target/bin/acme_extract diff --git a/target/bin/acme_extract b/target/bin/acme_extract new file mode 100644 index 00000000..5b7afacf --- /dev/null +++ b/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='', help='Path to acme.json') +parser.add_argument('fqdn', metavar='', 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) diff --git a/target/scripts/check-for-changes.sh b/target/scripts/check-for-changes.sh index bb0adaf7..dc65ca09 100755 --- a/target/scripts/check-for-changes.sh +++ b/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 diff --git a/target/scripts/helper-functions.sh b/target/scripts/helper-functions.sh index 832e9533..628fe638 100755 --- a/target/scripts/helper-functions.sh +++ b/target/scripts/helper-functions.sh @@ -150,51 +150,43 @@ export -f _sanitize_ipv4_to_subnet_cidr function _extract_certs_from_acme { - local KEY - # shellcheck disable=SC2002 - 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}" ]] + local CERT_DOMAIN=${1} + if [[ -z ${CERT_DOMAIN} ]] then - mkdir -p "/etc/letsencrypt/live/${HOSTNAME}/" - - 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 + _notify 'err' "_extract_certs_from_acme | CERT_DOMAIN is empty" 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 diff --git a/test/mail_ssl_letsencrypt.bats b/test/mail_ssl_letsencrypt.bats index 36b3e8df..5550eed5 100644 --- a/test/mail_ssl_letsencrypt.bats +++ b/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 }