fix(san-cert): serve the held certificate while its replacement issues - #103
Merged
Conversation
## 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
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 2/5
- In
internal/server/san_cert_manager.go, the staging→production ACME transition can keep serving the staging cert while a new one is queued, causing public TLS handshake failures for all clients until issuance finishes; this is a concrete availability risk during cutover—switch to a valid production-chain fallback or force immediate cert replacement before serving traffic.
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/san_cert_manager.go">
<violation number="1" location="internal/server/san_cert_manager.go:474">
P1: When a service moves from the staging ACME directory to production, this branch still serves the staging certificate while replacement is queued, so public TLS clients reject every handshake until issuance completes. Do not serve a certificate with `directoryMismatch`; let registered domains reprovision and let dynamic domains fail fast while the issuer replaces it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
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.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…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.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
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
internal/server/san_cert_manager.go—GetCertificateno longer blocks the TLS handshake on a synchronous ACME order for a registered domain whose certificate is merely due for replacement (inside 24h of expiry, or issued by a directory its service has moved away from). While the certificate is still valid it keeps serving, and the replacement is queued on the domain issuer — which already brings dedup, quarantine, preflight-skip for held certs, the shared rate bucket, and directory-awareissuablechecks. Synchronous handshake provisioning remains only where it buys anything: first issuance and actual expiry.The failure mode this removes: reaching the <24h window means the renewer has been failing for weeks (ACME outage, quarantine-deferred batch), and the old code chose that exact moment to gamble every handshake on a live order — failing connections while a valid certificate sat in memory.
Closes #101
Test plan
TestSANCertManager_GetCertificate_ServesExpiringRegisteredCertAndQueuesReplacement— held cert served, no synchronous order, replacement queued with the owning serviceTestSANCertManager_GetCertificate_ExpiredRegisteredCertReprovisionsSynchronously— an expired cert is replaced on the spot, handshake gets the fresh certTestSANCertManager_GetCertificate_MismatchedDirectoryCertServedWhileReplacementQueues— post---tls-staging-flip cert keeps serving while the replacement queuesmake test(1827 tests),go test -race,make lint,gofmt,go vetall cleanDeviations & judgment calls
acme-v02.api.letsencrypt.org/acme/new-acct(rejected for the example.com contact) because the sync order lazily built a real client bundle for the overridden directory. The fix removes the network call from that path; noted as a caution for future cert tests —clientsForDirectoryregisters live accounts wherever an overridden-directory order escapes a test without an injected fake bundle.Summary by cubic
Keep serving a still‑valid certificate during TLS handshakes while its replacement issues asynchronously to avoid outages when renewal is degraded. Previously handshakes blocked for expiring (<24h) or directory‑mismatched certs; now only first issuance, expired certs, and registered mismatches block, and waiters refuse expired or mismatched held certs after failed orders.
Written for commit 2899d3c. Summary will update on new commits.