fix(traefik): stop Let's Encrypt from overriding domains set to Certificate: None - #4963
fix(traefik): stop Let's Encrypt from overriding domains set to Certificate: None#4963onlyilkr wants to merge 17 commits into
Conversation
The store was rewritten in place on both the local and the remote path. The remote command truncated acme.json with `>` before writing and used `;` to chain chmod, so a dropped SSH stream, a failing base64 or a full disk left an empty, world-readable store behind and destroyed every Let's Encrypt certificate on that server. `fs.writeFileSync` had the same truncate-in-place exposure. Both paths now write a sibling temp file, set mode 0600 on it and rename it over the target, which is atomic within a directory. The remote commands are chained with `&&` so a failure stops the sequence, and the temp file is removed on failure.
The helper lived inside the tRPC router, so the shared-host guard, the only thing preventing the deletion of a certificate another domain still needs, had no test coverage. It now lives next to the domain services it uses and is exported, with unit tests for the purge, the guard, the letsencrypt short circuit and the failure path. A purge failure no longer fails the mutation either: the domain row was already updated and the router already regenerated, so an unreachable server made the user see an error for an update that succeeded. Failures are logged and reported as "no reload required".
The startup pass purged ACME certificates without the shared-host guard the mutation path applies, so a host still served by another domain with certificateType "letsencrypt" lost its certificate at boot and Traefik was restarted on top of it. It now reuses hasOtherLetsencryptDomainForHost and only purges genuinely unused hosts. The purge also ran after manageDomain. Once the router carries its `tls` key, routerNeedsTlsFix is false, so a purge that threw was never retried on a later boot. Purging first leaves the router untouched on failure and the next boot retries the whole domain.
…hange The domain update mutation already returned traefikReloadRequired but no caller read it. Traefik only reads acme.json at startup, so without a restart the purge has no effect and can be undone when Traefik next rewrites the store from memory. The dialog now raises a toast when the field is true.
The four reconciliation behaviours were asserted inside one large `it` with a sentence-long title, so a failure said nothing about which one broke. They are now separate cases over a shared beforeEach fixture, plus coverage for the shared-host guard and the purge-before-regenerate ordering.
Raising it as a second toast collapsed it onto the success one, since the Toaster does not set expand, and no other place in the app fires two toasts for a single action. It is now the success toast's description, with a longer duration because the message asks the user to go and restart Traefik rather than just confirming what happened.
dosubot
Bot
added
size:L
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A purge is a read-modify-write over a file Traefik owns. On a remote server the read is an SSH round trip, so two domains switched off Let's Encrypt at the same time both read the original store and the later write puts back whatever the earlier one removed, leaving a stale certificate that keeps shadowing the Origin CA one. Purges for a server now run one at a time. The chain survives a failed purge so one broken SSH connection cannot block later ones.
The domain query ran outside the per-application error isolation, so a database failure propagated to the shared catch in server.ts. Everything sequenced after it was skipped: the backup cron jobs, the restart notifications, the enterprise cron jobs and the deployment worker, all while the HTTP server was already listening. The pass is best effort and now never rejects.
Traefik owns acme.json and rewrites it in full whenever it issues or renews a certificate, with no locking protocol to join. A snapshot taken before such a write would have dropped the new certificate on rename. The store is now re-read and compared just before the swap, and the purge starts over if it changed. That narrows the window to the final round trip rather than closing it, which is as far as this can go without stopping Traefik to edit its own state. If a purged certificate is reinstated by a write we lost the race to, the startup reconciliation purges it again on the next boot.
The startup pass only purged an application's stale certificates when
its routers still needed regeneration. Once a router carried `tls: {}`
routerNeedsTlsFix went false and the application was skipped entirely,
so a purge that had exhausted its retry budget or lost a race against
Traefik was never revisited.
Split reconcileDomainTls into two phases: phase 1 regenerates router
configs as before, phase 2 purges every domain's host regardless of
whether its router needed regeneration. Phase 2 groups domains by
server rather than by application, since acme.json is one file per
server, cutting repeated reads (and remote SSH round trips) down to
one per server per pass.
|
@Siumauricio all three review-bot findings are addressed and the threads are resolved. One open question for you, carried over from the last one: I have a prototype that closes the remaining acme.json write race properly by stopping Traefik around the purge instead of racing it. I deliberately kept it out of this PR because it trades a bounded, self-healing race for an availability risk and changes what settings.reloadTraefik does. Happy to open it as a follow-up, or to drop the "Dokploy writes to acme.json" approach altogether if you would rather that file stayed Traefik's alone. |
Resolves the toast conflict in handle-domain.tsx. Both sides added a description to the same toast.success call: canary asks compose users to redeploy (898af1f), this branch asks application users to restart Traefik after a certificate purge. The two never apply at once. purgeStaleCertificate only runs in the update mutation's `domain.applicationId` branch, so traefikReloadRequired is always false for compose domains, and the redeploy hint is gated on domainType === "compose". They are also different remedies: a compose domain is a docker label that lands on the next deploy, while a purged acme.json entry only takes effect once Traefik re-reads the store at startup. Kept both as mutually exclusive branches picking a single description.
|
|
||
| const removed = await purgeAcmeCertificates(purgeableHosts, serverId); | ||
| if (removed.length > 0) { | ||
| await reloadDockerResource("dokploy-traefik", serverId); |
There was a problem hiding this comment.
If Traefik issues or renews an unrelated certificate after the purge's final comparison but before its whole-file replacement, the stale snapshot overwrites that certificate and this immediate restart discards the in-memory copy, leaving the affected hostname without its expected certificate until ACME issuance succeeds again.
Knowledge Base Used: Traefik Networking
There was a problem hiding this comment.
@Siumauricio this one is a product call rather than a technical one, and I'd rather have your read before I build it out. Greptile's P1 above is valid, I verified it, but the fix depends on a decision I can't make on your behalf.
What the finding is
initDomainTlsReconciliation purges stale entries from acme.json at startup and then reloads Traefik immediately. Traefik owns that file and rewrites it in full on issue or renewal, so there is a window between our final "unchanged?" check and the rename where a certificate Traefik just wrote gets overwritten by our snapshot.
Until now the consolation was that the lost certificate stays in Traefik's memory and keeps being served, and the startup pass re-purges on the next boot. The immediate reload removes that consolation: it discards the in-memory copy right away, so an unrelated hostname is left without its certificate until ACME issues a new one.
Two things worth stating plainly:
This is entirely new behaviour from this PR. Nothing on canary reads or writes acme.json. The only existing references set its permissions (traefik-setup.ts, server-setup.ts). This PR is the first code in Dokploy to write to Traefik's certificate store, so the whole race class comes with the fix, not before it.
It would also be the first automatic service restart in Dokploy. reloadDockerResource has exactly two other callers, settings.reloadServer and settings.reloadTraefik, both adminProcedure mutations behind an explicit click and both audited. The startup chain in server.ts sets up crons, schedules and notifications. Nothing there restarts infrastructure. That seems like a line worth deciding deliberately rather than inheriting from a bug fix.
The purge itself is load-bearing
Just so the options are judged on the right premise: dropping the acme.json work entirely is not on the table.
The router fix (tls: {}) stops new issuance, but Traefik still serves an already-issued certificate from its store, and it wins on specificity. An exact-match app.example.com from Let's Encrypt beats a wildcard *.example.com Origin CA certificate.
On my instance, emptying the ACME store is what moved all 13 hostnames onto Origin CA. Nothing else changed. Existing installs need the entry gone.
The three options
Same starting point for all three: an operator upgrades Dokploy on a server where domains were switched to Certificate: None months ago, so acme.json still holds stale Let's Encrypt certificates for them.
A: purge and reload immediately (what the PR does now).
Their sites go down for a few seconds during an upgrade they thought was routine, with no warning. When it comes back the problem is fixed for good.
Rare bad case: if Traefik was writing a certificate in that window, that hostname breaks until ACME reissues.
B: purge, don't reload.
Nothing goes down and the operator notices nothing, including that the problem is still there, because Traefik keeps serving the old certificates from memory. It resolves on the next Traefik restart, whenever that happens.
Risk: if Traefik rewrites its store before that restart, our removal is undone and the next boot has to redo it. That loop can outlast the certificate's expiry, which is the failure the issue is about.
C: stop Traefik, purge, start it again.
Identical cost to A from the operator's side, same few seconds. But nothing can race us because Traefik is not running while we write, so A's bad case disappears.
New risk: if the start fails, Traefik stays down, and on a single-server install that takes the Dokploy UI with it.
A and C cost the operator the same thing. C is strictly more correct for that price. B is the only one that costs nothing, and it pays for that by leaving the timing of the fix to chance.
What I'd suggest
Split the PR, and keep this one to the parts that are already settled:
- the router fix on both the file-config and Docker-label paths
- the
acme.jsonpurge on the mutation path, which returnstraefikReloadRequiredand surfaces "Restart Traefik to apply the certificate change for this domain" in the success toast, leaving the restart to the operator
That is all user-triggered, introduces no automatic restart, and the residual race stays behind a restart the operator chooses. That is exactly the design I described in the issue. Greptile's P1 does not apply to it.
Then the startup reconciliation becomes its own PR, where A/B/C can be argued on its own merits, and where B would also need a UI surface to be honest. An operator with no reason to restart Traefik will never learn that they should.
The closest existing pattern is UpdateServerButton in the sidebar footer, which polls a condition and shows a persistent affordance when it is true. The same shape would work here with settings.reloadTraefik as the action.
The cost of splitting is that existing installs don't get fixed automatically on upgrade. They'd need to re-save each affected domain, which now does the right thing end to end and tells them to restart Traefik. The manual path is documented in Dokploy/website#172 .
If you'd rather keep it all in one PR, tell me which of A, B or C you want and I'll build it. My own preference is C for the reconciliation path, because it closes the race for the same downtime A already spends. But it is your call whether Dokploy should ever restart a service without being asked.
One thing I could not verify
Whether Traefik flushes its in-memory ACME store to acme.json on SIGTERM. If it does, a stop or a forced restart could write the purged certificate back and defeat the cleanup in both A and C.
I have not been able to test this properly, and it would change the analysis, so I'd rather flag it than assume either way.
1 participant
What is this PR about?
A domain set to
Certificate: Nonekeeps being served its old Let's Encrypt certificate, and Traefik keeps trying to renew it. Once that certificate expires, Cloudflare inFull (Strict)mode returns Error 526 even though a valid Cloudflare Origin CA certificate is installed and the domain looks correctly configured.The cause is not that the
websecureentrypoint carries acertResolverdefault. It is that the router never overrides it. Traefik applies an entrypoint's TLS default only to routers whose TLS is nil (pkg/server/aggregator.go,applyModel):and
certificateType: "none"wrote exactly that nil. Writing an empty TLS block instead leaves the entrypoint untouched, so there is notraefik.ymlmigration and no risk oftls:ending up null, which would disable TLS and turn 526 into 525 across every site.utils/traefik/forward-auth.tsalready handled the identical case this way, so this part is really a consistency fix.Three changes:
1. Routers override the entrypoint default.
certificateType: "none"now writestls: {}. The same widening coverscertificateType: "custom"with an emptycustomCertResolver, which previously matched no branch and fell through to nil as well. The Docker-label path gets the equivalent change; it already emittedtls=truefornone, so Compose domains were never hit by the original bug.2. The stale certificate is removed. Fixing the router stops new issuance but leaves the old certificate in the store, and an exact-hostname Let's Encrypt certificate still wins over a wildcard Origin CA one. When a domain moves off Let's Encrypt its entry is now dropped from
acme.json, guarded so a hostname another domain still serves with Let's Encrypt is left alone. The store is written atomically (temp file,0600, rename) rather than truncated in place, so a failed write cannot destroy the certificates on that server.3. Existing installations are reconciled.
manageDomainruns on domain create and update but not on deploy, so an installation that already has the broken router config on disk would not pick the fix up until every affected domain was re-saved by hand. A one-shot pass at startup regenerates only the router configs that are actually missing their TLS key. It is idempotent, it preserves manual edits becausemanageDomainreplaces only its own router and service keys, and failures are isolated per application so an unreachable remote server cannot block startup.Things worth knowing while reviewing
acme.jsonis read once at startup and served from memory afterwards. Restarting Traefik briefly drops every site on the server, which is too much of a side effect for saving one domain, so the mutation reports it and the success toast tells the user to restart. The startup pass does reload automatically, once per server and only when something was actually removed, because at boot that is far less disruptive. Happy to change either behaviour if you would rather have it the other way.applicationId.certificate.ymlis read at startup and on any reload triggered by a top-level change, just not on its own. Different problem.Checklist
canarybranch.Issues related (if applicable)
closes #4949
Testing
Unit tests cover all three certificate types on both the file-config and the Docker-label path, the ACME store helper including the shared-host guard and the failure path, and the reconciliation pass. 105 tests pass across the affected files, and
packages/serverandapps/dokploytypecheck clean.Verified end to end on a local instance:
Certificate: Noneon a domaintls: {}Let's Encrypttls: { certResolver: letsencrypt }, unchanged from beforeNoneacme.json, the other certificate untouched,Accountpreserved, file still0600, no temp file left behindtlskey deleted by hand, then a restartReconciled TLS config for 1 domain(s) on <app>, key restoredScreenshots (if applicable)
Greptile Summary
The PR prevents domains using Certificate: None from inheriting the default Let's Encrypt resolver and adds stale ACME-certificate cleanup plus startup reconciliation.
Confidence Score: 4/5
The PR is not yet safe to merge because startup reconciliation can immediately restart Traefik after a residual ACME-store race has discarded an unrelated active certificate.
Although the author mitigated concurrent writes by re-reading acme.json before replacement, Traefik can still write during the final comparison-to-rename interval. The new reconciliation path then restarts Traefik immediately, contradicting the earlier assumption that the lost certificate remains available in memory and exposing a TLS failure until reissuance.
Files Needing Attention: packages/server/src/utils/traefik/acme.ts; packages/server/src/setup/domain-tls-reconciliation.ts
Reviews (5): Last reviewed commit: "Merge branch 'canary' into fix/4949-cert..." | Re-trigger Greptile
Context used (3)