handshake: avoid expired wildcard cache match on demand

certificates_test.go

@@ -27,27 +27,27 @@ func TestUnexportedGetCertificate(t *testing.T) {
 	cfg := &Config{Logger: defaultTestLogger, certCache: certCache}
 
 	// When cache is empty
-	if _, matched, defaulted := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "example.com"}); matched || defaulted {
+	if _, matched, defaulted, _ := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "example.com"}); matched || defaulted {
 		t.Errorf("Got a certificate when cache was empty; matched=%v, defaulted=%v", matched, defaulted)
 	}
 
 	// When cache has one certificate in it
 	firstCert := Certificate{Names: []string{"example.com"}}
 	certCache.cache["0xdeadbeef"] = firstCert
 	certCache.cacheIndex["example.com"] = []string{"0xdeadbeef"}
-	if cert, matched, defaulted := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "example.com"}); !matched || defaulted || cert.Names[0] != "example.com" {
+	if cert, matched, defaulted, _ := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "example.com"}); !matched || defaulted || cert.Names[0] != "example.com" {
 		t.Errorf("Didn't get a cert for 'example.com' or got the wrong one: %v, matched=%v, defaulted=%v", cert, matched, defaulted)
 	}
 
 	// When retrieving wildcard certificate
 	certCache.cache["0xb01dface"] = Certificate{Names: []string{"*.example.com"}}
 	certCache.cacheIndex["*.example.com"] = []string{"0xb01dface"}
-	if cert, matched, defaulted := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "sub.example.com"}); !matched || defaulted || cert.Names[0] != "*.example.com" {
+	if cert, matched, defaulted, _ := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "sub.example.com"}); !matched || defaulted || cert.Names[0] != "*.example.com" {
 		t.Errorf("Didn't get wildcard cert for 'sub.example.com' or got the wrong one: %v, matched=%v, defaulted=%v", cert, matched, defaulted)
 	}
 
 	// When no certificate matches and SNI is provided, return no certificate (should be TLS alert)
-	if cert, matched, defaulted := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "nomatch"}); matched || defaulted {
+	if cert, matched, defaulted, _ := cfg.getCertificateFromCache(&tls.ClientHelloInfo{ServerName: "nomatch"}); matched || defaulted {
 		t.Errorf("Expected matched=false, defaulted=false; but got matched=%v, defaulted=%v (cert: %v)", matched, defaulted, cert)
 	}
 }

handshake.go

@@ -106,12 +106,14 @@ func (cfg *Config) GetCertificateWithContext(ctx context.Context, clientHello *t
 // If a match is found, matched will be true. If no matches are found, matched
 // will be false and a "default" certificate will be returned with defaulted
 // set to true. If defaulted is false, then no certificates were available.
+// If matched or defaulted is true, matchedName is the cache lookup name that
+// selected the certificate.
 //
 // The logic in this function is adapted from the Go standard library,
 // which is by the Go Authors.
 //
 // This function is safe for concurrent use.
-func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Certificate, matched, defaulted bool) {
+func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Certificate, matched, defaulted bool, matchedName string) {
 	name := normalizedName(hello.ServerName)
 
 	if name == "" {
@@ -120,6 +122,7 @@ func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Cer
 			addr := localIPFromConn(hello.Conn)
 			cert, matched = cfg.selectCert(hello, addr)
 			if matched {
+				matchedName = addr
 				return
 			}
 		}
@@ -129,13 +132,15 @@ func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Cer
 			normDefault := normalizedName(cfg.DefaultServerName)
 			cert, defaulted = cfg.selectCert(hello, normDefault)
 			if defaulted {
+				matchedName = normDefault
 				return
 			}
 		}
 	} else {
 		// if SNI is specified, try an exact match first
 		cert, matched = cfg.selectCert(hello, name)
 		if matched {
+			matchedName = name
 			return
 		}
 
@@ -147,6 +152,7 @@ func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Cer
 			candidate := strings.Join(labels, ".")
 			cert, matched = cfg.selectCert(hello, candidate)
 			if matched {
+				matchedName = candidate
 				return
 			}
 		}
@@ -162,6 +168,7 @@ func (cfg *Config) getCertificateFromCache(hello *tls.ClientHelloInfo) (cert Cer
 		normFallback := normalizedName(cfg.FallbackServerName)
 		cert, defaulted = cfg.selectCert(hello, normFallback)
 		if defaulted {
+			matchedName = normFallback
 			return
 		}
 	}
@@ -273,20 +280,33 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client
 	logger := logWithRemote(cfg.Logger.Named("handshake"), hello)
 
 	// First check our in-memory cache to see if we've already loaded it
