diff --git a/auth/bearer_test.go b/auth/bearer_test.go index cd1dca2..e317311 100644 --- a/auth/bearer_test.go +++ b/auth/bearer_test.go @@ -18,9 +18,9 @@ import ( func unavailableValidator(t *testing.T) *JWTValidator { t.Helper() return &JWTValidator{ - entries: []issuerEntry{{ + entries: []*issuerEntry{{ issuer: "https://dex.example", - jwksURI: "http://127.0.0.1:0/jwks", // unregistered in cache → Get fails + jwksURI: "http://127.0.0.1:0/jwks", // already "resolved", unregistered in cache → Get fails }}, cache: jwk.NewCache(context.Background()), } diff --git a/auth/jwt.go b/auth/jwt.go index 5e0255b..f555a68 100644 --- a/auth/jwt.go +++ b/auth/jwt.go @@ -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. diff --git a/auth/jwt_bootrace_test.go b/auth/jwt_bootrace_test.go new file mode 100644 index 0000000..754c913 --- /dev/null +++ b/auth/jwt_bootrace_test.go @@ -0,0 +1,115 @@ +package auth + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/require" +) + +// The boot race, pinned. +// +// WHAT HAPPENED, THREE TIMES +// gitea-mcp starts before Authentik is serving. OIDC discovery returns 502/503. +// The constructor errors, the caller is left holding nil, and `nil` means "this +// server does not do JWT auth" — so every subsequent Authentik JWT is +// string-compared against the static token and rejected: +// +// WARN mcp auth rejected reason=static_token_mismatch token_type=jwt status=401 +// +// Permanently, because discovery was attempted exactly once. The pod stays 1/1 +// Running and readiness-green throughout. +// +// 2026-07-17 gitea-mcp#57 ~1 debugging session +// 2026-09-07 mcp-chassis#4 ~16 hours +// 2026-09-14 (this) ~6 days, found by chance +// +// Each recovery was a human running `kubectl rollout restart`. +// +// WHY THE FIX IS SMALL +// Everything after construction is already correct. `Validate` distinguishes +// ErrUnavailable (issuer unreachable → 503, retry) from a definitive rejection +// (→ 401), and BearerMiddleware already honours that. The correct behaviour was +// unreachable for this case only because the validator never got built. +// +// So: build it anyway, resolve discovery lazily, and let the existing machinery +// do what it already does. + +// A validator must be constructible while its issuer is down, must report +// ErrUnavailable rather than a rejection while it stays down, and must start +// working when the issuer returns — without a restart. +func TestNewMultiJWTValidator_SurvivesIssuerDownAtBoot(t *testing.T) { + ctx := context.Background() + iss := newTestIssuer(t) + // Three, because three resolution attempts happen before recovery is + // asserted: the constructor's eager warm-up, the Validate below, and the + // Ready below. Each must see the issuer still down. + iss.discoveryFailures = 3 + + // 1. Construction succeeds despite discovery being down. This is the whole + // bug: today this returns (nil, err) and the caller degrades silently. + v, err := NewMultiJWTValidator(ctx, + []IssuerConfig{{IssuerURL: iss.url, Audience: "test-aud"}}, + WithRetryInterval(0), // no suppression, so the test drives the retries + ) + require.NoError(t, err, "a validator whose issuer is down at boot must still be constructed") + require.NotNil(t, v, "nil would be indistinguishable from 'JWT auth disabled'") + + // 2. While down, a well-formed token is UNAVAILABLE, never a rejection. + // BearerMiddleware turns this into 503 + retry; a rejection would become + // 401 static_token_mismatch, which is the lie that cost three sessions. + tok := iss.mint(t, "test-aud", "alice") + _, err = v.Validate(ctx, tok) + require.Error(t, err) + require.ErrorIs(t, err, ErrUnavailable, + "issuer down must be transient, not a verdict on the token") + + // 3. The server knows it is degraded, so it can stop calling itself healthy. + ready, why := v.Ready(ctx) + require.False(t, ready) + require.Error(t, why, "Ready must say WHY, so /healthz can report it") + + // 4. The issuer recovers. The SAME validator starts working — no restart, + // no rollout, no human. + require.GreaterOrEqual(t, iss.discoveryAttempts(), 1, "discovery must be retried, not cached as failed") + sub, err := v.Validate(ctx, tok) + require.NoError(t, err, "a recovered issuer must work without reconstructing the validator") + require.Equal(t, "alice", sub) + + ready, why = v.Ready(ctx) + require.True(t, ready) + require.NoError(t, why) +} + +// Regression guard. A server that genuinely wants static-token-only auth passes +// no issuer, and must keep getting nil — that is what makes BearerMiddleware +// report static_token_mismatch correctly for it. Fixing the boot race must not +// turn every static-only server into a JWT server. +func TestNewMultiJWTValidator_NoIssuer_StillReturnsNilNil(t *testing.T) { + v, err := NewMultiJWTValidator(context.Background(), []IssuerConfig{{IssuerURL: ""}}) + require.NoError(t, err) + require.Nil(t, v, "no issuer configured still means JWT auth disabled") +} + +// A permanently bad issuer must not look like a transient one forever in a way +// that hides a config error — but it must still be ErrUnavailable rather than a +// token verdict, because the token was never the problem. The distinction that +// matters is the one Ready() reports. +func TestJWTValidator_UnresolvableIssuer_IsUnavailableAndNotReady(t *testing.T) { + ctx := context.Background() + v, err := NewMultiJWTValidator(ctx, + []IssuerConfig{{IssuerURL: "http://127.0.0.1:1/nonexistent"}}, + WithRetryInterval(0), + ) + require.NoError(t, err) + require.NotNil(t, v) + + _, err = v.Validate(ctx, "not.a.real.token") + require.True(t, errors.Is(err, ErrUnavailable), + "an unreachable issuer is unavailable, whatever the token looks like") + + ready, why := v.Ready(ctx) + require.False(t, ready) + require.Error(t, why) +} diff --git a/auth/jwt_test.go b/auth/jwt_test.go index 5603f40..d0bfb11 100644 --- a/auth/jwt_test.go +++ b/auth/jwt_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -24,12 +25,37 @@ type testIssuer struct { url string priv jwk.Key requireBearer string // if set, discovery+JWKS return 401 without this bearer + + // mu guards the boot-race fields below, which a test mutates while the + // httptest server is serving. + mu sync.Mutex + // discoveryFailures is how many initial discovery requests answer 503 — + // the IdP-not-yet-serving case that disabled OAuth for six days + // (mcp-chassis#4, gitea-mcp#57). + discoveryFailures int + discoveryHits int } func (ti *testIssuer) authed(r *http.Request) bool { return ti.requireBearer == "" || r.Header.Get("Authorization") == "Bearer "+ti.requireBearer } +// discoveryDown reports whether this discovery request should answer 503, and +// counts the attempt. Attempts are counted even while down, so a test can assert +// that resolution was RETRIED rather than cached as a permanent failure. +func (ti *testIssuer) discoveryDown() bool { + ti.mu.Lock() + defer ti.mu.Unlock() + ti.discoveryHits++ + return ti.discoveryHits <= ti.discoveryFailures +} + +func (ti *testIssuer) discoveryAttempts() int { + ti.mu.Lock() + defer ti.mu.Unlock() + return ti.discoveryHits +} + // bearerRT adds a static Authorization header to every request — a minimal stand-in // for the in-cluster case where the JWKS fetch carries the pod's SA token. type bearerRT struct{ token string } @@ -62,6 +88,10 @@ func newTestIssuer(t *testing.T) *testIssuer { _ = json.NewEncoder(w).Encode(set) }) mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + if ti.discoveryDown() { + w.WriteHeader(http.StatusServiceUnavailable) + return + } if !ti.authed(r) { w.WriteHeader(http.StatusUnauthorized) return @@ -172,9 +202,29 @@ func TestNewMultiJWTValidator_UsesPerIssuerHTTPClientForAuthedFetch(t *testing.T iss := newTestIssuer(t) iss.requireBearer = "fetch-secret" // discovery + JWKS now demand this bearer - // nil client (http.DefaultClient, no credential) → discovery 401 → build fails. - _, err := NewMultiJWTValidator(ctx, []IssuerConfig{{IssuerURL: iss.url, Audience: "sa-poc"}}) - require.Error(t, err, "unauthenticated fetch must fail against an auth-gated issuer") + // nil client (http.DefaultClient, no credential) → discovery 401 → the issuer + // never resolves. + // + // This used to assert that the CONSTRUCTOR errored. It no longer does: since + // mcp-chassis#4, construction cannot fail on an unreachable issuer, because + // "could not resolve" and "JWT auth not wanted" must stop collapsing into the + // same nil. The property that actually matters is unchanged and asserted more + // directly here — without the credential, a correctly-minted token from this + // very issuer still gets nowhere, and the validator says it is not ready. + unauth, err := NewMultiJWTValidator(ctx, + []IssuerConfig{{IssuerURL: iss.url, Audience: "sa-poc"}}, + WithRetryInterval(0), + ) + require.NoError(t, err) + require.NotNil(t, unauth) + + ready, why := unauth.Ready(ctx) + require.False(t, ready, "an issuer whose discovery 401s is not ready") + require.Error(t, why) + + _, err = unauth.Validate(ctx, iss.mint(t, "sa-poc", "poc-pod")) + require.Error(t, err, "unauthenticated fetch must authenticate nobody, even with a genuine token") + require.ErrorIs(t, err, ErrUnavailable) // A client that presents the fetch credential → build succeeds, token validates. client := &http.Client{Transport: bearerRT{token: "fetch-secret"}}