diff --git a/pkg/auth/jwks/jwks.go b/pkg/auth/jwks/jwks.go new file mode 100644 index 0000000000..0fc5961144 --- /dev/null +++ b/pkg/auth/jwks/jwks.go @@ -0,0 +1,494 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package jwks provides a shared JWKS fetch-and-cache primitive used by every +// component that resolves verification keys over HTTP (pkg/auth's +// TokenValidator and the auth server's token-exchange validator). +// +// Fetcher consolidates machinery that previously existed as two diverging +// implementations: URL validation (ValidateJWKSURL), per-client response-body +// capping (limitedBodyTransport), lazy registration with refresh-on-retry, +// stale-on-error caching, rate-limited refresh on unknown key IDs, and a +// fetch-failure backoff gate before the first successful fetch. +// +// Stale-on-error requires no extra caching layer here: httprc only stores a +// resource's value after a successful transform, jwk.Cache.Lookup returns the +// last stored set, and a failed Refresh leaves that set in place — so a +// transient outage at an endpoint that has already been reached once never +// surfaces as a lookup failure. +package jwks + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/http" + "sync" + "time" + + "github.com/lestrrat-go/httprc/v3" + "github.com/lestrrat-go/jwx/v3/jwk" + + "github.com/stacklok/toolhive/pkg/networking" +) + +const ( + // DefaultBodyLimit caps every JWKS response body at 1 MiB. This prevents + // resource exhaustion from unexpectedly large responses: the JWKS fetch is + // handed to jwx's jwk.Cache, which has no equivalent cap of its own + // (httprc.MaxBufferSize is ~1000 MiB, and its transformer does an + // unbounded io.ReadAll under that ceiling before parsing). + DefaultBodyLimit = 1 << 20 + + // DefaultMaxKeys caps the number of keys accepted from a JWKS to prevent + // CPU amplification from a hostile endpoint serving many keys. + DefaultMaxKeys = 100 + + // DefaultFetchFailureBackoff bounds how often EnsureRegistered retries a + // JWKS fetch that has never once succeeded. Key resolution runs before a + // token's signature is checked, so without this an authenticated client + // could force a real outbound fetch to a broken endpoint on every single + // request. + DefaultFetchFailureBackoff = 30 * time.Second + + // DefaultMinKidRefreshInterval bounds how often Lookup forces the cache to + // fetch its JWKS ahead of jwx's own background refresh schedule when a + // token names a kid the cached set doesn't have. Key resolution runs + // before the token's signature is trusted, so without this floor a client + // presenting a syntactically valid token that merely names a made-up kid + // could force a fresh fetch on every attempt. + DefaultMinKidRefreshInterval = 30 * time.Second + + // DefaultRegistrationTimeout bounds the initial registration's ready-wait: + // Register blocks until the resource's first fetch completes, so without a + // budget a slow or broken endpoint would block callers for the full + // context duration. + DefaultRegistrationTimeout = 10 * time.Second +) + +// Fetcher fetches, caches, and refreshes one issuer's (or validator's) JWKS. +// +// Each Fetcher MUST have its own jwk.Cache — never share a cache across +// Fetcher instances. A cache per Fetcher, rather than one shared across every +// configured issuer, is what makes two issuers resolving to the same jwks_url +// (e.g. two Microsoft Entra v1 tenants, which share one tenant-independent +// JWKS endpoint) a non-event: httprc keys a cached resource by URL alone and +// only honors jwk.WithHTTPClient on a URL's first Register call, so a shared +// cache would have the second such issuer silently inherit the first one's +// *http.Client — defeating InsecureAllowHTTP/AllowPrivateIPs's per-issuer +// guarantee for it. Splitting the cache per Fetcher makes that collision +// unrepresentable instead of guarding against it. +// +// A Fetcher is safe for concurrent use. All JWKS objects retrieved through it +// should be treated read-only, as they are shared among all consumers and the +// underlying jwk.Cache. No key material is ever logged — errors report key +// counts and key IDs only. +type Fetcher struct { + // Configuration, immutable after construction. + httpClient *http.Client // dedicated to this Fetcher; see buildHTTPClient + insecureAllowHTTP bool + allowPrivateIPs bool + caBundlePath string + caBundleUsesSystemRoots bool + authTokenFile string + timeout time.Duration // 0 = networking.HttpClientBuilder default + disableKeepAlives bool + sameHostRedirects bool + workers int // 0 = httprc default worker pool + bodyLimit int64 + maxKeys int + refreshInterval time.Duration // 0 = derive from response headers + fetchFailureBackoff time.Duration + minKidRefreshInterval time.Duration + registrationTimeout time.Duration + + // cache is this Fetcher's own jwk.Cache — see the type doc comment for + // why it must never be shared. + cache *jwk.Cache + + mu sync.Mutex + // fetched is true once at least one JWKS fetch has succeeded since + // process start, for fetchedURL. Until then, EnsureRegistered forces a + // synchronous fetch on every call, since there is no cached value yet to + // fall back on and jwx's own background schedule offers no way to wait + // for its result. + fetched bool + // fetchedURL is the URL EnsureRegistered last succeeded for. A call for a + // different URL (e.g. after OIDC discovery resolves a new jwks_uri) is + // treated as a first fetch for that URL: it must actually register the + // new resource, not be short-circuited by the previous URL's success. + fetchedURL string + // lastKidRefresh is the last time Lookup forced a fetch for an unknown + // kid; see minKidRefreshInterval. + lastKidRefresh time.Time + // fetchFailedAt and fetchErr gate retries once registered but never + // fetched: key resolution runs before signature verification, so without + // this an authenticated client can otherwise drive one real outbound fetch + // per request just by naming an issuer whose endpoint is down. fetchErr is + // served directly while the gate is closed, so the caller still gets a + // specific error instead of a generic "try later". Not consulted once + // fetched is true: a healthy issuer, or one serving stale-but-valid keys + // through a later background-refresh failure, must never be gated. + fetchFailedAt time.Time + fetchErr error +} + +// NewFetcher creates a Fetcher with its own HTTP client (built from the given +// options) and its own jwk.Cache running its own background worker pool for +// the life of the process. +// +// ctx is used only to start the cache's background worker pool; +// context.Background() semantics apply — there is no per-call context to root +// it in, and the pool is meant to outlive any single call. +func NewFetcher(ctx context.Context, opts ...Option) (*Fetcher, error) { + f := &Fetcher{ + bodyLimit: DefaultBodyLimit, + maxKeys: DefaultMaxKeys, + fetchFailureBackoff: DefaultFetchFailureBackoff, + minKidRefreshInterval: DefaultMinKidRefreshInterval, + registrationTimeout: DefaultRegistrationTimeout, + } + for _, opt := range opts { + opt(f) + } + + if f.httpClient == nil { + client, err := f.buildHTTPClient() + if err != nil { + return nil, err + } + f.httpClient = client + } + + // One jwk.Cache per Fetcher (see the type doc comment for why), each + // running its own background worker pool. WithWorkers caps that pool — + // jwx's cache via httprc defaults to five workers, roughly three + // goroutines per resource including its controller loop and wait-group + // waiter. + var httprcOpts []httprc.NewClientOption + if f.workers > 0 { + httprcOpts = append(httprcOpts, httprc.WithWorkers(f.workers)) + } + cache, err := jwk.NewCache(ctx, httprc.NewClient(httprcOpts...)) + if err != nil { + return nil, fmt.Errorf("failed to create JWKS cache: %w", err) + } + f.cache = cache + + return f, nil +} + +// applyCABundle configures the builder's CA trust mode: pinned-only when +// WithCABundle was used, additive to the system roots when +// WithSystemRootsPlusCABundle was used. +func applyCABundle(builder *networking.HttpClientBuilder, f *Fetcher) { + if f.caBundlePath == "" { + return + } + if f.caBundleUsesSystemRoots { + builder.WithSystemRootsPlusCABundle(f.caBundlePath) + } else { + builder.WithCABundle(f.caBundlePath) + } +} + +// buildHTTPClient constructs the Fetcher's dedicated HTTP client from its +// configuration. Deliberately networking.NewHttpClientBuilder(), not +// NewHostScopedClientBuilder: that helper ORs INSECURE_DISABLE_URL_VALIDATION +// and an auto-localhost exemption into BOTH the HTTP-scheme and private-IP +// gates, so an unrelated env var — or an issuer that merely happens to be on +// localhost — would silently widen AllowPrivateIPs regardless of what the +// operator set, defeating the point of splitting the two flags per issuer. +func (f *Fetcher) buildHTTPClient() (*http.Client, error) { + builder := networking.NewHttpClientBuilder(). + WithInsecureAllowHTTP(f.insecureAllowHTTP). + WithPrivateIPs(f.allowPrivateIPs) + if f.timeout > 0 { + builder = builder.WithTimeout(f.timeout) + } + if f.disableKeepAlives { + // Keep-alive connections are disabled: this client dials a jwks_url — + // a host taken from an untrusted discovery document — only on a fixed + // refresh schedule plus occasional on-demand refreshes; no hot path + // here to trade the per-dial SSRF check away for. + builder = builder.WithDisableKeepAlives(true) + } + if f.authTokenFile != "" { + builder = builder.WithTokenFromFile(f.authTokenFile) + } + applyCABundle(builder, f) + httpClient, err := builder.Build() + if err != nil { + return nil, fmt.Errorf("failed to build HTTP client: %w", err) + } + if f.sameHostRedirects { + // Guard against a discovery/JWKS redirect hop landing on a different, + // unvetted host — the same policy the transparent proxy data path + // applies to a response derived from an untrusted remote server (see + // SameHostRedirectPolicy's doc comment). + httpClient.CheckRedirect = networking.SameHostRedirectPolicy() + } + if f.bodyLimit > 0 { + // Cap every response body this client reads — the JWKS fetch is + // handed to jwx's jwk.Cache, which has no equivalent cap of its own. + // Wrapped OUTSIDE httpClient.Transport (which Build() always sets — + // see networking's builder) so the private-IP dial guard and + // ValidatingTransport's scheme check still run first, on the inner, + // unwrapped transport. + httpClient.Transport = &limitedBodyTransport{ + base: httpClient.Transport, + max: f.bodyLimit, + } + } + return httpClient, nil +} + +// HTTPClient returns the Fetcher's dedicated HTTP client: the client every +// JWKS fetch goes through, carrying this Fetcher's own transport policy +// (scheme and private-IP guards, CA trust, body cap). Callers that need an +// auxiliary request to the same issuer under the same policy — most notably +// OIDC discovery to resolve the jwks_url — must use this client, so their +// requests are guarded by exactly the policy the JWKS fetch is guarded by. +func (f *Fetcher) HTTPClient() *http.Client { + return f.httpClient +} + +// EnsureRegistered registers jwksURL with the Fetcher's cache, fetching it for +// the first time if needed. It is called lazily by Lookup and KeySet to avoid +// blocking construction. +// +// The JWKS fetch is retried until one succeeds (fetched stays false +// otherwise), gated by fetchFailureBackoff: key resolution runs before the +// token's signature is checked, so without this gate an authenticated client +// could force a real outbound attempt on every request to an endpoint that is +// down. The gate only applies before the first successful fetch — once +// fetched is true, a healthy issuer or one serving stale-but-valid keys +// through a later refresh failure is unaffected. While closed, the last error +// is replayed directly rather than retried. +func (f *Fetcher) EnsureRegistered(ctx context.Context, jwksURL string) error { + f.mu.Lock() + defer f.mu.Unlock() + + if f.fetched && f.fetchedURL == jwksURL { + return nil + } + if time.Since(f.fetchFailedAt) < f.fetchFailureBackoff { + return f.fetchErr + } + + if err := f.registerOrRefresh(ctx, jwksURL); err != nil { + f.fetchErr = err + f.fetchFailedAt = time.Now() + return err + } + f.fetched = true + f.fetchedURL = jwksURL + f.fetchErr = nil + return nil +} + +// registerOrRefresh performs the actual registration/fetch attempt for +// jwksURL; EnsureRegistered holds f.mu across this call, single-flighting it +// so concurrent callers don't pile up N redundant fetches. +// +// Whether jwksURL is already registered is asked of f.cache.IsRegistered +// directly, never remembered in a field: Register's own registration step is a +// channel send to the cache's backend goroutine and can fail after enqueue but +// before receipt (context deadline), in which case nothing was actually +// registered. A locally remembered "we called Register" flag can't distinguish +// that from "registered, only the fetch failed", and would wrongly keep +// retrying via Refresh — which errors on a URL the cache never heard of — +// forever after. Asking the cache directly is authoritative either way. +// +// IsRegistered makes no network call but isn't free: it's a round-trip over +// that same channel, so it blocks if the backend is busy and returns false +// (not an error) on context expiry. Its own timeout below matters for that +// reason — a false from an expired context just routes to Register, whose +// "already registered" error is transient and absorbed below. +func (f *Fetcher) registerOrRefresh(ctx context.Context, jwksURL string) error { + // Detach from the caller's request context throughout this function: + // net/http cancels ctx when the client disconnects, and this runs before + // the token's signature is even checked, so an aborted connection must not + // cut off work other in-flight validations are waiting on (mu, held by the + // caller), nor let repeating the abort drive unbounded outbound requests + // to the JWKS endpoint. + detached := context.WithoutCancel(ctx) + + // Validate the JWKS URL in the single choke point every registration + // passes through — whether it was hand-configured or just discovered. A + // configured URL may never pass through any discovery step, so checking + // only there would leave hand-configured URLs unvalidated. + if err := ValidateJWKSURL(jwksURL, f.insecureAllowHTTP, f.allowPrivateIPs); err != nil { + return fmt.Errorf("jwks_url %q is invalid: %w", jwksURL, err) + } + + registeredCtx, cancel := context.WithTimeout(detached, f.registrationTimeout) + registered := f.cache.IsRegistered(registeredCtx, jwksURL) + cancel() + + if registered { + // Already registered with this Fetcher's own cache, on a prior call + // whose own fetch never completed successfully — the only way this + // can be true, now that each Fetcher has its own cache. Register + // would error on an already-tracked URL, so retry via Refresh + // instead. + fetchCtx, cancel := context.WithTimeout(detached, f.registrationTimeout) + defer cancel() + if _, err := f.cache.Refresh(fetchCtx, jwksURL); err != nil { + return fmt.Errorf("failed to fetch JWKS: %w", err) + } + return nil + } + + // A newly created httprc.Resource is always scheduled to fetch + // immediately, so Register's own default WithWaitReady(true) blocks on + // that single automatic fetch. An explicit Refresh call right here would + // race it and issue a genuine second outbound request — that's why the + // registered branch above, not this one, is where Refresh is used. + // + // The CA-aware client must be passed per-resource: jwx >= 3.1.0 injects + // its own default client at the resource level when none is given here, + // which takes precedence over any client-level configuration and silently + // drops custom CA support. + fetchCtx, cancel := context.WithTimeout(detached, f.registrationTimeout) + defer cancel() + registerOpts := []jwk.RegisterOption{jwk.WithHTTPClient(f.httpClient)} + if f.refreshInterval > 0 { + registerOpts = append(registerOpts, jwk.WithConstantInterval(f.refreshInterval)) + } + if err := f.cache.Register(fetchCtx, jwksURL, registerOpts...); err != nil { + switch { + case errors.Is(err, httprc.ErrResourceAlreadyExists()): + // Absorbed as non-fatal: the URL is already registered with this + // cache (e.g. a previous attempt registered it before this state + // was tracked), which is exactly the success state EnsureRegistered + // exists to reach. + default: + // Note this includes httprc.ErrNotReady: Register's ready-wait + // only ever ends in nil (fetch succeeded) or the context error + // that interrupted it (see httprc's controller.Add), so a + // not-ready here always means the first fetch did not complete + // within the registration budget — i.e. a failed fetch. Treating + // it as success would strand a never-successfully-fetched + // resource with no retry path other than jwx's own background + // schedule, so it is gated and replayed like any other fetch + // failure instead. + return fmt.Errorf("failed to register JWKS: %w", err) + } + } + return nil +} + +// Lookup returns the key with the given key ID from the cached JWKS, fetching +// it first if this is the first use (see EnsureRegistered). When the kid is +// absent from the cached set — the situation a legitimate key rotation +// produces — Lookup forces an immediate re-fetch ahead of jwx's own background +// refresh schedule and retries once. +// +// The forced refresh is gated by minKidRefreshInterval and single-flighted via +// f.mu: a token naming a kid absent from the cached set runs before signature +// verification, so without the gate a client presenting made-up kids could +// force a fresh fetch on every attempt. +func (f *Fetcher) Lookup(ctx context.Context, jwksURL, kid string) (jwk.Key, error) { + if err := f.EnsureRegistered(ctx, jwksURL); err != nil { + return nil, err + } + + set, err := f.cache.Lookup(ctx, jwksURL) + if err != nil { + return nil, fmt.Errorf("failed to lookup JWKS: %w", err) + } + if err := f.checkKeyCount(set); err != nil { + return nil, err + } + + key, found := set.LookupKeyID(kid) + if found { + return key, nil + } + + // The kid isn't among the keys we have cached — possibly a legitimate + // rotation the cache hasn't caught up with yet (jwx's own background + // refresh floor is 15 minutes by default). Force an immediate re-fetch and + // retry once before giving up. + f.RefreshOnUnknownKid(ctx, jwksURL) + + set, err = f.cache.Lookup(ctx, jwksURL) + if err != nil { + return nil, fmt.Errorf("failed to lookup JWKS: %w", err) + } + if err := f.checkKeyCount(set); err != nil { + return nil, err + } + + key, found = set.LookupKeyID(kid) + if !found { + return nil, fmt.Errorf("key ID %s not found in JWKS", kid) + } + return key, nil +} + +// KeySet returns the whole cached JWKS for jwksURL, fetching it first if this +// is the first use (see EnsureRegistered). Callers that must inspect every key +// (e.g. to bridge the set into another JWT library) use this instead of +// Lookup. +func (f *Fetcher) KeySet(ctx context.Context, jwksURL string) (jwk.Set, error) { + if err := f.EnsureRegistered(ctx, jwksURL); err != nil { + return nil, err + } + + set, err := f.cache.Lookup(ctx, jwksURL) + if err != nil { + return nil, fmt.Errorf("failed to lookup JWKS: %w", err) + } + if err := f.checkKeyCount(set); err != nil { + return nil, err + } + return set, nil +} + +// RefreshOnUnknownKid forces the cache to re-fetch its JWKS immediately, ahead +// of jwx's own background refresh schedule, when a token names a kid the last +// cached JWKS doesn't have. It is used internally by Lookup, and by callers +// that verify signatures over the whole key set themselves (and therefore +// detect an unknown kid outside of Lookup). +// +// Gated by minKidRefreshInterval and single-flighted via f.mu, the same mutex +// EnsureRegistered uses for its own initial fetch: a token naming a kid absent +// from the cached set runs before signature verification, so without the gate +// a client presenting made-up kids could force a fresh fetch on every attempt. +// +// Errors are logged, not returned: the caller has already failed to find the +// kid once and will simply fail again if the refresh didn't produce a usable +// key, which is the correct outcome for a genuinely invalid token. Only the +// kid's absence is logged, never any key material. +func (f *Fetcher) RefreshOnUnknownKid(ctx context.Context, jwksURL string) { + f.mu.Lock() + defer f.mu.Unlock() + + if time.Since(f.lastKidRefresh) < f.minKidRefreshInterval { + return + } + f.lastKidRefresh = time.Now() + + fetchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*f.registrationTimeout) + defer cancel() + if _, err := f.cache.Refresh(fetchCtx, jwksURL); err != nil { + //nolint:gosec // G706: JWKS URL is from server configuration or OIDC discovery + slog.Debug("JWKS refresh on unknown kid failed", "jwks_url", jwksURL, "error", err) + } +} + +// checkKeyCount enforces the maxKeys cap on a fetched set: a hostile endpoint +// serving many keys must not amplify verification CPU. Only counts are +// reported — never key material. +func (f *Fetcher) checkKeyCount(set jwk.Set) error { + if f.maxKeys <= 0 { + return nil + } + if n := set.Len(); n > f.maxKeys { + return fmt.Errorf("JWKS contains too many keys: %d (max %d)", n, f.maxKeys) + } + return nil +} diff --git a/pkg/auth/jwks/jwks_test.go b/pkg/auth/jwks/jwks_test.go new file mode 100644 index 0000000000..234c588df9 --- /dev/null +++ b/pkg/auth/jwks/jwks_test.go @@ -0,0 +1,580 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/lestrrat-go/jwx/v3/jwk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newECKey generates a fresh ECDSA P-256 key and imports its public half as a +// jwk.Key carrying the given key ID. +func newECKey(t *testing.T, kid string) (jwk.Key, *ecdsa.PrivateKey) { + t.Helper() + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + key, err := jwk.Import(&priv.PublicKey) + require.NoError(t, err) + require.NoError(t, key.Set(jwk.KeyIDKey, kid)) + require.NoError(t, key.Set(jwk.AlgorithmKey, "ES256")) + require.NoError(t, key.Set(jwk.KeyUsageKey, "sig")) + return key, priv +} + +// jwksDoc serialises the given public keys into a JWKS JSON document. +func jwksDoc(t *testing.T, keys ...jwk.Key) []byte { + t.Helper() + + set := jwk.NewSet() + for _, key := range keys { + require.NoError(t, set.AddKey(key)) + } + raw, err := json.Marshal(set) + require.NoError(t, err) + return raw +} + +// mutableJWKSServer is a JWKS endpoint whose payload and status can be changed +// mid-test, and which counts how many times it was fetched. +type mutableJWKSServer struct { + *httptest.Server + + hits atomic.Int32 + mu sync.Mutex + body []byte + status int +} + +func newMutableJWKSServer(t *testing.T, initial []byte) *mutableJWKSServer { + t.Helper() + + s := &mutableJWKSServer{body: initial, status: http.StatusOK} + s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + s.hits.Add(1) + s.mu.Lock() + defer s.mu.Unlock() + if s.status != http.StatusOK { + w.WriteHeader(s.status) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(s.body) + })) + t.Cleanup(s.Close) + return s +} + +func (s *mutableJWKSServer) set(body []byte) { + s.mu.Lock() + defer s.mu.Unlock() + s.body = body + s.status = http.StatusOK +} + +func (s *mutableJWKSServer) setStatus(status int) { + s.mu.Lock() + defer s.mu.Unlock() + s.status = status +} + +// newShortFetcher builds a Fetcher with fast timeouts so tests never block on +// the production-sized registration budget or backoff windows. +func newShortFetcher(t *testing.T, opts ...Option) *Fetcher { + t.Helper() + + base := []Option{ + WithInsecureAllowHTTP(true), + WithAllowPrivateIPs(true), + WithRegistrationTimeout(2 * time.Second), + } + opts = append(base, opts...) + f, err := NewFetcher(context.Background(), opts...) + require.NoError(t, err) + return f +} + +// TestFetcher_LookupResolvesKidAndRefreshesOnMiss proves the core Lookup seam: +// a kid present in the cached set resolves without a refetch, a kid the cache +// has never seen forces one rate-limited refresh, and — once the gate has +// elapsed — another unknown kid may trigger exactly one more refresh. +func TestFetcher_LookupResolvesKidAndRefreshesOnMiss(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + key2, _ := newECKey(t, "k2") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + f := newShortFetcher(t, WithMinKidRefreshInterval(150*time.Millisecond)) + + // First lookup registers and fetches. + _, err := f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load(), "the first lookup must fetch exactly once") + + // Cache hit: no new fetch. + _, err = f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load(), "a cached kid must not trigger a refetch") + + // Rotate: kid2 is unknown, so exactly one refresh is forced and it must + // surface the new key. + srv.set(jwksDoc(t, key2)) + _, err = f.Lookup(context.Background(), srv.URL, "k2") + require.NoError(t, err, "an unknown kid must trigger a refresh and resolve after rotation") + require.Equal(t, int32(2), srv.hits.Load()) + + // Within the (short) gate a third kid must NOT force another refresh... + key3, _ := newECKey(t, "k3") + srv.set(jwksDoc(t, key3)) + _, err = f.Lookup(context.Background(), srv.URL, "k3") + require.Error(t, err, "a kid refresh within the rate-limit gate must not happen") + require.Equal(t, int32(2), srv.hits.Load(), "no fetch may occur while the kid-refresh gate is closed") + + // ...but after the gate elapses, the unknown kid resolves. + time.Sleep(250 * time.Millisecond) + _, err = f.Lookup(context.Background(), srv.URL, "k3") + require.NoError(t, err) + require.Equal(t, int32(3), srv.hits.Load()) +} + +// TestFetcher_UnknownKidRefreshRateLimitedByDefault proves the DEFAULT +// minKidRefreshInterval (30s) closes the refresh gate immediately after one +// forced refresh, so a client spamming made-up kids cannot drive a fetch per +// request. +func TestFetcher_UnknownKidRefreshRateLimitedByDefault(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + f := newShortFetcher(t) + + _, err := f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load()) + + // First miss forces the one allowed refresh; the key still isn't there. + _, err = f.Lookup(context.Background(), srv.URL, "made-up-kid") + require.Error(t, err) + require.Equal(t, int32(2), srv.hits.Load()) + + // Subsequent misses within the default 30s gate must not fetch again. + for range 3 { + _, err = f.Lookup(context.Background(), srv.URL, "made-up-kid") + require.Error(t, err) + } + require.Equal(t, int32(2), srv.hits.Load(), + "the default kid-refresh gate must prevent a second forced fetch within 30s") +} + +// TestFetcher_StaleOnError proves stale-on-error comes free from httprc/jwx +// semantics: once a set has been fetched, a later failing refresh (here a 500 +// on the endpoint) must NOT evict it — the previously stored keys keep +// validating. +func TestFetcher_StaleOnError(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + f := newShortFetcher(t) + + _, err := f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err) + + srv.setStatus(http.StatusInternalServerError) + + _, err = f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err, "a failed refresh must not evict the previously fetched key set") + + set, err := f.KeySet(context.Background(), srv.URL) + require.NoError(t, err) + _, found := set.LookupKeyID("k1") + assert.True(t, found, "KeySet must keep serving the stale-but-valid set") +} + +// TestFetcher_FetchFailureBackoffGatesRetries proves the fetch-failure backoff: +// before the first successful fetch, repeated EnsureRegistered calls must +// replay the stored error instead of hitting the endpoint again; after the +// backoff elapses and the endpoint recovers, registration succeeds through the +// Refresh path (the resource was already registered by the failed attempt). +func TestFetcher_FetchFailureBackoffGatesRetries(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + srv.setStatus(http.StatusInternalServerError) + + f := newShortFetcher(t, WithFetchFailureBackoff(400*time.Millisecond), WithRegistrationTimeout(300*time.Millisecond)) + + ctx := context.Background() + + // First attempt genuinely fetches (and fails). + err := f.EnsureRegistered(ctx, srv.URL) + require.Error(t, err) + require.Equal(t, int32(1), srv.hits.Load()) + + // Within the backoff window the stored error is replayed, no new fetch. + errReplay := f.EnsureRegistered(ctx, srv.URL) + require.Error(t, errReplay) + assert.Contains(t, errReplay.Error(), "context deadline exceeded", + "the stored fetch error must be replayed, not retried") + require.Equal(t, int32(1), srv.hits.Load(), + "a never-successfully-fetched endpoint must not be re-fetched within the backoff window") + + // After the backoff elapses, the retry goes through Refresh (the resource + // is registered) and succeeds now that the endpoint recovered. + time.Sleep(600 * time.Millisecond) + srv.set(jwksDoc(t, key1)) + require.NoError(t, f.EnsureRegistered(ctx, srv.URL)) + _, err = f.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(2), srv.hits.Load()) +} + +// TestFetcher_FailedRefreshOnUnknownKidPreservesStaleSet proves the stale-set +// invariant under a failed refresh-on-unknown-kid: when the forced re-fetch +// Lookup issues for an unknown kid fails (endpoint erroring), the previously +// fetched key set must survive — the kid that was already known keeps +// resolving from cache, with no additional fetch. +func TestFetcher_FailedRefreshOnUnknownKidPreservesStaleSet(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + f := newShortFetcher(t, WithMinKidRefreshInterval(time.Millisecond)) + + ctx := context.Background() + + // First lookup registers, fetches, and resolves k1. + _, err := f.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load()) + + // The endpoint starts failing; an unknown kid forces RefreshOnUnknownKid, + // whose re-fetch hits the 500 and fails. The lookup for the unknown kid + // must still fail, and must have genuinely attempted the refresh. + srv.setStatus(http.StatusInternalServerError) + _, err = f.Lookup(ctx, srv.URL, "k2") + require.Error(t, err, "an unknown kid whose forced refresh failed must not resolve") + require.Equal(t, int32(2), srv.hits.Load(), + "the unknown-kid lookup must have attempted exactly one refresh") + + // The failed refresh must not have evicted the stale set: k1 still + // resolves, served from cache without a new fetch. + hitsBeforeStaleLookup := srv.hits.Load() + _, err = f.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err, "the stale cached key set must survive a failed unknown-kid refresh") + require.Equal(t, hitsBeforeStaleLookup, srv.hits.Load(), + "the stale k1 lookup must be served from cache without a new fetch") +} + +// TestFetcher_BodyLimit proves the default 1 MiB response-body cap: a JWKS +// response larger than the cap can never be fully read, so the fetch never +// succeeds and Lookup fails. +func TestFetcher_BodyLimit(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + valid := jwksDoc(t, key1) + + // A well-formed JWKS padded past the cap with an unrelated field: it would + // parse fine if read in full, so only the cap cutting the read short makes + // the fetch fail. + var doc map[string]any + require.NoError(t, json.Unmarshal(valid, &doc)) + doc["padding"] = strings.Repeat("a", int(DefaultBodyLimit)+1024) + oversized, err := json.Marshal(doc) + require.NoError(t, err) + + srv := newMutableJWKSServer(t, oversized) + + f := newShortFetcher(t, WithRegistrationTimeout(300*time.Millisecond)) + + _, err = f.Lookup(context.Background(), srv.URL, "k1") + require.Error(t, err, "a JWKS response exceeding the body cap must fail to fetch") + require.Equal(t, int32(1), srv.hits.Load()) +} + +// TestLimitedBodyTransport asserts directly on the body cap transport. jwx +// surfaces every fetch failure as its own ready-wait timeout, so the cap's +// error never reaches a Fetcher caller and cannot be distinguished there from +// a 500 or a parse failure — asserting on the transport itself is the only way +// to pin that reading past the cap errors rather than truncating silently. +func TestLimitedBodyTransport(t *testing.T) { + t.Parallel() + + const bodyCap = 1024 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(strings.Repeat("a", 8*1024))) + })) + t.Cleanup(srv.Close) + + client := srv.Client() + client.Transport = &limitedBodyTransport{base: client.Transport, max: bodyCap} + + resp, err := client.Get(srv.URL) + require.NoError(t, err, "the cap applies to reading the body, not to the round trip") + t.Cleanup(func() { _ = resp.Body.Close() }) + + body, err := io.ReadAll(resp.Body) + require.Error(t, err, "reading past the cap must fail rather than truncate silently: "+ + "a truncated JWKS would be parsed as though it were the whole document") + var maxBytesErr *http.MaxBytesError + require.ErrorAs(t, err, &maxBytesErr, "the error must surface the body limit") + assert.LessOrEqual(t, int64(len(body)), int64(bodyCap), + "no more than the cap may be delivered before the error") +} + +// TestFetcher_PerInstanceFlagIsolation proves the package's core invariant: +// each Fetcher owns its own jwk.Cache and HTTP client, so configuring one +// Fetcher never leaks into another. Two Fetchers point at the SAME plain-HTTP +// JWKS URL: the permissive one fetches and succeeds; the strict one (which +// forbids plain HTTP) is rejected by its own URL policy without ever hitting +// the endpoint, and without disturbing the permissive Fetcher's cache. +func TestFetcher_PerInstanceFlagIsolation(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + permissive := newShortFetcher(t) + + // Deliberately NOT newShortFetcher: the strict Fetcher must keep the + // default HTTPS-only policy. It still allows private IPs so the URL + // policy — not the dial guard — is what rejects it. + strict, err := NewFetcher(context.Background(), + WithAllowPrivateIPs(true), WithRegistrationTimeout(2*time.Second)) + require.NoError(t, err) + + ctx := context.Background() + + _, err = permissive.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err, "the Fetcher permitting HTTP must succeed against the http:// endpoint") + require.Equal(t, int32(1), srv.hits.Load()) + + _, err = strict.Lookup(ctx, srv.URL, "k1") + require.Error(t, err, "the Fetcher forbidding plain HTTP must be rejected for its own policy") + assert.Contains(t, err.Error(), "must use HTTPS", + "the rejection must come from the strict Fetcher's own URL policy") + require.Equal(t, int32(1), srv.hits.Load(), + "the strict Fetcher must fail policy validation before any fetch") + + // The strict Fetcher's failed attempt must not have disturbed the + // permissive Fetcher's registered cache: a further lookup is still a + // cache hit. + _, err = permissive.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load()) +} + +// TestFetcher_CachesArePerInstance proves two Fetchers configured identically +// and pointed at the same URL each maintain their own cache — both fetch +// independently (2 total hits), never sharing a registration. +func TestFetcher_CachesArePerInstance(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + fa := newShortFetcher(t) + fb := newShortFetcher(t) + + ctx := context.Background() + _, err := fa.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err) + _, err = fb.Lookup(ctx, srv.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(2), srv.hits.Load(), + "each Fetcher must fetch through its own cache, not share one registration") +} + +// TestFetcher_MaxKeysCapsKeyCount proves the too-many-keys guard: a JWKS +// serving more than the default maximum is rejected rather than trusted. +func TestFetcher_MaxKeysCapsKeyCount(t *testing.T) { + t.Parallel() + + keys := make([]jwk.Key, DefaultMaxKeys+1) + for i := range keys { + key, _ := newECKey(t, fmt.Sprintf("k%d", i)) + keys[i] = key + } + srv := newMutableJWKSServer(t, jwksDoc(t, keys...)) + + f := newShortFetcher(t) + + _, err := f.Lookup(context.Background(), srv.URL, "k0") + require.Error(t, err) + assert.Contains(t, err.Error(), "too many keys") +} + +// TestFetcher_RefreshIntervalIsPinned confirms WithRefreshInterval actually +// threads jwk.WithConstantInterval through to the underlying httprc.Resource, +// even though the JWKS endpoint advertises a much longer Cache-Control +// max-age — an external endpoint must not get to choose how long its keys are +// cached (see the WithRefreshInterval doc comment). +func TestFetcher_RefreshIntervalIsPinned(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // A long max-age that would push httprc's derived interval far past + // the pinned interval if the constant interval were not applied. + w.Header().Set("Cache-Control", "max-age=2592000") // 30 days + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(jwksDoc(t, key1)) + })) + t.Cleanup(srv.Close) + + f := newShortFetcher(t, WithRefreshInterval(5*time.Minute)) + + _, err := f.Lookup(context.Background(), srv.URL, "k1") + require.NoError(t, err) + + resource, err := f.cache.LookupResource(context.Background(), srv.URL) + require.NoError(t, err) + assert.Equal(t, 5*time.Minute, resource.ConstantInterval(), + "registered resource must ignore the endpoint's own Cache-Control max-age") +} + +// TestFetcher_EnsureRegisteredToleratesAlreadyRegistered proves the +// ErrResourceAlreadyExists absorption: re-running EnsureRegistered for a URL +// that is already registered with the Fetcher's cache (e.g. after an external +// state reset) must not fail. +func TestFetcher_EnsureRegisteredToleratesAlreadyRegistered(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + + f := newShortFetcher(t) + + ctx := context.Background() + require.NoError(t, f.EnsureRegistered(ctx, srv.URL)) + + // Simulate the state an OIDC re-discovery reset used to produce: the + // fetched marker is cleared but the URL is still registered with the + // cache, so the next EnsureRegistered takes the IsRegistered→Refresh path. + f.mu.Lock() + f.fetched = false + f.mu.Unlock() + + require.NoError(t, f.EnsureRegistered(ctx, srv.URL), + "re-registering an already-registered URL must not fail") +} + +// TestFetcher_EnsureRegisteredRegistersNewURLAfterSuccess proves the URL +// switch: once a URL has been fetched successfully, EnsureRegistered for a +// DIFFERENT URL (e.g. after OIDC discovery resolves a new jwks_uri) must not +// be short-circuited by the previous success — it must register and fetch the +// new resource. +func TestFetcher_EnsureRegisteredRegistersNewURLAfterSuccess(t *testing.T) { + t.Parallel() + + key1, _ := newECKey(t, "k1") + key2, _ := newECKey(t, "k2") + srvA := newMutableJWKSServer(t, jwksDoc(t, key1)) + srvB := newMutableJWKSServer(t, jwksDoc(t, key2)) + + f := newShortFetcher(t) + + ctx := context.Background() + _, err := f.Lookup(ctx, srvA.URL, "k1") + require.NoError(t, err) + require.Equal(t, int32(1), srvA.hits.Load()) + + _, err = f.Lookup(ctx, srvB.URL, "k2") + require.NoError(t, err, "a URL other than the fetched one must be registered and fetched") + require.Equal(t, int32(1), srvB.hits.Load()) +} + +// TestValidateJWKSURL exercises ValidateJWKSURL: the SSRF guard applied on +// every register/refresh, and shared with pkg/authserver/config.go's +// config-time check so the two can't drift out of sync. +func TestValidateJWKSURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + insecureAllowHTTP bool + wantErr string + }{ + {name: "https accepted", url: "https://issuer.example.com/jwks"}, + {name: "http rejected", url: "http://issuer.example.com/jwks", wantErr: "must use HTTPS"}, + { + name: "userinfo with password rejected", + url: "https://user:hunter2@issuer.example.com/jwks", + wantErr: "must not contain userinfo", + }, + { + // url.Parse sets User for a bare username too, and net/http would + // still send it as a Basic auth header. + name: "userinfo without password rejected", + url: "https://user@issuer.example.com/jwks", + wantErr: "must not contain userinfo", + }, + { + name: "userinfo rejected even with insecureAllowHTTP", + url: "http://user:hunter2@issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must not contain userinfo", + }, + { + name: "http accepted with insecureAllowHTTP", + url: "http://issuer.example.com/jwks", + insecureAllowHTTP: true, + }, + { + name: "ftp rejected even with insecureAllowHTTP", + url: "ftp://issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must use HTTPS", + }, + { + name: "no scheme rejected even with insecureAllowHTTP", + url: "//issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must use HTTPS", + }, + {name: "loopback IP literal rejected", url: "https://127.0.0.1/jwks", wantErr: "private or loopback"}, + {name: "private IP literal rejected", url: "https://10.1.2.3/jwks", wantErr: "private or loopback"}, + {name: "malformed URL rejected", url: "://not-a-url", wantErr: "invalid URL"}, + {name: "missing host rejected", url: "https:///jwks", wantErr: "host is required"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateJWKSURL(tt.url, tt.insecureAllowHTTP, false) + if tt.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} diff --git a/pkg/auth/jwks/options.go b/pkg/auth/jwks/options.go new file mode 100644 index 0000000000..95fea02021 --- /dev/null +++ b/pkg/auth/jwks/options.go @@ -0,0 +1,158 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import ( + "net/http" + "time" +) + +// Option is a functional option for NewFetcher. +type Option func(*Fetcher) + +// WithHTTPClient supplies a pre-built *http.Client to use as-is for every +// JWKS fetch (and exposed via HTTPClient). When set, the Fetcher skips +// building its own client from the flag options below — but still wraps the +// client's Transport with the body-cap transport unless the body limit is +// disabled, since jwx's cache has no cap of its own. +func WithHTTPClient(client *http.Client) Option { + return func(f *Fetcher) { + f.httpClient = client + } +} + +// WithInsecureAllowHTTP permits plain-HTTP JWKS URLs. Development and testing +// only — never set in production. +func WithInsecureAllowHTTP(allow bool) Option { + return func(f *Fetcher) { + f.insecureAllowHTTP = allow + } +} + +// WithAllowPrivateIPs permits JWKS endpoints resolving to private or loopback +// addresses. Use only when the issuer is hosted inside the same cluster and +// has no public endpoint. +func WithAllowPrivateIPs(allow bool) Option { + return func(f *Fetcher) { + f.allowPrivateIPs = allow + } +} + +// WithCABundle sets the path to a PEM CA certificate bundle. When set, ONLY +// certificates from that bundle are trusted (system roots are not included) — +// pinned-only trust, mirroring networking's HttpClientBuilder.WithCABundle. +func WithCABundle(path string) Option { + return func(f *Fetcher) { + f.caBundlePath = path + f.caBundleUsesSystemRoots = false + } +} + +// WithSystemRootsPlusCABundle sets the path to a PEM CA certificate bundle and +// preserves trust in the system root pool — additive trust, mirroring +// networking's HttpClientBuilder.WithSystemRootsPlusCABundle. Use this when +// the upstream may use either a publicly trusted certificate or a private CA. +func WithSystemRootsPlusCABundle(path string) Option { + return func(f *Fetcher) { + f.caBundlePath = path + f.caBundleUsesSystemRoots = true + } +} + +// WithAuthTokenFile sets the path to a file containing a bearer token sent as +// Authorization on every JWKS fetch. +func WithAuthTokenFile(path string) Option { + return func(f *Fetcher) { + f.authTokenFile = path + } +} + +// WithTimeout sets the HTTP client timeout for JWKS fetches. Zero keeps +// networking's default. +func WithTimeout(timeout time.Duration) Option { + return func(f *Fetcher) { + f.timeout = timeout + } +} + +// WithDisableKeepAlives disables HTTP keep-alive on the transport. When true, +// each request uses a fresh connection, ensuring the per-dial SSRF check fires +// on every request rather than being bypassed by a reused connection. +func WithDisableKeepAlives(disable bool) Option { + return func(f *Fetcher) { + f.disableKeepAlives = disable + } +} + +// WithSameHostRedirects restricts redirect hops to the host of the original +// request, guarding against a discovery/JWKS redirect landing on a different, +// unvetted host. +func WithSameHostRedirects(enable bool) Option { + return func(f *Fetcher) { + f.sameHostRedirects = enable + } +} + +// WithWorkers caps the background worker pool of the Fetcher's own cache. +// Zero keeps httprc's default of five workers. +func WithWorkers(workers int) Option { + return func(f *Fetcher) { + f.workers = workers + } +} + +// WithBodyLimit caps every JWKS response body at limit bytes. Zero disables +// the cap — not recommended for endpoints derived from untrusted discovery +// documents. Defaults to DefaultBodyLimit. +func WithBodyLimit(limit int64) Option { + return func(f *Fetcher) { + f.bodyLimit = limit + } +} + +// WithMaxKeys caps the number of keys accepted from a JWKS. Zero disables the +// cap. Defaults to DefaultMaxKeys. +func WithMaxKeys(limit int) Option { + return func(f *Fetcher) { + f.maxKeys = limit + } +} + +// WithRefreshInterval pins the fixed interval at which the cache re-fetches +// its JWKS in the background. It is passed to Register via +// jwk.WithConstantInterval, which makes the resource ignore the response's +// Cache-Control/Expires headers entirely rather than merely bounding them — +// deliberately: absent this override, httprc derives the interval from those +// headers, clamped to [15m, 30 days], so a hostile or misconfigured issuer +// could otherwise extend our own key-retention window up to a month simply by +// setting a long max-age. Zero (the default) keeps header-derived scheduling. +func WithRefreshInterval(interval time.Duration) Option { + return func(f *Fetcher) { + f.refreshInterval = interval + } +} + +// WithFetchFailureBackoff bounds how often EnsureRegistered retries a JWKS +// fetch that has never once succeeded. Defaults to DefaultFetchFailureBackoff. +func WithFetchFailureBackoff(backoff time.Duration) Option { + return func(f *Fetcher) { + f.fetchFailureBackoff = backoff + } +} + +// WithMinKidRefreshInterval bounds how often Lookup forces a refresh for an +// unknown key ID. Defaults to DefaultMinKidRefreshInterval. +func WithMinKidRefreshInterval(interval time.Duration) Option { + return func(f *Fetcher) { + f.minKidRefreshInterval = interval + } +} + +// WithRegistrationTimeout bounds the initial registration's ready-wait. +// Defaults to DefaultRegistrationTimeout. +func WithRegistrationTimeout(timeout time.Duration) Option { + return func(f *Fetcher) { + f.registrationTimeout = timeout + } +} diff --git a/pkg/auth/jwks/transport.go b/pkg/auth/jwks/transport.go new file mode 100644 index 0000000000..39cd548947 --- /dev/null +++ b/pkg/auth/jwks/transport.go @@ -0,0 +1,38 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import "net/http" + +// limitedBodyTransport wraps an http.RoundTripper to cap every response body +// at max bytes, via http.MaxBytesReader rather than io.LimitReader: the +// latter truncates silently, which would let a caller parse a cut-off JWKS +// document as if it were complete, where the former surfaces a +// *http.MaxBytesError instead. Its error text ("http: request body too +// large") is written for the request-body case MaxBytesReader was designed +// for, so it reads oddly for a capped response — an accepted rough edge +// rather than justifying a custom ReadCloser. +// +// The nil first argument (http.ResponseWriter) is safe: MaxBytesReader only +// reaches it through a type assertion (`l.w.(requestTooLarger)`) used to tell +// a real server connection to close early, which — on a nil interface value +// — safely evaluates to false rather than panicking (net/http/request.go). +type limitedBodyTransport struct { + base http.RoundTripper + max int64 +} + +// RoundTrip delegates to base and then caps the returned body. The cap +// applies to every response, including a non-2xx one whose body the caller +// doesn't intend to parse — draining it is only ever io.Discard-ed, not +// unbounded, so this errs on the safe side rather than special-casing status +// codes. +func (t *limitedBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err != nil { + return nil, err + } + resp.Body = http.MaxBytesReader(nil, resp.Body, t.max) + return resp, nil +} diff --git a/pkg/auth/jwks/url.go b/pkg/auth/jwks/url.go new file mode 100644 index 0000000000..a14174a08f --- /dev/null +++ b/pkg/auth/jwks/url.go @@ -0,0 +1,64 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import ( + "errors" + "fmt" + "net" + "net/url" + + "github.com/stacklok/toolhive/pkg/networking" +) + +// ValidateJWKSURL checks that jwksURL parses, has a host, uses HTTPS unless +// insecureAllowHTTP permits plain HTTP — and only exactly the "http" scheme, +// not any other non-https scheme such as "file" or "ftp" — and, when the +// host is an IP literal, is not a private or loopback address unless +// allowPrivateIPs permits that. Both flags come from the specific Fetcher +// (or issuer) being fetched, never from a validator-wide or self-issuer +// setting. This prevents SSRF attacks where a compromised discovery document — +// or a hand-configured jwks_url — points to internal services. +// +// This is the single implementation shared by the runtime choke point in +// Fetcher.registerOrRefresh (on every fetch) and pkg/authserver/config.go's +// config-time check (validateJWKSEndpointURL): the two must not drift out of +// sync, or a laxer runtime check would silently defeat the config-time guard. +// +// Deliberately env-immune: neither flag is widened by +// INSECURE_DISABLE_URL_VALIDATION or any other environment variable, so an +// unrelated env var can never silently disable this SSRF-relevant check. +func ValidateJWKSURL(jwksURL string, insecureAllowHTTP, allowPrivateIPs bool) error { + u, err := url.Parse(jwksURL) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + + if u.Host == "" { + return errors.New("host is required") + } + + // Unlike issuer_url, a jwks_url carrying userinfo would actually work — + // net/http turns it into a Basic auth header on every JWKS fetch — which + // is precisely why it is rejected rather than tolerated: it would put a + // live credential in the RunConfig, in this function's error strings, and + // in any log that quotes the URL. A JWKS endpoint is public by + // definition (it serves verification keys), so there is no legitimate + // reason to authenticate to one. + if u.User != nil { + return errors.New("must not contain userinfo (credentials in the URL)") + } + + if u.Scheme != "https" && (u.Scheme != "http" || !insecureAllowHTTP) { + return fmt.Errorf("must use HTTPS, got %q", u.Scheme) + } + + host := u.Hostname() + ip := net.ParseIP(host) + if ip != nil && !allowPrivateIPs && networking.IsPrivateIP(ip) { + return errors.New("must not point to a private or loopback address") + } + + return nil +} diff --git a/pkg/auth/token.go b/pkg/auth/token.go index 7147daefc4..a84ca2f44e 100644 --- a/pkg/auth/token.go +++ b/pkg/auth/token.go @@ -20,10 +20,10 @@ import ( "github.com/cenkalti/backoff/v5" "github.com/golang-jwt/jwt/v5" - "github.com/lestrrat-go/httprc/v3" "github.com/lestrrat-go/jwx/v3/jwk" "github.com/stacklok/toolhive-core/env" + "github.com/stacklok/toolhive/pkg/auth/jwks" "github.com/stacklok/toolhive/pkg/auth/upstreamtoken" "github.com/stacklok/toolhive/pkg/authserver/server/keys" "github.com/stacklok/toolhive/pkg/networking" @@ -360,7 +360,7 @@ type TokenValidator struct { jwksURL string clientID string clientSecret string // Optional client secret for introspection - jwksClient *jwk.Cache + jwks *jwks.Fetcher introspectURL string // Optional introspection endpoint client *http.Client // HTTP client for making requests resourceURL string // (RFC 9728) @@ -373,16 +373,11 @@ type TokenValidator struct { upstreamTokenReader upstreamtoken.TokenReader // keyProvider provides in-process JWKS key lookups from the embedded auth - // server's key provider. When set, getKeyFromJWKS resolves keys locally - // before falling back to HTTP. Eliminates self-referential HTTP calls. - // nil when no embedded auth server is configured. + // server's key provider. When set, JWKS key resolution resolves keys + // locally before falling back to HTTP. Eliminates self-referential HTTP + // calls. nil when no embedded auth server is configured. keyProvider keys.PublicKeyProvider - // Lazy JWKS registration - jwksRegistered bool - jwksRegistrationMu sync.Mutex - jwksRegistrationErr error - // Lazy OIDC discovery - allows deferring discovery until first validation request. // This is needed when the OIDC provider (auth server) is the same pod and starts after // the token validator is created. @@ -652,7 +647,10 @@ func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts .. return nil, err } - // Create HTTP client with CA bundle and auth token support for JWKS + // Create HTTP client with CA bundle and auth token support for OIDC + // discovery and introspection. The JWKS fetch path uses the dedicated + // jwks.Fetcher built below, which constructs its own client from the + // same flags — one Fetcher (and therefore one cache) per validator. httpClient, err := networking.NewHttpClientBuilder(). WithCABundle(config.CACertPath). WithPrivateIPs(config.AllowPrivateIP). @@ -664,16 +662,28 @@ func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts .. } config.httpClient = httpClient - // Create a new JWKS client with auto-refresh - // In jwx v3, NewCache requires an httprc.Client - httprcClient := httprc.NewClient(httprc.WithHTTPClient(httpClient)) - cache, err := jwk.NewCache(ctx, httprcClient) + // Build the JWKS fetcher: it owns JWKS URL validation, lazy registration, + // the response-body cap, the max-key-count cap, rate-limited refresh on + // unknown key IDs, and the fetch-failure backoff gate (see + // pkg/auth/jwks). The registration timeout keeps today's 5-second + // ready-wait budget; the header-derived refresh schedule is kept (no + // pinned interval). The 1 MiB body cap, 100-key cap, 30s unknown-kid + // refresh gate, and 30s fetch-failure backoff are new hardening that + // previously only existed on the token-exchange path. + fetcherOpts := []jwks.Option{ + jwks.WithInsecureAllowHTTP(config.InsecureAllowHTTP), + jwks.WithAllowPrivateIPs(config.AllowPrivateIP), + jwks.WithAuthTokenFile(config.AuthTokenFile), + jwks.WithRegistrationTimeout(5 * time.Second), + } + if config.CACertPath != "" { + fetcherOpts = append(fetcherOpts, jwks.WithCABundle(config.CACertPath)) + } + fetcher, err := jwks.NewFetcher(ctx, fetcherOpts...) if err != nil { - return nil, fmt.Errorf("failed to create JWKS cache: %w", err) + return nil, fmt.Errorf("failed to create JWKS fetcher: %w", err) } - // Skip synchronous JWKS registration - will be done lazily on first use - // Resolve client secret from config or environment variable clientSecret := resolveClientSecret(config.ClientSecret, o.envReader) @@ -690,7 +700,7 @@ func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts .. introspectURL: config.IntrospectionURL, clientID: config.ClientID, clientSecret: clientSecret, - jwksClient: cache, + jwks: fetcher, client: config.httpClient, resourceURL: config.ResourceURL, scopes: config.Scopes, @@ -720,57 +730,6 @@ func validateGoogleTokeninfoAudience(config TokenValidatorConfig) error { return nil } -// ensureJWKSRegistered ensures that the JWKS URL is registered with the cache. -// This is called lazily on first use to avoid blocking startup. -// On failure, registration is retried on subsequent calls (transient failures -// should not permanently disable the validator). -func (v *TokenValidator) ensureJWKSRegistered(ctx context.Context) error { - v.jwksRegistrationMu.Lock() - defer v.jwksRegistrationMu.Unlock() - - // Already registered successfully - nothing to do - if v.jwksRegistered { - return nil - } - - // Create context with 5-second timeout for JWKS registration - registrationCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - // Attempt registration. The CA-aware client must be passed per-resource: - // jwx >= 3.1.0 injects its own default client at the resource level when - // none is given here, which takes precedence over the client-level one - // configured in NewTokenValidator and silently drops custom CA support. - err := v.jwksClient.Register(registrationCtx, v.jwksURL, jwk.WithHTTPClient(v.client)) - switch { - case err == nil: - // Registered and the first fetch succeeded. - case errors.Is(err, httprc.ErrNotReady()): - // The resource is registered and httprc will keep fetching it in the - // background; only the first fetch has not completed within the - // registration budget. Treat it as registered: Lookup returns a - // not-ready error until a background fetch succeeds. Retrying Register - // instead would fail with ErrResourceAlreadyExists forever. - //nolint:gosec // G706: JWKS URL is from server configuration or OIDC discovery - slog.Debug( - "JWKS URL registered but first fetch not ready; fetching continues in background", - "jwks_url", v.jwksURL, "error", err, - ) - case errors.Is(err, httprc.ErrResourceAlreadyExists()): - // The URL is already in httprc's resource map, e.g. a previous attempt - // returned ErrNotReady before this state was tracked, or OIDC - // re-discovery produced the same URL after resetting the flag. - default: - v.jwksRegistrationErr = fmt.Errorf("failed to register JWKS URL: %w", err) - // Do NOT set jwksRegistered = true -- allow retry on next call - return v.jwksRegistrationErr - } - - v.jwksRegistered = true - v.jwksRegistrationErr = nil - return nil -} - // OIDC discovery retry configuration constants. const ( // oidcDiscoveryMaxAttempts is the maximum number of OIDC discovery attempts @@ -857,12 +816,10 @@ func (v *TokenValidator) ensureOIDCDiscovered(ctx context.Context) error { v.jwksURL = doc.JWKSURI v.oidcDiscovered = true v.oidcDiscoveryErr = nil - // Reset JWKS registration so it re-registers with the newly discovered URL. - // Acquire jwksRegistrationMu to safely reset the flag, since ensureJWKSRegistered - // reads it under that mutex. Lock ordering: oidcDiscoveryMu -> jwksRegistrationMu. - v.jwksRegistrationMu.Lock() - v.jwksRegistered = false - v.jwksRegistrationMu.Unlock() + // No JWKS registration state to reset here: the jwks.Fetcher registers + // per URL (EnsureRegistered asks the cache whether the URL is already + // registered), so a newly discovered URL registers naturally on first + // lookup. //nolint:gosec // G706: issuer and JWKS URL are from OIDC discovery slog.Debug( "oidc discovery succeeded", @@ -945,27 +902,20 @@ func (v *TokenValidator) getKeyFromJWKS(ctx context.Context, token *jwt.Token) ( return nil, ErrMissingJWKSURL } - // Ensure JWKS is registered before attempting to use it - if err := v.ensureJWKSRegistered(ctx); err != nil { - return nil, fmt.Errorf("JWKS registration failed: %w", err) - } - kid, err := validateTokenHeader(token) if err != nil { return nil, err } - // Get the key set from the JWKS - // In jwx v3, Get is replaced with Lookup - keySet, err := v.jwksClient.Lookup(ctx, v.jwksURL) + // Resolve the key through the shared fetcher: it registers and fetches + // the JWKS lazily (see EnsureRegistered), rate-limits refreshes for + // unknown key IDs, and caps response-body and key-count. Registration + // and fetch failures are retried on subsequent calls (transient failures + // should not permanently disable the validator) — gated by the fetcher's + // fetch-failure backoff. + key, err := v.jwks.Lookup(ctx, v.jwksURL, kid) if err != nil { - return nil, fmt.Errorf("failed to lookup JWKS: %w", err) - } - - // Get the key with the matching key ID - key, found := keySet.LookupKeyID(kid) - if !found { - return nil, fmt.Errorf("key ID %s not found in JWKS", kid) + return nil, err } // Get the raw key diff --git a/pkg/auth/token_jwks_test.go b/pkg/auth/token_jwks_test.go new file mode 100644 index 0000000000..1cb6876e60 --- /dev/null +++ b/pkg/auth/token_jwks_test.go @@ -0,0 +1,311 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package auth + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/lestrrat-go/jwx/v3/jwk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newECKey generates a fresh ECDSA P-256 key and imports its public half as a +// jwk.Key carrying the given key ID. +func newECKey(t *testing.T, kid string) (jwk.Key, *ecdsa.PrivateKey) { + t.Helper() + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + key, err := jwk.Import(&priv.PublicKey) + require.NoError(t, err) + require.NoError(t, key.Set(jwk.KeyIDKey, kid)) + require.NoError(t, key.Set(jwk.AlgorithmKey, "ES256")) + require.NoError(t, key.Set(jwk.KeyUsageKey, "sig")) + return key, priv +} + +// jwksDoc serialises the given public keys into a JWKS JSON document. +func jwksDoc(t *testing.T, keys ...jwk.Key) []byte { + t.Helper() + + set := jwk.NewSet() + for _, key := range keys { + require.NoError(t, set.AddKey(key)) + } + raw, err := json.Marshal(set) + require.NoError(t, err) + return raw +} + +// mutableJWKSServer is a JWKS endpoint whose payload and status can be changed +// mid-test, and which counts how many times it was fetched. +type mutableJWKSServer struct { + *httptest.Server + + hits atomic.Int32 + mu sync.Mutex + body []byte + status int +} + +func newMutableJWKSServer(t *testing.T, initial []byte) *mutableJWKSServer { + t.Helper() + + s := &mutableJWKSServer{body: initial, status: http.StatusOK} + s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + s.hits.Add(1) + s.mu.Lock() + defer s.mu.Unlock() + if s.status != http.StatusOK { + w.WriteHeader(s.status) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(s.body) + })) + t.Cleanup(s.Close) + return s +} + +func (s *mutableJWKSServer) set(body []byte) { + s.mu.Lock() + defer s.mu.Unlock() + s.body = body + s.status = http.StatusOK +} + +func (s *mutableJWKSServer) setStatus(status int) { + s.mu.Lock() + defer s.mu.Unlock() + s.status = status +} + +// signJWSToken signs claims with the given key under the given kid. +func signJWSToken(t *testing.T, priv *ecdsa.PrivateKey, kid, issuer string) string { + t.Helper() + + token := jwt.NewWithClaims(jwt.SigningMethodES256, jwt.MapClaims{ + "iss": issuer, + "aud": "test-audience", + "exp": time.Now().Add(time.Hour).Unix(), + "sub": "test-user", + }) + token.Header["kid"] = kid + signed, err := token.SignedString(priv) + require.NoError(t, err) + return signed +} + +const jwksTestIssuer = "test-issuer" + +// newJWSTestValidator builds a TokenValidator pointed at the given JWKS URL. +func newJWSTestValidator(t *testing.T, jwksURL string) *TokenValidator { + t.Helper() + + v, err := NewTokenValidator(context.Background(), TokenValidatorConfig{ + Issuer: jwksTestIssuer, + Audience: "test-audience", + JWKSURL: jwksURL, + AllowPrivateIP: true, + InsecureAllowHTTP: true, // loopback httptest server over plain HTTP + }) + require.NoError(t, err) + return v +} + +// TestTokenValidator_SlowFirstFetchStillSucceeds replaces the old +// registration-state test for ErrNotReady tolerance with a black-box check: a +// JWKS endpoint whose first fetch is slow (but completes within the +// registration budget) must still yield a working validator on first use. +func TestTokenValidator_SlowFirstFetchStillSucceeds(t *testing.T) { + t.Parallel() + + key, priv := newECKey(t, testKeyID) + + hit := make(chan struct{}, 1) + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + hit <- struct{}{} + // Slow, not broken: the first fetch completes within the 5s + // registration budget. + time.Sleep(300 * time.Millisecond) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(jwksDoc(t, key)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + validator := newJWSTestValidator(t, srv.URL+"/jwks") + + claims, err := validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, jwksTestIssuer)) + require.NoError(t, err, "a slow-but-successful first fetch must not fail validation") + require.Equal(t, "test-user", claims["sub"]) +} + +// TestTokenValidator_OIDCRediscoveryToNewJWKSURLReregisters replaces the old +// registration-state test for ErrResourceAlreadyExists with a black-box check: +// after OIDC re-discovery resolves a NEW jwks URL, the fetcher registers the +// new URL cleanly (the old URL staying registered in the cache must not get +// in the way) and validation works end to end. +func TestTokenValidator_OIDCRediscoveryToNewJWKSURLReregisters(t *testing.T) { + t.Parallel() + + key, priv := newECKey(t, testKeyID) + keySetDoc := jwksDoc(t, key) + + jwksA := newMutableJWKSServer(t, keySetDoc) + jwksB := newMutableJWKSServer(t, keySetDoc) + + // Discovery document advertising jwks A, switchable to jwks B. + var advertise atomic.Pointer[string] + advertise.Store(&jwksA.URL) + discovery := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": jwksTestIssuer, + "jwks_uri": *advertise.Load() + "/jwks", + }) + })) + t.Cleanup(discovery.Close) + + validator, err := NewTokenValidator(context.Background(), TokenValidatorConfig{ + Issuer: discovery.URL, + Audience: "test-audience", + AllowPrivateIP: true, + InsecureAllowHTTP: true, + }) + require.NoError(t, err) + + // First validation: lazy discovery resolves jwks A and registers it. + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, discovery.URL)) + require.NoError(t, err) + + // Re-discovery resolves jwks B this time: clear the discovery state as a + // fresh discovery would, keeping the fetcher's cache as it is (URL A + // registered). + advertise.Store(&jwksB.URL) + validator.oidcDiscoveryMu.Lock() + validator.oidcDiscovered = false + validator.oidcDiscoveryMu.Unlock() + validator.jwksURL = "" + + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, discovery.URL)) + require.NoError(t, err, "re-discovery to a new jwks URL must re-register cleanly") +} + +// TestTokenValidator_JWKSBodyCap proves the newly-extended inbound hardening: +// a JWKS response larger than the default 1 MiB body cap can never be fully +// read, so the fetch never succeeds and validation fails (jwx surfaces every +// fetch failure as its own ready-wait timeout — see +// TestMultiIssuerTokenValidator_FetchJWKS in the tokenexchange package for the +// same behavior; the cap's error itself is pinned by TestLimitedBodyTransport +// in pkg/auth/jwks). +func TestTokenValidator_JWKSBodyCap(t *testing.T) { + t.Parallel() + + key, priv := newECKey(t, testKeyID) + var doc map[string]any + require.NoError(t, json.Unmarshal(jwksDoc(t, key), &doc)) + doc["padding"] = make([]byte, 1<<20+1024) + oversized, err := json.Marshal(doc) + require.NoError(t, err) + + srv := newMutableJWKSServer(t, oversized) + + validator := newJWSTestValidator(t, srv.URL+"/jwks") + + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, jwksTestIssuer)) + require.Error(t, err, "a JWKS response exceeding the body cap must fail validation") + require.Equal(t, int32(1), srv.hits.Load()) + + // Within the fetch-failure backoff window, the stored error is replayed + // without hitting the endpoint again. + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, jwksTestIssuer)) + require.Error(t, err) + require.Equal(t, int32(1), srv.hits.Load(), + "a never-successfully-fetched endpoint must not be re-fetched within the backoff window") +} + +// TestTokenValidator_KeyRotationRefreshesOnUnknownKid proves rate-limited +// refresh-on-unknown-kid on the inbound path: a token signed with a rotated +// key validates on the very first attempt after rotation (one forced refresh), +// but a SECOND rotation immediately afterwards does not force another fetch — +// the 30s default gate must hold. +func TestTokenValidator_KeyRotationRefreshesOnUnknownKid(t *testing.T) { + t.Parallel() + + key1, priv1 := newECKey(t, "v1") + key2, priv2 := newECKey(t, "v2") + key3, priv3 := newECKey(t, "v3") + + srv := newMutableJWKSServer(t, jwksDoc(t, key1)) + validator := newJWSTestValidator(t, srv.URL+"/jwks") + + // Prime the cache with v1. + _, err := validator.ValidateToken(context.Background(), + signJWSToken(t, priv1, "v1", jwksTestIssuer)) + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load()) + + // Rotate to v2: the unknown kid forces exactly one refresh and the new + // key must validate on this first attempt. + srv.set(jwksDoc(t, key2)) + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv2, "v2", jwksTestIssuer)) + require.NoError(t, err, "a token signed with a rotated key must validate after the forced refresh") + require.Equal(t, int32(2), srv.hits.Load()) + + // Rotate again and immediately present a v3 token: the unknown-kid + // refresh gate (30s default) must be closed, so no fetch happens and + // validation fails. + srv.set(jwksDoc(t, key3)) + _, err = validator.ValidateToken(context.Background(), + signJWSToken(t, priv3, "v3", jwksTestIssuer)) + require.Error(t, err, "a second rotation within the kid-refresh gate must not resolve") + require.Equal(t, int32(2), srv.hits.Load(), + "the default kid-refresh gate must prevent a second forced fetch within 30s") +} + +// TestTokenValidator_StaleOnFetchError proves stale-on-error on the inbound +// path: once a JWKS has been fetched successfully, a later failing refresh +// must not evict it — previously fetched keys keep validating. +func TestTokenValidator_StaleOnFetchError(t *testing.T) { + t.Parallel() + + key, priv := newECKey(t, testKeyID) + srv := newMutableJWKSServer(t, jwksDoc(t, key)) + validator := newJWSTestValidator(t, srv.URL+"/jwks") + + _, err := validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, jwksTestIssuer)) + require.NoError(t, err) + require.Equal(t, int32(1), srv.hits.Load()) + + // The endpoint breaks: the previously fetched key set must still validate + // (stale-on-error), and no background refresh may have re-fetched. + srv.setStatus(http.StatusInternalServerError) + + claims, err := validator.ValidateToken(context.Background(), + signJWSToken(t, priv, testKeyID, jwksTestIssuer)) + require.NoError(t, err, "a failing refresh must not evict the previously fetched key set") + assert.Equal(t, "test-user", claims["sub"]) + require.Equal(t, int32(1), srv.hits.Load()) +} diff --git a/pkg/auth/token_test.go b/pkg/auth/token_test.go index 5f5aaa8fee..8faafabf09 100644 --- a/pkg/auth/token_test.go +++ b/pkg/auth/token_test.go @@ -94,18 +94,6 @@ func TestTokenValidator(t *testing.T) { t.Fatalf("Failed to create token validator: %v", err) } - // Ensure JWKS is registered before lookup - err = validator.ensureJWKSRegistered(ctx) - if err != nil { - t.Fatalf("Failed to register JWKS: %v", err) - } - - // Force a refresh of the JWKS cache - _, err = validator.jwksClient.Lookup(ctx, jwksServer.URL) - if err != nil { - t.Fatalf("Failed to refresh JWKS cache: %v", err) - } - // Test cases testCases := []struct { name string @@ -242,18 +230,6 @@ func TestTokenValidatorMiddleware(t *testing.T) { t.Fatalf("Failed to create token validator: %v", err) } - // Ensure JWKS is registered before lookup - err = validator.ensureJWKSRegistered(ctx) - if err != nil { - t.Fatalf("Failed to register JWKS: %v", err) - } - - // Force a refresh of the JWKS cache - _, err = validator.jwksClient.Lookup(ctx, jwksServer.URL) - if err != nil { - t.Fatalf("Failed to refresh JWKS cache: %v", err) - } - // Create a test handler testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Get the identity from the context @@ -679,18 +655,6 @@ func TestNewTokenValidatorWithOIDCDiscovery(t *testing.T) { t.Fatalf("Failed to sign token: %v", err) } - // Ensure JWKS is registered before lookup - err = validator.ensureJWKSRegistered(ctx) - if err != nil { - t.Fatalf("Failed to register JWKS: %v", err) - } - - // Force a refresh of the JWKS cache - _, err = validator.jwksClient.Lookup(ctx, validator.jwksURL) - if err != nil { - t.Fatalf("Failed to refresh JWKS cache: %v", err) - } - validatedClaims, err := validator.ValidateToken(ctx, tokenString) if err != nil { t.Fatalf("Failed to validate token: %v", err) @@ -2390,9 +2354,6 @@ func TestMiddleware_UpstreamTokenEnrichment(t *testing.T) { CACertPath: caCertPath, AllowPrivateIP: true, }, opts...) require.NoError(t, vErr) - require.NoError(t, v.ensureJWKSRegistered(context.Background())) - _, lErr := v.jwksClient.Lookup(context.Background(), jwksServer.URL) - require.NoError(t, lErr) return v } @@ -3012,70 +2973,6 @@ func TestValidateToken_DiscoveryFailsWithKeyProvider(t *testing.T) { }) } -func TestEnsureJWKSRegistered_NonFatalRegistrationErrors(t *testing.T) { - t.Parallel() - - t.Run("ErrNotReady marks the JWKS as registered", func(t *testing.T) { - t.Parallel() - // A JWKS endpoint that never succeeds keeps the resource from - // becoming ready within the registration budget. - jwksServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - })) - t.Cleanup(jwksServer.Close) - caCertPath := writeTestServerCert(t, jwksServer) - - validator, err := NewTokenValidator(context.Background(), TokenValidatorConfig{ - Issuer: "test-issuer", - Audience: "test-audience", - JWKSURL: jwksServer.URL, - ClientID: "test-client", - CACertPath: caCertPath, - AllowPrivateIP: true, - }) - require.NoError(t, err) - - // Bound the ready-wait well below the 5s registration budget. - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - defer cancel() - require.NoError(t, validator.ensureJWKSRegistered(ctx), - "ErrNotReady must be treated as registered-but-pending") - require.True(t, validator.jwksRegistered) - - // Lookup surfaces not-ready until a background fetch succeeds. - _, err = validator.jwksClient.Lookup(context.Background(), jwksServer.URL) - require.Error(t, err) - }) - - t.Run("ErrResourceAlreadyExists marks the JWKS as registered", func(t *testing.T) { - t.Parallel() - jwksServer, caCertPath := createTestJWKSServer(t, jwk.NewSet()) - t.Cleanup(jwksServer.Close) - - validator, err := NewTokenValidator(context.Background(), TokenValidatorConfig{ - Issuer: "test-issuer", - Audience: "test-audience", - JWKSURL: jwksServer.URL, - ClientID: "test-client", - CACertPath: caCertPath, - AllowPrivateIP: true, - }) - require.NoError(t, err) - require.NoError(t, validator.ensureJWKSRegistered(context.Background())) - - // Simulate the reset done after OIDC re-discovery: the flag is - // cleared but the URL is still in httprc's resource map, so the - // next registration attempt returns ErrResourceAlreadyExists. - validator.jwksRegistrationMu.Lock() - validator.jwksRegistered = false - validator.jwksRegistrationMu.Unlock() - - require.NoError(t, validator.ensureJWKSRegistered(context.Background()), - "re-registering an already-registered URL must not fail") - require.True(t, validator.jwksRegistered) - }) -} - func TestNewTokenValidator_GoogleTokeninfoRequiresAudience(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index 7eb7d7119c..481767ed8b 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/stacklok/toolhive/pkg/auth/jwks" oauthserver "github.com/stacklok/toolhive/pkg/authserver/server" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/handlers" @@ -1178,7 +1179,8 @@ func validateTrustedIssuers(issuers []tokenexchange.TrustedIssuer, selfIssuer st // (multi_issuer_validator.go) enforces the same invariant inside // NewMultiIssuerTokenValidator, so a caller constructing a validator // without routing through Config.Validate is still covered. Note that - // ensureRegistered's ValidateJWKSURL does NOT cover it — that check is + // the fetcher's jwks.ValidateJWKSURL (applied on every register and + // refresh, see pkg/auth/jwks) does NOT cover it — that check is // gated on net.ParseIP, so it only rejects private IP *literals*, and a // discovery document advertising a private *hostname* passes it // cleanly. Checking here and in the constructor is deliberate @@ -1222,9 +1224,9 @@ func validateTrustedIssuers(issuers []tokenexchange.TrustedIssuer, selfIssuer st // OIDC issuer-identifier rules (no query/fragment/trailing-slash) since a // JWKS endpoint legitimately carries those. // -// Delegates to tokenexchange.ValidateJWKSURL, the same predicate the runtime -// choke point (ensureRegistered, called on every JWKS fetch) enforces — the two -// were previously separate implementations that had drifted apart (a +// Delegates to jwks.ValidateJWKSURL, the same predicate the runtime choke +// point (jwks.Fetcher.EnsureRegistered, called on every JWKS fetch) enforces — +// the two were previously separate implementations that had drifted apart (a // runtime check laxer than this one would silently defeat this config-time // guard), so this is now the single source of truth for both. // @@ -1236,7 +1238,7 @@ func validateTrustedIssuers(issuers []tokenexchange.TrustedIssuer, selfIssuer st // only relaxing the scheme. This helper takes its "insecure" bits solely // from the issuer's own explicit InsecureAllowHTTP/AllowPrivateIPs fields. func validateJWKSEndpointURL(rawURL string, insecureAllowHTTP, allowPrivateIPs bool) error { - return tokenexchange.ValidateJWKSURL(rawURL, insecureAllowHTTP, allowPrivateIPs) + return jwks.ValidateJWKSURL(rawURL, insecureAllowHTTP, allowPrivateIPs) } // warnTrustedIssuerAudiences logs a warning for each TrustedIssuer whose diff --git a/pkg/authserver/server/tokenexchange/multi_issuer_validator.go b/pkg/authserver/server/tokenexchange/multi_issuer_validator.go index f702bf0689..98770e945d 100644 --- a/pkg/authserver/server/tokenexchange/multi_issuer_validator.go +++ b/pkg/authserver/server/tokenexchange/multi_issuer_validator.go @@ -10,9 +10,7 @@ import ( "fmt" "io" "log/slog" - "net" "net/http" - "net/url" "slices" "strings" "sync" @@ -21,12 +19,11 @@ import ( celgo "cel.dev/cel-go/cel" "github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4/jwt" - "github.com/lestrrat-go/httprc/v3" "github.com/lestrrat-go/jwx/v3/jwk" "github.com/stacklok/toolhive-core/cel" + "github.com/stacklok/toolhive/pkg/auth/jwks" "github.com/stacklok/toolhive/pkg/authserver/server" - "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -35,32 +32,20 @@ const ( httpTimeout = 10 * time.Second // maxResponseBodySize is the maximum size of HTTP response bodies read - // from external OIDC discovery documents AND JWKS fetches (1 MiB). This - // prevents resource exhaustion from unexpectedly large responses. The - // discovery read enforces it directly via io.LimitReader; the JWKS read - // goes through jwx instead, so it is enforced by wrapping every issuer's - // *http.Client transport with limitedBodyTransport — see where that - // client is built in NewMultiIssuerTokenValidator. + // from external OIDC discovery documents (1 MiB). This prevents resource + // exhaustion from unexpectedly large responses. The discovery read + // enforces it directly via io.LimitReader; the JWKS fetch goes through + // jwx instead, so it is enforced by each issuer's jwks.Fetcher body cap. maxResponseBodySize = 1 << 20 - // maxJWKSKeys caps the number of keys accepted from an external JWKS to - // prevent CPU amplification from a hostile endpoint serving many keys. - maxJWKSKeys = 100 - - // minKidRefreshInterval bounds how often refreshOnUnknownKid forces an - // issuer's jwk.Cache to fetch its JWKS ahead of jwx's own background - // refresh schedule. verifySignature's kidMatched check runs before the - // subject token's signature is trusted, so without this floor a client - // presenting a syntactically valid JWT that merely names a made-up kid - // could force a fresh fetch to the external IdP on every attempt. - minKidRefreshInterval = 30 * time.Second - - // jwksFetchFailureBackoff bounds how often ensureRegistered retries a - // JWKS fetch for an issuer that has never yet succeeded. Key resolution - // runs before the subject token's signature is checked, so without this - // an authenticated client holding the token-exchange grant could force a - // real outbound fetch to a broken external IdP on every single request. - jwksFetchFailureBackoff = 30 * time.Second + // jwksDiscoveryBackoff bounds how often resolveJWKSURL retries OIDC + // discovery for an issuer whose discovery has never once succeeded. + // Discovery runs before the subject token's signature is checked, so + // without this an authenticated client holding the token-exchange grant + // could force a real outbound discovery request to a broken external IdP + // on every single request. (The JWKS fetch itself is gated separately — + // see jwks.DefaultFetchFailureBackoff.) + jwksDiscoveryBackoff = 30 * time.Second // jwksRefreshInterval is the fixed interval at which each issuer's // jwk.Cache re-fetches its JWKS in the background. It is passed to @@ -247,10 +232,10 @@ type MultiIssuerTokenValidator struct { // externalIssuerConfig holds the configuration and cached state for an external // OIDC issuer. The embedded TrustedIssuer is treated as immutable after // construction: resolveActorAuthorization reads TrustedIssuer.AllowedActors -// and actorMatcher (and discoverJWKSURL/fetchJWKS read httpClient) on every -// validation without holding mu, since mu protects JWKS state only. Mutating -// TrustedIssuer fields in place after NewMultiIssuerTokenValidator returns is -// a data race. +// and actorMatcher (and discoverJWKSURL reads the fetcher's HTTP client) on +// every validation without holding mu, since mu protects JWKS state only. +// Mutating TrustedIssuer fields in place after NewMultiIssuerTokenValidator +// returns is a data race. type externalIssuerConfig struct { TrustedIssuer @@ -258,89 +243,39 @@ type externalIssuerConfig struct { // construction and is immutable thereafter. actorMatcher *cel.CompiledExpression - // httpClient is dedicated to this issuer, built once at construction - // time from its own InsecureAllowHTTP/AllowPrivateIPs. A single - // validator-wide client couldn't enforce per-issuer SSRF/transport - // policy: http.Client.CheckRedirect and Transport.DialContext have no - // way to know which issuer's fetch they are guarding. - httpClient *http.Client - - // jwksCache is this issuer's own jwk.Cache, registered with httpClient - // above via jwk.WithHTTPClient (see registerOrRefresh). A cache per - // issuer, rather than one shared across every configured issuer, is what - // makes two issuers resolving to the same jwks_url (e.g. two Microsoft - // Entra v1 tenants, which share one tenant-independent JWKS endpoint) a - // non-event: httprc keys a cached resource by URL alone and only honors - // jwk.WithHTTPClient on a URL's first Register call, so a shared cache - // would have the second such issuer silently inherit the first one's - // *http.Client — defeating InsecureAllowHTTP/AllowPrivateIPs's per-issuer - // guarantee for it. Splitting the cache per issuer makes that collision - // unrepresentable instead of guarding against it. - jwksCache *jwk.Cache + // jwks is this issuer's own jwks.Fetcher, built once at construction time + // from its own InsecureAllowHTTP/AllowPrivateIPs. A single validator-wide + // fetcher couldn't enforce per-issuer SSRF/transport policy: + // http.Client.CheckRedirect and Transport.DialContext have no way to know + // which issuer's fetch they are guarding. The Fetcher owns the issuer's + // jwk.Cache and *http.Client too: a cache per issuer, rather than one + // shared across every configured issuer, is what makes two issuers + // resolving to the same jwks_url (e.g. two Microsoft Entra v1 tenants, + // which share one tenant-independent JWKS endpoint) a non-event — httprc + // keys a cached resource by URL alone and only honors jwk.WithHTTPClient + // on a URL's first Register call, so a shared cache would have the second + // such issuer silently inherit the first one's *http.Client — defeating + // InsecureAllowHTTP/AllowPrivateIPs's per-issuer guarantee for it. + // Splitting the cache per issuer makes that collision unrepresentable + // instead of guarding against it. + jwks *jwks.Fetcher mu sync.Mutex // jwksURL is resolved from OIDC discovery, or copied from // TrustedIssuer.JWKSURL when hand-configured. Once set, it is never - // cleared: unlike the JWKS document itself (which jwksCache keeps fresh - // on its own schedule — see ensureRegistered), a change to + // cleared: unlike the JWKS document itself (which the fetcher's cache + // keeps fresh on its own schedule), a change to // the *endpoint URL* an issuer serves its keys from requires a process // restart to pick up. Neither Microsoft Entra nor Okta documents rotating // that URL, only the keys served at it, and every other TrustedIssuer // field already requires a restart to take effect, so this is // consistent with the rest of this config's lifecycle. jwksURL string - // fetched is true once at least one JWKS fetch has succeeded for this - // issuer since process start. Until then, ensureRegistered forces a - // synchronous fetch on every call, since there is no cached value yet to - // fall back on and jwx's own background schedule offers no way to wait - // for its result. - fetched bool - // lastKidRefresh is the last time refreshOnUnknownKid forced a fetch; - // see minKidRefreshInterval. - lastKidRefresh time.Time - // fetchFailedAt and fetchErr gate retries once registered but never fetched: - // key resolution runs before signature verification, so without this an - // authenticated client can otherwise drive one real outbound fetch per - // token-exchange request just by naming an issuer whose endpoint is - // down. fetchErr is served directly while the gate is closed, so the - // caller still gets a specific error instead of a generic "try later". - // Not consulted once fetched is true: a healthy issuer, or one serving - // stale-but-valid keys through a later background-refresh failure, must - // never be gated. - fetchFailedAt time.Time - fetchErr error -} - -// limitedBodyTransport wraps an http.RoundTripper to cap every response body -// at max bytes, via http.MaxBytesReader rather than io.LimitReader: the -// latter truncates silently, which would let a caller parse a cut-off JWKS -// document as if it were complete, where the former surfaces a -// *http.MaxBytesError instead. Its error text ("http: request body too -// large") is written for the request-body case MaxBytesReader was designed -// for, so it reads oddly for a capped response — an accepted rough edge -// rather than justifying a custom ReadCloser. -// -// The nil first argument (http.ResponseWriter) is safe: MaxBytesReader only -// reaches it through a type assertion (`l.w.(requestTooLarger)`) used to tell -// a real server connection to close early, which — on a nil interface value -// — safely evaluates to false rather than panicking (net/http/request.go). -type limitedBodyTransport struct { - base http.RoundTripper - max int64 -} - -// RoundTrip delegates to base and then caps the returned body. The cap -// applies to every response, including a non-2xx one whose body the caller -// doesn't intend to parse — draining it is only ever io.Discard-ed, not -// unbounded, so this errs on the safe side rather than special-casing status -// codes. -func (t *limitedBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := t.base.RoundTrip(req) - if err != nil { - return nil, err - } - resp.Body = http.MaxBytesReader(nil, resp.Body, t.max) - return resp, nil + // discoveryFailedAt and discoveryErr gate discovery retries for an issuer + // whose OIDC discovery has never once succeeded — see + // jwksDiscoveryBackoff. Not consulted once jwksURL is set. + discoveryFailedAt time.Time + discoveryErr error } // newActorMatcherEngine creates a CEL engine for admin-authored actor matcher @@ -472,11 +407,11 @@ func cloneJWTBearerGrantPolicy(policy *JWTBearerGrantPolicy) *JWTBearerGrantPoli } // newExternalIssuerConfig builds the *externalIssuerConfig for a single -// already-validated TrustedIssuer: a dedicated HTTP client (scoped to that -// issuer's own InsecureAllowHTTP/AllowPrivateIPs), its body-size-capped -// transport, and its own jwk.Cache. Called once per issuer from -// NewMultiIssuerTokenValidator's constructor loop, after validateTrustedIssuer -// and the startup warnings have already run for ti. +// already-validated TrustedIssuer: a dedicated jwks.Fetcher scoped to that +// issuer's own InsecureAllowHTTP/AllowPrivateIPs, which owns the issuer's +// HTTP client (with its body-size-capped transport) and jwk.Cache. Called +// once per issuer from NewMultiIssuerTokenValidator's constructor loop, after +// validateTrustedIssuer and the startup warnings have already run for ti. func newExternalIssuerConfig(ti TrustedIssuer) (*externalIssuerConfig, error) { // Clone AllowedActors and AllowedDelegateClients so a caller mutating // their original slices in place (e.g. a future config reload) cannot @@ -492,71 +427,51 @@ func newExternalIssuerConfig(ti TrustedIssuer) (*externalIssuerConfig, error) { return nil, fmt.Errorf("issuer_url %q: %w", ti.IssuerURL, err) } - // Deliberately networking.NewHttpClientBuilder(), not - // NewHostScopedClientBuilder: that helper ORs - // INSECURE_DISABLE_URL_VALIDATION and an auto-localhost exemption into - // BOTH the HTTP-scheme and private-IP gates, so an unrelated env var — - // or a trusted issuer that merely happens to be on localhost — would - // silently widen AllowPrivateIPs regardless of what the operator set, - // defeating the point of splitting the two flags per issuer. - // - // Keep-alive connections are disabled: this client dials jwks_uri, a host taken - // from an untrusted discovery document, only on the fixed - // jwksRefreshInterval schedule plus occasional on-demand refreshes — - // no hot path here to trade the per-dial SSRF check away for. - builder := networking.NewHttpClientBuilder(). - WithInsecureAllowHTTP(ti.InsecureAllowHTTP). - WithPrivateIPs(ti.AllowPrivateIPs). - WithTimeout(httpTimeout). - WithDisableKeepAlives(true) - if ti.CAFilePath != "" { - builder = builder.WithSystemRootsPlusCABundle(ti.CAFilePath) - } - httpClient, err := builder.Build() - if err != nil { - return nil, fmt.Errorf("issuer_url %q: failed to build HTTP client: %w", ti.IssuerURL, err) - } - // Guard against a discovery/JWKS redirect hop landing on a - // different, unvetted host — the same policy the transparent proxy + // One jwks.Fetcher per issuer, whose HTTP client is built inside the + // fetcher from this issuer's own flags (see networking's + // NewHttpClientBuilder — deliberately not NewHostScopedClientBuilder, + // see the fetcher's buildHTTPClient comment for why env vars must not + // widen these gates). Keep-alive is disabled: the client dials + // jwks_uri, a host taken from an untrusted discovery document, only on + // the fixed jwksRefreshInterval schedule plus occasional on-demand + // refreshes — no hot path here to trade the per-dial SSRF check away + // for. Same-host redirects guard a discovery/JWKS redirect hop landing + // on a different, unvetted host — the same policy the transparent proxy // data path applies to a response derived from an untrusted remote // server (see SameHostRedirectPolicy's doc comment). - httpClient.CheckRedirect = networking.SameHostRedirectPolicy() - - // Cap every response body this client reads at maxResponseBodySize — - // discovery already enforces this itself via io.LimitReader - // (discoverJWKSURL), but the JWKS fetch is handed to jwx's jwk.Cache - // below, which has no equivalent cap of its own (httprc.MaxBufferSize - // is ~1000 MiB, and its transformer does an unbounded io.ReadAll under - // that ceiling before parsing). Wrapped OUTSIDE httpClient.Transport - // (which Build() always sets — see networking's builder) so the - // private-IP dial guard and ValidatingTransport's scheme check still - // run first, on the inner, unwrapped transport. - httpClient.Transport = &limitedBodyTransport{ - base: httpClient.Transport, - max: maxResponseBodySize, - } - - // One jwk.Cache per issuer (see externalIssuerConfig.jwksCache's doc - // comment for why), each running its own background worker pool - // (jwk.NewCache -> httprc.Client.Start) for the life of the process. - // WithWorkers(1) caps that pool to one worker per issuer instead of - // httprc's default five — budget roughly three goroutines per issuer - // including its controller loop and wait-group waiter. + fetcherOpts := []jwks.Option{ + jwks.WithInsecureAllowHTTP(ti.InsecureAllowHTTP), + jwks.WithAllowPrivateIPs(ti.AllowPrivateIPs), + jwks.WithTimeout(httpTimeout), + jwks.WithDisableKeepAlives(true), + jwks.WithSameHostRedirects(true), + // Pin the background refresh interval — see jwksRefreshInterval's + // doc comment for why an issuer must not choose it via Cache-Control. + jwks.WithRefreshInterval(jwksRefreshInterval), + // One background worker per issuer instead of httprc's default five — + // budget roughly three goroutines per issuer including its controller + // loop and wait-group waiter. Body cap, max key count, fetch-failure + // backoff, and the unknown-kid refresh gate keep the jwks package + // defaults (1 MiB / 100 keys / 30s / 30s). + jwks.WithWorkers(1), + } + if ti.CAFilePath != "" { + fetcherOpts = append(fetcherOpts, jwks.WithSystemRootsPlusCABundle(ti.CAFilePath)) + } // context.Background() is deliberate: there's no per-call context to - // root this in, and the loop is meant to outlive any single call, - // stopped only via jwk.Cache.Shutdown — which nothing here calls, - // matching pkg/auth/token.go's TokenValidator. - jwksCache, err := jwk.NewCache(context.Background(), httprc.NewClient(httprc.WithWorkers(1))) + // root the cache's background worker pool in, and the pool is meant to + // outlive any single call, stopped only via jwk.Cache.Shutdown — which + // nothing here calls, matching pkg/auth/token.go's TokenValidator. + fetcher, err := jwks.NewFetcher(context.Background(), fetcherOpts...) if err != nil { - return nil, fmt.Errorf("issuer_url %q: failed to create JWKS cache: %w", ti.IssuerURL, err) + return nil, fmt.Errorf("issuer_url %q: failed to create JWKS fetcher: %w", ti.IssuerURL, err) } return &externalIssuerConfig{ TrustedIssuer: ti, actorMatcher: actorMatcher, jwksURL: ti.JWKSURL, - httpClient: httpClient, - jwksCache: jwksCache, + jwks: fetcher, }, nil } @@ -663,12 +578,12 @@ func (v *MultiIssuerTokenValidator) verifyExternalSignature( return jwt.Claims{}, nil, fmt.Errorf("subject token is not a valid JWT: %w", err) } - jwks, err := v.lookupJWKS(ctx, issuerConfig) + keySet, err := v.lookupJWKS(ctx, issuerConfig) if err != nil { return jwt.Claims{}, nil, fmt.Errorf("failed to fetch JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) } - standardClaims, extraClaims, kidMatched, err := verifySignature(parsedToken, jwks) + standardClaims, extraClaims, kidMatched, err := verifySignature(parsedToken, keySet) if err != nil && !kidMatched { // The token's kid isn't among the keys we have cached — possibly a // legitimate rotation the issuer's cache hasn't caught up with yet @@ -676,10 +591,12 @@ func (v *MultiIssuerTokenValidator) verifyExternalSignature( // Force an immediate re-fetch and retry once before giving up; a // spoofed-kid attempt (kidMatched true) never reaches this branch, // since a refresh can't change what signature the token was made - // with. - v.refreshOnUnknownKid(ctx, issuerConfig) - if refreshedJWKS, lookupErr := v.lookupJWKS(ctx, issuerConfig); lookupErr == nil { - standardClaims, extraClaims, _, err = verifySignature(parsedToken, refreshedJWKS) + // with. The fetcher rate-limits the forced refresh (see + // jwks.Fetcher.RefreshOnUnknownKid) so repeated made-up kids cannot + // drive a fetch per request. + issuerConfig.jwks.RefreshOnUnknownKid(ctx, issuerConfig.jwksURL) + if refreshedKeySet, lookupErr := v.lookupJWKS(ctx, issuerConfig); lookupErr == nil { + standardClaims, extraClaims, _, err = verifySignature(parsedToken, refreshedKeySet) } } if err != nil { @@ -797,135 +714,60 @@ func checkMayActAllowed(extraClaims map[string]any, selfIssuer string, issuerCon return validateMayActShape(extraClaims, selfIssuer, true) } -// ensureRegistered resolves issuerConfig.jwksURL — via OIDC discovery on -// first use — and registers it with issuerConfig's own jwk.Cache (see -// registerOrRefresh for the Register-vs-Refresh decision). Discovery -// happens at most once per issuer for the life of the process; see +// resolveJWKSURL resolves issuerConfig.jwksURL — via OIDC discovery on first +// use — under issuerConfig.mu so concurrent validations single-flight it. +// Discovery happens at most once per issuer for the life of the process; see // externalIssuerConfig's jwksURL doc comment. // -// The JWKS fetch is retried until one succeeds (fetched stays false -// otherwise), gated by jwksFetchFailureBackoff: key resolution runs before -// the subject token's signature is checked, so without this gate an -// authenticated client could force a real outbound attempt on every -// request to an issuer whose endpoint is down. The gate only applies -// before the first successful fetch — once fetched is true, a healthy -// issuer or one serving stale-but-valid keys through a later refresh -// failure is unaffected. While closed, the last error is replayed -// directly rather than retried. -// -// Every background refresh, not just the first request-triggered one, -// goes through the maxResponseBodySize-capped transport (see -// limitedBodyTransport) despite having no request or client authentication -// of its own — without that cap a compromised issuer could force an -// oversized allocation on every autonomous refresh, indefinitely. -func (v *MultiIssuerTokenValidator) ensureRegistered(ctx context.Context, issuerConfig *externalIssuerConfig) error { +// Discovery failures are retried until one succeeds (jwksURL stays empty +// otherwise), gated by jwksDiscoveryBackoff: discovery runs before the +// subject token's signature is checked, so without this gate an authenticated +// client could force a real outbound discovery request on every request to an +// issuer whose discovery endpoint is down. While closed, the last error is +// replayed directly rather than retried. The JWKS fetch itself is gated +// separately, inside the issuer's jwks.Fetcher (see jwks.Fetcher's +// EnsureRegistered). +func (v *MultiIssuerTokenValidator) resolveJWKSURL(ctx context.Context, issuerConfig *externalIssuerConfig) error { issuerConfig.mu.Lock() defer issuerConfig.mu.Unlock() - if issuerConfig.fetched { + if issuerConfig.jwksURL != "" { return nil } - if time.Since(issuerConfig.fetchFailedAt) < jwksFetchFailureBackoff { - return issuerConfig.fetchErr - } - - if err := v.registerOrRefresh(ctx, issuerConfig); err != nil { - issuerConfig.fetchErr = err - issuerConfig.fetchFailedAt = time.Now() - return err + if time.Since(issuerConfig.discoveryFailedAt) < jwksDiscoveryBackoff { + return issuerConfig.discoveryErr } - issuerConfig.fetched = true - issuerConfig.fetchErr = nil - return nil -} -// registerOrRefresh performs the actual discovery/registration/fetch -// attempt for issuerConfig; ensureRegistered holds issuerConfig.mu across -// this call, single-flighting it per issuer so concurrent validations -// don't pile up N redundant fetches. -// -// Whether jwksURL is already registered is asked of -// issuerConfig.jwksCache.IsRegistered directly, never remembered in a -// field: Register's own registration step is a channel send to the -// cache's backend goroutine and can fail after enqueue but before receipt -// (context deadline), in which case nothing was actually registered. A -// locally remembered "we called Register" flag can't distinguish that -// from "registered, only the fetch failed", and would wrongly keep -// retrying via Refresh — which errors on a URL the cache never heard of — -// forever after. Asking the cache directly is authoritative either way. -// -// IsRegistered makes no network call but isn't free: it's a round-trip -// over that same channel, so it blocks if the backend is busy and returns -// false (not an error) on context expiry. Its own timeout below matters -// for that reason — a false from an expired context just routes to -// Register, whose "already registered" error is transient and absorbed by -// ensureRegistered's backoff gate. -func (v *MultiIssuerTokenValidator) registerOrRefresh(ctx context.Context, issuerConfig *externalIssuerConfig) error { - // Detach from the caller's request context throughout this function: net/http - // cancels ctx when the client disconnects, and this runs before the subject - // token's signature is even checked, so an aborted connection must not cut off - // work other in-flight validations of this issuer are waiting on (mu, held by - // the caller), nor let repeating the abort drive unbounded outbound requests - // to the external IdP. + // Detach from the caller's request context: net/http cancels ctx when the + // client disconnects, and this runs before the subject token's signature + // is even checked, so an aborted connection must not cut off work other + // in-flight validations of this issuer are waiting on (mu, held here), nor + // let repeating the abort drive unbounded outbound requests to the + // external IdP. detached := context.WithoutCancel(ctx) - - if issuerConfig.jwksURL == "" { - discoverCtx, cancel := context.WithTimeout(detached, httpTimeout) - jwksURL, err := v.discoverJWKSURL(discoverCtx, issuerConfig) - cancel() - if err != nil { - return fmt.Errorf("OIDC discovery failed for %s: %w", issuerConfig.IssuerURL, err) - } - issuerConfig.jwksURL = jwksURL - } - - // Validate the JWKS URL here, in the single choke point every - // registration passes through — whether it was hand-configured on - // TrustedIssuer or just discovered above. A configured JWKSURL never - // reaches discoverJWKSURL, so checking only there would leave - // hand-configured URLs unvalidated. - if err := ValidateJWKSURL(issuerConfig.jwksURL, issuerConfig.InsecureAllowHTTP, issuerConfig.AllowPrivateIPs); err != nil { - return fmt.Errorf("jwks_url for issuer %s is invalid: %w", issuerConfig.IssuerURL, err) - } - - registeredCtx, cancel := context.WithTimeout(detached, httpTimeout) - registered := issuerConfig.jwksCache.IsRegistered(registeredCtx, issuerConfig.jwksURL) + discoverCtx, cancel := context.WithTimeout(detached, httpTimeout) + jwksURL, err := v.discoverJWKSURL(discoverCtx, issuerConfig) cancel() - - if registered { - // Already registered with this issuer's own cache, on a prior call - // whose own fetch never completed successfully — the only way this - // can be true, now that each issuer has its own cache. Register - // would error on an already-tracked URL, so retry via Refresh - // instead. - fetchCtx, cancel := context.WithTimeout(detached, httpTimeout) - defer cancel() - if _, err := issuerConfig.jwksCache.Refresh(fetchCtx, issuerConfig.jwksURL); err != nil { - return fmt.Errorf("failed to fetch JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) - } - return nil + if err != nil { + issuerConfig.discoveryErr = fmt.Errorf("OIDC discovery failed for %s: %w", issuerConfig.IssuerURL, err) + issuerConfig.discoveryFailedAt = time.Now() + return issuerConfig.discoveryErr } - // A newly created httprc.Resource is always scheduled to fetch - // immediately, so Register's own default WithWaitReady(true) blocks on - // that single automatic fetch. An explicit Refresh call right here would - // race it and issue a genuine second outbound request — that's why the - // registered branch above, not this one, is where Refresh is used. - fetchCtx, cancel := context.WithTimeout(detached, httpTimeout) - defer cancel() - if err := issuerConfig.jwksCache.Register(fetchCtx, issuerConfig.jwksURL, - jwk.WithHTTPClient(issuerConfig.httpClient), jwk.WithConstantInterval(jwksRefreshInterval)); err != nil { - return fmt.Errorf("failed to register JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) - } + issuerConfig.jwksURL = jwksURL + issuerConfig.discoveryErr = nil return nil } -// lookupJWKS returns issuerConfig's current JWKS, registering and fetching it -// first if this is the first use (see ensureRegistered). jwk.Cache serves the -// last successfully fetched Set even while a later background refresh is -// failing (httprc only stores a value after a successful fetch), so a -// transient outage at an issuer that has already been reached once no longer -// surfaces as a validation failure. +// lookupJWKS returns issuerConfig's current JWKS, discovering the issuer's +// jwks_url on first use (see resolveJWKSURL) and fetching it through the +// issuer's own jwks.Fetcher. Registration, refresh-on-retry, the +// fetch-failure backoff gate, stale-on-error, and the response-body cap now +// live in the fetcher (see pkg/auth/jwks): jwk.Cache serves the last +// successfully fetched Set even while a later background refresh is failing +// (httprc only stores a value after a successful fetch), so a transient +// outage at an issuer that has already been reached once no longer surfaces +// as a validation failure. // Each call converts the cached Set into a fresh *jose.JSONWebKeySet — the // two libraries' key representations aren't shared, so nothing here is // visible to, or mutable by, any other concurrent caller. @@ -933,28 +775,25 @@ func (v *MultiIssuerTokenValidator) lookupJWKS( ctx context.Context, issuerConfig *externalIssuerConfig, ) (*jose.JSONWebKeySet, error) { - if err := v.ensureRegistered(ctx, issuerConfig); err != nil { + if err := v.resolveJWKSURL(ctx, issuerConfig); err != nil { return nil, err } - set, err := issuerConfig.jwksCache.Lookup(ctx, issuerConfig.jwksURL) + set, err := issuerConfig.jwks.KeySet(ctx, issuerConfig.jwksURL) if err != nil { - return nil, fmt.Errorf("failed to lookup JWKS: %w", err) + return nil, fmt.Errorf("failed to fetch JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) } - jwks, err := bridgeJWKSet(set) + keySet, err := bridgeJWKSet(set) if err != nil { return nil, err } - if len(jwks.Keys) == 0 { + if len(keySet.Keys) == 0 { return nil, errors.New("JWKS contains no keys") } - if len(jwks.Keys) > maxJWKSKeys { - return nil, fmt.Errorf("JWKS contains too many keys: %d (max %d)", len(jwks.Keys), maxJWKSKeys) - } - return jwks, nil + return keySet, nil } // bridgeJWKSet converts a jwx jwk.Set into the go-jose jose.JSONWebKeySet @@ -968,38 +807,11 @@ func bridgeJWKSet(set jwk.Set) (*jose.JSONWebKeySet, error) { if err != nil { return nil, fmt.Errorf("failed to marshal JWKS: %w", err) } - var jwks jose.JSONWebKeySet - if err := json.Unmarshal(raw, &jwks); err != nil { + var keySet jose.JSONWebKeySet + if err := json.Unmarshal(raw, &keySet); err != nil { return nil, fmt.Errorf("failed to parse JWKS: %w", err) } - return &jwks, nil -} - -// refreshOnUnknownKid forces issuerConfig's own jwk.Cache to re-fetch its -// JWKS immediately, ahead of jwx's own background refresh schedule, when a -// subject token names a kid the last cached JWKS doesn't have — the -// situation a legitimate key rotation produces. Gated by minKidRefreshInterval -// and single-flighted via issuerConfig.mu, the same mutex ensureRegistered -// uses for its own initial fetch. -// -// Errors are logged, not returned: the caller (validateExternalToken) has -// already failed signature verification once and will simply fail again if -// the refresh didn't produce a usable key, which is the correct outcome for -// a genuinely invalid token. -func (*MultiIssuerTokenValidator) refreshOnUnknownKid(ctx context.Context, issuerConfig *externalIssuerConfig) { - issuerConfig.mu.Lock() - defer issuerConfig.mu.Unlock() - - if time.Since(issuerConfig.lastKidRefresh) < minKidRefreshInterval { - return - } - issuerConfig.lastKidRefresh = time.Now() - - fetchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*httpTimeout) - defer cancel() - if _, err := issuerConfig.jwksCache.Refresh(fetchCtx, issuerConfig.jwksURL); err != nil { - slog.Debug("JWKS refresh on unknown kid failed", "issuer", issuerConfig.IssuerURL, "error", err) - } + return &keySet, nil } // peekIssuer parses a JWT without signature verification to extract the "iss" claim. @@ -1023,7 +835,9 @@ func peekIssuer(rawToken string) (string, error) { // discoverJWKSURL performs OIDC discovery to resolve the JWKS URL for an issuer. // It fetches the OpenID Connect discovery document at {issuerURL}/.well-known/openid-configuration -// and extracts the jwks_uri field, using issuerConfig's own dedicated HTTP client. +// and extracts the jwks_uri field, using the issuer's own dedicated HTTP +// client (the fetcher's client, so discovery is guarded by exactly the +// transport policy the JWKS fetch is guarded by). // // IssuerURL itself may legitimately carry a trailing slash (see // validateTrustedIssuerURL in pkg/authserver/config.go — Microsoft Entra ID @@ -1040,7 +854,7 @@ func (*MultiIssuerTokenValidator) discoverJWKSURL(ctx context.Context, issuerCon return "", fmt.Errorf("failed to create discovery request: %w", err) } - resp, err := issuerConfig.httpClient.Do(req) + resp, err := issuerConfig.jwks.HTTPClient().Do(req) if err != nil { return "", fmt.Errorf("discovery request failed: %w", err) } @@ -1075,54 +889,6 @@ func (*MultiIssuerTokenValidator) discoverJWKSURL(ctx context.Context, issuerCon return doc.JWKSURI, nil } -// ValidateJWKSURL checks that jwksURL parses, has a host, uses HTTPS unless -// insecureAllowHTTP permits plain HTTP — and only exactly the "http" scheme, -// not any other non-https scheme such as "file" or "ftp" — and, when the -// host is an IP literal, is not a private or loopback address unless -// allowPrivateIPs permits that. Both flags come from the specific -// TrustedIssuer being fetched (see ensureRegistered), never from a validator-wide -// or self-issuer setting. This prevents SSRF attacks where a compromised -// discovery document — or a hand-configured jwks_url — points to internal -// services. -// -// This is the single implementation shared by the runtime choke point above -// (ensureRegistered, on every fetch) and pkg/authserver/config.go's config-time -// check (validateJWKSEndpointURL): the two must not drift out of sync, or a -// laxer runtime check would silently defeat the config-time guard. -func ValidateJWKSURL(jwksURL string, insecureAllowHTTP, allowPrivateIPs bool) error { - u, err := url.Parse(jwksURL) - if err != nil { - return fmt.Errorf("invalid URL: %w", err) - } - - if u.Host == "" { - return errors.New("host is required") - } - - // Unlike issuer_url, a jwks_url carrying userinfo would actually work — - // net/http turns it into a Basic auth header on every JWKS fetch — which - // is precisely why it is rejected rather than tolerated: it would put a - // live credential in the RunConfig, in this function's error strings, and - // in any log that quotes the URL. A JWKS endpoint is public by - // definition (it serves verification keys), so there is no legitimate - // reason to authenticate to one. - if u.User != nil { - return errors.New("must not contain userinfo (credentials in the URL)") - } - - if u.Scheme != "https" && (u.Scheme != "http" || !insecureAllowHTTP) { - return fmt.Errorf("must use HTTPS, got %q", u.Scheme) - } - - host := u.Hostname() - ip := net.ParseIP(host) - if ip != nil && !allowPrivateIPs && networking.IsPrivateIP(ip) { - return errors.New("must not point to a private or loopback address") - } - - return nil -} - // validateTrustedIssuer checks a single TrustedIssuer for structural validity // before it is admitted into issuers: required fields, no collision with // selfIssuer or an already-registered issuer, an ActorClaim that diff --git a/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go b/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go index 4338f42bbe..3605ba6db1 100644 --- a/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go +++ b/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go @@ -9,7 +9,6 @@ import ( "encoding/json" "encoding/pem" "fmt" - "io" "log/slog" "net/http" "net/http/httptest" @@ -26,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stacklok/toolhive/pkg/auth/jwks" "github.com/stacklok/toolhive/pkg/networking" ) @@ -104,6 +104,18 @@ func newMultiValidator( return v } +// discoveryFetcher builds a jwks.Fetcher around the given client, for tests +// exercising discoverJWKSURL directly: discovery reads the fetcher's client +// (issuerConfig.jwks.HTTPClient()), so there is no separate per-issuer client +// field to set anymore. +func discoveryFetcher(t *testing.T, client *http.Client) *jwks.Fetcher { + t.Helper() + + f, err := jwks.NewFetcher(context.Background(), jwks.WithHTTPClient(client)) + require.NoError(t, err) + return f +} + // externalClaims returns standard JWT claims for a token issued by the external issuer. func externalClaims() jwt.Claims { now := time.Now() @@ -1224,55 +1236,6 @@ func TestMultiIssuerTokenValidator_JWKSCaching(t *testing.T) { assert.Equal(t, int32(1), fetchCount.Load(), "JWKS should be fetched only once due to caching") } -// TestMultiIssuerTokenValidator_JWKSRefreshIntervalIsPinned confirms -// registerOrRefresh actually threads jwk.WithConstantInterval(jwksRefreshInterval) -// through to the underlying httprc.Resource, even though the JWKS endpoint -// advertises a much longer Cache-Control max-age. Without the constant -// interval, httprc would derive the refresh schedule from that header -// instead — see jwksRefreshInterval's doc comment for why an external -// issuer must not get to choose how long we keep its keys cached. -// -// This can't be observed by waiting for a second fetch without a wall-clock -// sleep (forbidden by this repo's testing rules), so it asserts directly on -// the registered resource's ConstantInterval() instead of on fetch timing. -func TestMultiIssuerTokenValidator_JWKSRefreshIntervalIsPinned(t *testing.T) { - t.Parallel() - - selfJWKS := newTestJWKS(t) - externalJWKS := newTestJWKS(t) - - mux := http.NewServeMux() - mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { - // A long max-age that would push httprc's derived interval far past - // jwksRefreshInterval if the constant interval were not applied. - w.Header().Set("Cache-Control", "max-age=2592000") // 30 days - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) - }) - jwksServer := httptest.NewServer(mux) - t.Cleanup(jwksServer.Close) - - trustedIssuers := []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", - AllowedActors: []string{"ext-agent"}, - AllowedDelegateClients: []string{anyDelegateClient}, - }} - - validator := newMultiValidator(t, selfJWKS, trustedIssuers) - - rawToken := externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": "ext-agent"}) - _, err := validator.Validate(context.Background(), rawToken) - require.NoError(t, err) - - issuerConfig := validator.issuers[testExternalIssuer] - resource, err := issuerConfig.jwksCache.LookupResource(context.Background(), issuerConfig.jwksURL) - require.NoError(t, err) - assert.Equal(t, jwksRefreshInterval, resource.ConstantInterval(), - "registered resource must ignore the endpoint's own Cache-Control max-age") -} - func TestNewMultiIssuerTokenValidator_Validation(t *testing.T) { t.Parallel() @@ -1881,7 +1844,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, srv.URL + "/jwks" }, }, @@ -1910,7 +1873,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { } return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL + "/", AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: client, + jwks: discoveryFetcher(t, client), }, srv.URL + "/jwks" }, }, @@ -1922,7 +1885,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { // A raw control character is rejected by url.Parse inside // http.NewRequestWithContext, before any network I/O. TrustedIssuer: TrustedIssuer{IssuerURL: "http://example.com/\x00", AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: &http.Client{}, + jwks: discoveryFetcher(t, &http.Client{}), }, "" }, errContains: "failed to create discovery request", @@ -1935,7 +1898,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { srv.Close() // closed before use: the port is now guaranteed unreachable. return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, errContains: "discovery request failed", @@ -1949,7 +1912,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, errContains: "discovery endpoint returned status 500", @@ -1964,7 +1927,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, // The distinguishing substring from json.Unmarshal itself, not @@ -1999,7 +1962,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, errContains: "unexpected end of JSON input", @@ -2017,7 +1980,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, errContains: "does not match expected issuer", @@ -2035,7 +1998,7 @@ func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { }) return &externalIssuerConfig{ TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, - httpClient: srv.Client(), + jwks: discoveryFetcher(t, srv.Client()), }, "" }, errContains: "missing 'jwks_uri'", @@ -2148,10 +2111,10 @@ func TestMultiIssuerTokenValidator_KidMismatch(t *testing.T) { assert.Nil(t, result) } -// TestValidateJWKSURL exercises ValidateJWKSURL directly: this is the SSRF -// guard applied in ensureRegistered to every JWKS URL for a given issuer, whether -// hand-configured on TrustedIssuer or resolved via discovery, and shared -// verbatim with pkg/authserver/config.go's config-time check +// TestValidateJWKSURL exercises jwks.ValidateJWKSURL directly: this is the SSRF +// guard applied by jwks.Fetcher.EnsureRegistered to every JWKS URL for a given +// issuer, whether hand-configured on TrustedIssuer or resolved via discovery, +// and shared verbatim with pkg/authserver/config.go's config-time check // (validateJWKSEndpointURL) so the two can't drift out of sync. The // equivalent check on redirect hops (networking.SameHostRedirectPolicy) and // the dial-time IP guard (networking.NewHostScopedClientBuilder) are @@ -2218,7 +2181,7 @@ func TestValidateJWKSURL(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := ValidateJWKSURL(tt.url, tt.insecureAllowHTTP, false) + err := jwks.ValidateJWKSURL(tt.url, tt.insecureAllowHTTP, false) if tt.wantErr == "" { assert.NoError(t, err) return @@ -2229,30 +2192,31 @@ func TestValidateJWKSURL(t *testing.T) { } } -// TestMultiIssuerTokenValidator_FetchJWKS exercises ensureRegistered's and +// TestMultiIssuerTokenValidator_FetchJWKS exercises the fetcher's and // lookupJWKS's error paths through the full Validate path, bypassing OIDC -// discovery via a preconfigured JWKSURL. Registration and the JWKS's own -// zero-keys/too-many-keys checks now go through the issuer's own jwk.Cache and -// lookupJWKS respectively rather than a private HTTP fetch. +// discovery via a preconfigured JWKSURL. Registration and the JWKS's +// too-many-keys check now go through the issuer's own jwks.Fetcher (whose +// registration, body cap, and key-count internals are exercised directly by +// pkg/auth/jwks' own tests), while the zero-keys check stays in lookupJWKS. // // For "non-200 response" and "malformed JSON body", verified empirically: // httprc.Resource's `ready` channel only ever closes on a *successful* -// fetch, so Register's WithWaitReady(true) wait (ensureRegistered's +// fetch, so Register's WithWaitReady(true) wait (the fetcher's // first-ever-registration path) can't distinguish "still fetching" from // "fetch failed" — it just blocks until fetchCtx's own deadline and returns // a generic timeout, discarding the real HTTP/parse error, which is why // these two cases assert on "context deadline exceeded" rather than on // jwx's own status-code or parse-error text (that text only ever surfaces // on a *later* validation of the same never-yet-succeeded issuer, via -// ensureRegistered's Refresh-based retry branch — not exercised by a single +// the fetcher's Refresh-based retry branch — not exercised by a single // Validate call here). "zero keys" and "too many keys" are unaffected: a -// JWKS that parses but fails this file's own key-count checks still -// registers successfully, so lookupJWKS's checks run immediately. +// JWKS that parses but fails the key-count checks still +// registers successfully, so those checks run immediately. func TestMultiIssuerTokenValidator_FetchJWKS(t *testing.T) { t.Parallel() - // Built once: maxJWKSKeys+1 distinct public keys for the "too many keys" case. - tooManyKeys := make([]jose.JSONWebKey, maxJWKSKeys+1) + // Built once: jwks.DefaultMaxKeys+1 distinct public keys for the "too many keys" case. + tooManyKeys := make([]jose.JSONWebKey, jwks.DefaultMaxKeys+1) for i := range tooManyKeys { key := newECDSAJWK(t, fmt.Sprintf("k%d", i)) tooManyKeys[i] = key.Public() @@ -2449,7 +2413,8 @@ func TestMultiIssuerTokenValidator_KeyRotationRefreshesImmediately(t *testing.T) } // TestMultiIssuerTokenValidator_UnknownKidRefreshIsRateLimited proves -// refreshOnUnknownKid's minKidRefreshInterval gate: repeated subject tokens +// jwks.Fetcher.RefreshOnUnknownKid's minKidRefreshInterval gate: repeated +// subject tokens // naming a kid absent from the cached JWKS must not force a fetch per // request — only the first ever unknown-kid attempt (within the interval) // may do so. @@ -2494,13 +2459,15 @@ func TestMultiIssuerTokenValidator_UnknownKidRefreshIsRateLimited(t *testing.T) } // Only the first unknown-kid attempt should have forced a refresh; the - // gate must hold for the remaining four within minKidRefreshInterval. + // gate must hold for the remaining four within the fetcher's + // minKidRefreshInterval window. assert.Equal(t, primedFetches+1, fetchCount.Load(), "repeated unknown-kid tokens within the window must not each force a fetch") } // TestMultiIssuerTokenValidator_NeverFetchedRetryIsRateLimited proves -// ensureRegistered's jwksFetchFailureBackoff gate: an issuer whose endpoint +// jwks.WithFetchFailureBackoff's fetch-failure backoff gate: an issuer whose +// endpoint // has never once succeeded must not be re-fetched on every request — key // resolution runs before signature verification, so without this gate any // client holding a subject token naming this issuer could drive one real @@ -2530,13 +2497,14 @@ func TestMultiIssuerTokenValidator_NeverFetchedRetryIsRateLimited(t *testing.T) rawToken := signExternalToken(t, newECDSAJWK(t, "any-kid"), externalClaims()) // First attempt: no cached value yet, so this one genuinely fetches (and - // blocks on Register's own wait — see ensureRegistered's doc comment). + // blocks on Register's own wait — see jwks.Fetcher.EnsureRegistered's + // doc comment). _, err := validator.Validate(context.Background(), rawToken) require.Error(t, err) require.Equal(t, int32(1), fetchCount.Load()) - // Further attempts within jwksFetchFailureBackoff must replay the stored - // error instead of fetching again. + // Further attempts within the fetch-failure backoff window must replay the + // stored error instead of fetching again. for range 3 { _, err := validator.Validate(context.Background(), rawToken) require.Error(t, err) @@ -2548,7 +2516,8 @@ func TestMultiIssuerTokenValidator_NeverFetchedRetryIsRateLimited(t *testing.T) // TestMultiIssuerTokenValidator_SharedJWKSURL_SamePolicy proves that two // issuers resolving to the identical jwksURL under the identical HTTP // transport policy both validate successfully — each through its own -// jwk.Cache and *http.Client (see externalIssuerConfig.jwksCache). This is +// jwks.Fetcher (see externalIssuerConfig.jwks), which owns its own jwk.Cache +// and *http.Client. This is // the common real-world case: Microsoft Entra v1 tenants share one // tenant-independent JWKS endpoint, so two Entra tenants configured as // separate trusted issuers collide on the same jwks_url by construction. @@ -2697,101 +2666,3 @@ func TestMultiIssuerTokenValidator_SharedJWKSURL_DifferingPolicy(t *testing.T) { "neither blocked by nor inheriting issuer A's stricter policy") assert.Equal(t, issuerBURL, resultB.ExternalIssuer) } - -// TestMultiIssuerTokenValidator_RetryAfterFetchFailureRefreshes proves the -// regression-safety half of the ensureRegistered rewrite (removing -// externalIssuerConfig.added in favor of asking issuerConfig.jwksCache.IsRegistered -// directly): once a JWKS fetch has failed but the resource was genuinely -// registered with the issuer's own cache, a later retry — once -// jwksFetchFailureBackoff has elapsed — must refresh the existing -// registration rather than attempt to register it again, which would fail -// with "already registered". -func TestMultiIssuerTokenValidator_RetryAfterFetchFailureRefreshes(t *testing.T) { - t.Parallel() - - selfJWKS := newTestJWKS(t) - externalJWKS := newTestJWKS(t) - - var succeed atomic.Bool - var fetchCount atomic.Int32 - mux := http.NewServeMux() - mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { - fetchCount.Add(1) - if !succeed.Load() { - w.WriteHeader(http.StatusInternalServerError) - return - } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) - }) - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - - trustedIssuers := []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: srv.URL + "/jwks", - AllowedActors: []string{"ext-agent"}, - AllowedDelegateClients: []string{anyDelegateClient}, - }} - validator := newMultiValidator(t, selfJWKS, trustedIssuers) - - tokenWithID := func(jti string) string { - claims := externalClaims() - claims.ID = jti - return externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) - } - - // First attempt: the endpoint is broken. This genuinely registers the - // resource with the issuer's own cache (per httprc.Controller.Add's own - // ordering), but the fetch itself fails. - _, err := validator.Validate(context.Background(), tokenWithID("jti-1")) - require.Error(t, err) - require.Equal(t, int32(1), fetchCount.Load()) - - // Force the backoff gate open, as if jwksFetchFailureBackoff had elapsed, - // without waiting the real 30s out. - issuerConfig := validator.issuers[testExternalIssuer] - issuerConfig.mu.Lock() - issuerConfig.fetchFailedAt = time.Now().Add(-jwksFetchFailureBackoff - time.Second) - issuerConfig.mu.Unlock() - - // The endpoint now recovers. The retry must go through Refresh — a - // second Register call against the same URL would fail with "already - // registered". - succeed.Store(true) - result, err := validator.Validate(context.Background(), tokenWithID("jti-2")) - require.NoError(t, err, "retry after a registered-but-failed fetch must refresh, not re-register") - require.NotNil(t, result) -} - -// TestLimitedBodyTransport asserts directly on the body cap that protects the -// JWKS fetch path. A direct test is necessary rather than sufficient coverage -// via Validate: jwx surfaces every fetch failure as its own WaitReady timeout, -// so the cap's error never reaches a caller and cannot be distinguished there -// from a 500, a parse failure, or a kid mismatch. Asserting on the cap itself -// is the only way to pin it — the oversized case in -// TestMultiIssuerTokenValidator_FetchJWKS proves the fetch fails, not why. -func TestLimitedBodyTransport(t *testing.T) { - t.Parallel() - - const bodyCap = 1024 - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(strings.Repeat("a", 8*1024))) - })) - t.Cleanup(srv.Close) - - client := srv.Client() - client.Transport = &limitedBodyTransport{base: client.Transport, max: bodyCap} - - resp, err := client.Get(srv.URL) - require.NoError(t, err, "the cap applies to reading the body, not to the round trip") - t.Cleanup(func() { _ = resp.Body.Close() }) - - body, err := io.ReadAll(resp.Body) - require.Error(t, err, "reading past the cap must fail rather than truncate silently: "+ - "a truncated JWKS would be parsed as though it were the whole document") - assert.LessOrEqual(t, int64(len(body)), int64(bodyCap), - "no more than the cap may be delivered before the error") -} diff --git a/pkg/authserver/upstream/oidc.go b/pkg/authserver/upstream/oidc.go index 742a6d5454..58c1017975 100644 --- a/pkg/authserver/upstream/oidc.go +++ b/pkg/authserver/upstream/oidc.go @@ -185,6 +185,12 @@ func NewOIDCProvider( if err != nil { return nil, fmt.Errorf("failed to create HTTP client: %w", err) } + // Note: this provider's ID-token verification uses coreos/go-oidc's + // RemoteKeySet for its JWKS — the third JWKS mechanism in the codebase, + // alongside pkg/auth/jwks.Fetcher (TokenValidator, token exchange) and + // the key provider of the embedded auth server. It is deliberately out + // of scope for the shared fetcher (issue #6319): replacing it would mean + // reimplementing go-oidc's ID-token verification and nonce handling. p := &OIDCProviderImpl{ oidcConfig: config,