-	cert, matched, defaulted := cfg.getCertificateFromCache(hello)
+	cert, matched, defaulted, matchedName := cfg.getCertificateFromCache(hello)
+	var loadExactFromStorage bool
 	if matched {
 		logger.Debug("matched certificate in cache",
 			zap.Strings("subjects", cert.Names),
 			zap.Bool("managed", cert.managed),
 			zap.Time("expiration", expiresAt(cert.Leaf)),
 			zap.String("hash", cert.hash))
 		if cert.managed && cfg.OnDemand != nil && loadOrObtainIfNecessary {
-			// On-demand certificates are maintained in the background, but
-			// maintenance is triggered by handshakes instead of by a timer
-			// as in maintain.go.
-			return cfg.optionalMaintenance(ctx, cfg.Logger.Named("on_demand"), cert, hello)
+			serverName := normalizedName(hello.ServerName)
+			if cert.Expired() && matchedName != serverName && strings.Contains(matchedName, "*") {
+				logger.Debug("cached wildcard certificate is expired; trying exact certificate for SNI",
+					zap.String("server_name", serverName),
+					zap.String("matched_identifier", matchedName),
+					zap.Strings("subjects", cert.Names),
+					zap.Time("expiration", expiresAt(cert.Leaf)))
+				loadExactFromStorage = true
+			} else {
+				// On-demand certificates are maintained in the background, but
+				// maintenance is triggered by handshakes instead of by a timer
+				// as in maintain.go.
+				return cfg.optionalMaintenance(ctx, cfg.Logger.Named("on_demand"), cert, hello)
+			}
+		}
+		if !loadExactFromStorage {
+			return cert, nil
 		}
-		return cert, nil
 	}
 
 	name, err := cfg.getNameFromClientHello(hello)
