handshake: propagate leader load/obtain outcome to all waiters

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()

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