fix(san-cert): honor per-service --tls-staging in the SAN cert manager - #100
Merged
Conversation
## Summary --tls-staging set ServiceOptions.ACMEDirectory, but the field only ever reached the per-service autocert path. With --acme-email set, every service uses the shared SAN manager, which issued everything against the run-level --acme-directory (production by default) — dynamic domains included. One operator burned a zone's weekly production rate limit seeding test subdomains that were supposed to hit staging. The manager now keeps one ACME identity per directory: the run-level directory keeps the original account and clients; any other directory gets its own account (acme_user_staging.json / acme_user_<hash>.json), registered lazily on the first order. Deploys record the service's directory; every issuance path — handshake, dynamic issuer, renewer — resolves the identity through the one obtainCertificate choke point, so batches never mix directories. Certificates record their issuing directory (default-safe against old state files), and a staging<->production flip re-issues on the renewer's next reconcile instead of waiting for the renewal window. The certificate store export carries the extra account files. ## Test Coverage - san_cert_directories_test.go: directory overrides, owner resolution (registered/dynamic/pending/wildcard), account-file naming, cert provenance stamping, state round-trip + legacy state, per-directory obtainer selection, handshake batch partition, deploy wiring - domain_renewal_test.go: immediate re-issue on directory switch, unknown-owner keeps the recorded directory - service_test.go: ACMEDirectory URL validation - cert_store_restore_test.go: extra account keys + Directory field survive export/restore ## Verification - [x] gofmt -l internal/ cmd/ clean - [x] make test passes - [x] go test -race ./internal/server/ clean (1801 tests) - [x] make lint (golangci-lint) 0 issues
There was a problem hiding this comment.
1 issue found across 16 files
Confidence score: 4/5
- In
internal/server/cert_store_archive.go, archive validation currently accepts unexpectedcerts/acme_user_*.jsonfilenames, which could restore unintended ACME identities from crafted or malformed archives and lead to incorrect certificate account state—tighten matching to onlyacme_user_staging.jsonor an 8-character lowercase hex suffix to align with documented behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/server/cert_store_archive.go">
<violation number="1" location="internal/server/cert_store_archive.go:275">
P3: Unexpected `certs/acme_user_*.json` names pass archive validation and are restored as ACME identities; constrain matching to `acme_user_staging.json` or an 8-character lowercase hex hash so archives retain the documented closed entry set.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- A mixed-service legacy certificate now flags a directory mismatch when ANY owned domain wants a different directory, and its renewal splits into one order per desired directory, each under its owner's identity - The ARI replaces marker rides only an order staying at the issuing CA; a switched partition sends none (RFC 9773 has the target CA reject an identifier it never issued, which would have wedged the switch) - RegisterDomain's covered-by-existing-certificate fast paths require the covering certificate's directory to match the registering service, so a staging host under a production wildcard queues its own order; the handshake and the dynamic issuer treat such coverage as due for replacement (GetCertificate fallthrough + directory-aware HasValidCertificate) - A wildcard's owner is resolved only through domains the certificate actually serves — pending members getting their own certificate cannot drag the wildcard onto their directory — and concrete domains outrank wildcard coverage scans, deterministically (smallest covered name) - createCertManager clears the per-service directory override up front, so a redeploy off the shared SAN manager (static cert, on-demand URL, TLS disabled) cannot leave retained certificates renewing against a stale staging identity - The export's no-state guard treats per-directory account keys as estate metadata (an account-only store still exports), and isExtraAccountKeyFile is a closed set: acme_user_staging.json or an 8-char lowercase hex hash
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Second review round on PR #100: - renewPartition now pins each order (and its adoption stamp) to the partition's computed directory via managerObtainer.ObtainAt, so a domain whose owner is temporarily unresolvable — a wildcard whose only covered member is pending, say — renews under its certificate's recorded identity instead of silently falling back to the run-level directory - the ARI replaces marker is spent only once the CA accepted the order carrying it: an order the CA refused leaves the marker for the next same-directory partition, while a locally failed adoption still counts as spent (the CA already honored it) - the account-keys-only export test now exercises the lone-staging-key estate and verifies the archive it produces
mhenrixon
added a commit
that referenced
this pull request
Aug 13, 2026
Review finding on PR #103: the serve-stale rule was written for the expiry window, where reaching it means renewal has been failing and a live order would likely fail too. A directory mismatch is the opposite state — fresh operator intent with a healthy ACME — and the held certificate may be untrusted by exactly the clients the flip was made for (staging -> production). Registered domains therefore reprovision on the handshake again, as #100 shipped. Dynamic domains deliberately stay on the serve-stale path even on a mismatch: hard-failing every tenant handshake while the issuer drains a rate-limited queue would turn one flag flip into a fleet outage.
mhenrixon
added a commit
that referenced
this pull request
Aug 13, 2026
#103) * fix(san-cert): serve the held certificate while its replacement issues ## Summary For a registered domain inside the last 24h of its certificate's life (or holding a certificate from a directory its service moved away from), GetCertificate blocked the TLS handshake on a synchronous ACME order and failed the handshake on error — while a still-valid certificate sat in memory. Reaching that state means proactive renewal has been failing, which is exactly when a live order is most likely to fail too. Registered domains now mirror the dynamic path: while the certificate is valid it keeps serving, and the replacement is queued on the domain issuer (dedup, quarantine, shared rate bucket, directory-aware checks). Synchronous handshake provisioning remains only where it buys anything: first issuance and actual expiry. ## Test Coverage - TestSANCertManager_GetCertificate_ServesExpiringRegisteredCertAndQueuesReplacement - TestSANCertManager_GetCertificate_ExpiredRegisteredCertReprovisionsSynchronously - TestSANCertManager_GetCertificate_MismatchedDirectoryCertServedWhileReplacementQueues ## Verification - [x] gofmt -l internal/ cmd/ clean - [x] make test passes (1827 tests, -race clean) - [x] make lint 0 issues Closes #101 * fix(san-cert): registered directory-mismatch reprovisions synchronously Review finding on PR #103: the serve-stale rule was written for the expiry window, where reaching it means renewal has been failing and a live order would likely fail too. A directory mismatch is the opposite state — fresh operator intent with a healthy ACME — and the held certificate may be untrusted by exactly the clients the flip was made for (staging -> production). Registered domains therefore reprovision on the handshake again, as #100 shipped. Dynamic domains deliberately stay on the serve-stale path even on a mismatch: hard-failing every tenant handshake while the issuer drains a rate-limited queue would turn one flag flip into a fleet outage. * fix(san-cert): waiter refuses a still-mismatched certificate after a failed order Second review round on PR #103: a handshake that waits out another handshake's provisioning order returned getCertForDomain unconditionally — so when the order failed, the waiter was handed the very wrong-directory certificate the trigger was trying to replace. The waiter now goes through getServableCertForDomain, which refuses a certificate whose directory the registered owner has moved away from; the waiter's handshake fails cleanly and the next one retries the order. getCertForDomain had no other callers and is removed. * fix(san-cert): waiter also refuses an expired certificate Third review round on PR #103: getServableCertForDomain checked the directory but not NotAfter, so a waiter behind a failed order for an expired certificate was handed a certificate every client rejects. Refusing it keeps the failure server-side and retryable — the same rule the directory check applies.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--tls-stagingnever reached the shared SAN cert manager: the flag setServiceOptions.ACMEDirectory, but that field only fed the per-service autocert path. Whenever the proxy runs with--acme-email, every service's certs come from the SAN manager, which issued everything — deploy hosts and--tls-domains-sourcedynamic domains alike — against the run-level--acme-directory(production by default). That's how ~1,000 staging-intended test subdomains burned a zone's weekly production rate limit.This PR makes the manager keep one ACME identity per directory and honors the per-service directory end-to-end:
internal/server/san_cert_directories.go(new): per-service directory overrides (SetServiceDirectory), domain→owner→directory resolution (registered, dynamic, pending, wildcard-via-covered-name), per-directory client bundles with lazily registered accounts, and theobtainersForresolver.internal/server/san_cert_issuance.go:obtainCertificate— the one choke point every issuance path funnels through — picks the owning service's identity; strategy (DNS-01 zone routing, HTTP-01 fallback, wildcard refusal) is unchanged per identity.internal/server/san_cert_manager.go:registeredDomainsnow tracks owners (rebuilt on boot, no state impact);ManagedCertrecords its issuingDirectory(omitempty, old state files read as run-level); the handshake batch path partitions pending domains by directory so one order never mixes identities. The dynamic issuer needed no change — its batches are already single-service.internal/server/domain_renewal.go: a directory mismatch (staging↔production flip) re-issues on the next reconcile instead of at the renewal window; a cert with no resolvable owner keeps its recorded directory (no churn while services re-attach after a restart). ARI queries route to the cert's recorded directory; an unbuilt bundle falls back to the window heuristic rather than registering an account for a read-only query.internal/server/service.go+service_options_validation.go: the deploy wiresSetServiceDirectorybefore registering hosts, andACMEDirectorymust parse as an http(s) URL (it arrives over RPC; the CLI only ever sends the staging constant).internal/server/cert_store_{export,archive,restore}.go: per-directory account files (acme_user_staging.json,acme_user_<hash>.json) are part of the exported estate — previously the exporter warned on them and the strict archive reader rejected them.Account file naming: the run-level directory keeps
acme_user.json(existing accounts keep working on upgrade); the well-known LE staging URL getsacme_user_staging.json; anything elseacme_user_<sha256[:8]>.json.Closes #99
Test plan
make test— full suite greengo test -race ./internal/server/— 1801 tests, no data racesgofmt -l internal/ cmd/— empty;make lint— 0 issues;go vet ./...cleanmake build— cleanmake dockersmoke test — skipped locally (Docker daemon not running); CI builds the imageDirectoryfieldDeviations & judgment calls
acme_user_staging.jsonand the strict archive reader rejected archives containing it — without explicit support, a staging identity would be silently dropped from backups. Extra keys get the same parse/ECDSA validation as the primary.managerObtainer.GetRenewalInfowas hardwired to the primary client, but a staging cert's ARI record lives at the staging CA. Routed by the certificate's recorded directory;reconcilechecksdirectoryChangedbeforeshouldRenewso ARI is never consulted at a directory that a pending switch has already invalidated.serviceDirectoriesentries linger after service removal, consistent withregisteredDomains; both self-heal on redeploy/restart, and the renewer's unknown-owner rule keeps orphaned certs from churning.san_cert_manager.gois at 925 lines ondash— the coding-style rule's "688, near the ceiling" note is stale. This branch adds ~25 net lines there; all new logic lives insan_cert_directories.go. Splitting the manager file is real debt, but out of scope here.Summary by cubic
Honors per‑service
--tls-stagingin the shared SAN cert manager by keeping one ACME identity per directory and routing issuance, renewal, batching, and ARI by the owning service. Prevents accidental production issuance and handles directory flips safely.Bug Fixes
SetServiceDirectoryand resolves by domain owner (registered/dynamic/pending/wildcard); batches never mix directories, including the handshake path.ObtainAt; adoption stampsDirectory. ARI “replaces” is sent only when staying with the issuing CA and is spent only once the CA accepts an order (CA refusal keeps it for later; local adoption failure still consumes it). Unknown‑owner certs keep their recorded directory.GetCertificateandHasValidCertificate.Directory.Migration
acme_user_staging.jsonandacme_user_<hash>.json; restore writes them back and validates keys. Account‑only stores export cleanly.ServiceOptions.ACMEDirectorymust be an http(s) URL.Written for commit e646d00. Summary will update on new commits.