From 9c4a3dfbdbdd819e96a0cdf8049d32c6e36f1dd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ersin=20KO=C3=87?= Date: Wed, 4 Nov 2026 13:08:22 +0200 Subject: [PATCH 1/2] fix(certmanager): drop stale in-flight handshakes after ForceRenew A handshake that started on the pre-renewal autocert manager republished its leaf into lastCert after ForceRenew had cleared it. Publish only while the handshake's manager is still current. --- certmanager/forcerenew_stale_test.go | 135 +++++++++++++++++++++++++++ certmanager/manager.go | 24 +++-- 2 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 certmanager/forcerenew_stale_test.go diff --git a/certmanager/forcerenew_stale_test.go b/certmanager/forcerenew_stale_test.go new file mode 100644 index 0000000..3c9b18d --- /dev/null +++ b/certmanager/forcerenew_stale_test.go @@ -0,0 +1,135 @@ +package certmanager + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "io" + "log/slog" + "math/big" + "sync" + "testing" + "time" + + "golang.org/x/crypto/acme/autocert" +) + +// gatedCertCache serves one cached cert; the first Get for gateKey blocks until +// released, so a handshake can be held in flight across ForceRenew. +type gatedCertCache struct { + mu sync.Mutex + data map[string][]byte + gateKey string + entered chan struct{} + release chan struct{} + once sync.Once +} + +func (c *gatedCertCache) Get(_ context.Context, key string) ([]byte, error) { + c.mu.Lock() + b, ok := c.data[key] + c.mu.Unlock() + if !ok { + return nil, autocert.ErrCacheMiss + } + if key == c.gateKey && c.entered != nil { + c.once.Do(func() { close(c.entered); <-c.release }) + } + return b, nil +} +func (c *gatedCertCache) Put(_ context.Context, key string, d []byte) error { + c.mu.Lock() + c.data[key] = d + c.mu.Unlock() + return nil +} +func (c *gatedCertCache) Delete(_ context.Context, key string) error { + c.mu.Lock() + delete(c.data, key) + c.mu.Unlock() + return nil +} + +func frEntry(t *testing.T, domain string) []byte { + key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + tmpl := &x509.Certificate{SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: domain}, DNSNames: []string{domain}, + NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(400 * 24 * time.Hour)} + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + kb, _ := x509.MarshalECPrivateKey(key) + return append(pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: kb}), pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})...) +} + +func frHello(domain string) *tls.ClientHelloInfo { + return &tls.ClientHelloInfo{ServerName: domain, SupportedVersions: []uint16{tls.VersionTLS12}, + SignatureSchemes: []tls.SignatureScheme{tls.ECDSAWithP256AndSHA256}, SupportedCurves: []tls.CurveID{tls.CurveP256}, + CipherSuites: []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}} +} + +func frManager(t *testing.T, cache *gatedCertCache, domain string) *Manager { + m := New(domain, "a@example.test", t.TempDir(), false, slog.New(slog.NewTextHandler(io.Discard, nil))) + m.acm.Cache = cache + return m +} + +// TestForceRenew_StaleHandshakeDoesNotRepublish pins that a handshake started +// against the pre-renewal autocert manager cannot restore lastCert after +// ForceRenew cleared it. +func TestForceRenew_StaleHandshakeDoesNotRepublish(t *testing.T) { + const domain = "example.test" + entry := frEntry(t, domain) + + t.Run("control: un-raced handshake publishes", func(t *testing.T) { + c := &gatedCertCache{data: map[string][]byte{domain: entry}, gateKey: domain} + m := frManager(t, c, domain) + if _, err := m.TLSConfig().GetCertificate(frHello(domain)); err != nil { + t.Fatal(err) + } + if m.lastCert == nil { + t.Fatal("lastCert not set by handshake") + } + }) + + t.Run("stale handshake dropped, later handshake publishes", func(t *testing.T) { + c := &gatedCertCache{data: map[string][]byte{domain: entry}, gateKey: domain, entered: make(chan struct{}), release: make(chan struct{})} + m := frManager(t, c, domain) + cfg := m.TLSConfig() + done := make(chan error, 1) + go func() { _, err := cfg.GetCertificate(frHello(domain)); done <- err }() + <-c.entered + if err := m.ForceRenew(context.Background()); err != nil { + t.Fatal(err) + } + close(c.release) + if err := <-done; err != nil { + t.Fatalf("stale handshake: %v", err) + } + m.mu.RLock() + stale := m.lastCert != nil + m.mu.RUnlock() + if stale { + t.Fatal("stale handshake republished the pre-renewal leaf") + } + if info := m.Info(); info.Issuer != "" { + t.Fatalf("Info reports a certificate after ForceRenew: %+v", info) + } + + // A handshake on the renewed manager still publishes. + c.mu.Lock() + c.data[domain] = entry + c.mu.Unlock() + if _, err := cfg.GetCertificate(frHello(domain)); err != nil { + t.Fatal(err) + } + if m.Info().Issuer == "" { + t.Fatal("post-renewal handshake did not publish") + } + }) +} diff --git a/certmanager/manager.go b/certmanager/manager.go index 79a7efc..2e1f82a 100644 --- a/certmanager/manager.go +++ b/certmanager/manager.go @@ -86,19 +86,19 @@ func (m *Manager) TLSConfig() *tls.Config { // resolves the autocert manager per handshake so a ForceRenew takes // effect on listeners that were configured before it. tlsCfg.GetCertificate = func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - cert, err := m.current().GetCertificate(hello) + // F003: pin the autocert manager this handshake runs against. A + // ForceRenew that lands mid-handshake swaps the manager and clears + // lastCert; the stale handshake must not republish the old leaf. + acm := m.current() + cert, err := acm.GetCertificate(hello) if err != nil { return nil, err } if cert != nil && cert.Leaf != nil { - m.mu.Lock() - m.lastCert = cert.Leaf - m.mu.Unlock() + m.publishLeaf(acm, cert.Leaf) } else if cert != nil && len(cert.Certificate) > 0 { if parsed, parseErr := x509.ParseCertificate(cert.Certificate[0]); parseErr == nil { - m.mu.Lock() - m.lastCert = parsed - m.mu.Unlock() + m.publishLeaf(acm, parsed) } } return cert, nil @@ -117,6 +117,16 @@ func (m *Manager) HTTPHandler(fallback http.Handler) http.Handler { }) } +// publishLeaf records leaf as the last served certificate, but only while acm +// is still the active autocert manager. +func (m *Manager) publishLeaf(acm *autocert.Manager, leaf *x509.Certificate) { + m.mu.Lock() + defer m.mu.Unlock() + if m.acm == acm { + m.lastCert = leaf + } +} + // Info returns information about the currently active certificate. func (m *Manager) Info() *CertInfo { m.mu.RLock() From 30ce9fd808b89984432f24e683e16b979b27adf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ersin=20KO=C3=87?= Date: Wed, 4 Nov 2026 13:09:50 +0200 Subject: [PATCH 2/2] chore: renumber audit finding F003 -> F54 to avoid ID collision --- certmanager/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certmanager/manager.go b/certmanager/manager.go index 2e1f82a..01c2be2 100644 --- a/certmanager/manager.go +++ b/certmanager/manager.go @@ -86,7 +86,7 @@ func (m *Manager) TLSConfig() *tls.Config { // resolves the autocert manager per handshake so a ForceRenew takes // effect on listeners that were configured before it. tlsCfg.GetCertificate = func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - // F003: pin the autocert manager this handshake runs against. A + // F54: pin the autocert manager this handshake runs against. A // ForceRenew that lands mid-handshake swaps the manager and clears // lastCert; the stale handshake must not republish the old leaf. acm := m.current()