fix(ingestion): build /mcp's bearer and the metrics wrap inside buildMux (#154) #183

Merged
mathias merged 2 commits from w3/154-mcp-auth-wiring into main 2026-10-05 17:48:44 +00:00
5 changed files with 146 additions and 56 deletions
+32 -1
View File
@@ -12,6 +12,8 @@ import (
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/mathiasbq/hyperguild/ingestion/internal/metrics"
) )
// fakeIssuer answers OIDC discovery with 502 for the first fail requests, as // fakeIssuer answers OIDC discovery with 502 for the first fail requests, as
@@ -73,8 +75,37 @@ func TestBuildMuxAnswers503ToAJWTWhileTheIssuerIsUnresolved(t *testing.T) {
srv, _ := fakeIssuer(t, 1000) srv, _ := fakeIssuer(t, 1000)
v, err := buildJWTValidator(context.Background(), slog.New(slog.DiscardHandler), srv.URL, "brain") v, err := buildJWTValidator(context.Background(), slog.New(slog.DiscardHandler), srv.URL, "brain")
require.NoError(t, err) require.NoError(t, err)
mux, err := buildMux(restAuth{token: routesTok, validator: v, realm: "brain"}, testHandler(t), nil) mux, err := buildMux(restAuth{token: routesTok, validator: v, realm: "brain"}, testHandler(t),
okHandler("mcp"), metrics.New(), nil)
require.NoError(t, err) require.NoError(t, err)
code, _ := do(t, mux, "GET", "/pending", "Bearer aaa.bbb.ccc") code, _ := do(t, mux, "GET", "/pending", "Bearer aaa.bbb.ccc")
assert.Equal(t, http.StatusServiceUnavailable, code) assert.Equal(t, http.StatusServiceUnavailable, code)
} }
// PR #183 review: /mcp shares REST's bearer, but only the static token was
// asserted on it. With the issuer unresolved, a JWT on /mcp answers 503 too,
// so /mcp's middleware was built with the JWT validator.
func TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved(t *testing.T) {
srv, _ := fakeIssuer(t, 1000)
v, err := buildJWTValidator(context.Background(), slog.New(slog.DiscardHandler), srv.URL, "brain")
require.NoError(t, err)
mux, err := buildMux(restAuth{token: routesTok, validator: v, realm: "brain"}, testHandler(t),
okHandler("mcp"), metrics.New(), nil)
require.NoError(t, err)
code, _ := do(t, mux, "POST", "/mcp", "Bearer aaa.bbb.ccc")
assert.Equal(t, http.StatusServiceUnavailable, code)
}
// PR #183 review: a 401 on /mcp names the protected-resource metadata in
// WWW-Authenticate (what the claude.ai OAuth connector discovers from), so
// /mcp's middleware was built with resourceMetadataURL.
func TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate(t *testing.T) {
const rm = "https://brain.example/.well-known/oauth-protected-resource"
mux, err := buildMux(restAuth{token: routesTok, realm: "brain", resourceMetadataURL: rm}, testHandler(t),
okHandler("mcp"), metrics.New(), nil)
require.NoError(t, err)
rec := httptest.NewRecorder()
mux.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil))
require.Equal(t, http.StatusUnauthorized, rec.Code)
assert.Contains(t, rec.Header().Get("WWW-Authenticate"), `resource_metadata="`+rm+`"`)
}
+14 -17
View File
@@ -516,8 +516,6 @@ func main() {
} }
} }
public["/mcp"] = chassisauth.BearerMiddleware(mcpToken, jwtValidator, "brain", resourceMetadataURL, mcpSrv)
// POST /capture (#53/#54): the uniform capture REST door. Needs a ticket // POST /capture (#53/#54): the uniform capture REST door. Needs a ticket
// tracker to file action items, so it only mounts when Gitea is // tracker to file action items, so it only mounts when Gitea is
// configured. It reuses the MCP server's graph-wired brain store (one // configured. It reuses the MCP server's graph-wired brain store (one
@@ -541,7 +539,7 @@ func main() {
public["POST /capture"] = captureH public["POST /capture"] = captureH
// Same use-case behind the MCP `capture` tool (#55 relay) so MCP-native // Same use-case behind the MCP `capture` tool (#55 relay) so MCP-native
// harnesses (claude.ai, Crush, Pi, LLM Council) reach capture through // harnesses (claude.ai, Crush, Pi, LLM Council) reach capture through
// the existing /mcp OAuth connector. mcpSrv is already wrapped above; // the existing /mcp OAuth connector. buildMux wraps this same mcpSrv;
// WithCapture mutates the same instance, so the tool appears live. // WithCapture mutates the same instance, so the tool appears live.
mcpSrv.WithCapture(captureSvc, jwtValidator, mcpToken, "local-cli", resolver) mcpSrv.WithCapture(captureSvc, jwtValidator, mcpToken, "local-cli", resolver)
// hyperguild#97: a governance-file rewrite gets the same trail a // hyperguild#97: a governance-file rewrite gets the same trail a
@@ -583,19 +581,18 @@ func main() {
os.Exit(1) os.Exit(1)
} }
// /metrics — unauthenticated Prometheus endpoint. kube-prometheus-stack // /metrics — unauthenticated Prometheus endpoint, scraped via the
// scrapes it via the ServiceMonitor in k3s/apps/supervisor/. The metrics // ServiceMonitor in k3s/apps/supervisor/. buildMux registers it and wraps
// middleware below wraps every other registered handler so it observes // the whole router in the registry's middleware.
// real request latency. /metrics itself is excluded from its own //
// observation by registering it on the outer mux (post-wrap). // The whole router, /mcp's bearer included, is built in buildMux so its
reg := metrics.New() // tests are tests of production wiring (PR #152 review, hyperguild#154).
public["GET /metrics"] = http.HandlerFunc(reg.Handler()) // The REST API's credential is the same as /mcp's. Untested here: the
// restAuth fields and the public map this call passes in (e.g. a nil
// The whole router, built in one place so the test of buildMux is a test // validator would drop JWT auth on /mcp and REST and the suite stays
// of production wiring (PR #152 review). The REST API's credential is the // green), and ListenAndServe serving the result unchanged (hyperguild#182).
// same as /mcp's. handler, err := buildMux(restAuth{token: mcpToken, validator: jwtValidator, realm: "brain",
mux, err := buildMux(restAuth{token: mcpToken, validator: jwtValidator, realm: "brain", resourceMetadataURL: resourceMetadataURL}, h, mcpSrv, metrics.New(), public)
resourceMetadataURL: resourceMetadataURL}, h, public)
if err != nil { if err != nil {
logger.Error("build router", "err", err) logger.Error("build router", "err", err)
os.Exit(1) os.Exit(1)
@@ -616,7 +613,7 @@ func main() {
"watch_interval", watchIntervalLog, "watch_interval", watchIntervalLog,
"mcp_enabled", true, "mcp_enabled", true,
) )
if err := http.ListenAndServe(addr, reg.Middleware(mux)); err != nil { if err := http.ListenAndServe(addr, handler); err != nil {
logger.Error("server stopped", "err", err) logger.Error("server stopped", "err", err)
os.Exit(1) os.Exit(1)
} }
+32 -18
View File
@@ -8,22 +8,22 @@ import (
chassisauth "git.d-ma.be/mathias/mcp-chassis/auth" chassisauth "git.d-ma.be/mathias/mcp-chassis/auth"
"github.com/mathiasbq/hyperguild/ingestion/internal/api" "github.com/mathiasbq/hyperguild/ingestion/internal/api"
"github.com/mathiasbq/hyperguild/ingestion/internal/metrics"
) )
// publicRoutes is the complete set of patterns allowed on the outer mux, // publicRoutes is the complete set of patterns a caller may register on the
// outside the REST API's auth (hyperguild#149). Each authenticates itself or is // outer mux, outside the REST API's auth (hyperguild#149). Each authenticates
// meant to be open: /mcp (bearer/JWT), /capture (bearer/JWT), the webhook // itself or is meant to be open: /capture (bearer/JWT), the webhook (HMAC),
// (HMAC), /oauth/token (client secret), OAuth metadata and /metrics (open). // /oauth/token (client secret) and OAuth metadata (open). /mcp and /metrics
// A REST pattern added here would beat the authenticated catch-all by // are not here: buildMux builds them itself (hyperguild#154). A REST pattern
// specificity and ship open, which is why buildMux refuses anything else. // added here would beat the authenticated catch-all by specificity and ship
// open, which is why buildMux refuses anything else.
var publicRoutes = map[string]bool{ var publicRoutes = map[string]bool{
"/mcp": true,
"POST /capture": true, "POST /capture": true,
"POST /webhooks/brain-sync": true, "POST /webhooks/brain-sync": true,
"GET /.well-known/oauth-protected-resource": true, "GET /.well-known/oauth-protected-resource": true,
"GET /.well-known/oauth-authorization-server": true, "GET /.well-known/oauth-authorization-server": true,
"POST /oauth/token": true, "POST /oauth/token": true,
"GET /metrics": true,
} }
// restAuth is the credential the REST API checks: the same as /mcp's. // restAuth is the credential the REST API checks: the same as /mcp's.
@@ -34,16 +34,25 @@ type restAuth struct {
resourceMetadataURL string resourceMetadataURL string
} }
// buildMux is the server's whole router. The REST API is mounted at "/" // buildMux builds the handler main passes to ListenAndServe unchanged. The
// behind bearer auth that buildMux constructs itself, so a caller cannot pass // REST API is mounted at "/" and /mcp is registered, both behind bearer auth
// a no-op; public handlers are registered as given, and only for patterns on // that buildMux constructs itself from auth (one credential), so a caller
// the publicRoutes allowlist. It returns an http.Handler, not the // cannot pass a no-op. GET /metrics serves reg, and the whole router is
// *ServeMux, so nothing can register a route on the result afterwards and // wrapped in reg's middleware. Other public handlers are registered as given,
// bypass the allowlist. // and only for patterns on the publicRoutes allowlist. It returns an
func buildMux(auth restAuth, h *api.Handler, public map[string]http.Handler) (http.Handler, error) { // http.Handler, not the *ServeMux, so nothing can register a route on the
// result. What stays untested is main's call site: the restAuth fields and
// the public map main passes in, and ListenAndServe serving this handler
// unchanged (hyperguild#182). The tests build their own arguments.
func buildMux(
auth restAuth, h *api.Handler, mcpHandler http.Handler, reg *metrics.Registry, public map[string]http.Handler,
) (http.Handler, error) {
if auth.token == "" { if auth.token == "" {
return nil, errors.New("buildMux: REST auth needs a token") return nil, errors.New("buildMux: REST auth needs a token")
} }
if mcpHandler == nil || reg == nil {
return nil, errors.New("buildMux: needs the MCP handler and the metrics registry")
}
mux := http.NewServeMux() mux := http.NewServeMux()
for pattern, handler := range public { for pattern, handler := range public {
if !publicRoutes[pattern] { if !publicRoutes[pattern] {
@@ -51,10 +60,15 @@ func buildMux(auth restAuth, h *api.Handler, public map[string]http.Handler) (ht
} }
mux.Handle(pattern, handler) mux.Handle(pattern, handler)
} }
mountREST(mux, h, func(next http.Handler) http.Handler { bearer := func(next http.Handler) http.Handler {
return chassisauth.BearerMiddleware(auth.token, auth.validator, auth.realm, auth.resourceMetadataURL, next) return chassisauth.BearerMiddleware(auth.token, auth.validator, auth.realm, auth.resourceMetadataURL, next)
}) }
return mux, nil mux.Handle("/mcp", bearer(mcpHandler))
// /metrics is registered on the mux and so observed by its own
// middleware, like any route.
mux.Handle("GET /metrics", reg.Handler())
mountREST(mux, h, bearer)
return reg.Middleware(mux), nil
} }
// mountREST registers the REST API on its own mux and mounts it at "/" behind // mountREST registers the REST API on its own mux and mounts it at "/" behind
@@ -9,8 +9,6 @@ import (
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/mathiasbq/hyperguild/ingestion/internal/metrics"
) )
// wrongTokSentinel is a presented credential distinctive enough that any // wrongTokSentinel is a presented credential distinctive enough that any
@@ -53,8 +51,8 @@ func TestBuildMuxAcceptedLogLineOmitsTheBareStaticToken(t *testing.T) {
// An authenticated REST request is labelled with the REST mux's pattern: // An authenticated REST request is labelled with the REST mux's pattern:
// the inner mux sets it on the same *http.Request the outer middleware reads. // the inner mux sets it on the same *http.Request the outer middleware reads.
func TestMetricsLabelAnAuthenticatedRESTRequestWithItsRoutePattern(t *testing.T) { func TestMetricsLabelAnAuthenticatedRESTRequestWithItsRoutePattern(t *testing.T) {
reg := metrics.New() mux, reg := testMuxAndRegistry(t)
code, _ := do(t, reg.Middleware(testMux(t)), "GET", "/pending", "Bearer "+routesTok) code, _ := do(t, mux, "GET", "/pending", "Bearer "+routesTok)
require.Equal(t, http.StatusOK, code) require.Equal(t, http.StatusOK, code)
rec := httptest.NewRecorder() rec := httptest.NewRecorder()
@@ -67,8 +65,8 @@ func TestMetricsLabelAnAuthenticatedRESTRequestWithItsRoutePattern(t *testing.T)
// so it was labelled path="/" and a 4xx on /query could not be told from one // so it was labelled path="/" and a 4xx on /query could not be told from one
// on /write. It is now labelled with the REST pattern it was addressed to. // on /write. It is now labelled with the REST pattern it was addressed to.
func TestMetricsLabelARejectedRESTRequestWithItsRoutePattern(t *testing.T) { func TestMetricsLabelARejectedRESTRequestWithItsRoutePattern(t *testing.T) {
reg := metrics.New() mux, reg := testMuxAndRegistry(t)
code, _ := do(t, reg.Middleware(testMux(t)), "POST", "/query", "") code, _ := do(t, mux, "POST", "/query", "")
require.Equal(t, http.StatusUnauthorized, code) require.Equal(t, http.StatusUnauthorized, code)
rec := httptest.NewRecorder() rec := httptest.NewRecorder()
@@ -79,8 +77,8 @@ func TestMetricsLabelARejectedRESTRequestWithItsRoutePattern(t *testing.T) {
// A path no REST route matches keeps the catch-all label, so an attacker // A path no REST route matches keeps the catch-all label, so an attacker
// probing random URLs cannot grow the series set. // probing random URLs cannot grow the series set.
func TestMetricsLabelARejectedUnknownPathWithTheCatchAll(t *testing.T) { func TestMetricsLabelARejectedUnknownPathWithTheCatchAll(t *testing.T) {
reg := metrics.New() mux, reg := testMuxAndRegistry(t)
code, _ := do(t, reg.Middleware(testMux(t)), "POST", "/no-such-route-zq", "") code, _ := do(t, mux, "POST", "/no-such-route-zq", "")
require.Equal(t, http.StatusUnauthorized, code) require.Equal(t, http.StatusUnauthorized, code)
rec := httptest.NewRecorder() rec := httptest.NewRecorder()
+62 -12
View File
@@ -12,6 +12,7 @@ import (
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/mathiasbq/hyperguild/ingestion/internal/api" "github.com/mathiasbq/hyperguild/ingestion/internal/api"
"github.com/mathiasbq/hyperguild/ingestion/internal/metrics"
"github.com/mathiasbq/hyperguild/ingestion/internal/pipeline" "github.com/mathiasbq/hyperguild/ingestion/internal/pipeline"
) )
@@ -21,20 +22,29 @@ func okHandler(body string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(body)) }) return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(body)) })
} }
// testMux builds the router exactly as main does: buildMux with the full // testMux builds the router through buildMux, the function main calls, with
// public set main registers (stub handlers) and real bearer config. // every allowlisted public pattern (stub handlers), a stub MCP server and a
// hyperguild#149: the REST API is authenticated as a whole; the PR #152 // static token only: no JWT validator and no resourceMetadataURL (the tests
// review showed a test of a COPY of the router let main.go regress unseen. // in jwt_validator_test.go pass those). It does not use main's own restAuth
// or public map (hyperguild#182). hyperguild#149: the REST API is
// authenticated as a whole; the PR #152 review showed a test of a COPY of
// the router let main.go regress unseen.
func testMux(t *testing.T) http.Handler { func testMux(t *testing.T) http.Handler {
t.Helper() t.Helper()
h := testHandler(t) mux, _ := testMuxAndRegistry(t)
return mux
}
func testMuxAndRegistry(t *testing.T) (http.Handler, *metrics.Registry) {
t.Helper()
public := map[string]http.Handler{} public := map[string]http.Handler{}
for p := range publicRoutes { for p := range publicRoutes {
public[p] = okHandler("public " + p) public[p] = okHandler("public " + p)
} }
mux, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, public) reg := metrics.New()
mux, err := buildMux(restAuth{token: routesTok, realm: "brain"}, testHandler(t), okHandler("mcp"), reg, public)
require.NoError(t, err) require.NoError(t, err)
return mux return mux, reg
} }
func testHandler(t *testing.T) *api.Handler { func testHandler(t *testing.T) *api.Handler {
@@ -79,8 +89,9 @@ func TestBuildMuxAuthenticatesAnUnregisteredPath(t *testing.T) {
} }
// buildMux registers public handlers as given, without adding auth: each // buildMux registers public handlers as given, without adding auth: each
// public route owns its own (bearer on /mcp, HMAC on the webhook, client // public route owns its own (bearer/JWT on /capture, HMAC on the webhook,
// secret on /oauth/token) or is meant to be open (/metrics, OAuth metadata). // client secret on /oauth/token) or is meant to be open (OAuth metadata).
// /mcp and /metrics are not public routes: buildMux builds them itself.
func TestBuildMuxDoesNotWrapPublicRoutes(t *testing.T) { func TestBuildMuxDoesNotWrapPublicRoutes(t *testing.T) {
r := testMux(t) r := testMux(t)
for p := range publicRoutes { for p := range publicRoutes {
@@ -98,14 +109,53 @@ func TestBuildMuxDoesNotWrapPublicRoutes(t *testing.T) {
// over the authenticated catch-all (more specific pattern), so it is refused. // over the authenticated catch-all (more specific pattern), so it is refused.
func TestBuildMuxRefusesAPublicPatternOffTheAllowlist(t *testing.T) { func TestBuildMuxRefusesAPublicPatternOffTheAllowlist(t *testing.T) {
h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{}) h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{})
for _, p := range []string{"POST /query", "GET /pending", "/", "POST /metrics"} { // "/mcp" and "GET /metrics" are built by buildMux itself (hyperguild#154):
_, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, map[string]http.Handler{p: okHandler("x")}) // passing either as public would register them unauthenticated or twice.
for _, p := range []string{"POST /query", "GET /pending", "/", "POST /metrics", "/mcp", "GET /metrics"} {
_, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, okHandler("mcp"), metrics.New(),
map[string]http.Handler{p: okHandler("x")})
assert.Error(t, err, "public pattern %q must be refused", p) assert.Error(t, err, "public pattern %q must be refused", p)
} }
} }
func TestBuildMuxRefusesAnEmptyToken(t *testing.T) { func TestBuildMuxRefusesAnEmptyToken(t *testing.T) {
h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{}) h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{})
_, err := buildMux(restAuth{token: "", realm: "brain"}, h, nil) _, err := buildMux(restAuth{token: "", realm: "brain"}, h, okHandler("mcp"), metrics.New(), nil)
assert.Error(t, err) assert.Error(t, err)
} }
// hyperguild#154: /mcp's bearer was built in main.go, untested; the buildMux
// tests used stubs. buildMux now wraps the MCP handler itself, with the same
// credential as the REST API.
func TestBuildMuxRefusesMCPWithoutAValidToken(t *testing.T) {
r := testMux(t)
for _, authz := range []string{"", "Bearer wrong"} {
code, body := do(t, r, "POST", "/mcp", authz)
assert.Equal(t, http.StatusUnauthorized, code, "authz %q", authz)
assert.NotEqual(t, "mcp", body)
}
code, body := do(t, r, "POST", "/mcp", "Bearer "+routesTok)
assert.Equal(t, http.StatusOK, code)
assert.Equal(t, "mcp", body)
}
func TestBuildMuxRefusesANilMCPHandlerOrRegistry(t *testing.T) {
auth := restAuth{token: routesTok, realm: "brain"}
_, err := buildMux(auth, testHandler(t), nil, metrics.New(), nil)
assert.Error(t, err)
_, err = buildMux(auth, testHandler(t), okHandler("mcp"), nil, nil)
assert.Error(t, err)
}
// hyperguild#154: main wrapped buildMux's result in the metrics middleware
// before ListenAndServe, untested. buildMux now returns the handler main
// serves as-is, metrics included: a request through it is observed and
// exposed on its own GET /metrics.
func TestBuildMuxObservesRequestsAndServesMetrics(t *testing.T) {
r := testMux(t)
code, _ := do(t, r, "GET", "/pending", "Bearer "+routesTok)
require.Equal(t, http.StatusOK, code)
code, body := do(t, r, "GET", "/metrics", "")
require.Equal(t, http.StatusOK, code)
assert.Contains(t, body, `path="GET /pending"`)
}