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