diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 8f361ac..be37ad0 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,39 @@ 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 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. + // + // 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, "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 (or mismatched-registered): registered domains fall through + // to synchronous provisioning and dynamic ones to the issuer. } if isRegistered { @@ -517,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() } @@ -704,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() @@ -719,6 +740,17 @@ func (m *SANCertManager) getCertForDomain(domain string) (*tls.Certificate, erro 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 + } + return cert.Certificate, nil } diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index 8eb3cb4..c65f62e 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -460,3 +460,184 @@ 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 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 + + requests := []string{} + manager.SetDynamicCertRequester(func(domain, service string) { + requests = append(requests, domain) + }) + + manager.SetDynamicDomains("tenants", []string{"shop.tenant.net"}) + held, err := manager.adoptCertificate( + 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("tenants", LetsEncryptProduction) + + served, err := manager.GetCertificate(&tls.ClientHelloInfo{ServerName: "shop.tenant.net"}) + + require.NoError(t, err) + 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) +} + +// 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) +} + +// 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) +}