fix: Restore detection of letsencrypt certificate file changes (#2326)

The `DYNAMIC_FILES` var was quote wrapped, treating all filepaths to create checksums for as a single string that would be ignored instead of processed individually.

Removed the quotes, and changed the for loop to an array which accomplishes the same goal.


* fix: Prevent unnecessary change detection event

`acme.json` change would extract new cert files, which would then be hashed after restarting services and considered a change event, running through the logic again and restarting services once more when that was not required.

The checksum entries for those cert files are now replaced with new entries containing updated checksum hashes, after `acme.json` extraction.

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## `v10.3.0`
 
+**WARNING:** This release had a small regression affecting the detection of changes for certificates provisioned in `/etc/letsencrypt` with the config ENV `SSL_TYPE=letsencrypt`, unless you use Traefik's `acme.json`. If you rely on this functionality to restart Postfix and Dovecot when updating your cert files, this will not work and it is advised to upgrade to `v10.4.0` or newer prior to renewal of your certificates.
+
 - **[fix]** The Dovecot `userdb` will now additionally create "dummy" accounts for basic alias maps (_alias maps to a single real account managed by Dovecot, relaying to external providers aren't affected_) when `ENABLE_QUOTAS=1` (default) as a workaround for Postfix `quota-status` plugin querying Dovecot with inbound mail for a user, which Postfix uses to reject mail if quota has been exceeded (_to avoid risk of blacklisting from spammers abusing backscatter_) [#2248](https://github.com/docker-mailserver/docker-mailserver/pull/2248)
     - **NOTE:** If using aliases that map to another alias or multiple addresses, _this remains a risk_.
 - **[fix]** `setup email list` command will no longer attempt to query Dovecot quota status when `ENABLE_QUOTAS` is disabled [#2264](https://github.com/docker-mailserver/docker-mailserver/pull/2264)

target/scripts/check-for-changes.sh

@@ -70,34 +70,40 @@ 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:
+    # `acme.json` is only relevant to Traefik, and is where it stores the certificates it manages.
-    for FILE in ${CHANGED}
+    # When a change is detected it's assumed to be a possible cert renewal that needs to be
-    do
+    # extracted for `docker-mailserver` services to adjust to.
-      case "${FILE}" in
+    if [[ ${CHANGED} =~ /etc/letsencrypt/acme.json ]]
-        # This file is only relevant to Traefik, and is where it stores the certificates
+    then
-        # 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" )
       _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.
+
+      # This breaks early as we only need the first successful extraction.
+      # For more details see the `SSL_TYPE=letsencrypt` case handling in `setup-stack.sh`.
+      #
       # 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
         _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
+        if _extract_certs_from_acme "${CERT_DOMAIN}"
-        # are only intended for the `acme.json` file..?
+        then
-        * )
+          # Prevent an unnecessary change detection from the newly extracted cert files by updating their hashes in advance:
-          _notify 'warn' "No certificate found in '${FILE}'"
+          CERT_DOMAIN=$(_strip_wildcard_prefix "${CERT_DOMAIN}")
-          ;;
+          ACME_CERT_DIR="/etc/letsencrypt/live/${CERT_DOMAIN}"
 
-      esac
+          sed -i "\|${ACME_CERT_DIR}|d" "${CHKSUM_FILE}.new"
+          sha512sum "${ACME_CERT_DIR}"/*.pem >> "${CHKSUM_FILE}.new"
+
+          break
+        fi
       done
+    fi
+
+    # If monitored certificate files in /etc/letsencrypt/live have changed and no `acme.json` is in use,
+    # They presently have no special handling other than to trigger a change that will restart Postfix/Dovecot.
+    # TODO: That should be all that's required, unless the cert file paths have also changed (Postfix/Dovecot configs then need to be updated).
 
     # regenerate postfix accounts
     [[ ${SMTP_ONLY} -ne 1 ]] && _create_accounts

target/scripts/helper-functions.sh

@@ -187,9 +187,9 @@ function _extract_certs_from_acme
 }
 export -f _extract_certs_from_acme
 
-# Remove the `*.` prefix if it exists
+# Remove the `*.` prefix if it exists, else returns the input value
 function _strip_wildcard_prefix {
-  [[ "${1}" == "*."* ]] && echo "${1:2}"
+  [[ "${1}" == "*."* ]] && echo "${1:2}" || echo "${1}"
 }
 
 # ? --------------------------------------------- Notifications
@@ -224,7 +224,8 @@ export -f _notify
 # shellcheck disable=SC2034
 CHKSUM_FILE=/tmp/docker-mailserver-config-chksum
 
-# Compute checksums of monitored files.
+# Compute checksums of monitored files,
+# returned output is lines of hashed content + filepath pairs.
 function _monitored_files_checksums
 {
   # If a wildcard path pattern (or an empty ENV) would yield an invalid path
@@ -232,14 +233,15 @@ function _monitored_files_checksums
   shopt -s nullglob
 
   # React to any cert changes within the following letsencrypt locations:
-  local DYNAMIC_FILES
+  local CERT_FILES=(
-  for FILE in /etc/letsencrypt/live/"${SSL_DOMAIN}"/*.pem \
+    /etc/letsencrypt/live/"${SSL_DOMAIN}"/*.pem
-              /etc/letsencrypt/live/"${HOSTNAME}"/*.pem   \
+    /etc/letsencrypt/live/"${HOSTNAME}"/*.pem
     /etc/letsencrypt/live/"${DOMAINNAME}"/*.pem
-  do
+  )
-    DYNAMIC_FILES="${DYNAMIC_FILES} ${FILE}"
-  done
 
+  # CERT_FILES should expand to separate paths, not a single string;
+  # otherwise fails to generate checksums for these file paths.
+  #shellcheck disable=SC2068
   (
     cd /tmp/docker-mailserver || exit 1
     exec sha512sum 2>/dev/null -- \
@@ -248,7 +250,7 @@ function _monitored_files_checksums
       postfix-aliases.cf \
       dovecot-quotas.cf \
       /etc/letsencrypt/acme.json \
-      "${DYNAMIC_FILES}"
+      ${CERT_FILES[@]}
   )
 }
 export -f _monitored_files_checksums

test/mail_changedetector.bats

@@ -49,12 +49,12 @@ function teardown_file() {
 @test "checking changedetector: can detect changes & between two containers using same config" {
   echo "" >> "$(private_config_path mail_changedetector_one)/postfix-accounts.cf"
   sleep 15
-  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail changedetector"
+  run docker exec mail_changedetector_one /bin/bash -c "supervisorctl tail -3000 changedetector"
   assert_output --partial "postfix: stopped"
   assert_output --partial "postfix: started"
   assert_output --partial "Change detected"
   assert_output --partial "Removed lock"
-  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl tail changedetector"
+  run docker exec mail_changedetector_two /bin/bash -c "supervisorctl tail -3000 changedetector"
   assert_output --partial "postfix: stopped"
   assert_output --partial "postfix: started"
   assert_output --partial "Change detected"