diff --git a/docs/plans/2026-02-13-secrets-protection-design.md b/docs/plans/2026-02-13-secrets-protection-design.md new file mode 100644 index 0000000..9cd9628 --- /dev/null +++ b/docs/plans/2026-02-13-secrets-protection-design.md @@ -0,0 +1,266 @@ +# Secrets Protection Design + +## Problem + +Shellguard currently has zero protection against reading sensitive files or +leaking secrets. Any allowed command (`cat`, `grep`, `head`, `printenv`, etc.) +can freely access `.env`, private keys, cloud credentials, and similar files. +The `download_file` tool has no path validation at all. + +### Threat Model + +1. **Malicious exfiltration** — prompt injection or model misbehavior causes the + LLM to issue tool calls that read secrets (e.g., `cat .env`, `printenv`) +2. **Accidental exposure** — the LLM innocently reads secrets during + debugging/exploration and includes them in responses, exposing them in + logs/chat history +3. **Privacy** — users don't want secrets sent to LLM API endpoints at all + +All three concerns demand the same solution: **local heuristics that block or +redact secrets before they ever leave the machine.** No network calls, no LLM +involvement. + +## Architecture + +A new standalone `secrets` package (zero internal dependencies) provides two +capabilities integrated into the existing pipeline: + +``` +User Input + │ + ▼ + Parse (parser) + │ + ▼ + Validate (validator) ← existing manifest-based validation + │ + ▼ + CheckSecrets (secrets) ← NEW: rejects sensitive file access + │ + ▼ + Reconstruct (ssh) + │ + ▼ + Execute (ssh) + │ + ▼ + Truncate (output) + │ + ▼ + ScrubSecrets (secrets) ← NEW: redacts secrets from output + │ + ▼ + Return to LLM +``` + +Both stages are function fields on `Core` for test injection, following the +existing pattern (`Parse`, `Validate`, `Reconstruct`, `Truncate`). + +## Phase 1: Pre-execution Path Checking + +### Sensitive Path Patterns + +Default patterns are hardcoded in the package. Users can override via config. + +| Category | Patterns | +| ----------------- | ----------------------------------------------------------------------------------------- | +| Env files | `.env`, `.env.*` (`.env.local`, `.env.production`, etc.) | +| SSH keys | `.ssh/id_*`, `.ssh/authorized_keys`, `.ssh/known_hosts` | +| TLS/Certs | `*.pem`, `*.key`, `*.pfx`, `*.p12` | +| Cloud credentials | `.aws/credentials`, `.aws/config`, `.gcloud/credentials.db`, `.azure/`, `.config/gcloud/` | +| K8s/Docker | `.kube/config`, `.docker/config.json` | +| App credentials | `credentials.json`, `service-account*.json`, `.netrc`, `.pgpass`, `.my.cnf` | +| Git | `.git-credentials`, `.gitconfig` | +| System | `/etc/shadow`, `/etc/gshadow`, `/etc/master.passwd` | +| Generic | `*secret*`, `*credential*`, `*token*` in filenames | + +### How It Works + +A new function `secrets.CheckPipeline(pipeline, config)` is called from +`Core.Execute()` between `Validate` and `Reconstruct`. For each segment's args: + +1. Normalize the path (`path.Clean`, resolve `~`, strip trailing slashes) +2. Check the **basename** against filename patterns (`.env`, `id_rsa`, etc.) +3. Check the **full path** against directory patterns (`.ssh/`, `.aws/`, etc.) +4. If a match is found, return a `SecretsError` with the specific pattern matched + +### Special Command Handling + +- **`printenv` with no args** — blocked (dumps all env vars including secrets) +- **`printenv VAR_NAME`** — blocked if `VAR_NAME` matches sensitive env var + patterns: `*KEY*`, `*SECRET*`, `*TOKEN*`, `*PASSWORD*`, `*CREDENTIAL*`, `*AUTH*` +- **`download_file`** — `remotePath` checked against the same pattern set in + `Core.DownloadFile()` + +### Configuration + +```go +type SecretsConfig struct { + // AllowedPaths overrides default blocking for specific paths. + // e.g., [".env.example", "/app/config/credentials.json"] + AllowedPaths []string `yaml:"allowed_paths"` + + // AdditionalPatterns adds more patterns to the default set. + AdditionalPatterns []string `yaml:"additional_patterns"` + + // DisablePathCheck disables pre-execution path checking entirely. + DisablePathCheck bool `yaml:"disable_path_check"` + + // DisableOutputScrub disables post-execution output scrubbing. + DisableOutputScrub bool `yaml:"disable_output_scrub"` +} +``` + +Hard block by default. Users can allowlist specific paths if they explicitly +choose to allow access. + +## Phase 2: Post-execution Output Scrubbing + +Catches secrets that appear in command output — e.g., `grep -r "database" +/app/config/` might return lines containing connection strings with embedded +passwords. + +### Patterns (High Confidence — Low False Positive Risk) + +| Pattern | Example | Redacted to | +| ------------------ | ------------------------------------------------- | -------------------------------------- | +| AWS Access Key | `AKIA1234567890ABCDEF` | `AKIA***REDACTED***` | +| AWS Secret Key | 40-char base64 after `aws_secret_access_key` | `***REDACTED***` | +| GitHub tokens | `ghp_xxxx`, `gho_xxxx`, `ghs_xxxx`, `github_pat_` | `ghp_***REDACTED***` | +| Stripe/OpenAI keys | `sk-xxxx`, `sk_live_xxxx`, `pk_live_xxxx` | `sk-***REDACTED***` | +| Private key blocks | `-----BEGIN (RSA\|EC\|OPENSSH) PRIVATE KEY-----` | `***REDACTED_PRIVATE_KEY***` | +| Bearer tokens | `Authorization: Bearer xxxx` | `Authorization: Bearer ***REDACTED***` | + +### Patterns (Medium Confidence — Tightened to Reduce False Positives) + +| Pattern | Tightening Heuristic | +| ---------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | +| JWTs (`eyJ...`) | Require all 3 dot-separated segments to be valid base64 AND total length > 30 chars | +| Connection strings (`postgresql://user:pass@host`) | Only redact the password portion between `://user:` and `@` | +| Generic key-value (`password=`, `secret=`, `token=`) | Require value portion to look like a secret: min length, mixed case/digits, not a common word. `password_policy=strict` should NOT match. | + +### Implementation + +`secrets.ScrubOutput(text string) string` applies all compiled regex patterns. +Patterns are compiled once at package init. Output is already capped at 64KB by +the truncation stage, so running ~10-15 compiled regexes is sub-millisecond. + +## Integration Points (Changes to Existing Code) + +### `shellguard.go` + +Add `SecretsConfig` to `Config` struct. Pass it through to `Core`. + +### `server/server.go` + +Add two function fields to `Core`: + +```go +type Core struct { + // ... existing fields ... + CheckSecrets func(*parser.Pipeline) error // default: secrets.CheckPipeline + ScrubSecrets func(string) string // default: secrets.ScrubOutput +} +``` + +Update `Core.Execute()`: + +```go +func (c *Core) Execute(ctx context.Context, in ExecuteInput) (output.CommandResult, error) { + pipeline, err := c.Parse(in.Command) + // ... + err = c.Validate(pipeline, c.Registry) + // ... + err = c.CheckSecrets(pipeline) // NEW + // ... + cmd := c.Reconstruct(pipeline, ...) + result := c.Runner.Execute(ctx, host, cmd, timeout) + truncated := c.Truncate(...) + // Scrub both stdout and stderr // NEW + truncated.Stdout = c.ScrubSecrets(truncated.Stdout) + truncated.Stderr = c.ScrubSecrets(truncated.Stderr) + return truncated, nil +} +``` + +Update `Core.DownloadFile()` to check `remotePath` against secrets patterns. + +### No changes to `validator/` or `output/` + +Secrets checking is a separate stage, keeping clean separation of concerns. + +## Package Layout + +``` +secrets/ + secrets.go # Core types, SecretsConfig, constructor, SecretsError + paths.go # Sensitive path patterns, CheckPipeline(), CheckPath() + scrub.go # Output scrubbing patterns, ScrubOutput() + paths_test.go # Path checking unit tests + scrub_test.go # Output scrubbing unit tests + false-positive regression + security_test.go # Attack vector tests (bypass attempts) + fuzz_test.go # Fuzz tests for both path checking and scrubbing +``` + +## Testing Strategy + +Following existing conventions: `testing` only, no testify, `got`/`want` +assertions, `t.Helper()` in helpers. + +### Path Tests (`paths_test.go`) + +Table-driven tests covering all pattern categories: + +- **Blocked:** `cat .env`, `head .ssh/id_rsa`, `grep -r foo credentials.json`, + `printenv`, `printenv AWS_SECRET_KEY` +- **Allowed:** `cat README.md`, `head main.go`, `printenv PATH`, `printenv HOME` +- **Allowlist override:** `.env.example` in allowed list → `cat .env.example` passes +- **Path normalization:** `../../.env`, `./foo/../.env`, `~/.ssh/id_rsa` + +### Scrub Tests (`scrub_test.go`) + +Table-driven with input/expected output pairs: + +- **Positive:** Each pattern category with realistic examples +- **False-positive regression:** `token_count=5`, `password_policy=strict`, + `secret_garden.txt`, base64 strings that start with `eyJ` but aren't JWTs + +### Security Tests (`security_test.go`) + +`TestSec_` prefix. Bypass attempts: + +- Path traversal: `../../.env`, `/app/../../../etc/shadow` +- Encoding tricks: URL-encoded paths, unicode homoglyphs +- Indirect access: `find / -name .env`, `grep -rl password /etc/` +- Argument hiding: flags that take path values (`grep -f .env foo.txt`) + +### Fuzz Tests (`fuzz_test.go`) + +- `FuzzCheckPath` — random strings never panic, always return valid error or nil +- `FuzzScrubOutput` — random input never panics, output length ≤ input length + + redaction marker overhead + +### Cross-layer Tests (`security_pipeline_test.go`) + +Add cases to existing test file: + +- `cat .env` → rejected +- `cat README.md` → allowed +- `printenv` (bare) → rejected +- `printenv PATH` → allowed +- `head .aws/credentials` → rejected + +## Open Questions + +1. **`grep -f .env`** — Should we check flag values that take file paths? + Manifests know which flags have `takes_value: true`, but we'd need to know + which of those values are file paths. Could inspect based on the flag name + (e.g., `-f`, `--file`, `--include-from`). Start simple, iterate. + +2. **Symlinks** — We can't resolve symlinks before execution (the file is on the + remote host). The heuristic only checks the path string as written. This is + an accepted limitation of the local-heuristic approach. + +3. **`find` output** — `find / -name .env` doesn't read secrets, it lists paths. + Should we block `find` commands that search for sensitive filenames? Leaning + toward yes for `-name .env` patterns but this adds complexity. diff --git a/integration_shellguard_test.go b/integration_shellguard_test.go index a1f057b..ebbea41 100644 --- a/integration_shellguard_test.go +++ b/integration_shellguard_test.go @@ -138,6 +138,7 @@ func TestIntegrationToolNames(t *testing.T) { m := registry[tool] if m == nil { t.Fatalf("missing manifest for toolkit tool %q", tool) + return } if m.Deny { t.Fatalf("tool %q must be allowed, but manifest is deny=true", tool) diff --git a/manifest/loaddir_test.go b/manifest/loaddir_test.go index 628427a..ee4d0cc 100644 --- a/manifest/loaddir_test.go +++ b/manifest/loaddir_test.go @@ -51,6 +51,7 @@ allows_path_args: true f := m.GetFlag("-v") if f == nil { t.Fatal("GetFlag(\"-v\") = nil") + return } if got, want := f.Description, "verbose output"; got != want { t.Fatalf("flag Description = %q, want %q", got, want) diff --git a/manifest/manifests/journalctl.yaml b/manifest/manifests/journalctl.yaml index e8c6655..a1ade83 100644 --- a/manifest/manifests/journalctl.yaml +++ b/manifest/manifests/journalctl.yaml @@ -11,6 +11,26 @@ flags: takes_value: true - flag: "--until" takes_value: true + - flag: "-p" + takes_value: true + - flag: "--priority" + takes_value: true + - flag: "--no-pager" + - flag: "-t" + takes_value: true + - flag: "--identifier" + takes_value: true + - flag: "-k" + - flag: "--dmesg" + - flag: "-b" + - flag: "-o" + takes_value: true + - flag: "--output" + takes_value: true + - flag: "-r" + - flag: "--reverse" + - flag: "-x" + - flag: "--catalog" - flag: "-f" deny: true reason: "Follow mode hangs until timeout. Use --since/--until for bounded queries." diff --git a/manifest/manifests/ps.yaml b/manifest/manifests/ps.yaml index 30329c8..52cacb0 100644 --- a/manifest/manifests/ps.yaml +++ b/manifest/manifests/ps.yaml @@ -6,5 +6,14 @@ flags: - flag: "-f" - flag: "-o" takes_value: true + - flag: "--sort" + takes_value: true + - flag: "-p" + takes_value: true + - flag: "-u" + takes_value: true + - flag: "-C" + takes_value: true + - flag: "--no-headers" stdin: false stdout: true diff --git a/manifest/manifests/systemctl_list-units.yaml b/manifest/manifests/systemctl_list-units.yaml index bd80fb8..9deb9ba 100644 --- a/manifest/manifests/systemctl_list-units.yaml +++ b/manifest/manifests/systemctl_list-units.yaml @@ -4,5 +4,6 @@ category: services flags: - flag: "--type" takes_value: true + - flag: "--failed" stdin: false stdout: true diff --git a/manifest/manifests/top.yaml b/manifest/manifests/top.yaml index a13a6d7..40dc4fb 100644 --- a/manifest/manifests/top.yaml +++ b/manifest/manifests/top.yaml @@ -5,6 +5,14 @@ flags: - flag: "-b" - flag: "-n" takes_value: true + - flag: "-o" + takes_value: true + - flag: "-p" + takes_value: true + - flag: "-u" + takes_value: true + - flag: "-w" + takes_value: true - flag: "-d" deny: true reason: "Only batch mode with fixed iteration count is allowed" diff --git a/ssh/auth_test.go b/ssh/auth_test.go index ecd6e3e..7574e9a 100644 --- a/ssh/auth_test.go +++ b/ssh/auth_test.go @@ -499,7 +499,15 @@ func startTestAgentWithKey(t *testing.T) string { // startTestAgentKeyring starts an ssh-agent serving the given keyring. func startTestAgentKeyring(t *testing.T, keyring agent.Agent) string { t.Helper() - sockPath := filepath.Join(t.TempDir(), "agent.sock") + // Use os.MkdirTemp with default temp dir to avoid t.TempDir()'s long paths + // which can exceed the 103-byte limit for unix sockets on macOS. + dir, err := os.MkdirTemp("", "sg-agent-") + if err != nil { + t.Fatalf("create temp dir: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + + sockPath := filepath.Join(dir, "agent.sock") ln, err := net.Listen("unix", sockPath) if err != nil { t.Fatalf("listen: %v", err)