fix(auth): a validator whose issuer is down at boot is built anyway
CI / Lint / Test / Vet (push) Successful in 5s
CI / Lint / Test / Vet (push) Successful in 5s
Closes #4. Closes gitea-mcp#57. Filed 2026-07-17, filed again 2026-09-07, recurred 2026-09-14. Each occurrence ended with a human running `kubectl rollout restart`; the last one ran for six days and was found by chance while gathering context for unrelated work. THE BUG NewMultiJWTValidator resolved OIDC discovery eagerly and returned (nil, err) if the issuer was not answering. BearerMiddleware reads a nil validator as "this server does not do JWT auth", so it fell through to the static-token comparison and rejected every Authentik JWT: WARN mcp auth rejected reason=static_token_mismatch token_type=jwt status=401 Two states collapsed into one nil: static-token-only by design -> nil correct JWT required, IdP down at boot -> nil wrong, and permanent Permanent because discovery was attempted exactly once, at construction. WHY THE FIX IS SMALL Everything downstream was already right. Validate distinguishes ErrUnavailable (issuer unreachable -> 503, client retries) from a definitive rejection (-> 401), and BearerMiddleware already honours that — gitea-mcp#6 got it right. That path was simply unreachable when the validator was never built. So: build it anyway. jwks_uri now resolves lazily on first use and re-resolves on failure, no sooner than WithRetryInterval apart (default 30s) so a down issuer costs one discovery request per interval, not one per request. Until it resolves, Validate returns ErrUnavailable and the existing machinery does the rest. The constructor still warms discovery eagerly on a healthy start; it just no longer fails on it. Ready(ctx) reports whether every issuer resolved, and attempts resolution for any that have not. That makes a readiness probe the retry loop — no background goroutine, no context to leak, and the platform's own cadence drives recovery. A nil validator is Ready: "JWT auth disabled" is a configuration, not a fault. WHAT DID NOT CHANGE No issuer configured still returns (nil, nil). That is the one case where nil is not a lie, and collapsing it would turn every static-only server into a JWT server. Pinned by TestNewMultiJWTValidator_NoIssuer_StillReturnsNilNil. ONE EXISTING TEST CHANGED ITS MECHANISM, NOT ITS CLAIM TestNewMultiJWTValidator_UsesPerIssuerHTTPClientForAuthedFetch asserted that the constructor ERRORS when discovery 401s without the fetch credential. It cannot, now, by design. Rewritten to assert the property that actually mattered, and more directly than before: without the credential the validator is not Ready AND a correctly-minted token from that very issuer is refused as ErrUnavailable. The old test only checked that a constructor returned an error. Written test-first: jwt_bootrace_test.go red (API absent), then green. Also red in the behavioural sense — the constructor-errors assertion above had to be rewritten precisely because the semantics genuinely changed. Verified: golangci-lint 0 issues; go vet clean; go test -race -count=1 ./... ok. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+151
-32
@@ -19,6 +19,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/lestrrat-go/jwx/v2/jwk"
|
||||
@@ -48,12 +49,99 @@ type IssuerConfig struct {
|
||||
HTTPClient *http.Client
|
||||
}
|
||||
|
||||
// issuerEntry is a resolved IssuerConfig: OIDC discovery has run and jwks_uri is
|
||||
// registered in the shared cache.
|
||||
// issuerEntry is one trusted issuer. jwksURI is resolved LAZILY: OIDC discovery
|
||||
// may not be answerable when the process starts, and an issuer that is merely
|
||||
// not-yet-serving must not become a permanent auth outage (mcp-chassis#4,
|
||||
// gitea-mcp#57 — three recurrences, each ended by a human running
|
||||
// `kubectl rollout restart`).
|
||||
//
|
||||
// Everything downstream of resolution was already correct. Validate reports
|
||||
// ErrUnavailable when it cannot reach an issuer, and BearerMiddleware already
|
||||
// answers 503-and-retry for that. Resolving eagerly in the constructor was the
|
||||
// one step that made the correct path unreachable.
|
||||
type issuerEntry struct {
|
||||
issuer string
|
||||
audience string
|
||||
jwksURI string
|
||||
issuer string
|
||||
audience string
|
||||
httpClient *http.Client
|
||||
|
||||
mu sync.Mutex
|
||||
jwksURI string // "" until discovery has succeeded
|
||||
lastAttempt time.Time // zero until the first attempt
|
||||
lastErr error
|
||||
}
|
||||
|
||||
// resolve returns this issuer's jwks_uri, running OIDC discovery if it has not
|
||||
// succeeded yet. Failures are retried, no sooner than retryInterval apart, so a
|
||||
// down issuer costs one discovery request per interval rather than one per
|
||||
// inbound request.
|
||||
func (e *issuerEntry) resolve(ctx context.Context, cache *jwk.Cache, retryInterval, refreshInterval time.Duration) (string, error) {
|
||||
e.mu.Lock()
|
||||
defer e.mu.Unlock()
|
||||
|
||||
if e.jwksURI != "" {
|
||||
return e.jwksURI, nil
|
||||
}
|
||||
if !e.lastAttempt.IsZero() && time.Since(e.lastAttempt) < retryInterval {
|
||||
// Back off, but report the real reason rather than a generic one — the
|
||||
// operator needs the issuer's actual error, not "throttled".
|
||||
return "", e.lastErr
|
||||
}
|
||||
e.lastAttempt = time.Now()
|
||||
|
||||
client := e.httpClient
|
||||
if client == nil {
|
||||
client = http.DefaultClient
|
||||
}
|
||||
jwksURI, err := discoverJWKSURI(ctx, client, e.issuer)
|
||||
if err != nil {
|
||||
e.lastErr = fmt.Errorf("%w: issuer %s: %v", ErrUnavailable, e.issuer, err)
|
||||
return "", e.lastErr
|
||||
}
|
||||
|
||||
regOpts := []jwk.RegisterOption{jwk.WithMinRefreshInterval(refreshInterval)}
|
||||
if e.httpClient != nil {
|
||||
regOpts = append(regOpts, jwk.WithHTTPClient(e.httpClient))
|
||||
}
|
||||
if err := cache.Register(jwksURI, regOpts...); err != nil {
|
||||
e.lastErr = fmt.Errorf("%w: register jwks cache (%s): %v", ErrUnavailable, e.issuer, err)
|
||||
return "", e.lastErr
|
||||
}
|
||||
if _, err := cache.Refresh(ctx, jwksURI); err != nil {
|
||||
// Registered but unfetchable. Unregister so the next attempt re-runs
|
||||
// discovery cleanly rather than inheriting a half-built entry.
|
||||
_ = cache.Unregister(jwksURI)
|
||||
e.lastErr = fmt.Errorf("%w: initial jwks fetch (%s): %v", ErrUnavailable, e.issuer, err)
|
||||
return "", e.lastErr
|
||||
}
|
||||
|
||||
e.jwksURI, e.lastErr = jwksURI, nil
|
||||
return jwksURI, nil
|
||||
}
|
||||
|
||||
// Option configures a validator. Variadic so every existing caller compiles
|
||||
// unchanged — a shared library cannot afford a breaking signature for this.
|
||||
type Option func(*options)
|
||||
|
||||
type options struct {
|
||||
retryInterval time.Duration
|
||||
refreshInterval time.Duration
|
||||
}
|
||||
|
||||
// defaultRetryInterval bounds how often a down issuer is re-probed. A readiness
|
||||
// probe typically runs every 10s, so recovery lands within one interval of the
|
||||
// issuer coming back — without turning an IdP outage into a discovery flood.
|
||||
const defaultRetryInterval = 30 * time.Second
|
||||
|
||||
// defaultRefreshInterval is the JWKS cache's minimum refresh interval. 1h
|
||||
// preserves the value that was hardcoded here before options existed; making it
|
||||
// configurable is mcp-chassis#1.
|
||||
const defaultRefreshInterval = time.Hour
|
||||
|
||||
// WithRetryInterval sets how long to wait before re-attempting OIDC discovery
|
||||
// for an issuer that is not yet resolved. Zero disables the wait, which is for
|
||||
// tests; production wants the default.
|
||||
func WithRetryInterval(d time.Duration) Option {
|
||||
return func(o *options) { o.retryInterval = d }
|
||||
}
|
||||
|
||||
// JWTValidator validates Bearer JWTs against one or more trusted OIDC issuers.
|
||||
@@ -63,56 +151,80 @@ type issuerEntry struct {
|
||||
// on whether any issuer is configured, and pass nil through the rest of the
|
||||
// codebase without further branching.
|
||||
type JWTValidator struct {
|
||||
entries []issuerEntry
|
||||
entries []*issuerEntry
|
||||
cache *jwk.Cache
|
||||
opts options
|
||||
}
|
||||
|
||||
// NewJWTValidator builds a single-issuer validator — the common case, and
|
||||
// backward compatible with every existing caller. Empty issuerURL returns
|
||||
// (nil, nil) so callers can use one constructor regardless of whether an issuer
|
||||
// is configured.
|
||||
func NewJWTValidator(ctx context.Context, issuerURL, audience string) (*JWTValidator, error) {
|
||||
func NewJWTValidator(ctx context.Context, issuerURL, audience string, opts ...Option) (*JWTValidator, error) {
|
||||
if issuerURL == "" {
|
||||
return nil, nil
|
||||
}
|
||||
return NewMultiJWTValidator(ctx, []IssuerConfig{{IssuerURL: issuerURL, Audience: audience}})
|
||||
return NewMultiJWTValidator(ctx, []IssuerConfig{{IssuerURL: issuerURL, Audience: audience}}, opts...)
|
||||
}
|
||||
|
||||
// NewMultiJWTValidator builds a validator that trusts every issuer in the list.
|
||||
// For each, it fetches the OIDC discovery document, registers jwks_uri in a
|
||||
// shared cache, and warms it. Entries with an empty IssuerURL are skipped; an
|
||||
// empty/nil resulting set returns (nil, nil) — "JWT auth disabled".
|
||||
func NewMultiJWTValidator(ctx context.Context, issuers []IssuerConfig) (*JWTValidator, error) {
|
||||
cache := jwk.NewCache(ctx)
|
||||
var entries []issuerEntry
|
||||
func NewMultiJWTValidator(ctx context.Context, issuers []IssuerConfig, opts ...Option) (*JWTValidator, error) {
|
||||
o := options{retryInterval: defaultRetryInterval, refreshInterval: defaultRefreshInterval}
|
||||
for _, fn := range opts {
|
||||
fn(&o)
|
||||
}
|
||||
|
||||
var entries []*issuerEntry
|
||||
for _, ic := range issuers {
|
||||
if ic.IssuerURL == "" {
|
||||
continue
|
||||
}
|
||||
client := ic.HTTPClient
|
||||
if client == nil {
|
||||
client = http.DefaultClient
|
||||
}
|
||||
jwksURI, err := discoverJWKSURI(ctx, client, ic.IssuerURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("issuer %s: %w", ic.IssuerURL, err)
|
||||
}
|
||||
regOpts := []jwk.RegisterOption{jwk.WithMinRefreshInterval(time.Hour)}
|
||||
if ic.HTTPClient != nil {
|
||||
regOpts = append(regOpts, jwk.WithHTTPClient(ic.HTTPClient))
|
||||
}
|
||||
if err := cache.Register(jwksURI, regOpts...); err != nil {
|
||||
return nil, fmt.Errorf("register jwks cache (%s): %w", ic.IssuerURL, err)
|
||||
}
|
||||
if _, err := cache.Refresh(ctx, jwksURI); err != nil {
|
||||
return nil, fmt.Errorf("initial jwks fetch (%s): %w", ic.IssuerURL, err)
|
||||
}
|
||||
entries = append(entries, issuerEntry{issuer: ic.IssuerURL, audience: ic.Audience, jwksURI: jwksURI})
|
||||
entries = append(entries, &issuerEntry{
|
||||
issuer: ic.IssuerURL,
|
||||
audience: ic.Audience,
|
||||
httpClient: ic.HTTPClient,
|
||||
})
|
||||
}
|
||||
if len(entries) == 0 {
|
||||
// No issuer configured: JWT auth is genuinely disabled, and nil is the
|
||||
// right answer. This is the ONE case where nil is not a lie, which is
|
||||
// why the boot-race fix must not collapse it — see
|
||||
// TestNewMultiJWTValidator_NoIssuer_StillReturnsNilNil.
|
||||
return nil, nil
|
||||
}
|
||||
return &JWTValidator{entries: entries, cache: cache}, nil
|
||||
|
||||
v := &JWTValidator{entries: entries, cache: jwk.NewCache(ctx), opts: o}
|
||||
|
||||
// Try once, eagerly, so a healthy start still warms the cache before the
|
||||
// first request — but do NOT fail on it. A validator that exists and is
|
||||
// temporarily unresolved is strictly better than no validator: the first
|
||||
// answers 503-and-retry, the second answers 401 forever.
|
||||
for _, e := range v.entries {
|
||||
_, _ = e.resolve(ctx, v.cache, 0, o.refreshInterval)
|
||||
}
|
||||
return v, nil
|
||||
}
|
||||
|
||||
// Ready reports whether every trusted issuer has been resolved, attempting
|
||||
// resolution for any that have not. It is safe and intended to be called from a
|
||||
// readiness probe: the probe's own cadence then drives retry, so recovery needs
|
||||
// no background goroutine and no process restart.
|
||||
//
|
||||
// A nil validator is ready — "JWT auth disabled" is a configuration, not a
|
||||
// fault.
|
||||
func (v *JWTValidator) Ready(ctx context.Context) (bool, error) {
|
||||
if v == nil {
|
||||
return true, nil
|
||||
}
|
||||
for _, e := range v.entries {
|
||||
if _, err := e.resolve(ctx, v.cache, v.opts.retryInterval, v.opts.refreshInterval); err != nil {
|
||||
return false, err
|
||||
}
|
||||
}
|
||||
return true, nil
|
||||
}
|
||||
|
||||
// discoverJWKSURI fetches the OIDC discovery document from issuerURL and returns
|
||||
@@ -157,7 +269,14 @@ func (v *JWTValidator) Validate(ctx context.Context, rawToken string) (string, e
|
||||
var lastErr error
|
||||
sawDecision := false // at least one issuer reached a real verify verdict
|
||||
for _, e := range v.entries {
|
||||
keySet, err := v.cache.Get(ctx, e.jwksURI)
|
||||
jwksURI, err := e.resolve(ctx, v.cache, v.opts.retryInterval, v.opts.refreshInterval)
|
||||
if err != nil {
|
||||
// Not resolved yet — transient by construction (resolve wraps
|
||||
// ErrUnavailable). Try the next issuer.
|
||||
lastErr = err
|
||||
continue
|
||||
}
|
||||
keySet, err := v.cache.Get(ctx, jwksURI)
|
||||
if err != nil {
|
||||
// This issuer's JWKS is unreachable — transient. Try the next; only
|
||||
// if ALL issuers are unreachable do we surface ErrUnavailable.
|
||||
|
||||
Reference in New Issue
Block a user