@@ -383,7 +403,7 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client
 
 	if loadDynamically && loadOrObtainIfNecessary {
 		// Check to see if we have one on disk
-		loadedCert, err := cfg.loadCertFromStorage(ctx, logger, hello)
+		loadedCert, err := cfg.loadCertFromStorage(ctx, logger, hello, loadExactFromStorage)
 		if err == nil {
 			return loadedCert, nil
 		}
@@ -420,14 +440,16 @@ func (cfg *Config) getCertDuringHandshake(ctx context.Context, hello *tls.Client
 }
 
 // loadCertFromStorage loads the certificate for name from storage and maintains it
-// (as this is only called with on-demand TLS enabled).
-func (cfg *Config) loadCertFromStorage(ctx context.Context, logger *zap.Logger, hello *tls.ClientHelloInfo) (Certificate, error) {
+// (as this is only called with on-demand TLS enabled). If exactOnly is true, it
+// does not fall back to loading a wildcard certificate when the exact name is not
+// in storage.
+func (cfg *Config) loadCertFromStorage(ctx context.Context, logger *zap.Logger, hello *tls.ClientHelloInfo, exactOnly bool) (Certificate, error) {
 	name, err := cfg.getNameFromClientHello(hello)
 	if err != nil {
 		return Certificate{}, err
 	}
 	loadedCert, err := cfg.CacheManagedCertificate(ctx, name)
-	if errors.Is(err, fs.ErrNotExist) {
+	if errors.Is(err, fs.ErrNotExist) && !exactOnly {
 		// If no exact match, try a wildcard variant, which is something we can still use
 		labels := strings.Split(name, ".")
 		labels[0] = "*"
@@ -563,7 +585,7 @@ func (cfg *Config) obtainOnDemandCertificate(ctx context.Context, hello *tls.Cli
 	err = cfg.ObtainCertAsync(ctx, name)
 	if err == nil {
 		// load from storage while others wait to make the op as atomic as possible
-		cert, err = cfg.loadCertFromStorage(ctx, log, hello)
+		cert, err = cfg.loadCertFromStorage(ctx, log, hello, false)
 		if err != nil {
 			log.Error("loading newly-obtained certificate from storage", zap.String("server_name", name), zap.Error(err))
 		}

handshake_test.go

@@ -15,9 +15,15 @@ package certmagic
 
 import (
 	"context"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
 	"crypto/tls"
 	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
 	"errors"
+	"math/big"
 	"net"
 	"runtime"
 	"strings"
@@ -113,6 +119,87 @@ func TestGetCertificate(t *testing.T) {
 	}
 }
 
+func TestGetCertDuringHandshakeExpiredWildcardCacheObtainsExact(t *testing.T) {
+	const (
+		wildcardName = "*.example.com"
+		serverName   = "test.example.com"
+	)
+
+	ctx := context.Background()
+	certCache := &Cache{
+		cache:      make(map[string]Certificate),
+		cacheIndex: make(map[string][]string),
+		logger:     defaultTestLogger,
+	}
+
+	issuer := &handshakeTestIssuer{t: t}
+	cfg := newWithCache(certCache, Config{
+		Issuers:  []Issuer{issuer},
+		Storage:  &FileStorage{Path: t.TempDir()},
+		Logger:   defaultTestLogger,
+		OnDemand: &OnDemandConfig{},
+		OCSP:     OCSPConfig{DisableStapling: true},
+	})
+	certCache.options = CacheOptions{
+		GetConfigForCert:   func(Certificate) (*Config, error) { return cfg, nil },
+		RenewCheckInterval: DefaultRenewCheckInterval,
+		OCSPCheckInterval:  DefaultOCSPCheckInterval,
+		Logger:             defaultTestLogger,
+	}
+
+	now := time.Now()
+	wildcardKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating wildcard key: %v", err)
+	}
+	wildcardCertPEM := mustIssueCertificateForPublicKey(t, wildcardKey.Public(), []string{wildcardName}, now.Add(-4*time.Hour), now.Add(-2*time.Hour))
+	wildcardKeyPEM, err := PEMEncodePrivateKey(wildcardKey)
+	if err != nil {
+		t.Fatalf("encoding wildcard private key: %v", err)
+	}
+
+	err = cfg.saveCertResource(ctx, issuer, CertificateResource{
+		SANs:           []string{wildcardName},
+		CertificatePEM: wildcardCertPEM,
+		PrivateKeyPEM:  wildcardKeyPEM,
+	})
+	if err != nil {
+		t.Fatalf("saving wildcard certificate resource: %v", err)
+	}
+
+	wildcardCert, err := cfg.CacheManagedCertificate(ctx, wildcardName)
+	if err != nil {
+		t.Fatalf("caching wildcard certificate: %v", err)
+	}
+	if !wildcardCert.Expired() {
+		t.Fatalf("test wildcard certificate should be expired")
+	}
+
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("creating listener: %v", err)
+	}
+	defer l.Close()
+	conn, err := net.Dial("tcp", l.Addr().String())
+	if err != nil {
+		t.Fatalf("creating connection: %v", err)
+	}
+	defer conn.Close()
+
+	hello := &tls.ClientHelloInfo{ServerName: serverName, Conn: conn}
+	cert, err := cfg.getCertDuringHandshake(ctx, hello, true)
+	if err != nil {
+		t.Fatalf("getting certificate during handshake: %v", err)
+	}
+
+	if got := cert.Names[0]; got != serverName {
+		t.Fatalf("expected exact certificate for %s, got %s", serverName, got)
+	}
+	if got := issuer.issued.Load(); got != 1 {
+		t.Fatalf("expected exact certificate to be issued once, got %d", got)
+	}
+}
+
 // 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
@@ -227,3 +314,50 @@ func TestGetCertDuringHandshakeWaiterErrorPropagation(t *testing.T) {
 		t.Errorf("expected exactly 1 decision-func call (leader only), got %d", got)
 	}
 }
+
+type handshakeTestIssuer struct {
+	t      *testing.T
+	issued atomic.Int32
+}
+
+func (i *handshakeTestIssuer) Issue(ctx context.Context, csr *x509.CertificateRequest) (*IssuedCertificate, error) {
+	i.t.Helper()
+	i.issued.Add(1)
+
+	now := time.Now()
+	certPEM := mustIssueCertificateForPublicKey(i.t, csr.PublicKey, csr.DNSNames, now.Add(-time.Hour), now.Add(time.Hour))
+	return &IssuedCertificate{Certificate: certPEM}, nil
+}
+
+func (i *handshakeTestIssuer) IssuerKey() string { return "handshake_test" }
+
+func mustIssueCertificateForPublicKey(t *testing.T, publicKey any, dnsNames []string, notBefore, notAfter time.Time) []byte {
+	t.Helper()
+
+	signer, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating issuer key: %v", err)
+	}
+
+	commonName := ""
+	if len(dnsNames) > 0 {
+		commonName = dnsNames[0]
+	}
+	tmpl := &x509.Certificate{
+		SerialNumber:          big.NewInt(time.Now().UnixNano()),
+		Subject:               pkix.Name{CommonName: commonName},
+		DNSNames:              dnsNames,
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+		BasicConstraintsValid: true,
+	}
+
+	der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, publicKey, signer)
+	if err != nil {
+		t.Fatalf("creating certificate: %v", err)
+	}
+
+	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})
+}