From 6dcdfa539a07bb0c9ae71e5eeb68de7d37e99318 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 13 Aug 2026 10:42:26 +0200 Subject: [PATCH 1/4] fix(san-cert): serve the held certificate while its replacement issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- internal/server/san_cert_manager.go | 36 ++++++----- internal/server/san_cert_manager_test.go | 77 ++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 8f361ac..4bf2568 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -421,10 +421,12 @@ func (m *SANCertManager) UnregisterDomain(domain string, service string) error { // GetCertificate returns a certificate for the TLS handshake. // // Provisioning is gated by a hard allowlist: deploy-registered hosts provision -// synchronously (the original behavior), dynamic domains are queued for +// synchronously only when no still-valid certificate exists (first issuance, +// or expiry that renewal failed to prevent), dynamic domains are queued for // asynchronous issuance, and any other server name is refused outright so a // catch-all service cannot be used to burn rate limits on scanner-supplied -// names. +// names. A held certificate that is merely due for replacement keeps serving +// while its replacement is issued in the background. func (m *SANCertManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { domain := hello.ServerName if domain == "" { @@ -460,26 +462,28 @@ func (m *SANCertManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certif return cert.Certificate, nil } - if isRegistered { - if directoryMismatch { - slog.Info("Covering certificate is from another ACME directory, will reprovision", - "domain", domain, - "certificate_directory", cert.Directory, - ) - } else { - slog.Info("Certificate expiring soon, will reprovision", + // Due for replacement: expiring inside 24 hours, or issued by a + // directory the owning service has moved away from. While the + // certificate is still valid it keeps serving, and the replacement is + // queued for asynchronous issuance — reaching this state means + // proactive renewal has been failing, which is exactly when a + // synchronous order on the handshake is most likely to fail too, and + // a handshake that errors while a valid certificate is in hand is a + // self-inflicted outage. Evicted domains (neither registered nor + // dynamic) serve out the certificate they have with no replacement. + if time.Until(cert.NotAfter) > 0 { + if isRegistered || isDynamic { + slog.Info("Certificate due for replacement; serving held certificate meanwhile", "domain", domain, "expiresAt", cert.NotAfter, + "directory_mismatch", directoryMismatch, ) - } - } else if time.Until(cert.NotAfter) > 0 { - // Dynamic and evicted domains keep serving a still-valid - // certificate; the renewal loop is responsible for rotating it. - if isDynamic { - m.requestDynamicCertificate(domain, dynamicService) + m.requestDynamicCertificate(domain, owner) } return cert.Certificate, nil } + // Expired: nothing worth serving remains, so registered domains fall + // through to synchronous provisioning and dynamic ones to the issuer. } if isRegistered { diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index 8eb3cb4..8f5b913 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -460,3 +460,80 @@ func TestSANCertManager_InitializeAdoptsLegacyCacheWithoutDeadlock(t *testing.T) assert.True(t, manager.HasCertificate("legacy.test"), "the legacy certificate was not adopted") } + +// Issue #101: a registered domain in the last 24h of its certificate's life +// must keep serving the held certificate and replace it in the background — +// not gamble the handshake on a synchronous ACME order. +func TestSANCertManager_GetCertificate_ServesExpiringRegisteredCertAndQueuesReplacement(t *testing.T) { + manager := testSANCertManager(t) + obtainer := successfulObtainer(t) + manager.httpObtainer = obtainer + + requests := [][2]string{} + manager.SetDynamicCertRequester(func(domain, service string) { + requests = append(requests, [2]string{domain, service}) + }) + + require.NoError(t, manager.RegisterDomain("app.example.com", "web")) + held, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-89*24*time.Hour), time.Now().Add(2*time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) + + require.NoError(t, err, "a handshake must not fail while a valid certificate is held") + assert.Same(t, held.Certificate, served) + assert.Empty(t, obtainer.Calls(), "no synchronous order may ride the handshake") + require.Len(t, requests, 1, "a replacement must be queued asynchronously") + assert.Equal(t, [2]string{"app.example.com", "web"}, requests[0]) +} + +// An actually expired certificate serves nobody: the synchronous first-issuance +// path remains the right response for a registered domain. +func TestSANCertManager_GetCertificate_ExpiredRegisteredCertReprovisionsSynchronously(t *testing.T) { + manager := testSANCertManager(t) + obtainer := successfulObtainer(t) + manager.httpObtainer = obtainer + + require.NoError(t, manager.RegisterDomain("app.example.com", "web")) + _, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-90*24*time.Hour), time.Now().Add(-time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) + + require.NoError(t, err) + require.NotNil(t, served) + require.Len(t, obtainer.Calls(), 1, "an expired certificate must be replaced on the spot") + assert.True(t, served.Leaf.NotAfter.After(time.Now().Add(24*time.Hour)), "the handshake must get the fresh certificate") +} + +// A still-valid certificate from the wrong ACME directory (post --tls-staging +// flip) follows the same rule: serve what we hold, replace in the background. +func TestSANCertManager_GetCertificate_MismatchedDirectoryCertServedWhileReplacementQueues(t *testing.T) { + manager := testSANCertManager(t) + obtainer := successfulObtainer(t) + manager.httpObtainer = obtainer + + requests := []string{} + manager.SetDynamicCertRequester(func(domain, service string) { + requests = append(requests, domain) + }) + + require.NoError(t, manager.RegisterDomain("app.example.com", "staged")) + held, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + manager.SetServiceDirectory("staged", LetsEncryptProduction) + + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) + + require.NoError(t, err) + assert.Same(t, held.Certificate, served, "the held certificate keeps serving until its replacement lands") + assert.Empty(t, obtainer.Calls()) + assert.Equal(t, []string{"app.example.com"}, requests) +} From cfaf93867587caab0cbb0c5568f2209e5b4ae88f Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 13 Aug 2026 11:08:59 +0200 Subject: [PATCH 2/4] fix(san-cert): registered directory-mismatch reprovisions synchronously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/server/san_cert_manager.go | 19 +++++++-- internal/server/san_cert_manager_test.go | 52 +++++++++++++++++++----- 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 4bf2568..db51966 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -465,13 +465,24 @@ func (m *SANCertManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certif // Due for replacement: expiring inside 24 hours, or issued by a // directory the owning service has moved away from. While the // certificate is still valid it keeps serving, and the replacement is - // queued for asynchronous issuance — reaching this state means + // queued for asynchronous issuance — reaching the expiry window means // proactive renewal has been failing, which is exactly when a // synchronous order on the handshake is most likely to fail too, and // a handshake that errors while a valid certificate is in hand is a // self-inflicted outage. Evicted domains (neither registered nor // dynamic) serve out the certificate they have with no replacement. - if time.Until(cert.NotAfter) > 0 { + // + // A registered domain whose certificate came from the wrong directory + // is the exception: the mismatch is fresh operator intent (a + // --tls-staging flip), not a degraded renewal — ACME is presumably + // healthy, and a staging certificate is untrusted by public clients + // anyway — so the handshake reprovisions synchronously, exactly as a + // first issuance would. Dynamic domains stay on the serve-stale path + // even then: failing every tenant handshake at once while the issuer + // drains a rate-limited queue would turn one flag flip into a fleet + // outage. + syncReprovision := directoryMismatch && isRegistered + if time.Until(cert.NotAfter) > 0 && !syncReprovision { if isRegistered || isDynamic { slog.Info("Certificate due for replacement; serving held certificate meanwhile", "domain", domain, @@ -482,8 +493,8 @@ func (m *SANCertManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certif } return cert.Certificate, nil } - // Expired: nothing worth serving remains, so registered domains fall - // through to synchronous provisioning and dynamic ones to the issuer. + // Expired (or mismatched-registered): registered domains fall through + // to synchronous provisioning and dynamic ones to the issuer. } if isRegistered { diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index 8f5b913..e6b8d07 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -510,9 +510,39 @@ func TestSANCertManager_GetCertificate_ExpiredRegisteredCertReprovisionsSynchron assert.True(t, served.Leaf.NotAfter.After(time.Now().Add(24*time.Hour)), "the handshake must get the fresh certificate") } -// A still-valid certificate from the wrong ACME directory (post --tls-staging -// flip) follows the same rule: serve what we hold, replace in the background. -func TestSANCertManager_GetCertificate_MismatchedDirectoryCertServedWhileReplacementQueues(t *testing.T) { +// A registered domain holding a certificate from the wrong ACME directory +// (post --tls-staging flip) reprovisions synchronously: the mismatch is fresh +// operator intent, not a degraded renewal, and a wrong-CA certificate may be +// untrusted by the clients the flip was made for. +func TestSANCertManager_GetCertificate_MismatchedDirectoryCertReprovisionsSynchronously(t *testing.T) { + manager := testSANCertManager(t) + stagingObtainer := successfulObtainer(t) + manager.httpObtainer = stagingObtainer + + prodObtainer := successfulObtainer(t) + manager.directoryClients[LetsEncryptProduction] = &directoryClients{httpObtainer: prodObtainer} + + require.NoError(t, manager.RegisterDomain("app.example.com", "staged")) + held, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + manager.SetServiceDirectory("staged", LetsEncryptProduction) + + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) + + require.NoError(t, err) + require.NotNil(t, served) + assert.NotSame(t, held.Certificate, served, "the wrong-directory certificate must not keep serving") + require.Len(t, prodObtainer.Calls(), 1, "the replacement must be ordered at the service's directory") + assert.Empty(t, stagingObtainer.Calls()) +} + +// A dynamic domain in the same situation keeps serving: hard-failing every +// tenant handshake while the issuer drains a rate-limited queue would turn +// one --tls-staging flip into a fleet outage. +func TestSANCertManager_GetCertificate_MismatchedDynamicCertServedWhileIssuerReplaces(t *testing.T) { manager := testSANCertManager(t) obtainer := successfulObtainer(t) manager.httpObtainer = obtainer @@ -522,18 +552,18 @@ func TestSANCertManager_GetCertificate_MismatchedDirectoryCertServedWhileReplace requests = append(requests, domain) }) - require.NoError(t, manager.RegisterDomain("app.example.com", "staged")) + manager.SetDynamicDomains("tenants", []string{"shop.tenant.net"}) held, err := manager.adoptCertificate( - testCertResource(t, []string{"app.example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)), - []string{"app.example.com"}) + testCertResource(t, []string{"shop.tenant.net"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)), + []string{"shop.tenant.net"}) require.NoError(t, err) - manager.SetServiceDirectory("staged", LetsEncryptProduction) + manager.SetServiceDirectory("tenants", LetsEncryptProduction) - served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.example.com"}) + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "shop.tenant.net"}) require.NoError(t, err) - assert.Same(t, held.Certificate, served, "the held certificate keeps serving until its replacement lands") - assert.Empty(t, obtainer.Calls()) - assert.Equal(t, []string{"app.example.com"}, requests) + assert.Same(t, held.Certificate, served, "tenant handshakes keep serving while the issuer replaces") + assert.Empty(t, obtainer.Calls(), "no synchronous order may ride a tenant handshake") + assert.Equal(t, []string{"shop.tenant.net"}, requests) } From 2c1861387ce6622ef7b8d0992a02b1fef83937ef Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 13 Aug 2026 11:20:57 +0200 Subject: [PATCH 3/4] fix(san-cert): waiter refuses a still-mismatched certificate after a failed order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/server/san_cert_manager.go | 17 +++++++++-- internal/server/san_cert_manager_test.go | 39 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index db51966..88e1aa6 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -532,7 +532,7 @@ func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string // Wait for existing provisioning to complete select { case <-done: - return m.getCertForDomain(domain) + return m.getServableCertForDomain(domain) case <-ctx.Done(): return nil, ctx.Err() } @@ -719,8 +719,14 @@ func (m *SANCertManager) adoptCertificateAt(resource *certificate.Resource, sort return managed, nil } -// getCertForDomain retrieves a certificate for a domain -func (m *SANCertManager) getCertForDomain(domain string) (*tls.Certificate, error) { +// getServableCertForDomain retrieves the certificate covering a domain, +// refusing one the domain's owner would not accept. A handshake that waited +// out another handshake's order can find the store unchanged when that order +// failed; for a registered domain mid-directory-flip, handing it the +// still-mismatched certificate would serve the wrong CA to the exact clients +// the flip was made for — the waiter fails instead, and the next handshake +// retries the order. +func (m *SANCertManager) getServableCertForDomain(domain string) (*tls.Certificate, error) { m.mu.RLock() defer m.mu.RUnlock() @@ -734,6 +740,11 @@ func (m *SANCertManager) getCertForDomain(domain string) (*tls.Certificate, erro return nil, ErrCertNotFound } + if service, ok := m.registeredDomains[domain]; ok && service != "" && + !m.certMatchesServiceDirectoryLocked(cert, service) { + return nil, ErrCertNotFound + } + return cert.Certificate, nil } diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index e6b8d07..f6f3609 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -567,3 +567,42 @@ func TestSANCertManager_GetCertificate_MismatchedDynamicCertServedWhileIssuerRep assert.Empty(t, obtainer.Calls(), "no synchronous order may ride a tenant handshake") assert.Equal(t, []string{"shop.tenant.net"}, requests) } + +// A handshake that waits out another handshake's order must not be handed the +// wrong-directory certificate that order failed to replace. +func TestSANCertManager_GetCertificate_WaiterRefusesStillMismatchedCert(t *testing.T) { + manager := testSANCertManager(t) + manager.httpObtainer = successfulObtainer(t) + + require.NoError(t, manager.RegisterDomain("app.example.com", "staged")) + _, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + manager.SetServiceDirectory("staged", LetsEncryptProduction) + + // Occupy the provisioning slot, as a concurrent handshake's order would. + inflight := make(chan struct{}) + manager.mu.Lock() + manager.provisioning["_batch_"] = inflight + manager.mu.Unlock() + + type result struct { + cert *tls.Certificate + err error + } + results := make(chan result, 1) + go func() { + cert, err := manager.provisionCertificate(context.Background(), "app.example.com") + results <- result{cert, err} + }() + + // The order finishes WITHOUT adopting a replacement (it failed). + close(inflight) + + r := <-results + require.Error(t, r.err, "the waiter must not serve the certificate the failed order was replacing") + assert.ErrorIs(t, r.err, ErrCertNotFound) + assert.Nil(t, r.cert) +} From 2899d3c480fdd25c805d3bc6e298c9fc4b367042 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 13 Aug 2026 11:30:48 +0200 Subject: [PATCH 4/4] fix(san-cert): waiter also refuses an expired certificate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/server/san_cert_manager.go | 6 ++++ internal/server/san_cert_manager_test.go | 35 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 88e1aa6..be37ad0 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -740,6 +740,12 @@ func (m *SANCertManager) getServableCertForDomain(domain string) (*tls.Certifica return nil, ErrCertNotFound } + // An expired certificate fails at the client anyway; refusing it here + // keeps the failure server-side and retryable. + if time.Until(cert.NotAfter) <= 0 { + return nil, ErrCertNotFound + } + if service, ok := m.registeredDomains[domain]; ok && service != "" && !m.certMatchesServiceDirectoryLocked(cert, service) { return nil, ErrCertNotFound diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index f6f3609..c65f62e 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -606,3 +606,38 @@ func TestSANCertManager_GetCertificate_WaiterRefusesStillMismatchedCert(t *testi assert.ErrorIs(t, r.err, ErrCertNotFound) assert.Nil(t, r.cert) } + +// Same rule for expiry: after a failed order, the waiter refuses a +// certificate no client would accept rather than moving the failure +// client-side. +func TestSANCertManager_GetCertificate_WaiterRefusesExpiredCert(t *testing.T) { + manager := testSANCertManager(t) + manager.httpObtainer = successfulObtainer(t) + + require.NoError(t, manager.RegisterDomain("app.example.com", "web")) + _, err := manager.adoptCertificate( + testCertResource(t, []string{"app.example.com"}, time.Now().Add(-90*24*time.Hour), time.Now().Add(-time.Hour)), + []string{"app.example.com"}) + require.NoError(t, err) + + inflight := make(chan struct{}) + manager.mu.Lock() + manager.provisioning["_batch_"] = inflight + manager.mu.Unlock() + + type result struct { + cert *tls.Certificate + err error + } + results := make(chan result, 1) + go func() { + cert, err := manager.provisionCertificate(context.Background(), "app.example.com") + results <- result{cert, err} + }() + + close(inflight) + + r := <-results + require.ErrorIs(t, r.err, ErrCertNotFound) + assert.Nil(t, r.cert) +}