Repository navigation
fix(smtp): hot-reload renewed TLS certificates without restart - #524
Conversation
The relay read its certificate once at startup, so renewals by Traefik/certbot were ignored and ports 465/587 kept serving the old certificate until it expired. Watch the mounted certificate files and swap renewed certificates into both servers via SMTPServer#updateSecureContext(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
driaug
left a comment
There was a problem hiding this comment.
Thanks for picking this up, hot-reloading certs is a nice improvement. A few issues before it can go in. The first three are the important ones: certs issued after startup are never picked up, the certbot mount advice doesn't work because of relative symlinks, and an uncaught error during the swap can crash the relay. Details are inline.
| }); | ||
|
|
||
| // Pick up certificate renewals without a restart | ||
| if (certs) { |
There was a problem hiding this comment.
Certificates issued after startup are never picked up. The watcher only starts when certs exists at boot. On a fresh Dokploy/Coolify deploy the container often starts before Traefik has finished the ACME challenge for SMTP_DOMAIN. getCertificates() then returns null, no watcher is started, the 465 server is never created and 587 runs without TLS. When Traefik writes the cert a minute later, nothing happens until someone restarts the container, which is the case this PR sets out to fix.
Suggestion: always start the watcher, and create (or start listening on) the 465 server the first time a valid key/cert pair appears.
There was a problem hiding this comment.
Fixed in 873849c. The watcher now always starts. If no certificate is found at startup, it watches every source, and the first valid pair creates the 465 server and adds TLS to 587. 587 needs no restart, because STARTTLS reads the server's secure context per connection.
Tested the Dokploy case: started with an acme.json that held only another domain, then added the SMTP_DOMAIN cert. 465 came up and both ports served the new cert.
|
|
||
| If no certificates are mounted, SMTP runs without TLS. | ||
|
|
||
| Renewed certificates are picked up automatically within a minute of the mounted files changing, without restarting the container. Mount files that your ACME client updates in place, or mount the containing directory: a single-file bind mount of a file that gets replaced (such as certbot's `live/` symlinks) keeps pointing at the old certificate. |
There was a problem hiding this comment.
This doesn't work for certbot as written. The files in certbot's live/<domain>/ are relative symlinks (../../archive/<domain>/...). If you mount only live/<domain> as /certs, those links point at nothing inside the container, existsSync fails and SMTP silently runs without TLS. The PEM example just above still shows single-file mounts, which this paragraph says go stale.
Suggestion: tell certbot users to mount the whole /etc/letsencrypt directory and set CERT_PATH to /etc/letsencrypt/live/<domain>, and update the example to match.
There was a problem hiding this comment.
Good catch on the relative symlinks. While checking this I found two more problems for certbot users:
- certbot creates
live/andarchive/as0700root (privkey*.pemis0600), and the image runs asUSER plunk(UID 1001). So even with/etc/letsencryptmounted, the relay can't read the files. - The pm2 config in
docker-entrypoint-nginx.shhard-codesCERT_PATH: '/certs', so settingCERT_PATHhas no effect in the image.
So instead of mounting /etc/letsencrypt, the docs now say: mount a directory at /certs (not single files), and for certbot, copy the two files into it from a --deploy-hook (the key with -o 1001). That works with the default CERT_PATH and fixes both the renewal and the permissions. I updated the PEM example to the directory mount, and the matching examples in docker-compose.yml and apps/smtp/README.md.
If you'd rather go the CERT_PATH route, the entrypoint can pass it through as '${CERT_PATH:-/certs}', but the permissions would still need a fix on the host.
| } | ||
|
|
||
| for (const server of servers) { | ||
| server.updateSecureContext({key: certs.key, cert: certs.cert}); |
There was a problem hiding this comment.
A failed swap here can crash the relay. updateSecureContext runs with no try/catch inside a fs.watchFile listener. In smtp-server 3.18.3 it clears the server's secure context before building the new one from tlsOptions(this.options), which can include more options than the bare key/cert checked above. If that build throws, the server is left with no context (get('*') returns undefined) and the error escapes the watcher callback, so the process exits. With two servers, the ones updated before the error keep the new cert and the rest keep the old one.
Suggestion: wrap the loop in try/catch and put the previous key/cert back on all servers if any update fails.
There was a problem hiding this comment.
Fixed in 873849c. The update loop is now in a try/catch. If any updateSecureContext() throws, the previous pair goes back on every server, so no server is left without a context or on a different cert. The whole reload callback is also wrapped, so no error can escape the timer and exit the process.
For the first certificate there is no previous pair, so the 465 server is created first. Its constructor builds the context from the pair, so a bad pair throws before 587 is touched.
Tested with a cert/key mismatch while running: both ports kept the previous cert, and the process stayed up.
| let currentCert = initialCert; | ||
|
|
||
| const reload = () => { | ||
| const certs = loadFromTraefikAcme() ?? loadFromPemFiles(); |
There was a problem hiding this comment.
A reload can switch to a different certificate source. If the relay started on acme.json and a poll catches Traefik rewriting it in place, JSON.parse fails and this falls back to loadFromPemFiles(). If PEM files are also mounted, an older or different cert passes validation and is hot-swapped in until the next acme.json change. The reverse can happen in PEM mode.
This line also repeats the source order from getCertificates(). Suggestion: have one shared resolver that both startup and reload use, which remembers which source was active and reloads only from that source.
There was a problem hiding this comment.
Fixed. Startup and reloads now share one resolver (resolveCertificates) over one CERTIFICATE_SOURCES list. The watcher remembers which source was active and reloads only from it.
Tested: acme.json active, PEM files with a different cert present, then acme.json left truncated. Both ports kept the acme.json cert and logged the parse error, with no switch to the PEM cert.
| try { | ||
| // updateSecureContext() drops the live context before building the new one, so an invalid pair | ||
| // (e.g. key rewritten but cert not yet) would leave the servers without TLS - validate it first | ||
| tls.createSecureContext({key: certs.key, cert: certs.cert}); |
There was a problem hiding this comment.
A half-written fullchain.pem can pass this check. If an ACME client rewrites fullchain.pem in place and the 60s poll fires partway through, the file may hold only the leaf certificate. That still parses and matches the key, so it goes live on both servers without the intermediate, and strict clients fail verification until the file changes again.
Suggestion: before swapping, check the files are stable (e.g. re-stat them and confirm size and mtime haven't changed), and/or check that the chain includes more than the leaf when the original did.
There was a problem hiding this comment.
Fixed with the stability check. A change event records the size, mtime and inode of the watched files, then waits 5s and reads. The read is used only if none of the files changed from the event until the read finished. If one did, it waits another 5s and tries again.
Tested with a poll that caught fullchain.pem holding only the leaf, then appended the intermediate 2.5s later: the leaf-only file was never served, and the full chain went live after the files settled.
I didn't add the chain-length check. It would also block a legitimate switch to a cert without intermediates, and the stability check already covers this case.
| signale.success('Reloaded renewed TLS certificate'); | ||
| }; | ||
|
|
||
| for (const file of [ACME_JSON_PATH, path.join(CERT_PATH, 'privkey.pem'), path.join(CERT_PATH, 'fullchain.pem')]) { |
There was a problem hiding this comment.
Too many reloads and misleading logs. All three paths are watched no matter which source is in use, and each trigger reruns the loaders with their info/success logging. On Dokploy/Coolify, acme.json holds certs for every app on the host, so any other domain's renewal causes a synchronous read and JSON.parse of the whole file on the event loop and logs "Loaded certificate for ..." even though nothing was swapped. In PEM mode one renewal can trigger up to three reloads, one per file.
The PEM paths are also rebuilt here instead of reusing the ones loadFromPemFiles uses, so they can drift apart if the filenames ever change.
Suggestion: watch only the active source's files, share path constants with the loader, and use quiet loader variants on reload.
There was a problem hiding this comment.
Fixed:
- Only the active source's files are watched. All sources are watched only until the first cert is found.
- Reloads use a silent logger. "Loaded certificate for …" is now printed only at startup, and "Reloaded renewed TLS certificate" only when the cert actually changes.
- Events from several files are merged into one reload, because each event restarts the settle timer. A PEM renewal now gives one reload.
- The PEM paths are now
PEM_KEY_PATHandPEM_CERT_PATH, shared by the loader, the startup message and the watcher.
One part remains: on Dokploy/Coolify, another domain's renewal still causes one read and JSON.parse of acme.json, because that's the only way to tell whether our cert changed. It now happens once per change, after the file settles, and logs nothing.
| try { | ||
| // updateSecureContext() drops the live context before building the new one, so an invalid pair | ||
| // (e.g. key rewritten but cert not yet) would leave the servers without TLS - validate it first | ||
| tls.createSecureContext({key: certs.key, cert: certs.cert}); |
There was a problem hiding this comment.
Minor: each successful reload builds the TLS context N+1 times (this throwaway check plus one per server inside updateSecureContext). It's cheap, but if you add the try/catch-and-restore around the update loop (see the comment on line 171), this pre-check is no longer needed.
There was a problem hiding this comment.
Removed. The rollback covers this case now.
Review follow-up for the certificate hot-reload: - Watch for certificates even when none were found at startup, and start the port 465 server when the first valid pair appears. - Reload only from the source that was active at startup (no fallback from a half-written acme.json to PEM files), watch only that source's files, and load quietly on reload. - Use a read only if no watched file changed from the change event until the read finished, at least 5s later, so a half-written file is never served. Coalesces the per-file change events into one reload. - If updateSecureContext() throws, put the previous pair back on every server instead of pre-validating the pair, and never let an error escape the watcher. - Docs: mount the PEM directory instead of single files; for certbot, copy the files from a deploy hook, since certbot's live/ and archive/ are root-only and the container runs as UID 1001. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the thorough review! Pushed 873849c, which addresses all seven comments (replies inline). I also updated the PR description with the new behaviour and the re-test results, including the cases from your comments. |
Description
The SMTP relay reads its TLS certificate once at startup, so renewals by Traefik/certbot are ignored until the container restarts, and ports 465/587 end up serving an expired certificate (details in #523).
This watches the certificate files and swaps a changed certificate into both servers with
smtp-server'supdateSecureContext(). Implicit TLS (465) and STARTTLS (587) both read the server's secure context per connection, so new connections get the new certificate right away. Open connections are unaffected and the process doesn't restart.How it works:
SMTP_DOMAIN), every source is watched. The first valid pair starts the 465 server and adds TLS to 587.fullchain.pem(e.g. leaf only) is never served. This also merges the events of several files into one reload.updateSecureContext()drops a server's live context before it builds the new one. If an update throws (e.g. a key/cert mismatch mid-renewal), the previous pair goes back on every server. No error can escape the watcher and crash the relay.fs.watchFilestats the files every 60s. That works on bind mounts and Docker Desktop and needs no new dependency.live/andarchive/are root-only, and the container runs as UID 1001. Traefik rewrites acme.json in place, so its single-file mount still works.Type of Change
feat:New feature (MINOR version bump)fix:Bug fix (PATCH version bump)feat!:Breaking change - new feature (MAJOR version bump)fix!:Breaking change - bug fix (MAJOR version bump)docs:Documentation update (no version bump)chore:Maintenance/dependencies (no version bump)refactor:Code refactoring (no version bump)test:Adding tests (no version bump)perf:Performance improvement (PATCH version bump)Testing
tsc --noEmit,eslintandprettier --checkpass forapps/smtp.apps/smtplocally with the watch interval set to 2s and the settle delay to 3s, wrote certificate files while it ran, and checked what each port served withopenssl s_client(-starttls smtpfor 587). The test leaf certs are signed by a test intermediate, andfullchain.pemholds leaf + intermediate.PEM files, starting with an empty
/certs:acme.json with
SMTP_DOMAIN=smtp.test:CN=localhostis smtp-server's built-in certificate, which 587 offers before any certificate is mounted (unchanged by this PR). The same overwrite on unpatchednext(76b4efb) keeps serving certA indefinitely (output in #523).Checklist
apps/smtp; verified manually as above)Related Issues
Closes #523
🤖 Generated with Claude Code