From 989862b48cef62acbc4d4a6079e744504d9d3c49 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 5 May 2026 11:49:23 -0400 Subject: [PATCH] [POC] wire AllowedCommandPatterns + DeniedCommandPatterns through PAR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plumbs the two new rshell pattern axes from the runCommand task input to the rshell interpreter: RunCommandInputs gains: AllowedCommandPatterns [][]string DeniedCommandPatterns [][]string Run() passes them straight to interp.AllowedCommandPatterns(...) and interp.DeniedCommandPatterns(...) without any agent-side parsing (the JSON wire format preserves the structured token-list shape natively, so there's nothing to split or unflatten). Operator-side intersection is deliberately skipped — for the POC the backend is authoritative for patterns. CommandSpecs registration is also skipped; rshell's built-in spec for `ip` covers the demo target, and external-command pattern enforcement (kubectl/git/docker) needs an ExecHandler anyway, which is a much bigger lift. Includes a local replace directive in go.mod pointing at ~/dd/rshell. STRIP THIS BEFORE THIS PATCH GRADUATES OUT OF POC — it's dev scaffolding that lets the pattern work be tested before a tagged rshell release lands. Tests: - 5 new tests covering allow-pattern admit/block, deny-pattern block-even-when-name-allows, deny doesn't false-positive on siblings, missing pattern fields don't break the runner. - 4 existing assertions updated for rshell's new "not permitted by policy" error format (was "command not allowed"). Architectural cases (substitution-defeat, structural matching, deny-first precedence) live in rshell's own test suite; here we only verify field plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) --- go.mod | 6 + go.sum | 2 - .../remoteaction/rshell/run_command.go | 33 +++- .../remoteaction/rshell/run_command_test.go | 165 +++++++++++++++++- 4 files changed, 195 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index f79e46f0181e..3e41cc581f7c 100644 --- a/go.mod +++ b/go.mod @@ -1450,3 +1450,9 @@ replace ( github.com/DataDog/datadog-agent/test/new-e2e => ./test/new-e2e github.com/DataDog/datadog-agent/test/otel => ./test/otel ) + +// LOCAL DEV ONLY — points github.com/DataDog/rshell at the developer's +// local checkout so AllowedCommandPatterns/DeniedCommandPatterns work +// before a tagged rshell release. Strip this directive before merging +// out of the POC branch. +replace github.com/DataDog/rshell => /Users/matthew.deguzman/dd/rshell diff --git a/go.sum b/go.sum index 625ada12febc..c3891ca2cba5 100644 --- a/go.sum +++ b/go.sum @@ -2596,8 +2596,6 @@ github.com/DataDog/opa v0.0.0-20251126100856-d2e1e78e0816 h1:8diKw2aLYE6LsjWfYxD github.com/DataDog/opa v0.0.0-20251126100856-d2e1e78e0816/go.mod h1:7cPuErOAt7k/oVWAVJnxqAC6mwArrAazkvk0RXiih2A= github.com/DataDog/opentelemetry-ebpf-profiler v0.0.0-20260416144717-96f8924d6e18 h1:VbUS1B3mpSjponiKDWHXtofoNBp99Nckf5uZaBeQyPo= github.com/DataDog/opentelemetry-ebpf-profiler v0.0.0-20260416144717-96f8924d6e18/go.mod h1:34LtYk9Gh/SCItzi7v8ok9E5lk88AA5I+5aKoVKQClI= -github.com/DataDog/rshell v0.0.14 h1:1BJxm8FQ0uZ6amxItyKmlG4e2y2CPTNvjxtY/Nx1Pg0= -github.com/DataDog/rshell v0.0.14/go.mod h1:r2pK0NQpE1SL+ESYjlfhigWDEELPY5LacwLbisR9nxI= github.com/DataDog/sketches-go v1.4.8 h1:pFk9BNn+Rzv8IMIoPUttoOpOr3bJOqU3P6EP5wK+Lv8= github.com/DataDog/sketches-go v1.4.8/go.mod h1:a/wjRUqzqtGS8qRHRPDCs4EAQfmvPDZGDlMIF5mxXOE= github.com/DataDog/trivy v0.0.0-20260407220859-6cf8ddc1826c h1:oiwmRrkRVblZBI3SFVEnrwxkQ28W7nmwwI/VHgex0ZU= diff --git a/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go b/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go index 0f12aec06400..6b6c3e2b39ed 100644 --- a/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go +++ b/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go @@ -117,10 +117,21 @@ func (h *RunCommandHandler) filterAllowedPaths(backend []string) []string { // respective axis — rshell refuses to run any command or open any file. // A non-nil list is intersected with the operator config before being // handed to rshell. +// +// AllowedCommandPatterns and DeniedCommandPatterns are passed straight +// through to rshell without operator-side intersection. Each pattern is +// a token list; the agent does no parsing because the JSON wire format +// preserves the structured shape natively. See rshell's +// interp.AllowedCommandPatterns for matching semantics. The deny axis +// is evaluated first by rshell, so an entry in DeniedCommandPatterns +// blocks even when AllowedCommands or AllowedCommandPatterns would +// otherwise admit the call. type RunCommandInputs struct { - Command string `json:"command"` - AllowedCommands []string `json:"allowedCommands"` - AllowedPaths map[string][]string `json:"allowedPaths"` + Command string `json:"command"` + AllowedCommands []string `json:"allowedCommands"` + AllowedCommandPatterns [][]string `json:"allowedCommandPatterns,omitempty"` + DeniedCommandPatterns [][]string `json:"deniedCommandPatterns,omitempty"` + AllowedPaths map[string][]string `json:"allowedPaths"` } // RunCommandOutputs defines the outputs for the runCommand action. @@ -172,13 +183,25 @@ func (h *RunCommandHandler) Run( // Route sandbox diagnostics to a dedicated sink so they do not leak // into the action's stderr field. We discard the streaming output and // read the messages back via runner.Warnings() into SandboxWarnings. - runner, err := interp.New( + // + // Pattern axes are passed through directly — rshell validates token + // shape and (for multi-token patterns) requires a registered + // CommandSpec at New() time. Operator-side pattern intersection is + // out of scope for the POC; the backend is authoritative. + opts := []interp.RunnerOption{ interp.StdIO(nil, &stdout, &stderr), interp.WarningsWriter(io.Discard), interp.AllowedPaths(effectiveAllowedPaths), interp.ProcPath(resolveProcPath()), interp.AllowedCommands(effectiveAllowedCommands), - ) + } + if len(inputs.AllowedCommandPatterns) > 0 { + opts = append(opts, interp.AllowedCommandPatterns(inputs.AllowedCommandPatterns)) + } + if len(inputs.DeniedCommandPatterns) > 0 { + opts = append(opts, interp.DeniedCommandPatterns(inputs.DeniedCommandPatterns)) + } + runner, err := interp.New(opts...) if err != nil { return nil, fmt.Errorf("failed to create runner: %w", err) } diff --git a/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command_test.go b/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command_test.go index 53be4d6c7e2a..0c8c1290f1f9 100644 --- a/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command_test.go +++ b/pkg/privateactionrunner/bundles/remoteaction/rshell/run_command_test.go @@ -396,7 +396,7 @@ func TestRunCommandNoAllowedCommandsBlocksExecution(t *testing.T) { require.NoError(t, err) result := out.(*RunCommandOutputs) assert.Equal(t, 127, result.ExitCode) - assert.Contains(t, result.Stderr, "command not allowed") + assert.Contains(t, result.Stderr, "not permitted by policy") } func TestRunCommandWithWildcardOperatorAndBackendAllowed(t *testing.T) { @@ -424,7 +424,7 @@ func TestRunCommandDisallowedCommandBlocked(t *testing.T) { require.NoError(t, err) result := out.(*RunCommandOutputs) assert.Equal(t, 127, result.ExitCode) - assert.Contains(t, result.Stderr, "command not allowed") + assert.Contains(t, result.Stderr, "not permitted by policy") } func TestRunCommandOperatorIntersectionAllows(t *testing.T) { @@ -452,7 +452,7 @@ func TestRunCommandOperatorIntersectionBlocksDisjoint(t *testing.T) { require.NoError(t, err) result := out.(*RunCommandOutputs) assert.Equal(t, 127, result.ExitCode) - assert.Contains(t, result.Stderr, "command not allowed") + assert.Contains(t, result.Stderr, "not permitted by policy") } func TestRunCommandOperatorEmptyListBlocksEverything(t *testing.T) { @@ -465,7 +465,7 @@ func TestRunCommandOperatorEmptyListBlocksEverything(t *testing.T) { require.NoError(t, err) result := out.(*RunCommandOutputs) assert.Equal(t, 127, result.ExitCode) - assert.Contains(t, result.Stderr, "command not allowed") + assert.Contains(t, result.Stderr, "not permitted by policy") } func TestRunCommandBackendAllowedPathsRestrictsAccess(t *testing.T) { @@ -577,3 +577,160 @@ func TestResolveProcPathContainerizedWithoutHostMount(t *testing.T) { assert.Equal(t, "/proc", result) } + +// --- AllowedCommandPatterns / DeniedCommandPatterns plumbing --- +// +// The agent passes both pattern axes through to rshell unchanged — no +// parsing, no operator-side intersection. These tests verify the +// JSON-shaped task input flows correctly through ExtractInputs and into +// interp.New as the right RunnerOption. The architectural cases +// (substitution-defeat, structural matching, deny-first precedence) +// live in rshell's own test suite; here we only confirm the plumbing. + +// makeTaskWithPatterns extends makeTask with the two pattern axes. +// Either field may be nil to exercise "backend didn't send it". +func makeTaskWithPatterns(command string, allowedCommands []string, allowedPatterns, deniedPatterns [][]string, allowedPaths map[string][]string) *types.Task { + task := makeTask(command, allowedCommands) + if allowedPatterns != nil { + task.Data.Attributes.Inputs["allowedCommandPatterns"] = allowedPatterns + } + if deniedPatterns != nil { + task.Data.Attributes.Inputs["deniedCommandPatterns"] = deniedPatterns + } + if allowedPaths != nil { + task.Data.Attributes.Inputs["allowedPaths"] = allowedPaths + } + return task +} + +// TestAllowedCommandPatternsAdmitMatchingArgv verifies that an allow +// pattern from the backend reaches rshell and admits a matching +// invocation. ip is a built-in rshell command with a registered +// CommandSpec, so multi-token patterns work without extra config. +func TestAllowedCommandPatternsAdmitMatchingArgv(t *testing.T) { + handler := NewRunCommandHandler( + []string{setup.RShellPathAllowAll}, + []string{setup.RShellCommandAllowAllWildcard}, + ) + + out, err := handler.Run(context.Background(), + makeTaskWithPatterns( + "ip route show", + nil, // no name allowlist; pattern is the only authorisation + [][]string{{"ip", "route"}}, + nil, + map[string][]string{ + setup.RShellPathAllowMapDefaultKey: {"/tmp"}, + }, + ), nil) + + require.NoError(t, err) + result := out.(*RunCommandOutputs) + // Exit code is whatever ip itself returns (1 on macOS, 0 on Linux — + // either way NOT 127, which is the policy-refusal code). + assert.NotEqual(t, 127, result.ExitCode, "policy gate should admit; rshell stderr was: %q", result.Stderr) +} + +// TestAllowedCommandPatternsBlockNonMatchingArgv is the partner case: +// the pattern doesn't match, so the gate refuses with 127. +func TestAllowedCommandPatternsBlockNonMatchingArgv(t *testing.T) { + handler := NewRunCommandHandler( + []string{setup.RShellPathAllowAll}, + []string{setup.RShellCommandAllowAllWildcard}, + ) + + out, err := handler.Run(context.Background(), + makeTaskWithPatterns( + "ip addr show", + nil, + [][]string{{"ip", "route"}}, + nil, + map[string][]string{ + setup.RShellPathAllowMapDefaultKey: {"/tmp"}, + }, + ), nil) + + require.NoError(t, err) + result := out.(*RunCommandOutputs) + assert.Equal(t, 127, result.ExitCode) + assert.Contains(t, result.Stderr, "not permitted by policy") +} + +// TestDeniedCommandPatternsBlockEvenWhenNameAllowlistAdmits is the +// headline use case: allow ip wholesale by name, then carve out +// ip route specifically via a deny pattern. +func TestDeniedCommandPatternsBlockEvenWhenNameAllowlistAdmits(t *testing.T) { + handler := NewRunCommandHandler( + []string{setup.RShellPathAllowAll}, + []string{setup.RShellCommandAllowAllWildcard}, + ) + + out, err := handler.Run(context.Background(), + makeTaskWithPatterns( + "ip route show", + []string{"rshell:ip"}, // ip allowed by name… + nil, + [][]string{{"ip", "route"}}, // …but ip route is denied + map[string][]string{ + setup.RShellPathAllowMapDefaultKey: {"/tmp"}, + }, + ), nil) + + require.NoError(t, err) + result := out.(*RunCommandOutputs) + assert.Equal(t, 127, result.ExitCode) + assert.Contains(t, result.Stderr, "blocked by deny pattern") +} + +// TestDeniedCommandPatternsDoNotBlockSiblings confirms the deny is +// targeted, not blanket. With the same allow rule and same deny +// pattern, ip addr (a sibling subcommand) still admits. +func TestDeniedCommandPatternsDoNotBlockSiblings(t *testing.T) { + handler := NewRunCommandHandler( + []string{setup.RShellPathAllowAll}, + []string{setup.RShellCommandAllowAllWildcard}, + ) + + out, err := handler.Run(context.Background(), + makeTaskWithPatterns( + "ip addr show", + []string{"rshell:ip"}, + nil, + [][]string{{"ip", "route"}}, + map[string][]string{ + setup.RShellPathAllowMapDefaultKey: {"/tmp"}, + }, + ), nil) + + require.NoError(t, err) + result := out.(*RunCommandOutputs) + assert.NotEqual(t, 127, result.ExitCode) +} + +// TestPatternsAbsentFromTaskInputDoNotBreakRunner is the "backend +// didn't send the field" case. RunCommandInputs has both pattern +// fields tagged omitempty; missing JSON fields unmarshal to nil +// slices, which the agent passes through as zero-length, which +// rshell treats as no-patterns-configured. +func TestPatternsAbsentFromTaskInputDoNotBreakRunner(t *testing.T) { + handler := NewRunCommandHandler( + []string{setup.RShellPathAllowAll}, + []string{setup.RShellCommandAllowAllWildcard}, + ) + + out, err := handler.Run(context.Background(), + makeTaskWithPatterns( + "echo hello", + []string{"rshell:echo"}, + nil, // explicit nil — no patterns from backend + nil, // explicit nil — no denies from backend + map[string][]string{ + setup.RShellPathAllowMapDefaultKey: {"/tmp"}, + }, + ), nil) + + require.NoError(t, err) + result := out.(*RunCommandOutputs) + assert.Equal(t, 0, result.ExitCode) + assert.Equal(t, "hello\n", result.Stdout) +}