From c0c27a2dd677053525f98843f62a364382662ffc Mon Sep 17 00:00:00 2001 From: Daniel Bak Date: Tue, 1 Sep 2026 19:07:22 -0400 Subject: [PATCH] feat: add --forward-header for general trusted-proxy identity headers Stacked on #16. Generalizes the --forward-operator-identity mechanism: a repeatable --forward-header=NAME flag copies any named header from the inbound MCP request onto the outbound gRPC call, if present. Same trust model (the header must be minted by a trusted proxy in front of this server; grpcmcp does not verify it) but not tied to the X-Operator-Identity name specifically. Motivation: an OAuth-terminating reverse proxy in front of grpcmcp (e.g. obot-platform/mcp-oauth-proxy) asserts caller identity via its own header names (X-Forwarded-User, X-Forwarded-Access-Token), not X-Operator-Identity. --forward-operator-identity is unchanged and still useful for the existing internal-mTLS-proxy case. Extends the e2e script (a fourth grpcmcp instance with --forward-header=X-Forwarded-User) and the example backend (echoes back a forwarded header if present) to prove this crosses grpcmcp -> real gRPC call -> real backend, not just the in-process unit test. Two guards added after review: - --forward-header and --forward-operator-identity now require -hostport, matching the existing -tls-crt+hostport pattern. Without -hostport (stdio) there are no inbound HTTP headers, so either flag previously did nothing silently -- worse than an error for something an operator is relying on for identity attribution. - A --forward-header name (or X-Operator-Identity, for --forward-operator-identity) that collides with a header already set via -header/-bearer-env is now rejected at startup. Both mechanisms are chained last in the headers pipeline and overwrite unconditionally, so an operator configuring e.g. --forward-header=Authorization would let any inbound MCP client silently replace grpcmcp's own trusted backend credential on every call. Both guards verified empirically: reverted, confirmed the tests fail (not hang -- waitForExit bounds every exit-expected check to 2s so a regression fails fast instead of hanging the suite), restored, confirmed green. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01CJKesYekqPnbBrhM3swNHs --- README.md | 2 + example/echo.go | 9 +- forward_header_flag_validation_test.go | 139 +++++++++++++++++++++++++ forward_header_test.go | 48 +++++++++ main.go | 56 ++++++++++ scripts/e2e_test.sh | 28 ++++- 6 files changed, 280 insertions(+), 2 deletions(-) create mode 100644 forward_header_flag_validation_test.go create mode 100644 forward_header_test.go diff --git a/README.md b/README.md index 61cc666..321510e 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,8 @@ allows for clients older than `2025-06-18`. * `forward-operator-identity` - Copy the `X-Operator-Identity` header from inbound MCP requests onto outbound gRPC calls, so the backend can attribute agent calls to the human operator. The header is read per request, so it identifies the caller of that tool call. It must be minted by a trusted proxy in front of this server; grpcmcp does not verify it. This needs `hostport`, because stdio carries no HTTP headers. +* `forward-header` string - Copy a named header from inbound MCP requests onto outbound gRPC calls, if present. Repeatable (e.g. `--forward-header=X-Forwarded-User --forward-header=X-Forwarded-Access-Token`). Same trust model as `forward-operator-identity` -- the header must be minted by a trusted proxy in front of this server, and grpcmcp does not verify it -- generalized to whatever header name that proxy actually uses instead of `X-Operator-Identity` specifically. This needs `hostport`, for the same reason as above. + * `string64` - If set, expose 64-bit protobuf integer fields (`int64`, `uint64`, `sint64`, `fixed64`, `sfixed64`) as strings only in MCP JSON schemas. This avoids precision ambiguity for JavaScript-based clients and agents. By default, schemas continue to allow either JSON numbers or strings for compatibility. * `refresh-interval` duration - How often to re-run reflection so methods added to the backend appear without a restart. Defaults to `5m`. This applies when `reflect` is set and `descriptors` is not: a descriptor file wins over reflection for the initial load, so a refresh from reflection would replace the set the operator asked for. diff --git a/example/echo.go b/example/echo.go index eb3d5db..56d1ee4 100644 --- a/example/echo.go +++ b/example/echo.go @@ -110,7 +110,14 @@ func echoHandler(method protoreflect.MethodDescriptor) http.HandlerFunc { } resp := dynamicpb.NewMessage(outputDesc) - resp.Set(outMessageField, req.Get(messageField)) + message := req.Get(messageField).String() + // Surface a forwarded header in the response, if present, so a test + // can confirm grpcmcp actually forwarded it -- not just that grpcmcp's + // own code constructed the header, but that it arrived at the backend. + if forwardedUser := r.Header.Get("X-Forwarded-User"); forwardedUser != "" { + message += "|" + forwardedUser + } + resp.Set(outMessageField, protoreflect.ValueOfString(message)) respBytes, err := proto.Marshal(resp) if err != nil { diff --git a/forward_header_flag_validation_test.go b/forward_header_flag_validation_test.go new file mode 100644 index 0000000..1eaf71f --- /dev/null +++ b/forward_header_flag_validation_test.go @@ -0,0 +1,139 @@ +package main + +import ( + "bytes" + "os/exec" + "testing" + "time" +) + +// waitForExit runs cmd and waits up to timeout for it to exit on its own, +// rather than blocking forever if it doesn't -- which would hang the test +// suite instead of failing it when a regression makes grpcmcp start serving +// where it used to reject the flags and exit. +func waitForExit(t *testing.T, cmd *exec.Cmd, timeout time.Duration) (exited bool, err error) { + t.Helper() + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + defer cmd.Process.Kill() + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + + select { + case err := <-done: + return true, err + case <-time.After(timeout): + return false, nil + } +} + +// TestForwardHeaderRequiresHostport checks that -forward-header (and +// -forward-operator-identity) are rejected without -hostport, rather than +// silently doing nothing: stdio has no inbound HTTP headers to forward, so +// an operator relying on either flag for identity attribution would +// otherwise get no error and no forwarded header. +func TestForwardHeaderRequiresHostport(t *testing.T) { + bin := buildGrpcmcp(t) + descFile := emptyDescriptorFile(t) + + for _, flagName := range []string{"--forward-header=X-Forwarded-User", "--forward-operator-identity"} { + t.Run(flagName, func(t *testing.T) { + cmd := exec.Command(bin, flagName, "--descriptors="+descFile) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + exited, err := waitForExit(t, cmd, 2*time.Second) + if !exited { + t.Fatalf("expected the process to exit without -hostport; it is still running instead of rejecting the flag") + } + if err == nil { + t.Fatalf("expected a non-zero exit without -hostport, got success") + } + if !bytes.Contains(stderr.Bytes(), []byte("need -hostport")) { + t.Errorf("stderr = %q, want a message naming the -hostport requirement", stderr.String()) + } + }) + } +} + +// TestForwardHeaderCollisionRejected checks that -forward-header naming a +// header already set via -header (most dangerously Authorization) is +// rejected at startup, rather than letting an inbound MCP client silently +// override grpcmcp's own trusted backend credential. +func TestForwardHeaderCollisionRejected(t *testing.T) { + bin := buildGrpcmcp(t) + descFile := emptyDescriptorFile(t) + + cmd := exec.Command(bin, + "--hostport=localhost:0", + "--header=Authorization: Bearer backend-secret", + "--forward-header=Authorization", + "--descriptors="+descFile, + ) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + exited, err := waitForExit(t, cmd, 2*time.Second) + if !exited { + t.Fatalf("expected the process to reject a colliding -forward-header; it is still running instead") + } + if err == nil { + t.Fatalf("expected a non-zero exit for a colliding -forward-header, got success") + } + if !bytes.Contains(stderr.Bytes(), []byte("collides")) { + t.Errorf("stderr = %q, want a message naming the collision", stderr.String()) + } +} + +// TestForwardOperatorIdentityCollisionRejected mirrors +// TestForwardHeaderCollisionRejected for -forward-operator-identity, whose +// forwarded header name (X-Operator-Identity) is fixed rather than +// configurable. +func TestForwardOperatorIdentityCollisionRejected(t *testing.T) { + bin := buildGrpcmcp(t) + descFile := emptyDescriptorFile(t) + + cmd := exec.Command(bin, + "--hostport=localhost:0", + "--header=X-Operator-Identity: someone-else", + "--forward-operator-identity", + "--descriptors="+descFile, + ) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + exited, err := waitForExit(t, cmd, 2*time.Second) + if !exited { + t.Fatalf("expected the process to reject a colliding -forward-operator-identity; it is still running instead") + } + if err == nil { + t.Fatalf("expected a non-zero exit for a colliding -forward-operator-identity, got success") + } + if !bytes.Contains(stderr.Bytes(), []byte("collides")) { + t.Errorf("stderr = %q, want a message naming the collision", stderr.String()) + } +} + +// TestForwardHeaderNoCollisionStarts checks that the collision guard does not +// reject a -forward-header configuration with no actual collision. +func TestForwardHeaderNoCollisionStarts(t *testing.T) { + bin := buildGrpcmcp(t) + descFile := emptyDescriptorFile(t) + + cmd := exec.Command(bin, + "--hostport=localhost:0", + "--header=Authorization: Bearer backend-secret", + "--forward-header=X-Forwarded-User", + "--descriptors="+descFile, + ) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + exited, err := waitForExit(t, cmd, 1*time.Second) + if exited { + t.Fatalf("process exited, expected it to keep serving: %v, stderr: %s", err, stderr.String()) + } + // Still running after the timeout, as expected: no collision, nothing to reject. +} diff --git a/forward_header_test.go b/forward_header_test.go new file mode 100644 index 0000000..c293b00 --- /dev/null +++ b/forward_header_test.go @@ -0,0 +1,48 @@ +package main + +import ( + "context" + "net/http" + "testing" + + "github.com/Basic-Capital/grpcmcp/grpcmcp" + "github.com/mark3labs/mcp-go/mcp" +) + +func TestForwardHeadersCopiesNamedHeaders(t *testing.T) { + static := make(http.Header) + static.Set("Authorization", "Bearer token") + provider := forwardHeaders(grpcmcp.StaticHeaders(static), []string{"X-Forwarded-User", "X-Forwarded-Access-Token"}) + + request := mcp.CallToolRequest{} + request.Header = http.Header{} + request.Header.Set("X-Forwarded-User", "alice@basiccapital.com") + request.Header.Set("X-Forwarded-Access-Token", "opaque-token") + request.Header.Set("X-Forwarded-Email", "alice@basiccapital.com") // not in the forward list + + h, err := provider(context.Background(), request) + if err != nil { + t.Fatal(err) + } + if got := h.Get("X-Forwarded-User"); got != "alice@basiccapital.com" { + t.Errorf("X-Forwarded-User = %q, want alice@basiccapital.com", got) + } + if got := h.Get("X-Forwarded-Access-Token"); got != "opaque-token" { + t.Errorf("X-Forwarded-Access-Token = %q, want opaque-token", got) + } + if got := h.Get("X-Forwarded-Email"); got != "" { + t.Errorf("X-Forwarded-Email = %q, want empty: not in the configured forward list", got) + } + if got := h.Get("Authorization"); got != "Bearer token" { + t.Errorf("Authorization = %q, want static headers preserved", got) + } + + // Without any inbound headers, nothing is added. + h, err = provider(context.Background(), mcp.CallToolRequest{}) + if err != nil { + t.Fatal(err) + } + if got := h.Get("X-Forwarded-User"); got != "" { + t.Errorf("X-Forwarded-User = %q, want empty", got) + } +} diff --git a/main.go b/main.go index 6434ded..6bbc6cc 100644 --- a/main.go +++ b/main.go @@ -44,6 +44,29 @@ func operatorIdentityHeaders(base grpcmcp.ToolHeaderProvider) grpcmcp.ToolHeader } } +// forwardHeaders wraps base to copy each named header from the inbound MCP +// request onto the outbound gRPC call, if present. Like operatorIdentityHeaders, +// this trusts whatever minted the header -- grpcmcp does not verify it -- so +// it is only as safe as the proxy in front of this server. Unlike +// operatorIdentityHeaders, the header name is not fixed: this is the general +// mechanism for a reverse proxy (e.g. an OAuth-terminating proxy) that +// authenticates the caller and asserts identity via its own header names +// (e.g. X-Forwarded-User) rather than X-Operator-Identity specifically. +func forwardHeaders(base grpcmcp.ToolHeaderProvider, names []string) grpcmcp.ToolHeaderProvider { + return func(ctx context.Context, req mcp.CallToolRequest) (http.Header, error) { + h, err := base(ctx, req) + if err != nil { + return nil, err + } + for _, name := range names { + if v := req.Header.Get(name); v != "" { + h.Set(name, v) + } + } + return h, nil + } +} + type headerFlags http.Header func (s *headerFlags) String() string { @@ -192,6 +215,11 @@ func main() { useConnect := flag.Bool("connect", false, "Use connect protocol (instead of gRPC)") requireMethodOption := flag.String("require-method-option", "", "Only expose methods with this option (fieldNumber:value or fieldNumber:value1,value2, e.g. 50003:1 or 50003:1,2)") forwardOperatorIdentity := flag.Bool("forward-operator-identity", false, "Copy the X-Operator-Identity header from inbound MCP requests onto outbound gRPC calls. The header must be minted by a trusted proxy in front of this server; grpcmcp does not verify it.") + var forwardHeaderNames []string + flag.Func("forward-header", "Copy a named header from inbound MCP requests onto outbound gRPC calls, if present. Repeatable. The header must be minted by a trusted proxy in front of this server; grpcmcp does not verify it.", func(v string) error { + forwardHeaderNames = append(forwardHeaderNames, v) + return nil + }) string64 := flag.Bool("string64", false, "Expose 64-bit protobuf integer fields as strings only in JSON schemas") refreshInterval := flag.Duration("refresh-interval", 5*time.Minute, "How often to re-run reflection so new backend methods appear. Applies when reflect is set without descriptors.") refreshTimeout := flag.Duration("refresh-timeout", time.Minute, "Time limit for one reflection refresh attempt") @@ -243,6 +271,14 @@ func main() { fmt.Fprint(os.Stderr, "-tls-crt, -tls-key, and -ca-file need -hostport. Without it the server uses stdio, which has no TLS.\n") os.Exit(-1) } + // Same reasoning: stdio has no inbound HTTP headers to forward, so these + // flags would silently do nothing rather than forward anything, which is + // worse than an error for something an operator is relying on for identity + // attribution. + if (*forwardOperatorIdentity || len(forwardHeaderNames) > 0) && !serveHTTP { + fmt.Fprint(os.Stderr, "-forward-operator-identity and -forward-header need -hostport. Without it the server uses stdio, which has no HTTP headers to forward.\n") + os.Exit(-1) + } tlsBackendClient, err := backendTLSClient(*clientCAFile, *clientTLSCrt, *clientTLSKey) if err != nil { @@ -289,6 +325,23 @@ func main() { } } + // forwardHeaders and operatorIdentityHeaders overwrite whatever base already + // set for that header name (see forwardHeaders' doc comment). If a forwarded + // name collides with one set here from -header or -bearer-env -- most + // dangerously Authorization -- an inbound MCP client would silently replace + // grpcmcp's own trusted backend credential on every call. Reject that + // configuration outright rather than let it happen quietly. + for _, name := range forwardHeaderNames { + if http.Header(headers).Get(name) != "" { + fmt.Fprintf(os.Stderr, "-forward-header=%q collides with a header already set via -header or -bearer-env; refusing to let an inbound client override it.\n", name) + os.Exit(-1) + } + } + if *forwardOperatorIdentity && http.Header(headers).Get(operatorIdentityHeader) != "" { + fmt.Fprintf(os.Stderr, "-forward-operator-identity collides with a -header value already set for %s; refusing to let an inbound client override it.\n", operatorIdentityHeader) + os.Exit(-1) + } + ctx := context.Background() if *descriptors == "" && !*reflect { @@ -329,6 +382,9 @@ func main() { if *forwardOperatorIdentity { headersProvider = operatorIdentityHeaders(headersProvider) } + if len(forwardHeaderNames) > 0 { + headersProvider = forwardHeaders(headersProvider, forwardHeaderNames) + } // listChanged promises the client a notification when the tool set changes. // Only stdio can keep that promise. The HTTP server is stateless and serves diff --git a/scripts/e2e_test.sh b/scripts/e2e_test.sh index 9ec26af..1daf68c 100755 --- a/scripts/e2e_test.sh +++ b/scripts/e2e_test.sh @@ -27,7 +27,8 @@ WORKDIR=$(mktemp -d) BACKEND_PID= HTTP_PID= SSE_PID= -trap 'kill $BACKEND_PID $HTTP_PID $SSE_PID 2>/dev/null; wait 2>/dev/null; rm -rf "$WORKDIR"' EXIT +FWD_PID= +trap 'kill $BACKEND_PID $HTTP_PID $SSE_PID $FWD_PID 2>/dev/null; wait 2>/dev/null; rm -rf "$WORKDIR"' EXIT echo "== building ==" go build -o "$WORKDIR/grpcmcp" . || exit 1 @@ -48,9 +49,15 @@ echo "== starting grpcmcp --transport=sse (:8092) ==" SSE_PID=$! sleep 0.5 +echo "== starting grpcmcp --forward-header=X-Forwarded-User (:8093) ==" +"$WORKDIR/grpcmcp" --hostport=localhost:8093 --reflect --transport=http --forward-header=X-Forwarded-User >"$WORKDIR/fwd.log" 2>&1 & +FWD_PID=$! +sleep 0.5 + if ! kill -0 "$BACKEND_PID" 2>/dev/null; then echo "backend failed to start"; cat "$WORKDIR"/*.log; exit 1; fi if ! kill -0 "$HTTP_PID" 2>/dev/null; then echo "http server failed to start"; cat "$WORKDIR/http.log"; exit 1; fi if ! kill -0 "$SSE_PID" 2>/dev/null; then echo "sse server failed to start"; cat "$WORKDIR/sse.log"; exit 1; fi +if ! kill -0 "$FWD_PID" 2>/dev/null; then echo "forward-header server failed to start"; cat "$WORKDIR/fwd.log"; exit 1; fi INIT_BODY='{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"e2e","version":"1"}}}' LIST_BODY='{"jsonrpc":"2.0","id":2,"method":"tools/list"}' @@ -89,6 +96,25 @@ status=$(curl -s -o /dev/null -w '%{http_code}' -X POST http://localhost:8091/mc check "POST /mcp with bogus Mcp-Protocol-Version -> 400" "$status" "400" echo +echo +echo "== --forward-header=X-Forwarded-User (:8093) ==" + +curl -s -X POST http://localhost:8093/mcp \ + -H 'Content-Type: application/json' -H 'Accept: application/json, text/event-stream' \ + -d "$INIT_BODY" > /dev/null + +fwd_result=$(curl -s -X POST http://localhost:8093/mcp \ + -H 'Content-Type: application/json' -H 'Accept: application/json, text/event-stream' \ + -H 'X-Forwarded-User: alice@basiccapital.com' \ + -d "$CALL_BODY") +if echo "$fwd_result" | grep -q 'hello-e2e|alice@basiccapital.com'; then + echo " ok - X-Forwarded-User reached the backend over a real gRPC call" + PASS=$((PASS+1)) +else + echo " FAIL - X-Forwarded-User did not reach the backend: $fwd_result" + FAIL=$((FAIL+1)) +fi + echo "== SSE (:8092, deprecated) ==" if grep -q '\[deprecated\].*transport=sse' "$WORKDIR/sse.log"; then