From db0c0a67d6c6d86174b7e5c2ade7b996dca41f7b Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 11:51:00 +0530 Subject: [PATCH 01/11] feat(mcp): add resource-server foundation for HTTP MCP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authorizer must act as an OAuth 2.1 resource server before /mcp can be served over HTTP. Two rules make that safe, and they are mirror images: - ValidateAccessToken rejects every resource-bound (absolute-URI) audience at Authorizer's own surfaces. - ValidateMCPAccessToken accepts exactly one — /mcp — and nothing else. Neither has an "or" in it. An MCP token must not double as a GraphQL credential, and a login token must not reach the tool surface. Rather than branch on audience inside the shared resolver, the MCP transport gets its own bufconn-only gRPC server via a TokenResolver override. The boundary is then structural: two servers, neither able to accept the other's tokens. The audience comparison never touches request headers. parsers.GetHost falls back to X-Authorizer-URL when --url is unset, which would let a caller name the audience their own token must match. Config.MCPResource derives it from --url alone, and startup refuses --mcp-enabled without it. MCP is stricter than the first-party path on subject liveness too: userIsRevoked resolves a subject as a user only and returns "not revoked" when it finds nothing, so a deactivated service account's machine token keeps working until expiry. MCP uses subjectIsLive (user-then-client, fails closed) since agents and service accounts are its main callers. The shared core keeps the old rule — widening it is a separate change. Delegated (RFC 8693) tokens are deliberately not accepted at /mcp yet: they are stateless, so they fail the session lookup, and the delegated validator requires the bare host as audience. Widening that path gives up the byte-for-byte token comparison and must be its own decision. Refs authorizer-docs specs/2026-08-10-mcp-http-transport.md --- cmd/root.go | 25 ++ internal/config/config.go | 9 + internal/config/mcp.go | 65 +++++ internal/config/mcp_test.go | 62 +++++ internal/grpcsrv/interceptors/auth.go | 56 ++++- internal/grpcsrv/interceptors/auth_test.go | 100 ++++++-- internal/grpcsrv/interceptors/mcp_auth.go | 67 +++++ .../grpcsrv/interceptors/mcp_auth_test.go | 197 +++++++++++++++ internal/grpcsrv/server.go | 15 +- internal/http_handlers/openid_config.go | 27 +- internal/http_handlers/protected_resource.go | 80 ++++++ internal/http_handlers/provider.go | 3 + .../mcp_access_token_test.go | 231 ++++++++++++++++++ internal/integration_tests/mcp_prm_test.go | 127 ++++++++++ internal/mcp/exposed_methods_test.go | 108 ++++++++ internal/server/http_routes.go | 16 ++ internal/token/auth_token.go | 101 ++++++-- internal/token/delegated_access_token.go | 24 +- internal/token/mcp_access_token.go | 79 ++++++ internal/token/provider.go | 6 + 20 files changed, 1335 insertions(+), 63 deletions(-) create mode 100644 internal/config/mcp.go create mode 100644 internal/config/mcp_test.go create mode 100644 internal/grpcsrv/interceptors/mcp_auth.go create mode 100644 internal/grpcsrv/interceptors/mcp_auth_test.go create mode 100644 internal/http_handlers/protected_resource.go create mode 100644 internal/integration_tests/mcp_access_token_test.go create mode 100644 internal/integration_tests/mcp_prm_test.go create mode 100644 internal/mcp/exposed_methods_test.go create mode 100644 internal/token/mcp_access_token.go diff --git a/cmd/root.go b/cmd/root.go index 1e5e8a5c8..1a3c4d7ad 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -135,6 +135,17 @@ func init() { f.StringVar(&rootArgs.config.GRPCTLSKey, "grpc-tls-key", "", "Path to the TLS private key for the gRPC server") f.BoolVar(&rootArgs.config.GRPCInsecure, "grpc-insecure", false, "Allow the gRPC server to run without TLS (dev only)") + // MCP transport. Served at POST /mcp on the main HTTP listener (not its own + // port): it is plain HTTP that must be publicly reachable on the same origin + // as the OAuth metadata clients discover it through, and mounting it on the + // main router gives it the existing CORS, security-header, rate-limit and + // logging middleware. + f.BoolVar(&rootArgs.config.MCPEnabled, "mcp-enabled", false, + "Serve the MCP tool surface over HTTP at POST /mcp as an OAuth 2.1 resource server. "+ + "Requires --url: tokens are accepted only when their audience equals /mcp, and that "+ + "comparison must not depend on a request header. Off by default — it is a new "+ + "internet-facing authenticated surface") + // Organization flags f.StringVar(&rootArgs.config.OrganizationLogo, "organization-logo", defaultOrganizationLogo, "Logo of the organization") f.StringVar(&rootArgs.config.OrganizationName, "organization-name", defaultOrganizationName, "Name of the organization") @@ -400,6 +411,20 @@ func runRoot(c *cobra.Command, args []string) { } } + // MCP's entire security model is the audience check: a token is accepted at + // /mcp only if its `aud` equals this deployment's canonical /mcp. Without + // --url that URL would be derived from request headers (parsers.GetHost falls + // back to X-Authorizer-URL / X-Forwarded-Host), which means the caller would + // get to state the audience their own token must match — no check at all. + // Refuse the combination rather than serve an endpoint that looks + // authenticated and is not. + if rootArgs.config.MCPEnabled && rootArgs.config.MCPResource() == "" { + fmt.Fprintln(os.Stderr, "--mcp-enabled requires a valid --url (e.g. https://auth.example.com): "+ + "the MCP resource identifier that access tokens are bound to is derived from it, and "+ + "deriving it from request headers instead would let a caller choose their own audience") + os.Exit(1) + } + // Refuse to start without an admin secret. The previous default of // "password" was a publicly known credential — operators upgrading from // older versions must now supply --admin-secret explicitly. The strength diff --git a/internal/config/config.go b/internal/config/config.go index 090e956c1..18b085140 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -66,6 +66,15 @@ type Config struct { // When unset and GRPCInsecure is false, the server refuses to start. GRPCTLSCert string GRPCTLSKey string + + // MCPEnabled serves Authorizer's MCP tool surface over HTTP at POST /mcp on + // the main listener, as an OAuth 2.1 resource server. Off by default: it is a + // new internet-facing authenticated surface, and an auth server should not + // grow one silently. + // + // Requires AuthorizerURL. Startup refuses the combination without it — + // see MCPResource for why. + MCPEnabled bool // GRPCInsecure permits cleartext gRPC. For local dev only; production // should always set TLS material. GRPCInsecure bool diff --git a/internal/config/mcp.go b/internal/config/mcp.go new file mode 100644 index 000000000..9aaa1fe2a --- /dev/null +++ b/internal/config/mcp.go @@ -0,0 +1,65 @@ +package config + +import ( + "net/url" + "strings" +) + +// MCPResourcePath is the single path the MCP transport is served on. Fixed, not +// configurable: it is baked into the canonical resource URI that clients name in +// their RFC 8707 `resource` parameter and that tokens carry as `aud`, so making +// it an operator knob would only create ways for the three places that must +// agree to disagree. +const MCPResourcePath = "/mcp" + +// CanonicalURL returns the operator-configured --url reduced to scheme+host, +// with path, query, fragment, userinfo and any trailing slash stripped. Empty +// when --url is unset or is not a usable http(s) origin. +// +// Normalization matches parsers.SetTrustedURL exactly, and that is the whole +// point: parsers.GetHost returns the sanitized value, and it is the sanitized +// value that gets stamped as every token's `iss` claim and used to build every +// self-referential URL the discovery documents publish. Anything derived from +// the RAW --url instead lands one normalization behind and disagrees with the +// tokens the same server mints — an operator running `--url +// https://auth.example.com/auth` would publish an authorization server and a +// jwks_uri that both 404, and an issuer that no token matches, while startup +// reported everything fine. +func (c *Config) CanonicalURL() string { + if c == nil { + return "" + } + u, err := url.Parse(strings.TrimSpace(c.AuthorizerURL)) + if err != nil || u.User != nil || u.Host == "" { + return "" + } + if u.Scheme != "http" && u.Scheme != "https" { + return "" + } + return strings.TrimSuffix(u.Scheme+"://"+u.Host, "/") +} + +// MCPResource returns the canonical resource identifier of this deployment's MCP +// server: "/mcp". Empty when --url is unset. +// +// This is the ONE place the canonical form is computed. Three things must agree +// on it or the surface is either unreachable or unsafe: +// +// - the RFC 9728 protected resource metadata `resource` field, which is what +// clients read and then send as the RFC 8707 `resource` parameter; +// - the `aud` comparison in token.ValidateMCPAccessToken; +// - the documentation operators copy into their client config. +// +// It is derived from the operator-configured --url and never from a request. +// parsers.GetHost falls back to request headers when --url is unset, and an +// audience check against a header the caller controls authenticates anyone: an +// attacker would simply send X-Authorizer-URL naming whatever audience their +// token already has. That is why this returns empty rather than guessing, and +// why startup refuses --mcp-enabled without --url. +func (c *Config) MCPResource() string { + base := c.CanonicalURL() + if base == "" { + return "" + } + return base + MCPResourcePath +} diff --git a/internal/config/mcp_test.go b/internal/config/mcp_test.go new file mode 100644 index 000000000..440757d96 --- /dev/null +++ b/internal/config/mcp_test.go @@ -0,0 +1,62 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestMCPResource pins the canonical MCP resource identifier. +// +// Three independent things compare against this string — the RFC 9728 metadata +// clients read, the `aud` check in token.ValidateMCPAccessToken, and whatever an +// operator pastes into their client config. A trailing slash or a stray path +// surviving normalization here does not fail loudly; it makes every token's +// audience miss by one character and the surface returns 401 forever. +// +// The empty cases matter just as much: MCPResource returning "" is what makes +// startup refuse --mcp-enabled, so anything that cannot be trusted as a +// canonical origin must return "" rather than a best guess. +func TestMCPResource(t *testing.T) { + cases := []struct { + name string + url string + want string + }{ + {"plain https origin", "https://auth.example.com", "https://auth.example.com/mcp"}, + {"trailing slash is stripped", "https://auth.example.com/", "https://auth.example.com/mcp"}, + {"path is stripped", "https://auth.example.com/some/base", "https://auth.example.com/mcp"}, + {"query and fragment are stripped", "https://auth.example.com/?a=b#c", "https://auth.example.com/mcp"}, + {"explicit port is part of the origin", "https://auth.example.com:8443", "https://auth.example.com:8443/mcp"}, + {"http is allowed for local development", "http://localhost:8080", "http://localhost:8080/mcp"}, + {"surrounding whitespace is tolerated", " https://auth.example.com ", "https://auth.example.com/mcp"}, + + {"unset url yields no resource", "", ""}, + {"missing scheme yields no resource", "auth.example.com", ""}, + {"non-http scheme yields no resource", "ftp://auth.example.com", ""}, + {"userinfo yields no resource", "https://user:pass@auth.example.com", ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := &Config{AuthorizerURL: tc.url} + assert.Equal(t, tc.want, c.MCPResource()) + + // Every self-referential URL the discovery documents publish is + // built from CanonicalURL, so it must be exactly MCPResource minus + // the path. Letting them drift is what publishes an authorization + // server and a jwks_uri that 404 while `resource` looks right. + if tc.want == "" { + assert.Equal(t, "", c.CanonicalURL()) + } else { + assert.Equal(t, tc.want, c.CanonicalURL()+MCPResourcePath) + } + }) + } + + t.Run("a nil config yields no resource", func(t *testing.T) { + var c *Config + assert.Equal(t, "", c.MCPResource()) + assert.Equal(t, "", c.CanonicalURL()) + }) +} diff --git a/internal/grpcsrv/interceptors/auth.go b/internal/grpcsrv/interceptors/auth.go index e733a7aa7..16fa8eae6 100644 --- a/internal/grpcsrv/interceptors/auth.go +++ b/internal/grpcsrv/interceptors/auth.go @@ -44,9 +44,46 @@ var infrastructureServices = map[string]struct{}{ var methodDescCache sync.Map // map[string]protoreflect.MethodDescriptor +// TokenResolver turns a request into the caller's identity, or an error when the +// request carries no credential this surface accepts. It is the single point at +// which a gRPC server decides WHICH tokens authenticate it. +// +// The default (GetUserIDFromSessionOrAccessToken) accepts a browser session +// cookie or a first-party bearer token, and rejects every resource-bound +// audience. The MCP surface overrides it with MCPTokenResolver, which does the +// exact opposite on the audience and drops the cookie path entirely. Because the +// override is per-server rather than per-request, no token can cross between the +// two surfaces — see MCPTokenResolver. +type TokenResolver func(gc *gin.Context) (*token.SessionOrAccessTokenData, error) + // Auth returns a unary interceptor that enforces proto-declared auth policy. // log may be nil (rejections are then only counted, not logged). -func Auth(tp token.Provider, log *zerolog.Logger) grpc.UnaryServerInterceptor { +// +// resolve may be nil, in which case the caller's identity is resolved with +// tp.GetUserIDFromSessionOrAccessToken and the cookie-based paths below stay +// active — the behaviour every TCP-listening server uses. +// +// A non-nil resolve is the SOLE authority for that server. It replaces both +// identity-resolution sites (the admin fallback and the public path) AND +// disables the two paths that authenticate without consulting a resolver at all: +// the super-admin check (an admin cookie or the x-authorizer-admin-secret +// header) and the Session RPC's cookie-only branch. +// +// Disabling those is the point, not a side effect. A surface that declares its +// own token rule — MCP, whose rule is "the audience must name this MCP server" — +// must not be reachable with a credential that rule never saw. Leaving them +// active meant the boundary held only because no cookie-authenticated method +// happened to be mcp_tool-exposed, and transport.MetaFromGRPC reconstructs +// cookies from gRPC metadata, so a bridge that forwarded headers wholesale would +// have made a browser session authenticate a tool call on an internet-facing, +// CSRF-exempt endpoint. +func Auth(tp token.Provider, log *zerolog.Logger, resolve TokenResolver) grpc.UnaryServerInterceptor { + resolverIsSoleAuthority := resolve != nil + if resolve == nil { + resolve = func(gc *gin.Context) (*token.SessionOrAccessTokenData, error) { + return tp.GetUserIDFromSessionOrAccessToken(gc) + } + } return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { methodDesc, ok := methodDescriptor(info.FullMethod) if !ok { @@ -81,8 +118,10 @@ func Auth(tp token.Provider, log *zerolog.Logger) grpc.UnaryServerInterceptor { if serviceName == adminServiceName { // Platform super-admin: unchanged, and still the only identity that - // reaches the platform-wide operations. - if tp.IsSuperAdmin(gc) { + // reaches the platform-wide operations — except on a server whose + // resolver is the sole authority, where an admin cookie or admin + // secret is not a credential this surface accepts at all. + if !resolverIsSoleAuthority && tp.IsSuperAdmin(gc) { ctx = authctx.WithPrincipal(ctx, &authctx.Principal{IsSuperAdmin: true}) return handler(ctx, req) } @@ -103,7 +142,7 @@ func Auth(tp token.Provider, log *zerolog.Logger) grpc.UnaryServerInterceptor { // never inside a branch. A new admin method that forgets its gate would // now be reachable by any authenticated user, which is what // TestAdminMethodsAreGated exists to prevent. - tokenData, err := tp.GetUserIDFromSessionOrAccessToken(gc) + tokenData, err := resolve(gc) if err != nil || tokenData == nil || tokenData.UserID == "" { // No usable credential at all — reject before reaching a handler. if isPublicMethod(methodDesc) { @@ -139,8 +178,11 @@ func Auth(tp token.Provider, log *zerolog.Logger) grpc.UnaryServerInterceptor { // Session rotates the browser session cookie only; bearer tokens are ignored. // Guard on publicServiceName to prevent a future method named "Session" on - // another service from inheriting cookie-only auth. - if serviceName == publicServiceName && string(methodDesc.Name()) == sessionMethodName { + // another service from inheriting cookie-only auth. Skipped entirely when a + // resolver is the sole authority: a cookie is not a credential such a + // surface accepts, so Session falls through to the resolver and is rejected + // like any other unauthenticated call. + if !resolverIsSoleAuthority && serviceName == publicServiceName && string(methodDesc.Name()) == sessionMethodName { sessionToken, err := cookie.GetSession(gc) if err != nil || sessionToken == "" { return nil, status.Error(codes.Unauthenticated, "unauthorized") @@ -157,7 +199,7 @@ func Auth(tp token.Provider, log *zerolog.Logger) grpc.UnaryServerInterceptor { return handler(ctx, req) } - tokenData, err := tp.GetUserIDFromSessionOrAccessToken(gc) + tokenData, err := resolve(gc) if err != nil || tokenData == nil || tokenData.UserID == "" { return nil, status.Error(codes.Unauthenticated, "unauthorized") } diff --git a/internal/grpcsrv/interceptors/auth_test.go b/internal/grpcsrv/interceptors/auth_test.go index 16676cdad..a694f0a99 100644 --- a/internal/grpcsrv/interceptors/auth_test.go +++ b/internal/grpcsrv/interceptors/auth_test.go @@ -60,7 +60,7 @@ func (s *stubTokenProvider) ValidateBrowserSession(_ *gin.Context, encryptedSess func TestAuth_PublicMethodPassesThrough(t *testing.T) { stub := &stubTokenProvider{} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.MetaRequest{}, info(authorizerv1.AuthorizerService_Meta_FullMethodName), func(ctx context.Context, _ any) (any, error) { @@ -79,7 +79,7 @@ func TestAuth_PublicMethodPassesThrough(t *testing.T) { func TestAuth_AdminMethodRequiresSuperAdmin(t *testing.T) { t.Run("rejects missing admin auth", func(t *testing.T) { stub := &stubTokenProvider{superAdmin: false} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.AdminMetaRequest{}, info(authorizerv1.AuthorizerAdminService_AdminMeta_FullMethodName), func(_ context.Context, _ any) (any, error) { called = true @@ -98,7 +98,7 @@ func TestAuth_AdminMethodRequiresSuperAdmin(t *testing.T) { t.Run("attaches admin principal when authorized", func(t *testing.T) { stub := &stubTokenProvider{superAdmin: true} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.AdminMetaRequest{}, info(authorizerv1.AuthorizerAdminService_AdminMeta_FullMethodName), func(ctx context.Context, _ any) (any, error) { called = true @@ -138,7 +138,7 @@ func TestAuth_AdminMethodAllowsAuthenticatedNonSuperAdmin(t *testing.T) { Nonce: "nonce-1", }, } - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.OrgMembersRequest{}, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), func(ctx context.Context, _ any) (any, error) { called = true @@ -169,7 +169,7 @@ func TestAuth_AdminMethodRejectsBadCredential(t *testing.T) { "blank principal": {tokenData: &token.SessionOrAccessTokenData{LoginMethod: "basic_auth"}}, } { t.Run(name, func(t *testing.T) { - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.OrgMembersRequest{}, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), func(_ context.Context, _ any) (any, error) { called = true @@ -185,7 +185,7 @@ func TestAuth_AdminMethodRejectsBadCredential(t *testing.T) { func TestAuth_PrivatePublicServiceMethodRequiresUser(t *testing.T) { t.Run("rejects unauthenticated user", func(t *testing.T) { stub := &stubTokenProvider{tokenErr: status.Error(codes.Unauthenticated, "bad token")} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.ProfileRequest{}, info(authorizerv1.AuthorizerService_Profile_FullMethodName), func(_ context.Context, _ any) (any, error) { called = true @@ -206,7 +206,7 @@ func TestAuth_PrivatePublicServiceMethodRequiresUser(t *testing.T) { Nonce: "nonce-1", }, } - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.ProfileRequest{}, info(authorizerv1.AuthorizerService_Profile_FullMethodName), func(ctx context.Context, _ any) (any, error) { called = true @@ -228,7 +228,7 @@ func TestAuth_PrivatePublicServiceMethodRequiresUser(t *testing.T) { func TestAuth_InfrastructureServiceSkipsAuth(t *testing.T) { stub := &stubTokenProvider{} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), nil, &grpc.UnaryServerInfo{FullMethod: "/grpc.health.v1.Health/Check"}, func(_ context.Context, _ any) (any, error) { called = true @@ -244,7 +244,7 @@ func TestAuth_SessionRequiresCookieRejectsBearer(t *testing.T) { stub := &stubTokenProvider{ tokenData: &token.SessionOrAccessTokenData{UserID: "user-1"}, } - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( "authorization", "Bearer access-token", )) @@ -268,7 +268,7 @@ func TestAuth_SessionAcceptsCookie(t *testing.T) { Nonce: "nonce-1", }, } - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) cookieName := constants.AppCookieName + "_session" ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( "cookie", cookieName+"=sess-token", @@ -289,7 +289,7 @@ func TestAuth_SessionAcceptsCookie(t *testing.T) { func TestAuth_LogoutRequiresAuth(t *testing.T) { stub := &stubTokenProvider{tokenErr: status.Error(codes.Unauthenticated, "bad token")} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.LogoutRequest{}, info(authorizerv1.AuthorizerService_Logout_FullMethodName), func(_ context.Context, _ any) (any, error) { called = true @@ -305,7 +305,7 @@ func TestAuth_LogoutRequiresAuth(t *testing.T) { // it. Regression guard that scoping the `public` bypass did not lock admins out. func TestAuth_AdminLoginRemainsPublic(t *testing.T) { stub := &stubTokenProvider{superAdmin: false} - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.AdminLoginRequest{}, info(authorizerv1.AuthorizerAdminService_AdminLogin_FullMethodName), func(ctx context.Context, _ any) (any, error) { called = true @@ -352,7 +352,7 @@ func TestShouldRejectUnlistedService(t *testing.T) { // TestAuth_NilTokenProviderFailsClosed asserts the interceptor fails closed when // no TokenProvider is wired (e.g. during early startup). func TestAuth_NilTokenProviderFailsClosed(t *testing.T) { - mw := Auth(nil, nil) + mw := Auth(nil, nil, nil) called := false _, err := mw(context.Background(), &authorizerv1.ProfileRequest{}, info(authorizerv1.AuthorizerService_Profile_FullMethodName), func(_ context.Context, _ any) (any, error) { called = true @@ -371,7 +371,7 @@ func TestAuth_SessionOnlyAcceptsPublicService(t *testing.T) { stub := &stubTokenProvider{ sessionData: &token.SessionData{Subject: "user-1", LoginMethod: "basic_auth", Nonce: "n"}, } - mw := Auth(stub, nil) + mw := Auth(stub, nil, nil) cookieName := constants.AppCookieName + "_session" ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs("cookie", cookieName+"=tok")) _, err := mw(ctx, &authorizerv1.SessionRequest{}, info(authorizerv1.AuthorizerService_Session_FullMethodName), func(ctx context.Context, _ any) (any, error) { @@ -384,3 +384,75 @@ func TestAuth_SessionOnlyAcceptsPublicService(t *testing.T) { assert.Equal(t, 1, stub.sessionChecks) assert.Equal(t, 0, stub.userChecks) } + +// TestAuth_TokenResolverOverrideAppliesToBothSites pins the contract Auth's doc +// comment states: a non-nil TokenResolver replaces the default at EVERY +// identity-resolution site, not just one of them. +// +// This is the regression test for a real bug. The override was first wired into +// only the admin fallback, leaving the public path calling +// GetUserIDFromSessionOrAccessToken directly. Nothing caught it — it compiles, +// vets and lints clean, and the surface it breaks is the one that matters: +// MCP tools live on the PUBLIC AuthorizerService, so the MCP transport would +// have authenticated through the default resolver, which rejects the very +// resource-bound audience every MCP token is required to carry. The whole +// surface would have returned 401 with no failing test anywhere. +// +// The stub's counters are what make this airtight: asserting the custom resolver +// ran is not enough, because both could run. The default MUST NOT be consulted +// at all, or a token rejected by the surface's own rule could still be accepted +// by the default one. +func TestAuth_TokenResolverOverrideAppliesToBothSites(t *testing.T) { + cases := []struct { + name string + method string + }{ + {"public service", authorizerv1.AuthorizerService_Profile_FullMethodName}, + {"admin service non-super-admin fallback", authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName}, + } + + for _, tc := range cases { + t.Run(tc.name+" uses the override", func(t *testing.T) { + // Default resolver would succeed; the override decides instead. + stub := &stubTokenProvider{tokenData: &token.SessionOrAccessTokenData{UserID: "default-user"}} + resolverCalls := 0 + mw := Auth(stub, nil, func(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + resolverCalls++ + return &token.SessionOrAccessTokenData{UserID: "override-user"}, nil + }) + + var seen string + _, err := mw(context.Background(), nil, info(tc.method), func(ctx context.Context, _ any) (any, error) { + p, ok := authctx.FromContext(ctx) + require.True(t, ok, "an authenticated call must carry a principal") + seen = p.UserID + return nil, nil + }) + require.NoError(t, err) + assert.Equal(t, "override-user", seen, "the principal must come from the override, not the default") + assert.Equal(t, 1, resolverCalls) + assert.Zero(t, stub.userChecks, "the default resolver must not be consulted when an override is set") + }) + + t.Run(tc.name+" rejects when the override rejects", func(t *testing.T) { + // The inverse, and the one that actually protects the audience + // boundary: the default would ACCEPT this caller. If the default + // were still reachable, a token the surface's own rule rejected + // would authenticate anyway. + stub := &stubTokenProvider{tokenData: &token.SessionOrAccessTokenData{UserID: "default-user"}} + mw := Auth(stub, nil, func(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + return nil, status.Error(codes.Unauthenticated, "wrong audience") + }) + + called := false + _, err := mw(context.Background(), nil, info(tc.method), func(context.Context, any) (any, error) { + called = true + return nil, nil + }) + require.Error(t, err) + assert.Equal(t, codes.Unauthenticated, status.Code(err)) + assert.False(t, called, "the handler must not run for a caller the surface's resolver rejected") + assert.Zero(t, stub.userChecks, "a rejection must not fall back to the default resolver") + }) + } +} diff --git a/internal/grpcsrv/interceptors/mcp_auth.go b/internal/grpcsrv/interceptors/mcp_auth.go new file mode 100644 index 000000000..87327ebc3 --- /dev/null +++ b/internal/grpcsrv/interceptors/mcp_auth.go @@ -0,0 +1,67 @@ +package interceptors + +import ( + "fmt" + + "github.com/gin-gonic/gin" + + "github.com/authorizerdev/authorizer/internal/token" +) + +// MCPTokenResolver builds the TokenResolver used by the MCP surface's own, +// bufconn-only gRPC server. +// +// Two differences from the default resolver +// (token.Provider.GetUserIDFromSessionOrAccessToken), both narrowing: +// +// 1. Bearer only. There is no cookie fallback: MCP is a cookieless, +// Authorization-header protocol (MCP authorization §Access Token Usage), and +// a browser session cookie must never authenticate a tool call. Dropping the +// cookie path is also what makes /mcp safe to exempt from CSRF. +// 2. The MCP audience rule. resource is the canonical MCP resource URI, +// computed once from --url at wiring time and captured here, so no request +// header can influence which audience is accepted. +// +// Why this exists at all: ValidateAccessToken rejects every resource-bound +// audience at Authorizer's first-party surfaces, and an MCP token's audience is +// required to be exactly that. Rather than relaxing the shared rule — which would +// make an MCP token valid at /graphql too, defeating the binding — the MCP +// transport runs its own gRPC server whose auth interceptor uses this resolver. +// +// Passing a non-nil resolver also makes it the interceptor's SOLE authority: the +// super-admin check and the Session RPC's cookie-only branch are disabled, so +// point 1 above is enforced by the interceptor and not merely by this function +// declining to read cookies. Without that, a credential this resolver never saw +// could still authenticate a tool call. See interceptors.Auth. +// +// What this does NOT cover: a method marked `public` in proto is invoked with no +// principal at all, and the service layer then resolves the caller itself with +// the DEFAULT rule (service.callerTokenData / resolveFgaCaller). An MCP-exposed +// method that is both `public` and identity-resolving would therefore bypass +// everything here. TestExposedMCPToolsCannotBypassTheMCPTokenRule +// (internal/mcp) is what keeps that intersection empty. +func MCPTokenResolver(tp token.Provider, resource string) TokenResolver { + return func(gc *gin.Context) (*token.SessionOrAccessTokenData, error) { + accessToken, err := tp.GetAccessToken(gc) + if err != nil || accessToken == "" { + return nil, fmt.Errorf(`unauthorized`) + } + claims, err := tp.ValidateMCPAccessToken(gc, accessToken, resource) + if err != nil { + return nil, err + } + userID, ok := claims["sub"].(string) + if !ok || userID == "" { + return nil, fmt.Errorf(`unauthorized: missing sub claim`) + } + loginMethod, _ := claims["login_method"].(string) + nonce, _ := claims["nonce"].(string) + return &token.SessionOrAccessTokenData{ + UserID: userID, + LoginMethod: loginMethod, + Nonce: nonce, + ActorID: token.ImmediateActor(claims), + Scope: token.ClaimScopes(claims), + }, nil + } +} diff --git a/internal/grpcsrv/interceptors/mcp_auth_test.go b/internal/grpcsrv/interceptors/mcp_auth_test.go new file mode 100644 index 000000000..38d6364d2 --- /dev/null +++ b/internal/grpcsrv/interceptors/mcp_auth_test.go @@ -0,0 +1,197 @@ +package interceptors + +import ( + "context" + "fmt" + "net/http" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" + + authorizerv1 "github.com/authorizerdev/authorizer/gen/go/authorizer/v1" + "github.com/authorizerdev/authorizer/internal/authctx" + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/token" +) + +// mcpStubProvider records exactly which validation path a request took, so the +// tests below can assert that MCP callers reach ValidateMCPAccessToken and +// nothing else. +type mcpStubProvider struct { + token.Provider + + claims map[string]interface{} + err error + + seenToken string + seenResource string + mcpChecks int + defaultCheck int +} + +func (s *mcpStubProvider) GetAccessToken(gc *gin.Context) (string, error) { + auth := gc.Request.Header.Get("Authorization") + if len(auth) < 8 || auth[:7] != "Bearer " { + return "", fmt.Errorf("unauthorized") + } + return auth[7:], nil +} + +func (s *mcpStubProvider) ValidateMCPAccessToken(_ *gin.Context, accessToken, resource string) (map[string]interface{}, error) { + s.mcpChecks++ + s.seenToken = accessToken + s.seenResource = resource + if s.err != nil { + return nil, s.err + } + return s.claims, nil +} + +func (s *mcpStubProvider) GetUserIDFromSessionOrAccessToken(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + s.defaultCheck++ + return &token.SessionOrAccessTokenData{UserID: "default-rule-user"}, nil +} + +func mcpGinContext(header http.Header) *gin.Context { + req, _ := http.NewRequest(http.MethodPost, "/mcp", nil) + if header != nil { + req.Header = header + } + return &gin.Context{Request: req} +} + +// TestMCPTokenResolver covers the resolver in isolation. Until the transport +// lands nothing else constructs it, so without this the first execution of +// auth-critical code would be in production. +func TestMCPTokenResolver(t *testing.T) { + const resource = "https://auth.example.com/mcp" + + t.Run("a bearer token is validated against the configured resource", func(t *testing.T) { + stub := &mcpStubProvider{claims: map[string]interface{}{ + "sub": "user-1", + "login_method": "basic_auth", + "nonce": "n1", + "scope": []interface{}{"openid", "profile"}, + }} + h := http.Header{} + h.Set("Authorization", "Bearer tok-123") + + data, err := MCPTokenResolver(stub, resource)(mcpGinContext(h)) + require.NoError(t, err) + assert.Equal(t, "user-1", data.UserID) + assert.Equal(t, "basic_auth", data.LoginMethod) + assert.Equal(t, "n1", data.Nonce) + assert.Equal(t, []string{"openid", "profile"}, data.Scope) + + assert.Equal(t, "tok-123", stub.seenToken) + assert.Equal(t, resource, stub.seenResource, + "the resource must be the one captured at wiring time, never derived from the request") + assert.Equal(t, 1, stub.mcpChecks) + assert.Zero(t, stub.defaultCheck, "the MCP resolver must never consult the default rule") + }) + + t.Run("a cookie-only request is refused without touching either validator", func(t *testing.T) { + // The default resolver would accept a session cookie. MCP is a + // cookieless, Authorization-header protocol, and /mcp is CSRF-exempt + // precisely because no cookie can authenticate it. + stub := &mcpStubProvider{claims: map[string]interface{}{"sub": "user-1"}} + h := http.Header{} + h.Set("Cookie", "authorizer_session=whatever") + + _, err := MCPTokenResolver(stub, resource)(mcpGinContext(h)) + require.Error(t, err) + assert.Zero(t, stub.mcpChecks) + assert.Zero(t, stub.defaultCheck) + }) + + t.Run("a rejected token does not fall back to the default rule", func(t *testing.T) { + stub := &mcpStubProvider{err: fmt.Errorf("unauthorized: token audience is not this mcp server")} + h := http.Header{} + h.Set("Authorization", "Bearer wrong-audience") + + _, err := MCPTokenResolver(stub, resource)(mcpGinContext(h)) + require.Error(t, err) + assert.Zero(t, stub.defaultCheck) + }) + + t.Run("claims with no sub are refused", func(t *testing.T) { + stub := &mcpStubProvider{claims: map[string]interface{}{"login_method": "basic_auth"}} + h := http.Header{} + h.Set("Authorization", "Bearer tok") + + _, err := MCPTokenResolver(stub, resource)(mcpGinContext(h)) + require.Error(t, err) + }) +} + +// TestAuth_SoleAuthorityDisablesCookieAndAdminSecretPaths pins the property the +// MCP audience boundary depends on: when a server supplies its own resolver, +// that resolver is the ONLY way in. +// +// Two paths in the interceptor authenticate without consulting any resolver — +// tp.IsSuperAdmin (an admin cookie or the x-authorizer-admin-secret header) and +// the Session RPC's cookie-only branch. transport.MetaFromGRPC reconstructs +// cookies from gRPC metadata, so on the MCP server those would be reachable the +// moment the HTTP bridge forwarded request headers. Before this guard the +// boundary held only because neither Session nor any admin RPC happened to be +// mcp_tool-exposed — a proto annotation away from a browser cookie +// authenticating a tool call on an internet-facing, CSRF-exempt endpoint. +func TestAuth_SoleAuthorityDisablesCookieAndAdminSecretPaths(t *testing.T) { + t.Run("super-admin is not honoured when a resolver is set", func(t *testing.T) { + stub := &stubTokenProvider{superAdmin: true, tokenErr: status.Error(codes.Unauthenticated, "no")} + mw := Auth(stub, nil, func(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + return nil, status.Error(codes.Unauthenticated, "wrong audience") + }) + + called := false + _, err := mw(context.Background(), nil, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), + func(context.Context, any) (any, error) { called = true; return nil, nil }) + + require.Error(t, err) + assert.Equal(t, codes.Unauthenticated, status.Code(err)) + assert.False(t, called) + assert.Zero(t, stub.superAdminChecks, + "an admin cookie / admin secret is not a credential this surface accepts, so it must not even be checked") + }) + + t.Run("super-admin still works on a default server", func(t *testing.T) { + // The inverse: the guard must not disturb every existing gRPC server. + stub := &stubTokenProvider{superAdmin: true} + mw := Auth(stub, nil, nil) + + var isSuper bool + _, err := mw(context.Background(), nil, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), + func(ctx context.Context, _ any) (any, error) { + p, ok := authctx.FromContext(ctx) + require.True(t, ok) + isSuper = p.IsSuperAdmin + return nil, nil + }) + require.NoError(t, err) + assert.True(t, isSuper) + assert.Equal(t, 1, stub.superAdminChecks) + }) + + t.Run("the Session cookie branch is disabled when a resolver is set", func(t *testing.T) { + stub := &stubTokenProvider{sessionData: &token.SessionData{Subject: "cookie-user"}} + mw := Auth(stub, nil, func(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + return nil, status.Error(codes.Unauthenticated, "wrong audience") + }) + + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( + "cookie", constants.AppCookieName+"_session=sess-token", + )) + called := false + _, err := mw(ctx, &authorizerv1.SessionRequest{}, info(authorizerv1.AuthorizerService_Session_FullMethodName), + func(context.Context, any) (any, error) { called = true; return nil, nil }) + + require.Error(t, err) + assert.False(t, called) + assert.Zero(t, stub.sessionChecks, "a browser session must not authenticate a resolver-governed surface") + }) +} diff --git a/internal/grpcsrv/server.go b/internal/grpcsrv/server.go index 03bc3a693..2c6e73c03 100644 --- a/internal/grpcsrv/server.go +++ b/internal/grpcsrv/server.go @@ -29,6 +29,19 @@ type Dependencies struct { Config *config.Config ServiceProvider service.Provider TokenProvider token.Provider + // TokenResolver overrides how the auth interceptor turns a request into + // the caller's identity. nil means the default: a browser session cookie or + // a first-party bearer token. + // + // Set only by the MCP transport, which serves over an in-process bufconn and + // accepts exactly the resource-bound audience the default rejects. Because + // the rule lives on the server rather than the request, a token minted for + // /mcp cannot authenticate the TCP-listening server and vice versa. + // + // A non-nil value also disables the interceptor's cookie and admin-secret + // paths, so the resolver is the only way into that server. See + // interceptors.Auth and interceptors.MCPTokenResolver. + TokenResolver interceptors.TokenResolver } // Server wraps a *grpc.Server plus its listener address. @@ -54,7 +67,7 @@ func New(addr string, deps *Dependencies) (*Server, error) { // Records authorizer_api_operations_total{protocol,operation,status} // for every RPC (covers both gRPC and REST-via-gateway). interceptors.Metrics(), - interceptors.Auth(deps.TokenProvider, deps.Log), + interceptors.Auth(deps.TokenProvider, deps.Log, deps.TokenResolver), validate, // Innermost: wraps the handler directly so it can translate typed // service.Error values into proper gRPC status codes. Must stay diff --git a/internal/http_handlers/openid_config.go b/internal/http_handlers/openid_config.go index c0328903f..997188cfd 100644 --- a/internal/http_handlers/openid_config.go +++ b/internal/http_handlers/openid_config.go @@ -6,6 +6,22 @@ import ( "github.com/authorizerdev/authorizer/internal/parsers" ) +// supportedScopes is the scope list this deployment honours, advertised by BOTH +// discovery documents: OIDC/RFC 8414 authorization server metadata and RFC 9728 +// protected resource metadata. +// +// One list, because a client picks the scopes it requests from whichever +// document it read. A protected-resource document that omitted `offline_access` +// while the authorization server advertised it would send MCP clients into a +// flow that returns no refresh token, and the agent's session would die at +// access-token expiry with a 401 that looks like a broken integration. +// +// "phone" is advertised because phone_number/phone_number_verified are real, +// populated claims (see claims_supported). "address" is deliberately omitted — +// the User schema has no address fields, so claiming it would be false +// advertising. +var supportedScopes = []string{"openid", "email", "profile", "phone", "offline_access"} + // OpenIDConfigurationHandler handler for open-id configurations // Implements OpenID Connect Discovery 1.0 func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { @@ -53,14 +69,9 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { "id_token_signing_alg_values_supported": signingAlgs, // RECOMMENDED fields - "token_endpoint": issuer + "/oauth/token", - "userinfo_endpoint": issuer + "/userinfo", - // "phone" is advertised because phone_number/phone_number_verified - // are real, populated claims (see claims_supported below). "address" - // is deliberately omitted — the User schema has no address fields - // (street_address/locality/region/postal_code/country), so claiming - // support for it would be false advertising. - "scopes_supported": []string{"openid", "email", "profile", "phone", "offline_access"}, + "token_endpoint": issuer + "/oauth/token", + "userinfo_endpoint": issuer + "/userinfo", + "scopes_supported": supportedScopes, "claims_supported": []string{"aud", "exp", "iss", "iat", "sub", "given_name", "family_name", "middle_name", "nickname", "preferred_username", "picture", "email", "email_verified", "roles", "role", "gender", "birthdate", "phone_number", "phone_number_verified", "nonce", "updated_at", "created_at", "auth_time", "amr", "acr", "at_hash", "c_hash"}, "response_modes_supported": []string{"query", "fragment", "form_post", "web_message"}, "grant_types_supported": grantTypes, diff --git a/internal/http_handlers/protected_resource.go b/internal/http_handlers/protected_resource.go new file mode 100644 index 000000000..38800012e --- /dev/null +++ b/internal/http_handlers/protected_resource.go @@ -0,0 +1,80 @@ +package http_handlers + +import ( + "net/http" + + "github.com/gin-gonic/gin" +) + +// ProtectedResourceMetadataHandler serves OAuth 2.0 Protected Resource Metadata +// (RFC 9728) for this deployment's MCP server. +// +// The MCP authorization spec makes this mandatory — "MCP servers MUST implement +// OAuth 2.0 Protected Resource Metadata" — and it is the entry point of the whole +// discovery chain: an unauthenticated call to /mcp returns 401 with a +// WWW-Authenticate header pointing here, the client reads `authorization_servers` +// from this document, fetches the AS metadata from there, and only then starts the +// OAuth flow. Without this endpoint an MCP client has no way to learn where to +// authenticate, so the surface is unreachable no matter how correct the rest is. +// +// Served at ONE path: /.well-known/oauth-protected-resource/mcp. +// +// RFC 9728 §3.1 builds the metadata URL by inserting the well-known segment +// between the host and the PATH of the resource identifier, so this URL is the +// one that denotes "https:///mcp". The bare +// /.well-known/oauth-protected-resource denotes a different identifier — +// "https://", the origin with no path — and §3.3 requires a client to +// reject a document whose `resource` is not identical to the identifier it used +// to build the request. Serving this document there too would hand strict +// clients a mismatch to reject. Clients that probe the origin form are expected +// to reach us the documented way instead: the 401 from /mcp carries +// `WWW-Authenticate: Bearer resource_metadata=""`, which §5.1 makes +// the primary discovery mechanism, and well-known probing only the fallback. +// +// Here Authorizer is both the resource server and the authorization server, which +// is the easy case: `authorization_servers` names this same origin, so there is no +// third party to be confused about and no token to pass through to one. +// +// Deliberately public and cacheable: RFC 9728 §3.1 defines this as public client +// configuration. It carries no secret and no per-caller data — only where to +// authenticate and what to ask for. +func (h *httpProvider) ProtectedResourceMetadataHandler() gin.HandlerFunc { + return func(c *gin.Context) { + // Both derive from Config.CanonicalURL, which is the same normalization + // parsers.GetHost applies. The issuer advertised here therefore matches + // the `iss` claim on every token this server mints and the base of every + // URL the OIDC discovery document publishes. Reading the raw + // Config.AuthorizerURL instead would diverge the moment an operator's + // --url carried a path or a trailing slash — the metadata would name an + // authorization server that 404s while startup reported success. + base := h.Config.CanonicalURL() + resource := h.Config.MCPResource() + if base == "" || resource == "" { + // Unreachable in practice: the route is only registered when MCP is + // enabled, and startup refuses --mcp-enabled without --url. Answering + // 404 rather than emitting a document with an empty `resource` keeps + // the failure honest if that invariant ever breaks — a client that + // cannot discover us is far better than one that binds its tokens to + // an empty audience. + c.AbortWithStatus(http.StatusNotFound) + return + } + + c.JSON(http.StatusOK, gin.H{ + "resource": resource, + "authorization_servers": []string{base}, + // RFC 6750 §2.1 header form only. MCP forbids the token in the query + // string, and this server never reads it from a form body. + "bearer_methods_supported": []string{"header"}, + // Shared with the authorization server metadata rather than restated: + // a client that asks for exactly what this document advertises and + // gets no `offline_access` receives no refresh token, and the agent's + // session dies at access-token expiry. + "scopes_supported": supportedScopes, + // Clients use this to fetch the signing keys when they want to + // inspect a token locally; harmless to advertise and saves a probe. + "jwks_uri": base + "/.well-known/jwks.json", + "resource_documentation": "https://docs.authorizer.dev/core/mcp", + }) + } +} diff --git a/internal/http_handlers/provider.go b/internal/http_handlers/provider.go index 221dfe4a5..ee7e6b553 100644 --- a/internal/http_handlers/provider.go +++ b/internal/http_handlers/provider.go @@ -129,6 +129,9 @@ type Provider interface { SAMLIDPInitiatedHandler() gin.HandlerFunc // OpenIDConfigurationHandler is the main handler that handels all the openid configuration requests OpenIDConfigurationHandler() gin.HandlerFunc + // ProtectedResourceMetadataHandler serves RFC 9728 OAuth 2.0 Protected + // Resource Metadata for the MCP surface. Registered only when MCP is enabled. + ProtectedResourceMetadataHandler() gin.HandlerFunc // PlaygroundHandler is the main handler that handels all the playground requests PlaygroundHandler() gin.HandlerFunc // RevokeRefreshTokenHandler is the main handler that handels all the revoke refresh token requests diff --git a/internal/integration_tests/mcp_access_token_test.go b/internal/integration_tests/mcp_access_token_test.go new file mode 100644 index 000000000..b82eb2348 --- /dev/null +++ b/internal/integration_tests/mcp_access_token_test.go @@ -0,0 +1,231 @@ +package integration_tests + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage/schemas" + "github.com/authorizerdev/authorizer/internal/token" +) + +// mintStatefulAccessToken issues an access token bound to `resource` (RFC 8707) +// and registers it in the memory store exactly as /oauth/token does, so it is a +// genuine first-party token and not a hand-rolled JWT. Passing resource="" is +// how a test produces an ordinary login token, whose audience is the client id. +func mintStatefulAccessToken(t *testing.T, ts *testSetup, user *schemas.User, resource string) string { + t.Helper() + nonce := uuid.NewString() + tok, _, err := ts.TokenProvider.CreateAccessToken(&token.AuthTokenConfig{ + User: user, + Nonce: nonce, + Roles: []string{"user"}, + Scope: []string{"openid"}, + LoginMethod: constants.AuthRecipeMethodBasicAuth, + HostName: testAuthorizerHost(ts), + Resource: resource, + ExpireTime: "30m", + }) + require.NoError(t, err) + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + constants.AuthRecipeMethodBasicAuth+":"+user.ID, + constants.TokenTypeAccessToken+"_"+nonce, + tok, + time.Now().Add(time.Hour).Unix(), + )) + return tok +} + +// mintMachineAccessToken is mintStatefulAccessToken for a service account: `sub` +// is the client's surrogate id and there is no user. Same stateful registration, +// because the token endpoint registers machine tokens the same way. +func mintMachineAccessToken(t *testing.T, ts *testSetup, clientRowID, resource string) string { + t.Helper() + nonce := uuid.NewString() + tok, _, err := ts.TokenProvider.CreateAccessToken(&token.AuthTokenConfig{ + ServiceAccountID: clientRowID, + Nonce: nonce, + Scope: []string{"openid"}, + LoginMethod: constants.AuthRecipeMethodServiceAccount, + HostName: testAuthorizerHost(ts), + Resource: resource, + ExpireTime: "30m", + }) + require.NoError(t, err) + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + constants.AuthRecipeMethodServiceAccount+":"+clientRowID, + constants.TokenTypeAccessToken+"_"+nonce, + tok, + time.Now().Add(time.Hour).Unix(), + )) + return tok +} + +// TestMCPAccessTokenAudienceBoundary pins BOTH directions of the audience +// boundary between the MCP surface and Authorizer's first-party surfaces. +// +// This matters more than either half alone. The MCP specification requires that +// a server accept only tokens naming it as the audience; Authorizer additionally +// requires that a token naming the MCP server is NOT accepted anywhere else. One +// rule without the other is not a partial implementation, it is a vulnerability: +// +// - too loose at /mcp and a token minted for any other resource server (or an +// ordinary login token) authenticates MCP tool calls; +// - too loose on the first-party path and an MCP token — which a client may +// hand to a semi-trusted agent — becomes a full GraphQL/gRPC credential. +// +// These are also the regression tests for the decision-core extraction: the +// first-party rule is asserted here in its own right, so a future edit to the +// shared core that quietly widened it would fail. +func TestMCPAccessTokenAudienceBoundary(t *testing.T) { + cfg := getTestConfig() + // The canonical resource is derived from --url and nothing else. Setting it + // here is what the operator does at startup; without it MCPResource() is + // empty and the server refuses to enable MCP at all. + cfg.AuthorizerURL = "https://auth.example.com" + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + resource := cfg.MCPResource() + require.Equal(t, "https://auth.example.com/mcp", resource, + "the canonical resource form is what clients send as `resource` and what tokens carry as `aud` — it must not drift") + + now := time.Now().Unix() + user, err := ts.StorageProvider.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("mcp_" + uuid.NewString() + "@authorizer.dev"), + EmailVerifiedAt: &now, + SignupMethods: constants.AuthRecipeMethodBasicAuth, + Roles: "user", + }) + require.NoError(t, err) + + gc := &gin.Context{Request: ts.GinContext.Request} + + t.Run("a token bound to this MCP server is accepted at /mcp", func(t *testing.T) { + claims, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, mintStatefulAccessToken(t, ts, user, resource), resource) + require.NoError(t, vErr, "a correctly-audienced token must reach MCP, or the surface is dead code") + assert.Equal(t, user.ID, claims["sub"]) + }) + + t.Run("the same token is rejected at authorizer's own surfaces", func(t *testing.T) { + tok := mintStatefulAccessToken(t, ts, user, resource) + _, vErr := ts.TokenProvider.ValidateAccessToken(gc, tok) + require.Error(t, vErr, "an MCP-bound token must not double as a GraphQL/gRPC credential") + + // Assert at the ENTRY POINT gRPC, REST and GraphQL actually call, not + // just at ValidateAccessToken. GetUserIDFromSessionOrAccessToken falls + // back to ValidateDelegatedAccessToken whenever the first-party check + // fails, so the real boundary is the pair. Today the fallback also + // rejects (no `act` claim, and aud=/mcp is not the bare host), but + // asserting only the inner call would let a future relaxation of either + // delegated rule reopen MCP-token-authenticates-GraphQL with this test + // still green — the exact failure this test exists to prevent. + req := httptest.NewRequest(http.MethodPost, "/graphql", nil) + req.Header.Set("Authorization", "Bearer "+tok) + _, vErr = ts.TokenProvider.GetUserIDFromSessionOrAccessToken(&gin.Context{Request: req}) + require.Error(t, vErr, "an MCP-bound token must not resolve to an identity on the shared entry point") + }) + + t.Run("an ordinary login token is rejected at /mcp", func(t *testing.T) { + loginToken := mintStatefulAccessToken(t, ts, user, "") + // Sanity: it really is a working first-party token, so the rejection + // below is about audience and not about some unrelated defect. + _, vErr := ts.TokenProvider.ValidateAccessToken(gc, loginToken) + require.NoError(t, vErr) + + _, vErr = ts.TokenProvider.ValidateMCPAccessToken(gc, loginToken, resource) + require.Error(t, vErr, "the client_id audience every login produces must not authenticate MCP") + }) + + t.Run("a token bound to a different resource server is rejected at /mcp", func(t *testing.T) { + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, + mintStatefulAccessToken(t, ts, user, "https://evil.example.com/mcp"), resource) + require.Error(t, vErr) + }) + + t.Run("a token bound to the bare host is rejected at /mcp", func(t *testing.T) { + // The path is part of the identifier. Accepting the bare origin would + // mean any token minted for "this server" reached the tool surface. + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, + mintStatefulAccessToken(t, ts, user, "https://auth.example.com"), resource) + require.Error(t, vErr) + }) + + t.Run("an empty configured resource authenticates nobody", func(t *testing.T) { + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, mintStatefulAccessToken(t, ts, user, resource), "") + require.Error(t, vErr, "a misconfigured deployment must fail closed, not accept every audience") + }) + + t.Run("a token with no live session is rejected at /mcp", func(t *testing.T) { + // Not registered in the memory store: logout, password reset and admin + // revoke all work by removing that entry, so this is the revocation lever. + tok, _, mErr := ts.TokenProvider.CreateAccessToken(&token.AuthTokenConfig{ + User: user, + Nonce: uuid.NewString(), + Roles: []string{"user"}, + LoginMethod: constants.AuthRecipeMethodBasicAuth, + HostName: testAuthorizerHost(ts), + Resource: resource, + ExpireTime: "30m", + }) + require.NoError(t, mErr) + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, tok, resource) + require.Error(t, vErr) + }) + + t.Run("a garbage token is rejected at /mcp", func(t *testing.T) { + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, "not-a-jwt", resource) + require.Error(t, vErr) + }) +} + +// TestMCPAccessTokenServiceAccountLiveness pins the one place the MCP validator +// is deliberately STRICTER than the first-party path. +// +// userIsRevoked resolves a token's subject as a user only, and returns "not +// revoked" when it finds nothing — so a machine token, whose `sub` is a client +// row id, survives deactivation of the service account until it expires. That is +// inherited behaviour on existing surfaces. MCP's headline callers are agents and +// service accounts, so shipping it there knowingly would be worse: the validator +// uses subjectIsLive, which resolves user-then-client and fails closed. +func TestMCPAccessTokenServiceAccountLiveness(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = "https://auth.example.com" + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + resource := cfg.MCPResource() + gc := &gin.Context{Request: ts.GinContext.Request} + + client, err := ts.StorageProvider.AddClient(ctx, &schemas.Client{ + ClientID: "svc-" + uuid.NewString(), + Kind: constants.ClientKindServiceAccount, + Name: "mcp-agent", + AllowedScopes: "openid", + IsActive: true, + }) + require.NoError(t, err) + + t.Run("an active service account reaches /mcp", func(t *testing.T) { + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, mintMachineAccessToken(t, ts, client.ID, resource), resource) + require.NoError(t, vErr) + }) + + t.Run("a deactivated service account does not", func(t *testing.T) { + tok := mintMachineAccessToken(t, ts, client.ID, resource) + client.IsActive = false + _, uErr := ts.StorageProvider.UpdateClient(ctx, client) + require.NoError(t, uErr) + + _, vErr := ts.TokenProvider.ValidateMCPAccessToken(gc, tok, resource) + require.Error(t, vErr, "deactivating a service account must stop its live MCP tokens, not just block new ones") + }) +} diff --git a/internal/integration_tests/mcp_prm_test.go b/internal/integration_tests/mcp_prm_test.go new file mode 100644 index 000000000..3b7c183d6 --- /dev/null +++ b/internal/integration_tests/mcp_prm_test.go @@ -0,0 +1,127 @@ +package integration_tests + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestProtectedResourceMetadata pins the RFC 9728 document that starts the whole +// MCP discovery chain. +// +// A client that has never seen this deployment learns everything from here: which +// URI to name as its RFC 8707 `resource` (and therefore what audience its token +// will carry), and which authorization server to go to. Get `resource` wrong and +// every token a client obtains is bound to an audience /mcp will reject; get +// `authorization_servers` wrong and the client never reaches the OAuth flow at +// all. Both failures look identical from outside — a permanent 401 — which is why +// the exact strings are asserted rather than merely their presence. +func TestProtectedResourceMetadata(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = "https://auth.example.com" + cfg.MCPEnabled = true + ts := initTestSetup(t, cfg) + + router := gin.New() + // ONE path, matching the real router. RFC 9728 §3.1 inserts the well-known + // segment ahead of the resource identifier's PATH, so this URL denotes + // "/mcp"; the bare well-known path denotes the origin, and §3.3 has + // clients reject a document whose `resource` does not match the identifier + // they used to build the request. + router.GET("/.well-known/oauth-protected-resource/mcp", ts.HttpProvider.ProtectedResourceMetadataHandler()) + + get := func(t *testing.T, path string) map[string]any { + t.Helper() + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, path, nil) + // A hostile Host/X-Authorizer-URL must not change the advertised + // resource: the document is derived from --url alone. If it could be + // steered, an attacker could publish a resource identifier matching a + // token they already hold. + req.Host = "evil.example.com" + req.Header.Set("X-Authorizer-URL", "https://evil.example.com") + router.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var doc map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &doc)) + return doc + } + + t.Run("the document is correct at the RFC 9728 §3.1 path", func(t *testing.T) { + doc := get(t, "/.well-known/oauth-protected-resource/mcp") + assert.Equal(t, "https://auth.example.com/mcp", doc["resource"], + "this is the exact string clients send as `resource` and tokens carry as `aud`") + assert.Equal(t, []any{"https://auth.example.com"}, doc["authorization_servers"], + "RFC 9728 requires at least one authorization server; here Authorizer is its own") + assert.Equal(t, []any{"header"}, doc["bearer_methods_supported"], + "MCP forbids tokens in the query string — only the Authorization header") + assert.Contains(t, doc["scopes_supported"], "offline_access", + "a client that requests only what this document advertises must still be able to obtain a refresh token") + }) +} + +// TestProtectedResourceMetadataNormalizesIssuer pins the one thing that has to +// agree across three documents: the origin. +// +// `resource` is normalized (scheme+host) and so is the `iss` claim on every +// token, because both come from the same sanitizer parsers.GetHost uses. If the +// advertised authorization server and jwks_uri were built from the RAW --url +// instead, an operator whose --url carries a path or a trailing slash would +// publish an authorization server that 404s and an issuer no token matches, +// while startup reported everything fine — a discovery chain that dead-ends with +// no error anywhere. +func TestProtectedResourceMetadataNormalizesIssuer(t *testing.T) { + for _, rawURL := range []string{ + "https://auth.example.com/auth", // a path an operator might add behind a proxy + "https://auth.example.com/", // a trailing slash + } { + t.Run(rawURL, func(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = rawURL + cfg.MCPEnabled = true + ts := initTestSetup(t, cfg) + + router := gin.New() + router.GET("/.well-known/oauth-protected-resource/mcp", ts.HttpProvider.ProtectedResourceMetadataHandler()) + + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/.well-known/oauth-protected-resource/mcp", nil)) + require.Equal(t, http.StatusOK, w.Code) + + var doc map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &doc)) + assert.Equal(t, "https://auth.example.com/mcp", doc["resource"]) + assert.Equal(t, []any{"https://auth.example.com"}, doc["authorization_servers"]) + assert.Equal(t, "https://auth.example.com/.well-known/jwks.json", doc["jwks_uri"]) + }) + } +} + +// TestProtectedResourceMetadataFailsClosedWithoutURL asserts the handler refuses +// to emit a document when no canonical URL is configured, rather than publishing +// one with an empty `resource`. +// +// Startup already refuses --mcp-enabled without --url, so this is the second lock +// on the same door. It is worth having because the failure it prevents is silent: +// a document advertising resource:"" would lead clients to request tokens with an +// empty audience, which is exactly the audience an unconfigured deployment would +// then be comparing against. +func TestProtectedResourceMetadataFailsClosedWithoutURL(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = "" + cfg.MCPEnabled = true + ts := initTestSetup(t, cfg) + + router := gin.New() + router.GET("/.well-known/oauth-protected-resource", ts.HttpProvider.ProtectedResourceMetadataHandler()) + + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/.well-known/oauth-protected-resource", nil)) + assert.Equal(t, http.StatusNotFound, w.Code) +} diff --git a/internal/mcp/exposed_methods_test.go b/internal/mcp/exposed_methods_test.go new file mode 100644 index 000000000..2d69dc3cf --- /dev/null +++ b/internal/mcp/exposed_methods_test.go @@ -0,0 +1,108 @@ +package mcp + +import ( + "testing" + + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" + + authorizerv1 "github.com/authorizerdev/authorizer/gen/go/authorizer/v1" +) + +// identityFreeExposedMethods are the MCP-exposed RPCs that are ALSO marked +// `public` and are therefore allowed to be so. +// +// Adding a name here is a security assertion, not a formality: it claims the +// method never resolves a caller identity — no authctx.Principal read, no +// callerTokenData, no resolveFgaCaller, no direct TokenProvider use. Verify that +// in the service implementation before adding one. +var identityFreeExposedMethods = map[string]string{ + // internal/service/meta.go returns static deployment configuration and + // contains no reference to authctx, callerTokenData, resolveFgaCaller or + // TokenProvider. + "Meta": "returns static deployment configuration; resolves no caller", +} + +// TestExposedMCPToolsCannotBypassTheMCPTokenRule guards the assumption the MCP +// audience boundary actually rests on. +// +// The boundary is enforced by giving the MCP surface its own gRPC server whose +// auth interceptor uses MCPTokenResolver — a token is accepted only when its +// `aud` names this MCP server. But the interceptor attaches a principal only for +// methods that require auth. For a method marked `public` it calls the handler +// with no principal at all, and the service layer then resolves the caller +// itself: callerTokenData (internal/service/caller.go:30) and resolveFgaCaller +// (internal/service/fga.go) both fall back to +// TokenProvider.GetUserIDFromSessionOrAccessToken, which is the DEFAULT rule — +// the one that accepts an ordinary client_id-audience login token and rejects +// the resource-bound audience every MCP token carries. +// +// So a method that is both `mcp_tool.exposed` and `public` and that +// opportunistically resolves an identity would authenticate MCP callers with a +// token that never named the MCP server, silently, on an internet-facing, +// CSRF-exempt endpoint. Nothing would fail to compile and no other test covers +// it: today the property holds only because the single method in that +// intersection is Meta, which resolves nobody. +// +// This test turns that coincidence into a checked invariant. It is the same +// shape as TestAdminMethodsAreGated and TestNotFoundContractIsUniform: a static +// assertion over annotations, so a one-line proto change cannot quietly move the +// security boundary. +func TestExposedMCPToolsCannotBypassTheMCPTokenRule(t *testing.T) { + var checked int + protoregistry.GlobalFiles.RangeFiles(func(fd protoreflect.FileDescriptor) bool { + svcs := fd.Services() + for i := 0; i < svcs.Len(); i++ { + methods := svcs.Get(i).Methods() + for j := 0; j < methods.Len(); j++ { + m := methods.Get(j) + tool := mcpToolFromMethod(m) + if tool == nil || !tool.GetExposed() { + continue + } + checked++ + if !methodIsPublic(m) { + // Requires auth, so the interceptor resolves identity through + // this server's own resolver and attaches a principal. Safe. + continue + } + name := string(m.Name()) + if _, ok := identityFreeExposedMethods[name]; !ok { + t.Errorf("RPC %s is both (authorizer.v1.mcp_tool).exposed and (authorizer.v1.public), "+ + "so the MCP server's auth interceptor will invoke it with NO principal and its "+ + "service implementation would resolve the caller with the DEFAULT token rule — "+ + "accepting a login token that never named the MCP server as its audience. "+ + "Either drop the `public` annotation, or (only if the method resolves no caller "+ + "identity at all) add it to identityFreeExposedMethods with the reason.", + m.FullName()) + } + } + } + return true + }) + + if checked == 0 { + t.Fatal("found no mcp_tool-exposed methods — the proto registry was not linked in, so this test proved nothing") + } + t.Logf("checked %d mcp_tool-exposed methods", checked) +} + +// methodIsPublic mirrors interceptors.isPublicMethod. Duplicated rather than +// exported across packages because it is two lines and this test must read the +// annotation exactly as the interceptor does; a shared helper that drifted from +// the interceptor would make this test agree with itself instead of with the +// code it guards. +func methodIsPublic(m protoreflect.MethodDescriptor) bool { + opts := m.Options() + if opts == nil { + return false + } + switch v := proto.GetExtension(opts, authorizerv1.E_Public).(type) { + case bool: + return v + case *bool: + return v != nil && *v + } + return false +} diff --git a/internal/server/http_routes.go b/internal/server/http_routes.go index 394ee1127..fa67d7a74 100644 --- a/internal/server/http_routes.go +++ b/internal/server/http_routes.go @@ -99,6 +99,22 @@ func (s *server) NewRouter() *gin.Engine { // so serve the same handler as a thin alias (cheap interop win). router.GET("/.well-known/oauth-authorization-server", s.Dependencies.HTTPProvider.OpenIDConfigurationHandler()) router.GET("/.well-known/jwks.json", s.Dependencies.HTTPProvider.JWKsHandler()) + // RFC 9728 OAuth 2.0 Protected Resource Metadata for the MCP surface. + // Registered only when MCP is enabled — advertising a protected resource that + // does not exist would send clients through a discovery chain ending in 404. + // + // ONE path, per RFC 9728 §3.1: the well-known segment is inserted between the + // host and the resource identifier's PATH, so this URL is the one that denotes + // "/mcp". The bare /.well-known/oauth-protected-resource denotes the + // origin instead, and §3.3 has clients reject a document whose `resource` does + // not match the identifier they used — so serving it there as well would hand + // strict clients a mismatch. Discovery reaches this URL through the + // `WWW-Authenticate: Bearer resource_metadata="…"` header on /mcp's 401, which + // §5.1 defines as the primary mechanism. + if s.Dependencies.AppConfig != nil && s.Dependencies.AppConfig.MCPEnabled { + router.GET("/.well-known/oauth-protected-resource/mcp", + s.Dependencies.HTTPProvider.ProtectedResourceMetadataHandler()) + } // RFC 6749 §3.1 / OIDC Core §3.1.2.1: the authorization endpoint MUST // support GET and MAY support POST. router.GET("/authorize", s.Dependencies.HTTPProvider.AuthorizeHandler()) diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index 4eeb95ab2..a6d50bef8 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -460,6 +460,81 @@ func (p *provider) GetAccessToken(gc *gin.Context) (string, error) { // Function to validate access token for authorizer apis (profile, update_profile) func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map[string]interface{}, error) { + return p.validateStatefulAccessToken(gc, accessToken, p.firstPartyAudienceOK, p.userIsNotRevoked) +} + +// firstPartyAudienceOK is the audience rule for Authorizer's OWN protected +// resources (/userinfo, GraphQL, gRPC, REST). +// +// RFC 8707 audience restriction. A token minted with a resource indicator +// carries that resource (an absolute URI, e.g. "https://mcp.example.com") as +// its `aud` so it is usable ONLY at that external resource server — which +// validates it locally against the published JWKS, NOT here. (Not via +// /oauth/introspect either: that endpoint only answers for a token whose aud +// is the authenticated caller's own client_id, so a resource-bound token +// always introspects as inactive — see token.DelegatedAccessTokenTTL.) +// Accepting a resource-bound token here would defeat the audience restriction +// the token was issued with, so reject any `aud` that is an absolute URI +// (resource indicator form) other than the configured default audience. +// Legitimate client-bound tokens carry an opaque client_id `aud` (not a URI) +// and are unaffected. Introspection uses ParseJWTToken directly and is +// untouched. +// +// The MCP surface is the deliberate counterpart: it accepts EXACTLY the +// resource-bound audience this rule rejects, and nothing else. See +// ValidateMCPAccessToken. +func (p *provider) firstPartyAudienceOK(aud string) error { + if aud != "" && aud != p.config.ClientID { + if u, err := url.Parse(aud); err == nil && u.IsAbs() { + p.dependencies.Log.Debug().Str("aud", aud).Msg("access token rejected: resource-bound audience not valid at authorizer's own endpoints") + return fmt.Errorf(`unauthorized: token audience is a resource indicator`) + } + } + return nil +} + +// userIsNotRevoked is the subject-liveness rule for first-party surfaces: +// unchanged from the behaviour that shipped before the decision core was +// extracted. It resolves the subject as a USER only, so a machine token's +// service-account subject is not checked here — see subjectIsLive for the +// stricter rule the MCP and delegated surfaces use. +func (p *provider) userIsNotRevoked(gc *gin.Context, subject string) bool { + if p.userIsRevoked(gc, subject) { + p.dependencies.Log.Debug().Str("user_id", subject).Msg("access token rejected: user revoked") + return false + } + return true +} + +// validateStatefulAccessToken is the single decision core behind every +// STATEFUL access-token check: signature and expiry, a live memory-store +// session entry whose stored digest matches the presented token, subject +// liveness, audience, issuer/claims, and token type. +// +// Both human tokens and client_credentials machine tokens flow through here — +// createMachineAccessToken stamps the same `nonce` and `login_method` shape, and +// the token endpoint registers machine tokens in the memory store exactly as +// human ones, so the session lookup below is uniform. +// +// Exactly two checks vary per surface, and they are the two parameters: +// +// - audienceOK decides which `aud` values that surface accepts. This is what +// makes RFC 8707 audience binding real: a token is accepted only where its +// audience says it belongs. +// - subjectLive decides how the subject's liveness is confirmed. First-party +// surfaces resolve the subject as a user; surfaces whose callers are +// routinely service accounts resolve user-then-client and fail closed. +// +// Everything else is identical on purpose. A new surface adds a policy pair +// here rather than a second copy of this function, so a fix to the session +// digest comparison or the claims check cannot land on one surface and miss +// another. +func (p *provider) validateStatefulAccessToken( + gc *gin.Context, + accessToken string, + audienceOK func(aud string) error, + subjectLive func(gc *gin.Context, subject string) bool, +) (map[string]interface{}, error) { res := make(map[string]interface{}) if accessToken == "" { @@ -496,9 +571,8 @@ func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map return res, fmt.Errorf(`unauthorized`) } - if p.userIsRevoked(gc, userID) { - p.dependencies.Log.Debug().Str("user_id", userID).Msg("access token rejected: user revoked") - return res, fmt.Errorf(`unauthorized: user revoked`) + if !subjectLive(gc, userID) { + return res, fmt.Errorf(`unauthorized: subject is not active`) } // /userinfo and the generic session-or-access-token resolver present no @@ -508,25 +582,8 @@ func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map // the JWT signature) as the expected value; the checks above already // establish the token is a genuine, unexpired, unrevoked Authorizer token. aud, _ := res["aud"].(string) - - // RFC 8707 audience restriction. A token minted with a resource indicator - // carries that resource (an absolute URI, e.g. "https://mcp.example.com") as - // its `aud` so it is usable ONLY at that external resource server — which - // validates it locally against the published JWKS, NOT here. (Not via - // /oauth/introspect either: that endpoint only answers for a token whose aud - // is the authenticated caller's own client_id, so a resource-bound token - // always introspects as inactive — see token.DelegatedAccessTokenTTL.) This path guards - // Authorizer's OWN protected resources (/userinfo, GraphQL, gRPC). Accepting - // a resource-bound token here would defeat the audience restriction the token - // was issued with, so reject any `aud` that is an absolute URI (resource - // indicator form) other than the configured default audience. Legitimate - // client-bound tokens carry an opaque client_id `aud` (not a URI) and are - // unaffected. Introspection uses ParseJWTToken directly and is untouched. - if aud != "" && aud != p.config.ClientID { - if u, err := url.Parse(aud); err == nil && u.IsAbs() { - p.dependencies.Log.Debug().Str("aud", aud).Msg("access token rejected: resource-bound audience not valid at authorizer's own endpoints") - return res, fmt.Errorf(`unauthorized: token audience is a resource indicator`) - } + if err := audienceOK(aud); err != nil { + return res, err } hostname := parsers.GetHost(gc) if ok, err := p.ValidateJWTClaims(res, &AuthTokenConfig{ diff --git a/internal/token/delegated_access_token.go b/internal/token/delegated_access_token.go index 1c9e82d0f..9f3264651 100644 --- a/internal/token/delegated_access_token.go +++ b/internal/token/delegated_access_token.go @@ -119,7 +119,7 @@ func (p *provider) ValidateDelegatedAccessToken(gc *gin.Context, accessToken str return res, fmt.Errorf(`unauthorized: originating session is no longer valid`) } - if !p.delegationSubjectIsLive(gc, userID) { + if !p.subjectIsLive(gc, userID) { return res, fmt.Errorf(`unauthorized: delegation subject is not active`) } @@ -189,16 +189,18 @@ func sameAudience(aud, hostname string) bool { return a != "" && h != "" && a == h } -// delegationSubjectIsLive reports whether the subject a delegated token was -// minted for is still active. +// subjectIsLive reports whether the subject a token was minted for is still +// active. Used by the surfaces whose callers are routinely service accounts +// rather than humans: RFC 8693 delegation (here) and MCP +// (ValidateMCPAccessToken). // // The subject is NOT always a user. RFC 8693 token exchange also accepts a // service account as the subject, which is how a multi-hop chain is expressed -// (agent A delegates to agent B). userIsRevoked only ever looked the subject up +// (agent A delegates to agent B), and a client_credentials token's `sub` is the +// service account's surrogate id. userIsRevoked only ever looked the subject up // as a USER, so for a service-account subject it found nothing, reported "not -// revoked", and the delegation kept working for the token's full lifetime after -// the service account had been deactivated — deactivation did not stop the -// chain it seeded. +// revoked", and the token kept working for its full lifetime after the service +// account had been deactivated — deactivation did not stop the chain it seeded. // // Resolution order mirrors how the token endpoint validates the subject at // mint time (see handleTokenExchangeGrant): try user, then client. @@ -206,7 +208,7 @@ func sameAudience(aud, hostname string) bool { // Fails CLOSED when the subject resolves to neither. A subject we cannot // confirm is live must not authenticate — the same rule the exchange applies // before it will seed a delegation at all. -func (p *provider) delegationSubjectIsLive(gc *gin.Context, subject string) bool { +func (p *provider) subjectIsLive(gc *gin.Context, subject string) bool { if p.dependencies.StorageProvider == nil || subject == "" { return false } @@ -214,7 +216,7 @@ func (p *provider) delegationSubjectIsLive(gc *gin.Context, subject string) bool if user, err := p.dependencies.StorageProvider.GetUserByID(gc, subject); err == nil && user != nil { if user.RevokedTimestamp != nil { p.dependencies.Log.Debug().Str("subject", subject). - Msg("delegated token rejected: subject user is revoked") + Msg("token rejected: subject user is revoked") return false } return true @@ -223,13 +225,13 @@ func (p *provider) delegationSubjectIsLive(gc *gin.Context, subject string) bool if client, err := p.dependencies.StorageProvider.GetClientByID(gc, subject); err == nil && client != nil { if !client.IsActive { p.dependencies.Log.Debug().Str("subject", subject). - Msg("delegated token rejected: subject service account is deactivated") + Msg("token rejected: subject service account is deactivated") return false } return true } p.dependencies.Log.Debug().Str("subject", subject). - Msg("delegated token rejected: subject resolves to neither an active user nor an active client") + Msg("token rejected: subject resolves to neither an active user nor an active client") return false } diff --git a/internal/token/mcp_access_token.go b/internal/token/mcp_access_token.go new file mode 100644 index 000000000..6fc93b90b --- /dev/null +++ b/internal/token/mcp_access_token.go @@ -0,0 +1,79 @@ +package token + +import ( + "fmt" + + "github.com/gin-gonic/gin" +) + +// ValidateMCPAccessToken validates an access token presented at Authorizer's +// own MCP surface (POST /mcp), where Authorizer acts as an OAuth 2.1 resource +// server for itself. +// +// # The audience is the whole point +// +// The MCP specification requires a client to name the MCP server as the RFC 8707 +// `resource` when it asks for a token, and requires the server to accept only +// tokens issued for itself: +// +// "MCP servers MUST only accept tokens specifically intended for themselves and +// MUST reject tokens that do not include them in the audience claim." +// +// `resource` becomes the token's `aud` at issuance (see accessTokenAudience), so +// enforcing that here is what makes the binding real rather than decorative. A +// token minted for any other resource — including one minted for Authorizer's own +// client_id, which is what every ordinary login produces — is rejected. +// +// This is the exact mirror image of firstPartyAudienceOK, which rejects every +// resource-bound audience at /userinfo, GraphQL, gRPC and REST. Between the two +// rules, a token is accepted at exactly one surface: the one it names. Neither +// rule has an "or" in it, and that is deliberate — an MCP token must not be +// replayable at /graphql, and a login token must not be replayable at /mcp. +// +// # resource is caller-supplied, never request-derived +// +// The canonical resource URI is passed in by the caller, which computes it ONCE +// at wiring time from the operator-configured --url. It is deliberately not +// derived from the request: parsers.GetHost falls back to request headers when +// --url is unset, and an audience check against an attacker-controllable header +// authenticates anyone. Startup refuses to enable MCP without --url; this +// signature is the second lock on the same door. +// +// # Everything else is the ordinary stateful check +// +// Signature, expiry, a live memory-store session entry whose stored digest +// matches the presented token, issuer/claims and token type all come from +// validateStatefulAccessToken — the same core ValidateAccessToken uses. MCP is +// NOT a weaker path: it differs from the first-party check in the audience rule +// (stricter) and in resolving subject liveness as user-then-client (also +// stricter — see subjectIsLive; a deactivated service account is rejected here +// even though the first-party path would still accept its token). +// +// # What is deliberately NOT accepted +// +// RFC 8693 delegated tokens. They are stateless by design — no nonce, no session +// entry — so they fail the core's session lookup, and ValidateDelegatedAccessToken +// requires `aud` to equal the bare server URL rather than the /mcp resource. An +// agent therefore cannot yet reach /mcp with a delegated token. That is a scoping +// decision, not an oversight: the delegated path gives up the byte-for-byte +// comparison against a stored token, and widening it is a deliberate edit to that +// function (see its doc comment), not a side effect of adding a transport. +func (p *provider) ValidateMCPAccessToken(gc *gin.Context, accessToken string, resource string) (map[string]interface{}, error) { + if resource == "" { + // Fail closed. An empty expected audience would make the comparison + // below accept-nothing via sameAudience, but relying on that is too + // subtle for an auth path: say no explicitly. + return map[string]interface{}{}, fmt.Errorf(`unauthorized: no mcp resource configured`) + } + return p.validateStatefulAccessToken(gc, accessToken, + func(aud string) error { + if !sameAudience(aud, resource) { + p.dependencies.Log.Debug().Str("aud", aud).Str("expected", resource). + Msg("access token rejected at mcp: audience names a different resource") + return fmt.Errorf(`unauthorized: token audience is not this mcp server`) + } + return nil + }, + p.subjectIsLive, + ) +} diff --git a/internal/token/provider.go b/internal/token/provider.go index 9a558cec9..c4560da98 100644 --- a/internal/token/provider.go +++ b/internal/token/provider.go @@ -86,6 +86,12 @@ type Provider interface { // ValidateAccessToken by exactly one property (no session lookup) and // stricter by one (audience must be this server) — see its doc comment. ValidateDelegatedAccessToken(gc *gin.Context, accessToken string) (map[string]interface{}, error) + // ValidateMCPAccessToken validates an access token presented at the MCP + // surface. Same stateful core as ValidateAccessToken, stricter on two + // checks: `aud` must equal the caller-supplied canonical MCP resource URI + // (RFC 8707 / MCP authorization), and subject liveness resolves + // user-then-client so a deactivated service account is rejected. + ValidateMCPAccessToken(gc *gin.Context, accessToken string, resource string) (map[string]interface{}, error) // ValidateAdminToken validates session token ValidateBrowserSession(gc *gin.Context, encryptedSession string) (*SessionData, error) // ValidateJWTClaims validates jwt claims From 138f98c466952d7e9b90a1cfff02c11ad01a45e2 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 17:05:39 +0530 Subject: [PATCH 02/11] security(token): deactivating a service account revokes its tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject liveness resolved a token's `sub` as a USER and treated "no such user" as "not revoked". A client_credentials token's `sub` is the service account's row id, never a user id, so the lookup missed every time and reported the caller live. Setting a service account inactive therefore blocked new issuance and did nothing to the tokens already out: an operator revoking a compromised machine identity got a success response and a credential that kept working at GraphQL, gRPC and REST until it expired. A control that looks like it enforces and silently does not is worse than no control — the operator reads it as effective and stops looking. The decision core now uses subjectIsLive, which resolves user-then-client and fails closed when the subject is neither. The delegated path already used it; this brings the first-party path in line, so there is one subject rule rather than two, and the parameter that selected between them is gone. Failing closed on an unresolvable subject also matters as a fallback: DeleteUser purges sessions through asyncutil.Go best-effort, so a failed or racing purge previously left a deleted user's token authenticating. Not backward compatible, deliberately, and scoped to exactly that: live users and active service accounts are unaffected, and machine tokens pay one extra client lookup. Landing it now rather than in 2.5.0 because service accounts, client_credentials and workload identity are all new in 2.4.0 — there is no installed base, so this is a bug that never shipped instead of a behaviour change to a released feature. Industry norm is looser (Auth0, Okta and Keycloak treat client_credentials tokens as non-revocable before expiry, bounded by short TTLs); this repo already carries the session-store check that makes revocation possible, it just never applied to the subjects that needed it. Refs authorizer-docs specs/2026-08-10-mcp-http-transport.md §6 --- CHANGELOG.md | 2 + .../machine_token_liveness_test.go | 109 ++++++++++++++++++ .../mcp_access_token_test.go | 22 +++- internal/token/auth_token.go | 48 ++++---- internal/token/delegated_access_token.go | 20 ++-- internal/token/mcp_access_token.go | 29 ++--- 6 files changed, 172 insertions(+), 58 deletions(-) create mode 100644 internal/integration_tests/machine_token_liveness_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 789364e45..2d1a349c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id ### Security +- **BEHAVIOUR CHANGE — deactivating a service account now revokes its live access tokens.** Token validation resolved a token's subject as a *user* only and treated "no such user" as "not revoked". A `client_credentials` token's `sub` is the service account's row id, never a user id, so that lookup missed every time and reported the caller live: setting a service account inactive blocked new token issuance but did nothing to tokens already outstanding, which kept working at GraphQL, gRPC and REST until they expired. Subject liveness now resolves user-then-client and fails closed when the subject is neither, so revoking a compromised machine identity takes effect immediately. Tokens belonging to active service accounts and live users are unaffected. Service accounts, `client_credentials` and workload identity are all new in this release, so no previously-released behaviour changes. + - **OIDC/OAuth2 specification compliance for Enterprise IdP integration**: `/authorize` now returns RFC 6749 error codes (invalid_request, unauthorized_client, unsupported_response_type) instead of freeform strings; errors after `redirect_uri` validation redirect to the RP per spec instead of returning JSON. ID tokens now include `auth_time` claim on all issuance paths (OIDC Core §2 requirement for `max_age`). Discovery endpoint advertises `"none"` in `token_endpoint_auth_methods_supported` for PKCE-only public clients. `token_type` normalized to `"Bearer"` (capitalized). In-memory state store enforced with 10-minute TTL; DB state store enforces 600-second read-time TTL. `Cache-Control` caching added to discovery endpoint ([#604](https://github.com/authorizerdev/authorizer/pull/604)). - **RFC-compliant PKCE and redirect_uri security hardening**: S256 `code_challenge` now tolerates base64url padding (Auth0 compatibility). `client_secret` validation enforced whenever provided, even when PKCE is used (prevents secret bypass). `code_verifier` rejected when no `code_challenge` was registered (prevents PKCE bypass). `redirect_uri` URL-encoded in state to prevent `@@`-delimiter injection. `/oauth/token` now validates `redirect_uri` matches the `/authorize` registration (RFC 6749 §4.1.3). Authorize state removal is synchronous (prevents code reuse). Constant-time `redirect_uri` comparison ([#603](https://github.com/authorizerdev/authorizer/pull/603)). - **Introspection authentication & backchannel SSRF hardening**: `/oauth/introspect` now requires `client_secret` when configured (previously omitting secret bypassed auth entirely). Timing-safe `crypto/subtle.ConstantTimeCompare` used for all secret validation. Backchannel logout SSRF fixed by routing through `SafeHTTPClient` (upfront DNS, IP pinning, rejects private/loopback). Session rollover goroutine errors now logged instead of silently discarded ([#606](https://github.com/authorizerdev/authorizer/pull/606)). diff --git a/internal/integration_tests/machine_token_liveness_test.go b/internal/integration_tests/machine_token_liveness_test.go new file mode 100644 index 000000000..cb3b4f969 --- /dev/null +++ b/internal/integration_tests/machine_token_liveness_test.go @@ -0,0 +1,109 @@ +package integration_tests + +import ( + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// TestDeactivatingAServiceAccountStopsItsLiveTokens pins the revocation control +// for machine identities on Authorizer's OWN surfaces — GraphQL, gRPC and REST, +// all of which resolve the caller through ValidateAccessToken. +// +// The check used to resolve a token's subject as a USER only and treat "not +// found" as "not revoked". A client_credentials token's `sub` is the service +// account's row id (schemas.Client.ID), never a user id, so the lookup missed +// every time and reported the caller live. Deactivating a service account +// therefore blocked new token issuance and did nothing whatsoever to the tokens +// already outstanding: an operator revoking a compromised machine identity got a +// success response and a credential that kept working until it expired. +// +// This is the test that would have caught it. It asserts at +// GetUserIDFromSessionOrAccessToken as well as at ValidateAccessToken, because +// the former is what the gRPC interceptor and the GraphQL resolvers actually +// call, and it falls back to the delegated validator when the first-party check +// fails — a boundary that has to hold at the entry point, not one layer below it. +func TestDeactivatingAServiceAccountStopsItsLiveTokens(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + client, err := ts.StorageProvider.AddClient(ctx, &schemas.Client{ + ClientID: "svc-" + uuid.NewString(), + Kind: constants.ClientKindServiceAccount, + Name: "worker", + AllowedScopes: "openid", + IsActive: true, + }) + require.NoError(t, err) + + bearerContext := func(tok string) *gin.Context { return bearerGinContext(t, ts, tok) } + + t.Run("an active service account's token is accepted", func(t *testing.T) { + tok := mintMachineAccessToken(t, ts, client.ID, "") + claims, vErr := ts.TokenProvider.ValidateAccessToken(bearerContext(tok), tok) + require.NoError(t, vErr, "an active service account must keep working — this is the guard against fixing the bug by breaking the feature") + assert.Equal(t, client.ID, claims["sub"]) + + data, rErr := ts.TokenProvider.GetUserIDFromSessionOrAccessToken(bearerContext(tok)) + require.NoError(t, rErr) + assert.Equal(t, client.ID, data.UserID) + }) + + t.Run("deactivating the account rejects tokens already issued", func(t *testing.T) { + tok := mintMachineAccessToken(t, ts, client.ID, "") + + client.IsActive = false + _, uErr := ts.StorageProvider.UpdateClient(ctx, client) + require.NoError(t, uErr) + + _, vErr := ts.TokenProvider.ValidateAccessToken(bearerContext(tok), tok) + require.Error(t, vErr, "deactivation must stop live tokens, not merely block new issuance") + + _, rErr := ts.TokenProvider.GetUserIDFromSessionOrAccessToken(bearerContext(tok)) + require.Error(t, rErr, "the entry point gRPC and GraphQL actually call must reject it too") + }) +} + +// TestSubjectLivenessFailsClosedForAnUnknownSubject covers the second half of the +// same change: a subject that resolves to neither a user nor a client. +// +// The old rule failed OPEN here — "no user row, so nothing says they are revoked". +// It matters as a fallback rather than as a live hole: DeleteUser purges the +// caller's sessions, but does so through asyncutil.Go on a best-effort basis +// (admin_users.go), so a failed or racing purge left a token whose subject no +// longer exists in any table still authenticating. +func TestSubjectLivenessFailsClosedForAnUnknownSubject(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + user, err := ts.StorageProvider.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("liveness_" + uuid.NewString() + "@authorizer.dev"), + SignupMethods: constants.AuthRecipeMethodBasicAuth, + Roles: "user", + }) + require.NoError(t, err) + + tok := mintStatefulAccessToken(t, ts, user, "") + gc := bearerGinContext(t, ts, tok) + + // Sanity: the token works while the subject exists, so the rejection below is + // about the deletion and not about some unrelated defect in the fixture. + _, vErr := ts.TokenProvider.ValidateAccessToken(gc, tok) + require.NoError(t, vErr) + + // Delete the row directly, leaving the session entry behind — the state a + // failed async purge produces. + require.NoError(t, ts.StorageProvider.DeleteUser(ctx, user)) + + _, vErr = ts.TokenProvider.ValidateAccessToken(gc, tok) + require.Error(t, vErr, "a subject that resolves to neither a user nor a client must not authenticate") +} diff --git a/internal/integration_tests/mcp_access_token_test.go b/internal/integration_tests/mcp_access_token_test.go index b82eb2348..a67f69bd7 100644 --- a/internal/integration_tests/mcp_access_token_test.go +++ b/internal/integration_tests/mcp_access_token_test.go @@ -2,7 +2,6 @@ package integration_tests import ( "net/http" - "net/http/httptest" "testing" "time" @@ -17,6 +16,23 @@ import ( "github.com/authorizerdev/authorizer/internal/token" ) +// bearerGinContext builds a request carrying `tok` as a bearer token, addressed +// to the test server's REAL host. +// +// The host is not incidental. ValidateJWTClaims compares the token's `iss` +// against parsers.GetHost(gc), and httptest.NewRequest defaults the host to +// "example.com" — so a context built that way rejects every token this suite +// mints with an issuer mismatch. A negative assertion would then pass for a +// reason unrelated to the rule under test, and would keep passing after that +// rule was removed. +func bearerGinContext(t *testing.T, ts *testSetup, tok string) *gin.Context { + t.Helper() + req, err := http.NewRequest(http.MethodPost, testAuthorizerHost(ts)+"/graphql", nil) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+tok) + return &gin.Context{Request: req} +} + // mintStatefulAccessToken issues an access token bound to `resource` (RFC 8707) // and registers it in the memory store exactly as /oauth/token does, so it is a // genuine first-party token and not a hand-rolled JWT. Passing resource="" is @@ -128,9 +144,7 @@ func TestMCPAccessTokenAudienceBoundary(t *testing.T) { // asserting only the inner call would let a future relaxation of either // delegated rule reopen MCP-token-authenticates-GraphQL with this test // still green — the exact failure this test exists to prevent. - req := httptest.NewRequest(http.MethodPost, "/graphql", nil) - req.Header.Set("Authorization", "Bearer "+tok) - _, vErr = ts.TokenProvider.GetUserIDFromSessionOrAccessToken(&gin.Context{Request: req}) + _, vErr = ts.TokenProvider.GetUserIDFromSessionOrAccessToken(bearerGinContext(t, ts, tok)) require.Error(t, vErr, "an MCP-bound token must not resolve to an identity on the shared entry point") }) diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index a6d50bef8..8aad76d25 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -460,7 +460,7 @@ func (p *provider) GetAccessToken(gc *gin.Context) (string, error) { // Function to validate access token for authorizer apis (profile, update_profile) func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map[string]interface{}, error) { - return p.validateStatefulAccessToken(gc, accessToken, p.firstPartyAudienceOK, p.userIsNotRevoked) + return p.validateStatefulAccessToken(gc, accessToken, p.firstPartyAudienceOK) } // firstPartyAudienceOK is the audience rule for Authorizer's OWN protected @@ -493,19 +493,6 @@ func (p *provider) firstPartyAudienceOK(aud string) error { return nil } -// userIsNotRevoked is the subject-liveness rule for first-party surfaces: -// unchanged from the behaviour that shipped before the decision core was -// extracted. It resolves the subject as a USER only, so a machine token's -// service-account subject is not checked here — see subjectIsLive for the -// stricter rule the MCP and delegated surfaces use. -func (p *provider) userIsNotRevoked(gc *gin.Context, subject string) bool { - if p.userIsRevoked(gc, subject) { - p.dependencies.Log.Debug().Str("user_id", subject).Msg("access token rejected: user revoked") - return false - } - return true -} - // validateStatefulAccessToken is the single decision core behind every // STATEFUL access-token check: signature and expiry, a live memory-store // session entry whose stored digest matches the presented token, subject @@ -516,24 +503,19 @@ func (p *provider) userIsNotRevoked(gc *gin.Context, subject string) bool { // the token endpoint registers machine tokens in the memory store exactly as // human ones, so the session lookup below is uniform. // -// Exactly two checks vary per surface, and they are the two parameters: -// -// - audienceOK decides which `aud` values that surface accepts. This is what -// makes RFC 8707 audience binding real: a token is accepted only where its -// audience says it belongs. -// - subjectLive decides how the subject's liveness is confirmed. First-party -// surfaces resolve the subject as a user; surfaces whose callers are -// routinely service accounts resolve user-then-client and fail closed. +// Exactly ONE check varies per surface, and it is the parameter: audienceOK +// decides which `aud` values that surface accepts. This is what makes RFC 8707 +// audience binding real — a token is accepted only where its audience says it +// belongs. // -// Everything else is identical on purpose. A new surface adds a policy pair -// here rather than a second copy of this function, so a fix to the session -// digest comparison or the claims check cannot land on one surface and miss -// another. +// Everything else is identical on purpose. A new surface adds an audience policy +// here rather than a second copy of this function, so a fix to the session digest +// comparison, the subject-liveness rule or the claims check cannot land on one +// surface and miss another. func (p *provider) validateStatefulAccessToken( gc *gin.Context, accessToken string, audienceOK func(aud string) error, - subjectLive func(gc *gin.Context, subject string) bool, ) (map[string]interface{}, error) { res := make(map[string]interface{}) @@ -571,7 +553,17 @@ func (p *provider) validateStatefulAccessToken( return res, fmt.Errorf(`unauthorized`) } - if !subjectLive(gc, userID) { + // Subject liveness. Resolves the subject as a user, then as a client, and + // fails closed if it is neither — see subjectIsLive. + // + // This used to look the subject up as a USER only and treat "not found" as + // "not revoked". A client_credentials token's `sub` is a service account's + // row id, never a user, so that lookup missed every time and reported the + // caller live: deactivating a service account did nothing to the tokens it + // had already been issued. An operator revoking a compromised machine + // identity got a success response and a credential that kept working until + // it expired. + if !p.subjectIsLive(gc, userID) { return res, fmt.Errorf(`unauthorized: subject is not active`) } diff --git a/internal/token/delegated_access_token.go b/internal/token/delegated_access_token.go index 9f3264651..0ba1d777e 100644 --- a/internal/token/delegated_access_token.go +++ b/internal/token/delegated_access_token.go @@ -190,17 +190,19 @@ func sameAudience(aud, hostname string) bool { } // subjectIsLive reports whether the subject a token was minted for is still -// active. Used by the surfaces whose callers are routinely service accounts -// rather than humans: RFC 8693 delegation (here) and MCP -// (ValidateMCPAccessToken). +// active. It is the subject-liveness rule for EVERY stateful token check: +// validateStatefulAccessToken (so GraphQL, gRPC, REST and MCP alike) and RFC +// 8693 delegation. // -// The subject is NOT always a user. RFC 8693 token exchange also accepts a +// The subject is NOT always a user. A client_credentials token's `sub` is the +// service account's surrogate id, and RFC 8693 token exchange also accepts a // service account as the subject, which is how a multi-hop chain is expressed -// (agent A delegates to agent B), and a client_credentials token's `sub` is the -// service account's surrogate id. userIsRevoked only ever looked the subject up -// as a USER, so for a service-account subject it found nothing, reported "not -// revoked", and the token kept working for its full lifetime after the service -// account had been deactivated — deactivation did not stop the chain it seeded. +// (agent A delegates to agent B). userIsRevoked only ever looked the subject up +// as a USER and treated "not found" as "not revoked", so for a service-account +// subject it reported the caller live and the token kept working for its full +// lifetime after the account had been deactivated — deactivation stopped new +// tokens being issued but not the ones already out, and stopped nothing at all +// in a chain it had seeded. // // Resolution order mirrors how the token endpoint validates the subject at // mint time (see handleTokenExchangeGrant): try user, then client. diff --git a/internal/token/mcp_access_token.go b/internal/token/mcp_access_token.go index 6fc93b90b..bb9bbfdc4 100644 --- a/internal/token/mcp_access_token.go +++ b/internal/token/mcp_access_token.go @@ -42,12 +42,10 @@ import ( // # Everything else is the ordinary stateful check // // Signature, expiry, a live memory-store session entry whose stored digest -// matches the presented token, issuer/claims and token type all come from -// validateStatefulAccessToken — the same core ValidateAccessToken uses. MCP is -// NOT a weaker path: it differs from the first-party check in the audience rule -// (stricter) and in resolving subject liveness as user-then-client (also -// stricter — see subjectIsLive; a deactivated service account is rejected here -// even though the first-party path would still accept its token). +// matches the presented token, subject liveness, issuer/claims and token type +// all come from validateStatefulAccessToken — the same core ValidateAccessToken +// uses. MCP is NOT a weaker path: it differs from the first-party check in +// exactly one rule, the audience, and there it is the stricter of the two. // // # What is deliberately NOT accepted // @@ -65,15 +63,12 @@ func (p *provider) ValidateMCPAccessToken(gc *gin.Context, accessToken string, r // subtle for an auth path: say no explicitly. return map[string]interface{}{}, fmt.Errorf(`unauthorized: no mcp resource configured`) } - return p.validateStatefulAccessToken(gc, accessToken, - func(aud string) error { - if !sameAudience(aud, resource) { - p.dependencies.Log.Debug().Str("aud", aud).Str("expected", resource). - Msg("access token rejected at mcp: audience names a different resource") - return fmt.Errorf(`unauthorized: token audience is not this mcp server`) - } - return nil - }, - p.subjectIsLive, - ) + return p.validateStatefulAccessToken(gc, accessToken, func(aud string) error { + if !sameAudience(aud, resource) { + p.dependencies.Log.Debug().Str("aud", aud).Str("expected", resource). + Msg("access token rejected at mcp: audience names a different resource") + return fmt.Errorf(`unauthorized: token audience is not this mcp server`) + } + return nil + }) } From 969d4125b9838f0512ff052e357500caf4bbc485 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 18:04:22 +0530 Subject: [PATCH 03/11] security: make revocation survive a storage outage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the two stacked commits found the liveness change had swapped one failure mode for a worse one, and that fixing it properly exposed a real gap underneath. 1. subjectIsLive treated a storage ERROR as "subject not active". On the delegated path that was a fine trade; as the shared rule for every stateful token it meant a database failover 401s every authenticated request on GraphQL, gRPC and REST at once — and a 401 tells the SDKs the session expired, so a five-second blip becomes a fleet-wide forced logout reported as a bad credential. The code it replaced documented the opposite as deliberate: "fail open on DB errors so a transient storage blip can't take down every authenticated request". AGENTS.md names this exact conflation as one of two classes of production bug already suffered here. subjectLiveness now reports live AND known separately, using storage.IsNotFound to tell absence from failure. A confirmed-dead subject is still rejected everywhere; an unanswerable lookup is tolerated by first-party traffic and still fails closed on the stateless delegated path. 2. Tolerating an unknown is only safe when something else is the primary revocation mechanism. For users that is the memory-store session delete — a different system that survives a database outage. Service accounts had no such mechanism: UpdateClient set IsActive=false and returned, so the DB flag was the entire revocation story rather than defense-in-depth, and an outage would have re-opened it. Deactivation now purges the account's sessions, the same way revoking a user does. Deactivation is instant instead of dependent on a lookup being reachable at request time. 3. ValidateBrowserSession kept the old user-only fail-open check, so a deleted user whose async session purge failed was rejected by bearer token but still authenticated by cookie. Both credential types now use one rule. 4. resolverIsSoleAuthority skipped the interceptor's super-admin check, but service.requireSuperAdmin re-derives super-admin from meta.Request on its own — so the guard moved the check one layer down rather than removing it. A resolver-governed server now refuses AuthorizerAdminService outright. Costs nothing: no admin RPC is mcp_tool-exposed. Each fix is pinned by a test verified to fail without it. Refs authorizer-docs specs/2026-08-10-mcp-http-transport.md --- internal/grpcsrv/interceptors/auth.go | 49 +++++++---- internal/grpcsrv/interceptors/auth_test.go | 51 +++++++++++- .../machine_token_liveness_test.go | 70 ++++++++++++++++ internal/service/admin_clients.go | 27 +++++++ internal/token/auth_token.go | 48 +++++------ internal/token/delegated_access_token.go | 81 ++++++++++++++++--- 6 files changed, 271 insertions(+), 55 deletions(-) diff --git a/internal/grpcsrv/interceptors/auth.go b/internal/grpcsrv/interceptors/auth.go index 16fa8eae6..c1d42e25e 100644 --- a/internal/grpcsrv/interceptors/auth.go +++ b/internal/grpcsrv/interceptors/auth.go @@ -63,20 +63,23 @@ type TokenResolver func(gc *gin.Context) (*token.SessionOrAccessTokenData, error // tp.GetUserIDFromSessionOrAccessToken and the cookie-based paths below stay // active — the behaviour every TCP-listening server uses. // -// A non-nil resolve is the SOLE authority for that server. It replaces both -// identity-resolution sites (the admin fallback and the public path) AND -// disables the two paths that authenticate without consulting a resolver at all: -// the super-admin check (an admin cookie or the x-authorizer-admin-secret -// header) and the Session RPC's cookie-only branch. +// A non-nil resolve is the SOLE authority for that server. It replaces the +// identity-resolution site on the public path, refuses the AuthorizerAdminService +// outright, and disables the Session RPC's cookie-only branch. // -// Disabling those is the point, not a side effect. A surface that declares its +// Narrowing that far is the point, not a side effect. A surface that declares its // own token rule — MCP, whose rule is "the audience must name this MCP server" — -// must not be reachable with a credential that rule never saw. Leaving them -// active meant the boundary held only because no cookie-authenticated method -// happened to be mcp_tool-exposed, and transport.MetaFromGRPC reconstructs -// cookies from gRPC metadata, so a bridge that forwarded headers wholesale would -// have made a browser session authenticate a tool call on an internet-facing, -// CSRF-exempt endpoint. +// must not be reachable with a credential that rule never saw. Leaving the other +// paths active meant the boundary held only because no cookie-authenticated +// method happened to be mcp_tool-exposed, and transport.MetaFromGRPC +// reconstructs cookies from gRPC metadata, so a bridge that forwarded headers +// wholesale would have made a browser session authenticate a tool call on an +// internet-facing, CSRF-exempt endpoint. +// +// The admin service is refused wholesale rather than merely skipping its +// super-admin check, because service.requireSuperAdmin re-derives super-admin +// from meta.Request on its own — skipping the check here would move it one layer +// down, not remove it. func Auth(tp token.Provider, log *zerolog.Logger, resolve TokenResolver) grpc.UnaryServerInterceptor { resolverIsSoleAuthority := resolve != nil if resolve == nil { @@ -117,11 +120,25 @@ func Auth(tp token.Provider, log *zerolog.Logger, resolve TokenResolver) grpc.Un gc := &gin.Context{Request: meta.Request} if serviceName == adminServiceName { + // A resolver-governed surface does not serve the admin API at all. + // + // Skipping the IsSuperAdmin check here is NOT enough on its own: + // service.requireSuperAdmin re-derives super-admin from meta.Request + // (admin_provider.go), reading the admin cookie or the + // x-authorizer-admin-secret header that transport.MetaFromGRPC + // reconstructs from gRPC metadata. Disabling the check at this layer + // would only move it one layer down, so a caller holding a valid + // MCP-audience token plus an admin credential would still reach + // platform-wide operations on an internet-facing, CSRF-exempt + // surface. Refusing the whole service is the only version of this + // guard that actually holds, and it costs nothing: no admin RPC is + // mcp_tool-exposed, so nothing legitimate is being turned off. + if resolverIsSoleAuthority { + return nil, status.Error(codes.Unauthenticated, "unauthorized") + } // Platform super-admin: unchanged, and still the only identity that - // reaches the platform-wide operations — except on a server whose - // resolver is the sole authority, where an admin cookie or admin - // secret is not a credential this surface accepts at all. - if !resolverIsSoleAuthority && tp.IsSuperAdmin(gc) { + // reaches the platform-wide operations. + if tp.IsSuperAdmin(gc) { ctx = authctx.WithPrincipal(ctx, &authctx.Principal{IsSuperAdmin: true}) return handler(ctx, req) } diff --git a/internal/grpcsrv/interceptors/auth_test.go b/internal/grpcsrv/interceptors/auth_test.go index a694f0a99..2a8432a09 100644 --- a/internal/grpcsrv/interceptors/auth_test.go +++ b/internal/grpcsrv/interceptors/auth_test.go @@ -403,12 +403,14 @@ func TestAuth_SessionOnlyAcceptsPublicService(t *testing.T) { // at all, or a token rejected by the surface's own rule could still be accepted // by the default one. func TestAuth_TokenResolverOverrideAppliesToBothSites(t *testing.T) { + // Only the public service: a resolver-governed server refuses the admin + // service outright (see TestAuth_SoleAuthorityRefusesTheAdminService), so + // there is no admin identity-resolution site left to override. cases := []struct { name string method string }{ {"public service", authorizerv1.AuthorizerService_Profile_FullMethodName}, - {"admin service non-super-admin fallback", authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName}, } for _, tc := range cases { @@ -456,3 +458,50 @@ func TestAuth_TokenResolverOverrideAppliesToBothSites(t *testing.T) { }) } } + +// TestAuth_SoleAuthorityRefusesTheAdminService pins why skipping the interceptor's +// super-admin check was not, on its own, enough. +// +// service.requireSuperAdmin re-derives super-admin from meta.Request when the +// context carries no super-admin principal, reading the admin cookie or the +// x-authorizer-admin-secret header that transport.MetaFromGRPC reconstructs from +// gRPC metadata. Skipping the check here would therefore have moved it one layer +// down rather than removing it: a caller holding a valid MCP-audience token plus +// an admin credential would still have reached platform-wide operations on an +// internet-facing, CSRF-exempt surface. Refusing the service is the only version +// of the guard that holds, and it costs nothing — no admin RPC is +// mcp_tool-exposed. +func TestAuth_SoleAuthorityRefusesTheAdminService(t *testing.T) { + // The override would happily authenticate this caller; the service refusal + // must come first, before any identity is resolved at all. + stub := &stubTokenProvider{superAdmin: true, tokenData: &token.SessionOrAccessTokenData{UserID: "u1"}} + resolverCalls := 0 + mw := Auth(stub, nil, func(_ *gin.Context) (*token.SessionOrAccessTokenData, error) { + resolverCalls++ + return &token.SessionOrAccessTokenData{UserID: "mcp-user"}, nil + }) + + called := false + _, err := mw(context.Background(), nil, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), + func(context.Context, any) (any, error) { called = true; return nil, nil }) + + require.Error(t, err) + assert.Equal(t, codes.Unauthenticated, status.Code(err)) + assert.False(t, called, "no admin handler may run on a resolver-governed server") + assert.Zero(t, resolverCalls, "the admin service is refused outright, not authenticated and then rejected") + assert.Zero(t, stub.superAdminChecks) +} + +// TestAuth_DefaultServerStillServesTheAdminService is the inverse guard: the +// refusal above must not touch the TCP-listening server every deployment runs. +func TestAuth_DefaultServerStillServesTheAdminService(t *testing.T) { + stub := &stubTokenProvider{superAdmin: true} + mw := Auth(stub, nil, nil) + + called := false + _, err := mw(context.Background(), nil, info(authorizerv1.AuthorizerAdminService_OrgMembers_FullMethodName), + func(context.Context, any) (any, error) { called = true; return nil, nil }) + + require.NoError(t, err) + assert.True(t, called) +} diff --git a/internal/integration_tests/machine_token_liveness_test.go b/internal/integration_tests/machine_token_liveness_test.go index cb3b4f969..4401c48f2 100644 --- a/internal/integration_tests/machine_token_liveness_test.go +++ b/internal/integration_tests/machine_token_liveness_test.go @@ -2,14 +2,18 @@ package integration_tests import ( "testing" + "time" "github.com/gin-gonic/gin" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/authorizerdev/authorizer/internal/authctx" "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/service" "github.com/authorizerdev/authorizer/internal/storage/schemas" ) @@ -107,3 +111,69 @@ func TestSubjectLivenessFailsClosedForAnUnknownSubject(t *testing.T) { _, vErr = ts.TokenProvider.ValidateAccessToken(gc, tok) require.Error(t, vErr, "a subject that resolves to neither a user nor a client must not authenticate") } + +// adminSvc exposes the admin half of the service provider. The concrete value +// implements both interfaces; service.Provider only declares the public one. +func adminSvc(t *testing.T, ts *testSetup) service.AdminProvider { + t.Helper() + admin, ok := ts.ServiceProvider.(service.AdminProvider) + require.True(t, ok, "the service provider must also implement AdminProvider") + return admin +} + +// TestDeactivationPurgesServiceAccountSessions pins the PRIMARY revocation +// mechanism for machine identities, and it exists because of a gap the liveness +// work exposed rather than created. +// +// Token validation's subject-liveness check is deliberately defense-in-depth: it +// tolerates an unanswerable lookup, because it is the shared core for GraphQL, +// gRPC and REST and turning a database outage into "subject not active" would +// 401 every authenticated request at once. That tolerance is only safe when +// something else is the primary revocation mechanism — for users it is the +// memory-store session delete, which lives in a different system and survives a +// database outage. +// +// Service accounts had no such mechanism. UpdateClient set IsActive=false and +// returned; nothing touched the memory store. So the DB flag was not +// defense-in-depth for machine tokens, it was the entire revocation story, and a +// storage outage would have re-opened it. +// +// Asserting on the session store directly, not on validation: the point is that +// the credential is destroyed at its source, so revocation no longer depends on +// a lookup being reachable at request time. +func TestDeactivationPurgesServiceAccountSessions(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + client, err := ts.StorageProvider.AddClient(ctx, &schemas.Client{ + ClientID: "svc-" + uuid.NewString(), + Kind: constants.ClientKindServiceAccount, + Name: "purge-me", + AllowedScopes: "openid", + IsActive: true, + }) + require.NoError(t, err) + + nonce := uuid.NewString() + sessionKey := constants.AuthRecipeMethodServiceAccount + ":" + client.ID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionKey, constants.TokenTypeAccessToken+"_"+nonce, "live-machine-token", time.Now().Add(time.Hour).Unix())) + + _, gErr := ts.MemoryStoreProvider.GetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+nonce) + require.NoError(t, gErr, "fixture must start with a live session, or the assertion below proves nothing") + + active := false + // Super-admin via the principal, not via an admin cookie: requireSuperAdmin + // takes the principal branch first, and its fallback dereferences + // meta.Request without a nil guard. + adminCtx := authctx.WithPrincipal(ctx, &authctx.Principal{IsSuperAdmin: true}) + _, _, uErr := adminSvc(t, ts).UpdateClient(adminCtx, service.RequestMetadata{}, &model.UpdateClientRequest{ + ID: client.ID, + IsActive: &active, + }) + require.NoError(t, uErr) + + _, gErr = ts.MemoryStoreProvider.GetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+nonce) + require.Error(t, gErr, "deactivating a service account must destroy its live sessions, not merely set a flag a later lookup might not reach") +} diff --git a/internal/service/admin_clients.go b/internal/service/admin_clients.go index 967b49c4d..20e649b52 100644 --- a/internal/service/admin_clients.go +++ b/internal/service/admin_clients.go @@ -157,6 +157,7 @@ func (p *provider) UpdateClient(ctx context.Context, meta RequestMetadata, param } sa.AllowedScopes = scopes } + deactivating := params.IsActive != nil && !*params.IsActive && sa.IsActive if params.IsActive != nil { sa.IsActive = *params.IsActive } @@ -167,6 +168,32 @@ func (p *provider) UpdateClient(ctx context.Context, meta RequestMetadata, param return nil, nil, err } + // Deactivation must take effect on tokens ALREADY issued, not just block new + // ones. Machine tokens are registered in the memory store under + // "service_account:", exactly like a user's session, so the same + // purge that revokes a user revokes a service account. + // + // Without this the DB IsActive flag was the only thing standing between a + // deactivated service account and its live tokens — token validation's + // subject-liveness check. That check is deliberately defense-in-depth for + // users, because the session-store delete is the primary mechanism and it + // lives in a different system that survives a database outage; for service + // accounts there was no primary mechanism at all, so the secondary one was + // carrying the whole revocation story. Purging here restores the same + // primary/secondary structure users have, and makes deactivation instant + // rather than dependent on a lookup that may be unavailable. + // + // Synchronous and best-effort, matching DeleteUser's FGA tuple purge: a + // caller must not observe a successful deactivation while the account still + // holds live sessions, but a memory-store failure must not report an update + // that did happen as failed. + if deactivating { + if err := p.MemoryStoreProvider.DeleteAllUserSessions(updated.ID); err != nil { + log.Warn().Err(err).Str("client_id", updated.ID). + Msg("failed to purge sessions for deactivated service account; its live tokens remain valid until they expire") + } + } + p.AuditProvider.LogEvent(audit.Event{ Action: constants.AuditClientUpdatedEvent, Protocol: meta.Protocol, ActorType: constants.AuditActorTypeAdmin, diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index 8aad76d25..d70c26a5e 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -553,8 +553,8 @@ func (p *provider) validateStatefulAccessToken( return res, fmt.Errorf(`unauthorized`) } - // Subject liveness. Resolves the subject as a user, then as a client, and - // fails closed if it is neither — see subjectIsLive. + // Subject liveness. Rejects only a CONFIRMED dead subject — absent, revoked, + // or a deactivated service account. // // This used to look the subject up as a USER only and treat "not found" as // "not revoked". A client_credentials token's `sub` is a service account's @@ -563,7 +563,14 @@ func (p *provider) validateStatefulAccessToken( // had already been issued. An operator revoking a compromised machine // identity got a success response and a credential that kept working until // it expired. - if !p.subjectIsLive(gc, userID) { + // + // `known` is deliberately honoured rather than ignored. An unreachable + // database must not become "subject not active" here: this is the shared core + // for GraphQL, gRPC and REST, so that would 401 every authenticated request + // at once, and a 401 tells the SDKs the session expired. A storage outage has + // to surface as a 500 from the handler that actually needs the data, not as a + // fleet-wide forced logout. See subjectLiveness. + if live, known := p.subjectLiveness(gc, userID); known && !live { return res, fmt.Errorf(`unauthorized: subject is not active`) } @@ -698,35 +705,24 @@ func (p *provider) ValidateBrowserSession(gc *gin.Context, encryptedSession stri return nil, fmt.Errorf(`unauthorized: token expired`) } - if p.userIsRevoked(gc, res.Subject) { - p.dependencies.Log.Debug().Str("user_id", res.Subject).Msg("browser session rejected: user revoked") + // Same subject-liveness rule the bearer path uses, for the same reason: an + // account that has been deleted or revoked must stop authenticating on BOTH + // credential types. Leaving the cookie path on the old user-only, fail-open + // check meant a deleted user whose session purge failed or raced (DeleteUser + // does it through asyncutil.Go, best effort) kept browsing with a cookie + // while the same account's bearer token was correctly rejected. + // + // A session subject is always a user, so the client lookup inside + // subjectLiveness is only ever reached when the user row is confirmed absent + // — which is precisely the case this now catches. + if live, known := p.subjectLiveness(gc, res.Subject); known && !live { + p.dependencies.Log.Debug().Str("user_id", res.Subject).Msg("browser session rejected: subject is not active") return nil, fmt.Errorf(`unauthorized: user revoked`) } return &res, nil } -// userIsRevoked re-checks the DB RevokedTimestamp for a user resolved from an -// already-issued access token or browser session. This is defense-in-depth: -// the session-store deletion SCIM deactivate() (and account deactivation) -// perform is the primary revocation mechanism for these stateful tokens, but -// if that delete was missed or failed on this instance, a held token would -// otherwise keep authenticating requests until its natural exp. Mirrors the -// same demote-only pattern used by introspect.go/token.go/login.go: a lookup -// failure never blocks a request that otherwise validated (fail open on DB -// errors so a transient storage blip can't take down every authenticated -// request), only a confirmed RevokedTimestamp does. -func (p *provider) userIsRevoked(gc *gin.Context, userID string) bool { - if p.dependencies.StorageProvider == nil || userID == "" { - return false - } - user, err := p.dependencies.StorageProvider.GetUserByID(gc, userID) - if err != nil || user == nil { - return false - } - return user.RevokedTimestamp != nil -} - // CreateIDToken util to create the OIDC ID token JWT, based on user // information, roles config and CUSTOM_ACCESS_TOKEN_SCRIPT. // See the in-function block comment for the at_hash / c_hash / nonce diff --git a/internal/token/delegated_access_token.go b/internal/token/delegated_access_token.go index 0ba1d777e..bbe6d67a1 100644 --- a/internal/token/delegated_access_token.go +++ b/internal/token/delegated_access_token.go @@ -9,6 +9,7 @@ import ( "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/parsers" + "github.com/authorizerdev/authorizer/internal/storage" "github.com/authorizerdev/authorizer/internal/storage/schemas" ) @@ -207,33 +208,89 @@ func sameAudience(aud, hostname string) bool { // Resolution order mirrors how the token endpoint validates the subject at // mint time (see handleTokenExchangeGrant): try user, then client. // -// Fails CLOSED when the subject resolves to neither. A subject we cannot -// confirm is live must not authenticate — the same rule the exchange applies -// before it will seed a delegation at all. +// Fails CLOSED when the subject resolves to neither, and also when the store +// could not answer: a delegated token is stateless and short-lived, so a subject +// we cannot confirm is live must not authenticate — the same rule the exchange +// applies before it will seed a delegation at all. First-party callers make the +// opposite choice about an unanswerable lookup; see subjectLiveness. func (p *provider) subjectIsLive(gc *gin.Context, subject string) bool { - if p.dependencies.StorageProvider == nil || subject == "" { - return false + live, _ := p.subjectLiveness(gc, subject) + return live +} + +// subjectLiveness reports whether a token's subject is still active AND whether +// that could be determined at all: +// +// live=true — confirmed active +// live=false, known=true — confirmed absent, revoked, or deactivated +// known=false — the store could not answer (outage, timeout) +// +// The three-way answer exists because "the row is not there" and "the query +// failed" are different outcomes, and collapsing them is a mistake this codebase +// has already paid for — see AGENTS.md's not-found contract, and the note this +// function inherits from userIsRevoked, which it replaced: a lookup failure must +// never block a request that otherwise validated, "so a transient storage blip +// can't take down every authenticated request". +// +// That matters far more now than when only delegation consulted it. This is the +// liveness rule for every stateful access token and browser session, so treating +// an unreachable database as "subject not active" would 401 every authenticated +// request on GraphQL, gRPC and REST at once — and a 401 tells the SDKs the +// session expired, so a five-second failover becomes a fleet-wide forced logout +// reported to operators as a bad credential rather than an outage. +// +// So callers decide what an unknown means. First-party traffic tolerates it and +// lets the handler's own storage call surface the outage as a 500; the stateless +// delegated path fails closed (subjectIsLive). What neither tolerates is a +// CONFIRMED dead subject, which is the whole point: the subject is not always a +// user — a client_credentials token's `sub` is the service account's surrogate +// id, and RFC 8693 exchange accepts a service account as the subject too (agent A +// delegating to agent B). Resolving as a user only, and reading "no such user" as +// "not revoked", meant deactivating a service account did nothing to the tokens +// already issued to it. +// +// Resolution order mirrors how the token endpoint validates the subject at mint +// time (see handleTokenExchangeGrant): try user, then client. +func (p *provider) subjectLiveness(gc *gin.Context, subject string) (live, known bool) { + if subject == "" { + return false, true + } + if p.dependencies.StorageProvider == nil { + // No store to consult is not evidence of anything. + return false, false } - if user, err := p.dependencies.StorageProvider.GetUserByID(gc, subject); err == nil && user != nil { + user, err := p.dependencies.StorageProvider.GetUserByID(gc, subject) + switch { + case err == nil && user != nil: if user.RevokedTimestamp != nil { p.dependencies.Log.Debug().Str("subject", subject). Msg("token rejected: subject user is revoked") - return false + return false, true } - return true + return true, true + case !storage.IsNotFound(err): + p.dependencies.Log.Debug().Err(err).Str("subject", subject). + Msg("subject liveness undetermined: user lookup failed") + return false, false } - if client, err := p.dependencies.StorageProvider.GetClientByID(gc, subject); err == nil && client != nil { + client, err := p.dependencies.StorageProvider.GetClientByID(gc, subject) + switch { + case err == nil && client != nil: if !client.IsActive { p.dependencies.Log.Debug().Str("subject", subject). Msg("token rejected: subject service account is deactivated") - return false + return false, true } - return true + return true, true + case !storage.IsNotFound(err): + p.dependencies.Log.Debug().Err(err).Str("subject", subject). + Msg("subject liveness undetermined: client lookup failed") + return false, false } p.dependencies.Log.Debug().Str("subject", subject). Msg("token rejected: subject resolves to neither an active user nor an active client") - return false + return false, true } From a986b81df1e62c00831cc51823349901aa781324 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 18:24:40 +0530 Subject: [PATCH 04/11] security(oauth): keep the RFC 8707 resource across refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An access token minted with a `resource` indicator carries it as `aud`, and that audience is the whole mechanism stopping a token issued for one resource server being replayed at another. The binding survived exactly one token. The token endpoint's local `resource` was populated only inside the authorization_code branch, so on the refresh grant it was empty, accessTokenAudience fell back to the client id, and the rotated token came back UNBOUND — valid at Authorizer's own API, which is what the restriction existed to prevent. A user scoping a token to one resource server got that scope widened by the act of refreshing. It failed in the direction that hides it: the first token is correct, so every manual test and demo passes. Only the rotated one is wrong, and only after the access token lifetime has elapsed. The resource now travels on the refresh token, which is the only thing that survives between the authorization request and the rotation — the code is long gone by then. It joins roles, scope, login_method, auth_time, client_id and family_id, which already round-trip the same way, and is a reserved claim so CustomAccessTokenScript cannot rebind a token to a resource server the user never authorized. A refresh naming a different resource is rejected with invalid_target: RFC 8707 §2.2 permits a refresh to restrict the resource, never to switch it, and silently ignoring a mismatch would hand back a token for a resource the caller did not ask for. Same shape as the enforcement the authorization_code branch already applies. Grants that never used a resource indicator are untouched: no claim is emitted, and the rotated token keeps the client id as its audience. Pinned by a test for that case too, since the risk of a fix like this is stamping an empty resource. This is a prerequisite for serving MCP over HTTP — MCP tokens are audience-bound by specification and clients refresh proactively before expiry, so a connection would have worked for one token lifetime and then failed permanently — but the bug is independent of MCP and worth fixing on its own. Refs authorizer-docs specs/2026-08-10-mcp-http-transport.md §8 --- CHANGELOG.md | 2 + internal/http_handlers/token.go | 36 ++++ .../refresh_resource_binding_test.go | 184 ++++++++++++++++++ internal/token/auth_token.go | 25 +++ 4 files changed, 247 insertions(+) create mode 100644 internal/integration_tests/refresh_resource_binding_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d1a349c8..40a5e7242 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id ### Security +- **RFC 8707 resource binding now survives token refresh.** An access token minted with a `resource` indicator carries that resource as its `aud`, which is what stops it being replayed at a different resource server. That binding previously survived exactly one token: the refresh grant did not carry the resource forward, so the rotated access token fell back to the client id as its audience and came back **unbound** — usable at Authorizer's own API, which is precisely what the restriction exists to prevent. The resource is now stamped on the refresh token (a reserved claim, so `CUSTOM_ACCESS_TOKEN_SCRIPT` cannot forge it) and carried across every rotation. A refresh request that names a *different* resource is rejected with `invalid_target` (RFC 8707 §2.2 permits restricting the resource, never switching it). Grants that never used a resource indicator are unaffected — their tokens keep the client id as the audience and their claim set is unchanged. + - **BEHAVIOUR CHANGE — deactivating a service account now revokes its live access tokens.** Token validation resolved a token's subject as a *user* only and treated "no such user" as "not revoked". A `client_credentials` token's `sub` is the service account's row id, never a user id, so that lookup missed every time and reported the caller live: setting a service account inactive blocked new token issuance but did nothing to tokens already outstanding, which kept working at GraphQL, gRPC and REST until they expired. Subject liveness now resolves user-then-client and fails closed when the subject is neither, so revoking a compromised machine identity takes effect immediately. Tokens belonging to active service accounts and live users are unaffected. Service accounts, `client_credentials` and workload identity are all new in this release, so no previously-released behaviour changes. - **OIDC/OAuth2 specification compliance for Enterprise IdP integration**: `/authorize` now returns RFC 6749 error codes (invalid_request, unauthorized_client, unsupported_response_type) instead of freeform strings; errors after `redirect_uri` validation redirect to the RP per spec instead of returning JSON. ID tokens now include `auth_time` claim on all issuance paths (OIDC Core §2 requirement for `max_age`). Discovery endpoint advertises `"none"` in `token_endpoint_auth_methods_supported` for PKCE-only public clients. `token_type` normalized to `"Bearer"` (capitalized). In-memory state store enforced with 10-minute TTL; DB state store enforces 600-second read-time TTL. `Cache-Control` caching added to discovery endpoint ([#604](https://github.com/authorizerdev/authorizer/pull/604)). diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 68aeb1967..2a3d5f451 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -675,6 +675,42 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { authTime = int64(at) } + // RFC 8707 §2.2: the rotated access token stays bound to the resource + // the original grant named. The claim is stamped on the refresh token + // at mint time (CreateRefreshToken) because by now the authorization + // code that carried the binding is long gone. + // + // Carrying it is not a nicety. Without it `resource` stays empty on + // this branch, accessTokenAudience falls back to the client id, and a + // token the user deliberately scoped to one resource server comes back + // usable at Authorizer's own API — the audience restriction undone by + // the act of refreshing. It also failed silently in the direction that + // hides it: the FIRST token works, only the refreshed one is wrong. + boundResource, _ := claims["resource"].(string) + + // A client that names a resource on refresh must name the one the + // grant was bound to. RFC 8707 §2.2 allows a refresh request to + // restrict the resource, never to switch to a different one, and + // silently ignoring a mismatch would hand back a token for a resource + // the caller did not ask for. Mirrors the same enforcement the + // authorization_code branch applies to the echoed resource. + // + // PostFormArray, not PostForm: a repeated parameter must be rejected + // rather than silently resolved to the first value — same reasoning as + // the authorization_code branch above. + if requestResources := gc.PostFormArray("resource"); len(requestResources) > 0 { + if len(requestResources) != 1 || strings.TrimSpace(requestResources[0]) != boundResource { + metrics.RecordSecurityEvent("refresh_resource_mismatch", "token_endpoint") + log.Warn().Msg("rejected: resource parameter does not match the resource bound to this grant") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_target", + "error_description": "The resource parameter does not match the resource bound to this grant", + }) + return + } + } + resource = boundResource + nonce, ok := claims["nonce"].(string) if !ok || nonce == "" { log.Debug().Msg("Invalid nonce in refresh token") diff --git a/internal/integration_tests/refresh_resource_binding_test.go b/internal/integration_tests/refresh_resource_binding_test.go new file mode 100644 index 000000000..be0be8394 --- /dev/null +++ b/internal/integration_tests/refresh_resource_binding_test.go @@ -0,0 +1,184 @@ +package integration_tests + +import ( + "crypto/sha256" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// s256Challenge is the RFC 7636 S256 code challenge for a verifier. +func s256Challenge(verifier string) string { + sum := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(sum[:]) +} + +// postToken drives POST /oauth/token with form-encoded params and returns the +// decoded body. +func postToken(t *testing.T, router http.Handler, form url.Values) (int, map[string]any) { + t.Helper() + w := httptest.NewRecorder() + req, err := http.NewRequest(http.MethodPost, "/oauth/token", strings.NewReader(form.Encode())) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + router.ServeHTTP(w, req) + + var body map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &body) + return w.Code, body +} + +// TestRefreshPreservesTheResourceBinding pins RFC 8707 §2.2 across rotation. +// +// The resource indicator a client sends to /authorize becomes the access token's +// `aud`, which is what stops a token minted for one resource server being +// replayed at another. That binding used to survive exactly one token: the local +// `resource` in the token endpoint was populated only inside the +// authorization_code branch, so on refresh it was empty, accessTokenAudience fell +// back to the client id, and the rotated token came back UNBOUND — usable at +// Authorizer's own API, which is precisely what the restriction existed to +// prevent. +// +// The failure mode hid itself. The first token is correct, so every manual test +// and every demo passes; only the refreshed one is wrong, and only after the +// access token's lifetime has elapsed. For the MCP surface — whose tokens are +// audience-bound by specification and whose clients refresh proactively before +// expiry — it meant a connection that worked and then failed permanently. +func TestRefreshPreservesTheResourceBinding(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + resource := "https://mcp.example.com/mcp" + router, sessionToken := mcpSession(t, ts, []string{"openid", "offline_access"}) + + verifier := "a-code-verifier-that-is-long-enough-to-be-valid-000000" + challenge := s256Challenge(verifier) + + qs := url.Values{} + qs.Set("response_type", "code") + qs.Set("client_id", cfg.ClientID) + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("state", "st") + qs.Set("response_mode", "query") + qs.Set("scope", "openid offline_access") + qs.Set("code_challenge", challenge) + qs.Set("code_challenge_method", "S256") + qs.Set("resource", resource) + + code := codeFromRedirect(t, doAuthorizeGET(router, qs, sessionToken)) + + exchange := url.Values{} + exchange.Set("grant_type", "authorization_code") + exchange.Set("code", code) + exchange.Set("client_id", cfg.ClientID) + exchange.Set("redirect_uri", "http://localhost:3000/callback") + exchange.Set("code_verifier", verifier) + exchange.Set("resource", resource) + + status, body := postToken(t, router, exchange) + require.Equal(t, http.StatusOK, status, "body: %v", body) + + firstAccess, _ := body["access_token"].(string) + refreshToken, _ := body["refresh_token"].(string) + require.NotEmpty(t, firstAccess) + require.NotEmpty(t, refreshToken, "offline_access must yield a refresh token, or this test proves nothing") + + firstClaims, err := ts.TokenProvider.ParseJWTToken(firstAccess) + require.NoError(t, err) + require.Equal(t, resource, firstClaims["aud"], + "the authorization_code path must bind the audience — the regression under test is about what happens NEXT") + + t.Run("a refresh naming a different resource is refused", func(t *testing.T) { + // Runs FIRST: rejection happens before rotation, so the success case + // below still has a live refresh token. Refresh tokens rotate on use. + // + // RFC 8707 §2.2 lets a refresh restrict the resource, never switch it. + // Silently ignoring the mismatch would hand back a token for a resource + // the caller did not ask for. + refresh := url.Values{} + refresh.Set("grant_type", "refresh_token") + refresh.Set("refresh_token", refreshToken) + refresh.Set("client_id", cfg.ClientID) + refresh.Set("resource", "https://attacker.example.com/mcp") + + status, body := postToken(t, router, refresh) + assert.Equal(t, http.StatusBadRequest, status) + assert.Equal(t, "invalid_target", body["error"]) + }) + t.Run("the rotated access token keeps the audience", func(t *testing.T) { + refresh := url.Values{} + refresh.Set("grant_type", "refresh_token") + refresh.Set("refresh_token", refreshToken) + refresh.Set("client_id", cfg.ClientID) + + status, body := postToken(t, router, refresh) + require.Equal(t, http.StatusOK, status, "body: %v", body) + + rotated, _ := body["access_token"].(string) + require.NotEmpty(t, rotated) + + claims, pErr := ts.TokenProvider.ParseJWTToken(rotated) + require.NoError(t, pErr) + assert.Equal(t, resource, claims["aud"], + "a refreshed token must stay bound to the resource the grant named; falling back to the client id "+ + "silently widens a token the user scoped to one resource server") + }) + +} + +// TestRefreshWithoutResourceIsUnchanged is the regression guard for every +// deployment that has never used a resource indicator: their refresh tokens +// carry no `resource` claim, and the rotated access token must keep the client +// id as its audience exactly as before. +func TestRefreshWithoutResourceIsUnchanged(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + router, sessionToken := mcpSession(t, ts, []string{"openid", "offline_access"}) + verifier := "another-code-verifier-long-enough-to-be-valid-00000000" + + qs := url.Values{} + qs.Set("response_type", "code") + qs.Set("client_id", cfg.ClientID) + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("state", "st") + qs.Set("response_mode", "query") + qs.Set("scope", "openid offline_access") + qs.Set("code_challenge", s256Challenge(verifier)) + qs.Set("code_challenge_method", "S256") + + code := codeFromRedirect(t, doAuthorizeGET(router, qs, sessionToken)) + + exchange := url.Values{} + exchange.Set("grant_type", "authorization_code") + exchange.Set("code", code) + exchange.Set("client_id", cfg.ClientID) + exchange.Set("redirect_uri", "http://localhost:3000/callback") + exchange.Set("code_verifier", verifier) + + status, body := postToken(t, router, exchange) + require.Equal(t, http.StatusOK, status, "body: %v", body) + refreshToken, _ := body["refresh_token"].(string) + require.NotEmpty(t, refreshToken) + + refresh := url.Values{} + refresh.Set("grant_type", "refresh_token") + refresh.Set("refresh_token", refreshToken) + refresh.Set("client_id", cfg.ClientID) + + status, body = postToken(t, router, refresh) + require.Equal(t, http.StatusOK, status, "body: %v", body) + + rotated, _ := body["access_token"].(string) + claims, err := ts.TokenProvider.ParseJWTToken(rotated) + require.NoError(t, err) + assert.Equal(t, cfg.ClientID, claims["aud"], + "an unbound grant must keep the client id as the audience — the fix must not stamp an empty resource") +} diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index d70c26a5e..0491f8201 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -67,6 +67,11 @@ var reservedClaims = map[string]bool{ // could set it would point the check at a session that is still alive and // survive the logout that should have ended the delegation. "sid": true, + // resource carries the RFC 8707 resource indicator across refresh so the + // rotated access token keeps the audience the grant was bound to. A script + // that could set it would rebind a token to a resource server the user never + // authorized, which is the audience restriction working in reverse. + "resource": true, } // AuthTokenConfig is the configuration for auth token @@ -329,6 +334,26 @@ func (p *provider) CreateRefreshToken(cfg *AuthTokenConfig) (string, int64, erro "family_id": familyID, } + // RFC 8707 §2.2: a refreshed access token stays bound to the resource the + // original grant named. The refresh token is the only thing that survives + // between the authorization request and the rotation, so the binding has to + // travel on it — the authorization code is long gone by then. + // + // Without this the local `resource` in the token endpoint is empty on the + // refresh grant, accessTokenAudience falls back to the client id, and the + // rotated access token comes back UNBOUND: usable at Authorizer's own API, + // which is exactly what the resource restriction existed to prevent. It also + // broke every resource server silently, since the first token works and only + // the refreshed one does not. + // + // Emitted only when the grant was bound, so tokens from flows that never + // used a resource indicator keep their existing claim set byte for byte. + // Reserved (see reservedClaims) so CustomAccessTokenScript cannot rebind a + // token to a resource server the user never authorized. + if cfg.Resource != "" { + customClaims["resource"] = cfg.Resource + } + token, err := p.SignJWTToken(customClaims) if err != nil { return "", 0, err From 9eb334a2db12138d0f07bdd416dc2ccbbfd6b9bb Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 18:44:48 +0530 Subject: [PATCH 05/11] feat(mcp): serve MCP over HTTP at POST /mcp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the resource server. `--mcp-enabled` mounts the MCP tool surface on the main HTTP listener, where it inherits CORS, security headers, rate limiting, trusted-proxy handling, logging, metrics and graceful shutdown instead of re-implementing them behind a second port. Identity is now per request. `stampAuth` forwards the caller's own Authorization header, so one server serves every caller under their own token — replacing the process-wide --mcp-bearer that made stdio a one-process-one-user transport. ONLY that header crosses the bridge: transport.MetaFromGRPC reconstructs cookies and x-authorizer-url from gRPC metadata, so forwarding headers wholesale would hand an audience-bound surface a browser session or a caller-chosen host. MCP dispatches through its OWN bufconn-only gRPC server, never a listening one, whose interceptor accepts exactly the resource-bound audience the public server rejects and refuses cookies, admin secrets and the admin service outright. Two servers, so no token crosses between surfaces by construction. Authentication happens twice, deliberately. The route middleware answers a bad or missing credential with 401 plus the RFC 9728 §5.1 WWW-Authenticate challenge, because that 401 IS the protocol: a fresh client reads resource_metadata from it to start discovery, and an expired one reads it to refresh. Anthropic's connector docs are explicit that Claude does not honour WWW-Authenticate on a 200, so answering an expired token with a JSON-RPC error would leave clients looping on a dead token. Context does not survive the bufconn hop, so the interceptor re-resolves the identity handlers run under. One extra validation per request buys the correct HTTP status. Stateless + JSONResponse: the main listener's WriteTimeout would sever a long-lived SSE stream, and a stateless server needs no sticky sessions. GET is answered 405 per spec — every exposed tool is request/response. Also here: - RFC 8252 §7.3 loopback redirect matching. Native apps bind an ephemeral port and cannot register it, so exact matching made loopback redirects unusable — Claude Code's OAuth flow could never have completed. Narrowed hard: both URIs must be loopback, and only the port is ignored. - /mcp exempted from CSRF, which holds structurally rather than by convention: no cookie can authenticate this surface at any layer. - TestServer_StdioOnly replaced. It named its own exit condition — build an auth interceptor first — and that interceptor now exists. What it guarded is now "the transport is the shape the main listener can serve", not "there is no transport". - `authorizer mcp` marked deprecated for removal in 2.5.0, with a startup warn so supervised deployments see it too. Refs authorizer-docs specs/2026-08-10-mcp-http-transport.md §5 PR 2 --- cmd/mcp.go | 16 ++ cmd/root.go | 42 +++++ internal/http_handlers/authorize.go | 64 ++++++- internal/http_handlers/csrf.go | 13 ++ internal/http_handlers/protected_resource.go | 62 +++++++ internal/http_handlers/provider.go | 3 + .../redirect_uri_loopback_test.go | 52 ++++++ .../mcp_access_token_test.go | 22 +-- internal/integration_tests/mcp_http_test.go | 167 ++++++++++++++++++ internal/mcp/server.go | 96 ++++++++-- internal/mcp/stamp_test.go | 63 ++++++- internal/mcp/transport_test.go | 101 +++++++---- internal/server/http_routes.go | 15 ++ internal/server/server.go | 8 + internal/token/provider.go | 8 +- 15 files changed, 658 insertions(+), 74 deletions(-) create mode 100644 internal/http_handlers/redirect_uri_loopback_test.go create mode 100644 internal/integration_tests/mcp_http_test.go diff --git a/cmd/mcp.go b/cmd/mcp.go index 1d6bb0825..f176b0125 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -64,6 +64,17 @@ var mcpCmd = &cobra.Command{ } func init() { + // Cobra prints this above the command's output on every invocation. + // + // The stdio transport cannot be deployed: it runs a second copy of every + // provider (storage, memory store, FGA engine) and its identity is one + // process-wide --mcp-bearer, so a process serves exactly one user forever. + // `--mcp-enabled` replaces both properties — the MCP surface shares the + // running server's providers, and every request carries its own token. + mcpCmd.Deprecated = "the stdio MCP transport will be removed in 2.5.0. " + + "Run the server with --mcp-enabled and connect to POST /mcp instead. " + + "See https://docs.authorizer.dev/core/mcp" + mcpCmd.Flags().StringVar(&mcpArgs.bearer, "mcp-bearer", "", "Bearer token to attach to every outgoing gRPC call (carries the "+ "user identity for tools like Profile / Permissions / Session). "+ @@ -82,6 +93,11 @@ func runMCP(_ *cobra.Command, _ []string) { // JSON-RPC framing on stdout. log := zerolog.New(os.Stderr).With().Timestamp().Logger() + // Cobra's deprecation notice goes to the terminal; this puts it where an + // operator running under a supervisor will actually see it. + log.Warn().Msg("`authorizer mcp` (stdio) is deprecated and will be removed in 2.5.0 — " + + "run the server with --mcp-enabled and connect to POST /mcp instead") + // Wire all subsystems an MCP-exposed tool might need. As more ops // migrate into internal/service, this list stays the same — the // service-provider dependencies don't change per op, only the methods diff --git a/cmd/root.go b/cmd/root.go index 1a3c4d7ad..71cde37f8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net" + "net/http" "os" "os/signal" "strconv" @@ -23,8 +24,10 @@ import ( "github.com/authorizerdev/authorizer/internal/email" "github.com/authorizerdev/authorizer/internal/events" "github.com/authorizerdev/authorizer/internal/grpcsrv" + "github.com/authorizerdev/authorizer/internal/grpcsrv/interceptors" "github.com/authorizerdev/authorizer/internal/http_handlers" scimhttp "github.com/authorizerdev/authorizer/internal/http_handlers/scim" + "github.com/authorizerdev/authorizer/internal/mcp" "github.com/authorizerdev/authorizer/internal/memory_store" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/oauth" @@ -741,6 +744,44 @@ func runRoot(c *cobra.Command, args []string) { } rootArgs.server.GRPCPort = rootArgs.config.GRPCPort + // MCP surface, served at POST /mcp on the main HTTP listener when enabled. + // + // It gets its OWN gRPC server rather than reusing grpcSrv above, and that is + // the security boundary, not an implementation detail. This one never binds a + // port — the MCP handler dials it over an in-process bufconn — and its auth + // interceptor accepts exactly one kind of credential: a bearer token whose + // audience is this deployment's canonical /mcp. The port-listening server + // rejects that audience, and this one rejects everything the port-listening + // server accepts. Two objects, so a token minted for one surface cannot + // authenticate the other by construction rather than by a conditional. + // + // Every provider is shared with the main server. The `authorizer mcp` + // subcommand builds a second copy of the entire stack — storage, memory + // store, FGA engine and all — which is precisely what made it undeployable. + var mcpHandler http.Handler + if rootArgs.config.MCPEnabled { + mcpResource := rootArgs.config.MCPResource() + mcpGRPC, mErr := grpcsrv.New(":0", &grpcsrv.Dependencies{ + Log: &log, + Config: &rootArgs.config, + ServiceProvider: serviceProvider, + TokenProvider: tokenProvider, + TokenResolver: interceptors.MCPTokenResolver(tokenProvider, mcpResource), + }) + if mErr != nil { + log.Fatal().Err(mErr).Msg("failed to create mcp grpc server") + } + mcpSrv, mErr := mcp.New(&log, mcpGRPC.GRPCServer(), mcp.Options{ + Name: "authorizer", + Version: constants.VERSION, + }) + if mErr != nil { + log.Fatal().Err(mErr).Msg("failed to create mcp server") + } + mcpHandler = mcpSrv.Handler() + log.Info().Str("resource", mcpResource).Msg("MCP enabled at POST /mcp") + } + // Inbound SCIM 2.0 server (per-org user provisioning). Transport-thin // handler over the scim service; org resolved only from the bearer token. scimService := scim.New(&scim.Dependencies{ @@ -761,6 +802,7 @@ func runRoot(c *cobra.Command, args []string) { AppConfig: &rootArgs.config, HTTPProvider: httpProvider, ScimHandler: scimHandler, + MCPHandler: mcpHandler, GRPCServer: grpcSrv, } // Create the server diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index 9a3117dc2..1009a80a8 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -173,7 +173,7 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { if registered := client.ParsedRedirectURIs(); len(registered) > 0 { validRedirect = false for _, r := range registered { - if r == redirectURI { + if redirectURIMatches(r, redirectURI) { validRedirect = true break } @@ -1029,6 +1029,68 @@ func supportedResponseTypeSet(raw string) (string, bool) { return "", false } +// redirectURIMatches reports whether a presented redirect_uri satisfies a +// registered one. +// +// Exact string comparison, with one carve-out: [RFC 8252 §7.3] requires an +// authorization server to allow a native app to specify ANY port on a loopback +// redirect, because the app binds an ephemeral port at run time and cannot know +// it at registration. Exact matching alone makes loopback redirects unusable — +// a client registering "http://127.0.0.1/callback" then arrives on +// "http://127.0.0.1:53119/callback" and is refused every time. +// +// The carve-out is as narrow as it can be: +// +// - It applies only when BOTH the registered and the presented URI are +// loopback. A registered https://app.example.com/cb never matches anything +// but itself, so nothing about third-party redirect validation changes. +// - Only the port is ignored. Scheme, host, path and query must still match +// exactly, so it cannot be used to reach a different path on the same host. +// - The host is compared literally: a registration for "localhost" does not +// match "127.0.0.1". They are different names and RFC 8252 §8.3 discourages +// the former; a client that wants both registers both. +// +// This only ever widens what is accepted, and only for redirects that terminate +// on the user's own machine. The residual risk is the one RFC 8252 §8.3 and the +// MCP authorization spec both name — a local process racing for the port — which +// no server-side URI check can address and which is mitigated by displaying the +// redirect host at consent. +// +// [RFC 8252 §7.3]: https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 +func redirectURIMatches(registered, presented string) bool { + if registered == presented { + return true + } + r, err := url.Parse(registered) + if err != nil { + return false + } + p, err := url.Parse(presented) + if err != nil { + return false + } + if !isLoopbackHost(r.Hostname()) || !isLoopbackHost(p.Hostname()) { + return false + } + return r.Scheme == p.Scheme && + r.Hostname() == p.Hostname() && + r.Path == p.Path && + r.RawQuery == p.RawQuery +} + +// isLoopbackHost reports whether a hostname names the local machine. "localhost" +// is included because RFC 8252 §7.3's port rule is written for the IP literals, +// while real native clients — Claude Code among them — declare the name form and +// expect the same treatment. +func isLoopbackHost(host string) bool { + switch host { + case "127.0.0.1", "::1", "localhost": + return true + default: + return false + } +} + // isValidResourceIndicator enforces RFC 8707 §2 on a resource indicator: it // MUST be an absolute URI (scheme + hierarchical part) and MUST NOT contain a // fragment component. A relative reference, an opaque non-URI string, or any diff --git a/internal/http_handlers/csrf.go b/internal/http_handlers/csrf.go index 6ecc0e940..313fd82b7 100644 --- a/internal/http_handlers/csrf.go +++ b/internal/http_handlers/csrf.go @@ -58,6 +58,19 @@ func (h *httpProvider) CSRFMiddleware() gin.HandlerFunc { return } + // Exempt the MCP surface. Same rationale, and it holds structurally + // rather than by convention: MCPAuthMiddleware authenticates only a + // bearer token whose audience names this MCP server, the interceptor + // behind it accepts nothing else (no cookie, no admin secret), and the + // bridge to that interceptor forwards only the Authorization header. A + // cookie riding along on a cross-site request therefore cannot + // authenticate anything here, which is exactly the condition CSRF + // protection exists to create. + if c.Request.URL.Path == "/mcp" { + c.Next() + return + } + // Exempt POST /userinfo (OIDC Core §5.3.1 requires the UserInfo // endpoint to accept POST). Authenticated via a bearer access token, // not cookies, so CSRF does not apply — same rationale as diff --git a/internal/http_handlers/protected_resource.go b/internal/http_handlers/protected_resource.go index 38800012e..93fb81c2e 100644 --- a/internal/http_handlers/protected_resource.go +++ b/internal/http_handlers/protected_resource.go @@ -78,3 +78,65 @@ func (h *httpProvider) ProtectedResourceMetadataHandler() gin.HandlerFunc { }) } } + +// MCPAuthMiddleware authenticates a request to /mcp and, when it cannot, returns +// the RFC 9728 §5.1 challenge that starts the discovery chain. +// +// The 401 is not merely an error, it is the protocol. A client with no +// credential reads `resource_metadata` from WWW-Authenticate, fetches the +// metadata document, finds the authorization server and begins OAuth; a client +// whose token has expired reads the same 401 and refreshes. Anthropic's connector +// documentation is explicit that Claude refreshes reactively on a 401 and does +// not honour a WWW-Authenticate header on a 200 — so answering an expired token +// with a JSON-RPC error inside a 200 would leave the client looping on a dead +// token with no way to discover why. +// +// This authenticates a SECOND time: the in-process gRPC interceptor resolves the +// identity that handlers actually run under (interceptors.MCPTokenResolver), and +// context values do not survive the bufconn hop, so the check cannot be shared. +// The duplication buys the correct HTTP status, which is what makes the surface +// usable by a real client. It costs one extra token validation per request — +// worth optimising later, not worth trading the protocol for. +func (h *httpProvider) MCPAuthMiddleware() gin.HandlerFunc { + // Computed once: both derive from --url, which MCP requires, so neither can + // be influenced by the request. + resource := h.Config.MCPResource() + metadataURL := h.Config.CanonicalURL() + "/.well-known/oauth-protected-resource/mcp" + + return func(c *gin.Context) { + log := h.Log.With().Str("func", "MCPAuthMiddleware").Logger() + + // RFC 6750 §3: a request with NO credential gets a bare challenge. An + // `error` parameter is only correct once a credential was supplied and + // found wanting — reporting invalid_token to a client that sent nothing + // misdescribes a first contact as a failure. + accessToken, err := h.TokenProvider.GetAccessToken(c) + if err != nil || accessToken == "" { + c.Header("WWW-Authenticate", + `Bearer realm="authorizer", resource_metadata="`+metadataURL+`"`) + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{ + "error": "invalid_request", + "error_description": "authorization required", + }) + return + } + + if _, vErr := h.TokenProvider.ValidateMCPAccessToken(c, accessToken, resource); vErr != nil { + // Debug, not warn: a wrong-audience or expired token at this endpoint + // is the ordinary steady state of a client that needs to refresh, not + // evidence of an attack. + log.Debug().Err(vErr).Msg("mcp request rejected") + c.Header("WWW-Authenticate", + `Bearer realm="authorizer", error="invalid_token", `+ + `error_description="The access token is invalid, expired, or was not issued for this MCP server", `+ + `resource_metadata="`+metadataURL+`"`) + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{ + "error": "invalid_token", + "error_description": "The access token is invalid, expired, or was not issued for this MCP server", + }) + return + } + + c.Next() + } +} diff --git a/internal/http_handlers/provider.go b/internal/http_handlers/provider.go index ee7e6b553..a11efa311 100644 --- a/internal/http_handlers/provider.go +++ b/internal/http_handlers/provider.go @@ -132,6 +132,9 @@ type Provider interface { // ProtectedResourceMetadataHandler serves RFC 9728 OAuth 2.0 Protected // Resource Metadata for the MCP surface. Registered only when MCP is enabled. ProtectedResourceMetadataHandler() gin.HandlerFunc + // MCPAuthMiddleware authenticates /mcp and issues the RFC 9728 §5.1 + // WWW-Authenticate challenge when it cannot. + MCPAuthMiddleware() gin.HandlerFunc // PlaygroundHandler is the main handler that handels all the playground requests PlaygroundHandler() gin.HandlerFunc // RevokeRefreshTokenHandler is the main handler that handels all the revoke refresh token requests diff --git a/internal/http_handlers/redirect_uri_loopback_test.go b/internal/http_handlers/redirect_uri_loopback_test.go new file mode 100644 index 000000000..2ea471e2c --- /dev/null +++ b/internal/http_handlers/redirect_uri_loopback_test.go @@ -0,0 +1,52 @@ +package http_handlers + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestRedirectURIMatches pins both halves of RFC 8252 §7.3: loopback redirects +// must tolerate any port, and nothing else may. +// +// The negative cases carry the weight. This function widens redirect_uri +// validation, which is the check standing between a registered client and an +// authorization code being delivered to an attacker's endpoint, so every way the +// carve-out could leak past loopback is asserted explicitly. +func TestRedirectURIMatches(t *testing.T) { + cases := []struct { + name string + registered string + presented string + want bool + }{ + {"identical", "https://app.example.com/cb", "https://app.example.com/cb", true}, + + // RFC 8252 §7.3: a native app binds an ephemeral port at run time. + {"loopback ignores an added port", "http://127.0.0.1/callback", "http://127.0.0.1:53119/callback", true}, + {"loopback ignores a changed port", "http://127.0.0.1:1/callback", "http://127.0.0.1:53119/callback", true}, + {"localhost ignores a port", "http://localhost/callback", "http://localhost:3118/callback", true}, + {"ipv6 loopback ignores a port", "http://[::1]/callback", "http://[::1]:8080/callback", true}, + + // The carve-out must not reach anything that is not loopback. + {"public host keeps exact port matching", "https://app.example.com/cb", "https://app.example.com:8443/cb", false}, + {"public host does not match loopback", "https://app.example.com/cb", "http://127.0.0.1:9/cb", false}, + {"loopback does not match a public host", "http://127.0.0.1/cb", "https://attacker.example.com/cb", false}, + + // Only the port is ignored — never scheme, host, path or query. + {"path must still match", "http://127.0.0.1/callback", "http://127.0.0.1:9/evil", false}, + {"scheme must still match", "http://127.0.0.1/callback", "https://127.0.0.1:9/callback", false}, + {"query must still match", "http://127.0.0.1/cb?a=1", "http://127.0.0.1:9/cb?a=2", false}, + {"localhost and 127.0.0.1 are different names", "http://localhost/cb", "http://127.0.0.1:9/cb", false}, + + // A hostname that merely contains a loopback name is not loopback. + {"lookalike host is not loopback", "http://127.0.0.1/cb", "http://127.0.0.1.evil.com:9/cb", false}, + {"localhost subdomain is not loopback", "http://localhost/cb", "http://localhost.evil.com:9/cb", false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, redirectURIMatches(tc.registered, tc.presented)) + }) + } +} diff --git a/internal/integration_tests/mcp_access_token_test.go b/internal/integration_tests/mcp_access_token_test.go index a67f69bd7..f21fb9583 100644 --- a/internal/integration_tests/mcp_access_token_test.go +++ b/internal/integration_tests/mcp_access_token_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" "github.com/authorizerdev/authorizer/internal/token" @@ -46,7 +47,7 @@ func mintStatefulAccessToken(t *testing.T, ts *testSetup, user *schemas.User, re Roles: []string{"user"}, Scope: []string{"openid"}, LoginMethod: constants.AuthRecipeMethodBasicAuth, - HostName: testAuthorizerHost(ts), + HostName: parsers.GetHost(ts.GinContext), Resource: resource, ExpireTime: "30m", }) @@ -71,7 +72,7 @@ func mintMachineAccessToken(t *testing.T, ts *testSetup, clientRowID, resource s Nonce: nonce, Scope: []string{"openid"}, LoginMethod: constants.AuthRecipeMethodServiceAccount, - HostName: testAuthorizerHost(ts), + HostName: parsers.GetHost(ts.GinContext), Resource: resource, ExpireTime: "30m", }) @@ -186,7 +187,7 @@ func TestMCPAccessTokenAudienceBoundary(t *testing.T) { Nonce: uuid.NewString(), Roles: []string{"user"}, LoginMethod: constants.AuthRecipeMethodBasicAuth, - HostName: testAuthorizerHost(ts), + HostName: parsers.GetHost(ts.GinContext), Resource: resource, ExpireTime: "30m", }) @@ -201,15 +202,14 @@ func TestMCPAccessTokenAudienceBoundary(t *testing.T) { }) } -// TestMCPAccessTokenServiceAccountLiveness pins the one place the MCP validator -// is deliberately STRICTER than the first-party path. +// TestMCPAccessTokenServiceAccountLiveness pins that a deactivated service +// account cannot reach the MCP surface. // -// userIsRevoked resolves a token's subject as a user only, and returns "not -// revoked" when it finds nothing — so a machine token, whose `sub` is a client -// row id, survives deactivation of the service account until it expires. That is -// inherited behaviour on existing surfaces. MCP's headline callers are agents and -// service accounts, so shipping it there knowingly would be worse: the validator -// uses subjectIsLive, which resolves user-then-client and fails closed. +// Subject liveness is NOT an MCP-specific rule — the same check guards GraphQL, +// gRPC, REST and the browser session, and TestDeactivatingAServiceAccountStopsItsLiveTokens +// covers it there. It is asserted here as well because agents and service +// accounts are the MCP surface's headline callers, so this is the path where a +// regression would be felt first and noticed last. func TestMCPAccessTokenServiceAccountLiveness(t *testing.T) { cfg := getTestConfig() cfg.AuthorizerURL = "https://auth.example.com" diff --git a/internal/integration_tests/mcp_http_test.go b/internal/integration_tests/mcp_http_test.go new file mode 100644 index 000000000..126db2228 --- /dev/null +++ b/internal/integration_tests/mcp_http_test.go @@ -0,0 +1,167 @@ +package integration_tests + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/grpcsrv" + "github.com/authorizerdev/authorizer/internal/grpcsrv/interceptors" + "github.com/authorizerdev/authorizer/internal/mcp" + "github.com/authorizerdev/authorizer/internal/parsers" + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// mcpRouter wires the MCP surface exactly as internal/server does: the auth +// middleware that issues the RFC 9728 challenge, in front of the Streamable HTTP +// handler, dispatching over a bufconn-only gRPC server whose interceptor accepts +// only MCP-audience tokens. +// +// Building the real chain rather than stubbing it is the point — the properties +// under test (a 401 that carries discovery information, an audience boundary +// enforced two layers down) only exist when those pieces are assembled together. +func mcpRouter(t *testing.T, ts *testSetup, cfg *config.Config) http.Handler { + t.Helper() + log := zerolog.Nop() + + grpcSrv, err := grpcsrv.New(":0", &grpcsrv.Dependencies{ + Log: &log, + Config: cfg, + ServiceProvider: ts.ServiceProvider, + TokenProvider: ts.TokenProvider, + TokenResolver: interceptors.MCPTokenResolver(ts.TokenProvider, cfg.MCPResource()), + }) + require.NoError(t, err) + + mcpSrv, err := mcp.New(&log, grpcSrv.GRPCServer(), mcp.Options{Name: "authorizer", Version: "test"}) + require.NoError(t, err) + + router := gin.New() + router.Any("/mcp", ts.HttpProvider.MCPAuthMiddleware(), gin.WrapH(mcpSrv.Handler())) + return router +} + +func mcpPost(t *testing.T, router http.Handler, bearer, body string) *httptest.ResponseRecorder { + t.Helper() + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if bearer != "" { + req.Header.Set("Authorization", "Bearer "+bearer) + } + router.ServeHTTP(w, req) + return w +} + +// TestMCPHTTPSurface covers the transport end to end: the challenge that starts +// discovery, the audience boundary, and a tool call actually reaching a handler +// under the caller's own identity. +func TestMCPHTTPSurface(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = "https://auth.example.com" + cfg.MCPEnabled = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + // MCP requires --url, and with it set parsers.GetHost returns the canonical + // URL for every request regardless of headers. Pinning it here is what makes + // this test exercise the production shape: the token's `iss` and the audience + // check both resolve to the configured URL rather than to whatever Host an + // httptest request happens to carry. + parsers.SetTrustedURL(cfg.AuthorizerURL) + t.Cleanup(func() { parsers.SetTrustedURL("") }) + + resource := cfg.MCPResource() + router := mcpRouter(t, ts, cfg) + + now := time.Now().Unix() + user, err := ts.StorageProvider.AddUser(ctx, &schemas.User{ + Email: refs.NewStringRef("mcp_http_" + uuid.NewString() + "@authorizer.dev"), + EmailVerifiedAt: &now, + SignupMethods: constants.AuthRecipeMethodBasicAuth, + Roles: "user", + }) + require.NoError(t, err) + + t.Run("no credential returns a challenge that starts discovery", func(t *testing.T) { + w := mcpPost(t, router, "", initializeRPC) + + require.Equal(t, http.StatusUnauthorized, w.Code) + challenge := w.Header().Get("WWW-Authenticate") + assert.Contains(t, challenge, `resource_metadata="https://auth.example.com/.well-known/oauth-protected-resource/mcp"`, + "RFC 9728 §5.1: this pointer is the ONLY thing telling a fresh client where to authenticate") + assert.NotContains(t, challenge, "error=", + "RFC 6750 §3: an error parameter describes a credential that was supplied and rejected, not a first contact") + }) + + t.Run("an ordinary login token is refused with invalid_token", func(t *testing.T) { + // The audience boundary, from the outside. A token that authenticates + // GraphQL must not authenticate MCP. + w := mcpPost(t, router, mintStatefulAccessToken(t, ts, user, ""), initializeRPC) + + require.Equal(t, http.StatusUnauthorized, w.Code) + assert.Contains(t, w.Header().Get("WWW-Authenticate"), `error="invalid_token"`, + "a supplied-but-wrong credential must say so, and must be a 401 so the client refreshes rather than looping") + }) + + t.Run("a token for another resource server is refused", func(t *testing.T) { + w := mcpPost(t, router, mintStatefulAccessToken(t, ts, user, "https://other.example.com/mcp"), initializeRPC) + assert.Equal(t, http.StatusUnauthorized, w.Code) + }) + + t.Run("a correctly-audienced token reaches the tool surface", func(t *testing.T) { + bearer := mintStatefulAccessToken(t, ts, user, resource) + + w := mcpPost(t, router, bearer, initializeRPC) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + w = mcpPost(t, router, bearer, `{"jsonrpc":"2.0","id":2,"method":"tools/list"}`) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp struct { + Result struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + } `json:"result"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.NotEmpty(t, resp.Result.Tools, "the proto-annotated tool set must be discoverable") + + names := make([]string, 0, len(resp.Result.Tools)) + for _, tool := range resp.Result.Tools { + names = append(names, tool.Name) + } + assert.Contains(t, names, "profile", + "an identity-bearing tool must be exposed, or nothing proves the caller's token reached a handler") + }) + + t.Run("GET is refused once authenticated", func(t *testing.T) { + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/mcp", nil) + req.Header.Set("Accept", "text/event-stream") + req.Header.Set("Authorization", "Bearer "+mintStatefulAccessToken(t, ts, user, resource)) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusMethodNotAllowed, w.Code, + "no SSE stream is offered: the main listener's 60s write timeout would sever it") + }) +} + +// initializeRPC is the MCP handshake a client sends before anything else. +const initializeRPC = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` + + `"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"test","version":"1"}}}` diff --git a/internal/mcp/server.go b/internal/mcp/server.go index b77f5e65f..544d20533 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -1,6 +1,6 @@ -// Package mcp serves a curated subset of Authorizer's gRPC methods to -// LLM clients via the Model Context Protocol. Stdio is the ONLY supported -// transport — see the deliberate design note on Server below. +// Package mcp serves a curated subset of Authorizer's gRPC methods to LLM +// clients via the Model Context Protocol, over Streamable HTTP (the deployable +// transport) or stdio (development only). See the design note on Server. package mcp import ( @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "net" + "net/http" "strings" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -25,15 +26,25 @@ const bufSize = 1 << 20 // Server wraps an MCP server that bridges to an in-process gRPC server. // -// Design constraint: stdio is the ONLY supported transport. The MCP server -// has no auth/rate-limit/audit interceptors of its own — it relies entirely -// on the OS-level trust boundary of the subprocess (Claude Code spawns -// `authorizer mcp` as a child; only that process can write to its stdin). -// Exposing the MCP server over TCP / HTTP / SSE would invalidate that -// assumption and is intentionally NOT implementable: there is no RunHTTP / -// RunTCP / RunSSE method, and adding one without first implementing an -// auth layer is a security regression. The stdio-only contract is also -// enforced by TestServer_StdioOnly. +// Two transports, with very different security models. +// +// Handler() serves Streamable HTTP and is the deployable one. It carries no +// ambient authority: every tool call is authenticated by the caller's own bearer +// token, whose audience must name this MCP server (token.ValidateMCPAccessToken), +// and it runs on a gRPC server whose auth interceptor accepts nothing else — no +// cookies, no admin secret, no admin service. The route wrapper rejects a bad +// credential with a 401 so clients can start discovery or refresh. +// +// RunStdio has no auth of its own and relies entirely on the OS-level trust +// boundary of the subprocess: an MCP host spawns `authorizer mcp` as a child, and +// only that process can write to its stdin. Identity is the process-wide +// --mcp-bearer, so one process serves exactly one user. That is why it is a +// development transport and is deprecated for removal in 2.5.0. +// +// The earlier "stdio is the ONLY supported transport" constraint, and the +// TestServer_StdioOnly guard that enforced it, named their own exit condition: +// implement an auth interceptor for MCP first, then allow a network transport. +// That is what interceptors.MCPTokenResolver and the sole-authority guard are. type Server struct { log *zerolog.Logger mcpSrv *mcp.Server @@ -142,11 +153,31 @@ func (s *Server) cleanup() { _ = s.lis.Close() } -// stampAuth attaches the configured bearer and authorizer URL to the -// outgoing gRPC call. A no-op when neither is set. This is the bridge that -// lets gRPC handlers see "who is calling" (security audit H1) and which -// host minted the token (issuer validation) when invoked from MCP. -func (s *Server) stampAuth(ctx context.Context) context.Context { +// stampAuth attaches the caller's credential to the outgoing in-process gRPC +// call. This is the bridge that lets gRPC handlers see "who is calling". +// +// Over HTTP the credential is per REQUEST: reqHeader carries the Authorization +// header of the HTTP request that produced this tool call, so one server serves +// every caller under their own identity. Over stdio there is no HTTP request, so +// it falls back to the process-wide --mcp-bearer — one process, one user, which +// is exactly the limitation that makes stdio a development-only transport. +// +// ONLY the Authorization header crosses. Nothing else from the HTTP request is +// forwarded, and that is deliberate: transport.MetaFromGRPC reconstructs cookies +// and x-authorizer-url from gRPC metadata, so forwarding headers wholesale would +// let a browser session cookie — or a host header — reach the auth path of a +// surface whose whole security model is "the token's audience must name this MCP +// server". The interceptor refuses those credentials too (see +// interceptors.Auth's sole-authority guard), but the bridge should not be +// offering them in the first place. +// +// x-authorizer-url is likewise NOT forwarded. MCP requires --url, so +// parsers.GetHost short-circuits to the operator-configured value and a header +// could only disagree with it. +func (s *Server) stampAuth(ctx context.Context, reqHeader http.Header) context.Context { + if auth := reqHeader.Get("Authorization"); auth != "" { + return metadata.AppendToOutgoingContext(ctx, "authorization", auth) + } if s.bearer != "" { ctx = metadata.AppendToOutgoingContext(ctx, "authorization", "Bearer "+s.bearer) } @@ -156,6 +187,28 @@ func (s *Server) stampAuth(ctx context.Context) context.Context { return ctx } +// Handler serves MCP over Streamable HTTP. +// +// Stateless + JSONResponse, for two independent reasons. The main HTTP listener +// sets WriteTimeout: 60s, which would kill a long-lived SSE stream mid-flight; +// and a stateless server needs no sticky sessions, so an Authorizer deployment +// can scale horizontally without the MCP surface pinning a client to one +// replica. In this mode the SDK answers GET with 405 + Allow, which is +// spec-conformant — every exposed tool is request/response, so no server→client +// stream is needed. +// +// Authentication is NOT done here. It happens twice, on purpose: the route +// wrapper rejects a bad credential with a 401 so the client knows to refresh or +// start discovery, and the in-process gRPC interceptor resolves the identity +// that handlers actually run under. See the route registration in +// internal/server. +func (s *Server) Handler() http.Handler { + return mcp.NewStreamableHTTPHandler( + func(*http.Request) *mcp.Server { return s.mcpSrv }, + &mcp.StreamableHTTPOptions{Stateless: true, JSONResponse: true}, + ) +} + // registerTool wires one ToolBinding into the MCP server. The handler: // 1. Constructs a fresh proto.Message of the right type via dynamicpb // 2. Unmarshals JSON args into it @@ -184,8 +237,15 @@ func (s *Server) registerTool(b ToolBinding) { } } + // Extra is nil on transports that carry no HTTP request (stdio), where + // stampAuth falls back to the process-wide bearer. + var reqHeader http.Header + if req.Extra != nil { + reqHeader = req.Extra.Header + } + respMsg := dynamicpb.NewMessage(b.OutputDescriptor) - if err := s.gwConn.Invoke(s.stampAuth(ctx), b.FullMethod, reqMsg, respMsg); err != nil { + if err := s.gwConn.Invoke(s.stampAuth(ctx, reqHeader), b.FullMethod, reqMsg, respMsg); err != nil { s.log.Debug().Err(err).Str("tool", b.Name).Str("method", b.FullMethod).Msg("MCP tool invocation failed") // gRPC errors (Unimplemented, PermissionDenied, NotFound, ...) // become CallToolResult{IsError: true} with the gRPC status diff --git a/internal/mcp/stamp_test.go b/internal/mcp/stamp_test.go index f28932fef..1638d52fa 100644 --- a/internal/mcp/stamp_test.go +++ b/internal/mcp/stamp_test.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "net/http" "testing" "github.com/stretchr/testify/assert" @@ -9,21 +10,22 @@ import ( "google.golang.org/grpc/metadata" ) -// TestStampAuth covers the per-dispatch metadata bridge: the configured -// bearer must surface as `authorization` and the configured authorizer URL -// as `x-authorizer-url` (JWT issuer validation resolves the host from it — -// without it the in-process bufconn authority would reject every token). +// TestStampAuth covers the per-dispatch metadata bridge. The stdio cases below +// pass a nil header (there is no HTTP request), so the process-wide bearer and +// authorizer URL are what surface; TestStampAuthPerRequest covers the HTTP case, +// where the caller's own Authorization header must win and nothing else may +// cross. func TestStampAuth(t *testing.T) { t.Run("no bearer, no url is a no-op", func(t *testing.T) { s := &Server{} - ctx := s.stampAuth(context.Background()) + ctx := s.stampAuth(context.Background(), nil) _, ok := metadata.FromOutgoingContext(ctx) assert.False(t, ok) }) t.Run("bearer and url are both stamped", func(t *testing.T) { s := &Server{bearer: "tok-123", authorizerURL: "https://auth.example.com"} - ctx := s.stampAuth(context.Background()) + ctx := s.stampAuth(context.Background(), nil) md, ok := metadata.FromOutgoingContext(ctx) require.True(t, ok) assert.Equal(t, []string{"Bearer tok-123"}, md.Get("authorization")) @@ -32,10 +34,57 @@ func TestStampAuth(t *testing.T) { t.Run("bearer without url stamps only authorization", func(t *testing.T) { s := &Server{bearer: "tok-123"} - ctx := s.stampAuth(context.Background()) + ctx := s.stampAuth(context.Background(), nil) md, ok := metadata.FromOutgoingContext(ctx) require.True(t, ok) assert.Equal(t, []string{"Bearer tok-123"}, md.Get("authorization")) assert.Empty(t, md.Get("x-authorizer-url")) }) } + +// TestStampAuthPerRequest pins the property that makes one HTTP MCP server able +// to serve many callers: identity comes from the request, not the process. +// +// It also pins what must NOT cross. transport.MetaFromGRPC reconstructs cookies +// and x-authorizer-url from gRPC metadata, so forwarding headers wholesale would +// hand the auth path of an audience-bound surface a browser session cookie or a +// caller-chosen host. The interceptor refuses those anyway, but the bridge must +// not offer them. +func TestStampAuthPerRequest(t *testing.T) { + t.Run("the request's Authorization wins over the static bearer", func(t *testing.T) { + s := &Server{bearer: "process-wide", authorizerURL: "https://auth.example.com"} + h := http.Header{} + h.Set("Authorization", "Bearer caller-token") + + md, ok := metadata.FromOutgoingContext(s.stampAuth(context.Background(), h)) + require.True(t, ok) + assert.Equal(t, []string{"Bearer caller-token"}, md.Get("authorization"), + "an HTTP caller must be attributed to themselves, never to whoever started the process") + assert.Empty(t, md.Get("x-authorizer-url"), + "--url is mandatory for HTTP MCP, so a host hint is at best redundant and at worst caller-controlled") + }) + + t.Run("cookies and other headers never cross", func(t *testing.T) { + s := &Server{} + h := http.Header{} + h.Set("Authorization", "Bearer caller-token") + h.Set("Cookie", "authorizer_session=abc") + h.Set("X-Authorizer-URL", "https://evil.example.com") + h.Set("X-Authorizer-Admin-Secret", "hunter2") + + md, ok := metadata.FromOutgoingContext(s.stampAuth(context.Background(), h)) + require.True(t, ok) + assert.Equal(t, []string{"Bearer caller-token"}, md.Get("authorization")) + assert.Empty(t, md.Get("cookie")) + assert.Empty(t, md.Get("x-authorizer-url")) + assert.Empty(t, md.Get("x-authorizer-admin-secret")) + assert.Len(t, md, 1, "exactly one header crosses the bridge") + }) + + t.Run("an empty Authorization falls back to the stdio bearer", func(t *testing.T) { + s := &Server{bearer: "process-wide"} + md, ok := metadata.FromOutgoingContext(s.stampAuth(context.Background(), http.Header{})) + require.True(t, ok) + assert.Equal(t, []string{"Bearer process-wide"}, md.Get("authorization")) + }) +} diff --git a/internal/mcp/transport_test.go b/internal/mcp/transport_test.go index dd1d6fffe..9742a2887 100644 --- a/internal/mcp/transport_test.go +++ b/internal/mcp/transport_test.go @@ -1,42 +1,77 @@ package mcp import ( - "reflect" + "net/http" + "net/http/httptest" "strings" "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// TestServer_StdioOnly is a guard against accidentally adding a non-stdio -// transport to the MCP server. Stdio is the only supported transport — the -// security model relies on the OS-level trust boundary of the subprocess -// (Claude Code spawns `authorizer mcp` as a child; only that process can -// write to its stdin). Exposing MCP over TCP/HTTP/SSE without an auth -// interceptor would be a security regression, so this test fails the build -// if anyone adds RunHTTP / RunTCP / RunSSE / Listen* / Serve* etc. +// TestHandlerIsStateless pins the two transport options the HTTP surface depends +// on, by observing their effects rather than reading the struct back. +// +// This replaces TestServer_StdioOnly, which existed to stop a network transport +// being added before MCP had an auth layer, and which named its own exit +// condition: "implement an auth+rate-limit interceptor for MCP first, then +// update this test's allow-list." That interceptor now exists +// (interceptors.MCPTokenResolver plus the sole-authority guard), so the guard is +// replaced rather than deleted — the property worth protecting is no longer +// "there is no HTTP transport" but "the HTTP transport is the stateless, +// non-streaming shape the main listener can actually serve". // -// To deliberately add a new transport: implement an auth+rate-limit -// interceptor for MCP first, then update this test's allow-list. -func TestServer_StdioOnly(t *testing.T) { - allowed := map[string]struct{}{ - "RunStdio": {}, - "MCPServer": {}, // test accessor — not a transport - } - t.Logf("MCP Server exported methods allow-list: %v (anything outside this set indicates a new transport)", allowed) - - st := reflect.TypeOf((*Server)(nil)) - for i := 0; i < st.NumMethod(); i++ { - name := st.Method(i).Name - if _, ok := allowed[name]; ok { - continue - } - // Heuristic: any method whose name suggests serving / running / - // listening over a different transport is a red flag. - lower := strings.ToLower(name) - for _, banned := range []string{"http", "tcp", "sse", "websocket", "listen", "serve", "run"} { - if strings.Contains(lower, banned) { - t.Errorf("disallowed transport method %q on *Server: stdio is the only supported MCP transport. "+ - "Adding a network transport requires an MCP-side auth interceptor first; see Server type comment.", name) - } - } - } +// Why each matters: +// +// - GET must be refused. The main HTTP server sets WriteTimeout: 60s, so a +// long-lived SSE stream would be severed mid-flight with no error the client +// can distinguish from a network fault. A 405 tells the client immediately +// that this server does not offer a server→client stream. +// - No server-side session state. A stateful server pins a client to the +// replica that created its session, which would make the MCP surface the one +// part of Authorizer that cannot scale horizontally without sticky sessions. +// Asserted by sending an Mcp-Session-Id this process has never issued and +// requiring it to be served anyway — the observable form of "any replica can +// answer any request". Asserting on the response's session-id header instead +// would pin an SDK detail: in stateless mode an id may still be emitted, it +// is simply never validated. +func TestHandlerIsStateless(t *testing.T) { + // A real inner MCP server: the handler answers 400 "no server available" + // without one, which would make every assertion below vacuous. + s := &Server{mcpSrv: mcp.NewServer(&mcp.Implementation{Name: "authorizer", Version: "test"}, nil)} + + t.Run("GET is refused with an Allow header", func(t *testing.T) { + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/mcp", nil) + // Without this the SDK rejects the GET at Accept negotiation, before it + // ever reaches the stateless branch, and the assertion would pass for the + // wrong reason. + req.Header.Set("Accept", "text/event-stream") + s.Handler().ServeHTTP(w, req) + + assert.Equal(t, http.StatusMethodNotAllowed, w.Code, + "a stateless server must not offer an SSE stream the 60s write timeout would kill") + assert.Equal(t, "POST", w.Header().Get("Allow"), "RFC 9110 §15.5.6: a 405 MUST carry Allow") + }) + + t.Run("a request bearing an unknown session id is still served", func(t *testing.T) { + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(initializeBody)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Mcp-Session-Id", "issued-by-some-other-replica") + s.Handler().ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, + "a session this replica never created must not be rejected, or the surface needs sticky sessions; body: %s", w.Body.String()) + assert.Contains(t, w.Header().Get("Content-Type"), "application/json", + "JSONResponse: one JSON response per POST, not an event stream") + }) } + +// initializeBody is the MCP handshake every client sends first. +const initializeBody = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` + + `"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"t","version":"1"}}}` diff --git a/internal/server/http_routes.go b/internal/server/http_routes.go index fa67d7a74..62ca3ed78 100644 --- a/internal/server/http_routes.go +++ b/internal/server/http_routes.go @@ -114,6 +114,21 @@ func (s *server) NewRouter() *gin.Engine { if s.Dependencies.AppConfig != nil && s.Dependencies.AppConfig.MCPEnabled { router.GET("/.well-known/oauth-protected-resource/mcp", s.Dependencies.HTTPProvider.ProtectedResourceMetadataHandler()) + + // The MCP surface itself. Any method, not just POST: an unauthenticated + // GET must still receive the 401 challenge that starts discovery, and an + // authenticated one is answered 405 by the transport (no SSE stream — + // see mcp.Server.Handler). Registering POST alone would return gin's 404 + // instead, which tells a client nothing. + // + // Mounted on the main router deliberately, so it inherits CORS, security + // headers, rate limiting, trusted-proxy handling, request logging and + // metrics rather than re-implementing them behind a second listener. + if s.Dependencies.MCPHandler != nil { + router.Any("/mcp", + s.Dependencies.HTTPProvider.MCPAuthMiddleware(), + gin.WrapH(s.Dependencies.MCPHandler)) + } } // RFC 6749 §3.1 / OIDC Core §3.1.2.1: the authorization endpoint MUST // support GET and MAY support POST. diff --git a/internal/server/server.go b/internal/server/server.go index e54ba157b..70024e888 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -44,6 +44,14 @@ type Dependencies struct { // GRPCServer is the configured (but not yet listening) gRPC server. // nil disables both the gRPC listener and the REST `/v1/*` gateway. GRPCServer *grpcsrv.Server + // MCPHandler serves Streamable HTTP at POST /mcp. nil leaves the route + // unregistered; it is built only when --mcp-enabled is set. + // + // Note this is NOT GRPCServer's handler: MCP dispatches through its own + // bufconn-only gRPC server whose auth interceptor accepts exactly the + // resource-bound audience the public server rejects. Two servers, so no token + // can cross between the surfaces. See interceptors.MCPTokenResolver. + MCPHandler http.Handler // gatewayHandler / gatewayCleanup are built lazily inside Run when // GRPCServer is non-nil. Stored on the struct only to satisfy the // existing pattern of cleanup at Shutdown time. diff --git a/internal/token/provider.go b/internal/token/provider.go index c4560da98..2fbf7ff84 100644 --- a/internal/token/provider.go +++ b/internal/token/provider.go @@ -87,10 +87,10 @@ type Provider interface { // stricter by one (audience must be this server) — see its doc comment. ValidateDelegatedAccessToken(gc *gin.Context, accessToken string) (map[string]interface{}, error) // ValidateMCPAccessToken validates an access token presented at the MCP - // surface. Same stateful core as ValidateAccessToken, stricter on two - // checks: `aud` must equal the caller-supplied canonical MCP resource URI - // (RFC 8707 / MCP authorization), and subject liveness resolves - // user-then-client so a deactivated service account is rejected. + // surface. Same stateful core as ValidateAccessToken, differing in exactly + // one rule: `aud` must equal the caller-supplied canonical MCP resource URI + // (RFC 8707 / MCP authorization), which is the audience ValidateAccessToken + // rejects. Every other check, subject liveness included, is shared. ValidateMCPAccessToken(gc *gin.Context, accessToken string, resource string) (map[string]interface{}, error) // ValidateAdminToken validates session token ValidateBrowserSession(gc *gin.Context, encryptedSession string) (*SessionData, error) From 3843be9a96359fd89fffe59ee8dc82f4452f6247 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 18:56:22 +0530 Subject: [PATCH 06/11] docs(mcp): document the remote transport, deprecate stdio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes out the MCP work: the coverage gaps the review left open, and the user-facing docs. Tests: - validateMCPConfig extracted from runRoot so the --url requirement can be tested. Behind os.Exit(1) it could have been rewritten into something weaker — comparing AuthorizerURL to "" rather than asking whether a resource can be derived from it — with the whole suite staying green. - The smoke suite now exercises MCP over HTTP against the real binary and the real route table: 401 with the RFC 9728 challenge, the metadata document the challenge points at, and a login token being refused. This is the only place the --mcp-enabled route registration is covered; every other MCP test mounts the handler onto a router it builds itself, so dropping the route or its flag guard would have left them all green. Docs: - docs/core/mcp.md leads with the remote transport, documents the discovery chain end to end, and states plainly which clients work today: no RFC 7591 dynamic client registration yet, so clients that self-register are not supported and a pre-registered client ID is the path. Claude Code needs both loopback redirect URIs registered. - CHANGELOG gains Added entries for --mcp-enabled and RFC 8252 loopback matching, and a Deprecated entry for `authorizer mcp`. - ROADMAP 4.1 marked delivered, with CIMD/DCR and the consent screen it requires left open. Verified: go build, go vet, make test (42 packages), make lint, make smoke. --- CHANGELOG.md | 6 ++++ README.md | 1 + ROADMAP_V2.md | 2 +- cmd/mcp_config_test.go | 57 ++++++++++++++++++++++++++++++++++++ cmd/root.go | 44 +++++++++++++++++++++------- internal/e2e/smoke_test.go | 60 +++++++++++++++++++++++++++++++++++++- 6 files changed, 157 insertions(+), 13 deletions(-) create mode 100644 cmd/mcp_config_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 40a5e7242..38a031f3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id ### Added +- **Remote MCP server (`--mcp-enabled`)**: Authorizer serves its MCP tool surface over Streamable HTTP at `POST /mcp`, acting as an OAuth 2.1 resource server for itself. Implements RFC 9728 protected resource metadata at `/.well-known/oauth-protected-resource/mcp`, answers an unauthenticated or expired credential with `401` + `WWW-Authenticate: Bearer resource_metadata="…"` (the header MCP clients follow to discover the authorization server, and the status they refresh on), and enforces RFC 8707 audience binding — a token is accepted only when its `aud` is this deployment's canonical `/mcp`, which is the audience every other Authorizer endpoint rejects. Each request carries its own bearer token, so one server serves every caller under their own identity. The surface runs on a dedicated in-process gRPC server that accepts no cookie, no admin secret and no admin operation, so no token can cross between MCP and GraphQL/gRPC/REST. **Requires `--url`**; startup refuses the combination without it, because the audience comparison must not take input from the caller. Off by default. +- **RFC 8252 §7.3 loopback redirect URIs**: `redirect_uri` validation now ignores the port when both the registered and presented URIs are loopback (`127.0.0.1`, `[::1]`, `localhost`). Native apps bind an ephemeral port at run time and cannot register it in advance, so exact matching made loopback redirects unusable. Scheme, host, path and query must still match exactly, and non-loopback redirects are unchanged. - **Unified OAuth Client registry (machine & agent identity foundation)**: All clients (human, machine, agent) are registered in a single `authorizer_clients` table with a `kind` discriminator (`interactive` | `service_account`). Service accounts can use the `client_credentials` grant for machine-to-machine authentication, while agents can participate in delegation chains. Admin GraphQL/gRPC operations manage clients with secret generation (32-byte crypto/rand, bcrypt-12 at rest), scope-subset enforcement, and one-time secret reveal ([#648](https://github.com/authorizerdev/authorizer/pull/648)). - **Machine-to-machine (service-to-service) authentication**: Service account clients use the RFC 6749 §4.4 `client_credentials` grant at `/oauth/token` to mint access tokens for autonomous workloads. Tokens carry `login_method: service_account` and resolve to `service_account:` FGA subjects instead of users. Scope-subset enforcement and timing-safe authentication prevent privilege escalation ([#641](https://github.com/authorizerdev/authorizer/pull/641), [#642](https://github.com/authorizerdev/authorizer/pull/642), [#644](https://github.com/authorizerdev/authorizer/pull/644), [#645](https://github.com/authorizerdev/authorizer/pull/645), [#647](https://github.com/authorizerdev/authorizer/pull/647)). - **Secretless workload identity (RFC 7523 + SPIFFE JWT-SVID + Kubernetes TokenReview)**: Service accounts can authenticate via `client_assertion` (JWT-bearer) with `private_key_jwt` or `jwt-spiffe` assertion types. Trusted issuers validate assertion signatures, pin subject claims, and prevent replay via single-use `jti` in a bounded-TTL cache. When enabled, Kubernetes TokenReview API validates projected ServiceAccount tokens before issuance. All authentication paths share constant-time comparison and SSRF-hardened external fetch for JWKS ([#654](https://github.com/authorizerdev/authorizer/pull/654), [#659](https://github.com/authorizerdev/authorizer/pull/659)). @@ -62,6 +64,10 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id - **BREAKING: `/userinfo` now strictly filters claims by scope per OIDC Core §5.4.** The endpoint returns only `sub` plus the claims permitted by the standard scope groups (`profile`, `email`, `phone`, `address`) encoded in the access token. Previously, `/userinfo` returned the full user object regardless of scopes. Clients that request only the `openid` scope but read profile/email claims from `/userinfo` **must** now request those scopes explicitly. See https://docs.authorizer.dev/core/oauth2-oidc for the full scope→claim mapping. - **OAuth 2.1 standards compliance**: refresh-token reuse detection revokes the user's entire session family on replay (RFC 8707 compliance); `resource` parameter binding on authorization code flow (binds access token `aud` claim); new `--oauth21-strict` flag (default off) gates implicit-grant and PKCE-plain removal behind opt-in. New `GET /.well-known/oauth-authorization-server` thin alias of OIDC discovery for MCP compliance ([#693](https://github.com/authorizerdev/authorizer/pull/693)). +### Deprecated + +- **`authorizer mcp` (stdio transport)** — superseded by `--mcp-enabled`, removed in 2.5.0. The stdio subcommand ran a second copy of every provider (storage, memory store, embedded FGA engine) alongside the real server, and its identity was a single process-wide `--mcp-bearer`, so one process could only ever serve one user. Both are gone with the HTTP transport: the MCP surface shares the running server's providers, and every request carries its own token. The subcommand keeps working and now prints a deprecation notice. + ### Security - **RFC 8707 resource binding now survives token refresh.** An access token minted with a `resource` indicator carries that resource as its `aud`, which is what stops it being replayed at a different resource server. That binding previously survived exactly one token: the refresh grant did not carry the resource forward, so the rotated access token fell back to the client id as its audience and came back **unbound** — usable at Authorizer's own API, which is precisely what the restriction exists to prevent. The resource is now stamped on the refresh token (a reserved claim, so `CUSTOM_ACCESS_TOKEN_SCRIPT` cannot forge it) and carried across every rotation. A refresh request that names a *different* resource is rejected with `invalid_target` (RFC 8707 §2.2 permits restricting the resource, never switching it). Grants that never used a resource indicator are unaffected — their tokens keep the client id as the audience and their claim set is unchanged. diff --git a/README.md b/README.md index 98262bc78..17e5e189c 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ For production builds, tests, and Docker, see [Getting Started](#getting-started - ✅ SCIM 2.0 user and group provisioning with RFC 7644 compliance - ✅ Multi-tenant / org-scoped admin roles and isolation - ✅ GraphQL, REST, gRPC, and MCP APIs (all transports share the same service layer for public auth operations) +- ✅ Remote MCP server for AI agents — OAuth 2.1 protected, RFC 9728 discovery, RFC 8707 audience-bound tokens - ✅ Admin API — user management, webhooks, email templates, audit logs, and FGA model/tuples over GraphQL, gRPC, and REST transports - ✅ Rate limiting and security hardening (CSRF, CORS, HSTS, CSP, trusted proxies) - ✅ Prometheus metrics and health/readiness endpoints diff --git a/ROADMAP_V2.md b/ROADMAP_V2.md index 017b4d4fb..118d95232 100644 --- a/ROADMAP_V2.md +++ b/ROADMAP_V2.md @@ -366,7 +366,7 @@ Human approval and safe third-party access. - *Unlocks:* human-in-the-loop, agents calling Google/Slack/etc. on a user's behalf. ### Wave 4 — Enterprise hardening -- [ ] **MCP authorization** (OAuth 2.1 + RFC 9728 + RFC 8707) (4.1) and **ID-JAG / Cross-App Access** for enterprise-managed MCP. +- [x] **MCP authorization** (OAuth 2.1 + RFC 9728 + RFC 8707) (4.1) — delivered in 2.4.0 as `--mcp-enabled`: Authorizer is both the authorization server and the resource server for its own MCP surface. Still open: RFC 7591 dynamic client registration / CIMD for zero-touch client onboarding (and the `/authorize` consent screen CIMD requires), plus **ID-JAG / Cross-App Access** for enterprise-managed MCP. - [ ] **JIT / time-bound grants** (TTL tuples), **per-agent guardrails** (spend/rate limits), **consent management**. - *Unlocks:* enterprise-managed agent deployments at scale. diff --git a/cmd/mcp_config_test.go b/cmd/mcp_config_test.go new file mode 100644 index 000000000..737ba5491 --- /dev/null +++ b/cmd/mcp_config_test.go @@ -0,0 +1,57 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" +) + +// TestValidateMCPConfig guards the startup refusal that MCP's entire audience +// model rests on. +// +// A token is accepted at /mcp only if its `aud` equals this deployment's +// canonical /mcp. Derive that identifier from request headers — which is +// what parsers.GetHost does when --url is unset, falling back to +// X-Authorizer-URL, X-Forwarded-Host, then Host — and the caller supplies both +// sides of the comparison. Refusing to start is what makes the check mean +// something. +// +// The unusable-URL cases matter as much as the empty one: MCPResource() returns +// "" for a scheme-less or non-http --url too, so starting anyway would produce a +// surface that is enabled, advertises nothing (the metadata handler 404s) and +// rejects every token — broken in the direction that reports success. +func TestValidateMCPConfig(t *testing.T) { + t.Run("MCP disabled never fails, whatever --url says", func(t *testing.T) { + for _, u := range []string{"", "not-a-url", "https://auth.example.com"} { + require.NoError(t, validateMCPConfig(&config.Config{MCPEnabled: false, AuthorizerURL: u}), + "the guard must not affect deployments that do not run MCP") + } + }) + + t.Run("MCP enabled with a usable --url starts", func(t *testing.T) { + for _, u := range []string{ + "https://auth.example.com", + "https://auth.example.com/", // trailing slash is normalised away + "http://localhost:8080", // local development + } { + assert.NoError(t, validateMCPConfig(&config.Config{MCPEnabled: true, AuthorizerURL: u}), u) + } + }) + + t.Run("MCP enabled without a usable --url refuses to start", func(t *testing.T) { + for _, u := range []string{ + "", // the shipped default + "auth.example.com", // scheme omitted — the likely mistake + "ftp://auth.example.com", // not an http origin + "https://user:pw@auth.example.com", // userinfo + } { + err := validateMCPConfig(&config.Config{MCPEnabled: true, AuthorizerURL: u}) + require.Error(t, err, "--url %q cannot yield a resource identifier, so MCP must not start", u) + assert.Contains(t, err.Error(), "--url", + "the message must name the flag an operator has to set") + } + }) +} diff --git a/cmd/root.go b/cmd/root.go index 71cde37f8..b37350157 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -414,17 +414,8 @@ func runRoot(c *cobra.Command, args []string) { } } - // MCP's entire security model is the audience check: a token is accepted at - // /mcp only if its `aud` equals this deployment's canonical /mcp. Without - // --url that URL would be derived from request headers (parsers.GetHost falls - // back to X-Authorizer-URL / X-Forwarded-Host), which means the caller would - // get to state the audience their own token must match — no check at all. - // Refuse the combination rather than serve an endpoint that looks - // authenticated and is not. - if rootArgs.config.MCPEnabled && rootArgs.config.MCPResource() == "" { - fmt.Fprintln(os.Stderr, "--mcp-enabled requires a valid --url (e.g. https://auth.example.com): "+ - "the MCP resource identifier that access tokens are bound to is derived from it, and "+ - "deriving it from request headers instead would let a caller choose their own audience") + if err := validateMCPConfig(&rootArgs.config); err != nil { + fmt.Fprintln(os.Stderr, err.Error()) os.Exit(1) } @@ -839,3 +830,34 @@ func runRoot(c *cobra.Command, args []string) { } log.Info().Msg("Application terminated") } + +// validateMCPConfig refuses a configuration that would enable MCP without a +// usable canonical URL. +// +// This is the second lock on the audience door, and it is not optional. MCP's +// entire security model is one comparison: a token is accepted at /mcp only if +// its `aud` equals this deployment's canonical /mcp. Without --url that +// identifier would be derived from request headers — parsers.GetHost falls back +// to X-Authorizer-URL, then X-Forwarded-Host, then Host — so the caller would be +// supplying both sides of the comparison and there would be no check at all. +// +// It also rejects a --url that is merely unusable (no scheme, userinfo, a +// non-http scheme), because MCPResource() returns empty for those too. Starting +// anyway would produce a surface that is enabled, advertises nothing, and +// rejects every token: broken in a way that reports success. +// +// Extracted from runRoot so it can be tested. The inline version behind +// os.Exit(1) could be rewritten into something weaker — comparing AuthorizerURL +// to "" instead of asking whether a resource can be derived from it — with the +// whole suite staying green. +func validateMCPConfig(cfg *config.Config) error { + if !cfg.MCPEnabled { + return nil + } + if cfg.MCPResource() == "" { + return fmt.Errorf("--mcp-enabled requires a valid --url (e.g. https://auth.example.com): " + + "the MCP resource identifier that access tokens are bound to is derived from it, and " + + "deriving it from request headers instead would let a caller choose their own audience") + } + return nil +} diff --git a/internal/e2e/smoke_test.go b/internal/e2e/smoke_test.go index 3daa55460..905df4479 100644 --- a/internal/e2e/smoke_test.go +++ b/internal/e2e/smoke_test.go @@ -80,6 +80,11 @@ func TestReleaseSmoke(t *testing.T) { // withhold signup's token behind the MFA-setup gate instead of // returning it directly. "--disable-mfa", + // MCP over HTTP. --url is mandatory with it: the audience every MCP + // token is checked against is derived from --url alone, so the binary + // refuses to start without one. + "--mcp-enabled", + "--url=" + baseURL, } stopServer := startServer(t, bin, serverArgs, baseURL) @@ -247,7 +252,55 @@ func TestReleaseSmoke(t *testing.T) { assert.NotEmpty(t, res.AdminMeta.Roles) }) - // --- Surface 4: MCP (stdio subprocess) ------------------------------- + // --- Surface 4: MCP over HTTP ---------------------------------------- + // Runs against the REAL binary and the REAL route table, which is the only + // place the --mcp-enabled route registration is actually exercised: every + // other MCP test mounts the handler onto a router it builds itself, so a + // refactor that dropped the route (or the flag guard in front of it) would + // leave them all green. + t.Run("mcp http", func(t *testing.T) { + metadataURL := baseURL + "/.well-known/oauth-protected-resource/mcp" + + // RFC 9728 §5.1: an unauthenticated call must point the client at the + // metadata document. This is the entry point of the whole discovery + // chain — without it a fresh client has no way to learn where to + // authenticate. + resp, err := http.Post(baseURL+"/mcp", "application/json", strings.NewReader(mcpInitializeRPC)) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Contains(t, resp.Header.Get("WWW-Authenticate"), `resource_metadata="`+metadataURL+`"`) + + // The metadata document the challenge points at must exist and name the + // same resource identifier clients will send as `resource`. + metaResp, err := http.Get(metadataURL) + require.NoError(t, err) + defer func() { _ = metaResp.Body.Close() }() + require.Equal(t, http.StatusOK, metaResp.StatusCode) + var prm struct { + Resource string `json:"resource"` + AuthorizationServers []string `json:"authorization_servers"` + } + require.NoError(t, json.NewDecoder(metaResp.Body).Decode(&prm)) + assert.Equal(t, baseURL+"/mcp", prm.Resource) + assert.Equal(t, []string{baseURL}, prm.AuthorizationServers) + + // A login token authenticates GraphQL, REST and gRPC in this same test — + // it must not authenticate MCP. That is the audience boundary, observed + // from outside the process. + req, err := http.NewRequest(http.MethodPost, baseURL+"/mcp", strings.NewReader(mcpInitializeRPC)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "Bearer "+token) + wrongAud, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = wrongAud.Body.Close() }() + assert.Equal(t, http.StatusUnauthorized, wrongAud.StatusCode, + "a token minted for the client, not for /mcp, must be refused") + }) + + // --- Surface 5: MCP (stdio subprocess, deprecated) -------------------- // The MCP subcommand is a separate process sharing the sqlite store, so // stop the server first to avoid two writers on one sqlite file. stopServer() @@ -307,6 +360,11 @@ func TestReleaseSmoke(t *testing.T) { }) } +// mcpInitializeRPC is the MCP handshake a client sends first. +const mcpInitializeRPC = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` + + `"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"release-smoke","version":"1.0"}}}` + // buildBinary compiles the authorizer binary into a temp dir and returns its // path. Building from source guarantees the smoke run tests exactly the code // under release, not a stale artifact. From fbea334e8f41ad5feab12e58b16a9e543cea7f7b Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 19:26:24 +0530 Subject: [PATCH 07/11] fix(oauth): carry the RFC 8707 resource through the login redirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the new Playwright spec, which is the only test that drives a real browser through a first-time connection. When /authorize is reached WITHOUT a live session it does not issue a code — it redirects to the login UI with the request's parameters in the query, and the SPA replays them on a second /authorize once the user signs in. `resource` was not in that string, so the replayed request was unbound and the access token's `aud` fell back to the client id. The symptom was as bad as the bug. The flow completed, a token came back, and only its audience was wrong, so /mcp rejected every call with a 401 that reads like a credential problem. It only affected users who were NOT already signed in, so retrying after logging in elsewhere appeared to fix it. No Go test caught it: every integration test pre-establishes a session cookie, which skips this branch entirely. Both the browser spec and a Go regression test that deliberately omits the cookie now cover it. Also adds the e2e-playground MCP spec: the discovery challenge, the metadata document (including that the bare well-known path stays 404 per RFC 9728 §3.3), a full browser OAuth flow with `resource`, that the binding survives refresh, and that an MCP token is still refused at /userinfo. The compose stack gains --mcp-enabled. Note: e2e-playground/ is listed in this clone's .git/info/exclude, so the spec needed `git add -f`. Anyone else adding e2e specs on a similarly-configured clone will hit the same silent drop. --- e2e-playground/docker-compose.yml | 4 + e2e-playground/tests/mcp.spec.ts | 217 ++++++++++++++++++ internal/http_handlers/authorize.go | 16 ++ .../integration_tests/oauth21_mcp_test.go | 60 +++++ 4 files changed, 297 insertions(+) create mode 100644 e2e-playground/tests/mcp.spec.ts diff --git a/e2e-playground/docker-compose.yml b/e2e-playground/docker-compose.yml index 3d61a244e..b7c0b5330 100644 --- a/e2e-playground/docker-compose.yml +++ b/e2e-playground/docker-compose.yml @@ -17,6 +17,10 @@ services: - "--client-id=e2e-client-id" - "--client-secret=e2e-client-secret" - "--enable-signup=true" + # Remote MCP surface. --url above is what makes it legal: every token + # presented at /mcp is checked against /mcp, so the server refuses + # to start with --mcp-enabled and no --url. + - "--mcp-enabled" - "--app-cookie-secure=false" - "--admin-cookie-secure=false" - "--app-cookie-same-site=lax" diff --git a/e2e-playground/tests/mcp.spec.ts b/e2e-playground/tests/mcp.spec.ts new file mode 100644 index 000000000..36b204a28 --- /dev/null +++ b/e2e-playground/tests/mcp.spec.ts @@ -0,0 +1,217 @@ +// e2e-playground/tests/mcp.spec.ts +import { test, expect } from '@playwright/test'; +import { GraphQLClient, gql } from 'graphql-request'; +import crypto from 'node:crypto'; + +const BASE_URL = process.env.AUTHORIZER_BASE_URL || 'http://localhost:8080'; + +// The canonical MCP resource identifier is derived from the server's --url, and +// this suite runs inside the compose network (docker compose run playwright) so +// AUTHORIZER_BASE_URL is the same http://authorizer:8080 the server is +// configured with. Deriving it here rather than hardcoding keeps the spec +// honest if either value moves. +const MCP_ISSUER = BASE_URL; +const MCP_RESOURCE = `${BASE_URL}/mcp`; + +const client = new GraphQLClient(`${BASE_URL}/graphql`, { headers: { Origin: BASE_URL } }); + +function randomEmail() { + return `mcp-${crypto.randomUUID()}@example.com`; +} + +const INITIALIZE_RPC = { + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'e2e-playground', version: '1.0' }, + }, +}; + +const MCP_HEADERS = { + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', +}; + +test.describe('MCP — remote server', () => { + test('an unauthenticated call returns the challenge that starts discovery', async ({ request }) => { + const res = await request.post('/mcp', { headers: MCP_HEADERS, data: INITIALIZE_RPC }); + + expect(res.status()).toBe(401); + + // RFC 9728 §5.1. This pointer is the only thing telling a client that has + // never seen this deployment where to authenticate; without it the + // connection fails with nothing to diagnose. + const challenge = res.headers()['www-authenticate']; + expect(challenge).toContain( + `resource_metadata="${MCP_ISSUER}/.well-known/oauth-protected-resource/mcp"`, + ); + // RFC 6750 §3: `error` describes a credential that was supplied and + // rejected. A first contact carrying nothing is not an error. + expect(challenge).not.toContain('error='); + }); + + test('the metadata document names the resource clients must bind tokens to', async ({ request }) => { + const res = await request.get('/.well-known/oauth-protected-resource/mcp'); + expect(res.status()).toBe(200); + + const doc = await res.json(); + expect(doc.resource).toBe(MCP_RESOURCE); + expect(doc.authorization_servers).toEqual([MCP_ISSUER]); + expect(doc.bearer_methods_supported).toEqual(['header']); + // A client that requests only what this document advertises must still be + // able to obtain a refresh token, or the agent's session dies at the first + // access-token expiry. + expect(doc.scopes_supported).toContain('offline_access'); + + // RFC 9728 §3.1 inserts the well-known segment ahead of the resource + // identifier's PATH, so the bare origin form denotes a DIFFERENT resource + // and §3.3 has clients reject a document whose `resource` does not match + // what they asked for. Serving it there too would hand strict clients a + // mismatch, so it must not exist. + const bare = await request.get('/.well-known/oauth-protected-resource'); + expect(bare.status()).toBe(404); + }); + + test('a browser OAuth flow yields a token that works at /mcp, survives refresh, and is scoped to MCP alone', async ({ + page, + request, + }) => { + const clientId = 'e2e-client-id'; + const redirectUri = `${BASE_URL}/e2e-mcp-callback`; + const email = randomEmail(); + const password = 'Str0ngPassw0rd!'; + + const signup = gql` + mutation ($params: SignUpRequest!) { + signup(params: $params) { message } + } + `; + await client.request(signup, { params: { email, password, confirm_password: password } }); + + const codeVerifier = crypto.randomBytes(32).toString('base64url'); + const codeChallenge = crypto.createHash('sha256').update(codeVerifier).digest('base64url'); + const state = crypto.randomUUID(); + + const authorizeUrl = new URL('/authorize', BASE_URL); + authorizeUrl.searchParams.set('response_type', 'code'); + authorizeUrl.searchParams.set('client_id', clientId); + authorizeUrl.searchParams.set('redirect_uri', redirectUri); + // offline_access so the grant yields a refresh token — the refresh + // assertion below is the whole reason this test drives a real flow. + authorizeUrl.searchParams.set('scope', 'openid offline_access'); + authorizeUrl.searchParams.set('state', state); + authorizeUrl.searchParams.set('code_challenge', codeChallenge); + authorizeUrl.searchParams.set('code_challenge_method', 'S256'); + // RFC 8707: this is what binds the issued access token's `aud` to the MCP + // server, and it is what /mcp checks. + authorizeUrl.searchParams.set('resource', MCP_RESOURCE); + + await page.goto(authorizeUrl.toString()); + + await page.locator('#authorizer-login-email-or-phone-number').fill(email); + await page.locator('#authorizer-login-password').fill(password); + await page.locator('form[name="authorizer-login-form"] button[type="submit"]').click(); + + // First login for a new user hits the optional MFA-setup offer; the token + // is withheld until a factor is added or skipped. Tolerates the screen not + // appearing at all. + await page + .getByRole('button', { name: 'Skip for now' }) + .click({ timeout: 10_000 }) + .catch(() => {}); + + await page.waitForURL( + (url) => url.origin === BASE_URL && url.pathname === '/e2e-mcp-callback' && url.searchParams.has('code'), + ); + const code = new URL(page.url()).searchParams.get('code')!; + expect(code).toBeTruthy(); + + const tokenRes = await request.post('/oauth/token', { + form: { + grant_type: 'authorization_code', + code, + client_id: clientId, + redirect_uri: redirectUri, + code_verifier: codeVerifier, + resource: MCP_RESOURCE, + }, + }); + expect(tokenRes.status()).toBe(200); + const tokens = await tokenRes.json(); + expect(tokens.access_token).toBeTruthy(); + expect(tokens.refresh_token).toBeTruthy(); + + // --- the token reaches the tool surface --------------------------------- + const callMCP = (bearer: string, body: unknown) => + request.post('/mcp', { + headers: { ...MCP_HEADERS, Authorization: `Bearer ${bearer}` }, + data: body, + }); + + const init = await callMCP(tokens.access_token, INITIALIZE_RPC); + expect(init.status(), await init.text()).toBe(200); + + const tools = await callMCP(tokens.access_token, { jsonrpc: '2.0', id: 2, method: 'tools/list' }); + expect(tools.status()).toBe(200); + const toolNames = (await tools.json()).result.tools.map((t: { name: string }) => t.name); + expect(toolNames).toContain('profile'); + + // --- the binding survives refresh --------------------------------------- + // Before the resource was carried across rotation, the refreshed token's + // `aud` fell back to the client id and this call returned 401 forever. + // Access tokens live 30 minutes and MCP clients refresh proactively, so the + // failure only ever appeared in production, long after the flow was tested + // by hand. + const refreshRes = await request.post('/oauth/token', { + form: { grant_type: 'refresh_token', refresh_token: tokens.refresh_token, client_id: clientId }, + }); + expect(refreshRes.status()).toBe(200); + const refreshed = await refreshRes.json(); + expect(refreshed.access_token).toBeTruthy(); + expect(refreshed.access_token).not.toBe(tokens.access_token); + + const afterRefresh = await callMCP(refreshed.access_token, INITIALIZE_RPC); + expect(afterRefresh.status(), await afterRefresh.text()).toBe(200); + + // --- and it buys nothing outside MCP ------------------------------------ + // The other half of the audience boundary: a token scoped to the MCP server + // must not double as a first-party API credential, or handing one to a + // semi-trusted agent would hand over the whole account. + const userinfo = await request.get('/userinfo', { + headers: { Authorization: `Bearer ${refreshed.access_token}` }, + }); + expect(userinfo.status()).toBe(401); + }); + + test('an ordinary login token cannot authenticate MCP', async ({ request }) => { + // The inverse direction, and the one a client is most likely to try by + // accident: the token every SDK already holds must not open this surface. + const email = randomEmail(); + const password = 'Str0ngPassw0rd!'; + + const signup = gql` + mutation ($params: SignUpRequest!) { + signup(params: $params) { message access_token } + } + `; + const res = await client.request<{ signup: { access_token: string | null } }>(signup, { + params: { email, password, confirm_password: password }, + }); + + // MFA is on by default in this stack, which withholds signup's token. Skip + // rather than assert a weaker thing: the audience boundary is covered from + // the token side above, and a silently-empty bearer here would make this + // assertion pass for the wrong reason. + test.skip(!res.signup.access_token, 'signup token withheld behind MFA setup'); + + const mcp = await request.post('/mcp', { + headers: { ...MCP_HEADERS, Authorization: `Bearer ${res.signup.access_token}` }, + data: INITIALIZE_RPC, + }); + expect(mcp.status()).toBe(401); + expect(mcp.headers()['www-authenticate']).toContain('error="invalid_token"'); + }); +}); diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index 1009a80a8..5e459637f 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -390,6 +390,22 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { authState += "&code_challenge=" + url.QueryEscape(codeChallenge) authState += "&code_challenge_method=" + url.QueryEscape(codeChallengeMethod) } + // The RFC 8707 resource indicator has to survive this round-trip too, + // and its absence failed silently in the worst way: the flow completed, + // a token was issued, and only its `aud` was wrong — the client id + // instead of the resource the client asked for. The resource server then + // rejected every call with a 401 that looked like a credential problem. + // + // It only broke for users who were NOT already signed in, because a live + // session skips this branch entirely. So the first connection failed and + // a retry after logging in elsewhere succeeded, which is close to the + // worst possible symptom to debug. + // + // Already validated as an absolute URI without a fragment above; escaped + // here for the same reason every other value is. + if resource != "" { + authState += "&resource=" + url.QueryEscape(resource) + } if hasCodeFlow { authState += "&code=" + code diff --git a/internal/integration_tests/oauth21_mcp_test.go b/internal/integration_tests/oauth21_mcp_test.go index 9a5d8c967..fa5e28c86 100644 --- a/internal/integration_tests/oauth21_mcp_test.go +++ b/internal/integration_tests/oauth21_mcp_test.go @@ -381,3 +381,63 @@ func TestOAuth21_AuthServerMetadata(t *testing.T) { ts.Config.OAuth21Strict = false } + +// TestAuthorizeCarriesResourceThroughLogin pins that the RFC 8707 resource +// indicator survives the login round-trip. +// +// When /authorize is reached WITHOUT a live session it does not issue a code — +// it redirects to the login UI with the request's parameters encoded in the +// query, and the SPA replays them on a second /authorize call once the user has +// signed in. Every parameter that must reach the second call has to be in that +// string; `resource` was not. +// +// The consequence was invisible in exactly the way that costs the most time. The +// flow completed, a token came back, and only its `aud` was wrong — the client +// id instead of the resource server — so the MCP endpoint rejected every call +// with a 401 that read like a credential problem. And it only happened to users +// who were not already signed in, so retrying after logging in elsewhere +// "fixed" it. +// +// No integration test caught it because they all pre-establish a session cookie, +// which skips this branch entirely. This one deliberately does not. +func TestAuthorizeCarriesResourceThroughLogin(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + router := gin.New() + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + + const resource = "https://auth.example.com/mcp" + qs := url.Values{} + qs.Set("response_type", "code") + qs.Set("client_id", cfg.ClientID) + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("state", "st") + qs.Set("response_mode", "query") + qs.Set("scope", "openid") + qs.Set("code_challenge", s256Challenge("a-verifier-long-enough-to-be-valid-0000000000")) + qs.Set("code_challenge_method", "S256") + qs.Set("resource", resource) + + // No session cookie: this is a first-time connection, the case that broke. + w := httptest.NewRecorder() + req, err := http.NewRequest(http.MethodGet, "/authorize?"+qs.Encode(), nil) + require.NoError(t, err) + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusFound, w.Code, "an unauthenticated /authorize must redirect to the login UI: %s", w.Body.String()) + + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + // The login UI receives the parameters as a query string it replays. + carried, err := url.ParseQuery(loc.RawQuery) + require.NoError(t, err) + + assert.Equal(t, resource, carried.Get("resource"), + "the resource indicator must survive the login redirect, or the second /authorize mints an unbound token") + // Sanity that this assertion is reading the right string: parameters known + // to be forwarded are present too, so a future refactor that moved the + // payload elsewhere fails loudly instead of silently passing. + assert.Equal(t, cfg.ClientID, carried.Get("client_id")) + assert.NotEmpty(t, carried.Get("code_challenge")) +} From cdc71b0828615b5d4c3ed3134cab1cc36d63cb7a Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 19:36:24 +0530 Subject: [PATCH 08/11] security: fix cross-backend liveness, loopback URI, admin bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final review pass. Four fixes, one of them the most serious defect in the branch. 1. subjectLiveness was gated on storage.IsNotFound to decide whether to try the CLIENT lookup — but DynamoDB's GetUserByID returns a bare errors.New("no documets found") and Couchbase's returns gocb.ErrNoResult from the query path, and neither satisfies storage.IsNotFound. A machine token's subject is a client row id, so the user lookup ALWAYS misses; on those two backends it short-circuited and the client lookup never ran. That broke both directions at once. Delegated tokens whose subject is a service account — the agent-A-delegates-to-agent-B chain — were rejected outright, a regression this branch introduced. And deactivating a service account stopped revoking its live tokens, which is the headline fix of 138f98c4 silently doing nothing. CI runs SQLite only, so every test stayed green. The client lookup now runs whenever the user lookup did not POSITIVELY find a user, and absence is only CONFIRMED when both lookups say so. Correctness no longer depends on every backend spelling not-found the same way; a backend that does not degrades to "unknown", which callers already handle. 2. redirectURIMatches compared scheme/host/path/query and silently dropped fragment and userinfo, so everything omitted became a free field. A presented "http://127.0.0.1:9/cb#x" matched and then the response is appended by string concatenation, producing ".../cb#x?code=…" — the entire authorization response inside the fragment, so the app's loopback listener sees no code, no state and no error and the login hangs forever. RFC 6749 §3.1.2 forbids a fragment there. "http://evil.com@127.0.0.1/callback" also matched. Both are now rejected outright rather than compared. 3. The sole-authority admin refusal ran AFTER the `public` bypass, and AdminLogin is both admin and public — so the RPC that MINTS super-admin authority stayed reachable on the CSRF-exempt MCP surface. Moved ahead of the bypass. 4. The refresh resource check rejected any supplied `resource` when the grant carried none, which every refresh token minted before this branch does. The MCP spec has clients send `resource` on every token request, so upgrading a deployment would have turned every in-flight refresh into a permanent invalid_target. Now enforced only when the grant was bound, matching the authorization_code branch; an unbound grant stays unbound rather than letting a refresh add an audience nobody authorized. Verified: go build, go vet, make test (42 packages), make lint, make smoke, and the e2e-playground MCP spec. KNOWN GAP, not fixed here: DynamoDB's and Couchbase's GetUserByID violate the not-found contract AGENTS.md documents. Fix (1) makes this code robust to that, but the providers should still be corrected and TestNotFoundContractIsUniform extended to cover them, since other callers of storage.IsNotFound have the same exposure. --- internal/grpcsrv/interceptors/auth.go | 31 ++++++------ internal/http_handlers/authorize.go | 18 +++++++ .../redirect_uri_loopback_test.go | 7 +++ internal/http_handlers/token.go | 12 ++++- internal/token/delegated_access_token.go | 47 ++++++++++++------- 5 files changed, 80 insertions(+), 35 deletions(-) diff --git a/internal/grpcsrv/interceptors/auth.go b/internal/grpcsrv/interceptors/auth.go index c1d42e25e..ec0c1d5af 100644 --- a/internal/grpcsrv/interceptors/auth.go +++ b/internal/grpcsrv/interceptors/auth.go @@ -107,6 +107,15 @@ func Auth(tp token.Provider, log *zerolog.Logger, resolve TokenResolver) grpc.Un // must NOT skip admin auth — it falls through to the super-admin check // below (mirroring the Session RPC's explicit service guard, closing the // latent footgun where a future admin RPC is accidentally made public). + // A resolver-governed server does not serve the admin surface at all, + // and this has to be decided BEFORE the `public` bypass below. AdminLogin + // is both an admin RPC and `public`, so the bypass would hand it through + // untouched — leaving the one RPC that MINTS super-admin authority + // reachable on an internet-facing, CSRF-exempt surface, which is the + // opposite of what refusing the admin service is for. + if resolverIsSoleAuthority && serviceName == adminServiceName { + return nil, status.Error(codes.Unauthenticated, "unauthorized") + } if isPublicMethod(methodDesc) && (serviceName == publicServiceName || (serviceName == adminServiceName && string(methodDesc.Name()) == adminLoginMethodName)) { @@ -120,24 +129,12 @@ func Auth(tp token.Provider, log *zerolog.Logger, resolve TokenResolver) grpc.Un gc := &gin.Context{Request: meta.Request} if serviceName == adminServiceName { - // A resolver-governed surface does not serve the admin API at all. - // - // Skipping the IsSuperAdmin check here is NOT enough on its own: - // service.requireSuperAdmin re-derives super-admin from meta.Request - // (admin_provider.go), reading the admin cookie or the - // x-authorizer-admin-secret header that transport.MetaFromGRPC - // reconstructs from gRPC metadata. Disabling the check at this layer - // would only move it one layer down, so a caller holding a valid - // MCP-audience token plus an admin credential would still reach - // platform-wide operations on an internet-facing, CSRF-exempt - // surface. Refusing the whole service is the only version of this - // guard that actually holds, and it costs nothing: no admin RPC is - // mcp_tool-exposed, so nothing legitimate is being turned off. - if resolverIsSoleAuthority { - return nil, status.Error(codes.Unauthenticated, "unauthorized") - } // Platform super-admin: unchanged, and still the only identity that - // reaches the platform-wide operations. + // reaches the platform-wide operations. A resolver-governed server + // never gets here — the whole admin service is refused above, which + // is the only version of that guard that holds: merely skipping this + // check would move it one layer down, since service.requireSuperAdmin + // re-derives super-admin from meta.Request on its own. if tp.IsSuperAdmin(gc) { ctx = authctx.WithPrincipal(ctx, &authctx.Principal{IsSuperAdmin: true}) return handler(ctx, req) diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index 5e459637f..f53f8dfd6 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -1088,6 +1088,24 @@ func redirectURIMatches(registered, presented string) bool { if !isLoopbackHost(r.Hostname()) || !isLoopbackHost(p.Hostname()) { return false } + // Fragment and userinfo are rejected outright rather than compared, because + // comparing only scheme/host/path/query would silently accept URIs the exact + // match rejected — everything the comparison omits becomes a free field. + // + // A fragment is the worse of the two. RFC 6749 §3.1.2 forbids one on the + // redirection endpoint, and the response is appended by string + // concatenation: a presented "http://127.0.0.1:9/cb#x" carries no "?", so + // the result is ".../cb#x?code=…&state=…" and the entire authorization + // response lands inside the fragment. The app's loopback listener then + // receives a request with no code, no state and no error, and the login + // hangs forever instead of failing cleanly. + // + // Userinfo is the phishing shape: "http://evil.com@127.0.0.1/callback" reads + // as evil.com to a human skimming a consent screen while resolving to + // loopback. + if r.Fragment != "" || p.Fragment != "" || r.User != nil || p.User != nil { + return false + } return r.Scheme == p.Scheme && r.Hostname() == p.Hostname() && r.Path == p.Path && diff --git a/internal/http_handlers/redirect_uri_loopback_test.go b/internal/http_handlers/redirect_uri_loopback_test.go index 2ea471e2c..2eb045d15 100644 --- a/internal/http_handlers/redirect_uri_loopback_test.go +++ b/internal/http_handlers/redirect_uri_loopback_test.go @@ -39,6 +39,13 @@ func TestRedirectURIMatches(t *testing.T) { {"query must still match", "http://127.0.0.1/cb?a=1", "http://127.0.0.1:9/cb?a=2", false}, {"localhost and 127.0.0.1 are different names", "http://localhost/cb", "http://127.0.0.1:9/cb", false}, + // Fragment and userinfo are rejected, not compared. Comparing only + // scheme/host/path/query would make everything omitted a free field. + {"presented fragment is rejected", "http://127.0.0.1/callback", "http://127.0.0.1:9/callback#x", false}, + {"registered fragment is rejected", "http://127.0.0.1/callback#x", "http://127.0.0.1:9/callback#x", false}, + {"presented userinfo is rejected", "http://127.0.0.1/callback", "http://evil.com@127.0.0.1:9/callback", false}, + {"registered userinfo is rejected", "http://evil.com@127.0.0.1/callback", "http://127.0.0.1:9/callback", false}, + // A hostname that merely contains a loopback name is not loopback. {"lookalike host is not loopback", "http://127.0.0.1/cb", "http://127.0.0.1.evil.com:9/cb", false}, {"localhost subdomain is not loopback", "http://localhost/cb", "http://localhost.evil.com:9/cb", false}, diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 2a3d5f451..c50121e7d 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -698,7 +698,17 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { // PostFormArray, not PostForm: a repeated parameter must be rejected // rather than silently resolved to the first value — same reasoning as // the authorization_code branch above. - if requestResources := gc.PostFormArray("resource"); len(requestResources) > 0 { + // + // Enforced only when the grant WAS bound, mirroring the + // authorization_code branch above. A refresh token minted before this + // change carries no `resource` claim, and the MCP spec has clients + // send `resource` on every token request including refresh — so + // comparing against an empty binding would turn every pre-upgrade + // refresh into a permanent invalid_target the moment the deployment + // upgraded. An unbound grant stays unbound: the supplied value is + // ignored rather than honoured, because letting a refresh ADD a + // binding would let a client mint an audience nobody authorized. + if requestResources := gc.PostFormArray("resource"); len(requestResources) > 0 && boundResource != "" { if len(requestResources) != 1 || strings.TrimSpace(requestResources[0]) != boundResource { metrics.RecordSecurityEvent("refresh_resource_mismatch", "token_endpoint") log.Warn().Msg("rejected: resource parameter does not match the resource bound to this grant") diff --git a/internal/token/delegated_access_token.go b/internal/token/delegated_access_token.go index bbe6d67a1..2ba789502 100644 --- a/internal/token/delegated_access_token.go +++ b/internal/token/delegated_access_token.go @@ -260,37 +260,50 @@ func (p *provider) subjectLiveness(gc *gin.Context, subject string) (live, known return false, false } - user, err := p.dependencies.StorageProvider.GetUserByID(gc, subject) - switch { - case err == nil && user != nil: + user, userErr := p.dependencies.StorageProvider.GetUserByID(gc, subject) + if userErr == nil && user != nil { if user.RevokedTimestamp != nil { p.dependencies.Log.Debug().Str("subject", subject). Msg("token rejected: subject user is revoked") return false, true } return true, true - case !storage.IsNotFound(err): - p.dependencies.Log.Debug().Err(err).Str("subject", subject). - Msg("subject liveness undetermined: user lookup failed") - return false, false } - client, err := p.dependencies.StorageProvider.GetClientByID(gc, subject) - switch { - case err == nil && client != nil: + // The client lookup is attempted whenever the user lookup did not POSITIVELY + // find a user — never gated on the user error being a recognisable + // not-found. + // + // That distinction is the whole correctness of this function across + // backends. A machine token's subject is a client row id, so the user lookup + // always misses; if a miss on some backend produced an unrecognised error + // and short-circuited here, the client lookup would never run. On DynamoDB + // GetUserByID returns a bare errors.New("no documets found") and on + // Couchbase a gocb.ErrNoResult from the query path — neither satisfies + // storage.IsNotFound. Gating on it therefore broke both directions at once + // on exactly those two backends: delegated tokens with service-account + // subjects were rejected outright, and deactivating a service account + // stopped revoking its live tokens. CI runs SQLite only, so nothing failed. + client, clientErr := p.dependencies.StorageProvider.GetClientByID(gc, subject) + if clientErr == nil && client != nil { if !client.IsActive { p.dependencies.Log.Debug().Str("subject", subject). Msg("token rejected: subject service account is deactivated") return false, true } return true, true - case !storage.IsNotFound(err): - p.dependencies.Log.Debug().Err(err).Str("subject", subject). - Msg("subject liveness undetermined: client lookup failed") - return false, false } - p.dependencies.Log.Debug().Str("subject", subject). - Msg("token rejected: subject resolves to neither an active user nor an active client") - return false, true + // Neither table resolved the subject. Absence is only CONFIRMED when BOTH + // lookups said "no such row"; if either was merely inconclusive the honest + // answer is that we do not know, and callers choose what that means. + if storage.IsNotFound(userErr) && storage.IsNotFound(clientErr) { + p.dependencies.Log.Debug().Str("subject", subject). + Msg("token rejected: subject resolves to neither an active user nor an active client") + return false, true + } + + p.dependencies.Log.Debug().AnErr("user_lookup", userErr).AnErr("client_lookup", clientErr). + Str("subject", subject).Msg("subject liveness undetermined") + return false, false } From f0ba9a61e848e4560b12d60a99b2905ed5aee347 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 20:38:49 +0530 Subject: [PATCH 09/11] fix(storage): make not-found recognisable on dynamodb and couchbase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The root cause behind the subjectLiveness gate, fixed at the layer that owns it. AGENTS.md states the not-found contract is uniform across all six backends, and storage.IsNotFound is what callers use to tell "no such row" from "the query failed". Two backends did not honour it: - DynamoDB's getItemByHash returned a bare fmt.Errorf("no record found") for an absent item, and GetUserByID then replaced ANY error from it with errors.New("no documets found") — so absence was unrecognisable AND a genuine outage was reported as a missing user, which is the contract violated in both directions at once. - Couchbase's IsNotFound matched only gocb.ErrDocumentNotFound, but its query-based getters surface an empty result as gocb.ErrNoResult from Result.One(). Key-value reads were recognised; every N1QL getter was not. TestNotFoundContractIsUniform could not see either, because it detects getters returning (nil, nil) — not getters that return an error of the wrong shape. TestNotFoundIsRecognisableOnEveryBackend closes that: it asks each live backend for an id that cannot exist and requires storage.IsNotFound to say so. It is a runtime test on purpose — the defect is in what the driver returns, which no static check over the source can see — so it only means anything under `make test-all-db`, which is exactly how the original bug survived a green CI. Verified: make test-all-db passes on all seven backends (sqlite, postgres, mongodb, arangodb, scylladb, dynamodb, couchbase), and the new test fails on dynamodb when the ops.go fix is reverted. token.subjectLiveness keeps its defensive shape from the previous commit — it no longer DEPENDS on this being right on every backend — but with this fix the two agree instead of one compensating for the other. --- internal/storage/db/couchbase/errors.go | 13 +++- internal/storage/db/dynamodb/client.go | 3 +- internal/storage/db/dynamodb/ops.go | 6 +- internal/storage/db/dynamodb/user.go | 8 +- internal/storage/notfound_runtime_test.go | 91 +++++++++++++++++++++++ 5 files changed, 112 insertions(+), 9 deletions(-) create mode 100644 internal/storage/notfound_runtime_test.go diff --git a/internal/storage/db/couchbase/errors.go b/internal/storage/db/couchbase/errors.go index 8bd33a83b..cb1236509 100644 --- a/internal/storage/db/couchbase/errors.go +++ b/internal/storage/db/couchbase/errors.go @@ -6,8 +6,15 @@ import ( "github.com/couchbase/gocb/v2" ) -// IsNotFound reports whether err means "no such row" in this backend. gocb's -// ErrDocumentNotFound is canonical, so it is matched directly. +// IsNotFound reports whether err means "no such row" in this backend. +// +// Two sentinels, because this provider reads through two different gocb APIs. +// Key-value gets return ErrDocumentNotFound; N1QL queries that matched nothing +// surface as ErrNoResult from Result.One(). Matching only the first meant every +// query-based getter — GetUserByID and GetClientByID among them — reported +// absence as an unrecognised error, so callers using storage.IsNotFound to +// separate "no such row" from "the query failed" got the wrong answer on this +// backend alone. func IsNotFound(err error) bool { - return errors.Is(err, gocb.ErrDocumentNotFound) + return errors.Is(err, gocb.ErrDocumentNotFound) || errors.Is(err, gocb.ErrNoResult) } diff --git a/internal/storage/db/dynamodb/client.go b/internal/storage/db/dynamodb/client.go index bdedc40fa..4efc8cf78 100644 --- a/internal/storage/db/dynamodb/client.go +++ b/internal/storage/db/dynamodb/client.go @@ -2,7 +2,6 @@ package dynamodb import ( "context" - "errors" "fmt" "sort" "time" @@ -90,7 +89,7 @@ func (p *provider) GetClientByID(ctx context.Context, id string) (*schemas.Clien return nil, err } if sa.ID == "" { - return nil, errors.New("no document found") + return nil, fmt.Errorf("client not found: %w", ErrNotFound) } return &sa, nil } diff --git a/internal/storage/db/dynamodb/ops.go b/internal/storage/db/dynamodb/ops.go index b548f335c..a6709bceb 100644 --- a/internal/storage/db/dynamodb/ops.go +++ b/internal/storage/db/dynamodb/ops.go @@ -52,7 +52,11 @@ func (p *provider) getItemByHash(ctx context.Context, table, hashKey, hashValue return err } if len(res.Item) == 0 { - return fmt.Errorf("no record found") + // Wrapped so storage.IsNotFound recognises it. A bare error here made + // "the row is absent" indistinguishable from "the query failed" for + // every caller of every getter built on this helper — the exact + // conflation AGENTS.md's not-found contract exists to prevent. + return fmt.Errorf("record not found: %w", ErrNotFound) } return unmarshalItem(res.Item, out) } diff --git a/internal/storage/db/dynamodb/user.go b/internal/storage/db/dynamodb/user.go index 890bf930a..caf9f2829 100644 --- a/internal/storage/db/dynamodb/user.go +++ b/internal/storage/db/dynamodb/user.go @@ -275,9 +275,11 @@ func (p *provider) GetUserByExternalID(ctx context.Context, orgID, externalID st // GetUserByID to get user information from database using user ID func (p *provider) GetUserByID(ctx context.Context, id string) (*schemas.User, error) { var user schemas.User - err := p.getItemByHash(ctx, schemas.Collections.User, "id", id, &user) - if err != nil { - return nil, errors.New("no documets found") + // Return the underlying error rather than replacing it: getItemByHash already + // distinguishes absence (wrapped ErrNotFound) from failure, and flattening + // both into one opaque error reported a DynamoDB outage as a missing user. + if err := p.getItemByHash(ctx, schemas.Collections.User, "id", id, &user); err != nil { + return nil, err } normalizeUserOptionalPtrs(&user) return &user, nil diff --git a/internal/storage/notfound_runtime_test.go b/internal/storage/notfound_runtime_test.go new file mode 100644 index 000000000..31ebc140c --- /dev/null +++ b/internal/storage/notfound_runtime_test.go @@ -0,0 +1,91 @@ +package storage + +import ( + "context" + "net" + "os" + "testing" + "time" + + "github.com/google/uuid" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +// TestNotFoundIsRecognisableOnEveryBackend asserts that a single-entity getter +// reports an absent row with an error storage.IsNotFound actually recognises. +// +// TestNotFoundContractIsUniform, its static sibling, only detects a getter that +// returns (nil, nil). It cannot see the other half of the contract: a getter +// that correctly returns an ERROR, but one so shaped that IsNotFound says false. +// That gap shipped real bugs. DynamoDB's GetUserByID returned a bare +// errors.New("no documets found") and Couchbase's returned gocb.ErrNoResult from +// the N1QL path while its IsNotFound matched only ErrDocumentNotFound — so on +// exactly those two backends, callers branching on IsNotFound to separate "no +// such row" from "the query failed" got the wrong answer. +// +// The consequence was not theoretical. token.subjectLiveness used that branch to +// decide whether to look the subject up as a client; on DynamoDB and Couchbase +// it never got there, which both rejected every delegated service-account token +// and stopped service-account deactivation from revoking live tokens. Neither +// failed in CI, because CI runs SQLite. +// +// This test is deliberately a RUNTIME one: the defect lives in what the driver +// returns, which no static check over the source can see. It is therefore only +// meaningful under `make test-all-db` (or TEST_DBS naming a real backend); +// SQLite alone proves almost nothing here, which is precisely how the bug +// survived. +// +// Scoped to the two getters the liveness path depends on. Widening it to every +// getter would be better and is worth doing separately; asserting the two that +// caused a live bug is the part that must not regress. +func TestNotFoundIsRecognisableOnEveryBackend(t *testing.T) { + logger := zerolog.New(zerolog.NewTestWriter(t)).With().Timestamp().Logger() + + for _, dbType := range getTestDBTypes() { + t.Run(dbType, func(t *testing.T) { + if dbType == constants.DbTypeDynamoDB { + _ = os.Unsetenv("AWS_ACCESS_KEY_ID") + _ = os.Unsetenv("AWS_SECRET_ACCESS_KEY") + } + cfg := getTestDBConfig(dbType) + if dbType == constants.DbTypeCouchbaseDB { + conn, err := net.DialTimeout("tcp", "127.0.0.1:8091", 2*time.Second) + if err != nil { + t.Skipf("Couchbase not reachable on 127.0.0.1:8091: %v", err) + } + _ = conn.Close() + cfg.DatabaseUsername = "Administrator" + cfg.DatabasePassword = "password" + } + + provider, err := New(cfg, &Dependencies{Log: &logger}) + require.NoError(t, err, "could not open %s", dbType) + + ctx := context.Background() + // An id no row can have, so the ONLY correct answer is "not found". + absent := "definitely-absent-" + uuid.NewString() + + t.Run("GetUserByID", func(t *testing.T) { + user, uErr := provider.GetUserByID(ctx, absent) + assert.Nil(t, user, "a missing row must not also yield a value") + require.Error(t, uErr, "a single-entity getter must report absence as an error, never (nil, nil)") + assert.True(t, IsNotFound(uErr), + "absence must be distinguishable from failure: IsNotFound said false for %q. "+ + "Callers branch on this to tell a missing row from a database outage, and getting it "+ + "wrong turns one into the other.", uErr) + }) + + t.Run("GetClientByID", func(t *testing.T) { + client, cErr := provider.GetClientByID(ctx, absent) + assert.Nil(t, client) + require.Error(t, cErr) + assert.True(t, IsNotFound(cErr), + "absence must be distinguishable from failure: IsNotFound said false for %q", cErr) + }) + }) + } +} From dbc6563df7ddb419a331f05446b5bebb27f3d266 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Mon, 10 Aug 2026 20:50:12 +0530 Subject: [PATCH 10/11] docs(mcp): correct the stdio subcommand's stale help text The Long description and type comment still claimed stdio was the only supported transport, which stopped being true when --mcp-enabled landed in the same branch. Both now lead with the deprecation and point at the replacement. --- cmd/mcp.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/cmd/mcp.go b/cmd/mcp.go index f176b0125..85241421d 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -50,16 +50,21 @@ var mcpArgs struct { // `(authorizer.v1.mcp_tool).exposed` option; the MCP server discovers // them at startup. // -// Transport: STDIO ONLY. The MCP server has no auth/rate-limit interceptors -// of its own — the security model relies on the OS-level trust boundary of -// the subprocess. See internal/mcp/server.go's Server type comment. +// DEPRECATED, removed in 2.5.0. Superseded by --mcp-enabled on the server, +// which shares the running providers and authenticates every request +// separately. This transport has no auth of its own — it relies on the +// OS-level trust boundary of the subprocess. See internal/mcp.Server. var mcpCmd = &cobra.Command{ Use: "mcp", Short: "Serve Authorizer's MCP tool surface over stdio", - Long: "Exposes a subset of Authorizer's gRPC methods (those marked " + - "(authorizer.v1.mcp_tool).exposed=true in proto) as MCP " + - "tools, suitable for use with Claude Code or any MCP-compatible " + - "host. Stdio is the only supported transport.", + Long: "DEPRECATED — use --mcp-enabled on the server instead; this " + + "subcommand is removed in 2.5.0.\n\n" + + "Exposes a subset of Authorizer's gRPC methods (those marked " + + "(authorizer.v1.mcp_tool).exposed=true in proto) as MCP tools over " + + "stdio. It runs a second copy of every provider and serves a single " + + "user per process (--mcp-bearer), which is why it cannot be deployed. " + + "The server's own MCP surface at POST /mcp shares the running " + + "providers and authenticates each request separately.", Run: runMCP, } From c2411b7ef14d802816caca619b9edc0951976813 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 11 Aug 2026 10:43:28 +0530 Subject: [PATCH 11/11] docs(mcp): record why there is no dynamic client registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A future reviewer hitting the absent registration_endpoint should find the reasoning inline rather than file it as a gap. The MCP spec 2025-11-25 demoted DCR: authorization servers SHOULD support Client ID Metadata Documents and MAY support DCR, kept only "for backwards compatibility with earlier versions of the MCP authorization spec". Auth0 ships DCR Enterprise-only and disabled by default, behind ACLs or a proxy, and recommends CIMD instead; Anthropic steers directory traffic away from DCR because it registers a fresh client per connection. On a self-hosted product that is unbounded row growth in every operator's deployment. Also records, on the protected-resource handler, which clients can actually complete the flow today — verified against Claude Code 2.1.226, the browser OAuth path does not work without CIMD, and the static-token path does. Both comments carry the spec/vendor links so the next reader can check the reasoning rather than take it on trust. --- internal/http_handlers/openid_config.go | 31 +++++++++++++++++++- internal/http_handlers/protected_resource.go | 14 +++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/internal/http_handlers/openid_config.go b/internal/http_handlers/openid_config.go index 997188cfd..9e3df45ff 100644 --- a/internal/http_handlers/openid_config.go +++ b/internal/http_handlers/openid_config.go @@ -84,7 +84,36 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { "code_challenge_methods_supported": codeChallengeMethods, // RFC 8707 resource indicators are honored on the authorization_code // flow (resource query param → access token aud) and token-exchange. - "resource_indicators_supported": true, + "resource_indicators_supported": true, + // NO `registration_endpoint`, and that is deliberate — please do not + // "fix" it by adding RFC 7591 dynamic client registration. + // + // The MCP authorization spec (2025-11-25) demoted DCR: authorization + // servers **SHOULD** support Client ID Metadata Documents and **MAY** + // support DCR, which it keeps only "for backwards compatibility with + // earlier versions of the MCP authorization spec". CIMD is the + // recommended path, and the client priority order is pre-registered → + // CIMD → DCR → prompt the user. + // https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization + // + // DCR is also an open, unauthenticated write endpoint. Auth0 ships it + // Enterprise-only, disabled by default, and requires tenant ACLs or a + // reverse proxy in front, citing resource depletion, security probing, + // unvetted misconfigured clients and audit gaps — then recommends CIMD + // instead for production. + // https://auth0.com/ai/docs/mcp/guides/registering-your-mcp-client-application/dynamic-client-registration + // + // Anthropic's own connector guidance points the same way: DCR makes a + // client register afresh on every connection, so a self-hosted + // deployment accumulates client rows without bound. + // https://claude.com/docs/connectors/building/authentication + // + // `client_id_metadata_document_supported` is NOT advertised yet either, + // because advertising a capability that is not implemented is worse + // than omitting it — a client would select CIMD and then fail. Adding + // CIMD is tracked as the follow-up to the MCP transport work; it needs + // the URL-form client_id resolver plus an /authorize consent screen, + // which the spec makes mandatory once client identity is self-asserted. "revocation_endpoint": issuer + "/oauth/revoke", "revocation_endpoint_auth_methods_supported": []string{"client_secret_basic", "client_secret_post"}, "introspection_endpoint": issuer + "/oauth/introspect", diff --git a/internal/http_handlers/protected_resource.go b/internal/http_handlers/protected_resource.go index 93fb81c2e..245b1b49c 100644 --- a/internal/http_handlers/protected_resource.go +++ b/internal/http_handlers/protected_resource.go @@ -35,6 +35,20 @@ import ( // is the easy case: `authorization_servers` names this same origin, so there is no // third party to be confused about and no token to pass through to one. // +// # Which clients can actually complete this flow today +// +// Verified against Claude Code 2.1.226: the browser OAuth path does NOT work. It +// refuses with "Incompatible auth server: does not support dynamic client +// registration" and does not fall back to prompting for a client id. Everything +// this document advertises is correct and the flow is spec-conformant — the gap +// is client-side registration, not discovery. +// +// What IS verified working is a static bearer token bound to /mcp, minted +// via client_credentials with `resource`. That identifies a service account +// rather than a human, which is the honest scope of the surface until Client ID +// Metadata Documents land. See the note on `registration_endpoint` in +// openid_config.go for why the answer is CIMD and not RFC 7591 DCR. +// // Deliberately public and cacheable: RFC 9728 §3.1 defines this as public client // configuration. It carries no secret and no per-caller data — only where to // authenticate and what to ask for.