Merge pull request #622 from huang195/fix/tlsbridge-leaf-cache-wallclock

Fix: Gate TLS-bridge leaf cache on wall-clock validity (survive suspend)

Author: huang195

authbridge/authlib/tlsbridge/decision.go

@@ -114,7 +114,11 @@ func (s *SkipSet) Add(host string) {
 			delete(s.m, oldestK)
 		}
 	}
-	s.m[host] = now.Add(s.ttl)
+	// .Round(0) strips the monotonic reading so the expiry is a pure wall-clock
+	// time. Contains compares it against time.Now() via the wall clock, so an
+	// entry expires after skipTTL of real time even across a suspend (where the
+	// monotonic clock freezes and would otherwise keep the host skipped longer).
+	s.m[host] = now.Add(s.ttl).Round(0)
 }
 
 func (s *SkipSet) Contains(host string) bool {

authbridge/authlib/tlsbridge/minter.go

@@ -38,6 +38,12 @@ type cacheEntry struct {
 	expires time.Time
 }
 
+// renewBefore is the gap between the cache deadline (now+ttl, when Get
+// re-mints) and the leaf's NotAfter (now+ttl+renewBefore). It gives a
+// connection that grabbed the leaf just before re-mint ample remaining
+// validity, and a window for Get's NotAfter backstop to act.
+const renewBefore = time.Hour
+
 func NewMinter(src CASource, o MinterOpts) *Minter {
 	if o.CacheMax <= 0 {
 		o.CacheMax = 1024
@@ -71,7 +77,15 @@ func (m *Minter) GetCertificateForHost(host string) (*tls.Certificate, error) {
 	defer m.mu.Unlock()
 	if el, ok := m.items[host]; ok {
 		e := el.Value.(*cacheEntry)
-		if time.Now().Before(e.expires) {
+		// Gate freshness on WALL-clock deadlines, not the process monotonic
+		// clock. Across a host suspend / VM pause the monotonic clock freezes
+		// while wall time — and the leaf's x509 validity — keeps advancing, so
+		// a monotonic deadline would keep serving a leaf the client already
+		// rejects as expired. e.expires is monotonic-stripped (.Round(0) below);
+		// the Leaf.NotAfter check is the backstop tied to the cert's real
+		// validity, the only value the client actually verifies.
+		now := time.Now()
+		if now.Before(e.expires) && e.cert.Leaf != nil && now.Before(e.cert.Leaf.NotAfter) {
 			m.ll.MoveToFront(el)
 			return e.cert, nil
 		}
@@ -82,7 +96,10 @@ func (m *Minter) GetCertificateForHost(host string) (*tls.Certificate, error) {
 	if err != nil {
 		return nil, err
 	}
-	el := m.ll.PushFront(&cacheEntry{host: host, cert: cert, expires: time.Now().Add(m.ttl)})
+	// .Round(0) strips the monotonic reading so the deadline is a pure wall-clock
+	// time; comparisons against time.Now() then fall back to the wall clock and
+	// survive suspend (see the cache-hit gate above).
+	el := m.ll.PushFront(&cacheEntry{host: host, cert: cert, expires: time.Now().Add(m.ttl).Round(0)})
 	m.items[host] = el
 	for m.ll.Len() > m.max {
 		back := m.ll.Back()
@@ -102,8 +119,9 @@ func (m *Minter) mint(host string) (*tls.Certificate, error) {
 		SerialNumber: serial,
 		Subject:      pkix.Name{CommonName: host},
 		NotBefore:    time.Now().Add(-time.Minute),
-		// Leaf validity must exceed the cache TTL so a cached leaf never serves past expiry.
-		NotAfter:    time.Now().Add(m.ttl + time.Hour),
+		// Leaf validity outlasts the cache deadline (now+ttl) by renewBefore so
+		// a cached leaf is always re-minted before it can serve past expiry.
+		NotAfter:    time.Now().Add(m.ttl + renewBefore),
 		KeyUsage:    x509.KeyUsageDigitalSignature,
 		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 	}
@@ -116,8 +134,15 @@ func (m *Minter) mint(host string) (*tls.Certificate, error) {
 	if err != nil {
 		return nil, fmt.Errorf("tlsbridge: mint leaf for %s: %w", host, err)
 	}
+	leaf, err := x509.ParseCertificate(der)
+	if err != nil {
+		return nil, fmt.Errorf("tlsbridge: parse minted leaf for %s: %w", host, err)
+	}
 	return &tls.Certificate{
 		Certificate: [][]byte{der, caCert.Raw},
 		PrivateKey:  m.leafKey,
+		// Populate Leaf so Get can gate on the cert's real wall-clock NotAfter
+		// (and the TLS stack avoids re-parsing on each handshake).
+		Leaf: leaf,
 	}, nil
 }

authbridge/authlib/tlsbridge/minter_test.go

@@ -123,3 +123,58 @@ func TestMinter_LRUEvictsOldest(t *testing.T) {
 		t.Errorf("expected most-recent host \"c\" to still be cached")
 	}
 }
+
+// TestMinter_ReMintsWallClockExpiredLeaf exercises the NotAfter backstop: if a
+// cached leaf is ever wall-clock-expired while the cache deadline still reads
+// fresh, Get must gate on the leaf's real NotAfter and re-mint rather than
+// serve a cert the client rejects. (The primary suspend fix — the wall-clock
+// cache deadline — is guarded by TestMinter_CacheDeadlineIsWallClock.)
+func TestMinter_ReMintsWallClockExpiredLeaf(t *testing.T) {
+	m, _ := newTestMinter(t) // LeafTTL=time.Hour, so the cache deadline stays "fresh"
+	c1, err := m.GetCertificateForHost("h.example.com")
+	if err != nil {
+		t.Fatalf("first mint: %v", err)
+	}
+	// mint() must populate Leaf so the cache can gate on real validity.
+	m.mu.Lock()
+	e := m.items["h.example.com"].Value.(*cacheEntry)
+	if e.cert.Leaf == nil {
+		m.mu.Unlock()
+		t.Fatal("minted cert has no Leaf populated")
+	}
+	// Simulate the leaf having aged past its NotAfter while the monotonic cache
+	// deadline did not advance (host was suspended).
+	e.cert.Leaf.NotAfter = time.Now().Add(-time.Minute)
+	m.mu.Unlock()
+
+	c2, err := m.GetCertificateForHost("h.example.com")
+	if err != nil {
+		t.Fatalf("second mint: %v", err)
+	}
+	if &c1.Certificate[0][0] == &c2.Certificate[0][0] {
+		t.Fatal("served a wall-clock-expired cached leaf; expected a re-mint")
+	}
+	if c2.Leaf == nil || !time.Now().Before(c2.Leaf.NotAfter) {
+		t.Fatal("re-minted leaf is not valid")
+	}
+}
+
+// TestMinter_CacheDeadlineIsWallClock guards the actual suspend fix: the cache
+// freshness deadline must carry NO monotonic clock reading, so the freshness
+// check falls back to the wall clock and expires correctly across a host
+// suspend (where the monotonic clock freezes but wall time advances). A
+// monotonic-carrying deadline is exactly what made the cache keep serving a
+// wall-clock-expired leaf. time.Time's == compares the monotonic reading too,
+// so a deadline that still carried one would not equal its .Round(0) form.
+func TestMinter_CacheDeadlineIsWallClock(t *testing.T) {
+	m, _ := newTestMinter(t)
+	if _, err := m.GetCertificateForHost("h.example.com"); err != nil {
+		t.Fatalf("mint: %v", err)
+	}
+	m.mu.Lock()
+	exp := m.items["h.example.com"].Value.(*cacheEntry).expires
+	m.mu.Unlock()
+	if exp != exp.Round(0) {
+		t.Errorf("cache deadline carries a monotonic clock reading; must be wall-clock (.Round(0)) to survive suspend")
+	}
+}