diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8857a77e..f7fde0e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,43 @@ jobs: timeout-minutes: 3 run: sudo apt-get update && sudo apt-get install -y tmux + - name: Install sops and age + # internal/sops' integration tests drive the real sops binary: they + # mint an age identity, encrypt a fixture, and assert the measured + # behaviour `env set --sops` is built on — the exit status for an + # unchanged file, the absence of a trailing newline from + # `--extract --output`, and that untouched values keep byte-identical + # ciphertext. None of that is checkable without the binary. + # + # Installed from the release tarballs rather than apt, because Ubuntu's + # archive carries neither at a usable version. + # + # Bounded for the same reason the tmux step is: a download step that + # stops making progress leaves a PR with no Linux signal while reading + # as "CI is slow". + # Both downloads are CHECKSUM-VERIFIED before anything is installed. + # `sudo install` runs the asset as root's problem thereafter, so a + # swapped or compromised release would execute in CI with nothing + # between it and the runner. Pinning the tag alone does not help: a tag + # can be moved. + timeout-minutes: 3 + run: | + set -euo pipefail + curl -fsSL -o /tmp/sops \ + https://github.com/getsops/sops/releases/download/v3.13.3/sops-v3.13.3.linux.amd64 + echo 'e5bec3346a873ae91d871550f3e698c1aad962aff462a080e40f25fde17fef6b /tmp/sops' | sha256sum -c - + sudo install -m 0755 /tmp/sops /usr/local/bin/sops + + curl -fsSL -o /tmp/age.tgz \ + https://github.com/FiloSottile/age/releases/download/v1.2.1/age-v1.2.1-linux-amd64.tar.gz + echo '7df45a6cc87d4da11cc03a539a7470c15b1041ab2b396af088fe9990f7c79d50 /tmp/age.tgz' | sha256sum -c - + tar -xzf /tmp/age.tgz -C /tmp + sudo install -m 0755 /tmp/age/age /usr/local/bin/age + sudo install -m 0755 /tmp/age/age-keygen /usr/local/bin/age-keygen + + sops --version --disable-version-check + age-keygen --version + - name: Build run: go build ./... @@ -50,13 +87,23 @@ jobs: run: go vet ./... - name: Test - # FORGECTL_REQUIRE_TMUX turns those tests' skips into failures: on a - # runner where they are meant to run, a skip and a pass must not look - # alike. + # Each FORGECTL_REQUIRE_* turns a tool-absent skip into a failure: on a + # runner where these tests are meant to run, a skip and a pass must not + # look alike. The env gate is what enforces that, not the install step + # above — a step is a file anyone can edit in a PR, and deleting it + # would otherwise just make the tests quietly stop running. env: FORGECTL_REQUIRE_TMUX: '1' + FORGECTL_REQUIRE_SOPS_INTEGRATION: '1' run: go test ./... + - name: Real-sops integration tests (named) + # Same tests, -v: the run log naming each Integration test as PASS is + # the readable proof they executed rather than skipped. + env: + FORGECTL_REQUIRE_SOPS_INTEGRATION: '1' + run: go test -v -count=1 -run Integration ./internal/sops + - name: Real-tmux grammar tests (named) # Same tests, -v: the run log naming each *Isolated test as PASS is the # readable proof they executed rather than skipped. @@ -146,12 +193,57 @@ jobs: timeout-minutes: 5 run: brew list tmux || brew install tmux + - name: Install sops and age + # Darwin is where sops' behaviour was measured — the exit status for an + # unchanged file, the absent trailing newline from `--extract`, and the + # unbounded editor re-invocation loop — so it is the platform whose + # integration run is worth the most. + # + # NOT brew, unlike the tmux step above. This is a self-hosted runner + # whose Homebrew prefix is owned by another user: `brew install sops` + # fails with "The following directories are not writable by your user: + # /opt/homebrew". The tmux step survives only because tmux is already in + # the image, so its `brew list` short-circuits and `brew install` never + # runs. Anything NOT already installed is unreachable through brew here. + # + # So the release assets are installed into a runner-owned directory and + # that directory is prepended to PATH for later steps. Same + # checksum-before-install discipline as the ubuntu job, and the same + # reason: pinning the tag alone does not help, because a tag can move. + # + # The sops digest is from the publisher's own + # sops-v3.13.3.checksums.txt. age publishes no checksums file (only + # sigstore .proof files), so its digest was computed locally from the + # release asset — the same basis as the linux digest in the ubuntu job. + timeout-minutes: 5 + run: | + set -euo pipefail + mkdir -p "$HOME/.local/bin" + echo "$HOME/.local/bin" >> "$GITHUB_PATH" + + curl -fsSL -o /tmp/sops \ + https://github.com/getsops/sops/releases/download/v3.13.3/sops-v3.13.3.darwin.arm64 + echo 'b97c0d434aab577dc40310e8d22ff9e45eef4c80638ab978daae9b4681c59286 /tmp/sops' | shasum -a 256 -c - + install -m 0755 /tmp/sops "$HOME/.local/bin/sops" + + curl -fsSL -o /tmp/age.tgz \ + https://github.com/FiloSottile/age/releases/download/v1.2.1/age-v1.2.1-darwin-arm64.tar.gz + echo 'cf79875bd5970dc2dac60c87fa50cee1ff1f9a41b0eb273f65e174aff37c367a /tmp/age.tgz' | shasum -a 256 -c - + rm -rf /tmp/age + tar -xzf /tmp/age.tgz -C /tmp + install -m 0755 /tmp/age/age "$HOME/.local/bin/age" + install -m 0755 /tmp/age/age-keygen "$HOME/.local/bin/age-keygen" + + "$HOME/.local/bin/sops" --version --disable-version-check + "$HOME/.local/bin/age-keygen" --version + - name: Test (Darwin-gated coverage) # -v is deliberate: the run log naming each formerly-skipped test as - # PASS is the proof this coverage exists. FORGECTL_REQUIRE_TMUX makes a - # missing tmux a failure rather than a silent skip. + # PASS is the proof this coverage exists. Each FORGECTL_REQUIRE_* makes + # a missing tool a failure rather than a silent skip. env: FORGECTL_REQUIRE_TMUX: '1' + FORGECTL_REQUIRE_SOPS_INTEGRATION: '1' run: go test -v ./... helper: diff --git a/README.md b/README.md index 32f13a1e..da641693 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,14 @@ forgectl env get KEY --clipboard [--file .env] # value to clipboar forgectl env check [--file .env] [--example .env.example] # missing/extra keys, names only (see docs/commands/env.md for exit codes) forgectl env redact [--file .env] # print file with values masked **** # --file must name an env file (.env, .env.*, *.env); --any-file overrides, TTY-confirmed only +forgectl env set a.b.key --sops [--file secrets.sops.yaml] # one key into a SOPS-encrypted YAML file +# --sops takes a dotted path, defaults to secrets.sops.yaml at the repo root, and +# requires sops on PATH. The target must BOTH be named *.sops.yaml / *.sops.yml / +# *.enc.yaml / *.enc.yml / secrets.yaml / secrets.yml / secrets.*.yaml / +# secrets.*.yml AND carry a top-level sops: block; there is no --any-file escape. +# Untouched values keep byte-identical ciphertext, so the diff is the one key you set +# plus sops' own lastmodified and mac. Add *.sops.yaml.lock to .gitignore — the lock +# helper leaves a non-secret sibling behind by design. # branch — prune stale/orphaned git branches (alias: br) forgectl branch # dry-run report: local + remote branches, classified against diff --git a/docs/commands/env.md b/docs/commands/env.md index cbd269a2..5d029981 100644 --- a/docs/commands/env.md +++ b/docs/commands/env.md @@ -11,8 +11,47 @@ forgectl env get KEY --clipboard [--file .env] # value to clipboar forgectl env check [--file .env] [--example .env.example] # missing/extra keys, names only forgectl env redact [--file .env] # print file with values masked **** # --file must name an env file (.env, .env.*, *.env); --any-file overrides, TTY-confirmed only + +forgectl env set a.b.key --sops [--file secrets.sops.yaml] # one key into a SOPS-encrypted YAML file +# dotted path, arbitrary depth; defaults to secrets.sops.yaml at the REPO ROOT +# requires `sops` on PATH (`forgectl doctor` reports its version) ``` +## `--sops` — writing into a SOPS-encrypted file + +The same guarantee as the `.env` path, for SOPS YAML: the value arrives on piped stdin, a no-echo prompt, or `--clipboard`, and never enters an argv, terminal output, or a transcript. + +The two obvious alternatives both fail that. `sops set file '["a"]["b"]' '"value"'` puts the plaintext in argv — visible in `ps`, left in shell history. `sops file` opens `$EDITOR` on the whole decrypted document, which is a lot of exposed plaintext to paste one line into. + +**How it works.** `sops ` decrypts to a temp file, runs `$EDITOR`, and re-encrypts whatever comes back. forgectl sets `EDITOR` to itself (a hidden `__sops-edit` subcommand), passes the key path in the environment, and passes the value as a **file whose path** is in the environment. The value itself never enters an environment or an argv. + +**The diff is reviewable, deliberately.** The edit is line-wise text, not a YAML round-trip: re-emitting the document would reflow every block and reorder keys, and in an encrypted file every reflowed line is a ciphertext change. Untouched values keep byte-identical ciphertext, so a replace changes 3 lines — the value plus sops' own `lastmodified` and `mac` — and an add changes 2 and removes 1. + +**Success means it landed encrypted.** After the write, forgectl decrypts the value back and compares it byte-exactly, *and* re-parses the ciphertext to confirm the scalar at that exact path carries an `ENC[AES256_GCM,` marker. The second check is not redundant: a value stored in cleartext round-trips through a decrypt perfectly well, so a round-trip alone cannot detect it. + +**Target rules — both must hold, and there is no escape hatch:** + +- the filename matches `*.sops.yaml`, `*.sops.yml`, `*.enc.yaml`, `*.enc.yml`, `secrets.yaml`, `secrets.yml`, or `secrets.*.yaml`/`.yml` +- the file content carries a top-level `sops:` mapping + +`--any-file` is refused with `--sops` rather than silently ignored. A SOPS file under some other name is unreachable — that is a deliberate refusal, not a gap: the alternative is an interactive confirmation, and the confirmation path is where a time-of-check/time-of-use defect lived. Renaming the file costs less than that surface. + +**What it refuses, and why refusing is the right answer:** + +| Refusal | Reason | +|---|---| +| A missing block, at any depth | A block forgectl invented would encrypt fine and the consumer would read nothing from it | +| A path whose key *or any ancestor* falls outside the file's encryption rules | sops would write the value in **cleartext** beside its encrypted siblings — measured live with `unencrypted_suffix` in force | +| A path naming a block rather than a scalar | Writing a scalar over a mapping header strands its children | +| A dotted key *name* | `a.b.c` cannot distinguish `{a, b.c}` from `{a, b, c}`; escaping is a surface for a case no estate file has | +| The top-level `sops` block | It holds the file's own recipients, MAC, and rules | +| A value with a newline, a C0 control byte other than tab, or invalid UTF-8 | YAML forbids these in a scalar, and the resulting unparseable document makes sops re-invoke its editor **without bound** | +| A document shape the line model cannot bound | A sequence where a mapping was expected, tab indentation, a multi-document stream, a header with a trailing comment — each would mis-place the key and corrupt the file silently | + +**Out of scope:** reading or listing SOPS values, creating a missing file or block, non-scalar values, and key rotation or recipient management. + +**Gitignore `*.sops.yaml.lock`.** The lock helper leaves a non-secret sibling beside whatever it locked, by design. + **`env check`'s exit codes are part of its contract, not incidental:** exit `1` means the file and its example both exist but disagree — missing and/or extra keys (drift); exit `2` means either the env file or the `--example` file is absent, so no comparison could run at all. `env check --json` emits the drift as a single object on stdout, `{"missing":[...],"extra":[...]}`, for scripted callers. **Blessed value producers** for `env set`, non-inline patterns first: @@ -35,6 +74,7 @@ forgectl env set API_KEY # interactive, no ech Two things changed. Resolution happens exactly once, and its result travels as a value rather than a boolean, so the path a human confirms is the path that gets written. And that value carries an **open descriptor on the containing directory**, pinned at resolution, with every read, write, and rename performed relative to it — so no later operation re-walks the path by name. That second half is what closes the interesting case: a fix that carried only the path still let an *intermediate directory* be swapped during the confirmation, which redirected the write exactly as the original bug did. **What remains:** the directory is pinned by path immediately after resolution, so its own components are walked once more at that instant — a window of microseconds rather than of operator think-time, and the same ordinary same-uid local race that predates this command. Closing even that would need a component-by-component walk from the repository root. +- **`--sops` widens the authority `env set` grants, and the paragraph below predates it.** Granting a session `env set` now also grants write authority over repo-contained **SOPS documents** — a materially larger thing than a `.env`, because a SOPS file typically holds production credentials rather than local development ones. The bounds are the same in shape (repo containment, a filename allowlist, a content check) and there is no `--any-file` override on that route, but the *blast radius* of the authority is bigger. Grant it deliberately. - **Agent-write threat model, one line:** running `env set`/`env get` under an agent grants that agent write authority over repo-contained **env files** for the duration of the session — containment (refuses outside the git repo), the env-file-name rule (below), 0600 permissions, and atomic writes bound the blast radius, but they don't remove the authority itself. The two subcommands grant distinct authorities: `env set` is **write** authority (the agent can create or overwrite a key in the file); `env get --clipboard` is **read/exfil** authority (the agent can copy an existing secret to the clipboard, where — see the residual-risk note above — any local process or clipboard manager can then read it too). Granting one does not imply granting the other. **Safety notes:** diff --git a/docs/plans/2026-09-12-env-set-sops.md b/docs/plans/2026-09-12-env-set-sops.md new file mode 100644 index 00000000..f3c84043 --- /dev/null +++ b/docs/plans/2026-09-12-env-set-sops.md @@ -0,0 +1,315 @@ +--- +status: complete +branch: feat/env-set-sops +base_branch: fix/any-file-confirm +pr: forgectl#517 +base_pr: forgectl#515 +issue: forgectl#498 +approved_in: session +approved_session_id: 2d4b9aa6-61b9-4645-8591-016fae184f38 +--- + +# `env set --sops` — write one key into a SOPS file without exposing the value + +## Goal + +`forgectl env set agentgateway.llm_key_hermes --sops` writes one key into +`secrets.sops.yaml` from stdin, a no-echo prompt, or the clipboard; untouched +values keep byte-identical ciphertext and key order; and the write is proven to +have landed **encrypted** before success is reported. + +Closes forgectl#498. + +## Context + +`forgectl env set KEY` already solves "get a secret into a file without it +touching argv, a terminal, or a transcript" for `.env`. SOPS-encrypted YAML has +the identical problem and had no equivalent, so every estate secret write was +hand-managed. + +Both obvious workarounds are wrong. `sops set file '["a"]["b"]' '"value"'` puts +the plaintext in argv — visible in `ps`, left in shell history. `sops file` +drops you into vim to paste one line into a 200-line encrypted document. + +## Decisions taken + +| Decision | Choice | Why | +|---|---|---| +| Command shape | `--sops` flag on the existing `env set` | One verb; no new module manifest | +| Path depth | Arbitrary, dotted (`a.b.c.d`) | The general case costs little once the walk is recursive | +| Dots inside a key name | Refused | A dotted string cannot disambiguate `{a, b.c}` from `{a, b, c}` | +| SOPS driver | Shell out to the `sops` binary | No vendored cloud-KMS SDK tree, no coupling to SOPS' internal API | +| Target gate | Name allowlist **and** content check, no escape hatch | Operator's call; see § Deviations | +| `--sops` with `--any-file` | Refuse | Operator's call: the flag would imply a bypass that does not exist | + +## Alternatives declined + +- **A separate `forgectl secret` command group.** Cleaner separation, but + Cameron chose one verb. The code paths stay separate underneath. +- **`getsops/sops` as a Go library.** No subprocess and richer errors, at the + cost of a large transitive cloud-KMS tree — and the library path inherits + SOPS' YAML marshalling, which is the whole-file-reflow problem this change + exists to avoid. +- **`sops set`.** Puts the plaintext in argv, the exposure #498 was filed about. +- **Backslash-escaped dotted keys.** An escaping surface on every path for a + case no estate secrets file has. +- **Reusing `env.writeAtomic` for the restore.** Its `.env-*.tmp` name and + forced 0600 are `.env` semantics. + +## Panel + +Panel: plan-reviewer, security-posture-reviewer, red-team-reviewer ran — 31 +findings, 29 folded in, 2 declined. + +### Findings declined + +- **`govulncheck ./...` in CI** — worth doing, unrelated to this path. Filed as + forgectl#514. +- **A `cadence-hooks` issue** for its sops-decrypt guard's documented blind + spot — correct, but a change to another repo's docs. + +## Measured ground truth + +Every number here was measured on sops 3.13.3 / darwin 25.5, not read from +documentation. + +| Question | Measured | +|---|---| +| `EDITOR` handling | Shell-word split (quotes honoured), temp path appended as the only argument; self-exec works | +| `sops -d --extract --output` trailing newline | **None.** A 9-byte value writes a 9-byte file — so the read-back compares raw bytes with no strip | +| Editor leaves the temp unchanged | Exits **200**, `File has not changed, exiting.`, file untouched, returns immediately | +| Editor exits non-zero | Exits 201, clean failure, encrypted file byte-identical | +| Editor writes invalid YAML | sops re-invokes the editor **without bound**: 36,851 invocations and 8.4 MB of stderr in ~3 minutes, still going when killed | +| Diff size, replace | 3 changed lines: the content line plus sops' `lastmodified` and `mac` | +| Diff size, add | 2 insertions / 1 deletion | +| `.sops.yaml` discovery | Walks up from the **working directory**, not the input file's path | +| `unencrypted_suffix` | A matching key is written in **plaintext** beside `ENC[AES256_GCM,...]` siblings | + +## Shipped shape + +- **`internal/sops`** — the pure half (`path.go`, `value.go`, `edit.go`, + `file.go`) makes every decision and touches nothing; `driver.go` runs the + binary and the filesystem around those decisions. +- **`internal/cli/sops_edit.go`** — the hidden `__sops-edit` subcommand sops + invokes as its `EDITOR`, so the value travels by file and the key path by + environment. Neither is ever an argument to any process. +- **`internal/cli/env.go`** — the `--sops` flag, the branching key gate, the + repo-root default, the target gate, and the clipboard route. +- **`internal/exec/sensitive.go`** — two sops `CommandKind`s and four + `EnvMutation` constructors. +- **`internal/doctor`** — a `sops` check reporting the **version**. + +## Deviations + +**The execution seam is `exec.SensitiveRunner`, not `exec.Runner` as planned.** +The plan's own step 11 required capturing sops' output to a file, which +`exec.Runner` cannot do — its methods capture into strings. Worse, `runAndWrap` +logs child stderr at `Error` level, which survives any configured log level and +can be pointed at a file on disk, and retains it on a `*CommandError` fang +renders. A sops YAML parse error quotes the offending line, and that line is +`key: ''`. `SensitiveRunner` already existed for exactly this and +can render neither, and its 64 KiB stream cap is a second brake on the measured +8.4 MB stderr case. + +**The target gate refuses rather than confirms, and that reordered the whole +plan.** The plan routed a non-standard target through `resolveAllowAnyFile`. +The step-0 security review found that function carried a demonstrated RCE — the +confirmed path and the written path could be two different files — and that its +TTY probe reads stdin, so `--any-file` refuses whenever a value is piped, +making the plan's own `printf … | forgectl env set --sops` example impossible. +Cameron chose to refuse a non-standard name outright. The fix landed first, as +forgectl#515, and this branch is stacked on it. + +**Three env vars, not five.** The work directory is named once and the value, +nonce, result, counter, and error files sit at fixed names inside it. Every +variable is a name an attacker could try to set. + +**The nonce is not described as a privilege boundary, because it is not one.** +The plan claimed it stopped `__sops-edit` being "a bare arbitrary-YAML-write +primitive". It does not: a caller who can set the environment can also create +the directory and nonce file it names — and a caller who can exec forgectl can +already write YAML with a shell, so the subcommand grants no capability its +invoker lacked. What the nonce and the work-directory name constraint do bound +is a **stray or replayed** invocation. The output validation and the once-only +counter are the load-bearing guards. + +**An error relay was added.** The editor's refusals are the actionable ones — a +typo'd block name is the commonest mistake — and they live only in the child, +whose stderr is sops' stderr and therefore unsurfaceable. The child writes its +message to a file in the work directory and the driver surfaces that instead. +Safe because every relayed message originates in forgectl and names a rule, a +property the package's tests assert. + +**The name check precedes the existence check.** A refused name should refuse +on the name whatever the filesystem says, and answering existence first turns a +refused path into an existence oracle. + +**`env.RepoRoot` came back.** forgectl#515 deleted it as dead; the `--sops` +default lives at the repository root, so it now has a caller. + +**The macOS runner installs sops from the release asset, not Homebrew.** The +plan said "beside the `tmux` installs", and `brew install sops` fails outright +on the self-hosted runner: its Homebrew prefix is owned by another user, so the +step dies with `/opt/homebrew` not writable. The neighbouring tmux step only +survives because tmux is already in the image and its `brew list` +short-circuits — nothing absent is installable through brew there. Both +binaries now install from checksum-verified release assets into a runner-owned +directory added to `PATH`, matching the ubuntu job. + +**`openatCreate` retries a spurious `ENOENT`.** Not in the plan, and not +optional: `unix.Openat` with `O_CREAT` on darwin returned 582 spurious `ENOENT` +in 800 concurrent attempts where `os.OpenFile` with identical flags returned +800/800. Landed with forgectl#515. + +## Review round — three Criticals, all reproduced + +A two-arm Opus review (security, correctness) over the finished branch found +three Critical defects. None was a design mistake; all three were the same +shape of error — a check that looked right and could not go red on the case it +existed for. + +**The encryption-rule check tested only the leaf.** sops applies +`unencrypted_suffix` and friends to a key AND ITS WHOLE SUBTREE, so a path +whose *parent* carried `_unencrypted` passed the check and the secret landed in +plaintext with the command reporting success — reproduced end to end, the exact +failure the check was built to prevent. `WouldStoreCleartext` now takes +`[]string` and walks every segment with sops' real precedence; the signature +change is what stops the leaf-only call being written again. The same bug ran +backwards too: an ancestor-scoped `encrypted_regex` falsely refused every key +beneath the block it matched, which made the feature unusable on such a file. + +**The encrypted-at-path assertion was document-order dependent.** It scanned +for the first line whose trimmed text began with `leaf + ":"`, anywhere in the +document, so any same-named encrypted key elsewhere satisfied it — including +sops' own `mac`. Proven by reordering one write: identical input passed with +the secret in plaintext, or correctly went red, depending only on which line +came first. So the check the design calls "the one that matters most" was the +one that could not be made to go red on demand. It resolves the path through +`yaml.v3` now. + +**A bare prefix match destroyed a colon-bearing sibling.** `a:b: 'v'` is valid +YAML and decodes to the key `a:b`; setting `a` matched that line, and since the +replace cuts at the first colon the result was `a: 'new'` — another key and its +encrypted value gone, reported as `replaced a`, passing every downstream check +because extracting the path then returns exactly what was supplied. `findLeaf` +now requires a space or end-of-line after the colon. + +**Also folded in:** a leaf naming a block now refuses by name rather than +emitting YAML the child rejects; the staged plaintext and the decrypted +read-back are deleted the moment they are consumed, shrinking the window in +which a Ctrl-C could leave a committable secret in the work tree; `__sops-edit` +refuses a symlinked or non-mapping target, closing the one write in forgectl +that had no containment at all; the sops output capture happens only on the +path that reports it, rather than orphaning a file in `$TMPDIR` on every +successful run; both sops calls pass `--disable-version-check`; the protocol's +environment-variable names are now shared constants rather than literals +spelled in two packages; CI verifies the `sops` and `age` download checksums +before installing them; and `docs/commands/env.md` gained the `--sops` +reference plus a note that the flag widens the authority `env set` grants. + +**Three comments were corrected rather than deleted**, each having claimed a +control the code did not have: `readOutcome`'s stated reason for its default +was factually wrong about which path reaches it, `ReplaceSopsNonce` still +described the nonce as a privilege boundary and contradicted the two artifacts +that correctly do not, and the editor's write claimed a mode restatement +prevented a umask from widening a file it cannot affect. + +**Came back clean and worth recording:** the unbounded-loop defence held +against every value the reviewer could find, including the Unicode line breaks +`U+0085`/`U+2028`/`U+2029` that `NormalizeValue` permits — `U+0085` corrupted +the round-trip and the byte-exact comparison caught it, which is the check that +the declined trailing-newline strip would have masked. And `SetScalar`'s +refusal list turns out to be largely unreachable through the driver, because +the document it sees is sops' own yaml.v3 re-emission: tabs, CRLF, flow +mappings, anchors, and multi-document streams are all normalised away before +the line model sees them. The refusals stay as a contract on the function. + +## Two review findings from forgectl#515, fixed here + +CodeRabbit raised three findings on the base PR. One was already fixed there +(`dc8b7ef`); the other two land on this branch, because this branch contains +the base and the write path this feature adds depends on exactly that lock +correctness. + +**`openLock` did not validate the descriptor it returned.** `withFileLock` +Lstats the lock name and then opens it, and `O_NOFOLLOW` closes that window for +a symlink ONLY — a swap to a FIFO inside the same window is not a symlink, so +nothing caught it. Since flock locks an open file description, two writers on +two FIFO inodes would both believe they held the lock, and the parse→write +section that exists to prevent a lost update would stop preventing one. +`openLock` now does the same post-open regular-file check `openRegular` +already did. The doc comment claiming the Lstat refused a FIFO was corrected +rather than deleted — the fourth instance of this repo's signature defect, a +comment asserting a control the code did not have. + +Verified with a negative control: with the check reverted, the new `fifo` +subtest fails and the `symlink` subtest still passes, which is what proves the +new check is what adds the FIFO refusal rather than duplicating `O_NOFOLLOW`. + +**Four refusal branches abandoned an open directory descriptor.** A `Target` +owns a dirfd, and `resolveEnvTarget` returned `Target{}` on four refusal paths +without closing it, so a long-lived process refusing repeatedly retained one +descriptor per attempt. Every refusal now routes through one closure, which is +what keeps the next branch added there from leaking — a caller-side `defer` +gives no signal when a return is missed. The test fixture and the one direct +`ResolveTarget` test close theirs too. + +## Bot review round — four findings, all fixed + +**The block refusal echoed the leaf segment.** `SetScalar`'s +names-a-block refusal carried the segment in `%q`, and that message is relayed +out of the child process to the operator's terminal. `ParsePath`'s grammar +admits plenty of provider token formats as one valid segment, so a secret +pasted into the key slot arrived as the leaf and was printed — a direct +violation of this plan's own refusal rule. The message now names only the rule. +The test asserts no refusal echoes the leaf, with a negative control proving +the assertion goes red when it does. + +The ancestor segments still name the block they could not find, and the +asymmetry is deliberate: a mistyped block name is the commonest mistake on this +path and the only actionable thing the message can carry, and an ancestor is not +the paste site — a bare pasted secret is a single-segment path, whose whole walk +is the leaf. + +That first assertion was itself over-broad and had to be fixed before it meant +anything: the sequence refusal reads "only scalar keys in a mapping", and the +fixture's leaf was named `key`, so a substring match caught an English word +rather than an echo. The refusal fixtures now use an unmistakable leaf. + +**`openLock` gained `O_NONBLOCK`.** The comment justified its absence from a +darwin measurement, but POSIX leaves `O_RDWR` on a FIFO undefined and permits a +blocking open for a character device that supports non-blocking mode — either +of which would stall before the regular-file check ran. The flag has no effect +on regular-file I/O, which is the only case that reaches the return. + +**Two doc corrections.** The command reference listed a value's rejected bytes +as "a C0 control byte" when `NormalizeValue` accepts tab, and the README's +target allowlist omitted every `.yml` form the code accepts. + +## Out of scope + +- Reading or listing SOPS values (`env get --sops`, `env keys --sops`). +- Creating a missing block or a missing file. +- Non-scalar values (sequences, maps, multi-line strings). +- Key rotation, recipient management, `updatekeys`. +- A SOPS file under a name outside the allowlist. + +## Verification + +```bash +gofmt -l . # clean +go vet ./... # clean +go test ./... -count=1 # all packages pass +golangci-lint run # 0 issues +FORGECTL_REQUIRE_SOPS_INTEGRATION=1 go test -run Integration ./internal/sops +``` + +End to end against the built binary, in a scratch repo with an age identity: +add from piped stdin, byte-exact round-trip, zero plaintext occurrences, an +idempotent re-set reporting success, a rotation reporting `replaced`, three +refusals each leaving the file byte-identical, and no work directory left +behind. + +The integration gate was verified in both directions: with `sops` off PATH the +tests skip by default and **fail** under +`FORGECTL_REQUIRE_SOPS_INTEGRATION=1`. diff --git a/internal/cli/env.go b/internal/cli/env.go index bec264a6..6f706acb 100644 --- a/internal/cli/env.go +++ b/internal/cli/env.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path/filepath" "github.com/spf13/cobra" "golang.org/x/term" @@ -12,6 +13,7 @@ import ( clippkg "github.com/cameronsjo/forgectl/internal/clip" envpkg "github.com/cameronsjo/forgectl/internal/env" "github.com/cameronsjo/forgectl/internal/module" + sopspkg "github.com/cameronsjo/forgectl/internal/sops" "github.com/cameronsjo/forgectl/internal/termsafe" "github.com/cameronsjo/forgectl/internal/theme" ) @@ -82,19 +84,32 @@ func resolveEnvTarget(anyFile bool, file, cwd string, th theme.Theme) (envpkg.Ta if err != nil { return envpkg.Target{}, err } + // A refusal from here on abandons a target that OWNS an open descriptor on + // its containing directory, so the refusal has to close it. Callers only + // defer Close on a target they were actually given, and cannot close one + // the error path never handed back. Routing every refusal through one + // closure is what keeps the next branch added here from leaking: the + // caller-side `defer` pattern gives no signal at all when a return is + // missed, and the cost lands on the long-lived process (the TUI resolving + // repeatedly), not the one-shot command. + refuse := func(err error) (envpkg.Target, error) { + target.Close() + return envpkg.Target{}, err + } + clearErr := target.Clear() if clearErr == nil { return target, nil } if !anyFile { - return envpkg.Target{}, clearErr + return refuse(clearErr) } if !isTerminal() { // Phrased to lead with a word, not the flag: fang title-cases the // first token when it renders an error, so "--any-file requires …" // reaches the user as "--Any-File requires …" — a flag spelling // that does not exist and that someone will reasonably try to type. - return envpkg.Target{}, errors.New("an interactive terminal is required for --any-file") + return refuse(errors.New("an interactive terminal is required for --any-file")) } // Prompts with the repo-relative resolved path: resolved so a human // cannot approve a file they never saw (a `.env` symlinked to @@ -103,10 +118,10 @@ func resolveEnvTarget(anyFile bool, file, cwd string, th theme.Theme) (envpkg.Ta // (forgectl#481). ok, err := confirmAnyFile(th, fmt.Sprintf("%q is not a recognized env file (.env, .env.*, or *.env) — operate on it anyway?", target.Rel())) if err != nil { - return envpkg.Target{}, err + return refuse(err) } if !ok { - return envpkg.Target{}, fmt.Errorf("refusing %s: --any-file confirmation declined", target.Rel()) + return refuse(fmt.Errorf("refusing %s: --any-file confirmation declined", target.Rel())) } return target, nil } @@ -132,14 +147,20 @@ func newEnvCmd(deps module.Deps) *cobra.Command { // log field — env's whole reason to exist is that a value never // prints, and a length is itself signal about a secret (the plan // declines a partial-redact reveal for the exact same reason). - client := envpkg.NewClient(clippkg.New(deps.Runner, clippkg.WithSensitive())) - return newEnvCmdForClient(client, deps.Theme) + clip := clippkg.New(deps.Runner, clippkg.WithSensitive()) + client := envpkg.NewClient(clip) + // The sops driver runs over the SENSITIVE seam, not deps.Runner: sops' + // stderr quotes the line it failed to parse, and that line holds the + // value. deps.SensitiveRunner is nil when a caller did not wire one, and + // nil is correct to pass through — the sops route refuses rather than + // silently falling back to the argv-logging path. + return newEnvCmdForClient(client, sopspkg.NewClient(deps.SensitiveRunner), clip, deps.Theme) } // newEnvCmdForClient builds the command over an already-constructed // client — split out so tests can inject a fake-wired *env.Client (mirrors // newYCmdForClient/newDockerCmdForClient) without going through newEnvCmd. -func newEnvCmdForClient(client *envpkg.Client, th theme.Theme) *cobra.Command { +func newEnvCmdForClient(client *envpkg.Client, sopsClient *sopspkg.Client, clip *clippkg.Client, th theme.Theme) *cobra.Command { var file string var anyFile bool @@ -182,7 +203,7 @@ argv and transcript; forgectl can't close a channel it doesn't own.`, cmd.AddCommand( newEnvKeysCmd(&file, &anyFile, th), - newEnvSetCmd(client, &file, &anyFile, th), + newEnvSetCmd(client, sopsClient, clip, &file, &anyFile, th), newEnvGetCmd(client, &file, &anyFile, th), newEnvCheckCmd(&file, &anyFile, th), newEnvRedactCmd(&file, &anyFile, th), @@ -251,8 +272,9 @@ func newEnvKeysCmd(file *string, anyFile *bool, th theme.Theme) *cobra.Command { } // newEnvSetCmd builds `env set`. -func newEnvSetCmd(client *envpkg.Client, file *string, anyFile *bool, th theme.Theme) *cobra.Command { +func newEnvSetCmd(client *envpkg.Client, sopsClient *sopspkg.Client, clip *clippkg.Client, file *string, anyFile *bool, th theme.Theme) *cobra.Command { var clipboard bool + var useSops bool cmd := &cobra.Command{ Use: "set KEY", @@ -260,18 +282,39 @@ func newEnvSetCmd(client *envpkg.Client, file *string, anyFile *bool, th theme.T Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { key := args[0] - // Checked here, BEFORE reading stdin or touching the clipboard — - // not just inside the domain pipeline — so a hostile key shape - // (env set KEY=VALUE) refuses without ever consuming input. - // "ValidKey first, refuse before touching the file or reading - // input" applies to the CLI's own input-sourcing step, too. - if !envpkg.ValidKey(key) { + // The key gate BRANCHES, and it stays in this position. + // + // Checking here — before stdin is read or the clipboard is + // touched — is what makes a hostile key shape (`env set + // KEY=VALUE`) refuse without consuming input. But the two routes + // have different grammars: ValidKey forbids dots and hyphens, + // so under --sops it would reject every dotted path the feature + // exists to accept. Branching keeps the ordering and admits the + // right shape on each route. + if useSops { + if _, err := sopspkg.ParsePath(key); err != nil { + return err + } + } else if !envpkg.ValidKey(key) { return fmt.Errorf("key must match %s; values are piped or --clipboard, never argv", envKeyPattern) } + + // --any-file is inert on the sops route (it never reaches the + // env-file allowlist), so accepting it silently would imply a + // bypass that does not exist. Refusing says so. + if useSops && *anyFile { + return errors.New("--any-file does not apply with --sops; a SOPS target must match " + sopspkg.NameShapes()) + } + cwd, err := os.Getwd() if err != nil { return err } + + if useSops { + return runEnvSetSops(cmd, sopsClient, clip, *file, cwd, key, clipboard) + } + target, err := resolveEnvTarget(*anyFile, *file, cwd, th) if err != nil { return err @@ -301,9 +344,100 @@ func newEnvSetCmd(client *envpkg.Client, file *string, anyFile *bool, th theme.T }, } cmd.Flags().BoolVar(&clipboard, "clipboard", false, "read the value from the clipboard (wins over piped stdin)") + cmd.Flags().BoolVar(&useSops, "sops", false, "write one dotted KEY path into a SOPS-encrypted YAML file instead of a .env file") return cmd } +// sopsDefaultFile is the target `--sops` uses when --file was not given. It +// resolves against the REPOSITORY ROOT rather than the current directory, +// which is where these files live; without that the .env default would +// silently aim at the wrong file and the failure would read as a sops problem +// rather than a path one. +const sopsDefaultFile = "secrets.sops.yaml" + +// runEnvSetSops is the --sops route: resolve and gate the target, source the +// value, hand both to the driver. +// +// # The gate, and why there is no escape hatch +// +// A SOPS file is named secrets.sops.yaml, which the .env allowlist refuses — +// so this route needs an allowlist of its own rather than a bypass of that +// one. Three conditions, all required: the path resolves inside the +// repository (env.ResolveTarget), its basename matches a SOPS shape, and its +// CONTENT carries a top-level sops: mapping. The name check alone would admit +// a plain YAML file someone called secrets.sops.yaml; the content check alone +// would admit any encrypted document anywhere in the tree. +// +// A SOPS file under some other name is unreachable, deliberately. The +// alternative is an interactive confirmation, and that is the path that +// carried the time-of-check/time-of-use defect fixed in the commit this +// branch sits on. Refusing costs a rename and takes a whole class of bug off +// the table. +func runEnvSetSops(cmd *cobra.Command, sopsClient *sopspkg.Client, clip *clippkg.Client, file, cwd, key string, clipboard bool) error { + // The .env default would aim at the wrong file, so substitute this + // route's own default when --file was not given. Changed() reads the + // parent's persistent flag correctly — pflag shares the *Flag pointer. + if !cmd.Flags().Changed("file") { + root, err := envpkg.RepoRoot(cwd) + if err != nil { + return err + } + file = filepath.Join(root, sopsDefaultFile) + } + + target, err := envpkg.ResolveTarget(file, cwd) + if err != nil { + return err + } + defer target.Close() + + // The NAME check precedes the existence check, and the order matters twice + // over. A refused name should refuse on the name whatever the filesystem + // says — "not found" is the wrong reason and sends the operator looking + // for a missing file rather than at their --file argument. And answering + // existence first turns a refused path into an existence oracle, which is + // a disclosure a refusal has no business making. + if !sopspkg.IsSOPSFileName(filepath.Base(target.Abs())) { + return fmt.Errorf("refusing %s: --sops requires a target named one of %s", target.Rel(), sopspkg.NameShapes()) + } + if !target.Exists { + // Creating a file is out of scope, and a rule-named refusal beats a + // raw os.Open error that reads as an internal fault. + return fmt.Errorf("%s not found; --sops edits an existing SOPS file and does not create one", target.Rel()) + } + + // Sourced after the target is gated, so a refusable target never consumes + // input — the same ordering the key gate follows. + var value string + if clipboard { + // client.SetFromClipboard runs the whole .env pipeline and would + // append a plaintext KEY=value line to an encrypted file, so the + // clipboard is read directly here and handed to the sops driver. + value, err = clip.Paste(cmd.Context()) + } else { + value, err = resolveSetValue(cmd) + } + if err != nil { + return err + } + + outcome, err := sopsClient.SetValue(cmd.Context(), target, key, value) + if err != nil { + return err + } + + // The outcomes are distinguished because replacing a key is a rotation + // and adding one is new configuration — an operator reading a transcript + // wants to know which happened. + switch outcome { + case sopspkg.OutcomeAdded: + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "added %s to %s\n", key, target.Rel()) + default: + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "replaced %s in %s\n", key, target.Rel()) + } + return nil +} + // resolveSetValue reads the value `set` will use when --clipboard wasn't // given: piped stdin when the real stdin isn't a terminal, else an // interactive no-echo prompt. The trailing-newline strip and empty-value diff --git a/internal/cli/env_sops_test.go b/internal/cli/env_sops_test.go new file mode 100644 index 00000000..c26c556b --- /dev/null +++ b/internal/cli/env_sops_test.go @@ -0,0 +1,436 @@ +package cli + +// Test plan for the --sops route's CLI gate (internal/cli/env.go's +// runEnvSetSops) and the hidden editor (sops_edit.go). +// +// These never run sops: they pin the CLI's own refusals, which all fire BEFORE +// the subprocess. internal/sops' gated integration tests cover the subprocess. +// +// The gate +// [x] A dotted path is accepted where ValidKey would reject it — the branch +// [x] A hostile key shape still refuses, and before any input is read +// [x] --any-file with --sops refuses rather than being silently inert +// [x] A target with a non-SOPS name refuses, naming the allowed shapes +// [x] A missing target refuses without attempting creation +// [x] A .env target refuses (the two allowlists do not overlap) +// [x] No refusal echoes the value +// +// The editor +// [x] Refuses with no work directory in the environment +// [x] Refuses on a nonce mismatch +// [x] Refuses a second invocation in one run + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/cameronsjo/forgectl/internal/theme" +) + +const sopsFixtureDoc = `agentgateway: + llm_key_hermes: ENC[AES256_GCM,data:abc,type:str] +sops: + mac: ENC[AES256_GCM,data:def,type:str] + version: 3.13.3 +` + +// sopsCLIFixture writes a repo containing a plausible encrypted file. +func sopsCLIFixture(t *testing.T) string { + t.Helper() + repo := t.TempDir() + initEnvGitRepo(t, repo) + if err := os.WriteFile(filepath.Join(repo, "secrets.sops.yaml"), []byte(sopsFixtureDoc), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return repo +} + +// sopsWorkdirFixture creates a directory named the way the driver names its +// own, because resolveWorkdir requires that prefix. A bare t.TempDir() is +// rejected — which is the constraint working, and the reason this helper +// exists rather than each test hand-rolling a path. +func sopsWorkdirFixture(t *testing.T) string { + t.Helper() + dir := filepath.Join(t.TempDir(), workdirPrefix+"fixture") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatalf("Mkdir: %v", err) + } + return dir +} + +// runEnvSet drives the command tree and returns both streams and its error. +func runEnvSet(t *testing.T, repo string, args ...string) (stdoutText, stderrText string, err error) { + t.Helper() + t.Chdir(repo) + client, _ := envFixture() + cmd := newEnvTestCmd(client, theme.Theme{}) + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs(args) + runErr := cmd.ExecuteContext(context.Background()) + return stdout.String(), stderr.String(), runErr +} + +// TestEnvSetSops_DottedPathPassesTheGate proves the key gate BRANCHES. +// +// `agentgateway.llm_key_hermes` fails env.ValidKey, which forbids dots — so +// before the branch existed, every --sops invocation refused on the key before +// --sops was ever consulted. +// +// The command is expected to FAIL here, just for a later reason: these tests +// wire a fake sensitive runner, so the driver gets no real sops. Asserting +// err != nil and then that the message is not the key-pattern one is what +// keeps this from being vacuous — an earlier version returned early when err +// was nil, which would have passed on a tree where the gate refused +// everything for some unrelated reason. +func TestEnvSetSops_DottedPathPassesTheGate(t *testing.T) { + repo := sopsCLIFixture(t) + forceNonTTY(t) + + _, _, err := runEnvSet(t, repo, "set", "agentgateway.llm_key_hermes", "--sops") + if err == nil { + t.Fatal("command succeeded, want it to reach the driver and fail there (no real sops is wired)") + } + if strings.Contains(err.Error(), envKeyPattern) { + t.Errorf("error = %q, want the dotted path to pass the key gate rather than hit ValidKey", err.Error()) + } + // And it must have got past the target gate too, or this would be + // asserting only that some earlier check fired. + for _, earlier := range []string{"requires a target named one of", "does not create one", "--any-file does not apply"} { + if strings.Contains(err.Error(), earlier) { + t.Errorf("error = %q, want it to reach the driver rather than stop at the target gate", err.Error()) + } + } +} + +func TestEnvSetSops_Refusals(t *testing.T) { + const sentinel = "s3ntinel-VALUE-77x" + + cases := []struct { + name string + setup func(t *testing.T, repo string) + args []string + wantMsg string + }{ + { + name: "a hostile key shape", + args: []string{"set", "KEY=VALUE", "--sops"}, + wantMsg: "path segments must match", + }, + { + name: "a path with a shell metacharacter", + args: []string{"set", "block.key;rm -rf /", "--sops"}, + wantMsg: "path segments must match", + }, + { + name: "--any-file combined with --sops", + args: []string{"set", "block.key", "--sops", "--any-file"}, + wantMsg: "--any-file does not apply with --sops", + }, + { + name: "a target whose name is not a SOPS shape", + setup: func(t *testing.T, repo string) { + if err := os.WriteFile(filepath.Join(repo, "values.yaml"), []byte(sopsFixtureDoc), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + }, + args: []string{"set", "block.key", "--sops", "--file", "values.yaml"}, + wantMsg: "requires a target named one of", + }, + { + name: "a .env target", + args: []string{"set", "block.key", "--sops", "--file", ".env"}, + wantMsg: "requires a target named one of", + }, + { + name: "a missing target", + args: []string{"set", "block.key", "--sops", "--file", "secrets.prod.yaml"}, + wantMsg: "does not create one", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + repo := sopsCLIFixture(t) + if c.setup != nil { + c.setup(t, repo) + } + forceTTYWithPassword(t, sentinel, nil) + + stdout, stderr, err := runEnvSet(t, repo, c.args...) + if err == nil { + t.Fatalf("command succeeded, want a refusal\nstdout: %q", stdout) + } + if !strings.Contains(err.Error(), c.wantMsg) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.wantMsg) + } + assertNoSecretInOutput(t, sentinel, stdout, stderr+err.Error()) + + // The encrypted fixture must be untouched by any refusal. + got, readErr := os.ReadFile(filepath.Join(repo, "secrets.sops.yaml")) //nolint:gosec // G304: a fixture this test created + if readErr != nil { + t.Fatalf("ReadFile: %v", readErr) + } + if string(got) != sopsFixtureDoc { + t.Error("the encrypted fixture changed on a refusal") + } + }) + } +} + +// TestEnvSetSops_HostileKeyRefusesBeforeReadingInput pins the ordering the +// .env route already defends: a refusable key must not consume stdin, or a +// secret piped in is read (and held in memory) for a command that was never +// going to run. +func TestEnvSetSops_HostileKeyRefusesBeforeReadingInput(t *testing.T) { + repo := sopsCLIFixture(t) + forceNonTTY(t) + t.Chdir(repo) + + client, _ := envFixture() + cmd := newEnvTestCmd(client, theme.Theme{}) + cmd.SetOut(new(bytes.Buffer)) + cmd.SetErr(new(bytes.Buffer)) + // A reader that records whether it was consulted at all. + probe := &readProbe{data: "s3ntinel-VALUE-77x"} + cmd.SetIn(probe) + cmd.SetArgs([]string{"set", "not a valid path", "--sops"}) + + if err := cmd.ExecuteContext(context.Background()); err == nil { + t.Fatal("command succeeded, want a refusal") + } + if probe.reads != 0 { + t.Errorf("stdin was read %d time(s) before the key refusal, want 0", probe.reads) + } +} + +// readProbe counts Read calls so a test can assert stdin was never consulted. +type readProbe struct { + data string + reads int + pos int +} + +func (r *readProbe) Read(p []byte) (int, error) { + r.reads++ + if r.pos >= len(r.data) { + return 0, os.ErrClosed + } + n := copy(p, r.data[r.pos:]) + r.pos += n + return n, nil +} + +// TestEnvSetSops_ClipboardRouteReadsTheClipboardDirectly pins the wiring the +// plan called out as easy to get wrong: `--sops --clipboard` must NOT go +// through env.Client.SetFromClipboard, which runs the whole .env pipeline and +// would append a plaintext `KEY=value` line to an encrypted file. +// +// The assertion is that the clipboard was PASTED FROM and the encrypted file +// is unchanged. The command then fails, because these tests wire a fake +// sensitive runner rather than a real sops — which is the point: the paste has +// already happened by then, so a route that never pasted would fail this test +// while a route that appended a plaintext line would fail the file check. +func TestEnvSetSops_ClipboardRouteReadsTheClipboardDirectly(t *testing.T) { + repo := sopsCLIFixture(t) + forceNonTTY(t) + t.Chdir(repo) + + const sentinel = "s3ntinel-VALUE-77x" + client, _ := envFixture() + cmd, clipFake := newEnvTestCmdWithClip(client, theme.Theme{}) + clipFake.RunFunc = func(name string, _ []string) (string, error) { + if name == "pbpaste" { + return sentinel, nil + } + return "", nil + } + + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"set", "agentgateway.from_clipboard", "--sops", "--clipboard"}) + err := cmd.ExecuteContext(context.Background()) + + pasted := false + for _, call := range clipFake.Calls { + if call.Name == "pbpaste" { + pasted = true + } + } + if !pasted { + t.Error("the clipboard was never pasted from on the --sops --clipboard route") + } + + // Whatever happened next, the encrypted file must not have gained a + // plaintext line. + got, readErr := os.ReadFile(filepath.Join(repo, "secrets.sops.yaml")) //nolint:gosec // G304: a fixture this test created + if readErr != nil { + t.Fatalf("ReadFile: %v", readErr) + } + if string(got) != sopsFixtureDoc { + t.Errorf("the encrypted file changed: %q", got) + } + if strings.Contains(string(got), sentinel) { + t.Error("the clipboard value was written into the encrypted file in plaintext") + } + + errText := "" + if err != nil { + errText = err.Error() + } + assertNoSecretInOutput(t, sentinel, stdout.String(), stderr.String()+errText) +} + +func TestSopsEdit_RefusesWithoutTheProtocol(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T) (workdir string) + want string + }{ + { + name: "no work directory in the environment", + setup: func(t *testing.T) string { return "" }, + want: "invoked by forgectl", + }, + { + name: "no nonce file", + setup: func(t *testing.T) string { + dir := sopsWorkdirFixture(t) + t.Setenv(sopsNonceEnv, "whatever") + return dir + }, + want: "invoked by forgectl", + }, + { + name: "a nonce mismatch", + setup: func(t *testing.T) string { + dir := sopsWorkdirFixture(t) + if err := os.WriteFile(filepath.Join(dir, sopsNonceFile), []byte("the-real-nonce"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + t.Setenv(sopsNonceEnv, "a-guess") + return dir + }, + want: "invoked by forgectl", + }, + { + name: "an empty nonce in the environment", + setup: func(t *testing.T) string { + dir := sopsWorkdirFixture(t) + if err := os.WriteFile(filepath.Join(dir, sopsNonceFile), []byte("the-real-nonce"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + t.Setenv(sopsNonceEnv, "") + return dir + }, + want: "invoked by forgectl", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + doc := filepath.Join(t.TempDir(), "doc.yaml") + if err := os.WriteFile(doc, []byte("block:\n k: 'v'\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + original, err := os.ReadFile(doc) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + + workdir := c.setup(t) + t.Setenv(sopsWorkdirEnv, workdir) + t.Setenv(sopsPathEnv, "block.k") + + if err := runSopsEdit(doc); err == nil { + t.Fatal("runSopsEdit succeeded, want a refusal") + } else if !strings.Contains(err.Error(), c.want) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.want) + } + + after, err := os.ReadFile(doc) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !bytes.Equal(original, after) { + t.Error("the document was modified despite the refusal") + } + }) + } +} + +// TestSopsEdit_OnceOnly proves the counter terminates a loop: whatever drives +// a second invocation in one run, the second refuses. +func TestSopsEdit_OnceOnly(t *testing.T) { + workdir := sopsWorkdirFixture(t) + const nonce = "a-test-nonce" + if err := os.WriteFile(filepath.Join(workdir, sopsNonceFile), []byte(nonce), 0o600); err != nil { + t.Fatalf("WriteFile nonce: %v", err) + } + if err := os.WriteFile(filepath.Join(workdir, sopsValueFile), []byte("a-value"), 0o600); err != nil { + t.Fatalf("WriteFile value: %v", err) + } + t.Setenv(sopsWorkdirEnv, workdir) + t.Setenv(sopsNonceEnv, nonce) + t.Setenv(sopsPathEnv, "block.k") + + doc := filepath.Join(t.TempDir(), "doc.yaml") + if err := os.WriteFile(doc, []byte("block:\n k: 'old'\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if err := runSopsEdit(doc); err != nil { + t.Fatalf("the first invocation failed: %v", err) + } + edited, err := os.ReadFile(doc) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(edited), "k: 'a-value'") { + t.Errorf("the first invocation did not write the value: %q", edited) + } + + if err := runSopsEdit(doc); err == nil { + t.Fatal("the second invocation succeeded, want a refusal") + } else if !strings.Contains(err.Error(), "more than once") { + t.Errorf("error = %q, want it to name the once-only rule", err.Error()) + } +} + +// TestSopsEdit_RecordsTheOutcome pins the relay the driver depends on for the +// added-versus-replaced distinction. +func TestSopsEdit_RecordsTheOutcome(t *testing.T) { + workdir := sopsWorkdirFixture(t) + const nonce = "a-test-nonce" + if err := os.WriteFile(filepath.Join(workdir, sopsNonceFile), []byte(nonce), 0o600); err != nil { + t.Fatalf("WriteFile nonce: %v", err) + } + if err := os.WriteFile(filepath.Join(workdir, sopsValueFile), []byte("a-value"), 0o600); err != nil { + t.Fatalf("WriteFile value: %v", err) + } + t.Setenv(sopsWorkdirEnv, workdir) + t.Setenv(sopsNonceEnv, nonce) + t.Setenv(sopsPathEnv, "block.added") + + doc := filepath.Join(t.TempDir(), "doc.yaml") + if err := os.WriteFile(doc, []byte("block:\n existing: 'x'\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := runSopsEdit(doc); err != nil { + t.Fatalf("runSopsEdit: %v", err) + } + + recorded, err := os.ReadFile(filepath.Join(workdir, sopsResultFile)) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile result: %v", err) + } + if got := strings.TrimSpace(string(recorded)); got != "added" { + t.Errorf("recorded outcome = %q, want %q", got, "added") + } +} diff --git a/internal/cli/env_test.go b/internal/cli/env_test.go index 92a3f1d2..ab13b3cb 100644 --- a/internal/cli/env_test.go +++ b/internal/cli/env_test.go @@ -66,7 +66,10 @@ import ( clippkg "github.com/cameronsjo/forgectl/internal/clip" envpkg "github.com/cameronsjo/forgectl/internal/env" "github.com/cameronsjo/forgectl/internal/exec" + "github.com/spf13/cobra" + "github.com/cameronsjo/forgectl/internal/module" + sopspkg "github.com/cameronsjo/forgectl/internal/sops" "github.com/cameronsjo/forgectl/internal/theme" ) @@ -88,6 +91,41 @@ func envFixture() (*envpkg.Client, *exec.FakeRunner) { return client, fake } +// newEnvTestCmd builds the env command tree, supplying the two collaborators +// the --sops route needs. +// +// Both are real objects over fakes rather than nil. A nil sops client would +// make any test that accidentally reached the --sops route panic, and a panic +// says "this test is broken" where a failed assertion says "this code is +// wrong" — the distinction matters for the 42 tests here that are about the +// .env route and should never touch sops at all. +func newEnvTestCmd(client *envpkg.Client, th theme.Theme) *cobra.Command { + cmd, _ := newEnvTestCmdWithClip(client, th) + return cmd +} + +// newEnvTestCmdWithClip is newEnvTestCmd plus a handle on the clipboard's own +// FakeRunner, for the tests that drive the --sops route's clipboard source. +// That route reads the clipboard DIRECTLY rather than through +// env.Client.SetFromClipboard, because SetFromClipboard runs the whole .env +// pipeline and would append a plaintext KEY=value line to an encrypted file — +// so it needs its own reachable fake. +func newEnvTestCmdWithClip(client *envpkg.Client, th theme.Theme) (*cobra.Command, *exec.FakeRunner) { + clipFake := &exec.FakeRunner{} + cmd := newEnvCmdForClient( + client, + sopspkg.NewClient(&exec.FakeSensitiveRunner{}), + // WithSensitive() matches what newEnvCmd builds in production. Without + // it the clipboard layer logs the pasted byte count, and a length is + // itself signal about a secret — it distinguishes key types and tracks + // rotations, which is the same reason `redact` masks to a fixed ****. + // A fixture that omits it could not catch a regression that dropped it. + clippkg.New(clipFake, clippkg.WithGOOS("darwin"), clippkg.WithSensitive()), + th, + ) + return cmd, clipFake +} + // forceNonTTY overrides the isTerminal seam to false (the piped-stdin // branch) for the duration of the test, restoring it via t.Cleanup. func forceNonTTY(t *testing.T) { @@ -145,7 +183,7 @@ func TestEnvKeysCmd_NamesOnly(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -168,7 +206,7 @@ func TestEnvKeysCmd_SkipsMalformedNote(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -194,7 +232,7 @@ func TestEnvKeysCmd_EmptyFile_EmptyStdout(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -214,7 +252,7 @@ func TestEnvKeysCmd_MissingFile_Errors(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"keys"}) @@ -235,7 +273,7 @@ func TestEnvSetCmd_FromPipedStdin(t *testing.T) { const sentinel = "s3ntinel-VALUE-77x" client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetIn(strings.NewReader(sentinel + "\n")) cmd.SetOut(&stdout) @@ -269,7 +307,7 @@ func TestEnvSetCmd_StripsTrailingNewline(t *testing.T) { forceNonTTY(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader(input)) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -302,7 +340,7 @@ func TestEnvSetCmd_Clipboard(t *testing.T) { } return "", nil } - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"set", "KEY", "--clipboard"}) @@ -333,7 +371,7 @@ func TestEnvSetCmd_ClipboardWinsOverPipedStdin(t *testing.T) { } return "", nil } - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("from-stdin-value\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -359,7 +397,7 @@ func TestEnvSetCmd_TTYPrompt_ViaSeam(t *testing.T) { forceTTYWithPassword(t, sentinel, nil) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -400,7 +438,7 @@ func TestEnvSetCmd_NewFile_0600(t *testing.T) { forceNonTTY(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("value1\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -425,7 +463,7 @@ func TestEnvSetCmd_EmptyStdin_Refused(t *testing.T) { forceNonTTY(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -450,7 +488,7 @@ func TestEnvSetCmd_HostileArgvKey_RefusedNoArgumentEcho(t *testing.T) { hostileKey := "KEY=" + hostileValue client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) // Even with stdin piped, the key check must fire before it's read. cmd.SetIn(strings.NewReader("unrelated\n")) var stdout, stderr bytes.Buffer @@ -484,7 +522,7 @@ func TestEnvSetCmd_DuplicateKey_Refused(t *testing.T) { forceNonTTY(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("3\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -509,7 +547,7 @@ func TestEnvSetCmd_EmptyStdin_KeyShapedSecretArg_NoTokenEcho(t *testing.T) { const keyShapedSecret = "SEKRIT_valuelikelooking_ab12cd34" client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("")) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) @@ -574,7 +612,7 @@ func TestEnvGetCmd_Clipboard_ConfirmationOnly(t *testing.T) { slogBuf := captureSlog(t) client, fake := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -603,7 +641,7 @@ func TestEnvGetCmd_RequiresClipboard(t *testing.T) { t.Chdir(repo) client, fake := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -629,7 +667,7 @@ func TestEnvGetCmd_MissingKey_Errors(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"get", "MISSING", "--clipboard"}) @@ -646,7 +684,7 @@ func TestEnvGetCmd_HostileArgvValue_RefusedNoArgumentEcho(t *testing.T) { const hostileValue = "SENTINEL_should_never_appear!!" client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -679,7 +717,7 @@ func TestEnvGetCmd_KeyShapedSecret_RefusedNoArgumentEcho(t *testing.T) { const keyShapedSecret = "sk_live_S3NTINEL_valid_key_shape" client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -709,7 +747,7 @@ func TestEnvCheckCmd_NoDrift_ExitZero(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -742,7 +780,7 @@ func TestEnvCheckCmd_ExtraOnly_PrintsOnlyExtraSection(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -777,7 +815,7 @@ func TestEnvCheckCmd_MissingKey_ExitOne(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -807,7 +845,7 @@ func TestEnvCheckCmd_ExtraKey_ReportedExitOne(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -836,7 +874,7 @@ func TestEnvCheckCmd_MissingExampleFile_ExitTwo(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"check"}) @@ -867,7 +905,7 @@ func TestEnvCheckCmd_MissingFile_ExitTwo(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"check"}) @@ -898,7 +936,7 @@ func TestEnvCheckCmd_JSON_MissingFile_OneStderrObject_ExitTwo(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -946,7 +984,7 @@ func TestEnvCheckCmd_JSON_MissingExampleFile_OneStderrObject_ExitTwo(t *testing. t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -984,7 +1022,7 @@ func TestEnvCheckCmd_JSON_Clean_EmptyArraysNotNull(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -1024,7 +1062,7 @@ func TestEnvCheckCmd_JSON_Drift_ReportsNamesAndExitsOne(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -1068,7 +1106,7 @@ func TestEnvCheckCmd_FileAndExampleFlagsCompose(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"check", "--file", ".env.prod", "--example", ".env.example"}) @@ -1091,7 +1129,7 @@ func TestEnvRedactCmd_MasksValues(t *testing.T) { slogBuf := captureSlog(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout, stderr bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(&stderr) @@ -1124,7 +1162,7 @@ func TestEnvRedactCmd_MultilinePEM_NoBodyLine(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -1145,7 +1183,7 @@ func TestEnvRedactCmd_MissingFile_Errors(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"redact"}) @@ -1183,7 +1221,7 @@ func TestEnvCmds_NonEnvFile_Refused(t *testing.T) { forceNonTTY(t) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("payload\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -1215,7 +1253,7 @@ func TestEnvKeysCmd_EnvShapedNames_Accepted(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) var stdout bytes.Buffer cmd.SetOut(&stdout) cmd.SetErr(new(bytes.Buffer)) @@ -1243,7 +1281,7 @@ func TestEnvSetCmd_AnyFile_NonTTY_RefusedOutright(t *testing.T) { forceNonTTY(t) // isTerminal() == false — --any-file must refuse before ever prompting client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("value\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -1284,7 +1322,7 @@ func TestEnvSetCmd_AnyFile_TTYConfirmedYes_Allowed(t *testing.T) { t.Cleanup(func() { confirmAnyFile = prevConfirm }) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"set", "KEY", "--file", ".git/config", "--any-file"}) @@ -1320,7 +1358,7 @@ func TestEnvSetCmd_AnyFile_TTYConfirmedNo_Refused(t *testing.T) { t.Cleanup(func() { confirmAnyFile = prevConfirm }) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetIn(strings.NewReader("value1\n")) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) @@ -1363,7 +1401,7 @@ func TestEnvCheckCmd_AnyFile_ConfirmsBothFileAndExample(t *testing.T) { t.Cleanup(func() { confirmAnyFile = prevConfirm }) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"check", "--file", "file.cfg", "--example", "example.cfg", "--any-file"}) @@ -1519,7 +1557,7 @@ func TestEnvSetCmd_ConfirmedPathIsWrittenPath(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"set", "fsmonitor", "--file", "link", "--any-file"}) @@ -1634,7 +1672,7 @@ func TestEnvSetCmd_ParentSwapDuringConfirmation(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"set", "fsmonitor", "--file", filepath.Join("sub", "config"), "--any-file"}) @@ -1686,7 +1724,7 @@ func TestEnvKeysCmd_OutsideRepo_Refused(t *testing.T) { t.Chdir(repo) client, _ := envFixture() - cmd := newEnvCmdForClient(client, theme.Theme{}) + cmd := newEnvTestCmd(client, theme.Theme{}) cmd.SetOut(new(bytes.Buffer)) cmd.SetErr(new(bytes.Buffer)) cmd.SetArgs([]string{"keys", "--file", "../outside/secret.env"}) diff --git a/internal/cli/root.go b/internal/cli/root.go index 438fd85f..e7afd271 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -147,5 +147,11 @@ arguments for a menu over every command group.`, // leaf verb outside the registry, alongside --version. root.AddCommand(newVersionCmd()) + // The editor `env set --sops` points sops at — forgectl re-invoking + // itself. Registered outside the registry because it is not a verb anyone + // runs: it is half of an internal protocol, and its own guards (not its + // Hidden flag) are what make it safe to expose at all. + root.AddCommand(newSopsEditCmd()) + return root } diff --git a/internal/cli/sops_edit.go b/internal/cli/sops_edit.go new file mode 100644 index 00000000..9656910d --- /dev/null +++ b/internal/cli/sops_edit.go @@ -0,0 +1,288 @@ +package cli + +import ( + "crypto/subtle" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + "gopkg.in/yaml.v3" + + execpkg "github.com/cameronsjo/forgectl/internal/exec" + sopspkg "github.com/cameronsjo/forgectl/internal/sops" +) + +// The fixed file names inside the work directory. Only the directory's path +// travels in the child's environment; every file within it is at a name both +// sides already know, so there is nothing an attacker could redirect by +// setting a variable. +const ( + sopsNonceFile = "nonce" + sopsValueFile = "value" + sopsResultFile = "result" + sopsCountFile = "count" +) + +// Environment variables the driver sets and this command reads, aliased from +// internal/exec rather than re-spelled. They were literals in both places +// once, and a rename on either side compiled clean, passed every unit test, +// and broke only the real subprocess. +const ( + sopsWorkdirEnv = execpkg.EnvSopsWorkdir + sopsPathEnv = execpkg.EnvSopsPath + sopsNonceEnv = execpkg.EnvSopsNonce +) + +// newSopsEditCmd builds the hidden `__sops-edit` subcommand: the editor sops +// invokes, which is forgectl re-invoking itself. +// +// # Why an editor at all +// +// `sops set file '["a"]["b"]' '"value"'` puts the plaintext in argv, visible +// in `ps` and left in shell history — the exposure this feature exists to +// close. `sops ` instead decrypts to a temp file, runs $EDITOR on it, +// and re-encrypts whatever comes back. So the value can travel by file while +// the key path travels by environment, and no process ever takes the secret +// as an argument. +// +// # Three guards, and what each one actually bounds +// +// 1. A nonce, checked against a file in the work directory. This bounds a +// STRAY invocation — sops re-running the editor after a run's files are +// gone, a replay from a stale environment, a hand-typed invocation that +// forgot the protocol. It is deliberately NOT described as a privilege +// boundary, because it is not one: a caller who can set this process's +// environment can also create the directory and nonce file it names, so +// the nonce buys nothing against them. Nor does it need to — a caller who +// can exec forgectl can already write YAML with a shell, so this +// subcommand grants no capability its invoker lacked. `Hidden: true` is +// presentation, not a control, for the same reason. +// 2. Output validation. It parses its own result and exits non-zero rather +// than handing sops a document sops cannot read. This one IS load-bearing: +// it is what keeps sops out of its unbounded editor re-invocation loop +// (measured on 3.13.3: 36,851 invocations and 8.4 MB of stderr in three +// minutes). A non-zero editor exit gives a clean rc=201 with the +// encrypted file byte-identical. +// 3. Once only. A counter file created O_EXCL means a second invocation in +// one run refuses, so any loop that does start terminates on its first +// retry — a second brake on the same failure as guard 2, arriving from a +// different direction. +func newSopsEditCmd() *cobra.Command { + return &cobra.Command{ + Use: "__sops-edit FILE", + Short: "Internal: the editor sops invokes; not for direct use", + Hidden: true, + Args: cobra.ExactArgs(1), + // The parent's error rendering is fine, but usage on failure would + // put this command's spelling in front of a user who never typed it. + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + err := runSopsEdit(args[0]) + // Relay the reason to the driver, which cannot read it any other + // way: this process's stderr is sops' stderr, and the driver + // refuses to surface that stream because a sops parse error + // quotes the line holding the value. + // + // Only THESE messages are relayed, and that is what makes it safe. + // Every one of them originates in forgectl and names a rule, never + // an argument or a value — a property the sops package's own tests + // assert. Without the relay, a typo'd block name (the commonest + // mistake there is) reaches the operator as "sops refused the + // edit", with the actionable part in a temp file. + recordSopsError(err) + return err + }, + } +} + +// sopsErrorFile is where the editor leaves its refusal for the driver. +const sopsErrorFile = "error" + +// workdirPrefix is the basename prefix the driver's work directory carries. +const workdirPrefix = ".forgectl-sops-" + +// resolveWorkdir reads and constrains the work directory. +// +// The constraints are narrow but real: absolute, and a basename the driver +// itself generates. They do not make the environment a trust boundary — a +// caller who can set it can create a matching directory — and they are not +// claimed to. What they do is stop this subcommand from being pointed at an +// arbitrary EXISTING directory, so a stray or replayed invocation fails +// loudly instead of reading four files out of somewhere unrelated. +func resolveWorkdir() (string, error) { + raw := os.Getenv(sopsWorkdirEnv) + if raw == "" { + return "", errors.New("this command is invoked by forgectl, not directly") + } + clean := filepath.Clean(raw) + if !filepath.IsAbs(clean) || !strings.HasPrefix(filepath.Base(clean), workdirPrefix) { + return "", errors.New("this command is invoked by forgectl, not directly") + } + return clean, nil +} + +// recordSopsError writes err's message into the work directory. Failures here +// are dropped deliberately: the relay is a diagnostic improvement, and losing +// it must not change the outcome the operator sees. +func recordSopsError(err error) { + if err == nil { + return + } + // Through the same constraint as the read path, not a raw Getenv: two + // notions of "the work directory" in one file is how they drift apart. + workdir, wdErr := resolveWorkdir() + if wdErr != nil { + return + } + _ = os.WriteFile(filepath.Join(workdir, sopsErrorFile), []byte(err.Error()), 0o600) //nolint:gosec // G304/G703: a fixed filename under the directory resolveWorkdir already constrained; the environment is not a trust boundary here (see newSopsEditCmd guard 1) +} + +// runSopsEdit performs the one edit, in the order the guards require: prove +// the caller is us, claim the single invocation, then touch the document. +func runSopsEdit(tempPath string) error { + workdir, err := resolveWorkdir() + if err != nil { + return err + } + + if err := checkSopsNonce(workdir); err != nil { + return err + } + // Claimed BEFORE the edit, not after. A counter incremented on success + // would let a failing edit be retried forever, which is the loop the + // counter exists to stop. + if err := claimSopsInvocation(workdir); err != nil { + return err + } + + path, err := sopspkg.ParsePath(os.Getenv(sopsPathEnv)) + if err != nil { + return err + } + + valueBytes, err := os.ReadFile(filepath.Join(workdir, sopsValueFile)) //nolint:gosec // G304/G703: a fixed filename under the directory resolveWorkdir already constrained; the environment is not a trust boundary here (see newSopsEditCmd guard 1) + if err != nil { + return errors.New("the value file is unreadable") + } + value, err := sopspkg.NormalizeValue(string(valueBytes)) + if err != nil { + return err + } + + doc, err := readEditorTarget(tempPath) + if err != nil { + return err + } + + edited, outcome, err := sopspkg.SetScalar(doc, path, value) + if err != nil { + return err + } + + // Parse what we are about to hand back. sops answers an unparseable + // document by re-invoking its editor, forever; refusing here converts + // that into one clean failure with the encrypted file untouched. + var probe map[string]any + if err := yaml.Unmarshal(edited, &probe); err != nil { + return errors.New("the edited document does not parse as YAML; refusing to hand it back") + } + + // The result is recorded before the document is written, so the driver + // can distinguish "edited and reported" from "wrote something and died". + if err := os.WriteFile(filepath.Join(workdir, sopsResultFile), []byte(outcome.String()), 0o600); err != nil { //nolint:gosec // G304/G703: a fixed filename under the directory resolveWorkdir already constrained; the environment is not a trust boundary here (see newSopsEditCmd guard 1) + return errors.New("could not record the outcome") + } + + // 0600 is the mode sops created its temp file as, restated so a NEW file + // would get it too. It is not a control over the existing file: os.WriteFile + // applies a mode only at creation, so the target keeps whatever permissions + // it already had. An earlier version of this comment claimed it prevented a + // umask from widening the file, which it does not do. + if err := os.WriteFile(tempPath, edited, 0o600); err != nil { //nolint:gosec // G306: 0600, the mode sops itself used + return errors.New("could not write the edited document") + } + return nil +} + +// readEditorTarget reads the document sops handed us, refusing a target this +// command has no business writing. +// +// # Why this exists even though the nonce is not a boundary +// +// It closes a containment ASYMMETRY rather than a privilege one. Every other +// write in forgectl goes through env.ResolveTarget, a pinned directory +// descriptor, and a symlink refusal. This one wrote wherever argv[1] pointed, +// following symlinks — demonstrated writing through a symlink into a file +// outside any repository, with no sops involvement, by replaying a work +// directory that a killed run had left on disk. +// +// That is not an escalation: reading the leftover nonce needs the same uid, +// and the same uid can write YAML with a shell. What it was is the one write +// in forgectl with no containment at all, which turns a leftover directory +// from "a secret at rest" into "a live capability at rest". Two checks fix the +// asymmetry: refuse a symlink, and require the target to already be a +// mapping-shaped YAML document — sops' decrypted buffer always is, and an +// arbitrary file someone aimed us at very often is not. +// +//nolint:gosec // G703: the path is argv from sops; the Lstat, symlink, regular-file and YAML-shape checks below ARE the containment, and there is no canonical location to compare it against — sops chooses its own temp path. +func readEditorTarget(path string) ([]byte, error) { + info, err := os.Lstat(path) + if err != nil { + return nil, errors.New("the document sops provided is unreadable") + } + if info.Mode()&os.ModeSymlink != 0 { + return nil, errors.New("the document to edit is a symlink; refusing") + } + if !info.Mode().IsRegular() { + return nil, errors.New("the document to edit is not a regular file; refusing") + } + + doc, err := os.ReadFile(path) //nolint:gosec // G304: the path sops passed as its editor argument, Lstat-checked immediately above + if err != nil { + return nil, errors.New("the document sops provided is unreadable") + } + + // A mapping at the root is what sops' decrypted buffer always is, and what + // SetScalar needs in order to mean anything. + var probe map[string]any + if err := yaml.Unmarshal(doc, &probe); err != nil || probe == nil { + return nil, errors.New("the document to edit is not a YAML mapping; refusing") + } + return doc, nil +} + +// checkSopsNonce requires the environment's nonce to equal the one in the +// work directory. +// +// subtle.ConstantTimeCompare is not about timing — see the type comment for +// why this is not a privilege boundary — it is about not writing a comparison +// a later reader has to reason about. The refusal names neither value. +// +//nolint:gosec // G703/G304: workdir comes from this process's own environment, which is not a trust boundary; see newSopsEditCmd's guard 1. +func checkSopsNonce(workdir string) error { + want, err := os.ReadFile(filepath.Join(workdir, sopsNonceFile)) //nolint:gosec // G304/G703: a fixed filename under the directory resolveWorkdir already constrained; the environment is not a trust boundary here (see newSopsEditCmd guard 1) + if err != nil { + return errors.New("this command is invoked by forgectl, not directly") + } + got := os.Getenv(sopsNonceEnv) + if len(got) == 0 || subtle.ConstantTimeCompare([]byte(got), want) != 1 { + return errors.New("this command is invoked by forgectl, not directly") + } + return nil +} + +// claimSopsInvocation creates the counter file exclusively, so exactly one +// invocation per run can proceed. +func claimSopsInvocation(workdir string) error { + f, err := os.OpenFile(filepath.Join(workdir, sopsCountFile), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) //nolint:gosec // G304/G703: a fixed filename under the directory resolveWorkdir already constrained; the environment is not a trust boundary here (see newSopsEditCmd guard 1) + if err != nil { + if os.IsExist(err) { + return errors.New("the editor was invoked more than once in one run; refusing") + } + return fmt.Errorf("could not claim the invocation: %w", err) + } + return f.Close() +} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 8110b172..76e98e7b 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -19,6 +19,7 @@ import ( "os" osexec "os/exec" "path/filepath" + "strings" "github.com/cameronsjo/forgectl/internal/bench" "github.com/cameronsjo/forgectl/internal/bless" @@ -116,6 +117,7 @@ func Run(ctx context.Context, d Deps) Report { checks = append(checks, checkBinary(d, "tmux", "tmux not found on PATH — install with `brew install tmux`")) checks = append(checks, checkBinary(d, "ghostty", "ghostty not found on PATH — install from https://ghostty.org")) checks = append(checks, checkBinary(d, "cmux", "cmux not found on PATH — see https://github.com/cameronsjo/cmux")) + checks = append(checks, checkSops(ctx, d)) checks = append(checks, checkGh(ctx, d)) checks = append(checks, benchChecks(ctx, d)...) checks = append(checks, checkTrustStore(d)) @@ -198,6 +200,41 @@ func checkGh(ctx context.Context, d Deps) Check { return Check{Name: "gh", State: StateOK, Detail: "authenticated"} } +// checkSops reports the sops VERSION, not merely its presence. +// +// `env set --sops` depends on behaviour that is version-specific and measured +// rather than documented: the exit status for an unchanged file, the absence of +// a trailing newline from `--extract --output`, and the editor re-invocation +// loop on an unparseable document. A doctor line that said only "found" would +// leave the one fact a future debugging session needs out of the report. +// +// A missing sops is StateSkip rather than StateFail: it is needed only for +// `env set --sops`, and a machine that never writes an encrypted secret is not +// unhealthy for lacking it. +func checkSops(ctx context.Context, d Deps) Check { + if _, err := d.LookPath("sops"); err != nil { + return Check{ + Name: "sops", + State: StateSkip, + Detail: "not found on PATH — only needed for `forgectl env set --sops`", + Hint: "install with `brew install sops`", + } + } + out, err := d.Runner.Run(ctx, "sops", "--version", "--disable-version-check") + if err != nil { + return Check{Name: "sops", State: StateFail, Detail: err.Error(), Hint: "reinstall with `brew reinstall sops`"} + } + return Check{Name: "sops", State: StateOK, Detail: firstLine(out)} +} + +// firstLine trims a command's output to its first line. `sops --version` can +// append an update notice, and a multi-line Detail breaks the report's +// one-check-per-line shape. +func firstLine(s string) string { + line, _, _ := strings.Cut(s, "\n") + return strings.TrimSpace(line) +} + // benchChecks folds bench.Status's hearth and chronicle components into doctor // Checks, translating bench's own State vocabulary rather than re-probing // anything itself. diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 1cf3231c..5bbc69c7 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -168,6 +168,59 @@ func TestCheckGh(t *testing.T) { } } +func TestCheckSops(t *testing.T) { + // Absent is StateSkip, not StateFail: sops is needed only for + // `env set --sops`, and a machine that never writes an encrypted secret + // is not unhealthy for lacking it. StateFail would make `doctor` exit + // non-zero on every machine that does not use the feature. + d := Deps{LookPath: fakeLookPath()} + check := checkSops(context.Background(), d) + if check.State != StateSkip { + t.Errorf("sops absent: state = %q, want skip", check.State) + } + if check.Hint == "" { + t.Error("sops absent: hint is empty, want the install command") + } + + // Present but unrunnable. + fr := &exec.FakeRunner{RunFunc: func(_ string, _ []string) (string, error) { + return "", &exec.CommandError{Name: "sops", Stderr: "bad binary", Err: errors.New("exit status 1")} + }} + d = Deps{LookPath: fakeLookPath("sops"), Runner: fr} + if check := checkSops(context.Background(), d); check.State != StateFail || check.Hint == "" { + t.Errorf("sops unrunnable: state = %q, hint = %q; want fail with a hint", check.State, check.Hint) + } + + // Present and runnable: the Detail must carry the VERSION, not merely + // "found". The behaviour `env set --sops` is built on is version-specific + // and measured rather than documented — the editor re-invocation loop, the + // exit status for an unchanged file, the absent trailing newline from + // --extract — so the version is the one fact a later debugging session + // needs from this line. + fr = &exec.FakeRunner{RunFunc: func(_ string, _ []string) (string, error) { + return "sops 3.13.3 (latest)", nil + }} + d = Deps{LookPath: fakeLookPath("sops"), Runner: fr} + check = checkSops(context.Background(), d) + if check.State != StateOK { + t.Errorf("sops present: state = %q, want ok", check.State) + } + if !strings.Contains(check.Detail, "3.13.3") { + t.Errorf("detail = %q, want it to carry the version", check.Detail) + } + + // A multi-line answer is trimmed to its first line: `sops --version` can + // append an update notice, and a multi-line Detail breaks the report's + // one-check-per-line shape. + fr = &exec.FakeRunner{RunFunc: func(_ string, _ []string) (string, error) { + return "sops 3.13.3\nA new version is available!", nil + }} + d = Deps{LookPath: fakeLookPath("sops"), Runner: fr} + if check := checkSops(context.Background(), d); strings.Contains(check.Detail, "\n") { + t.Errorf("detail = %q, want a single line", check.Detail) + } +} + func TestFromBenchComponent(t *testing.T) { cases := []struct { in bench.State diff --git a/internal/env/dir_test.go b/internal/env/dir_test.go index ea0a29f9..4012d102 100644 --- a/internal/env/dir_test.go +++ b/internal/env/dir_test.go @@ -146,6 +146,94 @@ func TestDirPin_OpenRegular_OpensAnOrdinaryFile(t *testing.T) { } } +// TestDirPin_OpenLock_RefusesSwappedEntries covers the lock open specifically, +// rather than leaning on the openRegular table above. +// +// The reason it needs its own coverage is that withFileLock Lstats the lock +// name before calling openLock, so a reader can conclude the open is already +// protected. It is not: the Lstat's answer is stale by the time the open runs, +// and O_NOFOLLOW closes that window for a symlink ONLY. Asking openLock +// directly — with the entry already in place, exactly as a swap inside the +// window would leave it — is what tests the descriptor-side check rather than +// the caller's. +// +// What it costs to lose: flock locks an open file description, so two writers +// on two FIFO inodes both believe they hold the lock and the parse→write +// section stops preventing a lost update. +func TestDirPin_OpenLock_RefusesSwappedEntries(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T, dir, name string) + want error + }{ + { + name: "fifo", + setup: func(t *testing.T, dir, name string) { + if err := syscall.Mkfifo(filepath.Join(dir, name), 0o600); err != nil { + t.Skipf("Mkfifo unsupported: %v", err) + } + }, + want: errNotRegular, + }, + { + name: "symlink", + setup: func(t *testing.T, dir, name string) { + outside := filepath.Join(t.TempDir(), "victim") + if err := os.Symlink(outside, filepath.Join(dir, name)); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + }, + want: errIsSymlink, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + dir := t.TempDir() + pin, err := pinDir(dir) + if err != nil { + t.Fatalf("pinDir: %v", err) + } + t.Cleanup(pin.close) + c.setup(t, dir, ".env.lock") + + f, err := pin.openLock(".env.lock") + if f != nil { + _ = f.Close() + t.Error("openLock returned a usable descriptor, want a refusal") + } + if !isSentinel(err, c.want) { + t.Errorf("err = %v, want %v", err, c.want) + } + }) + } +} + +// The green that makes the two refusals above mean something: a guard that +// refused every lock open would satisfy them both and break every write. +func TestDirPin_OpenLock_CreatesAnOrdinaryLockFile(t *testing.T) { + dir := t.TempDir() + pin, err := pinDir(dir) + if err != nil { + t.Fatalf("pinDir: %v", err) + } + t.Cleanup(pin.close) + + f, err := pin.openLock(".env.lock") + if err != nil { + t.Fatalf("openLock on a fresh name: %v", err) + } + defer func() { _ = f.Close() }() + + info, err := os.Lstat(filepath.Join(dir, ".env.lock")) + if err != nil { + t.Fatalf("Lstat: %v", err) + } + if !info.Mode().IsRegular() { + t.Errorf("lock file mode = %v, want a regular file", info.Mode()) + } +} + func TestDirPin_PinDir_RefusesASymlinkedDirectory(t *testing.T) { base := t.TempDir() // Not `real`: that shadows a Go predeclared identifier, which the diff --git a/internal/env/dir_unix.go b/internal/env/dir_unix.go index b5687de1..25bd16a8 100644 --- a/internal/env/dir_unix.go +++ b/internal/env/dir_unix.go @@ -155,21 +155,48 @@ func openatCreate(dirfd int, name string, flags int, perm uint32) (int, error) { } // openLock creates or opens name inside the pinned directory as a lock file, -// refusing a symlink. +// refusing a symlink and anything that is not a regular file. // -// A FIFO here does NOT block — O_RDWR on a FIFO succeeds immediately — so the -// caller's Lstat is what refuses one, not this open. That is worth stating -// because the opposite is the intuitive guess: the blocking case is the -// read-only open above. +// O_NONBLOCK and the post-open check are both here, and the flag is not +// redundant with the one above it. O_RDWR on a FIFO returned immediately when +// measured on darwin 25.5, but POSIX leaves O_RDWR on a FIFO UNDEFINED, and it +// permits a blocking open for a character device that supports non-blocking +// mode — so a measurement on one platform is not a guarantee, and a blocking +// open would stall before the check below ever runs. O_NONBLOCK has no effect +// on regular-file I/O, which is the only case that reaches the return, so it +// costs nothing to hold. +// +// The post-open check has to live HERE, not in the caller. withFileLock does Lstat the +// name first, and O_NOFOLLOW closes the gap between that Lstat and this open +// for a SYMLINK only: a swap to a FIFO in the same window is not a symlink, so +// O_NOFOLLOW has nothing to say about it and the Lstat's answer is already +// stale. The descriptor is the only thing that can be asked about the inode +// actually opened, so the question gets asked of the descriptor — the same +// pairing openRegular above uses, for the same reason. +// +// What it costs to omit: flock locks an open file description, so two writers +// each holding a lock on a different FIFO inode both believe they hold the +// lock, and the parse→write section that exists to prevent a lost update stops +// preventing one. func (d *dirPin) openLock(name string) (*os.File, error) { - fd, err := openatCreate(d.fd, name, unix.O_RDWR|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0o600) + fd, err := openatCreate(d.fd, name, unix.O_RDWR|unix.O_NOFOLLOW|unix.O_NONBLOCK|unix.O_CLOEXEC, 0o600) if errors.Is(err, unix.ELOOP) { return nil, errIsSymlink } if err != nil { return nil, err } - return os.NewFile(uintptr(fd), name), nil + f := os.NewFile(uintptr(fd), name) + info, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, err + } + if !info.Mode().IsRegular() { + _ = f.Close() + return nil, errNotRegular + } + return f, nil } // lstat reports name's permission bits and whether it is a regular file, diff --git a/internal/env/env.go b/internal/env/env.go index 394c9222..1f0de1fd 100644 --- a/internal/env/env.go +++ b/internal/env/env.go @@ -12,6 +12,7 @@ import ( "context" "errors" "fmt" + "io" "io/fs" "os" "strings" @@ -258,6 +259,54 @@ func OpenTarget(target Target) (*os.File, error) { return f, nil } +// ReadTarget reads the whole file through the pinned descriptor. +// +// Exported for internal/sops, which needs the raw bytes of a SOPS document to +// run its format checks against — and must run them against the bytes it +// read, never a re-open by name, or the final path component can be swapped +// between the check and the use. +func ReadTarget(target Target) ([]byte, error) { + f, err := OpenTarget(target) + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + data, err := io.ReadAll(f) + if err != nil { + return nil, fmt.Errorf("read %s: %w", target.Rel(), err) + } + return data, nil +} + +// WriteTarget writes data over the target atomically, through the pinned +// descriptor. Exported for internal/sops' restore path, so a restore has the +// same containment every other write here does. +// +// writeAtomic's `tightened` report is deliberately dropped: a restore puts +// back bytes that were already present, so a permission note about it would +// describe the state before the operation that failed. +func WriteTarget(target Target, data []byte) error { + if err := target.validate(); err != nil { + return err + } + _, err := writeAtomic(target, data) + return err +} + +// WithFileLock runs fn while holding the target's exclusive lock. +// +// Exported for internal/sops, which holds it across an entire sops subprocess +// so a concurrent writer cannot interleave with a decrypt-edit-encrypt cycle. +// That is a far longer hold than the .env path needs, and it is the point: the +// child re-resolves the path by name, so the lock is what bounds the window a +// pinned descriptor cannot reach across a process boundary. +func WithFileLock(target Target, fn func() error) error { + if err := target.validate(); err != nil { + return err + } + return withFileLock(target, fn) +} + // parseFile opens and parses target, refusing a symlink for the reason // loadOrEmpty states. // diff --git a/internal/env/locate.go b/internal/env/locate.go index fc706406..41db727a 100644 --- a/internal/env/locate.go +++ b/internal/env/locate.go @@ -79,6 +79,20 @@ func (t Target) Close() { t.dir.close() } +// Abs returns the absolute resolved path. +// +// It exists for one narrow purpose: handing a path to a CHILD PROCESS that +// must open the file itself. Nothing in this package uses it, and nothing in +// this package should — every operation here goes through the pinned +// descriptor, which is what makes the resolution meaningful. +// +// A child given this string re-resolves it with its own implementation, so the +// pin's guarantee does not extend across that boundary. The caller is +// responsible for the window: `env set --sops` closes it by holding the file +// lock across the child's whole lifetime and verifying the result afterwards, +// rather than by trusting the path. +func (t Target) Abs() string { return t.path } + // validate refuses a Target that did not come from ResolveTarget. Since every // field that matters is unexported, the only such value is a literal built // outside this package, and the missing descriptor is what gives it away. @@ -265,6 +279,29 @@ func ResolveTarget(fileFlag, cwd string) (Target, error) { return t, nil } +// RepoRoot returns the resolved (symlink-following) repository root for cwd. +// +// A caller holding a Target should render paths with Target.Rel instead; this +// exists for a caller that needs the root BEFORE it has a target — `env set +// --sops`, whose default file lives at the repository root rather than in the +// current directory, and which must therefore know the root in order to build +// the path it then resolves. +func RepoRoot(cwd string) (string, error) { + absCwd, err := filepath.Abs(cwd) + if err != nil { + return "", fmt.Errorf("resolve cwd: %w", err) + } + root, err := findRepoRoot(absCwd) + if err != nil { + return "", err + } + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return "", fmt.Errorf("resolve repository root: %w", err) + } + return realRoot, nil +} + // findRepoRoot walks up from start looking for a .git entry — a directory // for an ordinary repo, a file for a worktree (its .git is a "gitdir: …" // pointer file). No up-walk helper exists elsewhere in forgectl; this is diff --git a/internal/env/locate_test.go b/internal/env/locate_test.go index 662e71e8..8f30f041 100644 --- a/internal/env/locate_test.go +++ b/internal/env/locate_test.go @@ -50,6 +50,7 @@ func locate(fileFlag, cwd string) (Target, error) { return Target{}, err } if err := target.Clear(); err != nil { + target.Close() return Target{}, err } return target, nil @@ -59,12 +60,17 @@ func locate(fileFlag, cwd string) (Target, error) { // equivalent of what the CLI hands a Client method. Tests take this route // rather than building a Target literal so a fixture cannot assert against a // target production would have refused. +// A Target owns an open directory descriptor, so the fixture registers its +// close the same way production defers one. Without it a package-wide test run +// retains one descriptor per resolution until the binary exits, which is a +// leak a table test can multiply well past a default ulimit. func mustTarget(t *testing.T, fileFlag, cwd string) Target { t.Helper() target, err := locate(fileFlag, cwd) if err != nil { t.Fatalf("locate(%q): %v", fileFlag, err) } + t.Cleanup(target.Close) return target } @@ -376,6 +382,7 @@ func TestResolveTarget_NonEnvFile_ResolvesButDoesNotClear(t *testing.T) { if err != nil { t.Fatalf("ResolveTarget: %v", err) } + defer target.Close() if !target.Exists { t.Error("Exists = false, want true") } diff --git a/internal/env/lock_unix.go b/internal/env/lock_unix.go index a37852f4..6c7a6ec4 100644 --- a/internal/env/lock_unix.go +++ b/internal/env/lock_unix.go @@ -32,13 +32,17 @@ import ( // stores a symlink as mode 120000, so a hostile repo delivers that by being // cloned — no local step required. // -// Two checks, and which one fires is worth stating precisely because the -// intuitive guess is wrong. The Lstat is what refuses a non-regular entry, -// including a FIFO — and the reason is NOT that the open would block: an -// O_RDWR open on a FIFO returns immediately (measured). It is that locking and -// then writing through a FIFO is meaningless. O_NOFOLLOW refuses a symlink, -// including a dangling one, and closes the window between the Lstat and the -// open. +// Three checks, and which one fires is worth stating precisely because the +// intuitive guess is wrong. The Lstat below refuses a non-regular entry that +// is already there, and gives the operator the actionable message — but it is +// NOT what makes the refusal sound, because its answer is stale the moment it +// returns. O_NOFOLLOW refuses a symlink, including a dangling one, and closes +// the Lstat→open window for a symlink ONLY. A swap to a FIFO inside that same +// window is not a symlink, so nothing above catches it; openLock's own +// post-open check on the descriptor is what does. The reason to refuse a FIFO +// is not that the open would block — an O_RDWR open on one returns immediately +// (measured) — it is that locking and writing through it is meaningless, and +// two writers on two FIFO inodes would both think they held this lock. // // Containment needs no separate check here, and the one that used to be here // was worse than nothing: sandbox.WithinWorkspace falls back to the diff --git a/internal/exec/sensitive.go b/internal/exec/sensitive.go index 0fa90c05..074bc06c 100644 --- a/internal/exec/sensitive.go +++ b/internal/exec/sensitive.go @@ -105,6 +105,21 @@ const ( KindHerdrProbe KindHerdrCleanup + // KindSopsEdit drives `sops ` with forgectl re-invoked as the + // editor. KindSopsExtract is the read-back that proves what landed. + // + // These route through the sensitive seam rather than Runner for a reason + // the ordinary path cannot satisfy: sops' stderr quotes the offending + // line of the document it failed to parse, and that line is + // `key: ''`. Runner's runAndWrap logs stderr at Error level — + // which survives any configured log level and can be pointed at a file on + // disk — and retains it on *CommandError, which fang renders. Here, + // nothing logged or returned can render a payload, and both streams are + // capped so the measured 8.4 MB of sops re-invocation stderr cannot grow + // the heap. + KindSopsEdit + KindSopsExtract + kindCount ) @@ -131,6 +146,9 @@ var kindNames = [kindCount]string{ KindHerdrReconcile: "herdr.reconcile", KindHerdrProbe: "herdr.probe", KindHerdrCleanup: "herdr.cleanup", + + KindSopsEdit: "sops.edit", + KindSopsExtract: "sops.extract", } // Valid reports whether k names a real operation. The zero value does not. @@ -334,6 +352,32 @@ const ( envKeyCmuxQuiet = "CMUX_QUIET" envKeyHerdrConfig = "HERDR_CONFIG_PATH" envKeyTmux = "TMUX" + + // The sops editor protocol. Three variables, not five: the work directory + // is named once and the value file, the result file, the nonce file, and + // the invocation counter all sit at fixed names inside it. Every name + // added here is a name an attacker could try to set, so the smaller + // surface is the point. + // + // Note what is NOT here: the value. Its containing directory's path + // travels; the secret itself never enters an environment, which is + // readable from /proc on Linux for the lifetime of the process. + envKeySopsEditor = "EDITOR" +) + +// The sops editor protocol's variable names, EXPORTED so the reading side +// (internal/cli's `__sops-edit`) references these rather than keeping its own +// copies. +// +// They were spelled twice, in two packages, with a comment on the other side +// describing itself as a mirror. Renaming one side compiled clean, passed +// every unit test, and broke only the real subprocess — which is covered +// exclusively by gated integration tests. A shared constant prevents the +// drift; a test asserting two literals are equal would only have detected it. +const ( + EnvSopsWorkdir = "FORGECTL_SOPS_WORKDIR" + EnvSopsPath = "FORGECTL_SOPS_PATH" + EnvSopsNonce = "FORGECTL_SOPS_NONCE" ) type envOp uint8 @@ -391,6 +435,39 @@ func UnsetTmux() EnvMutation { return EnvMutation{key: envKeyTmux, op: envOpUnset} } +// ReplaceSopsEditor points sops' EDITOR at a command. sops shell-word-splits +// the value (quotes honoured) and appends the decrypted temp file as the only +// argument — measured on 3.13.3, including the self-exec case. +func ReplaceSopsEditor(command string) EnvMutation { + return EnvMutation{key: envKeySopsEditor, value: Secret(command), op: envOpReplace} +} + +// ReplaceSopsWorkdir names the private directory holding the value file, the +// nonce, the result, and the invocation counter. +func ReplaceSopsWorkdir(path string) EnvMutation { + return EnvMutation{key: EnvSopsWorkdir, value: Secret(path), op: envOpReplace} +} + +// ReplaceSopsPath carries the dotted key path the editor must write. +func ReplaceSopsPath(path string) EnvMutation { + return EnvMutation{key: EnvSopsPath, value: Secret(path), op: envOpReplace} +} + +// ReplaceSopsNonce carries the per-run nonce the editor checks against the +// copy in its work directory. +// +// It bounds a STRAY invocation — sops re-running the editor after a run's +// files are gone, a replay from a stale environment, a hand-typed call that +// forgot the protocol. It is NOT a privilege boundary, and an earlier version +// of this comment claimed it was: a caller who can set this process's +// environment can also create the directory and nonce file it names, so the +// nonce buys nothing against them. It does not need to, either — a caller who +// can exec forgectl can already write YAML with a shell. The full reasoning +// is on internal/cli's newSopsEditCmd, and this comment used to contradict it. +func ReplaceSopsNonce(nonce string) EnvMutation { + return EnvMutation{key: EnvSopsNonce, value: Secret(nonce), op: envOpReplace} +} + func (EnvMutation) String() string { return Redacted } func (EnvMutation) GoString() string { return Redacted } func (EnvMutation) Format(f fmt.State, verb rune) { writeRedacted(f, verb) } diff --git a/internal/sops/driver.go b/internal/sops/driver.go new file mode 100644 index 00000000..7bfd3e44 --- /dev/null +++ b/internal/sops/driver.go @@ -0,0 +1,573 @@ +package sops + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/sha256" + "encoding/base32" + "errors" + "fmt" + "os" + osexec "os/exec" + "path/filepath" + "strings" + "time" + + "gopkg.in/yaml.v3" + + "github.com/cameronsjo/forgectl/internal/env" + "github.com/cameronsjo/forgectl/internal/exec" +) + +// editDeadline bounds the `sops` edit call. +// +// It is a real control, not politeness. sops re-invokes its editor forever on +// a document it cannot parse, and while __sops-edit's own validation and +// once-only counter are the primary brakes, neither runs if sops fails to +// parse the file for a reason the editor never touched. Measured: 36,851 +// invocations in about three minutes, so a minute is generous for a +// single-key edit and still bounds the pathological case. +const editDeadline = 60 * time.Second + +// extractDeadline bounds the read-back. A decrypt of one value is fast; a KMS +// round-trip is the slow case worth allowing for. +const extractDeadline = 30 * time.Second + +// outputCap bounds each captured stream. The runner's own ceiling is 64KiB and +// this narrows it: nothing sops says on a successful edit is long, and the +// interesting failure produced megabytes. +const outputCap = 16 << 10 + +// nonceBytes is the per-run nonce's length. Twenty bytes of base32 is well +// past guessable for a value that lives for one subprocess. +const nonceBytes = 20 + +// Client writes one scalar into a SOPS file. +type Client struct { + runner exec.SensitiveRunner +} + +// NewClient builds a Client over the sensitive execution seam. +// +// It takes SensitiveRunner rather than Runner deliberately. sops' stderr +// quotes the offending line of a document it failed to parse, and that line is +// `key: ''`; Runner logs stderr at Error level, which survives any +// log-level setting and can be pointed at a file, and retains it on an error +// fang renders. This seam can render neither. +func NewClient(runner exec.SensitiveRunner) *Client { + return &Client{runner: runner} +} + +// SetValue writes value at path in the target SOPS file and proves it landed +// encrypted before reporting success. +// +// The sequence, and why each step is where it is: +// +// 1. Refuse if `sops` is absent, before anything else — a missing binary +// should not look like a file problem. +// 2. Validate the path and the value BEFORE touching the file or reading +// input, the same ordering internal/env's `set` defends. +// 3. Take the file lock. Everything below happens under it. +// 4. Read the file once, under the lock, and run the SOPS checks against +// THOSE BYTES — never a re-open by name. +// 5. Refuse a key the file's own rules would store in cleartext. +// 6. Create the work directory as a SIBLING of the target, never $TMPDIR: +// os.Rename across filesystems returns EXDEV, so a $TMPDIR backup would +// fail to restore in exactly the error paths it exists for, while the +// message claimed it succeeded. +// 7. Write the value to a 0600 file and the nonce beside it. +// 8. Run sops, interpreting three outcomes rather than two. +// 9. Read back and verify — including that the line is ciphertext. +// 10. Restore on any failure from step 8 on, and PROVE the restore. +func (c *Client) SetValue(ctx context.Context, target env.Target, rawPath, rawValue string) (Outcome, error) { + // A nil runner means nobody wired the sensitive seam. Refusing is the + // only safe answer: the obvious fallback is exec.Runner, and that is the + // seam whose stderr logging is the reason this package does not use it. + // Failing closed here keeps a wiring mistake from quietly becoming a + // secret in a log file. + if c == nil || c.runner == nil { + return OutcomeUnspecified, errors.New("the sops writer has no execution seam wired; this is a forgectl bug, please report it") + } + + sopsBin, err := osexec.LookPath("sops") + if err != nil { + return OutcomeUnspecified, errors.New("sops not found on PATH; install it with 'brew install sops'") + } + + segments, err := ParsePath(rawPath) + if err != nil { + return OutcomeUnspecified, err + } + value, err := NormalizeValue(rawValue) + if err != nil { + return OutcomeUnspecified, err + } + + var outcome Outcome + lockErr := env.WithFileLock(target, func() error { + var err error + outcome, err = c.setLocked(ctx, sopsBin, target, segments, value) + return err + }) + if lockErr != nil { + return OutcomeUnspecified, lockErr + } + return outcome, nil +} + +// setLocked is the body that runs while the file lock is held. +func (c *Client) setLocked(ctx context.Context, sopsBin string, target env.Target, segments []string, value string) (Outcome, error) { + before, err := env.ReadTarget(target) + if err != nil { + return OutcomeUnspecified, err + } + + // Both checks run against the bytes just read, not a re-open. Re-opening + // by name between the check and the use is how the final path component + // gets swapped underneath a decision. + if !IsSOPSFile(before) { + return OutcomeUnspecified, fmt.Errorf("refusing %s: it has no top-level sops: block, so it is not a SOPS document", target.Rel()) + } + rules, err := ReadPlaintextRules(before) + if err != nil { + return OutcomeUnspecified, fmt.Errorf("refusing %s: %w", target.Rel(), err) + } + // The WHOLE path, not the leaf: sops applies these rules to a key and its + // entire subtree, so an ancestor decides the outcome. See + // WouldStoreCleartext for the measurement. + if cleartext, reason := rules.WouldStoreCleartext(segments); cleartext { + // Names the rule and the file, never the path — the sops grammar + // admits plenty of real credential shapes, so a token pasted into the + // key slot reaches here. + return OutcomeUnspecified, fmt.Errorf("refusing to write into %s: %s, so the value would be stored in the clear", target.Rel(), reason) + } + + work, err := newWorkDir(target) + if err != nil { + return OutcomeUnspecified, err + } + defer work.cleanup() + + if err := work.stage(before, value); err != nil { + return OutcomeUnspecified, err + } + + editor, err := selfEditorCommand() + if err != nil { + return OutcomeUnspecified, err + } + + editCtx, cancel := context.WithTimeout(ctx, editDeadline) + defer cancel() + + res, runErr := c.runner.RunSensitive(editCtx, exec.SensitiveCommand{ + Kind: exec.KindSopsEdit, + Path: exec.Secret(sopsBin), + Args: []exec.Arg{exec.MustFixed("--disable-version-check"), exec.EndOfOptions(), exec.Opaque(target.Abs())}, + Env: []exec.EnvMutation{ + exec.ReplaceSopsEditor(editor), + exec.ReplaceSopsWorkdir(work.dir), + exec.ReplaceSopsPath(strings.Join(segments, ".")), + exec.ReplaceSopsNonce(work.nonce), + }, + StdoutCap: outputCap, + StderrCap: outputCap, + }) + + switch { + case runErr == nil: + // Proceed to verification. + case res.ExitCode == sopsUnchangedExit: + // sops exits 200 with "File has not changed, exiting." when the + // editor handed back identical bytes — which happens whenever the key + // already holds this value. That is success, not failure, and it is + // still verified below. Reporting it as an error would fail every + // idempotent re-run. + default: + if restoreErr := work.restore(target); restoreErr != nil { + return OutcomeUnspecified, fmt.Errorf("sops refused the edit and the file could NOT be restored: %w", restoreErr) + } + // The editor's own refusal, when there is one, is the actionable + // message — a typo'd block name is the commonest mistake and its + // reason lives only in the child. Every relayed message originates in + // forgectl and names a rule rather than an argument; sops' own output + // is still never surfaced. + if reason := work.readEditorError(); reason != "" { + return OutcomeUnspecified, fmt.Errorf("%s — %s is unchanged", reason, target.Rel()) + } + // Only NOW is sops' own output worth keeping, and only because there + // is nothing better to offer. It is captured here rather than + // unconditionally after the run: capturing on every path left a file + // in $TMPDIR that nothing deleted and nothing referenced, on every + // successful run that produced any output at all — including every + // idempotent re-set, which always prints "File has not changed". A + // file whose stated purpose is to hold output that may quote + // `key: ''` must not accumulate unreferenced copies. + logPath, logErr := work.captureOutput(res) + return OutcomeUnspecified, sopsRefusalError(target, logPath, logErr) + } + + // The staged plaintext has served its purpose the moment sops returns, so + // it goes now rather than at cleanup. The work directory is a sibling of + // the target and therefore INSIDE the repository, and there is no signal + // handler on this path — a Ctrl-C during a KMS round-trip skips the + // deferred cleanup and leaves `value` (0600, the secret verbatim) where + // `git add -A` will commit it. Measured: reproduced on the first of forty + // kill attempts. Shrinking the window to the span where the file must + // exist is most of the fix; a signal handler and a location outside the + // work tree are tracked separately. + work.discardStagedValue() + + outcome, err := c.verify(ctx, sopsBin, target, segments, value, work) + if err != nil { + if restoreErr := work.restore(target); restoreErr != nil { + return OutcomeUnspecified, fmt.Errorf("%w — and the file could NOT be restored: %v", err, restoreErr) + } + return OutcomeUnspecified, err + } + return outcome, nil +} + +// sopsUnchangedExit is sops' status for "File has not changed, exiting." +// Measured on 3.13.3, and confirmed to return immediately rather than hang. +const sopsUnchangedExit = 200 + +// sopsRefusalError is the fixed message for a sops failure. It names the log +// path and nothing else; see captureOutput for why. +func sopsRefusalError(target env.Target, logPath string, logErr error) error { + if logErr != nil || logPath == "" { + return fmt.Errorf("sops refused the edit — %s is unchanged", target.Rel()) + } + return fmt.Errorf("sops refused the edit — %s is unchanged; details in %s", target.Rel(), logPath) +} + +// verify proves the write landed, and landed ENCRYPTED. +// +// Three assertions in order, and the third is the one that matters most: a +// decrypt round-trip alone passes happily against a value stored in +// cleartext, which is precisely the failure step 5 exists to prevent. Checking +// that the re-read ciphertext line carries an ENC[ marker is what makes this +// verifier able to go red on that. +func (c *Client) verify(ctx context.Context, sopsBin string, target env.Target, segments []string, value string, work *workDir) (Outcome, error) { + landedPath := filepath.Join(work.dir, "landed") + + extractCtx, cancel := context.WithTimeout(ctx, extractDeadline) + defer cancel() + + _, runErr := c.runner.RunSensitive(extractCtx, exec.SensitiveCommand{ + Kind: exec.KindSopsExtract, + Path: exec.Secret(sopsBin), + Args: []exec.Arg{ + exec.MustFixed("--decrypt"), + exec.MustFixed("--disable-version-check"), + exec.MustFixed("--extract"), + exec.Opaque(JoinExtract(segments)), + exec.MustFixed("--output"), + exec.Opaque(landedPath), + exec.EndOfOptions(), + exec.Opaque(target.Abs()), + }, + StdoutCap: outputCap, + StderrCap: outputCap, + }) + // A verification command that FAILED TO RUN must never read as a clean + // result, so a non-zero exit here is a refusal rather than a skipped + // check. + if runErr != nil { + return OutcomeUnspecified, errors.New("the write could not be verified — the file has been restored") + } + + landed, err := os.ReadFile(landedPath) //nolint:gosec // G304: a path this process created inside its own 0700 work dir + // Removed as soon as it is read: it is a second plaintext copy of the + // secret, in a directory that sits inside the repository. + work.discardLandedValue() + if err != nil { + return OutcomeUnspecified, errors.New("the write could not be verified — the file has been restored") + } + // Byte-exact, with no trailing-newline strip: measured, `sops --decrypt + // --extract --output` writes a scalar with NO terminator (an 8-byte value + // produces an 8-byte file), so stripping one would mask a real + // single-byte corruption. + if len(landed) == 0 || !bytes.Equal(landed, []byte(value)) { + // No digests. A SHA-256 of the intended plaintext, printed into a + // transcript that gets committed, is offline-verifiable — for a + // password, a PIN, or any value from a guessable set it IS the value. + // It is also not actionable. + return OutcomeUnspecified, errors.New("the value that landed does not match what was supplied — the file has been restored") + } + + after, err := env.ReadTarget(target) + if err != nil { + return OutcomeUnspecified, err + } + if err := assertEncryptedAtPath(after, segments); err != nil { + return OutcomeUnspecified, err + } + + return work.readOutcome(), nil +} + +// assertEncryptedAtPath requires the scalar at exactly path in the re-read +// ciphertext to carry a sops ENC marker. +// +// # Why this resolves the path instead of scanning lines +// +// The first version scanned for a line whose trimmed text began with +// `leaf + ":"`, anywhere in the document, and that made the assertion +// DOCUMENT-ORDER DEPENDENT. A same-named key elsewhere that happened to be +// encrypted satisfied it. Proven by reordering one write: with an encrypted +// `app.token` above a cleartext `notes_unencrypted.token`, the scan matched +// app's line and the run reported success with the secret in plaintext; move +// the cleartext line first and the same write correctly went red. +// +// So the check that the design calls the one that matters most was the check +// that could not be made to go red on demand. It resolves the path properly +// now. The prefix match was sloppy too — `leaf+":"` also matches +// `token:anything`. +func assertEncryptedAtPath(doc []byte, path []string) error { + var root yaml.Node + if err := yaml.Unmarshal(doc, &root); err != nil { + return errors.New("the re-read file does not parse as YAML — the file has been restored") + } + + node := &root + if node.Kind == yaml.DocumentNode && len(node.Content) > 0 { + node = node.Content[0] + } + for _, segment := range path { + next := mappingValue(node, segment) + if next == nil { + return errors.New("the written key could not be found in the re-read file — the file has been restored") + } + node = next + } + + if node.Kind != yaml.ScalarNode { + return errors.New("the written key is not a scalar in the re-read file — the file has been restored") + } + if strings.HasPrefix(node.Value, "ENC[AES256_GCM,") { + return nil + } + // NOT ciphertext. Said without quoting the value, which by definition + // holds the plaintext this refusal exists to report. + return errors.New("the value was written in the clear rather than encrypted — the file has been restored") +} + +// mappingValue returns the value node for key in a mapping node, or nil. +// +// A yaml.v3 mapping stores Content as alternating key, value pairs, so this +// steps by two. Anything that is not a mapping has no keys to look up, which +// is a miss rather than an error — the caller reports the path as unfound. +func mappingValue(node *yaml.Node, key string) *yaml.Node { + if node == nil || node.Kind != yaml.MappingNode { + return nil + } + for i := 0; i+1 < len(node.Content); i += 2 { + if node.Content[i].Value == key { + return node.Content[i+1] + } + } + return nil +} + +// executablePath is a seam over os.Executable. +// +// It exists for the integration tests, which must point the editor at a +// forgectl built from the CURRENT tree rather than at the test binary +// os.Executable actually names — a test binary has no __sops-edit subcommand, +// so without this the tests could only exercise the driver against whatever +// forgectl happened to be installed, which is the wrong thing to test. +var executablePath = os.Executable + +// selfEditorCommand builds the EDITOR value: this executable's resolved path +// plus the hidden subcommand, shell-quoted. +// +// os.Executable, never argv[0] — the caller controls argv[0], and Homebrew +// links forgectl into bin/ through a symlink that must be resolved so sops +// spawns the real binary. sops shell-word-splits EDITOR and honours quotes, +// measured on 3.13.3, so a path containing a space survives single-quoting. +func selfEditorCommand() (string, error) { + self, err := executablePath() + if err != nil { + return "", errors.New("could not resolve this executable's own path") + } + resolved, err := filepath.EvalSymlinks(self) + if err != nil { + resolved = self + } + return "'" + strings.ReplaceAll(resolved, "'", `'\''`) + "' __sops-edit", nil +} + +// workDir is the private 0700 directory holding the value, the nonce, the +// backup, and the outcome for one run. +type workDir struct { + dir string + nonce string + backup string +} + +// newWorkDir creates the directory as a SIBLING of the target. +// +// Not $TMPDIR, and that is not a preference: os.Rename across filesystems +// returns EXDEV, so a backup in $TMPDIR would fail to restore in exactly the +// error paths a backup exists for — while a message said it had succeeded. A +// sibling is also what keeps sops' own upward walk for .sops.yaml reaching the +// same rules it would from the target itself. +func newWorkDir(target env.Target) (*workDir, error) { + parent := filepath.Dir(target.Abs()) + dir, err := os.MkdirTemp(parent, ".forgectl-sops-") + if err != nil { + return nil, fmt.Errorf("create a work directory beside %s: %w", target.Rel(), err) + } + // 0700, not 0600: a directory needs its execute bit to be entered at all, + // which is what gosec's file-oriented rule does not model. + if err := os.Chmod(dir, 0o700); err != nil { //nolint:gosec // G302: 0700 on a DIRECTORY; the execute bit is required + _ = os.RemoveAll(dir) + return nil, fmt.Errorf("secure the work directory beside %s: %w", target.Rel(), err) + } + + buf := make([]byte, nonceBytes) + if _, err := rand.Read(buf); err != nil { + _ = os.RemoveAll(dir) + return nil, errors.New("could not generate a nonce") + } + + return &workDir{ + dir: dir, + nonce: base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(buf), + backup: filepath.Join(dir, "backup"), + }, nil +} + +// stage writes the ciphertext backup, the value, and the nonce. +func (w *workDir) stage(before []byte, value string) error { + if err := os.WriteFile(w.backup, before, 0o600); err != nil { + return errors.New("could not write the backup") + } + // No added newline: the read-back comparison is byte-exact, and a + // terminator here would make every value fail it. + if err := os.WriteFile(filepath.Join(w.dir, "value"), []byte(value), 0o600); err != nil { + return errors.New("could not stage the value") + } + if err := os.WriteFile(filepath.Join(w.dir, "nonce"), []byte(w.nonce), 0o600); err != nil { + return errors.New("could not stage the nonce") + } + return nil +} + +// captureOutput writes sops' streams to a 0600 file and returns its path. +// +// Never rendered, never logged, never wrapped into a returned error: a YAML +// parse error from sops quotes the offending line, which is `key: ''`. Its stderr also carries the operator's home path, which is a +// second reason. Writing it down under 0600 keeps it available for a +// deliberate read while keeping it out of a transcript. +func (w *workDir) captureOutput(res exec.SensitiveResult) (string, error) { + stdout, _ := res.Stdout.CopyBytesForParse() + stderr, _ := res.Stderr.CopyBytesForParse() + if len(stdout) == 0 && len(stderr) == 0 { + return "", nil + } + + // Outside the work directory, because the work directory is removed on + // the way out and this file has to outlive it to be worth naming. + f, err := os.CreateTemp("", "forgectl-sops-output-") + if err != nil { + return "", err + } + defer func() { _ = f.Close() }() + if err := f.Chmod(0o600); err != nil { + return "", err + } + if _, err := f.Write(stderr); err != nil { + return "", err + } + if _, err := f.Write(stdout); err != nil { + return "", err + } + return f.Name(), nil +} + +// readOutcome reports what __sops-edit recorded, defaulting to replaced. +// +// The rc=200 "file has not changed" path is NOT why the default exists, which +// an earlier version of this comment got backwards. The editor does run on +// that path — identical bytes are what it produced — and it records `result` +// before writing the document, so a real value is there to read. +// +// The default fires only when the child died before recording, or the file is +// unreadable. Reporting "replaced" then is a guess about a write of unknown +// shape, and it is the safer guess: verify has already proven the value is +// present and encrypted at the requested path, so the only question left is +// whether the key was new, and calling a new key "replaced" understates +// rather than overstates what happened. +func (w *workDir) readOutcome() Outcome { + recorded, err := os.ReadFile(filepath.Join(w.dir, "result")) + if err != nil { + return OutcomeReplaced + } + if strings.TrimSpace(string(recorded)) == OutcomeAdded.String() { + return OutcomeAdded + } + return OutcomeReplaced +} + +// maxEditorErrorBytes bounds the relayed message. It comes from a forgectl +// process, so it is trusted in origin, but reading an unbounded file into an +// error string is a habit worth not forming. +const maxEditorErrorBytes = 4096 + +// readEditorError returns the refusal __sops-edit recorded, or "". +// +// The content is trimmed to one line: every message that reaches here is a +// single-sentence refusal from forgectl's own code, and collapsing anything +// else keeps a surprise out of a rendered error. +func (w *workDir) readEditorError() string { + data, err := os.ReadFile(filepath.Join(w.dir, "error")) + if err != nil || len(data) == 0 { + return "" + } + if len(data) > maxEditorErrorBytes { + data = data[:maxEditorErrorBytes] + } + line, _, _ := strings.Cut(string(data), "\n") + return strings.TrimSpace(line) +} + +// restore puts the backup back and PROVES it, by digest, before returning +// success. A restore that reports success without checking is the one thing +// worse than no restore at all. +func (w *workDir) restore(target env.Target) error { + backup, err := os.ReadFile(w.backup) + if err != nil { + return errors.New("the backup is unreadable") + } + if err := env.WriteTarget(target, backup); err != nil { + return err + } + after, err := env.ReadTarget(target) + if err != nil { + return err + } + if sha256.Sum256(after) != sha256.Sum256(backup) { + return errors.New("the restored file does not match the backup") + } + return nil +} + +// discardStagedValue removes the plaintext value file. Errors are dropped: the +// deferred cleanup removes the whole directory anyway, so this is about +// shortening the window, not about being the only remover. +func (w *workDir) discardStagedValue() { + _ = os.Remove(filepath.Join(w.dir, "value")) +} + +// discardLandedValue removes the decrypted read-back. Same reasoning as +// discardStagedValue — it is a second plaintext copy of the same secret and +// has no reason to outlive the comparison it exists for. +func (w *workDir) discardLandedValue() { + _ = os.Remove(filepath.Join(w.dir, "landed")) +} + +func (w *workDir) cleanup() { _ = os.RemoveAll(w.dir) } diff --git a/internal/sops/driver_test.go b/internal/sops/driver_test.go new file mode 100644 index 00000000..aad0bb8c --- /dev/null +++ b/internal/sops/driver_test.go @@ -0,0 +1,460 @@ +package sops + +// Integration tests for driver.go. They mint an age identity, encrypt a +// fixture, and drive the REAL sops binary through the real sensitive runner. +// +// [x] A byte-exact round-trip: what goes in is what decrypts out +// [x] `--extract --output` writes NO trailing newline (the measurement the +// byte-exact comparison depends on) +// [x] The encrypted diff is one content line plus sops' two metadata lines, +// and untouched values keep byte-identical ciphertext +// [x] The value is NOT present in cleartext anywhere in the file +// [x] A wrong path refuses and leaves the file byte-identical +// [x] The rc=200 "file has not changed" case reports success +// [x] An unencrypted_suffix-matching path refuses before running sops +// [x] A non-SOPS file refuses +// [x] The EDITOR override actually took — no real editor was involved +// +// The skip is GATED. FORGECTL_REQUIRE_SOPS_INTEGRATION=1 turns a missing +// sops or age-keygen into a failure rather than a skip, and CI sets it. A CI +// step that installs a tool is a file anyone can edit in a PR; the env gate is +// what makes its removal go red instead of silently skipping the only tests +// that exercise the subprocess. + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/cameronsjo/forgectl/internal/env" + fcexec "github.com/cameronsjo/forgectl/internal/exec" +) + +// requireTools resolves the binaries these tests need, or decides between a +// skip and a failure based on the gate. +func requireTools(t *testing.T) { + t.Helper() + required := os.Getenv("FORGECTL_REQUIRE_SOPS_INTEGRATION") == "1" + for _, bin := range []string{"sops", "age-keygen"} { + if _, err := exec.LookPath(bin); err != nil { + if required { + t.Fatalf("%s is not on PATH and FORGECTL_REQUIRE_SOPS_INTEGRATION=1 — the integration tests must run here", bin) + } + t.Skipf("%s is not on PATH; set FORGECTL_REQUIRE_SOPS_INTEGRATION=1 to make this a failure", bin) + } + } +} + +const fixturePlaintext = `agentgateway: + llm_key_hermes: seedvalue + other_key: untouched + plain_unencrypted: visible +top: + a: b +` + +// sopsFixture builds a git repository containing an encrypted +// secrets.sops.yaml, and returns the repo root and a resolved Target for it. +func sopsFixture(t *testing.T) (repo string, target env.Target) { + t.Helper() + requireTools(t) + + // A short base dir, not t.TempDir(): the age key path and the work + // directory both live under here, and t.TempDir() embeds the whole test + // name, which has bitten socket-path limits elsewhere in this repo. + repo, err := os.MkdirTemp("", "fcsops") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(repo) }) + + if err := os.MkdirAll(filepath.Join(repo, ".git"), 0o750); err != nil { + t.Fatalf("MkdirAll .git: %v", err) + } + + keyPath := filepath.Join(repo, "age.key") + keyOut, err := exec.CommandContext(t.Context(), "age-keygen", "-o", keyPath).CombinedOutput() //nolint:gosec // G204: a fixed tool name with arguments this test constructed + if err != nil { + t.Fatalf("age-keygen: %v\n%s", err, keyOut) + } + pubOut, err := exec.CommandContext(t.Context(), "age-keygen", "-y", keyPath).Output() //nolint:gosec // G204: a fixed tool name with arguments this test constructed + if err != nil { + t.Fatalf("age-keygen -y: %v", err) + } + recipient := strings.TrimSpace(string(pubOut)) + + // unencrypted_suffix is in the creation rule on purpose: every encrypted + // file in the estate this feature targets carries it, so the cleartext + // refusal is exercised against a realistic file rather than a contrived + // one. + rules := "creation_rules:\n - path_regex: .*\n age: " + recipient + "\n unencrypted_suffix: _unencrypted\n" + if err := os.WriteFile(filepath.Join(repo, ".sops.yaml"), []byte(rules), 0o600); err != nil { + t.Fatalf("WriteFile .sops.yaml: %v", err) + } + + plainPath := filepath.Join(repo, "plain.yaml") + if err := os.WriteFile(plainPath, []byte(fixturePlaintext), 0o600); err != nil { + t.Fatalf("WriteFile plain.yaml: %v", err) + } + + t.Setenv("SOPS_AGE_KEY_FILE", keyPath) + encPath := filepath.Join(repo, "secrets.sops.yaml") + encrypt := exec.CommandContext(t.Context(), "sops", "--encrypt", "--output", encPath, plainPath) //nolint:gosec // G204: a fixed tool name with arguments this test constructed + // sops discovers .sops.yaml by walking up from its WORKING DIRECTORY, not + // from the input file's path — so without this it reports "config file not + // found, or has no creation rules" while the config sits right beside the + // input. + encrypt.Dir = repo + encOut, err := encrypt.CombinedOutput() + if err != nil { + t.Fatalf("sops --encrypt: %v\n%s", err, encOut) + } + if err := os.Remove(plainPath); err != nil { + t.Fatalf("Remove plain.yaml: %v", err) + } + + target, err = env.ResolveTarget("secrets.sops.yaml", repo) + if err != nil { + t.Fatalf("ResolveTarget: %v", err) + } + t.Cleanup(target.Close) + return repo, target +} + +// testClient builds a Client over the REAL sensitive runner, so these tests +// drive actual subprocesses rather than a fake. Pointing the editor at a +// built forgectl is buildForgectl's job, not this one. +func testClient(t *testing.T) *Client { + t.Helper() + return NewClient(fcexec.NewOSSensitiveRunner()) +} + +// buildForgectl compiles the module's main package to a temp path and points +// this process's os.Executable at it, so selfEditorCommand resolves to a real +// forgectl carrying this tree's __sops-edit. +func buildForgectl(t *testing.T) { + t.Helper() + // Gated like the tool checks: under FORGECTL_REQUIRE_SOPS_INTEGRATION a + // missing toolchain is a failure, not a skip. Without this, the gate had a + // hole — every test in this file would have skipped silently on a runner + // with no `go` on PATH while the gate reported nothing wrong. + if _, err := exec.LookPath("go"); err != nil { + if os.Getenv("FORGECTL_REQUIRE_SOPS_INTEGRATION") == "1" { + t.Fatalf("go is not on PATH and FORGECTL_REQUIRE_SOPS_INTEGRATION=1: %v", err) + } + t.Skipf("go is not on PATH: %v", err) + } + dir, err := os.MkdirTemp("", "fcbin") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + + bin := filepath.Join(dir, "forgectl") + // The module root is two levels up from internal/sops. + out, err := exec.CommandContext(t.Context(), "go", "build", "-o", bin, "../..").CombinedOutput() //nolint:gosec // G204: a fixed tool name with arguments this test constructed + if err != nil { + t.Fatalf("go build: %v\n%s", err, out) + } + prev := executablePath + executablePath = func() (string, error) { return bin, nil } + t.Cleanup(func() { executablePath = prev }) +} + +func TestIntegration_RoundTripAndDiffShape(t *testing.T) { + _, target := sopsFixture(t) + buildForgectl(t) + + before, err := os.ReadFile(target.Abs()) //nolint:gosec // G304: a fixture this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + + const value = "a-new-value-not-a-real-secret" + outcome, err := testClient(t).SetValue(context.Background(), target, "agentgateway.llm_key_hermes", value) + if err != nil { + t.Fatalf("SetValue: %v", err) + } + if outcome != OutcomeReplaced { + t.Errorf("outcome = %v, want replaced", outcome) + } + + after, err := os.ReadFile(target.Abs()) //nolint:gosec // G304: a fixture this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + + // The value must not appear in cleartext anywhere. + if bytes.Contains(after, []byte(value)) { + t.Error("the value appears in cleartext in the encrypted file") + } + + // A round-trip through sops proves what landed. + landed := extractValue(t, target.Abs(), `["agentgateway"]["llm_key_hermes"]`) + if landed != value { + t.Errorf("decrypted = %q, want %q", landed, value) + } + + // Untouched values keep byte-identical ciphertext, which is the property + // that makes a one-key write reviewable. + beforeLines := lineMap(before) + afterLines := lineMap(after) + for _, key := range []string{"other_key", "a"} { + if beforeLines[key] == "" { + t.Fatalf("fixture has no %q line to compare", key) + } + if beforeLines[key] != afterLines[key] { + t.Errorf("the untouched %q line changed:\n before %q\n after %q", key, beforeLines[key], afterLines[key]) + } + } + + // The changed-line count: one content line plus sops' lastmodified and + // mac. Counted directly rather than through git, so the test needs no + // repository history. + changed := 0 + for key, line := range afterLines { + if beforeLines[key] != line { + changed++ + } + } + if changed != 3 { + t.Errorf("%d lines changed, want 3 (the value plus sops' lastmodified and mac): %v", changed, changedKeys(beforeLines, afterLines)) + } + +} + +// TestIntegration_ExtractWritesNoTrailingNewline pins the measurement the +// driver's byte-exact comparison depends on. If sops ever started appending a +// terminator, the comparison would fail for every value and the driver would +// restore every write — so this is the assertion that explains why there is +// no strip. +// TestIntegration_EditorOverrideTook proves no real editor was involved, which +// the test plan claimed and nothing asserted. +// +// It matters because the whole design rests on sops running OUR editor. If the +// override silently failed, sops would fall back to $EDITOR or vi — and on a +// CI runner with neither, or with a non-interactive one, the run could still +// look like a pass for the wrong reason. Pointing the override at a script +// that records its invocation is the only way to see the difference. +func TestIntegration_EditorOverrideTook(t *testing.T) { + _, target := sopsFixture(t) + + marker := filepath.Join(t.TempDir(), "invoked") + script := filepath.Join(t.TempDir(), "fake-editor.sh") + body := "#!/bin/sh\necho \"$1\" > " + marker + "\nexit 1\n" + if err := os.WriteFile(script, []byte(body), 0o700); err != nil { //nolint:gosec // G306: a script this test must be able to execute + t.Fatalf("WriteFile: %v", err) + } + + prev := executablePath + // selfEditorCommand appends " __sops-edit", which the script ignores. + executablePath = func() (string, error) { return script, nil } + t.Cleanup(func() { executablePath = prev }) + + // Exits 1, so the driver restores and refuses — that is expected. What is + // under test is that the script ran at all. + _, err := testClient(t).SetValue(context.Background(), target, "agentgateway.llm_key_hermes", "v") + if err == nil { + t.Fatal("SetValue succeeded with an editor that exits 1, want a refusal") + } + + recorded, readErr := os.ReadFile(marker) //nolint:gosec // G304: a path this test created + if readErr != nil { + t.Fatalf("the editor override did not run — sops used something else: %v", readErr) + } + // sops passes the decrypted temp file as the only argument. + if handed := strings.TrimSpace(string(recorded)); handed == "" { + t.Error("the editor ran with no argument, want the decrypted temp path") + } else if handed == target.Abs() { + t.Errorf("the editor was handed the ENCRYPTED file (%s), want sops' decrypted temp copy", handed) + } +} + +func TestIntegration_ExtractWritesNoTrailingNewline(t *testing.T) { + _, target := sopsFixture(t) + + out := filepath.Join(t.TempDir(), "landed") + run := exec.CommandContext(t.Context(), "sops", "--decrypt", "--extract", `["agentgateway"]["llm_key_hermes"]`, "--output", out, target.Abs()) //nolint:gosec // G204: a fixed tool name with arguments this test constructed + if combined, err := run.CombinedOutput(); err != nil { + t.Fatalf("sops --extract: %v\n%s", err, combined) + } + got, err := os.ReadFile(out) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if want := "seedvalue"; string(got) != want { + t.Errorf("extract wrote %q, want exactly %q with no terminator", got, want) + } +} + +func TestIntegration_IdempotentReSetReportsSuccess(t *testing.T) { + _, target := sopsFixture(t) + buildForgectl(t) + + const value = "the-same-value-twice" + client := testClient(t) + if _, err := client.SetValue(context.Background(), target, "agentgateway.llm_key_hermes", value); err != nil { + t.Fatalf("first SetValue: %v", err) + } + + // The second write hands sops identical bytes, so sops exits 200 with + // "File has not changed, exiting." That is success — the key holds the + // requested value — and reporting it as a failure would break every + // idempotent re-run. + outcome, err := client.SetValue(context.Background(), target, "agentgateway.llm_key_hermes", value) + if err != nil { + t.Fatalf("second SetValue: %v — the rc=200 unchanged case must report success", err) + } + if outcome == OutcomeUnspecified { + t.Error("outcome is unspecified on the unchanged path") + } + if landed := extractValue(t, target.Abs(), `["agentgateway"]["llm_key_hermes"]`); landed != value { + t.Errorf("decrypted = %q, want %q", landed, value) + } +} + +func TestIntegration_RefusalsLeaveTheFileByteIdentical(t *testing.T) { + // Gated at the PARENT, not only inside each subtest's fixture. Without + // this the parent reports PASS on a machine with no sops while every + // subtest skips — a green that reached nothing, and under the CI gate a + // green sitting next to its own siblings' failures. + requireTools(t) + + cases := []struct { + name string + path string + wantMsg string + }{ + { + name: "a missing block", + path: "nosuchblock.key", + wantMsg: "no block", + }, + { + name: "a path the file would store in cleartext", + // The fixture's creation rule sets unencrypted_suffix, so sops + // would write this key in the clear beside its encrypted + // siblings — and a decrypt round-trip would pass. + path: "agentgateway.something_unencrypted", + wantMsg: "stored in the clear", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, target := sopsFixture(t) + buildForgectl(t) + + before, err := os.ReadFile(target.Abs()) //nolint:gosec // G304: a fixture this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + + const sentinel = "s3ntinel-VALUE-77x" + _, err = testClient(t).SetValue(context.Background(), target, c.path, sentinel) + if err == nil { + t.Fatal("SetValue returned nil error, want a refusal") + } + if !strings.Contains(err.Error(), c.wantMsg) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.wantMsg) + } + if strings.Contains(err.Error(), sentinel) { + t.Errorf("error %q echoed the value", err.Error()) + } + + after, err := os.ReadFile(target.Abs()) //nolint:gosec // G304: a fixture this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !bytes.Equal(before, after) { + t.Error("the file changed on a refusal, want it byte-identical") + } + // And no work directory was left behind beside it. + assertNoWorkDirLeft(t, filepath.Dir(target.Abs())) + }) + } +} + +func TestIntegration_NonSOPSFileRefused(t *testing.T) { + repo, _ := sopsFixture(t) + + plain := filepath.Join(repo, "secrets.yaml") + if err := os.WriteFile(plain, []byte("block:\n key: value\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + target, err := env.ResolveTarget("secrets.yaml", repo) + if err != nil { + t.Fatalf("ResolveTarget: %v", err) + } + defer target.Close() + + // The name passes the allowlist; the CONTENT is what refuses. That is the + // narrowing the two checks together provide. + _, err = testClient(t).SetValue(context.Background(), target, "block.key", "v") + if err == nil { + t.Fatal("SetValue against a plain YAML file returned nil error, want a refusal") + } + if !strings.Contains(err.Error(), "not a SOPS document") { + t.Errorf("error = %q, want it to name the missing sops: block", err.Error()) + } +} + +// extractValue decrypts one value through the real sops binary. +func extractValue(t *testing.T, file, address string) string { + t.Helper() + out := filepath.Join(t.TempDir(), "extracted") + run := exec.CommandContext(t.Context(), "sops", "--decrypt", "--extract", address, "--output", out, file) //nolint:gosec // G204: a fixed tool name with arguments this test constructed + if combined, err := run.CombinedOutput(); err != nil { + t.Fatalf("sops --extract: %v\n%s", err, combined) + } + got, err := os.ReadFile(out) //nolint:gosec // G304: a path this test created + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + return string(got) +} + +// lineMap indexes a document's `key: value` lines by key, so two versions can +// be compared per key rather than by diff position. +func lineMap(doc []byte) map[string]string { + out := map[string]string{} + for _, raw := range strings.Split(string(doc), "\n") { + trimmed := strings.TrimSpace(raw) + key, _, found := strings.Cut(trimmed, ":") + if !found || key == "" { + continue + } + out[key] = trimmed + } + return out +} + +func changedKeys(before, after map[string]string) []string { + var keys []string + for key, line := range after { + if before[key] != line { + keys = append(keys, key) + } + } + return keys +} + +// assertNoWorkDirLeft proves the deferred cleanup ran: a work directory left +// beside the target would hold the staged value at 0600, which is a durable +// secret on disk nobody asked for. +func assertNoWorkDirLeft(t *testing.T, dir string) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".forgectl-sops-") { + t.Errorf("a work directory was left behind: %s", e.Name()) + } + } +} diff --git a/internal/sops/edit.go b/internal/sops/edit.go new file mode 100644 index 00000000..5177a184 --- /dev/null +++ b/internal/sops/edit.go @@ -0,0 +1,428 @@ +package sops + +import ( + "errors" + "fmt" + "strings" +) + +// Outcome reports what SetScalar did, because the two cases mean different +// things to an operator: adding a key is new configuration, replacing one is a +// rotation. +type Outcome int + +const ( + // OutcomeUnspecified is the ineligible zero value. + OutcomeUnspecified Outcome = iota + // OutcomeAdded means the key was not present and a line was inserted. + OutcomeAdded + // OutcomeReplaced means an existing line's value was overwritten. + OutcomeReplaced +) + +func (o Outcome) String() string { + switch o { + case OutcomeAdded: + return "added" + case OutcomeReplaced: + return "replaced" + default: + return "unspecified" + } +} + +// line keeps a document line's text and its own terminator separate, so an +// insertion into a CRLF document is CRLF-terminated and a mixed-ending +// document is not silently normalised. +type line struct { + text string + ending string +} + +// indent returns the number of leading spaces. Tabs are refused document-wide +// before any line is inspected (see refuseUnsupportedDocument), so a space +// count is the whole story here. +func (l line) indent() int { + for i := 0; i < len(l.text); i++ { + if l.text[i] != ' ' { + return i + } + } + return len(l.text) +} + +// leadingWhitespace returns the run of spaces AND tabs at the start of the +// line. It is deliberately separate from indent(): indent() stops at the +// first non-space, so a tab-indented line reports indent 0 and its tab never +// appears in the slice indent() would suggest — which made the tab refusal +// silently inspect an empty string. +func (l line) leadingWhitespace() string { + for i := 0; i < len(l.text); i++ { + if l.text[i] != ' ' && l.text[i] != '\t' { + return l.text[:i] + } + } + return l.text +} + +func (l line) isBlank() bool { return strings.TrimSpace(l.text) == "" } + +func (l line) isComment() bool { return strings.HasPrefix(strings.TrimSpace(l.text), "#") } + +// isSequenceItem reports whether the line starts a YAML sequence entry. A +// sequence where a mapping was expected is a refusal, not something to edit. +func (l line) isSequenceItem() bool { + t := strings.TrimSpace(l.text) + return t == "-" || strings.HasPrefix(t, "- ") +} + +// SetScalar writes value at path in a YAML document, editing text line-wise. +// +// # Why this is not a YAML round-trip +// +// Decoding and re-emitting would reflow every other block and reorder keys, +// turning a one-line change into a whole-file diff nobody can review — and in +// an encrypted file every reflowed line is a ciphertext change, so the diff +// would also destroy the property that makes a one-key write auditable. A real +// write measures 3 insertions and 2 deletions: the content line plus sops' +// own lastmodified and mac. +// +// # What it refuses +// +// The line model can bound a block only for documents of a particular shape, +// and every shape it cannot bound is a REFUSAL rather than a guess. That +// asymmetry is the whole design: a mis-bounded block produces a corrupted +// encrypted file, or a duplicate YAML key, and neither is visible until a +// consumer reads the wrong value. A missing block — at any level — is likewise +// refused and never created, because a block this code invents encrypts +// perfectly well and the consumer reads nothing from it. +func SetScalar(doc []byte, path []string, value string) ([]byte, Outcome, error) { + if len(path) == 0 { + return nil, OutcomeUnspecified, errors.New("path is empty") + } + + lines := splitLines(string(doc)) + if err := refuseUnsupportedDocument(lines); err != nil { + return nil, OutcomeUnspecified, err + } + + // The top level: headers sit at indent 0 and the search covers the whole + // document. + start, end, wantIndent := 0, len(lines), 0 + + // Walk every segment but the last, narrowing to its block each time. + for _, segment := range path[:len(path)-1] { + headerIdx, err := findHeader(lines, start, end, wantIndent, segment) + if err != nil { + return nil, OutcomeUnspecified, err + } + blockStart, blockEnd := blockRange(lines, headerIdx, wantIndent) + childIndent, err := bodyIndent(lines, blockStart, blockEnd, wantIndent) + if err != nil { + return nil, OutcomeUnspecified, err + } + start, end, wantIndent = blockStart, blockEnd, childIndent + } + + leaf := path[len(path)-1] + if idx := findLeaf(lines, start, end, wantIndent, leaf); idx >= 0 { + if leafIsMappingHeader(lines, idx, wantIndent, leaf) { + // Names the rule and not the segment, the same discipline every + // other refusal in this package follows: ParsePath's grammar is + // wide enough that plenty of provider token formats parse as one + // valid segment, so a secret pasted into the key slot reaches + // here — and this message is relayed out of the child process to + // the operator's terminal, which is the transcript the feature + // exists to keep the value out of. + // + // It also leads with a word rather than the segment for a second + // reason: fang title-cases an error's first token when it renders, + // so opening with the key produced `"App" names a block` — a + // capitalisation of the operator's own key, reading as a different + // key than the one they typed. + return nil, OutcomeUnspecified, errors.New("the path names a block rather than a value; only a scalar key can be set") + } + lines[idx] = replaceValue(lines[idx], value) + return joinLines(lines), OutcomeReplaced, nil + } + + insertAt := insertionPoint(lines, start, end) + newLine := line{ + text: strings.Repeat(" ", wantIndent) + leaf + ": " + encodeScalar(value), + ending: endingFor(lines, insertAt), + } + // A document whose last line has no terminator: appending after it would + // otherwise splice the two lines together. Terminate what was the last + // line and leave the new one unterminated, so the file keeps its + // no-trailing-newline shape instead of gaining one. + if insertAt > 0 && insertAt == len(lines) && lines[insertAt-1].ending == "" { + lines[insertAt-1].ending = newLine.ending + newLine.ending = "" + } + lines = append(lines[:insertAt], append([]line{newLine}, lines[insertAt:]...)...) + return joinLines(lines), OutcomeAdded, nil +} + +// refuseUnsupportedDocument rejects whole-document shapes the line model +// cannot reason about, before any block is located. +func refuseUnsupportedDocument(lines []line) error { + for _, l := range lines { + // A tab in the leading whitespace makes every indent comparison in + // this file meaningless — and YAML forbids tabs as indentation + // anyway, so this is a malformed document rather than a limitation. + if strings.ContainsRune(l.leadingWhitespace(), '\t') { + return errors.New("document uses tab indentation; only spaces are supported") + } + // A multi-document stream has more than one root, so "the block at + // indent 0" is ambiguous and the walk could enter the wrong document. + if strings.HasPrefix(l.text, "---") || strings.HasPrefix(l.text, "...") { + return errors.New("document is a multi-document stream; only a single document is supported") + } + } + return nil +} + +// findHeader locates a mapping header for segment at exactly wantIndent within +// [start,end). +// +// The match requires the line to be nothing but `:` — no +// value, no trailing comment. A header carrying a comment (`block: # notes`) +// is refused explicitly rather than reported as missing, because "not found" +// would send the reader looking for a block that is plainly there. +func findHeader(lines []line, start, end, wantIndent int, segment string) (int, error) { + prefix := strings.Repeat(" ", wantIndent) + segment + ":" + for i := start; i < end; i++ { + l := lines[i] + if l.isBlank() || l.isComment() || l.indent() != wantIndent { + continue + } + if !strings.HasPrefix(l.text, prefix) { + continue + } + if strings.TrimSpace(l.text) != segment+":" { + return 0, fmt.Errorf("block %q carries a value or a trailing comment on its header line; only a bare `%s:` is supported", segment, segment) + } + return i, nil + } + return 0, fmt.Errorf("no block %q at this level; creating one is out of scope", segment) +} + +// blockRange returns the half-open line range holding a header's body. +// +// It ends at the first later line that is non-blank, NOT a comment, and +// indented at or below the header. Comments are excluded from the terminator +// on purpose: a comment sitting at column 0 in the middle of a block would +// otherwise cut the range short, the search would miss an existing sibling +// past it, and the key would be added a SECOND time — a duplicate YAML key, +// which is the worst outcome available here. bodyIndent refuses that document +// separately; this function simply must not make the decision. +func blockRange(lines []line, headerIdx, headerIndent int) (start, end int) { + start = headerIdx + 1 + for i := start; i < len(lines); i++ { + l := lines[i] + if l.isBlank() || l.isComment() { + continue + } + if l.indent() <= headerIndent { + return start, i + } + } + return start, len(lines) +} + +// bodyIndent derives the indent a child of this block must sit at. +// +// It is the indent of the block's first non-comment content line; an empty +// block falls back to the header's indent plus two. Comments are excluded +// from the derivation because ` # note` matches "indented content" perfectly +// well, and a comment indented differently from the real keys would silently +// reparent the inserted key. +func bodyIndent(lines []line, start, end, headerIndent int) (int, error) { + for i := start; i < end; i++ { + l := lines[i] + if l.isBlank() { + continue + } + if l.isComment() { + // A comment at or below the header's indent, inside the block, is + // ambiguous: it reads as belonging to the next block as easily as + // to this one, and the range rule above deliberately does not let + // it terminate the block. Rather than pick an interpretation, + // refuse. + if l.indent() <= headerIndent { + return 0, errors.New("a comment inside the block is indented at or below its header; the block's extent is ambiguous") + } + continue + } + // A sequence where a mapping was expected: inserting `key: value` + // would produce a document that is half list and half map. + if l.isSequenceItem() { + return 0, errors.New("the block holds a sequence, not a mapping; only scalar keys in a mapping are supported") + } + return l.indent(), nil + } + // An empty block. Two spaces past the header is the conventional depth, + // and there is nothing else to infer from. + return headerIndent + 2, nil +} + +// findLeaf locates an existing `:` line, or -1. +// +// The trailing colon stops `llm_key_hermes_old:` from matching +// `llm_key_hermes`, and the exact-indent requirement keeps a same-named key in +// a nested block from being mistaken for this one. +// +// # Why the colon is not enough on its own +// +// A bare prefix match on `leaf + ":"` also matches a DIFFERENT key whose name +// merely begins that way: the key `a:b` matches the leaf `a`, and since +// replaceValue cuts at the first colon, replacing `a` would rewrite +// ` a:b: 'v'` as ` a: 'new'` — destroying a real key and its encrypted +// value, reporting `replaced a`, and passing every downstream check, because +// the extract of `["block"]["a"]` then returns exactly the value supplied. +// +// `a:b: 'v'` is valid YAML and decodes to the key `a:b`, so it can legitimately +// be in a SOPS file. Requiring a space or end-of-line after the colon is what +// separates the two. It also refuses the `key:value` no-space shape, which +// YAML reads as a plain scalar rather than a mapping at all. +func findLeaf(lines []line, start, end, wantIndent int, leaf string) int { + prefix := strings.Repeat(" ", wantIndent) + leaf + ":" + for i := start; i < end; i++ { + l := lines[i] + if l.isBlank() || l.isComment() || l.indent() != wantIndent { + continue + } + if !strings.HasPrefix(l.text, prefix) { + continue + } + rest := l.text[len(prefix):] + if rest == "" || rest[0] == ' ' || rest[0] == '\t' { + return i + } + } + return -1 +} + +// leafIsMappingHeader reports whether the line at idx is a block header rather +// than a scalar assignment — `sub:` with indented content beneath it. +// +// Writing a scalar over a mapping header produces invalid YAML: the header's +// children survive at their old depth under what is now a scalar, which +// yaml.v3 rejects with `did not find expected key`. The child's own parse +// catches that and the encrypted file survives, but the operator gets "the +// edited document does not parse as YAML", which names nothing they can act +// on and reads as a forgectl bug. Refusing by name here says what is actually +// wrong: the path names a block, not a value. +func leafIsMappingHeader(lines []line, idx, wantIndent int, leaf string) bool { + // A line carrying anything after the colon is an assignment, not a header. + if strings.TrimSpace(lines[idx].text) != leaf+":" { + return false + } + for i := idx + 1; i < len(lines); i++ { + l := lines[i] + if l.isBlank() || l.isComment() { + continue + } + return l.indent() > wantIndent + } + return false +} + +// insertionPoint returns the index to insert a new key at, walking back past +// trailing blank lines so the key lands INSIDE the block rather than after the +// gap that separates it from whatever follows. +func insertionPoint(lines []line, start, end int) int { + at := end + for at > start && lines[at-1].isBlank() { + at-- + } + return at +} + +// replaceValue rewrites a line's value, preserving the line's own indent and +// any trailing inline comment — `llm_key: xxx # rotated 2026-01` keeps the +// note. Losing it would quietly discard operator context that is often the +// only record of why a key exists. +func replaceValue(l line, value string) line { + colon := strings.Index(l.text, ":") + head := l.text[:colon+1] + rest := l.text[colon+1:] + _, comment := splitValueAndComment(rest) + + text := head + " " + encodeScalar(value) + if comment != "" { + text += " " + comment + } + return line{text: text, ending: l.ending} +} + +// splitValueAndComment separates a scalar from a trailing `#` comment. +// +// It tracks quote state rather than searching for the first `#`, because a `#` +// inside a quoted scalar is data: `key: 'pass#word'` has no comment, and +// treating it as one would silently truncate the stored value. A `#` only +// begins a comment when it follows whitespace and sits outside quotes. +func splitValueAndComment(rest string) (value, comment string) { + var inSingle, inDouble bool + for i := 0; i < len(rest); i++ { + c := rest[i] + switch { + case c == '\'' && !inDouble: + inSingle = !inSingle + case c == '"' && !inSingle: + inDouble = !inDouble + case c == '#' && !inSingle && !inDouble: + if i > 0 && (rest[i-1] == ' ' || rest[i-1] == '\t') { + return strings.TrimRight(rest[:i], " \t"), rest[i:] + } + } + } + return strings.TrimRight(rest, " \t"), "" +} + +// endingFor picks the line terminator for a line inserted at idx: the +// terminator of the line it follows, falling back to the document's first +// terminator and finally to "\n" for a single-line document with none. +func endingFor(lines []line, idx int) string { + if idx > 0 && lines[idx-1].ending != "" { + return lines[idx-1].ending + } + for _, l := range lines { + if l.ending != "" { + return l.ending + } + } + return "\n" +} + +// splitLines splits s into lines, keeping each line's own terminator. A +// document with no trailing newline yields a final line with an empty ending, +// so joinLines reproduces the input byte-for-byte. +func splitLines(s string) []line { + var out []line + for len(s) > 0 { + idx := strings.IndexByte(s, '\n') + if idx < 0 { + out = append(out, line{text: s}) + break + } + text := s[:idx] + ending := "\n" + if strings.HasSuffix(text, "\r") { + text = text[:len(text)-1] + ending = "\r\n" + } + out = append(out, line{text: text, ending: ending}) + s = s[idx+1:] + } + return out +} + +func joinLines(lines []line) []byte { + var b strings.Builder + for _, l := range lines { + b.WriteString(l.text) + b.WriteString(l.ending) + } + return []byte(b.String()) +} diff --git a/internal/sops/edit_property_test.go b/internal/sops/edit_property_test.go new file mode 100644 index 00000000..58fc2728 --- /dev/null +++ b/internal/sops/edit_property_test.go @@ -0,0 +1,130 @@ +package sops + +// A property test over document shapes SetScalar was not designed around. +// +// The table in edit_test.go pins exact output for shapes the line model DOES +// handle, and the refusal table pins the shapes it rejects by name. Neither +// answers the question that actually matters for an encrypted file: for +// everything else — the YAML features nobody thought to enumerate — does +// SetScalar either refuse, or leave the document correct? +// +// So this asserts an invariant rather than an output. For each input, one of +// exactly two things must be true: +// +// - it refuses, which is always acceptable, or +// - it emits a document that still PARSES, retains every top-level key it +// had, and has the requested value reachable at the requested path. +// +// A mis-bounded block fails this three ways at once: the emitted document +// stops parsing (a map/sequence mix), or a top-level key vanishes (the key +// landed in the wrong block and displaced something), or the value is not +// where it was asked for. That is the failure worth catching, because in an +// encrypted file none of it is visible until a consumer reads the wrong value. + +import ( + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// mapLookup reads key from either map shape yaml.v3 produces. +// +// A mapping with a non-string key anywhere — `123: 'v'` is the case here — +// decodes as map[interface{}]interface{} rather than map[string]interface{}, +// so a walk that only handles the latter fails on the TYPE while the write it +// was checking is perfectly correct. Narrowing the assertion to one map shape +// would have reported a bug in SetScalar that does not exist. +func mapLookup(node any, key string) (any, bool) { + switch m := node.(type) { + case map[string]any: + v, ok := m[key] + return v, ok + case map[any]any: + v, ok := m[key] + return v, ok + default: + return nil, false + } +} + +func TestSetScalar_ShapeInvariant(t *testing.T) { + const probe = "PROBEVALUE" + + cases := []struct { + name string + doc string + path []string + }{ + {"an anchor on the block header", "block: &anc\n k: 'v'\nother:\n m: 'n'\n", []string{"block", "new"}}, + {"an alias as a sibling's value", "base:\n k: &a 'v'\nblock:\n m: *a\n", []string{"block", "new"}}, + {"a merge key inside the block", "base: &b\n x: '1'\nblock:\n <<: *b\n k: 'v'\n", []string{"block", "new"}}, + {"a flow mapping as the block", "block: {a: 1, b: 2}\n", []string{"block", "new"}}, + {"a flow mapping as a sibling", "block:\n inner: {a: 1}\n k: 'v'\n", []string{"block", "new"}}, + {"a double-quoted key", "block:\n \"quoted key\": 'v'\n", []string{"block", "new"}}, + {"a single-quoted key", "block:\n 'sq': 'v'\n", []string{"block", "new"}}, + {"a key with the same name as its block", "block:\n block: 'v'\n", []string{"block", "new"}}, + {"a leaf that is a prefix of a sibling", "block:\n newer: 'v'\n", []string{"block", "new"}}, + {"a sops block that is not last", "sops:\n mac: 'x'\nblock:\n k: 'v'\n", []string{"block", "new"}}, + {"a literal block scalar sibling", "block:\n text: |\n line one\n line two\n k: 'v'\n", []string{"block", "new"}}, + {"a folded block scalar sibling", "block:\n text: >\n folded\n k: 'v'\n", []string{"block", "new"}}, + {"a nested block of the same name", "block:\n block:\n k: 'v'\n k: 'w'\n", []string{"block", "new"}}, + {"a deeper sibling before a shallower one", "block:\n a:\n deep: '1'\n k: 'v'\n", []string{"block", "new"}}, + {"a sibling with an empty value", "block:\n empty:\n k: 'v'\n", []string{"block", "new"}}, + {"CRLF with a trailing comment", "block:\r\n k: 'v' # note\r\n", []string{"block", "k"}}, + {"a document-end marker", "block:\n k: 'v'\n...\n", []string{"block", "new"}}, + {"a tab inside a value rather than the indent", "block:\n k: \"has\ttab\"\n", []string{"block", "new"}}, + {"a three-space indent", "block:\n k: 'v'\n", []string{"block", "new"}}, + {"a one-space indent", "block:\n k: 'v'\n", []string{"block", "new"}}, + {"a ten-space indent", "block:\n k: 'v'\n", []string{"block", "new"}}, + {"a blank line between siblings", "block:\n a: '1'\n\n b: '2'\n", []string{"block", "new"}}, + {"a value that looks like a nested key", "block:\n k: 'a: b'\n", []string{"block", "new"}}, + {"a numeric-looking key", "block:\n 123: 'v'\n", []string{"block", "new"}}, + {"a null value sibling", "block:\n k: null\n", []string{"block", "new"}}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var before map[string]any + inputParses := yaml.Unmarshal([]byte(c.doc), &before) == nil + + got, _, err := SetScalar([]byte(c.doc), c.path, probe) + if err != nil { + // A refusal is always an acceptable answer. The named-shape + // refusals are pinned by TestSetScalar_Refusals; here the only + // requirement is that refusing leaves nothing behind. + if got != nil { + t.Errorf("refused but returned a document: %q", got) + } + return + } + + var after map[string]any + if perr := yaml.Unmarshal(got, &after); perr != nil { + if !inputParses { + t.Skip("the input was not valid YAML either; not a regression") + } + t.Fatalf("accepted a valid document and emitted an unparseable one: %v\n%s", perr, got) + } + + for key := range before { + if _, ok := after[key]; !ok { + t.Errorf("top-level key %q disappeared — the key landed in the wrong block", key) + } + } + + cur := any(after) + for _, segment := range c.path { + next, ok := mapLookup(cur, segment) + if !ok { + t.Fatalf("path %v is not reachable: found %T at %q", c.path, cur, segment) + } + cur = next + } + s, ok := cur.(string) + if !ok || !strings.Contains(s, probe) { + t.Errorf("value at %v decoded as %#v, want %q", c.path, cur, probe) + } + }) + } +} diff --git a/internal/sops/edit_test.go b/internal/sops/edit_test.go new file mode 100644 index 00000000..c36844bc --- /dev/null +++ b/internal/sops/edit_test.go @@ -0,0 +1,463 @@ +package sops + +// Test plan for edit.go +// +// SetScalar (Classification: pure text transform, table-driven, no sops +// binary and no filesystem) +// +// The reference suite, ported from the shell prototype this replaces: +// [x] 1 appends inside the block +// [x] 2 copies a four-space sibling's indent rather than assuming two +// [x] 3 replaces in place, preserving key order +// [x] 4 appends to a block that runs to EOF +// [x] 5 lands inside the block, not after a trailing blank line +// [x] 6 a prefix-sharing sibling (llm_key_hermes_old) is untouched +// [x] 7 a same-named key in another block is untouched +// [x] 8 stays out of a trailing sops: block +// [x] 9 a missing block refuses +// Added after review: +// [x] 10 three and four levels deep; a missing INTERMEDIATE block refuses +// [x] 11 each unsupported shape refuses rather than mis-bounding +// [x] 12 quoting survives a yaml.v3 round-trip (not an output-shape assert) +// [x] 13 control bytes and invalid UTF-8 refuse, with no value echoed +// [x] 14 a CRLF document gets a CRLF-terminated insertion +// [x] 15 replace preserves the line's own indent and trailing comment + +import ( + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +func TestSetScalar_Table(t *testing.T) { + cases := []struct { + name string + doc string + path []string + value string + want string + outcome Outcome + }{ + { + name: "appends inside the block", + doc: "agentgateway:\n existing: 'a'\ntop:\n b: 'c'\n", + path: []string{"agentgateway", "llm_key"}, + value: "secret", + want: "agentgateway:\n existing: 'a'\n llm_key: 'secret'\ntop:\n b: 'c'\n", + outcome: OutcomeAdded, + }, + { + name: "copies a four-space sibling indent", + doc: "block:\n only: 'x'\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\n only: 'x'\n added: 'v'\n", + outcome: OutcomeAdded, + }, + { + name: "copies a two-space sibling indent", + doc: "block:\n only: 'x'\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\n only: 'x'\n added: 'v'\n", + outcome: OutcomeAdded, + }, + { + name: "replaces in place preserving order", + doc: "block:\n first: 'a'\n target: 'old'\n last: 'z'\n", + path: []string{"block", "target"}, + value: "new", + want: "block:\n first: 'a'\n target: 'new'\n last: 'z'\n", + outcome: OutcomeReplaced, + }, + { + name: "appends to a block running to EOF", + doc: "block:\n only: 'x'", + path: []string{"block", "added"}, + value: "v", + want: "block:\n only: 'x'\n added: 'v'", + outcome: OutcomeAdded, + }, + { + name: "lands inside the block not after a blank line", + doc: "block:\n only: 'x'\n\ntop:\n b: 'c'\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\n only: 'x'\n added: 'v'\n\ntop:\n b: 'c'\n", + outcome: OutcomeAdded, + }, + { + name: "a prefix-sharing sibling is untouched", + doc: "block:\n llm_key_hermes_old: 'stale'\n", + path: []string{"block", "llm_key_hermes"}, + value: "fresh", + want: "block:\n llm_key_hermes_old: 'stale'\n llm_key_hermes: 'fresh'\n", + outcome: OutcomeAdded, + }, + { + name: "a same-named key in another block is untouched", + doc: "first:\n shared: 'one'\nsecond:\n shared: 'two'\n", + path: []string{"second", "shared"}, + value: "new", + want: "first:\n shared: 'one'\nsecond:\n shared: 'new'\n", + outcome: OutcomeReplaced, + }, + { + name: "stays out of a trailing sops block", + doc: "block:\n only: 'x'\nsops:\n mac: 'ENC[...]'\n version: 3.13.3\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\n only: 'x'\n added: 'v'\nsops:\n mac: 'ENC[...]'\n version: 3.13.3\n", + outcome: OutcomeAdded, + }, + { + name: "three levels deep", + doc: "a:\n b:\n c:\n leaf: 'old'\n", + path: []string{"a", "b", "c", "leaf"}, + value: "new", + want: "a:\n b:\n c:\n leaf: 'new'\n", + outcome: OutcomeReplaced, + }, + { + name: "three levels deep, adding", + doc: "a:\n b:\n c:\n existing: 'x'\n", + path: []string{"a", "b", "c", "added"}, + value: "v", + want: "a:\n b:\n c:\n existing: 'x'\n added: 'v'\n", + outcome: OutcomeAdded, + }, + { + name: "adds into an empty block at header indent plus two", + doc: "block:\ntop:\n b: 'c'\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\n added: 'v'\ntop:\n b: 'c'\n", + outcome: OutcomeAdded, + }, + { + name: "replace preserves the line indent and trailing comment", + doc: "block:\n target: 'old' # rotated 2026-01\n", + path: []string{"block", "target"}, + value: "new", + want: "block:\n target: 'new' # rotated 2026-01\n", + outcome: OutcomeReplaced, + }, + { + name: "a hash inside a quoted value is data, not a comment", + doc: "block:\n target: 'pass#word'\n", + path: []string{"block", "target"}, + value: "new", + want: "block:\n target: 'new'\n", + outcome: OutcomeReplaced, + }, + { + name: "CRLF document gets a CRLF insertion", + doc: "block:\r\n only: 'x'\r\n", + path: []string{"block", "added"}, + value: "v", + want: "block:\r\n only: 'x'\r\n added: 'v'\r\n", + outcome: OutcomeAdded, + }, + { + name: "a comment inside the block does not terminate it", + doc: "block:\n first: 'a'\n # a note about the next key\n second: 'b'\n", + path: []string{"block", "second"}, + value: "new", + want: "block:\n first: 'a'\n # a note about the next key\n second: 'new'\n", + outcome: OutcomeReplaced, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, outcome, err := SetScalar([]byte(c.doc), c.path, c.value) + if err != nil { + t.Fatalf("SetScalar: %v", err) + } + if string(got) != c.want { + t.Errorf("document =\n%q\nwant\n%q", got, c.want) + } + if outcome != c.outcome { + t.Errorf("outcome = %v, want %v", outcome, c.outcome) + } + }) + } +} + +func TestSetScalar_Refusals(t *testing.T) { + cases := []struct { + name string + doc string + path []string + wantMsg string + }{ + { + name: "a missing block", + doc: "other:\n k: 'v'\n", + path: []string{"nosuchblock", "s3ntinel_leaf"}, + wantMsg: "no block", + }, + { + name: "a missing intermediate block", + doc: "a:\n other:\n k: 'v'\n", + path: []string{"a", "missing", "s3ntinel_leaf"}, + wantMsg: "no block", + }, + { + name: "a sequence where a mapping was expected", + doc: "block:\n - one\n - two\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "sequence", + }, + { + name: "a comment indented at or below the header", + doc: "block:\n# a column-zero note inside the block\n k: 'v'\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "ambiguous", + }, + { + name: "tab indentation", + doc: "block:\n\tk: 'v'\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "tab indentation", + }, + { + name: "a multi-document stream", + doc: "---\nblock:\n k: 'v'\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "multi-document", + }, + { + name: "a header carrying a trailing comment", + doc: "block: # notes\n k: 'v'\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "trailing comment", + }, + { + name: "an empty path", + doc: "block:\n k: 'v'\n", + path: nil, + wantMsg: "path is empty", + }, + { + // A leaf naming a block would otherwise write a scalar over the + // header and leave its children stranded at their old depth, which + // yaml.v3 rejects. The child's own parse catches that and the + // encrypted file survives, but the operator gets "the edited + // document does not parse as YAML" — a message that names nothing + // they can act on and reads as a forgectl bug. + name: "a leaf that names a block rather than a value", + doc: "block:\n s3ntinel_leaf:\n k: 'v'\n", + path: []string{"block", "s3ntinel_leaf"}, + wantMsg: "names a block rather than a value", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + const sentinel = "s3ntinel-VALUE-77x" + got, outcome, err := SetScalar([]byte(c.doc), c.path, sentinel) + if err == nil { + t.Fatalf("SetScalar returned nil error, want a refusal; document =\n%q", got) + } + if !strings.Contains(err.Error(), c.wantMsg) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.wantMsg) + } + if outcome != OutcomeUnspecified { + t.Errorf("outcome = %v, want unspecified on a refusal", outcome) + } + // A refusal must never echo the value. + if strings.Contains(err.Error(), sentinel) { + t.Errorf("error %q echoed the value", err.Error()) + } + // Nor the LEAF segment. ParsePath's grammar admits plenty of + // provider token formats as a single valid segment, so a secret + // pasted into the key slot arrives as the leaf — and these + // messages are relayed out of the child process to the operator's + // terminal, which is the transcript this feature exists to keep + // the value out of. + // + // The asymmetry with the ancestor segments is deliberate, not an + // oversight: "no block %q at this level" names a mistyped block, + // which is the commonest mistake on this path and the only + // actionable thing the message can carry. An ancestor is not the + // paste site — a bare pasted secret is a single-segment path, + // whose whole walk is the leaf. + if leaf := lastSegment(c.path); leaf != "" && strings.Contains(err.Error(), leaf) { + t.Errorf("error %q echoed the leaf segment %q", err.Error(), leaf) + } + if got != nil { + t.Errorf("document = %q, want nil on a refusal", got) + } + }) + } +} + +// TestSetScalar_QuotingRoundTrips asserts the emitted document DECODES to the +// value that went in, rather than asserting the emitted line's text. +// +// That distinction is the whole point: comparing against `key: 'a”b'` would +// merely restate encodeScalar's implementation, so the test would agree with +// the code by construction and could not catch a quoting bug. Decoding with +// yaml.v3 asks the only question that matters — does a real YAML reader get +// the bytes back. +func TestSetScalar_QuotingRoundTrips(t *testing.T) { + values := []string{ + "plain", + "with: a colon", + "with # a hash", + "with 'single' quotes", + "it's got an apostrophe", + `with "double" quotes`, + "{braces: true}", + "*alias-looking", + "&anchor-looking", + "- dash-leading", + "[brackets]", + "trailing spaces ", + " leading spaces", + "100%", + "@at-leading", + "`backtick`", + "ENC[AES256_GCM,data:abc]", + "multi internal spaces", + "\\backslash\\path", + "value # with ' quote", + } + + for _, v := range values { + t.Run(v, func(t *testing.T) { + doc := "block:\n target: 'placeholder'\n" + got, _, err := SetScalar([]byte(doc), []string{"block", "target"}, v) + if err != nil { + t.Fatalf("SetScalar: %v", err) + } + + var decoded struct { + Block struct { + Target string `yaml:"target"` + } `yaml:"block"` + } + if err := yaml.Unmarshal(got, &decoded); err != nil { + t.Fatalf("the emitted document does not parse as YAML: %v\n%s", err, got) + } + if decoded.Block.Target != v { + t.Errorf("decoded = %q, want %q\nemitted:\n%s", decoded.Block.Target, v, got) + } + }) + } +} + +// TestSetScalar_AddedKeyRoundTrips is the same round-trip for the ADD path, +// which builds its line separately from the replace path. +func TestSetScalar_AddedKeyRoundTrips(t *testing.T) { + const value = "added ' # value" + doc := "block:\n other: 'x'\n" + got, outcome, err := SetScalar([]byte(doc), []string{"block", "added"}, value) + if err != nil { + t.Fatalf("SetScalar: %v", err) + } + if outcome != OutcomeAdded { + t.Fatalf("outcome = %v, want added", outcome) + } + + var decoded struct { + Block map[string]string `yaml:"block"` + } + if err := yaml.Unmarshal(got, &decoded); err != nil { + t.Fatalf("the emitted document does not parse as YAML: %v\n%s", err, got) + } + if decoded.Block["added"] != value { + t.Errorf("decoded = %q, want %q", decoded.Block["added"], value) + } + if decoded.Block["other"] != "x" { + t.Errorf("the untouched sibling decoded as %q, want %q", decoded.Block["other"], "x") + } +} + +// TestSetScalar_ColonBearingSiblingIsNotDestroyed is the regression test for a +// silent data-loss bug: a bare prefix match on `leaf + ":"` also matched a +// DIFFERENT key whose name merely began that way. +// +// `a:b: 'v'` is valid YAML and decodes to the key `a:b`, so it can legitimately +// sit in a SOPS file. Setting `a` matched that line, and since replaceValue +// cuts at the first colon, the result was `a: 'new'` — the key `a:b` and its +// encrypted value gone, reported as `replaced a`, and passing every downstream +// check, because extracting `["block"]["a"]` then returns exactly the value +// that was supplied. +// +// So this asserts the ORIGINAL key survives, not merely that the write +// happened. +func TestSetScalar_ColonBearingSiblingIsNotDestroyed(t *testing.T) { + const doc = "block:\n a:b: 'keepme'\n" + + got, outcome, err := SetScalar([]byte(doc), []string{"block", "a"}, "new") + if err != nil { + // Refusing is also acceptable — the point is that `a:b` is not eaten. + t.Logf("refused: %v", err) + return + } + if !strings.Contains(string(got), "a:b:") { + t.Fatalf("the key `a:b` was destroyed (outcome %v):\n%s", outcome, got) + } + + var decoded struct { + Block map[string]string `yaml:"block"` + } + if err := yaml.Unmarshal(got, &decoded); err != nil { + t.Fatalf("the emitted document does not parse: %v\n%s", err, got) + } + if decoded.Block["a:b"] != "keepme" { + t.Errorf("block[\"a:b\"] = %q, want %q — the original key lost its value", decoded.Block["a:b"], "keepme") + } + if decoded.Block["a"] != "new" { + t.Errorf("block[\"a\"] = %q, want %q", decoded.Block["a"], "new") + } +} + +// TestSetScalar_NoSpaceAfterColonIsNotAMapping pins the sibling case: YAML +// reads `key:value` with no space as a plain scalar, not a mapping, so it must +// not be matched as a key either. +func TestSetScalar_NoSpaceAfterColonIsNotAMapping(t *testing.T) { + const doc = "block:\n keep:value\n" + + got, _, err := SetScalar([]byte(doc), []string{"block", "keep"}, "new") + if err != nil { + t.Logf("refused: %v", err) + return + } + if !strings.Contains(string(got), "keep:value") { + t.Errorf("the `keep:value` scalar was rewritten as a key:\n%s", got) + } +} + +// TestSetScalar_NoDuplicateKeyOnReplace guards the failure that a mis-bounded +// block produces: a key added a second time. A duplicate YAML key is accepted +// by many parsers with a last-wins rule, so it does not fail loudly — it just +// makes the file's meaning depend on the reader. +func TestSetScalar_NoDuplicateKeyOnReplace(t *testing.T) { + doc := "block:\n first: 'a'\n # note\n target: 'old'\n\nsops:\n mac: 'ENC[x]'\n" + got, _, err := SetScalar([]byte(doc), []string{"block", "target"}, "new") + if err != nil { + t.Fatalf("SetScalar: %v", err) + } + if n := strings.Count(string(got), "target:"); n != 1 { + t.Errorf("document has %d `target:` lines, want 1:\n%s", n, got) + } + // yaml.v3 rejects a duplicate mapping key outright, so a successful + // decode is a second, independent check on the same property. + var probe map[string]any + if err := yaml.Unmarshal(got, &probe); err != nil { + t.Errorf("the emitted document does not parse (duplicate key?): %v\n%s", err, got) + } +} + +// lastSegment returns path's leaf, or "" for an empty path — the segment a +// pasted secret would occupy, which no refusal may echo. +func lastSegment(path []string) string { + if len(path) == 0 { + return "" + } + return path[len(path)-1] +} diff --git a/internal/sops/file.go b/internal/sops/file.go new file mode 100644 index 00000000..f7af74a2 --- /dev/null +++ b/internal/sops/file.go @@ -0,0 +1,236 @@ +package sops + +import ( + "errors" + "fmt" + "path/filepath" + "regexp" + "strings" + + "gopkg.in/yaml.v3" +) + +// maxRuleBytes bounds a suffix or regex read out of a file's own metadata. +// The value is file content, so it is attacker-influenced in a cloned +// repository; a rule longer than this is not a rule anyone wrote. +const maxRuleBytes = 1024 + +// metadata is the plaintext half of a SOPS document. The sops: block is never +// encrypted — that is what lets every check in this file run without a key, +// a subprocess, or a decryption. +type metadata struct { + Sops struct { + UnencryptedSuffix string `yaml:"unencrypted_suffix"` + EncryptedSuffix string `yaml:"encrypted_suffix"` + EncryptedRegex string `yaml:"encrypted_regex"` + UnencryptedRegex string `yaml:"unencrypted_regex"` + Mac string `yaml:"mac"` + Version string `yaml:"version"` + } `yaml:"sops"` +} + +// IsSOPSFile reports whether data parses as YAML and carries a top-level +// `sops:` mapping. +// +// It is a NARROWING check, never the whole gate: the caller also requires the +// filename to match the sops shapes below, and both must hold. A name test +// alone would accept a plain YAML file someone called secrets.sops.yaml; a +// content test alone would accept any encrypted document anywhere in the +// repository, which is exactly the widening the env-file allowlist exists to +// prevent. +func IsSOPSFile(data []byte) bool { + var probe map[string]yaml.Node + if err := yaml.Unmarshal(data, &probe); err != nil { + return false + } + node, ok := probe["sops"] + return ok && node.Kind == yaml.MappingNode +} + +// sopsNamePatterns is the filename allowlist for the --sops route. +// +// It is a separate list from internal/env's IsEnvFileName and deliberately so: +// a SOPS document is not an env file, and admitting one through the env +// allowlist would have meant widening a rule that exists to stop `env set` +// becoming a writer of arbitrary repository files. The reasoning carries over +// unchanged — repo-containment alone is not a bound worth having, because +// .git/config is inside the repository too. +// +// There is no --any-file escape hatch on this route. A SOPS file under some +// other name is unreachable, which is a deliberate refusal rather than a gap: +// the alternative is a confirmation prompt, and the confirmation path is the +// one that carried a time-of-check/time-of-use defect. Refusing is honest and +// costs a rename. +var sopsNamePatterns = []string{ + "*.sops.yaml", + "*.sops.yml", + "*.enc.yaml", + "*.enc.yml", + "secrets.yaml", + "secrets.yml", + "secrets.*.yaml", + "secrets.*.yml", +} + +// NameShapes renders the allowlist for an error message, so the refusal tells +// the operator what would have been accepted instead of just saying no. +func NameShapes() string { return strings.Join(sopsNamePatterns, ", ") } + +// IsSOPSFileName reports whether base — a basename, not a path — matches one +// of the allowed shapes. Matching is byte-exact, which on a case-insensitive +// filesystem means `SECRETS.YAML` is refused. That fails toward refusing a +// legitimate file rather than admitting an unintended one, which is the +// direction this check must err in. +func IsSOPSFileName(base string) bool { + for _, pattern := range sopsNamePatterns { + // filepath.Match's only error is a malformed pattern, and every + // pattern here is a literal in this file. + if ok, _ := filepath.Match(pattern, base); ok { + return true + } + } + return false +} + +// PlaintextRules decides whether a given key would be stored in CLEARTEXT by +// this file's own encryption rules. +type PlaintextRules struct { + unencryptedSuffix string + encryptedSuffix string + encryptedRegex *regexp.Regexp + unencryptedRegex *regexp.Regexp +} + +// ReadPlaintextRules extracts the encryption rules from a document's sops +// metadata. +// +// # Why this check exists at all +// +// sops applies these rules per key, and a key the rules exclude is written to +// the file IN THE CLEAR next to its encrypted siblings. Measured live on +// 3.13.3: with `unencrypted_suffix: _unencrypted` in force, a key named +// `foo_unencrypted` sat in plaintext while its neighbour read +// `ENC[AES256_GCM,...]`. So without this check, `env set` would cheerfully +// accept a secret and store it unencrypted while reporting success — and the +// decrypt-round-trip verification would pass, because a cleartext value +// round-trips perfectly. +// +// Every encrypted file in the estate this feature targets carries +// `unencrypted_suffix: _unencrypted`, so this is live on the exact files the +// feature is for, not a hypothetical. +func ReadPlaintextRules(data []byte) (PlaintextRules, error) { + var meta metadata + if err := yaml.Unmarshal(data, &meta); err != nil { + return PlaintextRules{}, errors.New("file does not parse as YAML") + } + + rules := PlaintextRules{ + unencryptedSuffix: meta.Sops.UnencryptedSuffix, + encryptedSuffix: meta.Sops.EncryptedSuffix, + } + + // sops' own default when a file configures no other rule. Applying it + // here rather than treating "no rule" as "everything is encrypted" is + // the fail-closed direction: a file that relies on the default would + // otherwise have its _unencrypted keys accepted. + if rules.unencryptedSuffix == "" && rules.encryptedSuffix == "" && + meta.Sops.EncryptedRegex == "" && meta.Sops.UnencryptedRegex == "" { + rules.unencryptedSuffix = "_unencrypted" + } + + var err error + if rules.encryptedRegex, err = compileRule(meta.Sops.EncryptedRegex, "encrypted_regex"); err != nil { + return PlaintextRules{}, err + } + if rules.unencryptedRegex, err = compileRule(meta.Sops.UnencryptedRegex, "unencrypted_regex"); err != nil { + return PlaintextRules{}, err + } + if len(rules.unencryptedSuffix) > maxRuleBytes || len(rules.encryptedSuffix) > maxRuleBytes { + return PlaintextRules{}, errors.New("the file's encryption-suffix rule is implausibly long") + } + + return rules, nil +} + +// compileRule compiles a regex read from file metadata. +// +// Go's regexp is RE2, which is linear-time and has no catastrophic +// backtracking, so an adversarial pattern from a cloned repository cannot +// turn this into a denial of service. The length bound is about plausibility +// rather than safety. +func compileRule(pattern, field string) (*regexp.Regexp, error) { + if pattern == "" { + return nil, nil + } + if len(pattern) > maxRuleBytes { + return nil, fmt.Errorf("the file's %s is implausibly long", field) + } + re, err := regexp.Compile(pattern) + if err != nil { + // Names the field, never the pattern: the pattern is file content and + // the field is what an operator needs in order to go fix it. + return nil, fmt.Errorf("the file's %s does not compile", field) + } + return re, nil +} + +// WouldStoreCleartext reports whether key falls outside this file's encryption +// rules, and why. +// +// # Why it takes the whole path and not just the leaf +// +// sops applies these rules to a key AND ITS WHOLE SUBTREE, so an ancestor +// decides the outcome for everything beneath it. Measured on 3.13.3: +// +// unencrypted_suffix: _unencrypted → notes_unencrypted.token CLEARTEXT +// encrypted_regex: ^app$ → app.token, app.inner.deep both ENCRYPTED +// +// Testing the leaf alone gets both cases wrong, in opposite directions. A path +// whose PARENT carries the suffix passes the check and the secret lands in +// plaintext, reported as success — the exact failure this function exists to +// prevent. And an ancestor-scoped encrypted_regex falsely refuses every key +// beneath the block it matches, which makes the feature unusable on such a +// file. +// +// The parameter is []string rather than a string so the leaf-only call cannot +// be written again by accident. That is the real fix; the walk is its +// consequence. +// +// Every reason names the RULE and the field it came from — never a segment, +// which may itself be a secret pasted into the wrong slot. +func (r PlaintextRules) WouldStoreCleartext(path []string) (bool, string) { + // An unencrypted rule matching ANY segment wins: everything beneath that + // segment is excluded from encryption, and this key is beneath it. + for _, segment := range path { + if r.unencryptedSuffix != "" && strings.HasSuffix(segment, r.unencryptedSuffix) { + return true, fmt.Sprintf("a key on this path ends with the file's unencrypted_suffix (%q)", r.unencryptedSuffix) + } + if r.unencryptedRegex != nil && r.unencryptedRegex.MatchString(segment) { + return true, "a key on this path matches the file's unencrypted_regex" + } + } + + // An encrypted rule is an allowlist, and a match at an ANCESTOR covers the + // subtree — so it is satisfied when any segment matches, and violated only + // when none does. + if r.encryptedSuffix != "" && !anySegment(path, func(s string) bool { + return strings.HasSuffix(s, r.encryptedSuffix) + }) { + return true, fmt.Sprintf("the file encrypts only keys ending with its encrypted_suffix (%q), and no key on this path does", r.encryptedSuffix) + } + if r.encryptedRegex != nil && !anySegment(path, r.encryptedRegex.MatchString) { + return true, "no key on this path matches the file's encrypted_regex" + } + + return false, "" +} + +// anySegment reports whether pred holds for at least one segment. +func anySegment(path []string, pred func(string) bool) bool { + for _, segment := range path { + if pred(segment) { + return true + } + } + return false +} diff --git a/internal/sops/file_test.go b/internal/sops/file_test.go new file mode 100644 index 00000000..8f264e78 --- /dev/null +++ b/internal/sops/file_test.go @@ -0,0 +1,337 @@ +package sops + +// Test plan for file.go +// +// IsSOPSFile +// [x] True for a document with a top-level sops: mapping +// [x] False for plain YAML, for invalid YAML, for a `sops:` that is a +// scalar or a sequence rather than a mapping, and for a nested one +// +// IsSOPSFileName +// [x] Accepted: the eight allowed shapes +// [x] Refused: .env names, a bare .yaml, a different case, a path rather +// than a basename +// +// ReadPlaintextRules / WouldStoreCleartext +// [x] unencrypted_suffix refuses a matching key and admits others +// [x] The _unencrypted DEFAULT applies when the file configures no rule +// [x] encrypted_suffix refuses a key that does NOT match +// [x] encrypted_regex refuses a key that does NOT match +// [x] unencrypted_regex refuses a key that DOES match +// [x] A reason names the rule, never the key +// [x] An uncompilable or implausibly long rule refuses + +import ( + "strings" + "testing" +) + +// segmentEchoed reports whether reason contains any path segment. Every +// segment is a candidate secret — the sops path grammar admits hyphens and so +// matches more real credential shapes than internal/env's ValidKey — so a +// reason must name the RULE and never the input. +func segmentEchoed(reason string, path []string) bool { + for _, segment := range path { + if strings.Contains(reason, segment) { + return true + } + } + return false +} + +func TestIsSOPSFile(t *testing.T) { + cases := []struct { + name string + doc string + want bool + }{ + { + name: "a real sops document", + doc: "key: ENC[AES256_GCM,data:abc]\nsops:\n mac: ENC[x]\n version: 3.13.3\n", + want: true, + }, + { + name: "plain yaml", + doc: "key: value\nother: thing\n", + want: false, + }, + { + name: "invalid yaml", + doc: "key: [unclosed\n", + want: false, + }, + { + name: "sops as a scalar", + doc: "sops: not-a-mapping\n", + want: false, + }, + { + name: "sops as a sequence", + doc: "sops:\n - one\n - two\n", + want: false, + }, + { + name: "sops nested rather than top level", + doc: "outer:\n sops:\n mac: ENC[x]\n", + want: false, + }, + { + name: "empty", + doc: "", + want: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := IsSOPSFile([]byte(c.doc)); got != c.want { + t.Errorf("IsSOPSFile = %v, want %v", got, c.want) + } + }) + } +} + +func TestIsSOPSFileName(t *testing.T) { + cases := []struct { + base string + want bool + }{ + {"secrets.sops.yaml", true}, + {"secrets.sops.yml", true}, + {"anything.sops.yaml", true}, + {"prod.enc.yaml", true}, + {"prod.enc.yml", true}, + {"secrets.yaml", true}, + {"secrets.yml", true}, + {"secrets.prod.yaml", true}, + {"secrets.staging.yml", true}, + + {".env", false}, + {".env.prod", false}, + {"prod.env", false}, + {"values.yaml", false}, + {"config.yml", false}, + {"secrets", false}, + {"secretsyaml", false}, + {"Makefile", false}, + {".git/config", false}, + // Byte-exact on purpose. APFS is case-insensitive, so this names the + // same file as secrets.yaml — refusing it errs toward refusing a + // legitimate file rather than admitting an unintended one. + {"SECRETS.YAML", false}, + // A path, not a basename. The caller passes filepath.Base; this + // pins that a full path does not sneak through a wildcard. + {"nested/secrets.yaml", false}, + } + for _, c := range cases { + t.Run(c.base, func(t *testing.T) { + if got := IsSOPSFileName(c.base); got != c.want { + t.Errorf("IsSOPSFileName(%q) = %v, want %v", c.base, got, c.want) + } + }) + } +} + +func TestWouldStoreCleartext(t *testing.T) { + cases := []struct { + name string + doc string + path []string + wantClear bool + wantReason string + }{ + // The ANCESTOR cases are the reason this takes a path rather than a + // leaf, and they were a live defect: a secret landed in plaintext and + // the command reported success. Measured against sops 3.13.3 — + // `unencrypted_suffix: _unencrypted` leaves the whole subtree under + // `notes_unencrypted` in the clear, and `encrypted_regex: ^app$` + // encrypts everything under `app` however deep. + { + name: "an ancestor carries the unencrypted_suffix", + doc: "sops:\n unencrypted_suffix: _unencrypted\n mac: ENC[x]\n", + path: []string{"notes_unencrypted", "token"}, + wantClear: true, + wantReason: "unencrypted_suffix", + }, + { + name: "a middle segment carries the unencrypted_suffix", + doc: "sops:\n unencrypted_suffix: _unencrypted\n mac: ENC[x]\n", + path: []string{"app", "notes_unencrypted", "deep", "token"}, + wantClear: true, + wantReason: "unencrypted_suffix", + }, + { + name: "an ancestor satisfies the encrypted_regex", + doc: "sops:\n encrypted_regex: '^app$'\n mac: ENC[x]\n", + path: []string{"app", "token"}, + wantClear: false, + }, + { + name: "a distant ancestor satisfies the encrypted_regex", + doc: "sops:\n encrypted_regex: '^app$'\n mac: ENC[x]\n", + path: []string{"app", "inner", "deep"}, + wantClear: false, + }, + { + name: "no segment satisfies the encrypted_regex", + doc: "sops:\n encrypted_regex: '^app$'\n mac: ENC[x]\n", + path: []string{"other", "token"}, + wantClear: true, + wantReason: "encrypted_regex", + }, + { + name: "an ancestor satisfies the encrypted_suffix", + doc: "sops:\n encrypted_suffix: _secret\n mac: ENC[x]\n", + path: []string{"api_secret", "token"}, + wantClear: false, + }, + { + name: "an ancestor matches the unencrypted_regex", + doc: "sops:\n unencrypted_regex: '^public_'\n mac: ENC[x]\n", + path: []string{"public_block", "token"}, + wantClear: true, + wantReason: "unencrypted_regex", + }, + { + name: "unencrypted_suffix matches", + doc: "sops:\n unencrypted_suffix: _unencrypted\n mac: ENC[x]\n", + path: []string{"foo_unencrypted"}, + wantClear: true, + wantReason: "unencrypted_suffix", + }, + { + name: "unencrypted_suffix does not match", + doc: "sops:\n unencrypted_suffix: _unencrypted\n mac: ENC[x]\n", + path: []string{"llm_key_hermes"}, + wantClear: false, + }, + { + // sops' own default when a file configures nothing else. Treating + // "no rule" as "everything is encrypted" would accept a key the + // real sops would write in the clear. + name: "the _unencrypted default applies with no rule configured", + doc: "sops:\n mac: ENC[x]\n version: 3.13.3\n", + path: []string{"token_unencrypted"}, + wantClear: true, + wantReason: "unencrypted_suffix", + }, + { + name: "the default admits an ordinary key", + doc: "sops:\n mac: ENC[x]\n", + path: []string{"ordinary_key"}, + wantClear: false, + }, + { + name: "encrypted_suffix refuses a non-matching key", + doc: "sops:\n encrypted_suffix: _secret\n mac: ENC[x]\n", + path: []string{"plain_key"}, + wantClear: true, + wantReason: "encrypted_suffix", + }, + { + name: "encrypted_suffix admits a matching key", + doc: "sops:\n encrypted_suffix: _secret\n mac: ENC[x]\n", + path: []string{"api_secret"}, + wantClear: false, + }, + { + name: "encrypted_regex refuses a non-matching key", + doc: "sops:\n encrypted_regex: '^(data|token)$'\n mac: ENC[x]\n", + path: []string{"other"}, + wantClear: true, + wantReason: "encrypted_regex", + }, + { + name: "encrypted_regex admits a matching key", + doc: "sops:\n encrypted_regex: '^(data|token)$'\n mac: ENC[x]\n", + path: []string{"token"}, + wantClear: false, + }, + { + name: "unencrypted_regex refuses a matching key", + doc: "sops:\n unencrypted_regex: '^public_'\n mac: ENC[x]\n", + path: []string{"public_url"}, + wantClear: true, + wantReason: "unencrypted_regex", + }, + { + name: "unencrypted_regex admits a non-matching key", + doc: "sops:\n unencrypted_regex: '^public_'\n mac: ENC[x]\n", + path: []string{"private_token"}, + wantClear: false, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + rules, err := ReadPlaintextRules([]byte(c.doc)) + if err != nil { + t.Fatalf("ReadPlaintextRules: %v", err) + } + isClear, reason := rules.WouldStoreCleartext(c.path) + if isClear != c.wantClear { + t.Fatalf("WouldStoreCleartext(%v) = %v (%q), want %v", c.path, isClear, reason, c.wantClear) + } + if !isClear { + return + } + if !strings.Contains(reason, c.wantReason) { + t.Errorf("reason = %q, want it to name %q", reason, c.wantReason) + } + // The reason names the RULE, never the key — a key slot holds a + // pasted secret often enough that this has to hold here too. + if segmentEchoed(reason, c.path) { + t.Errorf("reason %q echoed the key", reason) + } + }) + } +} + +func TestReadPlaintextRules_Refusals(t *testing.T) { + cases := []struct { + name string + doc string + want string + }{ + { + name: "invalid yaml", + doc: "sops: [unclosed\n", + want: "does not parse", + }, + { + name: "an uncompilable encrypted_regex", + doc: "sops:\n encrypted_regex: '('\n", + want: "encrypted_regex does not compile", + }, + { + name: "an uncompilable unencrypted_regex", + doc: "sops:\n unencrypted_regex: '[z-a]'\n", + want: "unencrypted_regex does not compile", + }, + { + name: "an implausibly long regex", + doc: "sops:\n encrypted_regex: '" + strings.Repeat("a", maxRuleBytes+1) + "'\n", + want: "implausibly long", + }, + { + name: "an implausibly long suffix", + doc: "sops:\n unencrypted_suffix: '" + strings.Repeat("a", maxRuleBytes+1) + "'\n", + want: "implausibly long", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, err := ReadPlaintextRules([]byte(c.doc)) + if err == nil { + t.Fatal("ReadPlaintextRules returned nil error, want a refusal") + } + if !strings.Contains(err.Error(), c.want) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.want) + } + // The rule is file content, which in a cloned repository is + // attacker-supplied. The message names the FIELD, not the value. + if strings.Contains(err.Error(), strings.Repeat("a", 32)) { + t.Errorf("error %q echoed the rule's value", err.Error()) + } + }) + } +} diff --git a/internal/sops/path.go b/internal/sops/path.go new file mode 100644 index 00000000..8a88583a --- /dev/null +++ b/internal/sops/path.go @@ -0,0 +1,101 @@ +// Package sops writes one scalar into a SOPS-encrypted YAML document without +// the value ever reaching an argv, a terminal, or a transcript. +// +// The package splits into a pure half and an effectful half. path.go, value.go, +// edit.go, and file.go make every decision — what a key path may look like, +// what a value may contain, which line to overwrite, whether the target is a +// SOPS document at all — and touch nothing. driver.go is the thin wrapper that +// runs the `sops` binary and the filesystem around those decisions. +// +// The value's route deserves stating once, because it is the reason the package +// exists: it arrives on stdin, a no-echo prompt, or the clipboard; it is written +// to a 0600 file; the FILE'S PATH travels in the child's environment and the +// value never does; the child reads the file and edits the decrypted document +// sops handed it. At no point is the value an argument to any process. +package sops + +import ( + "errors" + "regexp" + "strings" +) + +// maxPathBytes bounds a dotted path before it is split. A real key path is a +// handful of identifiers; anything near this is a caller mistake, and bounding +// it early keeps a pathological input away from the splitter and the walk. +const maxPathBytes = 1024 + +// maxPathSegments bounds the walk depth for the same reason. +const maxPathSegments = 16 + +// segmentPattern is the grammar one path segment must match. It is +// deliberately WIDER than internal/env's ValidKey (which forbids the hyphen), +// because real YAML keys in estate secrets files carry hyphens — and that +// width is exactly why a refusal here must never echo the argument. Plenty of +// provider token formats parse as a single valid segment, so a secret pasted +// into the key slot reaches this grammar and passes it. +var segmentPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_-]*$`) + +// PathGrammar is the rule ParsePath enforces, as prose for an error message. +// The message names the rule and never the input; see errBadPath. +const PathGrammar = "[A-Za-z0-9_][A-Za-z0-9_-]* separated by '.'" + +// errBadPath is the single refusal for every malformed path. One message for +// every cause is deliberate: a per-cause message ("segment 2 is empty", "a +// segment contains a dot") is a side channel that describes the rejected +// input, and the rejected input may be a secret. The rule is actionable on its +// own — a caller who reads it can see what shape was wanted. +func errBadPath() error { + return errors.New("path segments must match " + PathGrammar) +} + +// ParsePath splits a dotted key path into its segments. +// +// A key whose own name contains a dot is unreachable by design, and that is a +// deliberate refusal rather than a gap: a dotted string cannot distinguish the +// document {a: {b.c: v}} from {a: {b: {c: v}}}, so any escaping scheme would be +// guessing which one the caller meant. Refusing is honest; the alternative is +// an escaping surface on every path for a case no estate secrets file has. +func ParsePath(path string) ([]string, error) { + if path == "" || len(path) > maxPathBytes { + return nil, errBadPath() + } + segments := strings.Split(path, ".") + if len(segments) > maxPathSegments { + return nil, errBadPath() + } + for _, segment := range segments { + if !segmentPattern.MatchString(segment) { + return nil, errBadPath() + } + } + // `sops` at the root is the metadata block: the recipients, the MAC, the + // encryption rules. Writing a scalar into it would corrupt the file's own + // bookkeeping. + // + // It is currently unreachable anyway, because sops omits that block from + // the buffer it hands the editor, so the walk refuses with "no block". But + // that is an accident of sops' behaviour rather than a rule, and a rule is + // what this should rest on. + if segments[0] == sopsMetadataKey { + return nil, errors.New("the top-level `sops` block holds the file's own encryption metadata and cannot be written") + } + return segments, nil +} + +// sopsMetadataKey is the root key holding a SOPS document's metadata. +const sopsMetadataKey = "sops" + +// JoinExtract renders segments as the bracketed address `sops --extract` +// takes: ["a"]["b"]. Every segment has already passed segmentPattern, which +// admits no quote, backslash, or bracket, so there is nothing here that could +// break out of the quoting — the grammar is the escaping. +func JoinExtract(segments []string) string { + var b strings.Builder + for _, segment := range segments { + b.WriteString(`["`) + b.WriteString(segment) + b.WriteString(`"]`) + } + return b.String() +} diff --git a/internal/sops/path_test.go b/internal/sops/path_test.go new file mode 100644 index 00000000..d6f66626 --- /dev/null +++ b/internal/sops/path_test.go @@ -0,0 +1,220 @@ +package sops + +// Test plan for path.go and value.go +// +// ParsePath +// [x] Accepted: one segment, several segments, digits, underscores, hyphens +// [x] Refused: empty, a leading hyphen, a dotted key name, an empty +// segment, spaces, shell metacharacters, quotes and brackets, an +// over-long path, too many segments +// [x] A refusal names the RULE and never the argument +// +// NormalizeValue +// [x] Strips exactly one trailing newline (LF and CRLF), and no more +// [x] Refused: empty, an interior newline, a C0 control byte, DEL, +// invalid UTF-8, over the size ceiling +// [x] A refusal never echoes the value +// [x] Interior whitespace and a tab survive + +import ( + "strings" + "testing" +) + +func TestParsePath_Accepted(t *testing.T) { + cases := []struct { + in string + want []string + }{ + {"key", []string{"key"}}, + {"block.key", []string{"block", "key"}}, + {"a.b.c.d", []string{"a", "b", "c", "d"}}, + {"agentgateway.llm_key_hermes", []string{"agentgateway", "llm_key_hermes"}}, + {"with-hyphen.also-one", []string{"with-hyphen", "also-one"}}, + {"_leading_underscore", []string{"_leading_underscore"}}, + {"digits123.4th", []string{"digits123", "4th"}}, + {"MixedCase.Key", []string{"MixedCase", "Key"}}, + } + for _, c := range cases { + t.Run(c.in, func(t *testing.T) { + got, err := ParsePath(c.in) + if err != nil { + t.Fatalf("ParsePath(%q): %v", c.in, err) + } + if len(got) != len(c.want) { + t.Fatalf("ParsePath(%q) = %v, want %v", c.in, got, c.want) + } + for i := range got { + if got[i] != c.want[i] { + t.Errorf("segment %d = %q, want %q", i, got[i], c.want[i]) + } + } + }) + } +} + +func TestParsePath_Refused(t *testing.T) { + cases := []struct { + name string + in string + }{ + {"empty", ""}, + {"a bare dot", "."}, + {"a trailing dot", "block."}, + {"a leading dot", ".key"}, + {"a doubled dot", "block..key"}, + {"a leading hyphen", "-flag"}, + {"a space", "block.my key"}, + {"a slash", "block/key"}, + {"a shell metacharacter", "block.key;rm"}, + {"a quote", `block."key"`}, + {"a bracket", "block.key[0]"}, + {"a dollar", "block.$key"}, + {"a newline", "block.key\nother"}, + {"too long", strings.Repeat("a", maxPathBytes+1)}, + {"too many segments", strings.TrimSuffix(strings.Repeat("a.", maxPathSegments+2), ".")}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := ParsePath(c.in) + if err == nil { + t.Fatalf("ParsePath(%q) = %v, want a refusal", c.in, got) + } + // The refusal names the rule. It must never echo the argument: + // this grammar admits hyphens, so it is WIDER than internal/env's + // ValidKey and matches more real credential shapes — a token + // pasted into the path slot by mistake is plausibly a secret. + // + // Checked only for inputs long enough to be distinctive. The + // grammar itself contains "." and "-", so a one-character input + // would "match" the message trivially and the assertion would be + // about the wording rather than about a leak. + if len(c.in) > 3 && strings.Contains(err.Error(), c.in) { + t.Errorf("error %q echoed the argument", err.Error()) + } + if !strings.Contains(err.Error(), PathGrammar) { + t.Errorf("error = %q, want it to state the grammar", err.Error()) + } + }) + } +} + +// TestParsePath_DottedKeyIsUnreachable pins the deliberate limitation rather +// than leaving it implicit: a real key containing a dot cannot be addressed, +// because a dotted string cannot tell {a: {b.c: v}} from {a: {b: {c: v}}}. +// Refusing is honest; guessing is not. +func TestParsePath_DottedKeyIsUnreachable(t *testing.T) { + if _, err := ParsePath(`block.my\.key`); err == nil { + t.Error("a backslash-escaped dot was accepted; escaping is deliberately not supported") + } +} + +func TestJoinExtract(t *testing.T) { + got := JoinExtract([]string{"agentgateway", "llm_key"}) + if want := `["agentgateway"]["llm_key"]`; got != want { + t.Errorf("JoinExtract = %q, want %q", got, want) + } +} + +func TestNormalizeValue_Accepted(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"plain", "secret", "secret"}, + {"one trailing LF stripped", "secret\n", "secret"}, + {"one trailing CRLF stripped", "secret\r\n", "secret"}, + {"interior spaces survive", "a b c", "a b c"}, + {"a tab survives", "a\tb", "a\tb"}, + {"leading whitespace survives", " padded", " padded"}, + {"a hash survives", "pass#word", "pass#word"}, + {"a quote survives", "it's", "it's"}, + {"unicode survives", "pässwörd-日本", "pässwörd-日本"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := NormalizeValue(c.in) + if err != nil { + t.Fatalf("NormalizeValue: %v", err) + } + if got != c.want { + t.Errorf("NormalizeValue(%q) = %q, want %q", c.in, got, c.want) + } + }) + } +} + +func TestNormalizeValue_Refused(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"empty", "", "empty value"}, + {"only a newline", "\n", "empty value"}, + // Exactly ONE trailing newline is stripped, never more. A second one + // is interior once the first is gone, so it refuses — a greedy trim + // would instead silently alter a value whose real last byte is a + // newline. + {"two trailing newlines", "secret\n\n", "single line"}, + {"an interior newline", "two\nlines", "single line"}, + {"an interior CR", "two\rlines", "single line"}, + {"a NUL byte", "before\x00after", "control character"}, + {"an escape byte", "before\x1bafter", "control character"}, + {"a DEL byte", "before\x7fafter", "control character"}, + {"invalid UTF-8", "bad\xff\xfebytes", "valid UTF-8"}, + {"over the ceiling", strings.Repeat("a", maxValueBytes+1), "ceiling"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := NormalizeValue(c.in) + if err == nil { + t.Fatalf("NormalizeValue accepted %q, want a refusal", c.in) + } + if !strings.Contains(err.Error(), c.want) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.want) + } + if got != "" { + t.Errorf("NormalizeValue returned %q on a refusal, want the empty string", got) + } + // The whole input here is a candidate secret, so no refusal may + // carry any of it. Checked on a distinctive slice rather than the + // whole string, since a one-character input would match trivially. + if len(c.in) > 4 && strings.Contains(err.Error(), c.in[:4]) { + t.Errorf("error %q echoed part of the value", err.Error()) + } + }) + } +} + +// TestNormalizeValue_ControlByteIsWhatStopsTheLoop records why the control-byte +// refusal is a correctness requirement and not hygiene. +// +// YAML forbids C0 bytes outside tab and LF even inside a single-quoted scalar, +// so a value carrying one produces a document sops cannot parse — and sops +// responds to an unparseable document by re-invoking its editor forever. +// Measured on 3.13.3: 36,851 invocations and 8.4 MB of stderr in three +// minutes, still going when it was killed. +func TestNormalizeValue_ControlByteIsWhatStopsTheLoop(t *testing.T) { + // The byte is placed in the MIDDLE, so a trailing-newline strip cannot + // absorb it. A trailing \n is a legitimate artifact of the producing + // command and is stripped; an interior one is not, and that asymmetry is + // tested separately above. + for b := 0; b < 0x20; b++ { + in := "before" + string(rune(b)) + "after" + _, err := NormalizeValue(in) + if b == '\t' { + if err != nil { + t.Errorf("tab (0x%02x) was refused: %v — YAML permits it in a scalar", b, err) + } + continue + } + if err == nil { + t.Errorf("control byte 0x%02x was accepted, want a refusal", b) + } + } + if _, err := NormalizeValue("before\x7fafter"); err == nil { + t.Error("DEL (0x7f) was accepted, want a refusal") + } +} diff --git a/internal/sops/value.go b/internal/sops/value.go new file mode 100644 index 00000000..c1c028ce --- /dev/null +++ b/internal/sops/value.go @@ -0,0 +1,93 @@ +package sops + +import ( + "errors" + "strings" + "unicode/utf8" +) + +// maxValueBytes bounds one scalar. SOPS documents hold credentials, not blobs; +// a value past this is a wrong-input mistake (a whole file piped in by +// accident), and bounding it keeps a multi-megabyte paste out of the editor +// round-trip. +const maxValueBytes = 64 << 10 + +// NormalizeValue applies the input rules to a raw value and returns the scalar +// that will be written. +// +// Every refusal names the rule and never the value — the same discipline +// internal/env's errInvalidKey follows, for the same reason: this function's +// whole input is a secret, so a message that quoted it would write the secret +// into stderr and the session transcript, which is the one outcome the feature +// exists to prevent. +// +// The control-character and UTF-8 refusals are not hygiene. YAML forbids C0 +// control bytes outside tab and newline even inside a single-quoted scalar, so +// a value carrying one produces a document sops cannot parse — and a document +// sops cannot parse is the trigger for its unbounded editor re-invocation loop +// (measured on 3.13.3: 36,851 editor calls and 8.4 MB of stderr in three +// minutes, still going when it was killed). Refusing the byte here is the +// cheapest of the three brakes on that loop; __sops-edit's own YAML validation +// and its once-only counter are the other two. +func NormalizeValue(raw string) (string, error) { + if len(raw) > maxValueBytes { + return "", errors.New("value exceeds the 64KiB scalar ceiling") + } + + value := stripOneTrailingNewline(raw) + + switch { + case value == "": + return "", errors.New("empty value; refusing to set an empty value — edit the file directly if intended") + case !utf8.ValidString(value): + return "", errors.New("value is not valid UTF-8") + case strings.ContainsAny(value, "\n\r"): + return "", errors.New("value must be a single line; a scalar cannot hold a newline") + } + + for _, r := range value { + // Tab is the one C0 byte YAML permits in a scalar, and a real + // credential never carries it; DEL is not C0 but is equally + // unrenderable, so it refuses alongside them. + if (r < 0x20 && r != '\t') || r == 0x7F { + return "", errors.New("value contains a control character") + } + } + + return value, nil +} + +// stripOneTrailingNewline removes exactly one trailing "\n" or "\r\n" — what a +// piped value or a clipboard paste carries from the producing command's own +// line ending. Interior whitespace is never touched, and a value with no +// trailing newline (the interactive no-echo prompt) passes through unchanged. +// +// Exactly one, never a trim: a secret whose real last character is a newline is +// unusual but legal, and a greedy strip would silently corrupt it. This mirrors +// internal/env's own unexported stripTrailingNewline, which the .env path +// uses — the same rule, stated once per package rather than shared, because +// the packages are otherwise independent. +func stripOneTrailingNewline(s string) string { + if strings.HasSuffix(s, "\r\n") { + return s[:len(s)-2] + } + if strings.HasSuffix(s, "\n") { + return s[:len(s)-1] + } + return s +} + +// encodeScalar renders value as a single-quoted YAML scalar. +// +// Single quotes are the right container because YAML performs NO escape +// processing inside them: a backslash is a backslash, a `#` is not a comment, a +// leading `*` is not an alias, and `: ` is not a mapping. The one character +// that needs handling is the quote itself, which YAML escapes by doubling. +// That makes this function total over every input NormalizeValue admits. +// +// Verified by round-trip rather than by inspection: the tests re-parse the +// emitted document with yaml.v3 and compare the decoded scalar, so they assert +// the value survives rather than restating this function's own output shape. +func encodeScalar(value string) string { + return "'" + strings.ReplaceAll(value, "'", "''") + "'" +} diff --git a/internal/sops/verify_test.go b/internal/sops/verify_test.go new file mode 100644 index 00000000..19af4eff --- /dev/null +++ b/internal/sops/verify_test.go @@ -0,0 +1,129 @@ +package sops + +// Test plan for the encrypted-at-path assertion (driver.go) +// +// This is the check the whole feature's "proven encrypted" claim rests on, and +// the first version of it could not be made to go red on demand: it scanned +// for the first line whose trimmed text began with `leaf + ":"`, anywhere in +// the document, so a same-named encrypted key elsewhere satisfied it. +// +// [x] Passes when the scalar at exactly the path is ENC[ +// [x] REFUSES when the scalar at the path is cleartext, even though an +// encrypted same-named key appears EARLIER in the document — the +// document-order dependence that made the old check unsound +// [x] Refuses when it appears LATER too (order must not matter either way) +// [x] Refuses when `sops:`'s own encrypted `mac` would have satisfied a +// leaf named `mac` +// [x] Refuses a missing path, a non-mapping ancestor, and a non-scalar leaf +// [x] Refuses an unparseable document +// [x] No refusal echoes the cleartext value + +import ( + "strings" + "testing" +) + +func TestAssertEncryptedAtPath(t *testing.T) { + const secret = "s3ntinel-VALUE-77x" + + cases := []struct { + name string + doc string + path []string + wantErr string + }{ + { + name: "encrypted at the path", + doc: "app:\n token: ENC[AES256_GCM,data:abc,type:str]\n" + + "sops:\n mac: ENC[AES256_GCM,data:def,type:str]\n", + path: []string{"app", "token"}, + }, + { + // The case that proved the old line scan unsound. An encrypted + // `token` in `app` appears FIRST; the actual target is the + // cleartext `token` under `notes_unencrypted`. The old check + // matched app's line and passed, so the run reported success with + // the secret sitting in plaintext. + name: "cleartext at the path, encrypted same-named key earlier", + doc: "app:\n token: ENC[AES256_GCM,data:abc,type:str]\n" + + "notes_unencrypted:\n token: " + secret + "\n" + + "sops:\n mac: ENC[AES256_GCM,data:def,type:str]\n", + path: []string{"notes_unencrypted", "token"}, + wantErr: "written in the clear", + }, + { + // The mirror image. Order must not decide the verdict in either + // direction — the old check happened to be correct here, which is + // exactly what made the bug hard to see. + name: "cleartext at the path, encrypted same-named key later", + doc: "notes_unencrypted:\n token: " + secret + "\n" + + "app:\n token: ENC[AES256_GCM,data:abc,type:str]\n", + path: []string{"notes_unencrypted", "token"}, + wantErr: "written in the clear", + }, + { + // sops' own metadata block always contains an encrypted `mac`, so + // a leaf named `mac` had a guaranteed false pass available to it. + name: "a leaf named mac must not be satisfied by sops' own mac", + doc: "block:\n mac: " + secret + "\n" + + "sops:\n mac: ENC[AES256_GCM,data:def,type:str]\n", + path: []string{"block", "mac"}, + wantErr: "written in the clear", + }, + { + name: "the path is absent", + doc: "app:\n other: ENC[AES256_GCM,data:abc,type:str]\n", + path: []string{"app", "token"}, + wantErr: "could not be found", + }, + { + name: "an ancestor is not a mapping", + doc: "app: a-scalar\n", + path: []string{"app", "token"}, + wantErr: "could not be found", + }, + { + name: "the leaf is a mapping rather than a scalar", + doc: "app:\n token:\n nested: ENC[AES256_GCM,data:abc,type:str]\n", + path: []string{"app", "token"}, + wantErr: "not a scalar", + }, + { + name: "the document does not parse", + doc: "app:\n token: [unclosed\n", + path: []string{"app", "token"}, + wantErr: "does not parse", + }, + { + // A prefix match on the VALUE would be as wrong as one on the key: + // a value that merely mentions the marker is not ciphertext. + name: "a value that only mentions the marker", + doc: "app:\n token: 'see ENC[AES256_GCM, for details'\n", + path: []string{"app", "token"}, + wantErr: "written in the clear", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := assertEncryptedAtPath([]byte(c.doc), c.path) + if c.wantErr == "" { + if err != nil { + t.Fatalf("assertEncryptedAtPath: %v", err) + } + return + } + if err == nil { + t.Fatal("assertEncryptedAtPath returned nil, want a refusal") + } + if !strings.Contains(err.Error(), c.wantErr) { + t.Errorf("error = %q, want it to mention %q", err.Error(), c.wantErr) + } + // The refusal reports that a value is in the clear; it must not + // carry the value while doing so. + if strings.Contains(err.Error(), secret) { + t.Errorf("error %q echoed the cleartext value", err.Error()) + } + }) + } +}