feat(auth)!: establish secure remote transport boundaries by riccardomenegazzo · Pull Request #94 · sysdiglabs/sysdig-mcp-server

riccardomenegazzo · 2026-09-03T11:05:05Z

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

validate issuer, JWKS, audience, signing algorithm, expiry, and scopes for remote OAuth tokens
publish RFC 9728 protected-resource metadata and standards-based bearer challenges
enforce an exact Origin allowlist and exact-origin CORS
keep Sysdig host/token server-side and remove request/context overrides
remove request-header/token logging and add HTTP timeouts
document the breaking migration
prove end-to-end that inbound MCP tokens are never forwarded upstream

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

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.

Copilot

🟡 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 ReadHeaderTimeout and IdleTimeout, but it still has no overall ReadTimeout. Adding a ReadTimeout helps 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
+}


tembleking · 2026-09-04T10:30:38Z

 	sseServer := server.NewSSEServer(h.server, server.WithStaticBasePath(mountPath))
-	mux.Handle(mountPath, authMiddleware(sseServer))
+	security.mountMetadata(mux)
+	mux.Handle(mountPath, security.protect(sseServer))


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.

tembleking · 2026-09-04T10:30:40Z

+				http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
+				return
+			}
+			w.Header().Set("Access-Control-Allow-Origin", origin)


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.

tembleking · 2026-09-04T10:30:41Z

+	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 {


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.

tembleking · 2026-09-04T10:30:42Z

+		server := &http.Server{
+			Addr:              addr,
+			Handler:           handler.AsStreamableHTTP(cfg.MountPath, cfg.Stateless, setupRemoteSecurity(cfg)),
+			ReadHeaderTimeout: 10 * time.Second,


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.

tembleking · 2026-09-04T10:30:43Z

+		return slices.Clone(fallback)
+	}
+
+	return strings.FieldsFunc(value, func(r rune) bool {


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.

tembleking · 2026-09-04T10:30:45Z

+	signingAlgorithms []string,
+	requiredScopes []string,
+) *JWTVerifier {
+	ctx = oidc.ClientContext(ctx, &http.Client{Timeout: jwksRequestTimeout})


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.

tembleking · 2026-09-04T10:30:46Z

+	})
+}
+
+func validateAbsoluteURL(name, rawURL string) error {


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.

tembleking · 2026-09-04T10:30:48Z

@@ -4,7 +4,7 @@ buildGoLatestModule (finalAttrs: {
  version = "3.0.2";


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.

tembleking · 2026-09-04T10:30:49Z

+		return fmt.Errorf("Sysdig API host must be an absolute URL")
 	}
 	req.URL.Scheme = u.Scheme
 	req.URL.Host = u.Host


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.

tembleking · 2026-09-04T10:30:51Z

+			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 {


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.

tembleking · 2026-09-04T10:30:52Z

+	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 {


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.

tembleking · 2026-09-04T10:30:53Z

+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 != "" {


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.

tembleking · 2026-09-04T10:30:54Z

+		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) {


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.

tembleking · 2026-09-04T10:30:56Z

+	return nil
+}
+
+func validateOrigin(origin string) error {


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.

tembleking · 2026-09-04T10:30:57Z

+
+			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")


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 · 2026-09-04T11:14:21Z

@tembleking Thanks a lot for the thorough review, I’ve addressed the findings in the latest commit.
In particular, I fixed the SSE routing end-to-end, moved CORS handling onto mcp-go’s native primitives, tightened resource/mount-path and URL validation, normalized Origin handling, preserved Sysdig API path prefixes, added ReadTimeout coverage, and separated JWKS TLS policy from the Sysdig API TLS setting.
I also added regression tests for the reported cases and bumped the version to 4.0.0 to reflect the breaking change.
The updated CI workflow is currently waiting for approval to run on the fork PR. Once it runs, I’ll address anything else that comes up.
Thanks again! The review was very helpful in tightening both the security boundary and the operational behavior.

feat(auth)!: establish secure remote transport boundaries

7110997

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.

Copilot AI lite review requested due to automatic review settings September 3, 2026 11:05

riccardomenegazzo requested a review from a team as a code owner September 3, 2026 11:05

Copilot started reviewing on behalf of riccardomenegazzo September 3, 2026 11:05 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

tembleking reviewed Sep 4, 2026

View reviewed changes

fix(auth): address remote boundary review findings

6643c87

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 ~~feat(auth): establish secure remote transport boundaries~~ feat(auth)!: establish secure remote transport boundaries Sep 4, 2026

		@@ -4,7 +4,7 @@ buildGoLatestModule (finalAttrs: {
		version = "3.0.2";

Uh oh!

Conversation

riccardomenegazzo commented Sep 3, 2026

Why

What

Compatibility

Validation

References

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riccardomenegazzo commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants