From e48dee50314a82040e3b3f2217ae1c738301f2ac Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Mon, 13 Jul 2026 11:30:03 -0400 Subject: [PATCH 1/6] feat: add system service allowlist policy --- README.md | 20 +- SHELL_FEATURES.md | 3 +- builtins/builtins.go | 26 ++- builtins/remediation_test.go | 54 ++++++ cmd/rshell/main.go | 38 ++++ cmd/rshell/main_test.go | 52 ++++++ interp/api.go | 5 + interp/builtin_truncate_pentest_test.go | 3 +- interp/runner_exec.go | 5 +- interp/system_services.go | 123 ++++++++++++ interp/system_services_test.go | 176 ++++++++++++++++++ .../cmd/logrotate/errors/readonly_mode.yaml | 2 +- .../cmd/truncate/errors/readonly_mode.yaml | 4 +- .../truncate/errors/readonly_mode_help.yaml | 4 +- 14 files changed, 504 insertions(+), 11 deletions(-) create mode 100644 builtins/remediation_test.go create mode 100644 interp/system_services.go create mode 100644 interp/system_services_test.go diff --git a/README.md b/README.md index f2f784b7..bcef3f5b 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,7 @@ Every access path is default-deny: | Resource | Default | Opt-in | |----------------------|-------------------------------------|----------------------------------------------| | Command execution | All commands blocked (exit code 127)| `AllowedCommands` with namespaced command list (e.g. `rshell:cat`) | +| System services | All services and actions blocked | `AllowedSystemServices` with exact service/action grants | | External commands | Blocked (exit code 127) | Provide an `ExecHandler` | | Filesystem access | Blocked | Configure `AllowedPaths` with `PATH[:ro|:rw]` root specs | | Environment variables| Empty (no host env inherited) | Pass variables via the `Env` option | @@ -68,6 +69,23 @@ Every access path is default-deny: **AllowedCommands** restricts which commands (builtins or external) the interpreter may execute. Commands must be specified with the `rshell:` namespace prefix (e.g. `rshell:cat`, `rshell:echo`). If not set, no commands are allowed. +**AllowedSystemServices** configures exact service/action grants for future system-service builtins. Service names are matched exactly without adding suffixes, resolving aliases, changing case, or otherwise normalizing them: `mysql` and `mysql.service` are different grants. The supported actions are `read`, `reload`, and `restart`. Empty names, whitespace, path-like names, and glob patterns are rejected. The policy defaults to denying every service, remains enforced when all commands are allowed, and requires remediation mode before any grant can be authorized. + +```go +interp.AllowedSystemServices([]interp.SystemServiceControlGrant{ + { + Service: "mysql.service", + Actions: []interp.SystemServiceAction{ + interp.SystemServiceRestart, + interp.SystemServiceReload, + interp.SystemServiceRead, + }, + }, +}) +``` + +The development CLI accepts the equivalent syntax through `--allowed-services mysql.service:restart+reload+read,nginx.service:read`. The policy and authorization capability are implemented, but `systemctl` and `journalctl` builtins are not yet available. + **AllowedPaths** restricts all file operations to specified directories using Go's `os.Root` API for reads and openat-based write handling for writes. - **Sandbox mechanism:** Reads go through `os.Root`; writes are checked against the most-specific path mode and, on Unix, opened with a no-symlink `openat` walk. Files outside the allowlist cannot be opened, created, truncated, or appended to. @@ -83,7 +101,7 @@ Every access path is default-deny: **ProcPath** (Linux-only) overrides the proc filesystem root used by the `ps` builtin (default `/proc`). This is a privileged option set at runner construction time by trusted caller code — scripts cannot influence it. Access to the proc path is intentionally not subject to `AllowedPaths` restrictions. To avoid leaking secrets passed as CLI arguments, `ps` does not read `/proc//cmdline`; the `CMD` column reports only the process comm/executable name. -**RemediationMode** opts the runner into host-remediation mode, enabling file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` entries in the configured `AllowedPaths` and enabling remediation-only builtins such as `truncate` and `logrotate`. Targets outside the allowlist or inside read-only roots are rejected with `permission denied` (exit 1); symlinked write targets are rejected with `symlinks are not supported as write targets`; `/dev/null` is always accepted. `<>` (read-write open) remains blocked in all modes. CLI flag: `--mode remediation` (default: `--mode read-only`). The `logrotate` builtin is an rshell-safe log truncation helper, not a full `logrotate(8)` replacement. +**RemediationMode** opts the runner into host-remediation mode, enabling file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` entries in the configured `AllowedPaths` and enabling remediation-only builtins such as `truncate` and `logrotate`. Remediation-only commands are rejected by builtin dispatch before argument parsing when this mode is disabled. Targets outside the allowlist or inside read-only roots are rejected with `permission denied` (exit 1); symlinked write targets are rejected with `symlinks are not supported as write targets`; `/dev/null` is always accepted. `<>` (read-write open) remains blocked in all modes. CLI flag: `--mode remediation` (default: `--mode read-only`). The `logrotate` builtin is an rshell-safe log truncation helper, not a full `logrotate(8)` replacement. ## Shell Features diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index c8cb7b2e..d8617490 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -121,10 +121,11 @@ The in-shell `help` command mirrors these feature categories: run `help` for a c ## Execution - ✅ AllowedCommands — restricts which commands (builtins or external) may be executed; commands require the `rshell:` namespace prefix (e.g. `rshell:cat`); if not set, no commands are allowed +- ✅ AllowedSystemServices policy — configures exact, case-sensitive `read`, `reload`, and `restart` grants for system service names through `interp.AllowedSystemServices` or CLI `--allowed-services SERVICE:ACTION[+ACTION...]`; names are not normalized (`mysql` does not match `mysql.service`), glob and path-like names are rejected, all services are denied by default, and allowing all commands does not bypass this policy; the authorization capability is implemented for future consumers, but `systemctl` and `journalctl` builtins are not yet available - ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories. Entries may end with `:ro` or `:rw` to indicate read-only and read-write permissions, respectively; entries without a suffix default to read-only. In remediation mode, write operations are accepted only inside the most-specific matching `:rw` root. Cross-root symlink fallback is read-only to avoid TOCTOU on writes; on Unix, symlink components in write targets are rejected with `symlinks are not supported as write targets` via a no-follow `openat` walk - ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the entire script, not each individual command - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments); `ps` does not read `/proc//cmdline` -- ✅ RemediationMode — opt-in mode (`interp.WithMode(interp.ModeRemediation)` / `--mode remediation`) that enables file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` `AllowedPaths` and remediation-only builtins such as `truncate` and `logrotate`; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1); symlinked write targets fail with `symlinks are not supported as write targets`; `/dev/null` always accepted; `<>` remains blocked +- ✅ RemediationMode — opt-in mode (`interp.WithMode(interp.ModeRemediation)` / `--mode remediation`) that enables file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` `AllowedPaths` and remediation-only builtins such as `truncate` and `logrotate`; builtin dispatch rejects remediation-only commands before argument parsing when the mode is disabled; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1); symlinked write targets fail with `symlinks are not supported as write targets`; `/dev/null` always accepted; `<>` remains blocked - ❌ External commands — blocked by default; requires an ExecHandler to be configured and the binary to be within AllowedPaths - ❌ Background execution: `cmd &` - ❌ Coprocesses: `coproc` diff --git a/builtins/builtins.go b/builtins/builtins.go index c7d1949f..08acbfd5 100644 --- a/builtins/builtins.go +++ b/builtins/builtins.go @@ -57,6 +57,16 @@ type AllowedPath struct { Access AllowedPathAccess } +// SystemServiceAction identifies an operation that a builtin may perform on +// an explicitly configured system service. +type SystemServiceAction string + +const ( + SystemServiceRead SystemServiceAction = "read" + SystemServiceReload SystemServiceAction = "reload" + SystemServiceRestart SystemServiceAction = "restart" +) + // Command pairs a builtin name with its flag-declaring factory. MakeFlags // registers any flags on the provided FlagSet and returns the bound handler. // Commands that accept no flags may ignore fs via NoFlags. @@ -72,8 +82,8 @@ type Command struct { NormalizeArgs func(args []string) []string // RemediationOnly marks a builtin as only available in remediation mode. - // The help builtin uses this to move the command to the disabled list - // when the shell is in read-only mode. + // Register rejects the command before argument parsing in read-only mode; + // the help builtin also uses this to move the command to the disabled list. RemediationOnly bool } @@ -95,6 +105,7 @@ func (c Command) Register() { name := c.Name factory := c.MakeFlags normalize := c.NormalizeArgs + remediationOnly := c.RemediationOnly if _, exists := featureByName[name]; exists { panic("builtin name conflicts with rshell feature: " + name) } @@ -108,6 +119,11 @@ func (c Command) Register() { metaRegistry[name] = CommandMeta{Name: name, Description: c.Description, Help: c.Help, HasFlags: hasFlags, RemediationOnly: c.RemediationOnly} addToRegistry(name, func(ctx context.Context, callCtx *CallContext, args []string) Result { + if remediationOnly && !callCtx.RemediationMode { + callCtx.Errf("%s: command requires remediation mode\n", name) + return Result{Code: 1} + } + fs := pflag.NewFlagSet(name, pflag.ContinueOnError) fs.SetOutput(io.Discard) // handler formats errors itself handler := factory(fs) @@ -245,6 +261,12 @@ type CallContext struct { // commands. CommandAllowed func(name string) bool + // AuthorizeSystemServices reports whether all named services may be used + // for action under the current shell policy. Implementations must authorize + // the complete list before a builtin performs any operation so multi-service + // requests cannot partially execute. Service names are matched exactly. + AuthorizeSystemServices func(action SystemServiceAction, services ...string) error + // AllowedPathsList returns the resolved absolute paths and configured // access modes of the AllowedPaths sandbox roots. An empty/nil slice means // no allowed paths are configured, which blocks all filesystem access. diff --git a/builtins/remediation_test.go b/builtins/remediation_test.go new file mode 100644 index 00000000..60431f35 --- /dev/null +++ b/builtins/remediation_test.go @@ -0,0 +1,54 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package builtins + +import ( + "bytes" + "context" + "testing" +) + +func TestCommandRegisterEnforcesRemediationOnly(t *testing.T) { + const name = "test-remediation-only" + defer delete(registry, name) + defer delete(metaRegistry, name) + + called := false + Command{ + Name: name, + Description: "test remediation enforcement", + RemediationOnly: true, + MakeFlags: NoFlags(func(context.Context, *CallContext, []string) Result { + called = true + return Result{} + }), + }.Register() + + handler, ok := Lookup(name) + if !ok { + t.Fatal("registered command was not found") + } + + var stderr bytes.Buffer + result := handler(context.Background(), &CallContext{Stderr: &stderr}, nil) + if result.Code != 1 { + t.Fatalf("read-only result code = %d, want 1", result.Code) + } + if called { + t.Fatal("remediation-only handler ran in read-only mode") + } + if got, want := stderr.String(), name+": command requires remediation mode\n"; got != want { + t.Fatalf("stderr = %q, want %q", got, want) + } + + result = handler(context.Background(), &CallContext{Stderr: &stderr, RemediationMode: true}, nil) + if result.Code != 0 { + t.Fatalf("remediation result code = %d, want 0", result.Code) + } + if !called { + t.Fatal("remediation-only handler did not run in remediation mode") + } +} diff --git a/cmd/rshell/main.go b/cmd/rshell/main.go index 32cfd40b..68d227c8 100644 --- a/cmd/rshell/main.go +++ b/cmd/rshell/main.go @@ -39,6 +39,7 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. command string allowedPaths string allowedCommands string + allowedServices string allowAllCmds bool timeout time.Duration procPath string @@ -84,6 +85,11 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. cmds = strings.Split(allowedCommands, ",") } + serviceGrants, err := parseAllowedServices(allowedServices) + if err != nil { + return err + } + parsedMode := interp.Mode(mode) if parsedMode != interp.ModeReadOnly && parsedMode != interp.ModeRemediation { return fmt.Errorf("--mode must be one of: read-only, remediation") @@ -92,6 +98,7 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. execOpts := executeOpts{ allowedPaths: paths, allowedCommands: cmds, + allowedServices: serviceGrants, allowAllCommands: allowAllCmds, procPath: procPath, mode: parsedMode, @@ -144,6 +151,7 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. cmd.Flags().MarkHidden("command") //nolint:errcheck // flag is guaranteed to exist cmd.Flags().StringVarP(&allowedPaths, "allowed-paths", "p", "", "comma-separated list of PATH[:ro|:rw] directories the shell is allowed to access; entries without a suffix are read-only") cmd.Flags().StringVar(&allowedCommands, "allowed-commands", "", "comma-separated list of namespaced commands (e.g. rshell:cat,rshell:find)") + cmd.Flags().StringVar(&allowedServices, "allowed-services", "", "comma-separated exact service grants in SERVICE:ACTION[+ACTION...] form; actions: read, reload, restart") cmd.Flags().BoolVar(&allowAllCmds, "allow-all-commands", false, "allow execution of all commands (builtins and external)") cmd.Flags().DurationVar(&timeout, "timeout", 0, "maximum execution time for the entire shell run (e.g. 100ms, 5s, 1m)") cmd.Flags().StringVar(&procPath, "proc-path", "", "path to the proc filesystem used by ps (default \"/proc\")") @@ -215,6 +223,7 @@ func rejectLongCommand(rawArgs []string) error { type executeOpts struct { allowedPaths []string allowedCommands []string + allowedServices []interp.SystemServiceControlGrant allowAllCommands bool procPath string mode interp.Mode @@ -241,6 +250,9 @@ func execute(ctx context.Context, script, name string, opts executeOpts, stdin i } else if len(opts.allowedCommands) > 0 { runOpts = append(runOpts, interp.AllowedCommands(opts.allowedCommands)) } + if len(opts.allowedServices) > 0 { + runOpts = append(runOpts, interp.AllowedSystemServices(opts.allowedServices)) + } if opts.procPath != "" { runOpts = append(runOpts, interp.ProcPath(opts.procPath)) } @@ -256,3 +268,29 @@ func execute(ctx context.Context, script, name string, opts executeOpts, stdin i return runner.Run(ctx, prog) } + +func parseAllowedServices(value string) ([]interp.SystemServiceControlGrant, error) { + if value == "" { + return nil, nil + } + + entries := strings.Split(value, ",") + grants := make([]interp.SystemServiceControlGrant, 0, len(entries)) + for _, entry := range entries { + separator := strings.LastIndexByte(entry, ':') + if separator <= 0 || separator == len(entry)-1 { + return nil, fmt.Errorf("--allowed-services: invalid grant %q (expected SERVICE:ACTION[+ACTION...])", entry) + } + + actionNames := strings.Split(entry[separator+1:], "+") + actions := make([]interp.SystemServiceAction, len(actionNames)) + for i, action := range actionNames { + actions[i] = interp.SystemServiceAction(action) + } + grants = append(grants, interp.SystemServiceControlGrant{ + Service: entry[:separator], + Actions: actions, + }) + } + return grants, nil +} diff --git a/cmd/rshell/main_test.go b/cmd/rshell/main_test.go index df8315dd..713fb24f 100644 --- a/cmd/rshell/main_test.go +++ b/cmd/rshell/main_test.go @@ -179,6 +179,8 @@ func TestHelp(t *testing.T) { assert.Contains(t, stdout, "PATH[:ro|:rw]") assert.Contains(t, stdout, "entries without a suffix are read-only") assert.Contains(t, stdout, "--allowed-commands") + assert.Contains(t, stdout, "--allowed-services") + assert.Contains(t, stdout, "SERVICE:ACTION[+ACTION...]") assert.Contains(t, stdout, "--allow-all-commands") assert.Contains(t, stdout, "file-target output redirections within :rw AllowedPaths roots") assert.Contains(t, stdout, "--timeout") @@ -284,6 +286,56 @@ func TestAllowedCommandsUnknownNamespace(t *testing.T) { assert.Contains(t, stderr, "unknown namespace") } +func TestAllowedServicesFlag(t *testing.T) { + code, stdout, stderr := runCLI(t, + "--allow-all-commands", + "--allowed-services", "mysql.service:restart+reload+read,nginx.service:read", + "--mode", "remediation", + "-c", `echo hello`, + ) + assert.Equal(t, 0, code) + assert.Equal(t, "hello\n", stdout) + assert.Empty(t, stderr) +} + +func TestAllowedServicesFlagRejectsInvalidGrant(t *testing.T) { + code, _, stderr := runCLI(t, + "--allow-all-commands", + "--allowed-services", "mysql.service", + "-c", `echo hello`, + ) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "invalid grant") +} + +func TestAllowedServicesFlagRejectsUnknownAction(t *testing.T) { + code, _, stderr := runCLI(t, + "--allow-all-commands", + "--allowed-services", "mysql.service:stop", + "-c", `echo hello`, + ) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, `unsupported action "stop"`) +} + +func TestAllowedServicesFlagRejectsGlob(t *testing.T) { + code, _, stderr := runCLI(t, + "--allow-all-commands", + "--allowed-services", "mysql*.service:read", + "-c", `echo hello`, + ) + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "glob pattern") +} + +func TestParseAllowedServicesUsesLastColon(t *testing.T) { + grants, err := parseAllowedServices("tenant:mysql.service:read+reload") + require.NoError(t, err) + require.Len(t, grants, 1) + assert.Equal(t, "tenant:mysql.service", grants[0].Service) + assert.Equal(t, []interp.SystemServiceAction{interp.SystemServiceRead, interp.SystemServiceReload}, grants[0].Actions) +} + func TestAllowAllCommandsFlag(t *testing.T) { code, stdout, _ := runCLI(t, "--allow-all-commands", "-c", `echo hello`) assert.Equal(t, 0, code) diff --git a/interp/api.go b/interp/api.go index 724b7802..7ecf07e1 100644 --- a/interp/api.go +++ b/interp/api.go @@ -82,6 +82,11 @@ type runnerConfig struct { // command. Intended for testing convenience. allowAllCommands bool + // allowedSystemServices maps exact service names to their permitted + // actions. It is independent of allowAllCommands and defaults to denying + // every service. + allowedSystemServices systemServiceGrants + // maxExecutionTime bounds the duration of each Run call. Zero disables // the limit. When non-zero, Run derives a child context with this timeout. maxExecutionTime time.Duration diff --git a/interp/builtin_truncate_pentest_test.go b/interp/builtin_truncate_pentest_test.go index e601cdda..4f277836 100644 --- a/interp/builtin_truncate_pentest_test.go +++ b/interp/builtin_truncate_pentest_test.go @@ -45,7 +45,8 @@ func TestTruncatePentestSandboxReadOnly(t *testing.T) { _, stderr, code := runScript(t, "truncate -s 0 f.txt", dir, interp.AllowedPaths([]string{dir}), // Deliberately omit interp.WithMode(interp.ModeRemediation) — - // this means callCtx.Truncate is nil, which is the mode-guard path. + // central remediation-only dispatch must reject the command before its + // handler can reach the nil Truncate capability. ) assert.Equal(t, 1, code) assert.Contains(t, stderr, "truncate") diff --git a/interp/runner_exec.go b/interp/runner_exec.go index 07c5e206..d49d52d4 100644 --- a/interp/runner_exec.go +++ b/interp/runner_exec.go @@ -662,6 +662,7 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string) { CommandAllowed: func(n string) bool { return r.allowAllCommands || r.allowedCommands[n] }, + AuthorizeSystemServices: r.authorizeSystemServices, AllowedPathsList: func() []builtins.AllowedPath { return allowedPathsList(r.sandbox) }, @@ -696,7 +697,8 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string) { // available", which is the closest analogue to bash's // "exec read fails because read is a builtin, not an // executable on PATH" behaviour. - Proc: r.proc, + Proc: r.proc, + RemediationMode: r.remediationMode, } if r.remediationMode && r.sandbox != nil { child.Truncate = func(ctx context.Context, path string, size int64, create bool) error { @@ -789,6 +791,7 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string) { CommandAllowed: func(cmdName string) bool { return r.allowAllCommands || r.allowedCommands[cmdName] }, + AuthorizeSystemServices: r.authorizeSystemServices, AllowedPathsList: func() []builtins.AllowedPath { return allowedPathsList(r.sandbox) }, diff --git a/interp/system_services.go b/interp/system_services.go new file mode 100644 index 00000000..80c451bd --- /dev/null +++ b/interp/system_services.go @@ -0,0 +1,123 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package interp + +import ( + "fmt" + "strings" + + "github.com/DataDog/rshell/builtins" +) + +// SystemServiceAction identifies an operation that may be granted for a +// system service. +type SystemServiceAction = builtins.SystemServiceAction + +const ( + SystemServiceRead = builtins.SystemServiceRead + SystemServiceReload = builtins.SystemServiceReload + SystemServiceRestart = builtins.SystemServiceRestart +) + +// SystemServiceControlGrant grants Actions for one exact Service spelling. +// Service names are never normalized, expanded, or resolved as aliases. +type SystemServiceControlGrant struct { + Service string + Actions []SystemServiceAction +} + +type systemServiceGrants map[string]map[SystemServiceAction]struct{} + +const systemServiceWhitespace = " \t\n\v\f\r\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" + +// AllowedSystemServices configures the system services and actions that +// system-service builtins may use. A grant matches its Service exactly: for +// example, "mysql" and "mysql.service" are different service names. +// +// Empty service names, whitespace, control characters, path separators, and +// glob patterns are rejected. Supported actions are read, reload, and restart. +// Duplicate services and actions are accepted and combined idempotently. +// +// When not set (default), or when passed an empty slice, every system service +// is denied. This policy is not bypassed by allowing all commands. +func AllowedSystemServices(grants []SystemServiceControlGrant) RunnerOption { + return func(r *Runner) error { + allowed := make(systemServiceGrants, len(grants)) + for i, grant := range grants { + if err := validateSystemServiceName(grant.Service); err != nil { + return fmt.Errorf("AllowedSystemServices: grant %d: %w", i, err) + } + if len(grant.Actions) == 0 { + return fmt.Errorf("AllowedSystemServices: grant %d for %q has no actions", i, grant.Service) + } + + actions := allowed[grant.Service] + if actions == nil { + actions = make(map[SystemServiceAction]struct{}, len(grant.Actions)) + allowed[grant.Service] = actions + } + for _, action := range grant.Actions { + if !validSystemServiceAction(action) { + return fmt.Errorf("AllowedSystemServices: grant %d for %q has unsupported action %q", i, grant.Service, action) + } + actions[action] = struct{}{} + } + } + r.allowedSystemServices = allowed + return nil + } +} + +func validSystemServiceAction(action SystemServiceAction) bool { + switch action { + case SystemServiceRead, SystemServiceReload, SystemServiceRestart: + return true + default: + return false + } +} + +func validateSystemServiceName(service string) error { + if service == "" { + return fmt.Errorf("system service name must not be empty") + } + if strings.ContainsRune(service, '/') { + return fmt.Errorf("system service name %q must not contain a path separator", service) + } + for _, r := range service { + switch r { + case '*', '?', '[', ']': + return fmt.Errorf("system service name %q must not contain a glob pattern", service) + } + if strings.ContainsRune(systemServiceWhitespace, r) || r < ' ' || (r >= 0x7f && r <= 0x9f) { + return fmt.Errorf("system service name %q must not contain whitespace or control characters", service) + } + } + return nil +} + +func (r *Runner) authorizeSystemServices(action SystemServiceAction, services ...string) error { + if !r.remediationMode { + return fmt.Errorf("system service actions require remediation mode") + } + if !validSystemServiceAction(action) { + return fmt.Errorf("unsupported system service action %q", action) + } + if len(services) == 0 { + return fmt.Errorf("at least one system service is required") + } + + for _, service := range services { + if err := validateSystemServiceName(service); err != nil { + return err + } + actions := r.allowedSystemServices[service] + if _, ok := actions[action]; !ok { + return fmt.Errorf("system service %q is not allowed for action %q", service, action) + } + } + return nil +} diff --git a/interp/system_services_test.go b/interp/system_services_test.go new file mode 100644 index 00000000..d60a712e --- /dev/null +++ b/interp/system_services_test.go @@ -0,0 +1,176 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package interp + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAllowedSystemServicesAuthorizesExactServiceAndAction(t *testing.T) { + runner, err := New( + WithMode(ModeRemediation), + AllowedSystemServices([]SystemServiceControlGrant{ + { + Service: "mysql.service", + Actions: []SystemServiceAction{ + SystemServiceRestart, + SystemServiceReload, + SystemServiceRead, + }, + }, + { + Service: "nginx.service", + Actions: []SystemServiceAction{SystemServiceRead}, + }, + }), + ) + require.NoError(t, err) + defer runner.Close() + + require.NoError(t, runner.authorizeSystemServices(SystemServiceRestart, "mysql.service")) + require.NoError(t, runner.authorizeSystemServices(SystemServiceReload, "mysql.service")) + require.NoError(t, runner.authorizeSystemServices(SystemServiceRead, "mysql.service", "nginx.service")) + + err = runner.authorizeSystemServices(SystemServiceRestart, "nginx.service") + require.Error(t, err) + assert.Contains(t, err.Error(), `system service "nginx.service" is not allowed for action "restart"`) + + for _, service := range []string{"mysql", "MYSQL.service"} { + err = runner.authorizeSystemServices(SystemServiceRead, service) + require.Error(t, err) + assert.Contains(t, err.Error(), `system service "`+service+`" is not allowed`) + } +} + +func TestAllowedSystemServicesDefaultDenyIsIndependentOfAllowedCommands(t *testing.T) { + runner, err := New(WithMode(ModeRemediation), allowAllCommandsOpt()) + require.NoError(t, err) + defer runner.Close() + + err = runner.authorizeSystemServices(SystemServiceRead, "mysql.service") + require.Error(t, err) + assert.Contains(t, err.Error(), "not allowed") +} + +func TestAllowedSystemServicesRequiresRemediationMode(t *testing.T) { + runner, err := New(AllowedSystemServices([]SystemServiceControlGrant{ + { + Service: "mysql.service", + Actions: []SystemServiceAction{SystemServiceRead}, + }, + })) + require.NoError(t, err) + defer runner.Close() + + err = runner.authorizeSystemServices(SystemServiceRead, "mysql.service") + require.Error(t, err) + assert.Contains(t, err.Error(), "require remediation mode") +} + +func TestAllowedSystemServicesCopiesAndCombinesGrants(t *testing.T) { + actions := []SystemServiceAction{SystemServiceRead} + grants := []SystemServiceControlGrant{ + {Service: "mysql", Actions: actions}, + {Service: "mysql", Actions: []SystemServiceAction{SystemServiceRestart}}, + } + runner, err := New(WithMode(ModeRemediation), AllowedSystemServices(grants)) + require.NoError(t, err) + defer runner.Close() + + grants[0].Service = "changed" + actions[0] = SystemServiceReload + + require.NoError(t, runner.authorizeSystemServices(SystemServiceRead, "mysql")) + require.NoError(t, runner.authorizeSystemServices(SystemServiceRestart, "mysql")) +} + +func TestAllowedSystemServicesRejectsInvalidConfiguration(t *testing.T) { + tests := []struct { + name string + grant SystemServiceControlGrant + needle string + }{ + { + name: "empty service", + grant: SystemServiceControlGrant{Service: "", Actions: []SystemServiceAction{SystemServiceRead}}, + needle: "must not be empty", + }, + { + name: "whitespace", + grant: SystemServiceControlGrant{Service: "mysql service", Actions: []SystemServiceAction{SystemServiceRead}}, + needle: "whitespace or control characters", + }, + { + name: "unicode whitespace", + grant: SystemServiceControlGrant{Service: "mysql\u00a0service", Actions: []SystemServiceAction{SystemServiceRead}}, + needle: "whitespace or control characters", + }, + { + name: "path", + grant: SystemServiceControlGrant{Service: "/etc/systemd/system/mysql.service", Actions: []SystemServiceAction{SystemServiceRead}}, + needle: "path separator", + }, + { + name: "glob", + grant: SystemServiceControlGrant{Service: "mysql*.service", Actions: []SystemServiceAction{SystemServiceRead}}, + needle: "glob pattern", + }, + { + name: "no actions", + grant: SystemServiceControlGrant{Service: "mysql.service"}, + needle: "has no actions", + }, + { + name: "unknown action", + grant: SystemServiceControlGrant{Service: "mysql.service", Actions: []SystemServiceAction{"stop"}}, + needle: `unsupported action "stop"`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + runner, err := New(AllowedSystemServices([]SystemServiceControlGrant{test.grant})) + if runner != nil { + runner.Close() + } + require.Error(t, err) + assert.Contains(t, err.Error(), test.needle) + }) + } +} + +func TestAuthorizeSystemServicesRejectsInvalidRequests(t *testing.T) { + runner, err := New(WithMode(ModeRemediation), AllowedSystemServices([]SystemServiceControlGrant{ + { + Service: "mysql.service", + Actions: []SystemServiceAction{SystemServiceRead}, + }, + })) + require.NoError(t, err) + defer runner.Close() + + tests := []struct { + name string + action SystemServiceAction + services []string + needle string + }{ + {name: "unknown action", action: "stop", services: []string{"mysql.service"}, needle: "unsupported system service action"}, + {name: "no services", action: SystemServiceRead, needle: "at least one system service"}, + {name: "runtime glob", action: SystemServiceRead, services: []string{"mysql*.service"}, needle: "glob pattern"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := runner.authorizeSystemServices(test.action, test.services...) + require.Error(t, err) + assert.Contains(t, err.Error(), test.needle) + }) + } +} diff --git a/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml b/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml index 31c0ffcf..f177bcef 100644 --- a/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml +++ b/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml @@ -12,5 +12,5 @@ input: expect: stdout: |+ stderr: |+ - logrotate: filesystem capability not available (remediation mode required) + logrotate: command requires remediation mode exit_code: 1 diff --git a/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml b/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml index b85bf000..ca12439e 100644 --- a/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml +++ b/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml @@ -1,5 +1,5 @@ # Security test: truncate in read-only mode (no remediation) is rejected. -description: truncate without remediation mode fails with "capability not available". +description: truncate without remediation mode is rejected before its handler runs. setup: files: - path: f.txt @@ -11,6 +11,6 @@ input: expect: stdout: |+ stderr: |+ - truncate: filesystem capability not available (remediation mode required) + truncate: command requires remediation mode exit_code: 1 skip_assert_against_bash: true diff --git a/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml b/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml index d03a8e9b..b1dc61cb 100644 --- a/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml +++ b/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml @@ -1,4 +1,4 @@ -description: truncate --help in read-only mode fails with the same error as truncate itself. +description: truncate --help in read-only mode is rejected before help is displayed. input: allowed_paths: ["$DIR"] script: |+ @@ -6,6 +6,6 @@ input: expect: stdout: |+ stderr: |+ - truncate: filesystem capability not available (remediation mode required) + truncate: command requires remediation mode exit_code: 1 skip_assert_against_bash: true From 3a038cb45de89da34c2a2dbd6406fa593b7feb51 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 14 Jul 2026 14:13:54 -0400 Subject: [PATCH 2/6] fix: remove redundant remediation dispatch gate --- README.md | 2 +- SHELL_FEATURES.md | 2 +- builtins/builtins.go | 10 +--- builtins/remediation_test.go | 54 ------------------- interp/builtin_truncate_pentest_test.go | 3 +- interp/runner_exec.go | 3 +- .../cmd/logrotate/errors/readonly_mode.yaml | 2 +- .../cmd/truncate/errors/readonly_mode.yaml | 4 +- .../truncate/errors/readonly_mode_help.yaml | 4 +- 9 files changed, 11 insertions(+), 73 deletions(-) delete mode 100644 builtins/remediation_test.go diff --git a/README.md b/README.md index bcef3f5b..68fab48d 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ The development CLI accepts the equivalent syntax through `--allowed-services my **ProcPath** (Linux-only) overrides the proc filesystem root used by the `ps` builtin (default `/proc`). This is a privileged option set at runner construction time by trusted caller code — scripts cannot influence it. Access to the proc path is intentionally not subject to `AllowedPaths` restrictions. To avoid leaking secrets passed as CLI arguments, `ps` does not read `/proc//cmdline`; the `CMD` column reports only the process comm/executable name. -**RemediationMode** opts the runner into host-remediation mode, enabling file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` entries in the configured `AllowedPaths` and enabling remediation-only builtins such as `truncate` and `logrotate`. Remediation-only commands are rejected by builtin dispatch before argument parsing when this mode is disabled. Targets outside the allowlist or inside read-only roots are rejected with `permission denied` (exit 1); symlinked write targets are rejected with `symlinks are not supported as write targets`; `/dev/null` is always accepted. `<>` (read-write open) remains blocked in all modes. CLI flag: `--mode remediation` (default: `--mode read-only`). The `logrotate` builtin is an rshell-safe log truncation helper, not a full `logrotate(8)` replacement. +**RemediationMode** opts the runner into host-remediation mode, enabling file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` entries in the configured `AllowedPaths` and enabling remediation-only builtins such as `truncate` and `logrotate`. Targets outside the allowlist or inside read-only roots are rejected with `permission denied` (exit 1); symlinked write targets are rejected with `symlinks are not supported as write targets`; `/dev/null` is always accepted. `<>` (read-write open) remains blocked in all modes. CLI flag: `--mode remediation` (default: `--mode read-only`). The `logrotate` builtin is an rshell-safe log truncation helper, not a full `logrotate(8)` replacement. ## Shell Features diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index d8617490..c1bd29e2 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -125,7 +125,7 @@ The in-shell `help` command mirrors these feature categories: run `help` for a c - ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories. Entries may end with `:ro` or `:rw` to indicate read-only and read-write permissions, respectively; entries without a suffix default to read-only. In remediation mode, write operations are accepted only inside the most-specific matching `:rw` root. Cross-root symlink fallback is read-only to avoid TOCTOU on writes; on Unix, symlink components in write targets are rejected with `symlinks are not supported as write targets` via a no-follow `openat` walk - ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the entire script, not each individual command - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments); `ps` does not read `/proc//cmdline` -- ✅ RemediationMode — opt-in mode (`interp.WithMode(interp.ModeRemediation)` / `--mode remediation`) that enables file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` `AllowedPaths` and remediation-only builtins such as `truncate` and `logrotate`; builtin dispatch rejects remediation-only commands before argument parsing when the mode is disabled; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1); symlinked write targets fail with `symlinks are not supported as write targets`; `/dev/null` always accepted; `<>` remains blocked +- ✅ RemediationMode — opt-in mode (`interp.WithMode(interp.ModeRemediation)` / `--mode remediation`) that enables file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` `AllowedPaths` and remediation-only builtins such as `truncate` and `logrotate`; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1); symlinked write targets fail with `symlinks are not supported as write targets`; `/dev/null` always accepted; `<>` remains blocked - ❌ External commands — blocked by default; requires an ExecHandler to be configured and the binary to be within AllowedPaths - ❌ Background execution: `cmd &` - ❌ Coprocesses: `coproc` diff --git a/builtins/builtins.go b/builtins/builtins.go index 08acbfd5..2cee22b9 100644 --- a/builtins/builtins.go +++ b/builtins/builtins.go @@ -82,8 +82,8 @@ type Command struct { NormalizeArgs func(args []string) []string // RemediationOnly marks a builtin as only available in remediation mode. - // Register rejects the command before argument parsing in read-only mode; - // the help builtin also uses this to move the command to the disabled list. + // The help builtin uses this to move the command to the disabled list + // when the shell is in read-only mode. RemediationOnly bool } @@ -105,7 +105,6 @@ func (c Command) Register() { name := c.Name factory := c.MakeFlags normalize := c.NormalizeArgs - remediationOnly := c.RemediationOnly if _, exists := featureByName[name]; exists { panic("builtin name conflicts with rshell feature: " + name) } @@ -119,11 +118,6 @@ func (c Command) Register() { metaRegistry[name] = CommandMeta{Name: name, Description: c.Description, Help: c.Help, HasFlags: hasFlags, RemediationOnly: c.RemediationOnly} addToRegistry(name, func(ctx context.Context, callCtx *CallContext, args []string) Result { - if remediationOnly && !callCtx.RemediationMode { - callCtx.Errf("%s: command requires remediation mode\n", name) - return Result{Code: 1} - } - fs := pflag.NewFlagSet(name, pflag.ContinueOnError) fs.SetOutput(io.Discard) // handler formats errors itself handler := factory(fs) diff --git a/builtins/remediation_test.go b/builtins/remediation_test.go deleted file mode 100644 index 60431f35..00000000 --- a/builtins/remediation_test.go +++ /dev/null @@ -1,54 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2026-present Datadog, Inc. - -package builtins - -import ( - "bytes" - "context" - "testing" -) - -func TestCommandRegisterEnforcesRemediationOnly(t *testing.T) { - const name = "test-remediation-only" - defer delete(registry, name) - defer delete(metaRegistry, name) - - called := false - Command{ - Name: name, - Description: "test remediation enforcement", - RemediationOnly: true, - MakeFlags: NoFlags(func(context.Context, *CallContext, []string) Result { - called = true - return Result{} - }), - }.Register() - - handler, ok := Lookup(name) - if !ok { - t.Fatal("registered command was not found") - } - - var stderr bytes.Buffer - result := handler(context.Background(), &CallContext{Stderr: &stderr}, nil) - if result.Code != 1 { - t.Fatalf("read-only result code = %d, want 1", result.Code) - } - if called { - t.Fatal("remediation-only handler ran in read-only mode") - } - if got, want := stderr.String(), name+": command requires remediation mode\n"; got != want { - t.Fatalf("stderr = %q, want %q", got, want) - } - - result = handler(context.Background(), &CallContext{Stderr: &stderr, RemediationMode: true}, nil) - if result.Code != 0 { - t.Fatalf("remediation result code = %d, want 0", result.Code) - } - if !called { - t.Fatal("remediation-only handler did not run in remediation mode") - } -} diff --git a/interp/builtin_truncate_pentest_test.go b/interp/builtin_truncate_pentest_test.go index 4f277836..e601cdda 100644 --- a/interp/builtin_truncate_pentest_test.go +++ b/interp/builtin_truncate_pentest_test.go @@ -45,8 +45,7 @@ func TestTruncatePentestSandboxReadOnly(t *testing.T) { _, stderr, code := runScript(t, "truncate -s 0 f.txt", dir, interp.AllowedPaths([]string{dir}), // Deliberately omit interp.WithMode(interp.ModeRemediation) — - // central remediation-only dispatch must reject the command before its - // handler can reach the nil Truncate capability. + // this means callCtx.Truncate is nil, which is the mode-guard path. ) assert.Equal(t, 1, code) assert.Contains(t, stderr, "truncate") diff --git a/interp/runner_exec.go b/interp/runner_exec.go index d49d52d4..9e67ff64 100644 --- a/interp/runner_exec.go +++ b/interp/runner_exec.go @@ -697,8 +697,7 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string) { // available", which is the closest analogue to bash's // "exec read fails because read is a builtin, not an // executable on PATH" behaviour. - Proc: r.proc, - RemediationMode: r.remediationMode, + Proc: r.proc, } if r.remediationMode && r.sandbox != nil { child.Truncate = func(ctx context.Context, path string, size int64, create bool) error { diff --git a/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml b/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml index f177bcef..31c0ffcf 100644 --- a/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml +++ b/tests/scenarios/cmd/logrotate/errors/readonly_mode.yaml @@ -12,5 +12,5 @@ input: expect: stdout: |+ stderr: |+ - logrotate: command requires remediation mode + logrotate: filesystem capability not available (remediation mode required) exit_code: 1 diff --git a/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml b/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml index ca12439e..b85bf000 100644 --- a/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml +++ b/tests/scenarios/cmd/truncate/errors/readonly_mode.yaml @@ -1,5 +1,5 @@ # Security test: truncate in read-only mode (no remediation) is rejected. -description: truncate without remediation mode is rejected before its handler runs. +description: truncate without remediation mode fails with "capability not available". setup: files: - path: f.txt @@ -11,6 +11,6 @@ input: expect: stdout: |+ stderr: |+ - truncate: command requires remediation mode + truncate: filesystem capability not available (remediation mode required) exit_code: 1 skip_assert_against_bash: true diff --git a/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml b/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml index b1dc61cb..d03a8e9b 100644 --- a/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml +++ b/tests/scenarios/cmd/truncate/errors/readonly_mode_help.yaml @@ -1,4 +1,4 @@ -description: truncate --help in read-only mode is rejected before help is displayed. +description: truncate --help in read-only mode fails with the same error as truncate itself. input: allowed_paths: ["$DIR"] script: |+ @@ -6,6 +6,6 @@ input: expect: stdout: |+ stderr: |+ - truncate: command requires remediation mode + truncate: filesystem capability not available (remediation mode required) exit_code: 1 skip_assert_against_bash: true From c50bd9ecae7e4e4c6daab63267b4a66754b70330 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 14 Jul 2026 14:20:26 -0400 Subject: [PATCH 3/6] refactor: use unicode helpers for service validation --- interp/system_services.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/interp/system_services.go b/interp/system_services.go index 80c451bd..d89ad2c9 100644 --- a/interp/system_services.go +++ b/interp/system_services.go @@ -8,6 +8,7 @@ package interp import ( "fmt" "strings" + "unicode" "github.com/DataDog/rshell/builtins" ) @@ -31,8 +32,6 @@ type SystemServiceControlGrant struct { type systemServiceGrants map[string]map[SystemServiceAction]struct{} -const systemServiceWhitespace = " \t\n\v\f\r\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" - // AllowedSystemServices configures the system services and actions that // system-service builtins may use. A grant matches its Service exactly: for // example, "mysql" and "mysql.service" are different service names. @@ -92,7 +91,7 @@ func validateSystemServiceName(service string) error { case '*', '?', '[', ']': return fmt.Errorf("system service name %q must not contain a glob pattern", service) } - if strings.ContainsRune(systemServiceWhitespace, r) || r < ' ' || (r >= 0x7f && r <= 0x9f) { + if unicode.IsSpace(r) || unicode.IsControl(r) { return fmt.Errorf("system service name %q must not contain whitespace or control characters", service) } } From 55fb4c1c6e6a30f252a108626eb08179d03c7336 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 14 Jul 2026 14:39:03 -0400 Subject: [PATCH 4/6] fix: skip invalid system service grants --- README.md | 4 +- SHELL_FEATURES.md | 2 +- analysis/symbols_interp.go | 4 ++ cmd/rshell/main_test.go | 8 +-- interp/api.go | 54 +++++++++---------- interp/system_services.go | 18 ++++--- interp/system_services_test.go | 98 +++++++++++++++++----------------- 7 files changed, 96 insertions(+), 92 deletions(-) diff --git a/README.md b/README.md index 68fab48d..b3a6e8dd 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ Every access path is default-deny: **AllowedCommands** restricts which commands (builtins or external) the interpreter may execute. Commands must be specified with the `rshell:` namespace prefix (e.g. `rshell:cat`, `rshell:echo`). If not set, no commands are allowed. -**AllowedSystemServices** configures exact service/action grants for future system-service builtins. Service names are matched exactly without adding suffixes, resolving aliases, changing case, or otherwise normalizing them: `mysql` and `mysql.service` are different grants. The supported actions are `read`, `reload`, and `restart`. Empty names, whitespace, path-like names, and glob patterns are rejected. The policy defaults to denying every service, remains enforced when all commands are allowed, and requires remediation mode before any grant can be authorized. +**AllowedSystemServices** configures exact service/action grants for future system-service builtins. Service names are matched exactly without adding suffixes, resolving aliases, changing case, or otherwise normalizing them: `mysql` and `mysql.service` are different grants. The supported actions are `read`, `reload`, and `restart`. Grants without actions are ignored; entries with empty, whitespace-containing, path-like, or glob-pattern service names are skipped with a warning. The policy defaults to denying every service, remains enforced when all commands are allowed, and requires remediation mode before any grant can be authorized. ```go interp.AllowedSystemServices([]interp.SystemServiceControlGrant{ @@ -95,7 +95,7 @@ The development CLI accepts the equivalent syntax through `--allowed-services my - _Read-only mode (default):_ file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) are rejected at parse time (exit 2). - _Remediation mode:_ those redirections open through the same sandbox — writes inside `:rw` allowlist roots succeed; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1). - **Special targets:** The literal target `/dev/null` is always short-circuited to a discarded sink without going through the sandbox. `<>` (read-write open) is blocked in all modes. -- **Diagnostic messages:** Configured directories that cannot be opened (missing, not a directory, no permission) are skipped with a warning flushed once to the runner's stderr at construction time. Silence them or redirect them programmatically with `WarningsWriter(io.Writer)` or `Runner.Warnings()`. +- **Diagnostic messages:** Configured directories that cannot be opened and invalid system service names are skipped with a warning flushed once to the runner's stderr at construction time. Silence them or redirect them programmatically with `WarningsWriter(io.Writer)` or inspect them with `Runner.Warnings()`. > **Note:** The `ss`, `ip route`, and `df` builtins bypass `AllowedPaths` for their kernel-state reads. `ss` and `ip route` open `/proc/net/*` paths directly; `df` reads `/proc/self/mountinfo` (Linux) or calls `getfsstat(2)` (macOS), then issues `unix.Statfs(2)` against every kernel-reported mount point. These paths are hardcoded — never derived from user input — and `Statfs` returns metadata only (block / inode counts, filesystem type, block size). There is no sandbox-escape risk, but operators cannot use `AllowedPaths` to block `ss` from enumerating local sockets, `ip route` from reading the routing table, or `df` from reporting mount-table capacity — these reads succeed regardless of the configured path policy. diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index c1bd29e2..ea87e13b 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -121,7 +121,7 @@ The in-shell `help` command mirrors these feature categories: run `help` for a c ## Execution - ✅ AllowedCommands — restricts which commands (builtins or external) may be executed; commands require the `rshell:` namespace prefix (e.g. `rshell:cat`); if not set, no commands are allowed -- ✅ AllowedSystemServices policy — configures exact, case-sensitive `read`, `reload`, and `restart` grants for system service names through `interp.AllowedSystemServices` or CLI `--allowed-services SERVICE:ACTION[+ACTION...]`; names are not normalized (`mysql` does not match `mysql.service`), glob and path-like names are rejected, all services are denied by default, and allowing all commands does not bypass this policy; the authorization capability is implemented for future consumers, but `systemctl` and `journalctl` builtins are not yet available +- ✅ AllowedSystemServices policy — configures exact, case-sensitive `read`, `reload`, and `restart` grants for system service names through `interp.AllowedSystemServices` or CLI `--allowed-services SERVICE:ACTION[+ACTION...]`; names are not normalized (`mysql` does not match `mysql.service`), grants without actions are ignored, invalid names are skipped with warnings, all services are denied by default, and allowing all commands does not bypass this policy; the authorization capability is implemented for future consumers, but `systemctl` and `journalctl` builtins are not yet available - ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories. Entries may end with `:ro` or `:rw` to indicate read-only and read-write permissions, respectively; entries without a suffix default to read-only. In remediation mode, write operations are accepted only inside the most-specific matching `:rw` root. Cross-root symlink fallback is read-only to avoid TOCTOU on writes; on Unix, symlink components in write targets are rejected with `symlinks are not supported as write targets` via a no-follow `openat` walk - ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the entire script, not each individual command - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments); `ps` does not read `/proc//cmdline` diff --git a/analysis/symbols_interp.go b/analysis/symbols_interp.go index b079323d..98a460c5 100644 --- a/analysis/symbols_interp.go +++ b/analysis/symbols_interp.go @@ -80,6 +80,8 @@ var interpAllowedSymbols = []string{ "time.Duration", // 🟢 numeric duration type; pure type, no side effects. "time.Now", // 🟠 returns current time; read-only, no mutation. "time.Time", // 🟢 time value type; pure data, no side effects. + "unicode.IsControl", // 🟢 reports whether a rune is a Unicode control character; pure function, no I/O. + "unicode.IsSpace", // 🟢 reports whether a rune is a Unicode whitespace character; pure function, no I/O. // --- github.com/DataDog/datadog-agent/pkg/fleet/installer/telemetry --- (lightweight span tracer used by the Agent Installer) @@ -229,6 +231,8 @@ var interpPerModeSymbols = map[string][]string{ "time.Duration", // 🟢 numeric duration type; pure type, no side effects. "time.Now", // 🟠 returns current time; read-only, no mutation. "time.Time", // 🟢 time value type; pure data, no side effects. + "unicode.IsControl", // 🟢 reports whether a rune is a Unicode control character; pure function, no I/O. + "unicode.IsSpace", // 🟢 reports whether a rune is a Unicode whitespace character; pure function, no I/O. // --- github.com/DataDog/datadog-agent/pkg/fleet/installer/telemetry --- diff --git a/cmd/rshell/main_test.go b/cmd/rshell/main_test.go index 713fb24f..c0236b40 100644 --- a/cmd/rshell/main_test.go +++ b/cmd/rshell/main_test.go @@ -318,13 +318,15 @@ func TestAllowedServicesFlagRejectsUnknownAction(t *testing.T) { assert.Contains(t, stderr, `unsupported action "stop"`) } -func TestAllowedServicesFlagRejectsGlob(t *testing.T) { - code, _, stderr := runCLI(t, +func TestAllowedServicesFlagWarnsAndSkipsInvalidService(t *testing.T) { + code, stdout, stderr := runCLI(t, "--allow-all-commands", "--allowed-services", "mysql*.service:read", "-c", `echo hello`, ) - assert.Equal(t, 1, code) + assert.Equal(t, 0, code) + assert.Equal(t, "hello\n", stdout) + assert.Contains(t, stderr, "AllowedSystemServices: skipping") assert.Contains(t, stderr, "glob pattern") } diff --git a/interp/api.go b/interp/api.go index 7ecf07e1..5e33dd6c 100644 --- a/interp/api.go +++ b/interp/api.go @@ -54,18 +54,16 @@ type runnerConfig struct { // nil (default) blocks all file access; populate via AllowedPaths option. sandbox *allowedpaths.Sandbox - // sandboxWarnings holds diagnostic messages about skipped AllowedPaths + // configurationWarnings holds diagnostic messages about skipped allowlist // entries. Flushed to warningsWriter after all options are applied and - // defaults are set, so the output target is independent of option - // ordering. Retained on the runner after flush so callers can also - // retrieve them programmatically via [Runner.Warnings]. - sandboxWarnings []byte - - // warningsWriter is the destination for sandbox diagnostic messages - // (currently just AllowedPaths skip warnings). Defaults to r.stderr - // when unset; callers can route warnings to a separate sink (e.g. a - // dedicated buffer or logger) via [WarningsWriter] so they are not - // conflated with command stderr. + // defaults are set, so the output target is independent of option ordering. + // Retained after flush so callers can retrieve them via [Runner.Warnings]. + configurationWarnings []byte + + // warningsWriter is the destination for allowlist diagnostic messages. + // Defaults to r.stderr when unset; callers can route warnings to a separate + // sink (e.g. a dedicated buffer or logger) via [WarningsWriter] so they are + // not conflated with command stderr. warningsWriter io.Writer // hostPrefix is stored here so HostPrefix can be applied before or @@ -302,7 +300,7 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.stdout == nil || r.stderr == nil { StdIO(r.stdin, r.stdout, r.stderr)(r) } - // Default sandbox warnings to r.stderr so today's behaviour is + // Default configuration warnings to r.stderr so today's behaviour is // preserved for callers who do not opt in to a dedicated sink. if r.warningsWriter == nil { r.warningsWriter = r.stderr @@ -318,11 +316,11 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.remediationMode && r.sandbox != nil { r.sandbox.SetWritable() } - // Flush any sandbox warnings now that the warnings sink is guaranteed + // Flush any configuration warnings now that the warnings sink is guaranteed // to be set. The buffer is retained on the runner so callers can also // retrieve warnings via [Runner.Warnings]. - if len(r.sandboxWarnings) > 0 { - r.warningsWriter.Write(r.sandboxWarnings) + if len(r.configurationWarnings) > 0 { + r.warningsWriter.Write(r.configurationWarnings) } r.proc = builtins.NewProcProvider(r.procPath) return r, nil @@ -704,11 +702,11 @@ func (r *Runner) Close() error { return r.sandbox.Close() } -// WarningsWriter routes sandbox diagnostic messages (currently produced by -// [AllowedPaths] when a configured directory cannot be opened) to the given -// writer instead of the runner's stderr. +// WarningsWriter routes allowlist diagnostic messages (such as skipped +// [AllowedPaths] and [AllowedSystemServices] entries) to the given writer +// instead of the runner's stderr. // -// Sandbox diagnostics are buffered during option processing and flushed once +// Diagnostics are buffered during option processing and flushed once // during [New], after all other options have been applied. They are not // written again on subsequent runs. // @@ -729,22 +727,20 @@ func WarningsWriter(w io.Writer) RunnerOption { } } -// Warnings returns the sandbox diagnostic messages collected during runner -// construction (currently produced by [AllowedPaths] when a configured -// directory cannot be opened), one entry per warning. The slice is empty when -// no warnings were emitted. +// Warnings returns the allowlist diagnostic messages collected during runner +// construction, one entry per warning. The slice is empty when no warnings +// were emitted. // // Callers that integrate rshell as a library can use this to surface // configuration problems in their own structured output (e.g. a "warnings" // field in an API response) without parsing them out of stderr. func (r *Runner) Warnings() []string { - if len(r.sandboxWarnings) == 0 { + if len(r.configurationWarnings) == 0 { return nil } - s := string(r.sandboxWarnings) - // allowedpaths.New emits one warning per line, each terminated with - // "\n". Strip a single trailing newline before splitting so the result - // is one entry per warning rather than ending in a stray empty string. + s := string(r.configurationWarnings) + // Each warning is terminated with "\n". Strip a single trailing newline + // before splitting so the result does not end in a stray empty string. if strings.HasSuffix(s, "\n") { s = s[:len(s)-1] } @@ -774,7 +770,7 @@ func AllowedPaths(paths []string) RunnerOption { return err } r.sandbox = sb - r.sandboxWarnings = warnings + r.configurationWarnings = append(r.configurationWarnings, warnings...) return nil } } diff --git a/interp/system_services.go b/interp/system_services.go index d89ad2c9..0a3e3e7b 100644 --- a/interp/system_services.go +++ b/interp/system_services.go @@ -36,9 +36,11 @@ type systemServiceGrants map[string]map[SystemServiceAction]struct{} // system-service builtins may use. A grant matches its Service exactly: for // example, "mysql" and "mysql.service" are different service names. // -// Empty service names, whitespace, control characters, path separators, and -// glob patterns are rejected. Supported actions are read, reload, and restart. -// Duplicate services and actions are accepted and combined idempotently. +// Grants without actions are ignored. Empty service names and names containing +// whitespace, control characters, path separators, or glob patterns are +// skipped with a warning. Supported actions are read, reload, and restart; +// unsupported actions are rejected. Duplicate services and actions are +// accepted and combined idempotently. // // When not set (default), or when passed an empty slice, every system service // is denied. This policy is not bypassed by allowing all commands. @@ -46,11 +48,13 @@ func AllowedSystemServices(grants []SystemServiceControlGrant) RunnerOption { return func(r *Runner) error { allowed := make(systemServiceGrants, len(grants)) for i, grant := range grants { - if err := validateSystemServiceName(grant.Service); err != nil { - return fmt.Errorf("AllowedSystemServices: grant %d: %w", i, err) - } if len(grant.Actions) == 0 { - return fmt.Errorf("AllowedSystemServices: grant %d for %q has no actions", i, grant.Service) + continue + } + if err := validateSystemServiceName(grant.Service); err != nil { + warning := fmt.Sprintf("AllowedSystemServices: skipping grant %d: %v\n", i, err) + r.configurationWarnings = append(r.configurationWarnings, warning...) + continue } actions := allowed[grant.Service] diff --git a/interp/system_services_test.go b/interp/system_services_test.go index d60a712e..087c6177 100644 --- a/interp/system_services_test.go +++ b/interp/system_services_test.go @@ -6,6 +6,8 @@ package interp import ( + "bytes" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -90,59 +92,55 @@ func TestAllowedSystemServicesCopiesAndCombinesGrants(t *testing.T) { require.NoError(t, runner.authorizeSystemServices(SystemServiceRestart, "mysql")) } -func TestAllowedSystemServicesRejectsInvalidConfiguration(t *testing.T) { - tests := []struct { - name string - grant SystemServiceControlGrant - needle string - }{ - { - name: "empty service", - grant: SystemServiceControlGrant{Service: "", Actions: []SystemServiceAction{SystemServiceRead}}, - needle: "must not be empty", - }, - { - name: "whitespace", - grant: SystemServiceControlGrant{Service: "mysql service", Actions: []SystemServiceAction{SystemServiceRead}}, - needle: "whitespace or control characters", - }, - { - name: "unicode whitespace", - grant: SystemServiceControlGrant{Service: "mysql\u00a0service", Actions: []SystemServiceAction{SystemServiceRead}}, - needle: "whitespace or control characters", - }, - { - name: "path", - grant: SystemServiceControlGrant{Service: "/etc/systemd/system/mysql.service", Actions: []SystemServiceAction{SystemServiceRead}}, - needle: "path separator", - }, - { - name: "glob", - grant: SystemServiceControlGrant{Service: "mysql*.service", Actions: []SystemServiceAction{SystemServiceRead}}, - needle: "glob pattern", - }, - { - name: "no actions", - grant: SystemServiceControlGrant{Service: "mysql.service"}, - needle: "has no actions", - }, - { - name: "unknown action", - grant: SystemServiceControlGrant{Service: "mysql.service", Actions: []SystemServiceAction{"stop"}}, - needle: `unsupported action "stop"`, - }, +func TestAllowedSystemServicesSkipsEmptyAndInvalidGrants(t *testing.T) { + var warningOutput bytes.Buffer + missingPath := filepath.Join(t.TempDir(), "missing") + runner, err := New( + WithMode(ModeRemediation), + WarningsWriter(&warningOutput), + AllowedSystemServices([]SystemServiceControlGrant{ + {Service: "ignored.service"}, + {Service: "mysql.service", Actions: []SystemServiceAction{SystemServiceRead}}, + {Service: "", Actions: []SystemServiceAction{SystemServiceRead}}, + {Service: "mysql service", Actions: []SystemServiceAction{SystemServiceRead}}, + {Service: "mysql\u00a0service", Actions: []SystemServiceAction{SystemServiceRead}}, + {Service: "/etc/systemd/system/mysql.service", Actions: []SystemServiceAction{SystemServiceRead}}, + {Service: "mysql*.service", Actions: []SystemServiceAction{SystemServiceRead}}, + }), + // Applying AllowedPaths after AllowedSystemServices verifies that one + // allowlist option does not overwrite another option's warnings. + AllowedPaths([]string{missingPath}), + ) + require.NoError(t, err) + defer runner.Close() + + require.NoError(t, runner.authorizeSystemServices(SystemServiceRead, "mysql.service")) + assert.Len(t, runner.allowedSystemServices, 1) + assert.NotContains(t, runner.allowedSystemServices, "ignored.service") + + warnings := runner.Warnings() + require.Len(t, warnings, 6) + for _, needle := range []string{ + "AllowedSystemServices: skipping grant 2: system service name must not be empty", + "whitespace or control characters", + "path separator", + "glob pattern", + "AllowedPaths: skipping", + } { + assert.Contains(t, warningOutput.String(), needle) } + assert.NotContains(t, warningOutput.String(), "ignored.service") +} - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - runner, err := New(AllowedSystemServices([]SystemServiceControlGrant{test.grant})) - if runner != nil { - runner.Close() - } - require.Error(t, err) - assert.Contains(t, err.Error(), test.needle) - }) +func TestAllowedSystemServicesRejectsUnsupportedAction(t *testing.T) { + runner, err := New(AllowedSystemServices([]SystemServiceControlGrant{ + {Service: "mysql.service", Actions: []SystemServiceAction{"stop"}}, + })) + if runner != nil { + runner.Close() } + require.Error(t, err) + assert.Contains(t, err.Error(), `unsupported action "stop"`) } func TestAuthorizeSystemServicesRejectsInvalidRequests(t *testing.T) { From 549c99620a0d1e1f93ed59c1e79ded64d090981c Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 14 Jul 2026 14:44:09 -0400 Subject: [PATCH 5/6] refactor: reuse sandbox warning buffer --- interp/api.go | 36 +++++++++++++++++------------------- interp/system_services.go | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/interp/api.go b/interp/api.go index 5e33dd6c..5e508dad 100644 --- a/interp/api.go +++ b/interp/api.go @@ -54,13 +54,13 @@ type runnerConfig struct { // nil (default) blocks all file access; populate via AllowedPaths option. sandbox *allowedpaths.Sandbox - // configurationWarnings holds diagnostic messages about skipped allowlist - // entries. Flushed to warningsWriter after all options are applied and + // sandboxWarnings holds diagnostic messages emitted while applying runner + // options. Flushed to warningsWriter after all options are applied and // defaults are set, so the output target is independent of option ordering. // Retained after flush so callers can retrieve them via [Runner.Warnings]. - configurationWarnings []byte + sandboxWarnings []byte - // warningsWriter is the destination for allowlist diagnostic messages. + // warningsWriter is the destination for runner warning messages. // Defaults to r.stderr when unset; callers can route warnings to a separate // sink (e.g. a dedicated buffer or logger) via [WarningsWriter] so they are // not conflated with command stderr. @@ -300,7 +300,7 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.stdout == nil || r.stderr == nil { StdIO(r.stdin, r.stdout, r.stderr)(r) } - // Default configuration warnings to r.stderr so today's behaviour is + // Default warnings to r.stderr so today's behaviour is // preserved for callers who do not opt in to a dedicated sink. if r.warningsWriter == nil { r.warningsWriter = r.stderr @@ -316,11 +316,10 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.remediationMode && r.sandbox != nil { r.sandbox.SetWritable() } - // Flush any configuration warnings now that the warnings sink is guaranteed - // to be set. The buffer is retained on the runner so callers can also - // retrieve warnings via [Runner.Warnings]. - if len(r.configurationWarnings) > 0 { - r.warningsWriter.Write(r.configurationWarnings) + // Flush buffered warnings now that the warnings sink is guaranteed to be + // set. The buffer is retained so callers can retrieve it via [Runner.Warnings]. + if len(r.sandboxWarnings) > 0 { + r.warningsWriter.Write(r.sandboxWarnings) } r.proc = builtins.NewProcProvider(r.procPath) return r, nil @@ -702,9 +701,9 @@ func (r *Runner) Close() error { return r.sandbox.Close() } -// WarningsWriter routes allowlist diagnostic messages (such as skipped -// [AllowedPaths] and [AllowedSystemServices] entries) to the given writer -// instead of the runner's stderr. +// WarningsWriter routes diagnostic messages emitted while applying runner +// options (such as skipped [AllowedPaths] and [AllowedSystemServices] entries) +// to the given writer instead of the runner's stderr. // // Diagnostics are buffered during option processing and flushed once // during [New], after all other options have been applied. They are not @@ -727,18 +726,17 @@ func WarningsWriter(w io.Writer) RunnerOption { } } -// Warnings returns the allowlist diagnostic messages collected during runner -// construction, one entry per warning. The slice is empty when no warnings -// were emitted. +// Warnings returns the diagnostic messages collected during runner construction, +// one entry per warning. The slice is empty when no warnings were emitted. // // Callers that integrate rshell as a library can use this to surface // configuration problems in their own structured output (e.g. a "warnings" // field in an API response) without parsing them out of stderr. func (r *Runner) Warnings() []string { - if len(r.configurationWarnings) == 0 { + if len(r.sandboxWarnings) == 0 { return nil } - s := string(r.configurationWarnings) + s := string(r.sandboxWarnings) // Each warning is terminated with "\n". Strip a single trailing newline // before splitting so the result does not end in a stray empty string. if strings.HasSuffix(s, "\n") { @@ -770,7 +768,7 @@ func AllowedPaths(paths []string) RunnerOption { return err } r.sandbox = sb - r.configurationWarnings = append(r.configurationWarnings, warnings...) + r.sandboxWarnings = append(r.sandboxWarnings, warnings...) return nil } } diff --git a/interp/system_services.go b/interp/system_services.go index 0a3e3e7b..60e6d56b 100644 --- a/interp/system_services.go +++ b/interp/system_services.go @@ -53,7 +53,7 @@ func AllowedSystemServices(grants []SystemServiceControlGrant) RunnerOption { } if err := validateSystemServiceName(grant.Service); err != nil { warning := fmt.Sprintf("AllowedSystemServices: skipping grant %d: %v\n", i, err) - r.configurationWarnings = append(r.configurationWarnings, warning...) + r.sandboxWarnings = append(r.sandboxWarnings, warning...) continue } From 13c9cdaecd09e580df363a2e7a991a42e7e49a36 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Tue, 14 Jul 2026 14:51:13 -0400 Subject: [PATCH 6/6] fix: warn on unsupported service actions --- README.md | 2 +- SHELL_FEATURES.md | 2 +- cmd/rshell/main_test.go | 9 +++++---- interp/system_services.go | 16 +++++++++------- interp/system_services_test.go | 30 +++++++++++++++++++++--------- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index b3a6e8dd..0a2c197b 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ Every access path is default-deny: **AllowedCommands** restricts which commands (builtins or external) the interpreter may execute. Commands must be specified with the `rshell:` namespace prefix (e.g. `rshell:cat`, `rshell:echo`). If not set, no commands are allowed. -**AllowedSystemServices** configures exact service/action grants for future system-service builtins. Service names are matched exactly without adding suffixes, resolving aliases, changing case, or otherwise normalizing them: `mysql` and `mysql.service` are different grants. The supported actions are `read`, `reload`, and `restart`. Grants without actions are ignored; entries with empty, whitespace-containing, path-like, or glob-pattern service names are skipped with a warning. The policy defaults to denying every service, remains enforced when all commands are allowed, and requires remediation mode before any grant can be authorized. +**AllowedSystemServices** configures exact service/action grants for future system-service builtins. Service names are matched exactly without adding suffixes, resolving aliases, changing case, or otherwise normalizing them: `mysql` and `mysql.service` are different grants. The supported actions are `read`, `reload`, and `restart`. Grants without actions are ignored; entries with empty, whitespace-containing, path-like, or glob-pattern service names and unsupported actions are skipped with a warning. The policy defaults to denying every service, remains enforced when all commands are allowed, and requires remediation mode before any grant can be authorized. ```go interp.AllowedSystemServices([]interp.SystemServiceControlGrant{ diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index ea87e13b..3833ae17 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -121,7 +121,7 @@ The in-shell `help` command mirrors these feature categories: run `help` for a c ## Execution - ✅ AllowedCommands — restricts which commands (builtins or external) may be executed; commands require the `rshell:` namespace prefix (e.g. `rshell:cat`); if not set, no commands are allowed -- ✅ AllowedSystemServices policy — configures exact, case-sensitive `read`, `reload`, and `restart` grants for system service names through `interp.AllowedSystemServices` or CLI `--allowed-services SERVICE:ACTION[+ACTION...]`; names are not normalized (`mysql` does not match `mysql.service`), grants without actions are ignored, invalid names are skipped with warnings, all services are denied by default, and allowing all commands does not bypass this policy; the authorization capability is implemented for future consumers, but `systemctl` and `journalctl` builtins are not yet available +- ✅ AllowedSystemServices policy — configures exact, case-sensitive `read`, `reload`, and `restart` grants for system service names through `interp.AllowedSystemServices` or CLI `--allowed-services SERVICE:ACTION[+ACTION...]`; names are not normalized (`mysql` does not match `mysql.service`), grants without actions are ignored, invalid names and unsupported actions are skipped with warnings, all services are denied by default, and allowing all commands does not bypass this policy; the authorization capability is implemented for future consumers, but `systemctl` and `journalctl` builtins are not yet available - ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories. Entries may end with `:ro` or `:rw` to indicate read-only and read-write permissions, respectively; entries without a suffix default to read-only. In remediation mode, write operations are accepted only inside the most-specific matching `:rw` root. Cross-root symlink fallback is read-only to avoid TOCTOU on writes; on Unix, symlink components in write targets are rejected with `symlinks are not supported as write targets` via a no-follow `openat` walk - ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the entire script, not each individual command - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments); `ps` does not read `/proc//cmdline` diff --git a/cmd/rshell/main_test.go b/cmd/rshell/main_test.go index c0236b40..b634095c 100644 --- a/cmd/rshell/main_test.go +++ b/cmd/rshell/main_test.go @@ -308,14 +308,15 @@ func TestAllowedServicesFlagRejectsInvalidGrant(t *testing.T) { assert.Contains(t, stderr, "invalid grant") } -func TestAllowedServicesFlagRejectsUnknownAction(t *testing.T) { - code, _, stderr := runCLI(t, +func TestAllowedServicesFlagWarnsAndSkipsUnknownAction(t *testing.T) { + code, stdout, stderr := runCLI(t, "--allow-all-commands", "--allowed-services", "mysql.service:stop", "-c", `echo hello`, ) - assert.Equal(t, 1, code) - assert.Contains(t, stderr, `unsupported action "stop"`) + assert.Equal(t, 0, code) + assert.Equal(t, "hello\n", stdout) + assert.Contains(t, stderr, `skipping unsupported action "stop"`) } func TestAllowedServicesFlagWarnsAndSkipsInvalidService(t *testing.T) { diff --git a/interp/system_services.go b/interp/system_services.go index 60e6d56b..4746d888 100644 --- a/interp/system_services.go +++ b/interp/system_services.go @@ -39,8 +39,8 @@ type systemServiceGrants map[string]map[SystemServiceAction]struct{} // Grants without actions are ignored. Empty service names and names containing // whitespace, control characters, path separators, or glob patterns are // skipped with a warning. Supported actions are read, reload, and restart; -// unsupported actions are rejected. Duplicate services and actions are -// accepted and combined idempotently. +// unsupported actions are skipped with a warning. Duplicate services and +// actions are accepted and combined idempotently. // // When not set (default), or when passed an empty slice, every system service // is denied. This policy is not bypassed by allowing all commands. @@ -58,13 +58,15 @@ func AllowedSystemServices(grants []SystemServiceControlGrant) RunnerOption { } actions := allowed[grant.Service] - if actions == nil { - actions = make(map[SystemServiceAction]struct{}, len(grant.Actions)) - allowed[grant.Service] = actions - } for _, action := range grant.Actions { if !validSystemServiceAction(action) { - return fmt.Errorf("AllowedSystemServices: grant %d for %q has unsupported action %q", i, grant.Service, action) + warning := fmt.Sprintf("AllowedSystemServices: skipping unsupported action %q in grant %d for %q\n", action, i, grant.Service) + r.sandboxWarnings = append(r.sandboxWarnings, warning...) + continue + } + if actions == nil { + actions = make(map[SystemServiceAction]struct{}, len(grant.Actions)) + allowed[grant.Service] = actions } actions[action] = struct{}{} } diff --git a/interp/system_services_test.go b/interp/system_services_test.go index 087c6177..cdc915f9 100644 --- a/interp/system_services_test.go +++ b/interp/system_services_test.go @@ -132,15 +132,27 @@ func TestAllowedSystemServicesSkipsEmptyAndInvalidGrants(t *testing.T) { assert.NotContains(t, warningOutput.String(), "ignored.service") } -func TestAllowedSystemServicesRejectsUnsupportedAction(t *testing.T) { - runner, err := New(AllowedSystemServices([]SystemServiceControlGrant{ - {Service: "mysql.service", Actions: []SystemServiceAction{"stop"}}, - })) - if runner != nil { - runner.Close() - } - require.Error(t, err) - assert.Contains(t, err.Error(), `unsupported action "stop"`) +func TestAllowedSystemServicesSkipsUnsupportedActions(t *testing.T) { + var warningOutput bytes.Buffer + runner, err := New( + WithMode(ModeRemediation), + WarningsWriter(&warningOutput), + AllowedSystemServices([]SystemServiceControlGrant{ + {Service: "mysql.service", Actions: []SystemServiceAction{SystemServiceRead, "stop", SystemServiceReload}}, + {Service: "ignored.service", Actions: []SystemServiceAction{"enable"}}, + }), + ) + require.NoError(t, err) + defer runner.Close() + + require.NoError(t, runner.authorizeSystemServices(SystemServiceRead, "mysql.service")) + require.NoError(t, runner.authorizeSystemServices(SystemServiceReload, "mysql.service")) + assert.NotContains(t, runner.allowedSystemServices, "ignored.service") + + warnings := runner.Warnings() + require.Len(t, warnings, 2) + assert.Contains(t, warningOutput.String(), `AllowedSystemServices: skipping unsupported action "stop" in grant 0 for "mysql.service"`) + assert.Contains(t, warningOutput.String(), `AllowedSystemServices: skipping unsupported action "enable" in grant 1 for "ignored.service"`) } func TestAuthorizeSystemServicesRejectsInvalidRequests(t *testing.T) {