From d9087fd0b7fef8f98f7431ad2999f476c5364f69 Mon Sep 17 00:00:00 2001 From: viettranx Date: Fri, 27 Mar 2026 23:23:14 +0700 Subject: [PATCH] fix(security): harden MCP bridge tenant_id propagation - Guard uuid.Nil before converting to string in loop.go to avoid sending zero UUID as X-Tenant-ID header when no tenant is set - Return (ok, tenantVerified) from VerifyBridgeContext so callers only inject tenant_id when HMAC actually covers it (level 1) - Add unit tests for SignBridgeContext and VerifyBridgeContext including security test for tenant injection via HMAC fallback --- internal/agent/loop.go | 4 +- internal/gateway/server.go | 7 +- internal/providers/claude_cli_mcp.go | 14 ++- internal/providers/claude_cli_mcp_test.go | 120 ++++++++++++++++++++++ 4 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 internal/providers/claude_cli_mcp_test.go diff --git a/internal/agent/loop.go b/internal/agent/loop.go index b62c9b50..5a563493 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -519,9 +519,11 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) providers.OptChatID: req.ChatID, providers.OptPeerKind: req.PeerKind, providers.OptWorkspace: tools.ToolWorkspaceFromCtx(ctx), - providers.OptTenantID: store.TenantIDFromContext(ctx).String(), }, } + if tid := store.TenantIDFromContext(ctx); tid != uuid.Nil { + chatReq.Options[providers.OptTenantID] = tid.String() + } if l.thinkingLevel != "" && l.thinkingLevel != "off" { if tc, ok := l.provider.(providers.ThinkingCapable); ok && tc.SupportsThinking() { chatReq.Options[providers.OptThinkingLevel] = l.thinkingLevel diff --git a/internal/gateway/server.go b/internal/gateway/server.go index 382ac0be..107a3c7a 100644 --- a/internal/gateway/server.go +++ b/internal/gateway/server.go @@ -227,7 +227,8 @@ func bridgeContextMiddleware(gatewayToken string, next http.Handler) http.Handle // Verify HMAC signature over all context fields. tenantIDStr := r.Header.Get("X-Tenant-ID") sig := r.Header.Get("X-Bridge-Sig") - if !providers.VerifyBridgeContext(gatewayToken, agentIDStr, userID, channel, chatID, peerKind, workspace, tenantIDStr, sig) { + ok, tenantVerified := providers.VerifyBridgeContext(gatewayToken, agentIDStr, userID, channel, chatID, peerKind, workspace, tenantIDStr, sig) + if !ok { slog.Warn("security.mcp_bridge: invalid bridge context signature", "agent_id", agentIDStr, "user_id", userID) http.Error(w, `{"error":"invalid bridge context signature"}`, http.StatusForbidden) @@ -242,7 +243,9 @@ func bridgeContextMiddleware(gatewayToken string, next http.Handler) http.Handle if userID != "" { ctx = store.WithUserID(ctx, userID) } - if tenantIDStr != "" { + // Only inject tenant_id when HMAC actually covers it (level 1). + // Fallback levels (pre-tenantID sessions) must not trust unsigned tenant headers. + if tenantVerified && tenantIDStr != "" { if tid, err := uuid.Parse(tenantIDStr); err == nil { ctx = store.WithTenantID(ctx, tid) } diff --git a/internal/providers/claude_cli_mcp.go b/internal/providers/claude_cli_mcp.go index dcdda2a9..80fa2517 100644 --- a/internal/providers/claude_cli_mcp.go +++ b/internal/providers/claude_cli_mcp.go @@ -264,20 +264,26 @@ func SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspac } // VerifyBridgeContext checks the HMAC signature against the expected bridge context. +// Returns (ok, tenantVerified): ok indicates signature is valid, tenantVerified indicates +// the tenantID field was covered by the HMAC (only true at level 1). // Falls back to old formats for backward compatibility with sessions whose MCP config // was written before the workspace or tenantID fields were added. -func VerifyBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID, sig string) bool { +// Callers must NOT trust the tenantID header when tenantVerified is false. +func VerifyBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID, sig string) (bool, bool) { // Current format: all fields including tenantID expected := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID) if hmac.Equal([]byte(expected), []byte(sig)) { - return true + return true, true } // Fallback: without tenantID (pre-tenantID sessions) noTenant := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, "") if hmac.Equal([]byte(noTenant), []byte(sig)) { - return true + return true, false } // Fallback: without workspace or tenantID (oldest sessions) old := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, "", "") - return hmac.Equal([]byte(old), []byte(sig)) + if hmac.Equal([]byte(old), []byte(sig)) { + return true, false + } + return false, false } diff --git a/internal/providers/claude_cli_mcp_test.go b/internal/providers/claude_cli_mcp_test.go new file mode 100644 index 00000000..527cd21d --- /dev/null +++ b/internal/providers/claude_cli_mcp_test.go @@ -0,0 +1,120 @@ +package providers + +import ( + "testing" +) + +// --- SignBridgeContext tests --- + +func TestSignBridgeContext_Deterministic(t *testing.T) { + key := "test-secret" + sig1 := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/workspace", "tenant-abc") + sig2 := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/workspace", "tenant-abc") + if sig1 != sig2 { + t.Errorf("expected deterministic output, got %q and %q", sig1, sig2) + } + if sig1 == "" { + t.Error("expected non-empty signature") + } +} + +func TestSignBridgeContext_DifferentKey(t *testing.T) { + sig1 := SignBridgeContext("key-a", "agent1", "user1", "", "", "", "", "") + sig2 := SignBridgeContext("key-b", "agent1", "user1", "", "", "", "", "") + if sig1 == sig2 { + t.Error("different keys should produce different signatures") + } +} + +func TestSignBridgeContext_FieldOrder(t *testing.T) { + key := "test-secret" + sig1 := SignBridgeContext(key, "a", "b", "c", "d", "e", "f", "g") + sig2 := SignBridgeContext(key, "b", "a", "c", "d", "e", "f", "g") + if sig1 == sig2 { + t.Error("swapping field values should produce different signatures") + } +} + +// --- VerifyBridgeContext tests --- + +func TestVerifyBridgeContext_Level1_AllFields(t *testing.T) { + key := "gateway-token" + sig := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "tenant-123") + + ok, tenantVerified := VerifyBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "tenant-123", sig) + if !ok { + t.Error("expected ok=true for valid level 1 signature") + } + if !tenantVerified { + t.Error("expected tenantVerified=true for level 1 match") + } +} + +func TestVerifyBridgeContext_Level2_OldSessionWithWorkspace(t *testing.T) { + key := "gateway-token" + // Pre-tenantID session: signed with workspace but empty tenantID. + // Middleware now receives X-Tenant-ID header (e.g. new code adds it). + // Level 1 fails (tenantID mismatch), level 2 matches (ignores tenantID). + sig := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "") + + ok, tenantVerified := VerifyBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "new-tenant-id", sig) + if !ok { + t.Error("expected ok=true for level 2 fallback") + } + if tenantVerified { + t.Error("expected tenantVerified=false — tenant was not in original signature") + } +} + +func TestVerifyBridgeContext_Level3_NoWorkspaceNoTenant(t *testing.T) { + key := "gateway-token" + // Signature from the oldest format (no workspace, no tenantID) + sig := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "", "") + + ok, tenantVerified := VerifyBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "tenant-123", sig) + if !ok { + t.Error("expected ok=true for level 3 fallback") + } + if tenantVerified { + t.Error("expected tenantVerified=false for level 3 fallback") + } +} + +func TestVerifyBridgeContext_InvalidSig(t *testing.T) { + ok, tenantVerified := VerifyBridgeContext("key", "agent1", "user1", "", "", "", "", "", "invalid-sig") + if ok { + t.Error("expected ok=false for invalid signature") + } + if tenantVerified { + t.Error("expected tenantVerified=false for invalid signature") + } +} + +func TestVerifyBridgeContext_TenantNotTrustedOnFallback(t *testing.T) { + key := "gateway-token" + // Old session signed WITHOUT tenantID + oldSig := SignBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "") + + // Attacker replays old sig but adds a fake tenantID header + ok, tenantVerified := VerifyBridgeContext(key, "agent1", "user1", "telegram", "chat1", "direct", "/ws", "fake-tenant-id", oldSig) + if !ok { + t.Error("expected ok=true (sig valid via fallback)") + } + if tenantVerified { + t.Error("expected tenantVerified=false — tenant header not covered by HMAC, must not be trusted") + } +} + +func TestVerifyBridgeContext_EmptyFields(t *testing.T) { + key := "test-key" + sig := SignBridgeContext(key, "", "", "", "", "", "", "") + + ok, tenantVerified := VerifyBridgeContext(key, "", "", "", "", "", "", "", sig) + if !ok { + t.Error("expected ok=true for empty fields with valid signature") + } + // Empty fields match at all levels; level 1 matches first → tenantVerified=true + if !tenantVerified { + t.Error("expected tenantVerified=true when all fields empty (level 1 matches)") + } +}