diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e9c373c..6a3c10f 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -2,3 +2,9 @@ **Vulnerability:** TOCTOU race condition in `pkg/signing/verify.go` allowed concurrent duplicate requests with identical nonces to bypass replay protection because `isReplayedNonce` checked nonces before `recordNonce` was called at the end of message verification. **Learning:** Checking nonces separately from recording them leaves a race condition window under concurrent load, and checking nonces before signature verification allows unauthenticated requests to pollute or query nonce tracking. **Prevention:** Perform cryptographic signature verification first, followed by an atomic check-and-record operation for nonces under mutex lock. + +## 2026-08-13 - Fail-Closed Signature Verification for Relay Messages +**Vulnerability:** Newly added proxy actions (e.g. Kafka actions, Mongo status/stats, Redis info/slowlog/client list) were missing from an explicit opt-in `signedActions` map in `pkg/ws/handler.go`, allowing unsigned messages for those actions to silently bypass signature verification even when message signing was enabled. +**Learning:** Opt-in authorization/verification maps are fail-open security antipatterns. When new proxy modules or sub-actions are added, forgetting to register them in an explicit map leaves unauthenticated execution vectors. +**Prevention:** Enforce signature verification uniformly for all incoming control plane messages in `HandleMessage` (fail-closed, secure by default) when signature verification is enabled, eliminating manual opt-in map maintenance. + diff --git a/go.mod b/go.mod index 683fad8..d73015c 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module nudgebee/forager -go 1.25.12 +go 1.25.13 require ( cloud.google.com/go/auth v0.22.0 diff --git a/pkg/signing/sign.go b/pkg/signing/sign.go index d2fcf49..b38b3d5 100644 --- a/pkg/signing/sign.go +++ b/pkg/signing/sign.go @@ -91,6 +91,9 @@ var SigningFields = map[string][]string{ // Config sync: what datasources are being configured "datasource_config_sync": {"action", "account_id", "datasources"}, + // Config test: temporary datasource configuration and credentials + "test_datasource_config": {"action", "datasource"}, + // Action requests (new format): what action on which datasource with what params "db_query": {"action", "datasource_id", "params"}, "db_execute": {"action", "datasource_id", "params"}, @@ -131,21 +134,31 @@ func (s *Signer) Sign(msg []byte) ([]byte, error) { if actionRaw, ok := raw["action"]; ok { _ = json.Unmarshal(actionRaw, &action) } - // For legacy format, try body.action_name - if action == "" { - if bodyRaw, ok := raw["body"]; ok { - var body map[string]json.RawMessage - if json.Unmarshal(bodyRaw, &body) == nil { - if actionNameRaw, ok := body["action_name"]; ok { - _ = json.Unmarshal(actionNameRaw, &action) - } + // For legacy format, check body.action_name + isLegacyAction := false + if bodyRaw, ok := raw["body"]; ok { + var body struct { + ActionName string `json:"action_name"` + } + if json.Unmarshal(bodyRaw, &body) == nil && body.ActionName != "" { + isLegacyAction = true + if action == "" { + action = body.ActionName } } } fields := DefaultSigningFields - if f, ok := SigningFields[action]; ok { + if isLegacyAction { + // Legacy action request format: fields live inside top-level body object + fields = []string{"body"} + } else if f, ok := SigningFields[action]; ok { fields = f + } else if action == "" { + // Legacy HTTP proxy request format (no action field, has url) + if _, ok := raw["url"]; ok { + fields = []string{"method", "url", "header", "body"} + } } // Extract the fields to sign diff --git a/pkg/signing/verify_test.go b/pkg/signing/verify_test.go index 46eec59..4edb70d 100644 --- a/pkg/signing/verify_test.go +++ b/pkg/signing/verify_test.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "time" ) @@ -446,3 +447,104 @@ func TestVerify_ConcurrentReplay(t *testing.T) { t.Fatalf("expected %d replay rejections, got %d", goroutines-1, replayCount) } } + +func TestSignAndVerify_TestDatasourceConfig_AntiTamper(t *testing.T) { + pub, priv := generateTestKeypair() + v, err := NewVerifier(base64.StdEncoding.EncodeToString(pub), testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + + signer := testSigner(t, priv) + msg := []byte(`{"action":"test_datasource_config","datasource":{"type":"postgresql","config":{"host":"localhost"}}}`) + signed, err := signer.Sign(msg) + if err != nil { + t.Fatalf("Sign: %v", err) + } + + if err := v.Verify(signed); err != nil { + t.Fatalf("Verify valid test_datasource_config failed: %v", err) + } + + // Tamper with datasource config payload + tampered := strings.Replace(string(signed), "localhost", "evil.com", 1) + if err := v.Verify([]byte(tampered)); err == nil { + t.Fatal("expected verification error for tampered test_datasource_config host") + } +} + +func TestSignAndVerify_LegacyHTTP_AntiTamper(t *testing.T) { + pub, priv := generateTestKeypair() + v, err := NewVerifier(base64.StdEncoding.EncodeToString(pub), testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + + signer := testSigner(t, priv) + msg := []byte(`{"method":"GET","url":"/api/v1/metrics","header":{},"body":""}`) + signed, err := signer.Sign(msg) + if err != nil { + t.Fatalf("Sign: %v", err) + } + + if err := v.Verify(signed); err != nil { + t.Fatalf("Verify valid legacy HTTP request failed: %v", err) + } + + // Tamper with URL + tampered := strings.Replace(string(signed), "/api/v1/metrics", "/api/v1/admin/delete", 1) + if err := v.Verify([]byte(tampered)); err == nil { + t.Fatal("expected verification error for tampered legacy HTTP request URL") + } +} + +func TestSignAndVerify_LegacyAction_AntiTamper(t *testing.T) { + pub, priv := generateTestKeypair() + v, err := NewVerifier(base64.StdEncoding.EncodeToString(pub), testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + + signer := testSigner(t, priv) + msg := []byte(`{"body":{"action_name":"db_query","action_params":{"datasource_id":"ds-1"}}}`) + signed, err := signer.Sign(msg) + if err != nil { + t.Fatalf("Sign: %v", err) + } + + if err := v.Verify(signed); err != nil { + t.Fatalf("Verify valid legacy action request failed: %v", err) + } + + // Tamper with action_params + tampered := strings.Replace(string(signed), "ds-1", "ds-2", 1) + if err := v.Verify([]byte(tampered)); err == nil { + t.Fatal("expected verification error for tampered legacy action request params") + } +} + +func TestSignAndVerify_LegacyAction_EmptyActionField_AntiTamper(t *testing.T) { + pub, priv := generateTestKeypair() + v, err := NewVerifier(base64.StdEncoding.EncodeToString(pub), testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + + signer := testSigner(t, priv) + // Message with empty action field but legacy body format + msg := []byte(`{"action":"","body":{"action_name":"db_query","action_params":{"datasource_id":"ds-1"}}}`) + signed, err := signer.Sign(msg) + if err != nil { + t.Fatalf("Sign: %v", err) + } + + if err := v.Verify(signed); err != nil { + t.Fatalf("Verify valid legacy action request with empty action failed: %v", err) + } + + // Tamper with action_params + tampered := strings.Replace(string(signed), "ds-1", "ds-2", 1) + if err := v.Verify([]byte(tampered)); err == nil { + t.Fatal("expected verification error for tampered legacy action request params when action field is empty") + } +} diff --git a/pkg/ws/discovery_wire_test.go b/pkg/ws/discovery_wire_test.go index 6cc3cef..0628c22 100644 --- a/pkg/ws/discovery_wire_test.go +++ b/pkg/ws/discovery_wire_test.go @@ -166,17 +166,35 @@ func (r *recordingProxy) Close() error { return nil } // This pins them, so adding a fourth without registering it fails here rather // than in production. func TestEveryDiscoveryActionRequiresSignature(t *testing.T) { - // Mirrors the actions discovery.Proxy.HandleRequest dispatches on. + dummyKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAG5e/k5wQ5l5X+5b5W5d5e5f5g5h5i5j5k5l5m5n5o5 test@test" + verifier, err := signing.NewVerifier(dummyKey, testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + h := &Handler{registry: proxy.NewRegistry(), verifier: verifier, logger: testLogger()} + for _, action := range []string{"discovery_sweep", "discovery_ldap", "discovery_inventory"} { - if !signedActions[action] { - t.Errorf("%s is not in signedActions — it would bypass signature verification", action) + msg, _ := json.Marshal(map[string]any{"action": action, "request_id": "req-1"}) + resp, err := h.HandleMessage(context.Background(), msg) + if err != nil { + t.Fatalf("HandleMessage failed: %v", err) + } + var r proxy.ActionResponse + _ = json.Unmarshal(resp, &r) + if r.StatusCode != 403 { + t.Errorf("%s expected 403 for unsigned message, got %d", action, r.StatusCode) } } } -// The allowlist's default is the hazard, so state the invariant that matters: -// anything that reaches a host or a network must be signed. func TestActionsThatTouchRemoteSystemsAreSigned(t *testing.T) { + dummyKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAG5e/k5wQ5l5X+5b5W5d5e5f5g5h5i5j5k5l5m5n5o5 test@test" + verifier, err := signing.NewVerifier(dummyKey, testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + h := &Handler{registry: proxy.NewRegistry(), verifier: verifier, logger: testLogger()} + mustBeSigned := []string{ "ssh_command", "ssh_upload", "ssh_download", "ssh_list_dir", "db_query", "db_execute", @@ -185,8 +203,15 @@ func TestActionsThatTouchRemoteSystemsAreSigned(t *testing.T) { "datasource_config_sync", "test_datasource_config", } for _, action := range mustBeSigned { - if !signedActions[action] { - t.Errorf("%s executes against a remote system but is not signed", action) + msg, _ := json.Marshal(map[string]any{"action": action, "request_id": "req-1"}) + resp, err := h.HandleMessage(context.Background(), msg) + if err != nil { + t.Fatalf("HandleMessage failed: %v", err) + } + var r proxy.ActionResponse + _ = json.Unmarshal(resp, &r) + if r.StatusCode != 403 { + t.Errorf("%s expected 403 for unsigned message, got %d", action, r.StatusCode) } } } diff --git a/pkg/ws/handler.go b/pkg/ws/handler.go index e657058..b01b036 100644 --- a/pkg/ws/handler.go +++ b/pkg/ws/handler.go @@ -20,53 +20,6 @@ import ( "nudgebee/forager/pkg/signing" ) -// signedActions are actions that require signature verification when signing is enabled. -// All actions that can modify state or execute commands should be listed here. -var signedActions = map[string]bool{ - // Config sync — can push arbitrary datasources including RCE via MCP stdio - "datasource_config_sync": true, - - // Database — arbitrary SQL execution - "db_query": true, - "db_execute": true, - "db_metadata": true, - - // SSH — arbitrary command execution, file read/write - "ssh_command": true, - "ssh_upload": true, - "ssh_download": true, - "ssh_list_dir": true, - - // HTTP — SSRF, credential theft via redirect - "http_request": true, - - // MCP — arbitrary JSON-RPC to local processes - "mcp_request": true, - - // MongoDB — arbitrary queries/aggregations - "mongo_query": true, - "mongo_aggregate": true, - - // Redis — arbitrary command execution - "redis_command": true, - - // Config test — creates temporary proxy to test connectivity - "test_datasource_config": true, - - // Discovery — inventory executes commands on remote hosts over SSH, the - // same capability as ssh_command above. The commands themselves come - // from a signature-verified content pack, but the targets do not: an - // unsigned action lets a caller choose which hosts we connect to. - // - // Sweep sends probes across a network segment. Its scope is bounded by - // allowed_cidrs, but triggering one is not harmless — an unexpected scan - // reads as an attack originating from our agent, and some IDS and - // fail2ban configurations act on it. - "discovery_sweep": true, - "discovery_ldap": true, - "discovery_inventory": true, -} - // Handler dispatches incoming relay messages to the appropriate proxy module. type Handler struct { registry *proxy.Registry @@ -91,45 +44,37 @@ func NewHandler(registry *proxy.Registry, credStore *secrets.CloudPushStore, sec // All requests use a unified format: {request_id, datasource_id, action, params, ...} func (h *Handler) HandleMessage(ctx context.Context, msg []byte) ([]byte, error) { var envelope struct { - Action string `json:"action"` - RequestID string `json:"request_id"` - DatasourceID string `json:"datasource_id"` - Body struct { - ActionName string `json:"action_name"` - } `json:"body"` + Action string `json:"action"` + RequestID string `json:"request_id"` + DatasourceID string `json:"datasource_id"` + Body json.RawMessage `json:"body"` } if err := json.Unmarshal(msg, &envelope); err != nil { return nil, fmt.Errorf("unmarshal envelope: %w", err) } - // Resolve the effective action — legacy messages use body.action_name + // Resolve the effective action — legacy action messages use body.action_name effectiveAction := envelope.Action - if effectiveAction == "" { - effectiveAction = envelope.Body.ActionName + if effectiveAction == "" && len(envelope.Body) > 0 { + var body struct { + ActionName string `json:"action_name"` + } + _ = json.Unmarshal(envelope.Body, &body) + effectiveAction = body.ActionName } - // Verify signature for actions that require it - if signedActions[effectiveAction] { - if err := h.verifier.Verify(msg); err != nil { - h.logger.Error("message signature verification failed", - "action", effectiveAction, - "request_id", envelope.RequestID, - "err", err, - ) - if h.verifier.Enabled() { - return h.buildErrorResponse(envelope.RequestID, 403, "signature verification failed"), nil - } - } + // Verify signature for all incoming messages (fail-closed, secure by default) + if h.verifier == nil { + h.logger.Error("verifier is not initialized", "request_id", envelope.RequestID) + return h.buildErrorResponse(envelope.RequestID, 500, "internal server error: verifier not initialized"), nil } - // Legacy HTTP proxy requests (no action field) also require verification when signing is enabled - if effectiveAction == "" && h.verifier.Enabled() { - if err := h.verifier.Verify(msg); err != nil { - h.logger.Error("unsigned legacy HTTP request rejected", - "request_id", envelope.RequestID, - "err", err, - ) - return h.buildErrorResponse(envelope.RequestID, 403, "signature verification failed"), nil - } + if err := h.verifier.Verify(msg); err != nil { + h.logger.Error("message signature verification failed", + "action", effectiveAction, + "request_id", envelope.RequestID, + "err", err, + ) + return h.buildErrorResponse(envelope.RequestID, 403, "signature verification failed"), nil } switch envelope.Action { diff --git a/pkg/ws/handler_test.go b/pkg/ws/handler_test.go index 6c7cd43..251dc72 100644 --- a/pkg/ws/handler_test.go +++ b/pkg/ws/handler_test.go @@ -128,6 +128,21 @@ func TestHandler_HandleMessage_HTTPRequest_NoProxy(t *testing.T) { } } +func TestHandler_HandleMessage_HTTPRequest_StringBody(t *testing.T) { + h := newTestHandler(t) + msg := `{"method": "POST", "url": "/api/v1/metrics", "request_id": "req-str-body", "header": {}, "body": "raw-string-body"}` + resp, err := h.HandleMessage(context.Background(), []byte(msg)) + if err != nil { + t.Fatalf("HandleMessage failed for HTTP request with string body: %v", err) + } + + var r proxy.ActionResponse + _ = json.Unmarshal(resp, &r) + if r.StatusCode != 404 { + t.Fatalf("expected 404 for no http-proxy, got %d", r.StatusCode) + } +} + func TestHandler_ConfigSync_HTTPProxy(t *testing.T) { h := newTestHandler(t) @@ -303,3 +318,53 @@ func TestHandler_BuildErrorResponse(t *testing.T) { t.Fatalf("expected req-123, got %s", r.RequestID) } } + +func TestHandler_SignatureEnforcement_FailClosed(t *testing.T) { + // Create verifier with a dummy public key to enable signature verification + dummyKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAG5e/k5wQ5l5X+5b5W5d5e5f5g5h5i5j5k5l5m5n5o5 test@test" + verifier, err := signing.NewVerifier(dummyKey, testLogger()) + if err != nil { + t.Fatalf("NewVerifier: %v", err) + } + + h := &Handler{ + registry: proxy.NewRegistry(), + verifier: verifier, + logger: testLogger(), + } + + // Verify signature enforcement for standard actions as well as unknown/unregistered future actions (fail-closed) + actionsToTest := []string{ + "db_query", + "ssh_command", + "http_request", + "mcp_request", + "kafka_consumer_groups", + "mongo_server_status", + "redis_info", + "unknown_action", + "future_proxy_action", + } + + for _, action := range actionsToTest { + msg := map[string]any{ + "action": action, + "request_id": "req-sig-test", + } + data, _ := json.Marshal(msg) + + resp, err := h.HandleMessage(context.Background(), data) + if err != nil { + t.Fatalf("HandleMessage for action %s failed: %v", action, err) + } + + var r proxy.ActionResponse + if err := json.Unmarshal(resp, &r); err != nil { + t.Fatalf("unmarshal response for action %s: %v", action, err) + } + + if r.StatusCode != 403 { + t.Errorf("action %s expected 403 for unsigned message, got %d", action, r.StatusCode) + } + } +}