From 0c049a36afc07936d5f771739d60c91b4296c53a Mon Sep 17 00:00:00 2001 From: Paul Buonopane Date: Mon, 8 Jun 2026 00:24:54 -0400 Subject: [PATCH] handshake: propagate leader load/obtain outcome to all waiters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the goroutine leading a certificate load/obtain for a name fails (e.g. an on-demand permission denial) or succeeds but the cert is evicted from the cache before waiters re-look it up, waiters recursively re-enter the wait queue instead of receiving the leader's outcome. Under sustained handshake load for uncertificated names, these recursing goroutines accumulate without bound (observed at 30k+ goroutines, recursion depth 48), each pinning live TLS handshake state — multi-GB memory growth within minutes. Propagate the leader's returned certificate or error to all waiters via the existing certLoadWaiter fields (previously populated only on the external-cert-manager path). Waiters now return the leader's outcome directly; the recursive fallback remains only for the rare empty-cert+nil-error case. --- handshake.go | 16 +++--- handshake_test.go | 121 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 6 deletions(-) diff --git a/handshake.go b/handshake.go index a227015b..2b1b7a9c 100644 --- a/handshake.go +++ b/handshake.go @@ -269,7 +269,7 @@ func DefaultCertificateSelector(hello *tls.ClientHelloInfo, choices []Certificat // An error will be returned if and only if no certificate is available. // // This function is safe for concurrent use. -func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.ClientHelloInfo, loadOrObtainIfNecessary bool) (Certificate, error) { +func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.ClientHelloInfo, loadOrObtainIfNecessary bool) (retCert Certificate, retErr error) { logger := logWithRemote(cfg.Logger.Named("handshake"), hello) // First check our in-memory cache to see if we've already loaded it @@ -314,11 +314,9 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client timeout.Stop() } - // If the leader got a result from an external cert manager, use it - // directly — these certs are not added to the cache, so a recursive - // cache lookup would miss. For cached certs (on-demand, managed), - // the waiter result will be empty and we fall through to the - // original recursive lookup. + // Use the leader's outcome directly; fall back to the recursive + // cache lookup only if it finished with neither a cert nor an + // error. if !waiter.cert.Empty() || waiter.err != nil { return waiter.cert, waiter.err } @@ -334,6 +332,12 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client // unblock others and clean up when we're done defer func() { certLoadWaitChansMu.Lock() + // Propagate our outcome to waiters so they don't recursively + // re-enter the wait queue and pile up; don't overwrite a result + // already set by the external-manager path. + if waiter.cert.Empty() && waiter.err == nil { + waiter.cert, waiter.err = retCert, retErr + } close(waiter.done) delete(certLoadWaitChans, name) certLoadWaitChansMu.Unlock() diff --git a/handshake_test.go b/handshake_test.go index 6e4bd777..5adc8c37 100644 --- a/handshake_test.go +++ b/handshake_test.go @@ -14,10 +14,16 @@ package certmagic import ( + "context" "crypto/tls" "crypto/x509" + "errors" "net" + "runtime" + "strings" + "sync/atomic" "testing" + "time" ) func TestGetCertificate(t *testing.T) { @@ -106,3 +112,118 @@ func TestGetCertificate(t *testing.T) { t.Errorf("Expected IP cert, got: %v", cert) } } + +// TestGetCertDuringHandshakeWaiterErrorPropagation verifies that when the +// "leader" goroutine loading/obtaining a certificate for a name fails (e.g. +// an on-demand permission denial), the failure is propagated directly to all +// goroutines waiting on it. Without propagation, each waiter recursively +// re-enters the wait queue and eventually performs its own load/obtain +// attempt; under sustained handshake load for uncertificated names those +// recursing goroutines accumulate without bound, each pinning live TLS +// handshake state. +func TestGetCertDuringHandshakeWaiterErrorPropagation(t *testing.T) { + const serverName = "denied.example.com" + + c := &Cache{ + cache: make(map[string]Certificate), + cacheIndex: make(map[string][]string), + logger: defaultTestLogger, + } + + denyErr := errors.New("on-demand permission denied") + release := make(chan struct{}) + var decisionCalls atomic.Int32 + cfg := &Config{ + Logger: defaultTestLogger, + certCache: c, + OnDemand: &OnDemandConfig{ + DecisionFunc: func(ctx context.Context, name string) error { + decisionCalls.Add(1) + <-release // hold the leader so waiters queue behind it + return denyErr + }, + }, + } + + l, _ := net.Listen("tcp", "127.0.0.1:0") + defer l.Close() + conn, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatalf("failed to create test connection: %v", err) + } + defer conn.Close() + + hello := &tls.ClientHelloInfo{ServerName: serverName, Conn: conn} + ctx := context.Background() + + const numHandshakes = 5 + results := make(chan error, numHandshakes) + + // leader: takes the load slot for serverName and blocks in DecisionFunc + go func() { + _, err := cfg.getCertDuringHandshake(ctx, hello, true) + results <- err + }() + + // wait until the leader has registered itself in the wait queue + for { + certLoadWaitChansMu.Lock() + _, registered := certLoadWaitChans[serverName] + certLoadWaitChansMu.Unlock() + if registered { + break + } + time.Sleep(time.Millisecond) + } + + // waiters: queue behind the leader + for i := 1; i < numHandshakes; i++ { + go func() { + _, err := cfg.getCertDuringHandshake(ctx, hello, true) + results <- err + }() + } + + // Wait until every handshake goroutine is PARKED — the waiters blocked + // in the wait-queue select and the leader blocked on the release + // channel in DecisionFunc — before releasing the leader. Merely being + // inside getCertDuringHandshake isn't enough: a goroutine still in the + // preamble would miss the wait queue once the leader's cleanup runs + // and become a second leader. + parked := func() (selecting, receiving int) { + buf := make([]byte, 1<<20) + stacks := string(buf[:runtime.Stack(buf, true)]) + for _, g := range strings.Split(stacks, "\n\n") { + if !strings.Contains(g, ").getCertDuringHandshake(") { + continue + } + if strings.Contains(g, "[select]") { + selecting++ + } else if strings.Contains(g, "[chan receive]") { + receiving++ + } + } + return + } + for { + selecting, receiving := parked() + if selecting >= numHandshakes-1 && receiving >= 1 { + break + } + time.Sleep(time.Millisecond) + } + close(release) // leader now fails with denyErr + + for i := 0; i < numHandshakes; i++ { + if err := <-results; err == nil || !strings.Contains(err.Error(), denyErr.Error()) { + t.Errorf("expected deny error, got: %v", err) + } + } + + // The decision func must have been called exactly once: only the leader + // reaches the permission check; waiters receive its propagated error + // rather than recursing into their own load/obtain attempts. + if got := decisionCalls.Load(); got != 1 { + t.Errorf("expected exactly 1 decision-func call (leader only), got %d", got) + } +}