From 7add2af9ecdfff7935e1019a1e7d2d25afe70100 Mon Sep 17 00:00:00 2001 From: Thomas Desrosiers <681004+thomasdesr@users.noreply.github.com> Date: Tue, 15 Sep 2026 21:08:19 -0700 Subject: [PATCH] prober: cache CRLs across TLS probes (#21286) The TLS probe fetched and parsed the leaf certificate's CRL on every run. That is fine when the CRL is small, but some CAs publish CRLs of several megabytes: the one for the AWS ACM R2M04 intermediate is about 2.5MB, which at the default 15s interval is a continuous 170kB/s per probed node. Cache parsed CRLs by distribution point URL and reuse each for up to an hour, or until its NextUpdate if that comes first. An hour is the HTTP max-age Let's Encrypt serves on its root CRL. A CRL is cached only after its signature verifies, every use still re-verifies it against the probing leaf's issuer (the cache is keyed by URL alone), and a CRL without a NextUpdate is never cached since it declares no validity window. Concurrent probes fetch through singleflight.DoChanContext to avoid re-fetching a single CRL, with each waiter keeping its own deadline. A caller that missed the cache re-checks it inside the singleflight closure, since singleflight dedupes only calls that overlap. Leaf certificates whose issuer is missing from the presented chain now fail before any fetch. Previously the probe downloaded the CRL and then panicked in CheckSignatureFrom, which the prober recovered and recorded as a probe failure. Also update the TLS probe's doc comments, which said OCSP where the code checks a CRL. Fixes #21310 Signed-off-by: Thomas Desrosiers --- prober/tls.go | 145 +++++++++++++++++++++++++------ prober/tls_test.go | 206 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 323 insertions(+), 28 deletions(-) diff --git a/prober/tls.go b/prober/tls.go index 1247f9502..05c304444 100644 --- a/prober/tls.go +++ b/prober/tls.go @@ -13,7 +13,11 @@ "net/http" "net/netip" "slices" + "sync" "time" + + "tailscale.com/util/mak" + "tailscale.com/util/singleflight" ) const expiresSoon = 7 * 24 * time.Hour // 7 days from now @@ -26,7 +30,7 @@ // // The ProbeFunc connects to a hostPort (host:port string), does a TLS // handshake, verifies that the hostname matches the presented certificate, -// checks certificate validity time and OCSP revocation status. +// checks certificate validity time and CRL revocation status. // // The TLS config is optional and may be nil. func TLS(hostPort string, config *tls.Config) ProbeClass { @@ -59,13 +63,13 @@ func probeTLS(ctx context.Context, config *tls.Config, dialHostPort string) erro defer conn.Close() tlsConnState := conn.(*tls.Conn).ConnectionState() - return validateConnState(ctx, &tlsConnState) + return validateConnState(ctx, defaultCRLCache, &tlsConnState) } // validateConnState verifies certificate validity time in all certificates -// returned by the TLS server and checks OCSP revocation status for the +// returned by the TLS server and checks CRL revocation status for the // leaf cert. -func validateConnState(ctx context.Context, cs *tls.ConnectionState) (returnerr error) { +func validateConnState(ctx context.Context, crls *crlCache, cs *tls.ConnectionState) (returnerr error) { var errs []error defer func() { returnerr = errors.Join(errs...) @@ -117,37 +121,45 @@ func validateConnState(ctx context.Context, cs *tls.ConnectionState) (returnerr return } - err := checkCertCRL(ctx, leafCert.CRLDistributionPoints[0], leafCert, issuerCert) + err := crls.checkCertCRL(ctx, leafCert.CRLDistributionPoints[0], leafCert, issuerCert) if err != nil { errs = append(errs, fmt.Errorf("CRL verification failed for %v: %w", leafCert.Subject, err)) } return } -func checkCertCRL(ctx context.Context, crlURL string, leafCert, issuerCert *x509.Certificate) error { - hreq, err := http.NewRequestWithContext(ctx, "GET", crlURL, nil) - if err != nil { - return fmt.Errorf("could not create CRL GET request: %w", err) +var defaultCRLCache = &crlCache{now: time.Now} + +// crlRefreshInterval caps how long a cached CRL is reused, even when its +// NextUpdate (the x509.RevocationList field giving the deadline for the +// issuer's next CRL) is further out. An hour was chosen based on the +// Cache-Control max-age Let's Encrypt serves on its root CRL. +const crlRefreshInterval = time.Hour + +// crlCache caches parsed CRLs by distribution point URL, so that repeated +// probes do not refetch one on every run. +type crlCache struct { + now func() time.Time + fetch singleflight.Group[string, *x509.RevocationList] + + mu sync.Mutex + crls map[string]crlCacheEntry +} + +type crlCacheEntry struct { + crl *x509.RevocationList + refreshAt time.Time +} + +func (c *crlCache) checkCertCRL(ctx context.Context, crlURL string, leafCert, issuerCert *x509.Certificate) error { + if issuerCert == nil { + return fmt.Errorf("issuer certificate for %v not in presented chain", leafCert.Subject) } - hresp, err := http.DefaultClient.Do(hreq) - if err != nil { - return fmt.Errorf("CRL request failed: %w", err) - } - defer hresp.Body.Close() - if hresp.StatusCode != http.StatusOK { - return fmt.Errorf("crl: non-200 status code from CRL server: %s", hresp.Status) - } - lr := io.LimitReader(hresp.Body, 10<<20) // 10MB - crlB, err := io.ReadAll(lr) + crl, err := c.get(ctx, crlURL, issuerCert) if err != nil { return err } - crl, err := x509.ParseRevocationList(crlB) - if err != nil { - return fmt.Errorf("could not parse CRL: %w", err) - } - if err := crl.CheckSignatureFrom(issuerCert); err != nil { return fmt.Errorf("could not verify CRL signature: %w", err) } @@ -160,3 +172,88 @@ func checkCertCRL(ctx context.Context, crlURL string, leafCert, issuerCert *x509 return nil } + +// get returns the CRL at crlURL, refetching it once the cached copy is due +// for a refresh. A CRL is cached only once it verifies against the fetching +// caller's issuerCert; one with no NextUpdate declares no lifetime and is +// not cached. +func (c *crlCache) get(ctx context.Context, crlURL string, issuerCert *x509.Certificate) (*x509.RevocationList, error) { + if crl, ok := c.cached(crlURL); ok { + return crl, nil + } + + res := <-c.fetch.DoChanContext(ctx, crlURL, func(ctx context.Context) (*x509.RevocationList, error) { + // A caller that missed the cache above can reach here after another + // caller's fetch stored one; singleflight dedupes only overlapping calls. + if crl, ok := c.cached(crlURL); ok { + return crl, nil + } + crl, err := fetchCRL(ctx, crlURL) + if err != nil { + return nil, err + } + if err := crl.CheckSignatureFrom(issuerCert); err != nil { + return nil, fmt.Errorf("could not verify CRL signature: %w", err) + } + if !crl.NextUpdate.IsZero() { + c.store(crlURL, crl) + } + return crl, nil + }) + return res.Val, res.Err +} + +// cached returns the CRL cached for crlURL, if one is present and not yet due +// for a refresh. +func (c *crlCache) cached(crlURL string) (*x509.RevocationList, bool) { + c.mu.Lock() + e, ok := c.crls[crlURL] + c.mu.Unlock() + if !ok || !c.now().Before(e.refreshAt) { + return nil, false + } + return e.crl, true +} + +func (c *crlCache) store(crlURL string, crl *x509.RevocationList) { + now := c.now() + refreshAt := now.Add(crlRefreshInterval) + if crl.NextUpdate.Before(refreshAt) { + refreshAt = crl.NextUpdate + } + + c.mu.Lock() + defer c.mu.Unlock() + for url, e := range c.crls { + if !e.refreshAt.After(now) { + delete(c.crls, url) + } + } + mak.Set(&c.crls, crlURL, crlCacheEntry{crl: crl, refreshAt: refreshAt}) +} + +func fetchCRL(ctx context.Context, crlURL string) (*x509.RevocationList, error) { + hreq, err := http.NewRequestWithContext(ctx, "GET", crlURL, nil) + if err != nil { + return nil, fmt.Errorf("could not create CRL GET request: %w", err) + } + hresp, err := http.DefaultClient.Do(hreq) + if err != nil { + return nil, fmt.Errorf("CRL request failed: %w", err) + } + defer hresp.Body.Close() + if hresp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("crl: non-200 status code from CRL server: %s", hresp.Status) + } + lr := io.LimitReader(hresp.Body, 10<<20) // 10MB + crlB, err := io.ReadAll(lr) + if err != nil { + return nil, err + } + + crl, err := x509.ParseRevocationList(crlB) + if err != nil { + return nil, fmt.Errorf("could not parse CRL: %w", err) + } + return crl, nil +} diff --git a/prober/tls_test.go b/prober/tls_test.go index a32693762..caf910755 100644 --- a/prober/tls_test.go +++ b/prober/tls_test.go @@ -18,8 +18,12 @@ "net/http" "net/http/httptest" "strings" + "sync" + "sync/atomic" "testing" "time" + + "tailscale.com/tstest" ) var leafCert = x509.Certificate{ @@ -119,7 +123,7 @@ func() *x509.Certificate { } { t.Run(tt.name, func(t *testing.T) { cs := &tls.ConnectionState{PeerCertificates: []*x509.Certificate{tt.cert()}} - err := validateConnState(context.Background(), cs) + err := validateConnState(context.Background(), &crlCache{now: time.Now}, cs) if err == nil || !strings.Contains(err.Error(), tt.wantErr) { t.Errorf("unexpected error %q; want %q", err, tt.wantErr) } @@ -129,9 +133,11 @@ func() *x509.Certificate { type CRLServer struct { crlBytes []byte + requests atomic.Int32 // total requests served } func (s *CRLServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { + s.requests.Add(1) if s.crlBytes == nil { w.WriteHeader(http.StatusInternalServerError) return @@ -186,7 +192,15 @@ func parseECKey(t *testing.T, pemPriv string) *ecdsa.PrivateKey { return key } -func TestCRL(t *testing.T) { +// crlTestPKI is a CA that can sign CRLs plus a leaf certificate it issued. +type crlTestPKI struct { + caCert *x509.Certificate + caKey *ecdsa.PrivateKey + leaf *x509.Certificate +} + +func newCRLTestPKI(t *testing.T) crlTestPKI { + t.Helper() // Generate CA key and self-signed CA cert caKey := parseECKey(t, someECDSAKey1) @@ -218,6 +232,39 @@ func TestCRL(t *testing.T) { if err != nil { t.Fatal(err) } + return crlTestPKI{caCert: caCert, caKey: caKey, leaf: leafCertParsed} +} + +// crl signs a CRL from the test CA revoking the given serials as of +// thisUpdate. A zero nextUpdate omits the field and requires a zero +// thisUpdate, since x509.CreateRevocationList rejects a NextUpdate before +// ThisUpdate. +func (p crlTestPKI) crl(t *testing.T, thisUpdate, nextUpdate time.Time, revoked ...*big.Int) []byte { + t.Helper() + rl := x509.RevocationList{ + SignatureAlgorithm: p.caCert.SignatureAlgorithm, + Issuer: p.caCert.Subject, + ThisUpdate: thisUpdate, + NextUpdate: nextUpdate, + Number: big.NewInt(1), + } + for _, serial := range revoked { + rl.RevokedCertificateEntries = append(rl.RevokedCertificateEntries, x509.RevocationListEntry{ + SerialNumber: serial, + RevocationTime: thisUpdate, + ReasonCode: 1, // Key compromise + }) + } + b, err := x509.CreateRevocationList(rand.Reader, &rl, p.caCert, p.caKey) + if err != nil { + t.Fatal(err) + } + return b +} + +func TestCRL(t *testing.T) { + pki := newCRLTestPKI(t) + caCert, caKey, leafCertParsed := pki.caCert, pki.caKey, pki.leaf // Catch no CRL set by Let's Encrypt date. noCRLCert := leafCert @@ -243,7 +290,7 @@ func TestCRL(t *testing.T) { // Create a CRL that revokes the leaf cert using x509.CreateRevocationList now := time.Now() revoked := []x509.RevocationListEntry{{ - SerialNumber: leaf.SerialNumber, + SerialNumber: leafCertParsed.SerialNumber, RevocationTime: now, ReasonCode: 1, // Key compromise }} @@ -325,7 +372,9 @@ func TestCRL(t *testing.T) { crlServer.crlBytes = nil tt.cert.CRLDistributionPoints = []string{} } - err := validateConnState(context.Background(), cs) + // Each subtest needs its own cache: they share one CRL server + // URL, which is the cache key, but swap the bytes it serves. + err := validateConnState(context.Background(), &crlCache{now: time.Now}, cs) if err == nil && tt.wantErr == "" { return @@ -337,3 +386,152 @@ func TestCRL(t *testing.T) { }) } } + +func TestCRLCache(t *testing.T) { + pki := newCRLTestPKI(t) + start := time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC) + + newServer := func(t *testing.T, s *CRLServer) *httptest.Server { + srv := httptest.NewServer(s) + t.Cleanup(srv.Close) + return srv + } + check := func(c *crlCache, srv *httptest.Server) error { + return c.checkCertCRL(t.Context(), srv.URL, pki.leaf, pki.caCert) + } + mustCheck := func(t *testing.T, c *crlCache, srv *httptest.Server) { + t.Helper() + if err := check(c, srv); err != nil { + t.Fatalf("checkCertCRL: %v", err) + } + } + wantRequests := func(t *testing.T, s *CRLServer, want int32) { + t.Helper() + if got := s.requests.Load(); got != want { + t.Errorf("CRL server saw %d requests; want %d", got, want) + } + } + + t.Run("ReusedAcrossProbes", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour))} + srv := newServer(t, s) + + for range 5 { + mustCheck(t, c, srv) + clock.Advance(15 * time.Second) + } + wantRequests(t, s, 1) + }) + + t.Run("RefetchedAfterRefreshInterval", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour))} + srv := newServer(t, s) + + mustCheck(t, c, srv) + clock.Advance(crlRefreshInterval - time.Minute) + mustCheck(t, c, srv) + wantRequests(t, s, 1) + + clock.Advance(2 * time.Minute) + mustCheck(t, c, srv) + wantRequests(t, s, 2) + }) + + t.Run("RefetchedAfterNextUpdate", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(crlRefreshInterval/2))} + srv := newServer(t, s) + + mustCheck(t, c, srv) + clock.Advance(crlRefreshInterval/2 - time.Minute) + mustCheck(t, c, srv) + wantRequests(t, s, 1) + + clock.Advance(2 * time.Minute) + mustCheck(t, c, srv) + wantRequests(t, s, 2) + }) + + t.Run("ConcurrentCallersShareOneFetch", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour))} + srv := newServer(t, s) + + var wg sync.WaitGroup + errs := make([]error, 8) + for i := range errs { + wg.Go(func() { + errs[i] = check(c, srv) + }) + } + wg.Wait() + for i, err := range errs { + if err != nil { + t.Errorf("caller %d: %v", i, err) + } + } + wantRequests(t, s, 1) + }) + + t.Run("RevokedFromCache", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour), pki.leaf.SerialNumber)} + srv := newServer(t, s) + + for range 3 { + err := check(c, srv) + if err == nil || !strings.Contains(err.Error(), "has been revoked on") { + t.Fatalf("unexpected error %q; want revoked", err) + } + clock.Advance(15 * time.Second) + } + wantRequests(t, s, 1) + }) + + t.Run("WrongIssuerNotCached", func(t *testing.T) { + clock := tstest.NewClock(tstest.ClockOpts{Start: start}) + c := &crlCache{now: clock.Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour))} + srv := newServer(t, s) + + // The leaf is not a CA, so it cannot have signed the CRL. + err := c.checkCertCRL(t.Context(), srv.URL, pki.leaf, pki.leaf) + if err == nil || !strings.Contains(err.Error(), "could not verify CRL signature") { + t.Fatalf("unexpected error %q; want signature failure", err) + } + wantRequests(t, s, 1) + + mustCheck(t, c, srv) + wantRequests(t, s, 2) + }) + + t.Run("NoNextUpdateNotCached", func(t *testing.T) { + c := &crlCache{now: tstest.NewClock(tstest.ClockOpts{Start: start}).Now} + s := &CRLServer{crlBytes: pki.crl(t, time.Time{}, time.Time{})} + srv := newServer(t, s) + + for range 3 { + mustCheck(t, c, srv) + } + wantRequests(t, s, 3) + }) + + t.Run("NilIssuerReturnsError", func(t *testing.T) { + c := &crlCache{now: tstest.NewClock(tstest.ClockOpts{Start: start}).Now} + s := &CRLServer{crlBytes: pki.crl(t, start, start.Add(7*24*time.Hour))} + srv := newServer(t, s) + + err := c.checkCertCRL(t.Context(), srv.URL, pki.leaf, nil) + if err == nil || !strings.Contains(err.Error(), "not in presented chain") { + t.Fatalf("unexpected error %q; want missing issuer", err) + } + wantRequests(t, s, 0) + }) +}