feat(auth)!: establish secure remote transport boundaries - #94
feat(auth)!: establish secure remote transport boundaries#94riccardomenegazzo wants to merge 2 commits into
Conversation
Remote MCP access tokens and Sysdig API credentials cross different trust boundaries. Treating them as interchangeable exposed the configured upstream and token to client control.
There was a problem hiding this comment.
🟡 Changes recommended
A couple of security/operational hardening gaps remain (URL validation and remote HTTP server timeout coverage) that should be addressed to fully meet the intended boundary protections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the remote (streamable-http/sse) transports by separating MCP client authentication (OAuth JWT access tokens) from Sysdig upstream credentials, preventing request headers from selecting upstream host/token, and adding standards-based protected-resource metadata and CORS/origin enforcement.
Changes:
- Introduces remote OAuth JWT verification (issuer/audience/JWKS/signing alg/expiry + optional scopes) and publishes RFC 9728 protected-resource metadata with proper bearer challenges.
- Enforces exact Origin allowlisting for browser-based requests and removes request-derived Sysdig host/token overrides to preserve trust boundaries.
- Updates configuration validation and documentation for the breaking remote-security migration (Go 1.27+, new env vars, updated client guidance), plus adjusts tests accordingly.
File summaries
| File | Description |
|---|---|
| README.md | Documents the new remote auth boundary, required OAuth settings, origin allowlist behavior, and migration notes. |
| package.nix | Updates vendored dependency hash after Go module changes. |
| internal/infra/sysdig/client.go | Removes context-based Sysdig auth; enforces fixed server-side host/token usage and requires an absolute host URL. |
| internal/infra/sysdig/client_test.go | Updates unit tests to reflect fixed server-side Sysdig auth and absolute-host validation. |
| internal/infra/sysdig/client_permissions_integration_test.go | Removes context-token integration scenarios; adds env guard/skip when Sysdig creds aren’t provided. |
| internal/infra/mcp/remote_security.go | Adds remote transport security middleware: Origin allowlist + bearer token extraction + verifier integration + metadata/challenges. |
| internal/infra/mcp/mcp_handler.go | Wires RemoteSecurity into SSE/streamable-http handlers and mounts protected-resource metadata. |
| internal/infra/mcp/mcp_handler_test.go | Adds end-to-end tests for token verification, insufficient scope behavior, exact-origin CORS, and “never forward MCP token upstream”. |
| internal/infra/auth/token_verifier.go | Introduces TokenVerifier and JWTVerifier backed by remote JWKS (go-oidc) with optional scope enforcement. |
| internal/infra/auth/token_verifier_test.go | Adds JWT verifier tests (issuer/audience/expiry/scope/alg). |
| internal/config/config.go | Adds remote OAuth and origin configuration fields; validates transports, absolute URLs, allowed signing algs, and origin formatting. |
| internal/config/config_test.go | Updates tests to cover secured remote configs, URL/origin validation, signing alg rules, and env loading of new settings. |
| go.sum | Adds checksums for new OIDC/JWT-related dependencies. |
| go.mod | Adds OIDC/JWT dependencies and updates indirect oauth2 dependency. |
| docs/TROUBLESHOOTING.md | Updates troubleshooting guidance for new required config and remote 401/403 behavior. |
| cmd/server/main.go | Removes fallback/context Sysdig auth, wires remote security setup, and introduces http.Server timeouts for remote transports. |
| AGENTS.md | Updates handbook to reflect the new auth boundary, dependencies, and architecture notes for remote security. |
Review details
Suppressed comments (1)
cmd/server/main.go:179
- The SSE server sets
ReadHeaderTimeoutandIdleTimeout, but it still has no overallReadTimeout. Adding aReadTimeouthelps mitigate slow request bodies and keeps timeout behavior consistent across remote transports (SSE requests typically have no body, so this mainly adds defense-in-depth).
server := &http.Server{
Addr: addr,
Handler: handler.AsSSE(cfg.MountPath, setupRemoteSecurity(cfg)),
ReadHeaderTimeout: 10 * time.Second,
IdleTimeout: 2 * time.Minute,
- Files reviewed: 16/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| server := &http.Server{ | ||
| Addr: addr, | ||
| Handler: handler.AsStreamableHTTP(cfg.MountPath, cfg.Stateless, setupRemoteSecurity(cfg)), | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| IdleTimeout: 2 * time.Minute, | ||
| } |
| func validateAbsoluteURL(name, rawURL string) error { | ||
| u, err := url.Parse(rawURL) | ||
| if err != nil || !u.IsAbs() || u.Host == "" { | ||
| return fmt.Errorf("%s must be an absolute URL", name) | ||
| } | ||
| if u.User != nil || u.Fragment != "" { | ||
| return fmt.Errorf("%s must not contain user information or a fragment", name) | ||
| } | ||
| if u.Scheme != "https" && !(u.Scheme == "http" && isLoopbackHostname(u.Hostname())) { | ||
| return fmt.Errorf("%s must use https (http is allowed only for loopback development)", name) | ||
| } | ||
| return nil | ||
| } |
| sseServer := server.NewSSEServer(h.server, server.WithStaticBasePath(mountPath)) | ||
| mux.Handle(mountPath, authMiddleware(sseServer)) | ||
| security.mountMetadata(mux) | ||
| mux.Handle(mountPath, security.protect(sseServer)) |
There was a problem hiding this comment.
AsSSE mounts the SSE server at an exact-match ServeMux pattern that mcp-go's SSEServer never actually answers at. With the default mount path, GET /sysdig-mcp-server, GET .../sse, and POST .../message all return 404. The sse transport is currently non-functional end to end, which means the whole OAuth/CORS boundary this PR adds is dead code for it. Worth fixing the routing or dropping sse support until it is.
| http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) | ||
| return | ||
| } | ||
| w.Header().Set("Access-Control-Allow-Origin", origin) |
There was a problem hiding this comment.
Once the routing above is fixed and requests actually reach SSEServer.handleSSE, that handler unconditionally overwrites Access-Control-Allow-Origin with "*" unless WithSSECORS was passed to server.NewSSEServer. AsSSE never passes it, so the exact-origin allowlist set here gets undone for the SSE transport.
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Add("Vary", "Origin") | ||
| if origin := r.Header.Get("Origin"); origin != "" { | ||
| if _, allowed := s.allowedOrigins[origin]; !allowed { |
There was a problem hiding this comment.
This is an exact, case-sensitive string match, and validateOrigin in config.go never normalizes host case. An entry like SYSDIG_MCP_ALLOWED_ORIGINS=https://Client.Example.com passes validation but will never match the lowercased Origin header a real browser sends, causing a silent, permanent 403 for a correctly configured origin. Consider normalizing both sides (e.g. lowercase the host) before comparing.
| server := &http.Server{ | ||
| Addr: addr, | ||
| Handler: handler.AsStreamableHTTP(cfg.MountPath, cfg.Stateless, setupRemoteSecurity(cfg)), | ||
| ReadHeaderTimeout: 10 * time.Second, |
There was a problem hiding this comment.
Both http.Server instances (here and the sse case below at line 178) set ReadHeaderTimeout and IdleTimeout but never ReadTimeout, so a slow or stalled request body after valid headers can hold the connection open indefinitely. Suggest adding a ReadTimeout alongside the other two.
| return slices.Clone(fallback) | ||
| } | ||
|
|
||
| return strings.FieldsFunc(value, func(r rune) bool { |
There was a problem hiding this comment.
strings.FieldsFunc splits on space/tab/newline/comma but not \r. A SYSDIG_MCP_AUTH_SCOPES value with \r\n (e.g. pasted from a Windows-style env file) leaves a trailing \r on a scope, which will never match a token's clean claim, silently 403-ing every valid token. There is also no format validation on the scope strings themselves. Worth adding \r to the split set and/or trimming each field.
| signingAlgorithms []string, | ||
| requiredScopes []string, | ||
| ) *JWTVerifier { | ||
| ctx = oidc.ClientContext(ctx, &http.Client{Timeout: jwksRequestTimeout}) |
There was a problem hiding this comment.
SYSDIG_MCP_API_SKIP_TLS_VERIFICATION is only wired into the Sysdig API client in main.go, not into this JWKS http.Client. For an on-prem IdP with a self-signed cert, operators following docs/TROUBLESHOOTING.md's generic remedy will still get a permanent 401 (x509: certificate signed by unknown authority) here, making the documented fix a no-op for the JWKS path.
| }) | ||
| } | ||
|
|
||
| func validateAbsoluteURL(name, rawURL string) error { |
There was a problem hiding this comment.
validateAbsoluteURL checks scheme/host/user/fragment but never rejects a query string (unlike validateOrigin below, which does). A SYSDIG_MCP_RESOURCE_URL with a ?query passes validation and then leaks into both the JWT-audience comparison and the public RFC 9728 metadata document, where a canonicalization mismatch with the authorization server would break every token's audience check.
| @@ -4,7 +4,7 @@ buildGoLatestModule (finalAttrs: { | |||
| version = "3.0.2"; | |||
There was a problem hiding this comment.
version stays "3.0.2" even though this PR is a breaking change per its own commit convention (feat(auth)!:). publish.yaml only releases on a version diff against the latest tag, so without a bump this breaking change ships with no release, and a later unrelated patch bump would carry it in silently. The two previous breaking commits (#87, #91) both bumped MAJOR in-commit.
| return fmt.Errorf("Sysdig API host must be an absolute URL") | ||
| } | ||
| req.URL.Scheme = u.Scheme | ||
| req.URL.Host = u.Host |
There was a problem hiding this comment.
updateReqWithHostURL copies only Scheme and Host from the parsed SYSDIG_MCP_API_HOST, silently dropping any path component, and validateAbsoluteURL never warns about a path being present. A reverse-proxied host like https://gateway.example.com/sysdig-proxy passes config validation but every outbound request then drops the /sysdig-proxy prefix and silently hits the wrong path.
| w.Header().Set("Access-Control-Allow-Origin", origin) | ||
| w.Header().Set("Access-Control-Expose-Headers", "Mcp-Session-Id, WWW-Authenticate") | ||
|
|
||
| if r.Method == http.MethodOptions { |
There was a problem hiding this comment.
This treats any OPTIONS request carrying an Origin header as a CORS preflight, without checking Access-Control-Request-Method the way mcp-go's own CORSConfig.handlePreflight does. A non-preflight OPTIONS probe with an Origin header gets an unwarranted 204 and skips bearer-token verification entirely instead of the normal 401.
| if c.AuthJWKSURL == "" { | ||
| return fmt.Errorf("required configuration missing: SYSDIG_MCP_AUTH_JWKS_URL") | ||
| } | ||
| if err := validateAbsoluteURL("SYSDIG_MCP_RESOURCE_URL", c.ResourceURL); err != nil { |
There was a problem hiding this comment.
Nothing here cross-checks SYSDIG_MCP_RESOURCE_URL's path against SYSDIG_MCP_MOUNT_PATH, so the RFC 9728 / JWT-audience identity can silently diverge from the path actually served. E.g. MountPath stays the default /sysdig-mcp-server while ResourceURL=https://mcp.example.com (no path): the metadata then advertises the wrong resource, and a token scoped to a different root-level resource from the same authorization server would incorrectly pass the audience check.
| func (s RemoteSecurity) protect(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Add("Vary", "Origin") | ||
| if origin := r.Header.Get("Origin"); origin != "" { |
There was a problem hiding this comment.
Origin is read with Header.Get (first value only), while Authorization a few lines below is deliberately required to have exactly one value via Header.Values. Worth applying the same strict handling to Origin for consistency, even though browsers won't normally send it duplicated.
| return fmt.Errorf("SYSDIG_MCP_AUTH_SIGNING_ALGS must contain at least one asymmetric signing algorithm") | ||
| } | ||
| for _, algorithm := range c.AuthSigningAlgs { | ||
| if !slices.Contains([]string{"RS256", "RS384", "RS512", "PS256", "PS384", "PS512", "ES256", "ES384", "ES512", "EdDSA"}, algorithm) { |
There was a problem hiding this comment.
This hardcodes its own copy of the 10 JOSE asymmetric algorithm names instead of referencing exported constants from go-oidc (the dependency this PR adds). Two independently maintained copies of a security-relevant allowlist can drift silently if go-oidc adds or deprecates an algorithm.
| return nil | ||
| } | ||
|
|
||
| func validateOrigin(origin string) error { |
There was a problem hiding this comment.
validateOrigin duplicates most of validateAbsoluteURL's parse/IsAbs/Host/scheme logic, and the two have already drifted (only this one checks Path/RawQuery). Worth factoring out the shared checks so a future policy change (e.g. tightening the scheme rule) only needs to happen once.
|
|
||
| if r.Method == http.MethodOptions { | ||
| w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS") | ||
| w.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, Last-Event-ID, Mcp-Protocol-Version, Mcp-Session-Id") |
There was a problem hiding this comment.
The hand-rolled preflight response never sets Access-Control-Max-Age, unlike mcp-go's CORSConfig, so browsers can never cache the preflight result. Every browser-originated tools/call pays a fresh OPTIONS round trip first, roughly doubling latency compared to the library's cacheable default.
Fix SSE routing and CORS integration, tighten remote URL and scope validation, preserve Sysdig API path prefixes, add independent JWKS TLS policy, harden HTTP timeouts, and bump the breaking release to v4.0.0.
riccardomenegazzo
changed the title
|
@tembleking Thanks a lot for the thorough review, I’ve addressed the findings in the latest commit. |
Why
Remote MCP access tokens and Sysdig API credentials belong to different trust domains. The current remote path treats the caller bearer token as a Sysdig API token and lets request headers select the upstream host. This collapses the MCP and Sysdig trust boundaries.
What
Compatibility
Breaking for remote deployments: new OAuth and Origin settings are required. The stdio authentication flow is unchanged; all modes now require an absolute Sysdig API URL.
Validation
go generate, gofumpt, unit tests, race tests, golangci-lint, govulncheck, flake evaluation, and the full Nix static build all pass.
References