mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 12:18:59 +00:00
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
This commit is contained in:
1 parent
6551c2d1e2
commit
d9087fd0b7
4 files changed
+138
-7
No files matched your search
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user