fix(san-cert): serve the held certificate while its replacement issues

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
 }
 

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)
+}