CI / Lint / Test / Vet (push) Successful in 5s
Closes #1. Closes gitea-mcp#58. Closes gitea-mcp#60. Three small changes to one decision point, kept together because they are the same surface and the same incident. 1. A JWT IS NEVER A STATIC-TOKEN MISMATCH (gitea-mcp#58) The fall-through reason was chosen by which comparison ran last, not by what the caller presented. So a JWT against a server with no JWT auth configured logged: reason=static_token_mismatch token_type=jwt status=401 The two fields contradict each other, and the sentence reads as "you presented the wrong token" when the truth is "this server could not have accepted any token of that kind". The misdirection reached the user — claude.ai surfaces "Authorization with gitea failed. You can check your credentials and permissions." — and a debugging session went into re-running the OAuth flow while the credentials were valid throughout. Now `jwt_auth_not_configured` for a JWT; `static_token_mismatch` stays, and is asserted to stay, for an actual static-token mismatch. Fixing the misleading case must not delete the accurate one. Note this is now the ONLY way to reach that branch with a JWT: since v0.6.0 a validator that merely could not reach its issuer is non-nil and answers ErrUnavailable -> 503. A nil validator means the server was never given an issuer. 2. SUCCESS IS LOGGED, WITH THE PATH THAT GRANTED IT (gitea-mcp#60) The middleware logged only on rejection. After re-authing a connector there was no way to tell whether a request used the static token or a JWT, only inference from which failures were absent. A log that proves a request failed, and nothing about how the successful ones succeeded, cannot answer the question anyone actually has during an auth incident. INFO mcp auth accepted auth_path=jwt caller=alice token_fp=... remote=... INFO mcp auth accepted auth_path=static token_fp=... remote=... Same disclosure rules as deny: a fingerprint, never the credential. The JWT subject is included because distinguishing WHO is the point of that path; `caller` is omitted rather than empty for the static path, which has no identity to report. 3. JWKS REFRESH INTERVAL IS CONFIGURABLE (#1) It was hardcoded at 1h, so an issuer rotating its signing key could leave every consumer rejecting valid JWTs for up to an hour. WithRefreshInterval, defaulting to 1h so nothing changes for existing callers. Variadic, per #1's own preferred option — a new positional parameter would force a coordinated bump across every consumer. Deliberately NOT read from the environment inside the library: a shared lib calling os.Getenv is a hidden dependency that surprises its consumers. Config ownership stays at the edge. The effective value is logged when an issuer resolves, which is #1's second acceptance criterion and the thing an operator needs during a key-rotation incident. The test asserts the configured value reaches the registration site via that line — a silently-ignored option being the failure mode worth pinning. Whether jwx then honours it is jwx's to test. One of my own assertions was wrong and the test caught it: tokenType() says "opaque", not "static", for a non-JWT credential. Corrected to the existing vocabulary rather than changing the vocabulary to match a new test. Verified: golangci-lint 0 issues; go vet clean; go test -race -count=1 ok. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
156 lines
6.2 KiB
Go
156 lines
6.2 KiB
Go
package auth
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"testing"
|
|
"time"
|
|
|
|
"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)
|
|
}
|
|
|
|
// mcp-chassis#1: the JWKS min-refresh interval was hardcoded at 1h, so a signing
|
|
// key rotation could leave every consumer rejecting valid JWTs for up to an hour.
|
|
//
|
|
// The interval reaching jwx's cache is not observable from outside jwx, and
|
|
// testing jwx's own refresh behaviour is jwx's job. What IS this package's job,
|
|
// and what this asserts, is that the configured value reaches the registration
|
|
// site and is stated in the log — which is also #1's second acceptance criterion
|
|
// ("effective value logged at init"). A silently-ignored option is the failure
|
|
// mode worth pinning.
|
|
func TestWithRefreshInterval_ReachesRegistrationAndIsLogged(t *testing.T) {
|
|
for _, tc := range []struct {
|
|
name string
|
|
opts []Option
|
|
want string
|
|
}{
|
|
{name: "default is 1h, unchanged", want: "1h0m0s"},
|
|
{name: "override", opts: []Option{WithRefreshInterval(5 * time.Minute)}, want: "5m0s"},
|
|
} {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
iss := newTestIssuer(t)
|
|
logs := captureLogs(t)
|
|
|
|
v, err := NewMultiJWTValidator(t.Context(),
|
|
[]IssuerConfig{{IssuerURL: iss.url, Audience: "aud"}}, tc.opts...)
|
|
require.NoError(t, err)
|
|
require.NotNil(t, v)
|
|
|
|
var found bool
|
|
for _, l := range logs() {
|
|
if l["msg"] == "jwt issuer resolved" {
|
|
require.Equal(t, tc.want, l["jwks_refresh_interval"])
|
|
found = true
|
|
}
|
|
}
|
|
require.True(t, found, "resolving an issuer must state the effective refresh interval")
|
|
})
|
|
}
|
|
}
|