feat: add system service allowlist policy#552
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Introduces a default-deny “system service allowlist” policy to support future system-service builtins by adding a new interpreter option/API, a CLI flag for configuring grants, and plumbing for builtins to atomically authorize service operations.
Changes:
- Add
interp.AllowedSystemServicesand enforcement helper (Runner.authorizeSystemServices) with validation + warnings. - Expose service authorization through
builtins.CallContextand wire it ininterp/runner_exec.go. - Add
--allowed-servicesflag parsing plus unit tests and documentation updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
SHELL_FEATURES.md |
Documents the new AllowedSystemServices policy and its semantics. |
README.md |
Adds system services to the default-deny security model and describes the new policy/CLI syntax. |
interp/system_services.go |
Implements service grant storage, validation, warnings, and runtime authorization. |
interp/system_services_test.go |
Adds tests for exact-match authorization, default-deny, remediation gating, and warning behavior. |
interp/runner_exec.go |
Plumbs AuthorizeSystemServices into builtin call contexts. |
interp/api.go |
Adds runner config storage for allowed system services and generalizes warning buffering semantics. |
cmd/rshell/main.go |
Adds --allowed-services flag and parsing to interpreter options. |
cmd/rshell/main_test.go |
Adds CLI regression tests for --allowed-services parsing, errors, and warnings. |
builtins/builtins.go |
Introduces SystemServiceAction and adds AuthorizeSystemServices to CallContext. |
analysis/symbols_interp.go |
Allowlists unicode.IsControl / unicode.IsSpace symbols used by validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
read,reload, andrestartactions.interp.AllowedSystemServicesAPI and--allowed-servicesCLI flag.builtins.CallContext.RemediationOnlycentrally before builtin argument parsing.Motivation
Future
systemctlandjournalctlbuiltins need granular authorization so operators can restrict which services rshell may inspect or control. Command authorization alone is too broad because enabling either command could otherwise expose every system service.Behavior
Service grants use exact, case-sensitive matching:
mysqldoes not matchmysql.service.--allow-all-commandsdoes not bypass the service policy.Library configuration:
Future builtins can enforce the policy with:
This PR adds the authorization foundation only.
systemctlandjournalctlare not implemented yet.Remediation Enforcement
builtins.Command.RemediationOnlyis now enforced centrally by command registration before flags or help are parsed. Remediation mode is also propagated through nested builtin execution paths such asfind -execandxargs.Testing
make fmtgo test -p 1 ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120sgit diff --checkgofmt -l .