diff --git a/audits/2026-09-29-high-findings-validation-and-mitigation-plan.md b/audits/2026-09-29-high-findings-validation-and-mitigation-plan.md new file mode 100644 index 0000000..bfb20fd --- /dev/null +++ b/audits/2026-09-29-high-findings-validation-and-mitigation-plan.md @@ -0,0 +1,120 @@ +# Validation and Mitigation Plan: High-Priority Findings from the 2026-09-29 Repository Analysis + +Source audit: [2026-09-29-repository-analysis.md](2026-09-29-repository-analysis.md) (untracked in the working copy at the time of this writing; not present on this branch — refer to the copy in the main checkout). + +This document verifies the seven findings marked "Hoch" (High) in that audit — P01 through P07 — directly against the current source in this repository, assesses whether "High" is justified, and lays out a phased mitigation plan with effort estimates. + +## 1. Verification method + +Each finding was independently re-derived by reading the cited source files at their current line numbers (which had drifted slightly since the audit), not merely by trusting the audit's prose. P02 was additionally verified empirically by parsing the exact reproduction input through the actual `goccy/go-yaml` parser used by this codebase. Two follow-up checks (the `filter.nebel.required` git setting, and smudge behavior for untagged plaintext in a protected field) were done afterward because they materially change the mitigation plan's phasing. + +## 2. Verdict summary + +| ID | Verdict | Notes | +|---|---|---| +| P01 | **Confirmed** | Common-path operational bug, not attacker-driven. Re-running `init` unconditionally deletes and restores managed files from HEAD with no dirty check. | +| P02 | **Confirmed (empirically)** | Quote-unaware comment truncation in YAML span computation leaks a plaintext suffix of a quoted secret into the git blob. | +| P03 | **Confirmed** | Dotted key collisions (`"a.b"` vs. nested `a: {b: ...}`) silently resolve to the wrong field with no error. | +| P04 | **Confirmed, worse than stated** | Duplicate JSON keys leave the second occurrence in plaintext — and standard `encoding/json` map-unmarshal takes the *last* duplicate, meaning most consumers would actually use the plaintext value, not the encrypted one. | +| P05 | **Confirmed, narrower in practice** | Type metadata is genuinely outside the AEAD's authenticated data, so `type:str → type:bool` tampering passes authentication. However, smudge already passes through *any* untagged plaintext in a protected field with no error at all (`filterop.go:210`, intentional for migration, AC-6.7) — so an attacker with blob write access doesn't need the type-confusion trick; they can just substitute plaintext directly. P05's real distinguishing value is stealth (a type-flipped field still displays as `ENC[...]` in review) rather than reach. | +| P06 | **Confirmed** | The derived key is base64-encoded and passed as a literal `git config` subprocess argument — inconsistent with this codebase's own established threat model, which already refuses password arguments on its own CLI for the identical reason (argv is readable via `/proc//cmdline`). | +| P07 | **Confirmed, all 3 sub-issues** | (1) `*.env`-style root-only vs. recursive `.gitattributes` glob semantics diverge silently; (2) unescaped filenames with spaces break `.gitattributes` field parsing; (3) `doublestar.Match`'s error is discarded in `MatchRule`, so a malformed hand-edited glob silently never matches. | + +**All seven High findings stand.** None should be downgraded; P04 arguably deserves more weight than the audit gave it, and P05 should be read together with the smudge-passthrough gap noted above rather than in isolation. + +## 3. Priority order — what to fix first, and what to bundle together + +Ranked by importance to the tool's core promise (protecting secrets), not by implementation effort. The distinguishing question at each tier: does this bug leak the secret itself, or something weaker (the key, or integrity of already-protected data)? Within "leaks the secret" bugs, does it leak silently while reporting success (worst — false sense of security), and how easily does a normal user trigger it without doing anything unusual? + +**Tier 1 — Silent, complete plaintext exposure while the tool reports success.** These are the worst class of bug for an encryption tool: nothing looks wrong, but the secret is fully unprotected in the git blob. + +1. **P07(1)** — root-only vs. recursive glob mismatch. Highest priority of all: triggered by the single most natural pattern a user would write (`*.env`-style, exactly like `.gitignore` habits) combined with the equally common case of the file living in a subdirectory. `add file` reports success; git invokes the filter; nebel's own matcher silently disagrees and lets plaintext through. +2. **P04** — duplicate JSON keys. Full exposure of exactly the value most JSON consumers will actually use (last-write-wins on unmarshal), while the encrypted first copy is decoration. +3. **P03** — dotted path collision. Full exposure of the intended field; requires a specific naming collision, so somewhat less likely to occur than P07(1)/P04, but total when it does. +4. **P07(3)** — discarded glob-match errors. A malformed hand-edited rule silently protects nothing; requires manual `.gitattributes`/config editing to trigger, so lower likelihood than the above, same total impact. +5. **P07(2)** — unescaped filenames with spaces. Narrower trigger condition (spaces in filenames), same silent-total-bypass outcome. + +**Tier 2 — Large partial plaintext exposure.** + +6. **P02** — YAML quoted-string comment truncation. Not silent-total like Tier 1, but close in practice: in the audit's own example, only the first 4 characters end up encrypted and nearly the entire rest of the secret is left as plaintext. Triggers on any secret value containing a literal `" #"`, which is plausible for real passwords/tokens/connection strings. + +**Tier 3 — Data loss, not a leak.** Different axis entirely (destroys the user's own work, doesn't expose a secret to a third party), but ranked here because it's trivially easy to trigger with completely ordinary use — no attacker, no unusual input required. + +7. **P01** — re-running `init` destroys uncommitted/staged changes to managed files. + +**Tier 4 — Conditional local exposure, requires an additional attacker capability.** + +8. **P06** — derived key visible via process argument list. Only exploitable by a co-located observer who can already read another user's/process's argv (shared host, container escape, etc.) — a real gap and inconsistent with the tool's own stated threat model, but requires more attacker capability than "can read the git repo," which is all Tiers 1–3 require. + +**Tier 5 — Integrity-only, and largely redundant with an accepted design gap.** + +9. **P05** — type metadata not authenticated. Real, but as noted in §2, an attacker with the blob-write access this requires can already substitute plaintext directly via the pre-existing, intentional smudge passthrough — so this doesn't expand what such an attacker can already do, it only makes one specific form of tampering (type-flip) harder to spot in review. + +### What to fix together + +- **Package A — "filter/rule matching correctness" (P07(1), P07(2), P07(3)):** same files (`config.go`'s `Validate`/`MatchRule`, `add.go`'s `addGitattributesPattern`, `filterop.go`'s `Clean`), same underlying defect class (a registered rule doesn't actually guarantee git and nebel agree on what's matched), same test setup. Do this as one PR — it's also the single highest-priority fix, so it should be first regardless of grouping. + - **Status: done** (commit `7acffc9` on branch `worktree-audit-mitigation-plan`). `Clean`/`Smudge` now return `filterop.ErrNoMatchingRule` instead of silently passing content through when no rule matches a path git invoked the filter for; `Config.Validate` rejects a syntactically invalid glob via `doublestar.ValidatePattern`; `addGitattributesPattern` escapes leading `#`/`!` and embedded spaces. A new e2e test reproduces the exact P07(1) scenario (bare `*.env` glob, file in a subdirectory) against a real git repository and was confirmed to fail against the pre-fix code before passing against the fix. Full existing test suite (`go test ./...`) passes unchanged. +- **Package B — "fail-closed field resolution" (P03, P04):** different files (`path.go`/`json.go` vs. the same), but the identical defensive pattern (detect an ambiguous/duplicate field target and hard-error via `required=true` instead of silently protecting only one candidate). Natural to design and review together even though the code changes are in different functions. + - **Status: done** (commit `081f639` on branch `worktree-audit-mitigation-plan`). Both turned out to be one fix: `Locate` now cross-checks the configured path against the existing `Leaves()` walk and refuses with a new `ErrAmbiguousPath` if more than one leaf shares that path string, before resolving to any of them. This catches P03's literal-dot-vs-nested collision for both formats, and P04's JSON duplicate-key case for free (go-yaml's parser already rejects a literal duplicate YAML key on its own, confirmed empirically — only JSON needed this part). Verified directly against the pre-fix code: it silently returned the wrong/first value with no error in both repro cases. Full existing test suite passes unchanged. +- **Package C — P02 alone**, but worth opening the YAML renderer "under the hood" only once: if this work is scheduled, consider folding in the audit's medium-priority P10 (control characters not escaped) and P11 (multi-document YAML only partially handled) at the same time, since they touch the exact same rendering/parsing code path and the same test harness — doing them separately means paying the review/regression-risk cost of touching `yaml.go`'s scalar handling three times instead of once. (P10/P11 are not part of this High-severity validation pass; flagged here purely as an efficiency note.) + - **Status: done** (commit `7d63b6f` on branch `worktree-audit-mitigation-plan`; P10/P11 were not folded in — still open). `scalarText` no longer searches a quoted scalar's Origin for a trailing comment at all: verified empirically that a quoted scalar's Origin is always exactly the quoted literal verbatim with no trailing trivia ever folded in, so the search — which was finding a false match *inside* the quotes — is simply skipped for quoted scalars, and kept only for plain scalars, where it's a no-op in practice (the lexer itself can never let " #" survive into a plain scalar's own Origin). Verified directly against the pre-fix code: `Locate` on a quoted value containing " #" returned a 4-byte span while `span.Value` was already the full, correct value — the exact partial-plaintext-leak mechanism. + - **Bonus finding, not in the original audit:** while fixing this, discovered that literal (`|`) and folded (`>`) block scalars are already silently corrupted by the pre-fix code — `ast.LiteralNode.GetToken()` returns only the block's header token, not its content, so `Locate` would splice a ciphertext tag over the single `|`/`>` character. Confirmed directly against the pre-fix code. Added a new `ErrBlockScalarUnsupported` so this now fails loudly instead; proper block-scalar support remains unimplemented and out of scope here. +- **Package D — P01 alone.** Self-contained (`gitutil.go`/`init.go`), no shared code with the others — can be done in parallel by a different engineer with no coordination cost. + - **Status: done** (commit `ad0ceb7` on branch `worktree-audit-mitigation-plan`). Added `gitutil.DirtyFiles`, which runs a single `git diff --name-only HEAD -- ` to catch staged and unstaged changes alike; `CheckoutAll` now calls it before touching anything and refuses with a new `ErrDirtyManagedFiles` naming every dirty path. Verified directly against the pre-fix code with a standalone probe: an unstaged edit to a managed file was silently destroyed (`CheckoutAll` returned `decrypted=1, failures=nil, err=nil`, and the file's content reverted to HEAD with the edit gone). A clean repo (the ordinary post-`init` case) is confirmed to still succeed normally. +- **Package E — P06, and worth bundling with the audit's separate (non-defect) "harden local key storage" suggestion.** A single fix — a dedicated `0600` key file instead of `git config` — addresses both the argv-exposure bug and that hardening suggestion at once. + - **Status: done** (commit `8b830ea` on branch `worktree-audit-mitigation-plan`). `localkey.Set` now writes to a new `nebel-keys` file (0600) inside the shared git-common-dir instead of via `git config --local ` — closing the subprocess-argv exposure. `Get`/`All` still fall back to the legacy git-config location so an existing clone doesn't need to re-run `nebel init`. Several `cmd/nebel` e2e tests asserted directly on the old git-config storage as their way of checking key registration; updated them (via a new shared `registeredKeyValue` helper) to check the actual current location instead of silently becoming vacuous checks. Verified directly against the pre-fix code: `Set` left the raw base64 key sitting in plain `.git/config` content under `[filter "nebel"] key = ...` — written by exactly the subprocess call this fix removes the key's argument from. Full test suite passes unchanged. +- **Package F — P05, standalone, last.** Breaking ciphertext-format change with its own migration path; do not bundle with anything else, since it needs its own versioned release and rollout communication independent of all other fixes. + - **Status: done** (commits `ba8e9aa` and `e26e65c` on branch `worktree-audit-mitigation-plan`; the second commit corrects and strengthens the first after an independent review pass). Reused the existing ALGO-field mechanism (spec 03 AC-3.8) rather than inventing a new one: a new `AES256_SIV_TB` identifies a value tag whose type is bound into the AAD; `EncodeValue` always produces this form now, while `Parse` still accepts the legacy `AES256_SIV` form and exposes which one via a new `Tag.TypeBound` field, so `smudgeValues` reconstructs the exact AAD each form was actually encrypted under. Verified directly against the pre-fix code: a `type:str`→`type:bool` edit on an untouched ciphertext produced no authentication error and rendered the secret unquoted. + - **Stopgap restored:** the plan's Phase 1 stopgap for P05 (reject a decrypted value that isn't a valid literal for its claimed type before rendering it unquoted) was initially dropped and has been added back (`filterop.validTypeLiteral`) — it closes the structural-injection tail risk a legacy tag's still-possible type flip could otherwise reach. + - **Corrected scope claim:** "a legacy value converges onto the type-bound form the next time anything re-encrypts it" is true but incomplete — it only protects the *current committed* tag. An attacker with write access can still resurrect an old legacy ciphertext from git history with its type re-flipped, since old key versions stay registered indefinitely (spec 11 AC-11.5); fully closing that would require actively revoking a key version, which is out of scope here. This is now stated plainly in `internal/tag`'s doc comment and specs/03, not left implied. + - **Real interaction found and fixed:** a legacy value tag re-encrypts to different bytes the moment anything cleans it again (the AAD changed), and `git diff HEAD` — what Package D's `DirtyFiles` runs — must clean a filtered path to compare it at all. That made a repeated `nebel init` on a repo with an existing legacy tag, with zero real local edits, refuse via `ErrDirtyManagedFiles`. Confirmed with a new e2e test using a real repo and a real derived key (`cmd/nebel/legacytag_e2e_test.go`), which also caught that `git add --renormalize` alone doesn't fix it (it only updates the index; the check compares against HEAD) — committing the renormalized result is required too. The error text and doc comments now say so. + - Full test suite passes. Also updates `specs/03-value-encoding.md` (new AC-3.10). + +Recommended shipping order: **Package A → Package B → Package C → Package D and Package E in parallel → Package F** as a separate, later release. + +## 4. A structural fact that changes the plan + +`filter.nebel.required` is set to `true` at registration (`cmd/nebel/init.go:356`). This means git already treats a *failing* clean/smudge filter invocation as a hard error that blocks the operation (e.g. blocks `git add`/commit). Several of the cheapest mitigations below are "detect the bad case and return an error" rather than "detect and gracefully recover" — and because of `required=true`, those errors will actually be enforced by git, not just logged. This makes P03, P04, and P07(1)/(3) considerably cheaper to fix well than a full silent-recovery design would be. + +## 5. Phased mitigation plan + +### Phase 0 — Immediate, no code change (do now, in parallel with everything else) + +- **For any real installation already using nebel to protect live secrets:** scan the raw stored blobs of protected paths, including history (`git cat-file -p :` bypasses the smudge filter and shows exactly what's stored), for plaintext remnants matching the P02/P03/P04/P07/P08 patterns. Any secret found must be rotated at its source — nebel's own key rotation does not un-expose an already-leaked plaintext. This is the natural first justification for the audit's separately-noted missing `nebel status` command. +- Unrelated to the audit, but found while validating this plan: `git remote -v` in this repository currently resolves to a URL with a **live GitHub PAT embedded in plaintext**. Recommend rotating that token and switching to a credential helper immediately. + +### Phase 1 — Quick, non-breaking, fail-fast hardening (~3–4 developer-days total, incl. tests) + +| Item | Fix | Effort | +|---|---|---| +| P01 | Add a dirty-check (`git status --porcelain` / `git diff --quiet HEAD --` over managed files) before `gitutil.CheckoutAll`'s remove+checkout; abort with a clear error if any managed file has uncommitted changes. Single function (`gitutil.go`), reuse existing e2e test infra. Must not block a legitimate first `init` on a clean clone — needs a test for that path specifically. | 3–5h | +| P04 | Detect a second key matching the target path within the same JSON object in `descendObject`/`cleanValues`; hard-error instead of silently encrypting only the first. Small, contained change. | 2–4h | +| P07 (1)+(3) | Single high-leverage fix: make `Clean` (`filterop.go:57-61`) return an error when git invokes it for a path that matches no configured rule, instead of silently passing plaintext through. Because `required=true`, this blocks the commit. Also stop discarding `doublestar.Match`'s error in `MatchRule` and surface it as a hard error. This neutralizes both sub-issues without needing to reconcile nebel's glob semantics with `.gitattributes` semantics (that reconciliation becomes a pure usability improvement afterward, not a security fix). | 3–5h combined | +| P06 | Stop passing the derived key via `git config` subprocess argv. Either write directly to `.git/config` (resolve the real location via `git rev-parse --git-common-dir` to handle linked worktrees, respect `config.lock`), or — simpler and also addresses the audit's separate "harden local key storage" note — use a dedicated key file with `0600` permissions, with a fallback read path for the old config-based storage. | 4–8h | +| P05 (stopgap only) | On smudge, reject a decrypted value whose literal text isn't syntactically valid for its claimed type before rendering it unquoted (e.g. refuse to render `"not-a-bool"` as `type:bool`). Narrows the injection tail risk; does not close the underlying authentication gap. | 2–4h | + +### Phase 2 — Medium, non-breaking format-preserving fixes (~2–3 developer-days) + +| Item | Fix | Effort | +|---|---|---| +| P02 | Make `scalarText`/`yamlSpan` quote-aware: only truncate at an unquoted `#`/` #`; leave quoted, literal, and folded scalar spans untouched. Needs new tests for quoted strings containing `#`, block scalars, and CRLF combinations. Touches every YAML span computation — moderate regression risk, needs solid coverage before merge. | 1–2 days | +| P03 | Add path-collision detection: hard-error when a flattened dotted path is ambiguous with a nested path in the same document, rather than silently resolving to one of them. (Full escaping syntax for literal dots in key names is a larger, separate feature — recommend deferring unless real users actually hit this, ~2–3 additional days if needed later.) | 4–8h | +| P07 (2) | Validate/reject or escape pattern/filename characters (spaces, etc.) before writing an `.gitattributes` line at `add file` time. | 2–3h | + +### Phase 3 — Breaking format change, needs versioning and migration (~3–5 developer-days) + +| Item | Fix | Effort | +|---|---|---| +| P05 (proper fix) | Bind `type` into the AEAD's authenticated data (or into the authenticated plaintext itself) so tampering the type metadata fails authentication. This changes the ciphertext/tag format: every previously-committed tag was authenticated without type, so this needs a new tag/format version, backward-compatible decryption of old tags, and a migration path analogous to the existing key-rotation mechanism (spec 11). | 3–5 days incl. migration tooling and docs | + +### Design question to resolve with stakeholders, not purely a bug fix + +Smudge currently passes through *any* untagged plaintext in a protected field with no error at all, intentionally, to support gradual migration (AC-6.7). This is the more foundational relative of P05 and is a known limitation shared by filter-driven transparent-encryption tools generally (nebel, like git-crypt or unsigned sops usage, does not by itself defend against a git committer with write access substituting content — that requires signed commits / branch protection, outside this tool's scope). Worth an explicit "strict mode" option that fails smudge/status on an untagged protected field, opt-in so existing migration workflows keep working. This is a scope/product decision, not included in the effort estimates above. + +## 6. Total effort estimate + +- Phase 1: ~3–4 developer-days +- Phase 2: ~2–3 developer-days +- Phase 3: ~3–5 developer-days + +**Total: roughly 8–12 developer-days (1.5–2.5 weeks) for one engineer**, before touching the audit's medium-priority items (P08–P12), which were out of scope for this validation pass. diff --git a/cmd/nebel/add.go b/cmd/nebel/add.go index c4289c3..3ad8a7b 100644 --- a/cmd/nebel/add.go +++ b/cmd/nebel/add.go @@ -218,7 +218,7 @@ func ensureGitattributes(root string) error { // glob to the nebel filter, unless that exact line is already present. func addGitattributesPattern(root, glob string) error { path := filepath.Join(root, gitattributesName) - line := fmt.Sprintf("%s filter=nebel", glob) + line := fmt.Sprintf("%s filter=nebel", escapeGitattributesPattern(glob)) existing, err := os.ReadFile(path) if err != nil && !os.IsNotExist(err) { @@ -241,3 +241,25 @@ func addGitattributesPattern(root, glob string) error { } return nil } + +// escapeGitattributesPattern escapes the characters gitattributes' line +// syntax gives special meaning to, so a glob written verbatim into a +// pattern/attribute line still means what the caller intended: +// - a leading '#' starts a comment, and a leading '!' negates a +// pattern — either would silently turn the whole line into something +// other than a filter assignment for glob; +// - the line's fields are split on whitespace, so an embedded space +// would attach the rest of glob to the "filter=nebel" attribute +// instead of the pattern, leaving the intended file unmatched even +// though the command reports success. +// +// This does not touch glob metacharacters (*, ?, [, ], \) themselves — +// Rule.Files is intentionally a doublestar glob (see its doc comment), +// and escaping those would change what the pattern matches instead of +// merely how the line is parsed. +func escapeGitattributesPattern(glob string) string { + if strings.HasPrefix(glob, "#") || strings.HasPrefix(glob, "!") { + glob = `\` + glob + } + return strings.ReplaceAll(glob, " ", `\ `) +} diff --git a/cmd/nebel/add_test.go b/cmd/nebel/add_test.go new file mode 100644 index 0000000..bda6c39 --- /dev/null +++ b/cmd/nebel/add_test.go @@ -0,0 +1,33 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import "testing" + +// Regression test for the 2026-09-29 audit's P07 sub-issue 2: a glob +// written unescaped into a .gitattributes line breaks on the characters +// that syntax gives special meaning to, leaving the intended file with no +// effective filter assignment even though the add command reports success. +func TestEscapeGitattributesPattern(t *testing.T) { + tests := []struct { + name string + glob string + want string + }{ + {"no special characters", "secrets/*.pem", "secrets/*.pem"}, + {"embedded space", "My File.env", `My\ File.env`}, + {"multiple spaces", "a b c.env", `a\ b\ c.env`}, + {"leading hash", "#backup.env", `\#backup.env`}, + {"leading bang", "!important.env", `\!important.env`}, + {"glob metacharacters untouched", "**/*.env", "**/*.env"}, + {"bracket class untouched", "secrets/[ab].pem", "secrets/[ab].pem"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := escapeGitattributesPattern(tt.glob); got != tt.want { + t.Errorf("escapeGitattributesPattern(%q) = %q, want %q", tt.glob, got, tt.want) + } + }) + } +} diff --git a/cmd/nebel/e2e_test.go b/cmd/nebel/e2e_test.go index 41d90c5..5c889ac 100644 --- a/cmd/nebel/e2e_test.go +++ b/cmd/nebel/e2e_test.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strings" "sync" "testing" ) @@ -92,6 +93,37 @@ func runInExpectingError(t *testing.T, dir, pathEnv, name string, args ...string return string(out), err } +// registeredKeyValue returns the raw stored value (a base64-encoded key) +// for version in dir's local key storage, treating nebel as a black box +// rather than assuming which of its two storage locations actually holds +// it: the key file it writes to as of the 2026-09-29 audit's P06 fix +// (internal/localkey's keyFileName), checked first, falling back to the +// legacy git-config location a version could still be registered under — +// mirroring internal/localkey.Get's own precedence, so these tests keep +// working regardless of which one a given key landed in. +func registeredKeyValue(t *testing.T, dir, pathEnv string, version int) (string, bool) { + t.Helper() + commonDir := strings.TrimSpace(runIn(t, dir, pathEnv, "git", "rev-parse", "--git-common-dir")) + if data, err := os.ReadFile(filepath.Join(dir, commonDir, "nebel-keys")); err == nil { + prefix := fmt.Sprintf("%d=", version) + for _, line := range strings.Split(string(data), "\n") { + if val, ok := strings.CutPrefix(line, prefix); ok { + return val, true + } + } + } + + name := "filter.nebel.key" + if version != 1 { + name = fmt.Sprintf("filter.nebel.key%d", version) + } + val, ok, err := getLocalConfig(t, dir, pathEnv, name) + if err != nil { + return "", false + } + return val, ok +} + // initWithPassword runs `nebel init`, supplying the password through // the environment — the command line no longer accepts one. func initWithPassword(t *testing.T, dir, pathEnv, password string) { @@ -233,14 +265,14 @@ func TestJoinWithWrongPasswordFails(t *testing.T) { repo := newTestRepo(t, pathEnv) initWithPassword(t, repo, pathEnv, "correct-password") - keyBefore := runIn(t, repo, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key") + keyBefore, _ := registeredKeyValue(t, repo, pathEnv, 1) out, err := runInitWith(t, repo, pathEnv, []string{passwordEnv + "=wrong-password"}, "") if err == nil { t.Fatalf("nebel init with wrong password: want an error, got success: %s", out) } - keyAfter := runIn(t, repo, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key") + keyAfter, _ := registeredKeyValue(t, repo, pathEnv, 1) if keyBefore != keyAfter { t.Error("wrong password changed the registered local key") } diff --git a/cmd/nebel/filtermatch_e2e_test.go b/cmd/nebel/filtermatch_e2e_test.go new file mode 100644 index 0000000..4fabdee --- /dev/null +++ b/cmd/nebel/filtermatch_e2e_test.go @@ -0,0 +1,73 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Regression test for the 2026-09-29 audit's P07(1): a bare glob like +// "*.env" (no leading "**/") matches recursively in .gitattributes syntax +// but, per doublestar's semantics, only at the depth it's written at (see +// config.Rule.Files's doc comment). `nebel add file` writes such a pattern +// unchanged into both files, so a file living in a subdirectory is one +// git invokes the filter for but that config.MatchRule does not recognize. +// +// Before the fix, Clean silently returned this content unchanged and +// `git commit` succeeded with the secret stored as plaintext. The filter +// must instead fail the commit outright (filterop.ErrNoMatchingRule), +// since filter.nebel.required is set to true. +func TestAddFileBareGlobDoesNotSilentlyCommitPlaintextInSubdirectory(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + + initWithPassword(t, repo, pathEnv, "test-password-123") + runIn(t, repo, pathEnv, "nebel", "add", "file", "*.env") + + configDir := filepath.Join(repo, "config") + if err := os.MkdirAll(configDir, 0o755); err != nil { + t.Fatal(err) + } + plaintext := "DB_PASSWORD=super-secret\n" + envPath := filepath.Join(configDir, "prod.env") + if err := os.WriteFile(envPath, []byte(plaintext), 0o644); err != nil { + t.Fatal(err) + } + + out, err := runInExpectingError(t, repo, pathEnv, "git", "add", "--", "config/prod.env") + if err == nil { + t.Fatalf("git add config/prod.env: want an error (glob/attribute mismatch), got success:\n%s", out) + } + if !strings.Contains(out, "no rule") && !strings.Contains(out, "ErrNoMatchingRule") && !strings.Contains(out, "filterop:") { + t.Errorf("git add error output = %q, want it to mention the rule mismatch", out) + } + + // Confirm the secret was never staged at all, plaintext or otherwise — + // a failed clean must not leave a partial index entry behind. + lsFiles := runIn(t, repo, pathEnv, "git", "status", "--porcelain") + if strings.Contains(lsFiles, "A config/prod.env") { + t.Errorf("git status shows config/prod.env staged despite the filter error:\n%s", lsFiles) + } + + // The root-level case (matching depth) must still work exactly as + // before — this is a mismatch fix, not a semantics change. + rootPlaintext := "DB_PASSWORD=another-secret\n" + rootEnvPath := filepath.Join(repo, "root.env") + if err := os.WriteFile(rootEnvPath, []byte(rootPlaintext), 0o644); err != nil { + t.Fatal(err) + } + runIn(t, repo, pathEnv, "git", "add", "--", "root.env") + runIn(t, repo, pathEnv, "git", "commit", "-q", "-m", "add root-level env file") + + stored := runIn(t, repo, pathEnv, "git", "show", "HEAD:root.env") + if stored == rootPlaintext { + t.Error("root-level file matching the same bare glob was committed as plaintext") + } + if !strings.HasPrefix(stored, "ENC[") { + t.Errorf("root-level file's committed blob is not ciphertext: %q", stored) + } +} diff --git a/cmd/nebel/init_version_e2e_test.go b/cmd/nebel/init_version_e2e_test.go index 054ab8e..c6881c1 100644 --- a/cmd/nebel/init_version_e2e_test.go +++ b/cmd/nebel/init_version_e2e_test.go @@ -87,8 +87,8 @@ func TestInitVersionRecoversContentFromBeforeRotation(t *testing.T) { } // The v2 key registered earlier must still be there. - if _, err := runInExpectingError(t, clone, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key2"); err != nil { - t.Errorf("init --version 1 disturbed the already-registered v2 key: %v", err) + if _, ok := registeredKeyValue(t, clone, pathEnv, 2); !ok { + t.Error("init --version 1 disturbed the already-registered v2 key") } } @@ -104,7 +104,7 @@ func TestInitVersionUnknownFails(t *testing.T) { if err == nil { t.Fatalf("nebel init --version 99: want an error, got success:\n%s", out) } - if _, ok, kerr := getLocalConfig(t, clone, pathEnv, "filter.nebel.key99"); kerr == nil && ok { + if _, ok := registeredKeyValue(t, clone, pathEnv, 99); ok { t.Error("init --version 99 registered a key for a version that doesn't exist") } } diff --git a/cmd/nebel/legacytag_e2e_test.go b/cmd/nebel/legacytag_e2e_test.go new file mode 100644 index 0000000..3e07c76 --- /dev/null +++ b/cmd/nebel/legacytag_e2e_test.go @@ -0,0 +1,124 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "encoding/base64" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/MuffinUser/nebel/internal/siv" + "github.com/MuffinUser/nebel/internal/tag" +) + +// Regression test for an interaction between Package D (P01's dirty-check +// in gitutil.CheckoutAll) and Package F (P05's type-authenticated value +// tags), found during review rather than by the original audit: +// DirtyFiles compares by running the clean filter (that's what `git diff +// HEAD` against a filtered path does), and a legacy (pre-AC-3.10) value +// tag re-encrypts to different bytes the moment anything cleans it again +// — the AAD it's produced under changed — even when the secret itself was +// never touched. That makes a repeated `nebel init` refuse with +// ErrDirtyManagedFiles for a file the user never edited, until the legacy +// tag is upgraded and *committed* (renormalizing alone updates the index, +// not HEAD, so `git diff HEAD` still sees the old committed form either +// way — confirmed here, not merely asserted). +func TestInitRefusesOnLegacyValueTagUntilRenormalizedAndCommitted(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupValueRepo(t, pathEnv) + runIn(t, repo, pathEnv, "nebel", "add", "field", "config/staging.yaml", "database.password") + runIn(t, repo, pathEnv, "git", "add", ".") + runIn(t, repo, pathEnv, "git", "commit", "-q", "-m", "encrypt database.password") + + committed := runIn(t, repo, pathEnv, "git", "show", "HEAD:config/staging.yaml") + if !strings.Contains(committed, "AES256_SIV_TB,") { + t.Fatalf("test setup did not produce a type-bound tag:\n%s", committed) + } + + // Downgrade the committed tag to the legacy (pre-AC-3.10) form, + // re-encrypting the real secret under the legacy (type-unbound) AAD + // with this repo's actual registered key — reproducing exactly what a + // pre-Package-F build would have committed, not a synthetic stand-in. + keyB64, ok := registeredKeyValue(t, repo, pathEnv, 1) + if !ok { + t.Fatal("no key registered for version 1") + } + key, err := base64.StdEncoding.DecodeString(keyB64) + if err != nil { + t.Fatalf("decoding registered key: %v", err) + } + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.yaml", "database.password") + ciphertext, err := siv.Encrypt(key, []byte("s3cr3t"), legacyAAD) + if err != nil { + t.Fatalf("siv.Encrypt: %v", err) + } + legacyTag := fmt.Sprintf("ENC[%s,key:1,data:%s,type:str]", tag.AlgoAES256SIV, base64.StdEncoding.EncodeToString(ciphertext)) + legacyBlob := strings.Replace(stagingYAML, `password: "s3cr3t"`, `password: "`+legacyTag+`"`, 1) + + cfgPath := filepath.Join(repo, "config", "staging.yaml") + if err := os.WriteFile(cfgPath, []byte(legacyBlob), 0o644); err != nil { + t.Fatal(err) + } + // -c filter.nebel.clean=cat: commit the legacy blob's bytes exactly as + // written, the same technique TestPreRotationRepoMigration uses to + // simulate a commit predating a tag-format fix. + runIn(t, repo, pathEnv, "git", "-c", "filter.nebel.clean=cat", "add", "-A") + runIn(t, repo, pathEnv, "git", "-c", "filter.nebel.clean=cat", "commit", "-q", "-m", "simulate a pre-AC-3.10 commit") + + // First init after the simulated legacy commit: the working tree + // currently holds that same legacy-tagged text verbatim (nothing + // cleaned it yet), so it's not "dirty" relative to HEAD — this must + // still succeed, and it's what actually decrypts the field into the + // working tree for the first time. + initWithPassword(t, repo, pathEnv, "value-mode-password") + onDisk, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(onDisk), `password: "s3cr3t"`) { + t.Fatalf("first init did not decrypt the legacy tag:\n%s", onDisk) + } + + // Second init, simulating "run nebel init again" with no edits in + // between: the working tree is now plaintext, and re-cleaning it + // produces a fresh type-bound tag that differs from HEAD's still-legacy + // one — DirtyFiles sees that as a real difference and refuses, exactly + // the interaction this test is about. + out, err := runInitWithExpectingError(t, repo, pathEnv, "value-mode-password") + if err == nil { + t.Fatalf("second init: want ErrDirtyManagedFiles (legacy tag re-encrypts differently), got success:\n%s", out) + } + if !strings.Contains(out, "renormalize") { + t.Errorf("init error does not point at the documented fix:\n%s", out) + } + + // The fix as actually documented: renormalize AND commit. Renormalize + // alone only updates the index, not HEAD — `git diff HEAD` (what + // DirtyFiles runs) would still see the old committed form either way, + // which this confirms rather than assumes. + runIn(t, repo, pathEnv, "git", "add", "--renormalize", "--", "config/staging.yaml") + if _, err := runInitWithExpectingError(t, repo, pathEnv, "value-mode-password"); err == nil { + t.Fatal("init after renormalizing but before committing: want it to still refuse, got success") + } + runIn(t, repo, pathEnv, "git", "commit", "-q", "-m", "upgrade database.password to a type-bound tag") + + initWithPassword(t, repo, pathEnv, "value-mode-password") + afterUpgrade, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(afterUpgrade), `password: "s3cr3t"`) { + t.Errorf("init after the documented fix did not leave the field decrypted:\n%s", afterUpgrade) + } +} + +// runInitWithExpectingError runs `nebel init` with password, expecting it +// to fail, and returns its combined output for inspecting the error text. +func runInitWithExpectingError(t *testing.T, dir, pathEnv, password string) (string, error) { + t.Helper() + return runInitWith(t, dir, pathEnv, []string{passwordEnv + "=" + password}, "") +} diff --git a/cmd/nebel/password_e2e_test.go b/cmd/nebel/password_e2e_test.go index 692f081..3d6a9a8 100644 --- a/cmd/nebel/password_e2e_test.go +++ b/cmd/nebel/password_e2e_test.go @@ -114,7 +114,7 @@ func TestInitRejectsPasswordArgument(t *testing.T) { if !strings.Contains(out, "process list") { t.Errorf("error does not explain the exposure:\n%s", out) } - if _, err := runInExpectingError(t, clone, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key"); err == nil { + if _, ok := registeredKeyValue(t, clone, pathEnv, 1); ok { t.Error("a refused init registered a local key") } } @@ -138,7 +138,7 @@ func TestInitWithoutAnyPasswordSourceFails(t *testing.T) { if _, err := os.Stat(filepath.Join(clone, "secrets", "prod.pem")); err != nil { t.Fatal(err) } - if _, err := runInExpectingError(t, clone, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key"); err == nil { + if _, ok := registeredKeyValue(t, clone, pathEnv, 1); ok { t.Error("a failed init registered a local key") } } diff --git a/cmd/nebel/rotate_e2e_test.go b/cmd/nebel/rotate_e2e_test.go index 5405e6a..4e3c291 100644 --- a/cmd/nebel/rotate_e2e_test.go +++ b/cmd/nebel/rotate_e2e_test.go @@ -176,19 +176,19 @@ func TestRotateKeepsOldKeyRegistered(t *testing.T) { pathEnv := pathEnvWithBin(t) origin, plaintext := setupEncryptedOrigin(t, pathEnv, "origin-password") - keyV1Before := runIn(t, origin, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key") + keyV1Before, _ := registeredKeyValue(t, origin, pathEnv, 1) out, err := runRotateWith(t, origin, pathEnv, []string{passwordEnv + "=rotated-password"}, "") if err != nil { t.Fatalf("nebel rotate: %v\n%s", err, out) } - keyV1After := runIn(t, origin, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key") + keyV1After, _ := registeredKeyValue(t, origin, pathEnv, 1) if keyV1Before != keyV1After { t.Error("rotate disturbed the existing version's registered key") } - if _, err := runInExpectingError(t, origin, pathEnv, "git", "config", "--local", "--get", "filter.nebel.key2"); err != nil { - t.Errorf("rotate did not register the new version's key: %v", err) + if _, ok := registeredKeyValue(t, origin, pathEnv, 2); !ok { + t.Error("rotate did not register the new version's key") } // The pre-rotation content, still tagged version 1, must still decrypt. diff --git a/internal/config/config.go b/internal/config/config.go index 3553eae..84fee8e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -85,6 +85,14 @@ var ( // encrypt list, which it would ignore — a sign the author expected // per-value behaviour and would not get it. ErrFieldsOnFileRule = errors.New("config: mode: file rule must not list fields") + + // ErrInvalidPattern is returned when a rule's Files glob is not valid + // doublestar syntax. Rejecting it here, rather than at match time, + // matters because MatchRule discards doublestar.Match's error (its + // bool-only signature has no room for one) — an invalid pattern that + // slipped past this check would silently behave as "never matches" + // instead of surfacing the typo. + ErrInvalidPattern = errors.New("config: invalid glob pattern") ) // SaltBytes decodes Salt from base64. @@ -114,6 +122,9 @@ func (c *Config) CurrentVersion() int { // the one machine that happens to stage that file. func (c *Config) Validate() error { for _, r := range c.Rules { + if !doublestar.ValidatePattern(r.Files) { + return fmt.Errorf("%w: rule %q", ErrInvalidPattern, r.Files) + } switch r.Mode { case ModeFile: if len(r.Encrypt) > 0 { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index eaf3459..2b4c7a8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -162,6 +162,24 @@ rules:`+tt.rule+` } } +// A malformed glob pattern is caught when the repository is opened, not +// silently treated by MatchRule as "never matches" — MatchRule's bool-only +// signature discards doublestar.Match's error, so an invalid pattern that +// slipped past this check would be a silent, permanent bypass for that +// rule (audit 2026-09-29, P07 sub-issue 3). +func TestLoadRejectsInvalidGlobPattern(t *testing.T) { + path := writeTempConfig(t, ` +salt: c3RydWNyeXB0LXRlc3Qtc2FsdC0xNg== +canary: "ENC[AES256_SIV,data:aGVsbG8=]" +rules: + - files: "secrets/[.pem" + mode: file +`) + if _, err := Load(path); !errors.Is(err, ErrInvalidPattern) { + t.Errorf("Load() error = %v, want %v", err, ErrInvalidPattern) + } +} + // A malformed field path is caught when the repository is opened, not // silently on whichever machine first stages a matching file. func TestLoadRejectsBadFieldPath(t *testing.T) { diff --git a/internal/filterop/filterop.go b/internal/filterop/filterop.go index 1613abc..4824475 100644 --- a/internal/filterop/filterop.go +++ b/internal/filterop/filterop.go @@ -12,6 +12,7 @@ package filterop import ( "errors" "fmt" + "strconv" "github.com/MuffinUser/nebel/internal/config" "github.com/MuffinUser/nebel/internal/format" @@ -30,9 +31,24 @@ type Keyring map[int][]byte // this machine's keyring does not hold. var ErrKeyVersionMissing = errors.New("filterop: key version not registered locally") -// Clean encrypts input if filePath matches a mode: file rule; otherwise (no -// matching rule, or the value is already encrypted) it returns input -// unchanged. +// ErrNoMatchingRule is returned when Clean or Smudge is invoked for a path +// that no rule in .nebel.yaml matches. Git only calls this filter for a +// path .gitattributes marks filter=nebel, so reaching this case means +// .gitattributes and .nebel.yaml disagree about that path — most often +// because a bare pattern like "*.env" matches recursively in +// .gitattributes but, per doublestar's semantics (see config.Rule.Files), +// only at the depth it's written at. Passing content through unchanged +// here, as earlier versions did, would either commit it as plaintext +// (Clean) or leave the mismatch invisible (Smudge); erroring instead +// blocks the git operation, since filter.nebel.required is set to true. +var ErrNoMatchingRule = errors.New("filterop: path has git attribute filter=nebel but no rule in .nebel.yaml matches it — check .gitattributes and .nebel.yaml for a pattern mismatch") + +// Clean encrypts input if filePath matches a mode: file rule; if the value +// is already encrypted it returns input unchanged. A filePath that matches +// no rule at all is ErrNoMatchingRule: git only invokes this filter for a +// path .gitattributes marks filter=nebel, so a non-match here means +// .gitattributes and .nebel.yaml disagree about that path, not that the +// path is legitimately unmanaged. // // filePath must be the file's path relative to the repository root — it is // both the rule-matching key and part of the AAD binding the ciphertext to @@ -57,7 +73,7 @@ var ErrKeyVersionMissing = errors.New("filterop: key version not registered loca func Clean(cfg *config.Config, keyring Keyring, filePath string, input []byte) ([]byte, error) { rule, ok := cfg.MatchRule(filePath) if !ok { - return input, nil + return nil, fmt.Errorf("%w: %s", ErrNoMatchingRule, filePath) } version := cfg.CurrentVersion() key, haveKey := keyring[version] @@ -102,7 +118,9 @@ type Skipped struct { // at all passes through unchanged (AC-6.7 — e.g. a file not yet migrated). // A value that *is* tagged but fails to parse or authenticate is a genuine // corruption or tampering signal and is reported as an error rather than -// smudged into a wrong-but-plausible plaintext (AC-6.9). +// smudged into a wrong-but-plausible plaintext (AC-6.9). A filePath that +// matches no rule at all is ErrNoMatchingRule, for the same reason as in +// Clean: git would not have invoked this filter for it otherwise. // // Unlike Clean, Smudge selects its key by the value's own tag-declared // version (AC-6.13), not the config's current version. A well-formed tag @@ -116,7 +134,7 @@ type Skipped struct { func Smudge(cfg *config.Config, keyring Keyring, filePath string, input []byte) ([]byte, []Skipped, error) { rule, ok := cfg.MatchRule(filePath) if !ok { - return input, nil, nil + return nil, nil, fmt.Errorf("%w: %s", ErrNoMatchingRule, filePath) } if rule.Mode == config.ModeValue { return smudgeValues(rule, keyring, filePath, input) @@ -172,7 +190,12 @@ func cleanValues(rule config.Rule, version int, key []byte, haveKey bool, filePa return nil, fmt.Errorf("%w: current version %d — run `nebel init`", ErrKeyVersionMissing, version) } - ciphertext, err := siv.Encrypt(key, []byte(span.Value), siv.AAD(siv.ModeValue, filePath, field)) + // The type travels in the tag as plaintext metadata, not inside + // the ciphertext — so it's bound into the AAD instead, exactly + // like the file/field path components, otherwise it could be + // swapped (type:str -> type:bool) without needing the key at all + // (audit 2026-09-29, P05; spec 03 AC-3.10). + ciphertext, err := siv.Encrypt(key, []byte(span.Value), siv.AAD(siv.ModeValue, filePath, field, string(span.Type))) if err != nil { return nil, fmt.Errorf("filterop: clean %s at %s: %w", filePath, field, err) } @@ -226,16 +249,67 @@ func smudgeValues(rule config.Rule, keyring Keyring, filePath string, input []by continue } - plaintext, err := siv.Decrypt(key, parsed.Ciphertext, siv.AAD(siv.ModeValue, filePath, field)) + // A legacy (pre-P05-fix) tag's ciphertext was never authenticated + // with its type in the AAD to begin with — reconstructing it with + // the type included here would fail authentication against a + // perfectly legitimate, untampered legacy tag, exactly as it + // would against a tampered one. Only a tag using + // AlgoAES256SIVTypeBound (Tag.TypeBound) was encrypted that way. + aad := siv.AAD(siv.ModeValue, filePath, field) + if parsed.TypeBound { + aad = siv.AAD(siv.ModeValue, filePath, field, string(parsed.Type)) + } + plaintext, err := siv.Decrypt(key, parsed.Ciphertext, aad) if err != nil { return nil, nil, fmt.Errorf("filterop: smudge %s at %s: %w", filePath, field, err) } + // Stopgap for a legacy tag's still-unauthenticated type field + // (parsed.TypeBound == false, see the AAD selection above): a type + // flip there needs no key, and Render writes TypeInt/TypeFloat/ + // TypeBool out unquoted with no validation of its own. Refusing a + // plaintext that isn't a valid literal for its claimed type won't + // catch every flip (e.g. a string that happens to read "false" + // stays ambiguous either way — see + // TestSmudgeValuesLegacyTagTypeStillUnauthenticated), but it keeps + // a successful flip from unquoting arbitrary attacker-controlled + // text, which for a crafted value could otherwise inject + // structure into the surrounding document. A type-bound tag can't + // reach this in practice (its type can't change without failing + // the Decrypt above), so this never rejects legitimate content. + if err := validTypeLiteral(string(plaintext), parsed.Type); err != nil { + return nil, nil, fmt.Errorf("filterop: smudge %s at %s: %w", filePath, field, err) + } edits = append(edits, format.Edit{Span: span, Text: handler.Render(string(plaintext), parsed.Type)}) } out, err := splice(input, edits, "smudge", filePath) return out, skipped, err } +// ErrInvalidTypeLiteral is returned when a decrypted value isn't a +// syntactically valid literal for its tag's claimed non-string type — see +// validTypeLiteral. +var ErrInvalidTypeLiteral = errors.New("filterop: decrypted value is not a valid literal for its claimed type") + +// validTypeLiteral rejects a plaintext that doesn't look like a genuine +// int/float/bool literal before it gets rendered unquoted. TypeStr always +// passes: Render quotes it, so whatever bytes it contains stay inert data +// rather than becoming unquoted source syntax. +func validTypeLiteral(value string, t tag.Type) error { + var err error + switch t { + case tag.TypeBool: + _, err = strconv.ParseBool(value) + case tag.TypeInt: + _, err = strconv.ParseInt(value, 10, 64) + case tag.TypeFloat: + _, err = strconv.ParseFloat(value, 64) + } + if err != nil { + return fmt.Errorf("%w: %q is not a valid %s", ErrInvalidTypeLiteral, value, t) + } + return nil +} + func splice(input []byte, edits []format.Edit, op, filePath string) ([]byte, error) { out, err := format.Splice(input, edits) if err != nil { diff --git a/internal/filterop/filterop_test.go b/internal/filterop/filterop_test.go index ca443ee..4247b4e 100644 --- a/internal/filterop/filterop_test.go +++ b/internal/filterop/filterop_test.go @@ -5,6 +5,7 @@ package filterop import ( "bytes" + "errors" "testing" "github.com/MuffinUser/nebel/internal/config" @@ -101,23 +102,23 @@ func TestRoundTrip(t *testing.T) { } } -// A file that matches no rule passes through unchanged on both operations. -func TestUnmatchedFilePassesThrough(t *testing.T) { +// A path git invokes the filter for (it always names one filePath argument +// per invocation) but that no rule matches is a .gitattributes/.nebel.yaml +// mismatch, not a legitimately unmanaged file — git would not have called +// clean/smudge for it otherwise. Passing content through here would either +// commit it as plaintext (Clean) or hide the mismatch (Smudge), so both +// report ErrNoMatchingRule instead. +func TestUnmatchedFileErrors(t *testing.T) { input := []byte("plain content") - cleaned, err := Clean(testConfig(), testKeyring, "README.md", input) - if err != nil { - t.Fatalf("Clean: %v", err) - } - if !bytes.Equal(cleaned, input) { - t.Errorf("Clean() on an unmatched file changed the content: %q", cleaned) - } - smudged, _, err := Smudge(testConfig(), testKeyring, "README.md", input) - if err != nil { - t.Fatalf("Smudge: %v", err) + _, err := Clean(testConfig(), testKeyring, "README.md", input) + if !errors.Is(err, ErrNoMatchingRule) { + t.Errorf("Clean() error = %v, want %v", err, ErrNoMatchingRule) } - if !bytes.Equal(smudged, input) { - t.Errorf("Smudge() on an unmatched file changed the content: %q", smudged) + + _, _, err = Smudge(testConfig(), testKeyring, "README.md", input) + if !errors.Is(err, ErrNoMatchingRule) { + t.Errorf("Smudge() error = %v, want %v", err, ErrNoMatchingRule) } } diff --git a/internal/filterop/value_test.go b/internal/filterop/value_test.go index 9dc7314..db92bf4 100644 --- a/internal/filterop/value_test.go +++ b/internal/filterop/value_test.go @@ -5,11 +5,14 @@ package filterop import ( "bytes" + "encoding/base64" "errors" + "fmt" "strings" "testing" "github.com/MuffinUser/nebel/internal/config" + "github.com/MuffinUser/nebel/internal/siv" "github.com/MuffinUser/nebel/internal/tag" ) @@ -48,8 +51,8 @@ func TestCleanValuesEncryptsOnlyConfiguredFields(t *testing.T) { t.Errorf("clean disturbed content it should not touch (%q):\n%s", kept, got) } } - if !strings.Contains(string(got), `password: "ENC[AES256_SIV,`) { - t.Errorf("encrypted field is not a quoted ENC[...] tag:\n%s", got) + if !strings.Contains(string(got), `password: "ENC[AES256_SIV_TB,`) { + t.Errorf("encrypted field is not a quoted, type-bound ENC[...] tag:\n%s", got) } } @@ -326,3 +329,108 @@ func TestSmudgeValuesRejectsUntypedTag(t *testing.T) { t.Error("Smudge on an untyped tag: want an error, got nil") } } + +// Regression test for the 2026-09-29 audit's P05: a value's type field +// sits next to its ciphertext as plaintext metadata, not inside it, so +// changing type:str to type:bool needs no key at all. Before this fix, +// that silently changed how a decrypted string rendered (a boolean or +// number, unquoted, instead of a quoted string) without tripping SIV's +// own authentication, since the AAD never covered the type field. Now +// that EncodeValue's tag binds Type into the AAD, the same edit must fail +// authentication instead. +func TestSmudgeValuesRejectsTypeTampering(t *testing.T) { + cleaned, err := Clean(valueConfig(), testKeyring, "config/staging.yaml", []byte(valueDoc)) + if err != nil { + t.Fatalf("Clean: %v", err) + } + if !bytes.Contains(cleaned, []byte(",type:str]")) { + t.Fatalf("expected a type:str tag, got:\n%s", cleaned) + } + tampered := bytes.Replace(cleaned, []byte(",type:str]"), []byte(",type:bool]"), 1) + + if _, _, err := Smudge(valueConfig(), testKeyring, "config/staging.yaml", tampered); !errors.Is(err, siv.ErrAuth) { + t.Errorf("Smudge() on a type-tampered tag: error = %v, want %v", err, siv.ErrAuth) + } +} + +// Backward compatibility: a value tag committed before this fix +// (AlgoAES256SIV, encrypted with the pre-P05 AAD that never included the +// type) has no in-tool migration and must keep decrypting exactly as it +// always did — see the tag package's doc comment. This hand-builds one +// the way a pre-fix EncodeValue would have, rather than relying on the +// current (now type-bound) EncodeValue. +func TestSmudgeValuesAcceptsLegacyPreTypeBoundTag(t *testing.T) { + const legacyPassword = "old-secret" + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.yaml", "database.password") + ciphertext, err := siv.Encrypt(testKey, []byte(legacyPassword), legacyAAD) + if err != nil { + t.Fatalf("siv.Encrypt: %v", err) + } + legacyTag := fmt.Sprintf("ENC[%s,key:1,data:%s,type:str]", tag.AlgoAES256SIV, base64.StdEncoding.EncodeToString(ciphertext)) + + doc := strings.Replace(valueDoc, `password: "s3cr3t"`, `password: "`+legacyTag+`"`, 1) + got, _, err := Smudge(valueConfig(), testKeyring, "config/staging.yaml", []byte(doc)) + if err != nil { + t.Fatalf("Smudge on a legacy pre-type-bound tag: %v", err) + } + if !bytes.Contains(got, []byte("password: \""+legacyPassword+"\"")) { + t.Errorf("Smudge() = %q, want it to decrypt the legacy tag back to %q", got, legacyPassword) + } +} + +// Documents, rather than asserts as desirable, the residual limitation +// backward compatibility with TestSmudgeValuesAcceptsLegacyPreTypeBoundTag +// requires: a *legacy* tag's type field is not authenticated, exactly as +// it always was, since re-authenticating it under the new AAD formula +// would break every such tag already committed before this fix. A legacy +// value converges onto the safer, type-bound form the next time anything +// re-encrypts it (an ordinary edit, `nebel rotate`, or `git add +// --renormalize`) — see the tag package's doc comment. What remains is +// narrower than the original P05 finding, though, not the same gap: +// validTypeLiteral (see TestSmudgeValuesRejectsNonLiteralAfterLegacyTypeFlip) +// refuses a plaintext that isn't a genuine literal for the type it's been +// flipped to, so a flip only "succeeds" — as here — when the underlying +// plaintext happens to also be a valid literal of the new type, same as +// the string "false" is also a valid bool literal. +func TestSmudgeValuesLegacyTagTypeStillUnauthenticated(t *testing.T) { + const legacyValue = "false" + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.yaml", "database.password") + ciphertext, err := siv.Encrypt(testKey, []byte(legacyValue), legacyAAD) + if err != nil { + t.Fatalf("siv.Encrypt: %v", err) + } + legacyTag := fmt.Sprintf("ENC[%s,key:1,data:%s,type:str]", tag.AlgoAES256SIV, base64.StdEncoding.EncodeToString(ciphertext)) + tamperedTag := strings.Replace(legacyTag, ",type:str]", ",type:bool]", 1) + + doc := strings.Replace(valueDoc, `password: "s3cr3t"`, `password: "`+tamperedTag+`"`, 1) + got, _, err := Smudge(valueConfig(), testKeyring, "config/staging.yaml", []byte(doc)) + if err != nil { + t.Fatalf("Smudge on a type-tampered legacy tag: want no error (known limitation), got %v", err) + } + if !bytes.Contains(got, []byte("password: "+legacyValue)) { + t.Errorf("Smudge() = %q, want the unquoted rendering %q the type-flip produces", got, "password: "+legacyValue) + } +} + +// The stopgap validTypeLiteral adds: a legacy tag's type field still +// isn't authenticated (see TestSmudgeValuesLegacyTagTypeStillUnauthenticated), +// but flipping type:str to type:bool on a plaintext that isn't itself a +// valid bool literal — ordinary secret text, not crafted to also read as +// one — must now be refused rather than rendered unquoted. Unquoted +// arbitrary text is exactly how a crafted plaintext could inject +// structure into the surrounding document. +func TestSmudgeValuesRejectsNonLiteralAfterLegacyTypeFlip(t *testing.T) { + const legacyValue = "s3cr3t" + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.yaml", "database.password") + ciphertext, err := siv.Encrypt(testKey, []byte(legacyValue), legacyAAD) + if err != nil { + t.Fatalf("siv.Encrypt: %v", err) + } + legacyTag := fmt.Sprintf("ENC[%s,key:1,data:%s,type:str]", tag.AlgoAES256SIV, base64.StdEncoding.EncodeToString(ciphertext)) + tamperedTag := strings.Replace(legacyTag, ",type:str]", ",type:bool]", 1) + + doc := strings.Replace(valueDoc, `password: "s3cr3t"`, `password: "`+tamperedTag+`"`, 1) + if _, _, err := Smudge(valueConfig(), testKeyring, "config/staging.yaml", []byte(doc)); !errors.Is(err, ErrInvalidTypeLiteral) { + t.Errorf("Smudge() error = %v, want %v", err, ErrInvalidTypeLiteral) + } +} diff --git a/internal/format/format.go b/internal/format/format.go index f637dab..50e17ea 100644 --- a/internal/format/format.go +++ b/internal/format/format.go @@ -25,8 +25,43 @@ var ( // ErrNotScalar is returned when a path resolves to a mapping or // sequence rather than a single value. ErrNotScalar = errors.New("format: path does not resolve to a scalar") + + // ErrAmbiguousPath is returned when a configured path selects more + // than one distinct location in the same document. Two ways this + // happens: a literal key containing a dot collides with the + // equivalent nested path — dot notation can't tell "a.b" the flat key + // from "a" containing "b" apart, and a dot inside a key can't be + // escaped (ParsePath's doc comment) — or an outright duplicate key at + // the same nesting level (JSON's streaming decoder does not reject + // these the way go-yaml's parser does). Locate's traversal can only + // ever resolve to one of them; picking one silently, as earlier + // versions did, could protect the wrong (or only one) of several + // values while reporting success — the same silent-failure concern + // ErrNotFound already guards for a path that resolves to nothing. + ErrAmbiguousPath = errors.New("format: path is ambiguous") ) +// checkUnambiguous reports ErrAmbiguousPath if path selects more than one +// leaf in src (see ErrAmbiguousPath), by cross-checking against h.Leaves, +// which — unlike a targeted descent — visits every literal key in the +// document and so surfaces a collision a single-path lookup never would. +func checkUnambiguous(h Handler, src []byte, path string) error { + leaves, err := h.Leaves(src) + if err != nil { + return err + } + matches := 0 + for _, leaf := range leaves { + if leaf.Path == path { + matches++ + } + } + if matches > 1 { + return fmt.Errorf("%w: %q matches %d different values", ErrAmbiguousPath, path, matches) + } + return nil +} + // Span is one located scalar: where it sits in the source, and what it is. type Span struct { // Start and End bound the scalar as written, including any quotes, so diff --git a/internal/format/format_test.go b/internal/format/format_test.go index 08d8543..e914ebe 100644 --- a/internal/format/format_test.go +++ b/internal/format/format_test.go @@ -132,6 +132,115 @@ func TestLocate(t *testing.T) { } } +// Regression test for the 2026-09-29 audit's P02: a quoted YAML scalar +// containing a literal " #" (space-hash) inside the quotes must locate as +// its full quoted literal, not be truncated at the in-quote "#" as if it +// were a trailing comment. Splicing over a truncated span, as the pre-fix +// code did, would leave the untruncated remainder as literal plaintext in +// the git blob right next to the encrypted tag — confirmed directly +// against the pre-fix code (see the commit message). +func TestLocateQuotedScalarWithHash(t *testing.T) { + for _, tt := range []struct { + name string + src string + wantRaw string + wantValue string + }{ + {"double-quoted, hash inside, no trailing comment", + `password: "abc # geheim"` + "\n", + `"abc # geheim"`, "abc # geheim"}, + {"single-quoted, hash inside, no trailing comment", + `password: 'abc # geheim'` + "\n", + `'abc # geheim'`, "abc # geheim"}, + {"double-quoted, hash inside, plus a real trailing comment", + `password: "abc # geheim" # a real comment` + "\n", + `"abc # geheim"`, "abc # geheim"}, + {"double-quoted, hash inside, no trailing newline", + `password: "abc # geheim"`, + `"abc # geheim"`, "abc # geheim"}, + } { + t.Run(tt.name, func(t *testing.T) { + h, err := For("c.yaml") + if err != nil { + t.Fatal(err) + } + span, err := h.Locate([]byte(tt.src), "password") + if err != nil { + t.Fatalf("Locate: %v", err) + } + if got := tt.src[span.Start:span.End]; got != tt.wantRaw { + t.Errorf("span covers %q, want %q", got, tt.wantRaw) + } + if span.Value != tt.wantValue { + t.Errorf("Value = %q, want %q", span.Value, tt.wantValue) + } + + // The real-world consequence: splicing an encrypted tag over + // this span must leave nothing of the secret in the result. + const tagText = `"ENC[AES256_SIV,data:AAAA,type:str]"` + out, err := Splice([]byte(tt.src), []Edit{{Span: span, Text: tagText}}) + if err != nil { + t.Fatalf("Splice: %v", err) + } + if strings.Contains(string(out), "geheim") { + t.Errorf("spliced output still contains plaintext: %q", out) + } + }) + } +} + +// A plain (unquoted) scalar's own trailing comment must still be stripped +// exactly as before — this fix must not regress the case scalarText was +// originally written for. +func TestLocatePlainScalarWithTrailingComment(t *testing.T) { + h, err := For("c.yaml") + if err != nil { + t.Fatal(err) + } + src := "password: abc # a real comment\n" + span, err := h.Locate([]byte(src), "password") + if err != nil { + t.Fatalf("Locate: %v", err) + } + if got := src[span.Start:span.End]; got != "abc" { + t.Errorf("span covers %q, want %q", got, "abc") + } +} + +// Regression test for a corruption bug found while fixing P02: go-yaml's +// AST represents a literal (|) or folded (>) block scalar with a header +// token (just the "|"/">" indicator) separate from its actual content +// token, and *ast.LiteralNode.GetToken() — which Locate used +// unconditionally — returns only the header. Before this guard, Locate +// would splice a ciphertext tag over the single header character, +// destroying the document instead of protecting the secret beneath it. +func TestLocateRejectsBlockScalars(t *testing.T) { + for _, tt := range []struct{ name, src string }{ + {"literal block", "password: |\n line one\n line two\n"}, + {"folded block", "password: >\n line one\n line two\n"}, + } { + t.Run(tt.name, func(t *testing.T) { + h, err := For("c.yaml") + if err != nil { + t.Fatal(err) + } + if _, err := h.Locate([]byte(tt.src), "password"); !errors.Is(err, ErrBlockScalarUnsupported) { + t.Errorf("Locate() error = %v, want %v", err, ErrBlockScalarUnsupported) + } + + leaves, err := h.Leaves([]byte(tt.src)) + if err != nil { + t.Fatalf("Leaves: %v", err) + } + for _, leaf := range leaves { + if leaf.Path == "password" { + t.Errorf("Leaves() offered a block scalar field: %+v", leaf) + } + } + }) + } +} + // The CRLF regression: go-yaml's own Line/Column tracking miscounts // against raw "\r\n" — a real-world case, not a hypothetical one, since // Windows checking out a repo with core.autocrlf on reintroduces "\r" @@ -322,6 +431,57 @@ func TestLocateRejectsNonScalar(t *testing.T) { } } +// Regression tests for the 2026-09-29 audit's P03 and P04: a configured +// path that resolves to more than one distinct value in the same document +// must be refused, not silently resolved to whichever candidate a +// targeted descent happens to reach first. +func TestLocateRejectsAmbiguousPath(t *testing.T) { + for _, tt := range []struct { + name, file, src, path string + }{ + // P03: a literal key containing a dot ("a.b") collides with the + // equivalent nested path ("a" containing "b") — dot notation + // can't tell them apart. Before the fix, Locate always resolved + // to the nested value ("decoy"), silently leaving the flat key's + // "secret" unencrypted while reporting success. + {"json dotted key collides with nested path", "c.json", `{"a.b":"secret","a":{"b":"decoy"}}`, "a.b"}, + {"yaml dotted key collides with nested path", "c.yaml", "\"a.b\": secret\na:\n b: decoy\n", "a.b"}, + + // P04: encoding/json's streaming decoder does not reject a + // duplicate object key the way go-yaml's parser does (see + // TestYAMLParserRejectsDuplicateKeys) — before the fix, + // descendObject returned the first occurrence's span, leaving + // the second occurrence (which, per encoding/json's own + // map-unmarshal semantics, most consumers would treat as the + // effective value) in plaintext. + {"json duplicate key", "c.json", `{"password":"eins","password":"zwei"}`, "password"}, + } { + t.Run(tt.name, func(t *testing.T) { + h, err := For(tt.file) + if err != nil { + t.Fatal(err) + } + if _, err := h.Locate([]byte(tt.src), tt.path); !errors.Is(err, ErrAmbiguousPath) { + t.Errorf("Locate(%q) error = %v, want %v", tt.path, err, ErrAmbiguousPath) + } + }) + } +} + +// Documents, rather than fixes, existing behavior: go-yaml's parser +// already rejects an outright duplicate mapping key at parse time, unlike +// encoding/json's streaming decoder (see TestLocateRejectsAmbiguousPath's +// "json duplicate key" case) — so YAML needed no equivalent fix for P04. +func TestYAMLParserRejectsDuplicateKeys(t *testing.T) { + h, err := For("c.yaml") + if err != nil { + t.Fatal(err) + } + if _, err := h.Locate([]byte("password: eins\npassword: zwei\n"), "password"); err == nil { + t.Error("Locate() on a YAML document with a duplicate key: want an error, got nil") + } +} + // Render is the decryption side: a restored value must come back with its // original type, not as a quoted string (AC-3.2). func TestRender(t *testing.T) { diff --git a/internal/format/json.go b/internal/format/json.go index f605808..be7a310 100644 --- a/internal/format/json.go +++ b/internal/format/json.go @@ -21,11 +21,14 @@ type jsonHandler struct{} // possible: json.Unmarshal would discard the offsets, and re-marshalling // would normalize key order, indentation and number formatting across the // whole document. -func (jsonHandler) Locate(src []byte, path string) (Span, error) { +func (h jsonHandler) Locate(src []byte, path string) (Span, error) { steps, err := ParsePath(path) if err != nil { return Span{}, err } + if err := checkUnambiguous(h, src, path); err != nil { + return Span{}, err + } dec := json.NewDecoder(bytes.NewReader(src)) dec.UseNumber() diff --git a/internal/format/yaml.go b/internal/format/yaml.go index 01f45de..75b567b 100644 --- a/internal/format/yaml.go +++ b/internal/format/yaml.go @@ -5,6 +5,7 @@ package format import ( "bytes" + "errors" "fmt" "strings" @@ -28,11 +29,14 @@ type yamlHandler struct{} // resulting span back to real offsets in src, so Locate still splices // over the actual bytes on disk. When src has no "\r\n" at all, the map // is the identity and this is exactly the original behavior. -func (yamlHandler) Locate(src []byte, path string) (Span, error) { +func (h yamlHandler) Locate(src []byte, path string) (Span, error) { steps, err := ParsePath(path) if err != nil { return Span{}, err } + if err := checkUnambiguous(h, src, path); err != nil { + return Span{}, err + } m := newCRLFMap(src) @@ -58,6 +62,9 @@ func (yamlHandler) Locate(src []byte, path string) (Span, error) { } } + if _, isBlock := node.(*ast.LiteralNode); isBlock { + return Span{}, fmt.Errorf("%w: %q", ErrBlockScalarUnsupported, path) + } scalar, ok := node.(ast.ScalarNode) if !ok { return Span{}, fmt.Errorf("%w: %q is a %s", ErrNotScalar, path, node.Type()) @@ -65,6 +72,22 @@ func (yamlHandler) Locate(src []byte, path string) (Span, error) { return yamlSpan(src, m, scalar.GetToken(), path) } +// ErrBlockScalarUnsupported is returned when a configured path resolves to +// a literal (|) or folded (>) block scalar. +// +// *ast.LiteralNode (go-yaml's AST type for both styles) wraps two tokens: +// a header token holding just the "|" or ">" indicator itself, and a +// separate Value token holding the actual decoded multi-line text. +// *ast.LiteralNode satisfies ast.ScalarNode, and its GetToken() returns +// only the header — so treating it as an ordinary scalar, as this handler +// did before this check existed, would locate and splice a ciphertext tag +// over the single header character, corrupting the document instead of +// protecting the secret underneath it. This was found while fixing P02, +// not part of the audit that motivated it; there is no format-side +// handling for these styles to fall back on, so refusing them outright is +// the safe behavior until they're properly supported. +var ErrBlockScalarUnsupported = errors.New("format: literal (|) and folded (>) block scalars are not supported as mode: value fields") + // crlfMap translates byte offsets computed against a CRLF-normalized copy // of a source buffer (every "\r\n" replaced by "\n") back to offsets in // the original buffer. @@ -179,10 +202,36 @@ func yamlSpan(src []byte, m crlfMap, tk *token.Token, path string) (Span, error) } // scalarText strips the trivia the lexer folds into a token's Origin: the -// surrounding whitespace, and any comment that follows the value on the -// same line. +// surrounding whitespace, and — for a plain (unquoted) scalar only — any +// comment that follows the value on the same line. +// +// A quoted scalar's Origin is, verified empirically against goccy/go-yaml, +// always exactly the quoted literal verbatim (opening quote through +// closing quote), with no trailing trivia of any kind ever folded in — +// not a following comment, not trailing whitespace, regardless of what +// follows it in the source. Searching it for " #" would instead find one +// *inside* the quotes, wrongly truncating a value like `"abc # geheim"` +// to `"abc` and leaving the rest (`# geheim"`) as literal plaintext next +// to the encrypted tag (audit 2026-09-29, P02) — so a quoted scalar's +// trimmed Origin is returned as-is, unsearched. +// +// A plain scalar's Origin can never legitimately contain " #" as its own +// content in the first place: the YAML lexer itself treats " #" as the +// start of a comment and stops accumulating a plain scalar's Origin right +// there, whatever the author intended. The search below is therefore a +// no-op in practice for a well-formed plain scalar token; it is kept as +// the one non-quoted path through this function, rather than special- +// cased away, so a lexer that ever did fold trailing trivia in for some +// plain-scalar edge case would still be handled instead of silently +// trusted. func scalarText(origin string) string { text := strings.TrimSpace(origin) + if text == "" { + return text + } + if text[0] == '"' || text[0] == '\'' { + return text + } if hash := strings.Index(text, " #"); hash >= 0 { text = strings.TrimRight(text[:hash], " \t") } @@ -299,6 +348,11 @@ func (yamlHandler) Leaves(src []byte) ([]Leaf, error) { for i, item := range n.Values { walk(item, indexPath(path, i)) } + case *ast.LiteralNode: + // A block scalar (|, >) is never offered: see + // ErrBlockScalarUnsupported. This case must come before the + // ast.ScalarNode one below, which *ast.LiteralNode also + // satisfies but would report the wrong token. case ast.ScalarNode: // A scalar at the document root has no path to address it by. if path != "" { diff --git a/internal/gitutil/gitutil.go b/internal/gitutil/gitutil.go index 9752837..3a6e25c 100644 --- a/internal/gitutil/gitutil.go +++ b/internal/gitutil/gitutil.go @@ -184,6 +184,52 @@ func IsShallow(repoRoot string) (bool, error) { return strings.TrimSpace(string(out)) == "true", nil } +// ErrDirtyManagedFiles is returned when CheckoutAll is asked to re-smudge +// a managed file that has staged or unstaged local changes. Removing and +// re-checking-out such a file, as CheckoutAll otherwise unconditionally +// does, would silently discard that work: `git checkout HEAD -- ` +// overwrites the working tree from HEAD regardless of the index or of any +// unstaged edit, and CheckoutAll's own preceding os.Remove has no backup +// step of its own either (audit 2026-09-29, P01). +// +// DirtyFiles compares by running the clean filter (that's what `git diff +// HEAD` against a filtered path does), which can itself be the reason a +// file not actually edited shows up here: a mode: value field's tag from +// before spec 03 AC-3.10 authenticated its type re-encrypts to different +// bytes than what's committed the moment anything cleans it again, since +// the AAD it's produced under changed. That's a real difference the +// filter would otherwise stage silently, so still refusing here is the +// right default — the error message points at the fix (`git add +// --renormalize`, committed, upgrades the tag; renormalizing without +// committing only updates the index, and this check compares against +// HEAD regardless) rather than assuming it's always an actual edit. +var ErrDirtyManagedFiles = errors.New("gitutil: managed file(s) have local changes CheckoutAll would discard") + +// DirtyFiles returns the subset of files (repo-relative paths) whose +// working tree content differs from HEAD — staged and unstaged changes +// alike, since `git diff HEAD --` compares the working tree directly +// against HEAD regardless of what's in the index. That is exactly what +// matters to a caller about to overwrite the working tree from HEAD: an +// edit only staged, not committed, is just as much at risk as one that +// was never staged at all. +func DirtyFiles(repoRoot string, files []string) ([]string, error) { + if len(files) == 0 { + return nil, nil + } + args := append([]string{"diff", "--name-only", "HEAD", "--"}, files...) + cmd := exec.Command("git", args...) + cmd.Dir = repoRoot + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("gitutil: git diff HEAD: %w", wrapExitErr(err)) + } + trimmed := strings.TrimSpace(string(out)) + if trimmed == "" { + return nil, nil + } + return strings.Split(trimmed, "\n"), nil +} + // CheckoutFailure records why one managed file couldn't be re-smudged — // git's own trimmed error output, which already carries nebel's specific // message (AC-6.9's tamper error, say) rather than a cause CheckoutAll's @@ -223,6 +269,16 @@ type CheckoutFailure struct { // // decrypted+len(failures) == total managed files; total == 0 means no // managed files exist at all (e.g. .gitattributes was never committed). +// +// Before touching anything, every managed file is checked against HEAD +// (DirtyFiles): CheckoutAll's own os.Remove-then-checkout has no backup +// step, and `git checkout HEAD -- ` restores from HEAD regardless +// of the index, so a staged or unstaged local edit would otherwise be +// silently discarded (audit 2026-09-29, P01) — this is reachable through +// an ordinary repeated `nebel init` or `nebel init --version N`, not only +// through unusual input, so refusing outright rather than proceeding is +// the safe default; the caller decides how to advise the user from there +// (commit or stash first, typically). func CheckoutAll(repoRoot string) (decrypted int, failures []CheckoutFailure, err error) { files, err := ManagedFiles(repoRoot) if err != nil { @@ -231,6 +287,11 @@ func CheckoutAll(repoRoot string) (decrypted int, failures []CheckoutFailure, er if len(files) == 0 { return 0, nil, nil } + if dirty, dirtyErr := DirtyFiles(repoRoot, files); dirtyErr != nil { + return 0, nil, dirtyErr + } else if len(dirty) > 0 { + return 0, nil, fmt.Errorf("%w: %s (if these have not actually been edited, a mode: value field's tag may still be in the pre-authenticated-type form — run `git add --renormalize -- `, commit the result, then retry; renormalizing alone updates the index, not HEAD, which this check compares against)", ErrDirtyManagedFiles, strings.Join(dirty, ", ")) + } for _, f := range files { if err := os.Remove(filepath.Join(repoRoot, f)); err != nil && !os.IsNotExist(err) { diff --git a/internal/gitutil/gitutil_test.go b/internal/gitutil/gitutil_test.go index adb5253..5a6557d 100644 --- a/internal/gitutil/gitutil_test.go +++ b/internal/gitutil/gitutil_test.go @@ -4,6 +4,7 @@ package gitutil import ( + "errors" "os" "os/exec" "path/filepath" @@ -276,3 +277,91 @@ func TestCheckoutAllIsolatesPerFileSmudgeFailures(t *testing.T) { t.Errorf("bad.txt = %q, want the raw committed blob %q", bad, "bad-plaintext\n") } } + +// registerCatFilter wires the nebel filter to a no-op "cat" for both +// directions, enough for CheckoutAll's dirty-check to be exercised +// without needing a real clean/smudge transform. +func registerCatFilter(t *testing.T, dir string) { + t.Helper() + run := func(args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("config", "filter.nebel.clean", "cat") + run("config", "filter.nebel.smudge", "cat") + run("config", "filter.nebel.required", "true") +} + +// Regression test for the 2026-09-29 audit's P01: CheckoutAll's +// os.Remove-then-checkout has no backup step, and `git checkout HEAD --` +// restores from HEAD regardless of the index — so before this fix, a +// repeated `nebel init` (which calls CheckoutAll) silently discarded any +// local edit to a managed file, staged or not. Both must now be refused +// outright, with the local edit left completely untouched. +func TestCheckoutAllRefusesDirtyManagedFiles(t *testing.T) { + for _, tt := range []struct { + name string + stage bool + }{ + {"unstaged edit", false}, + {"staged edit", true}, + } { + t.Run(tt.name, func(t *testing.T) { + dir := newTempRepo(t) + if err := os.WriteFile(filepath.Join(dir, ".gitattributes"), []byte("*.txt filter=nebel\n"), 0o644); err != nil { + t.Fatal(err) + } + writeAndCommit(t, dir, "secret.txt", "original\n") + registerCatFilter(t, dir) + + const edited = "locally edited, not yet committed\n" + path := filepath.Join(dir, "secret.txt") + if err := os.WriteFile(path, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + if tt.stage { + cmd := exec.Command("git", "add", "--", "secret.txt") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v\n%s", err, out) + } + } + + _, _, err := CheckoutAll(dir) + if !errors.Is(err, ErrDirtyManagedFiles) { + t.Fatalf("CheckoutAll() error = %v, want %v", err, ErrDirtyManagedFiles) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("secret.txt was removed despite CheckoutAll refusing: %v", err) + } + if string(got) != edited { + t.Errorf("secret.txt = %q, want the local edit left untouched: %q", got, edited) + } + }) + } +} + +// The ordinary case CheckoutAll exists for — no local changes at all, as +// right after `nebel init` on a fresh clone — must not be blocked by the +// new dirty check. +func TestCheckoutAllSucceedsWhenNothingIsDirty(t *testing.T) { + dir := newTempRepo(t) + if err := os.WriteFile(filepath.Join(dir, ".gitattributes"), []byte("*.txt filter=nebel\n"), 0o644); err != nil { + t.Fatal(err) + } + writeAndCommit(t, dir, "secret.txt", "original\n") + registerCatFilter(t, dir) + + decrypted, failures, err := CheckoutAll(dir) + if err != nil { + t.Fatalf("CheckoutAll: %v", err) + } + if decrypted != 1 || len(failures) != 0 { + t.Errorf("CheckoutAll() = decrypted=%d failures=%v, want decrypted=1, no failures", decrypted, failures) + } +} diff --git a/internal/localkey/localkey.go b/internal/localkey/localkey.go index 9cc8c18..4288c3d 100644 --- a/internal/localkey/localkey.go +++ b/internal/localkey/localkey.go @@ -1,9 +1,9 @@ // Copyright (C) 2026 Marwin Moellers // SPDX-License-Identifier: GPL-3.0-or-later -// Package localkey stores derived AES-256-SIV keys in local (per-clone, not -// committed) git config, so their presence or absence is exactly the "has -// this machine run `nebel init`" signal the clean/smudge filter needs for +// Package localkey stores derived AES-256-SIV keys locally (per-clone, not +// committed), so their presence or absence is exactly the "has this +// machine run `nebel init`" signal the clean/smudge filter needs for // no-key passthrough (spec 06 AC-6.6). // // A machine may hold more than one key version at once (spec 11): every @@ -11,31 +11,60 @@ // cached, so it keeps being able to read content tagged with an older // version after the project rotates. // -// Keys sit in plaintext in .git/config on disk. That file is never -// committed or pushed, which is the same trust boundary git-crypt and -// transcrypt rely on for the same purpose. +// Keys sit in plaintext in a dedicated file inside the git directory (see +// keyFileName), never committed or pushed — the same trust boundary +// git-crypt and transcrypt rely on for the same purpose. A version +// registered before this file existed may still be found in the legacy +// location, local (not committed) git config (see readLegacyEntries); Get +// and All both still fall back to it, so an existing clone keeps working +// without needing to re-run `nebel init`. Set only ever writes to the new +// file. package localkey import ( "encoding/base64" "fmt" + "os" + "os/exec" + "path/filepath" + "sort" "strconv" "strings" "github.com/MuffinUser/nebel/internal/gitutil" ) -// configKeyPrefix is the local git config variable every version's key is -// stored under. Git config variable names may not contain "." themselves -// (only section/subsection separators may), so later versions suffix the -// number directly onto the name (key2, key3, ...) rather than adding -// another dot-separated segment. +// configKeyPrefix is the local git config variable a version's key was +// stored under before this file existed (see readLegacyEntries) — still +// read, for backward compatibility, but no longer written. Git config +// variable names may not contain "." themselves (only section/subsection +// separators may), so later versions suffixed the number directly onto +// the name (key2, key3, ...) rather than adding another dot-separated +// segment; version 1 kept the unsuffixed name every clone created before +// key rotation existed already wrote and read. const configKeyPrefix = "filter.nebel.key" -// configKeyFor returns the git config key a given version's derived key is -// stored under. Version 1 keeps the unsuffixed name every clone created -// before key rotation existed already writes and reads — so an upgraded -// binary keeps recognizing a key an older binary registered. +// keyFileName is the file, inside the git directory shared by every +// worktree of a clone (see gitCommonDir), that Set registers a key to. +// +// It replaces `git config --local ` as of the 2026-09-29 +// audit's P06 finding: that is a subprocess invocation, and its only way +// to set a value is to pass it as a literal argument — putting a +// ready-to-use derived decryption key directly into that process's +// argument list, readable by any other local user or process with +// sufficient privilege via /proc//cmdline for as long as the +// subprocess runs. This is the same class of exposure +// cmd/nebel/password.go's ErrPasswordArgument already refuses for the +// shared password itself; the derived key is at least as sensitive, and +// wasn't getting the same protection. Writing this file directly with +// Go's os package instead never puts the key on any process's argument +// list. Its 0o600 permissions (see writeKeyFile) are also a direct answer +// to a related, separate suggestion in the same audit to harden local key +// storage beyond .git/config's ambient permissions. +const keyFileName = "nebel-keys" + +// configKeyFor returns the legacy git config key a given version's key +// used to be stored under. func configKeyFor(version int) string { if version == 1 { return configKeyPrefix @@ -43,39 +72,114 @@ func configKeyFor(version int) string { return fmt.Sprintf("%s%d", configKeyPrefix, version) } -// Get returns the locally registered key for version. ok is false, with a -// nil error, when that version is not registered yet. -func Get(version int) (key []byte, ok bool, err error) { - encoded, ok, err := gitutil.ConfigGet(configKeyFor(version)) - if err != nil || !ok { - return nil, ok, err +// gitCommonDir returns the git directory shared across every worktree of +// this clone, as opposed to a linked worktree's own private git dir. +// Local git config is shared there by default (splitting it per-worktree +// requires the rare extensions.worktreeConfig) — storing the key file +// alongside it keeps it visible to every worktree exactly like the config +// entries it replaces were. +func gitCommonDir() (string, error) { + out, err := exec.Command("git", "rev-parse", "--git-common-dir").Output() + if err != nil { + return "", fmt.Errorf("localkey: git rev-parse --git-common-dir: %w", err) } - key, err = base64.StdEncoding.DecodeString(encoded) + dir := strings.TrimSpace(string(out)) + if !filepath.IsAbs(dir) { + // git commonly reports this relative to the caller's working + // directory (typically ".git") rather than as an absolute path. + abs, err := filepath.Abs(dir) + if err != nil { + return "", fmt.Errorf("localkey: resolving git common dir %q: %w", dir, err) + } + dir = abs + } + return dir, nil +} + +func keyFilePath() (string, error) { + dir, err := gitCommonDir() if err != nil { - return nil, false, fmt.Errorf("localkey: decoding stored key for version %d: %w", version, err) + return "", err } - return key, true, nil + return filepath.Join(dir, keyFileName), nil } -// Set registers key locally as version's key. Existing versions are left -// untouched (spec 11 AC-11.5): a machine that rotates, or fetches an older -// version via `nebel init --version N`, keeps reading everything it could -// read before. -func Set(version int, key []byte) error { - return gitutil.ConfigSet(configKeyFor(version), base64.StdEncoding.EncodeToString(key)) +// readKeyFile parses the key file into version -> base64-encoded key. A +// missing file is not an error: it means Set has never been called on +// this clone, or every key it holds still lives only in the legacy +// git-config location (see readLegacyEntries). +func readKeyFile() (map[int]string, error) { + path, err := keyFilePath() + if err != nil { + return nil, err + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return map[int]string{}, nil + } + return nil, fmt.Errorf("localkey: reading %s: %w", path, err) + } + + entries := map[int]string{} + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + versionStr, encoded, ok := strings.Cut(line, "=") + if !ok { + return nil, fmt.Errorf("localkey: malformed line in %s: %q", path, line) + } + version, err := strconv.Atoi(versionStr) + if err != nil { + return nil, fmt.Errorf("localkey: malformed version in %s: %q", path, line) + } + entries[version] = encoded + } + return entries, nil } -// All returns every version currently registered locally, keyed by version -// number. An empty, non-nil map means no `nebel init` has ever run on this -// clone (spec 06 AC-6.6's no-key passthrough state). -func All() (map[int][]byte, error) { - entries, err := gitutil.ConfigGetRegexp(`^filter\.nebel\.key[0-9]*$`) +// writeKeyFile serializes entries, one "version=base64key" line each in +// ascending version order (so the file diffs sensibly if ever inspected, +// though it is never committed), and writes them to the key file with +// 0o600 permissions — owner read/write only. +func writeKeyFile(entries map[int]string) error { + path, err := keyFilePath() + if err != nil { + return err + } + + versions := make([]int, 0, len(entries)) + for v := range entries { + versions = append(versions, v) + } + sort.Ints(versions) + + var b strings.Builder + for _, v := range versions { + fmt.Fprintf(&b, "%d=%s\n", v, entries[v]) + } + + if err := os.WriteFile(path, []byte(b.String()), 0o600); err != nil { + return fmt.Errorf("localkey: writing %s: %w", path, err) + } + return nil +} + +// readLegacyEntries reads every key still sitting in the pre-P06-fix +// storage location: local (not committed) git config, keyed by the same +// version numbers readKeyFile would use. A clone that registered a +// version before this file existed keeps it there — Set never migrates +// or deletes it — so Get and All both consult this as a fallback. +func readLegacyEntries() (map[int]string, error) { + raw, err := gitutil.ConfigGetRegexp(`^filter\.nebel\.key[0-9]*$`) if err != nil { return nil, fmt.Errorf("localkey: listing registered keys: %w", err) } - keyring := map[int][]byte{} - for name, encoded := range entries { + entries := map[int]string{} + for name, encoded := range raw { version := 1 if suffix, ok := strings.CutPrefix(name, configKeyPrefix); ok && suffix != "" { version, err = strconv.Atoi(suffix) @@ -83,9 +187,90 @@ func All() (map[int][]byte, error) { return nil, fmt.Errorf("localkey: unexpected local config key %q", name) } } - key, err := base64.StdEncoding.DecodeString(encoded) + entries[version] = encoded + } + return entries, nil +} + +func decodeKey(encoded string, version int) ([]byte, error) { + key, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("localkey: decoding stored key for version %d: %w", version, err) + } + return key, nil +} + +// Get returns the locally registered key for version. ok is false, with a +// nil error, when that version is not registered yet. +func Get(version int) (key []byte, ok bool, err error) { + entries, err := readKeyFile() + if err != nil { + return nil, false, err + } + if encoded, found := entries[version]; found { + key, err := decodeKey(encoded, version) + return key, true, err + } + + legacy, err := readLegacyEntries() + if err != nil { + return nil, false, err + } + if encoded, found := legacy[version]; found { + key, err := decodeKey(encoded, version) + return key, true, err + } + return nil, false, nil +} + +// Set registers key locally as version's key, writing it to the key file +// (see keyFileName's doc comment for why, in place of git config). Existing +// versions are left untouched (spec 11 AC-11.5): a machine that rotates, +// or fetches an older version via `nebel init --version N`, keeps reading +// everything it could read before — including anything still sitting in +// the legacy git-config location (see readLegacyEntries), which this +// never migrates or removes. +func Set(version int, key []byte) error { + entries, err := readKeyFile() + if err != nil { + return err + } + entries[version] = base64.StdEncoding.EncodeToString(key) + return writeKeyFile(entries) +} + +// All returns every version currently registered locally, keyed by version +// number — merging the key file with anything still left in the legacy +// git-config location (a version present in both, which Set can no longer +// produce going forward, resolves to the key file's copy). An empty, +// non-nil map means no `nebel init` has ever run on this clone (spec 06 +// AC-6.6's no-key passthrough state). +func All() (map[int][]byte, error) { + keyring := map[int][]byte{} + + fileEntries, err := readKeyFile() + if err != nil { + return nil, err + } + for version, encoded := range fileEntries { + key, err := decodeKey(encoded, version) + if err != nil { + return nil, err + } + keyring[version] = key + } + + legacy, err := readLegacyEntries() + if err != nil { + return nil, err + } + for version, encoded := range legacy { + if _, already := keyring[version]; already { + continue + } + key, err := decodeKey(encoded, version) if err != nil { - return nil, fmt.Errorf("localkey: decoding stored key for version %d: %w", version, err) + return nil, err } keyring[version] = key } diff --git a/internal/localkey/localkey_test.go b/internal/localkey/localkey_test.go index c47b221..38c0941 100644 --- a/internal/localkey/localkey_test.go +++ b/internal/localkey/localkey_test.go @@ -5,8 +5,11 @@ package localkey import ( "bytes" + "encoding/base64" "os" "os/exec" + "path/filepath" + "strings" "testing" ) @@ -65,11 +68,21 @@ func TestSetGetRoundTrip(t *testing.T) { } } -// version 1 is stored under the unsuffixed "filter.nebel.key" name — the -// same one every clone created before key rotation existed already reads -// and writes — so an upgraded binary keeps recognizing a key an older -// binary registered. -func TestVersion1UsesLegacyConfigName(t *testing.T) { +// Regression test for the 2026-09-29 audit's P06: Set must never put the +// key anywhere `git config` can reach, since writing it there is exactly +// what required exposing it as a subprocess argument in the first place. +// This directly proves the exposure path is closed: the base64 key must +// not appear anywhere in .git/config's raw file content after Set. +// +// The audit's actual concern — the key briefly appearing in a process's +// argument list, visible to another local user via /proc//cmdline — +// isn't itself observable from a Go test, since the git subprocess that +// used to receive it exits before a test could inspect its argv. Proving +// the key no longer ends up as a plain git-config value at all is the +// most direct available proxy: getting it there is exactly what forced +// passing it as a `git config` argument to begin with, so its absence +// here means that argument was never made. +func TestSetNeverWritesToGitConfig(t *testing.T) { dir := newTempRepo(t) chdir(t, dir) @@ -78,14 +91,69 @@ func TestVersion1UsesLegacyConfigName(t *testing.T) { t.Fatalf("Set: %v", err) } - cmd := exec.Command("git", "config", "--local", "--get", "filter.nebel.key") + configPath := filepath.Join(dir, ".git", "config") + raw, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("reading .git/config: %v", err) + } + encoded := base64.StdEncoding.EncodeToString(key) + if strings.Contains(string(raw), encoded) { + t.Errorf(".git/config contains the key after Set:\n%s", raw) + } + if strings.Contains(string(raw), "filter.nebel.key") { + t.Errorf(".git/config contains a filter.nebel.key entry after Set:\n%s", raw) + } +} + +// The key file Set writes to must be readable only by its owner — +// tighter than .git/config's ambient permissions, and a direct answer to +// a related "harden local key storage" suggestion from the same audit. +func TestKeyFileHasOwnerOnlyPermissions(t *testing.T) { + dir := newTempRepo(t) + chdir(t, dir) + + if err := Set(1, bytes.Repeat([]byte{0x11}, 64)); err != nil { + t.Fatalf("Set: %v", err) + } + + info, err := os.Stat(filepath.Join(dir, ".git", keyFileName)) + if err != nil { + t.Fatalf("stat key file: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("key file permissions = %o, want 0600", perm) + } +} + +// Backward compatibility: a version registered the old way, directly in +// local git config (as every clone did before this fix), must still be +// found by Get and All — an existing clone must keep working without +// needing to re-run `nebel init` just because this shipped. +func TestGetAndAllFallBackToLegacyGitConfig(t *testing.T) { + dir := newTempRepo(t) + chdir(t, dir) + + key := bytes.Repeat([]byte{0x22}, 64) + cmd := exec.Command("git", "config", "--local", "filter.nebel.key", base64.StdEncoding.EncodeToString(key)) cmd.Dir = dir - out, err := cmd.Output() + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git config: %v\n%s", err, out) + } + + got, ok, err := Get(1) + if err != nil || !ok { + t.Fatalf("Get(1) for a legacy-registered key: ok=%v err=%v", ok, err) + } + if !bytes.Equal(got, key) { + t.Errorf("Get(1) = %x, want %x", got, key) + } + + all, err := All() if err != nil { - t.Fatalf("git config --get filter.nebel.key: %v", err) + t.Fatalf("All: %v", err) } - if len(out) == 0 { - t.Error("version 1's key was not stored under filter.nebel.key") + if !bytes.Equal(all[1], key) { + t.Errorf("All()[1] = %x, want %x", all[1], key) } } diff --git a/internal/siv/siv.go b/internal/siv/siv.go index 78f9a95..408ca3f 100644 --- a/internal/siv/siv.go +++ b/internal/siv/siv.go @@ -42,17 +42,25 @@ var ( ErrAuth = errors.New("siv: authentication failed") ) -// AAD builds the associated data binding a ciphertext to one location. +// AAD builds the associated data binding a ciphertext to one location, plus +// any extra components a caller passes (spec 03 AC-3.10 binds a value's +// native type this way, so tampering with a tag's separately-stored type +// field — not itself ciphertext — fails authentication instead of silently +// changing how the decrypted value is rendered). // // Without this binding, two fields holding the same secret would encrypt to // identical ciphertext, and a ciphertext could be copy-pasted between fields // and still decrypt. Components are length-prefixed, so no two distinct -// (mode, filePath, fieldPath) triples can produce the same bytes. +// (mode, filePath, fieldPath, extra...) tuples can produce the same bytes — +// including between calls with a different number of extra components, +// since each one is prefixed by its own length rather than only separated +// from its neighbors. // // filePath is repo-relative. fieldPath is empty for ModeFile. -func AAD(mode Mode, filePath, fieldPath string) []byte { +func AAD(mode Mode, filePath, fieldPath string, extra ...string) []byte { var aad []byte - for _, part := range []string{string(mode), filePath, fieldPath} { + parts := append([]string{string(mode), filePath, fieldPath}, extra...) + for _, part := range parts { aad = binary.AppendUvarint(aad, uint64(len(part))) aad = append(aad, part...) } diff --git a/internal/tag/tag.go b/internal/tag/tag.go index b273fa2..42ec595 100644 --- a/internal/tag/tag.go +++ b/internal/tag/tag.go @@ -4,10 +4,11 @@ // Package tag encodes and decodes the inline ENC[...] marker used to store // an encrypted value in place. // -// Two forms share one parser: +// Three forms share one parser: // -// ENC[AES256_SIV,key:,data:] whole file (spec 06) -// ENC[AES256_SIV,key:,data:,type:str] one value (spec 03) +// ENC[AES256_SIV,key:,data:] whole file (spec 06) +// ENC[AES256_SIV,key:,data:,type:str] one value, legacy (spec 03) +// ENC[AES256_SIV_TB,key:,data:,type:str] one value, type-bound (spec 03 AC-3.10) // // key records which project key version (spec 04, spec 11) produced the // ciphertext, so smudge can select the matching key from the local keyring @@ -18,6 +19,41 @@ // since that is the form already committed in every repository created // before per-value mode existed. // +// The two value forms differ in whether the type field is itself +// authenticated. AES256_SIV_TB folds it into the ciphertext's AAD (see +// internal/siv.AAD and internal/filterop's use of it), so changing +// type:str to type:bool on an unmodified ciphertext — which needs no key, +// since the type field sits next to the ciphertext as plaintext metadata, +// not inside it — fails authentication instead of silently changing how +// the decrypted value renders (audit 2026-09-29, P05). AES256_SIV plays +// that same role for a value tag only because it predates this fix: every +// tag EncodeValue produces now uses AES256_SIV_TB, but a legacy +// AES256_SIV value tag already committed before this change remains +// parseable and must keep authenticating exactly as it always did, so +// Parse still accepts it — TypeBound distinguishes the two for a caller +// deciding which AAD to reconstruct. There is no separate migration step +// beyond that: a value converges to the new form the next time anything +// re-encrypts it (an ordinary edit, or the eager re-encryption `nebel +// rotate` and `git add --renormalize` already perform for spec 11), the +// same way a value converges onto a new key version. +// +// That convergence closes P05 only for the current, committed tag — +// not for the field's history. A legacy ciphertext, once committed, +// stays a valid AES256_SIV tag forever: nothing about upgrading the +// current commit changes it, or the key version it names (spec 11 +// AC-11.5 keeps every version's key registered indefinitely). An +// attacker with write access can resurrect that old blob from git +// history — or a stale branch, or another clone's reflog — with its +// type field re-flipped, and it authenticates exactly as it did the +// day it was committed, since Smudge picks the (unbound) legacy AAD +// for anything naming AES256_SIV. Reaching that ciphertext at all +// already requires the write access P05 assumes; validTypeLiteral +// (internal/filterop) additionally limits what a successful flip can +// render to a genuine literal of the new type, narrowing rather than +// closing the gap. Fully closing it would mean actively revoking the +// old key version — not merely superseding it — which spec 11's +// rotation does not do and is out of scope here. +// // key is mandatory (AC-3.6): this is a breaking format change from the // pre-rotation tag (no key field at all), landing alongside spec 11. // There is no in-tool migration: the config's own canary is checked @@ -38,9 +74,17 @@ import ( "strings" ) -// AlgoAES256SIV is the only algorithm identifier this build understands. +// AlgoAES256SIV is the algorithm identifier for a whole-file tag, and for +// a legacy value tag produced before AlgoAES256SIVTypeBound existed (see +// Tag.TypeBound). const AlgoAES256SIV = "AES256_SIV" +// AlgoAES256SIVTypeBound is the algorithm identifier EncodeValue now +// always produces: a value tag whose type field is bound into the +// ciphertext's AAD, so tampering with it fails authentication (spec 03 +// AC-3.10, audit 2026-09-29 P05). See Tag.TypeBound. +const AlgoAES256SIVTypeBound = "AES256_SIV_TB" + // Type is the native type of an encrypted scalar, recorded so decryption // can restore it. TypeNone marks a whole-file tag, which has none. type Type string @@ -73,6 +117,18 @@ type Tag struct { // Type is the original scalar type, or TypeNone for a whole-file tag. Type Type + + // TypeBound reports whether Type is itself part of what this tag's + // ciphertext authenticates (AlgoAES256SIVTypeBound), as opposed to + // separate, unauthenticated plaintext metadata sitting next to the + // ciphertext (the legacy AlgoAES256SIV value form). Always false for + // a whole-file tag, which carries no type field to bind in the first + // place. A caller reconstructing the AAD a value's ciphertext was + // encrypted under must include Type in it when this is true, and + // must not when it's false — using the wrong one either way fails + // authentication, since AAD mismatches are indistinguishable from a + // wrong key or tampered ciphertext. + TypeBound bool } const ( @@ -117,8 +173,14 @@ func Encode(ciphertext []byte, version int) string { // EncodeValue wraps ciphertext for a single scalar, recording the key // version that produced it (spec 11) and the type the plaintext had so // Decrypt can restore it (spec 03 AC-3.1, AC-3.2). +// +// Always uses AlgoAES256SIVTypeBound (spec 03 AC-3.10): every value tag +// this build produces binds its type field into the ciphertext's AAD. +// The caller must construct that AAD to match — see internal/siv.AAD and +// Tag.TypeBound's doc comment — or the ciphertext it just produced won't +// authenticate against its own tag. func EncodeValue(ciphertext []byte, version int, t Type) string { - return fmt.Sprintf("%s%s,key:%d,data:%s,type:%s%s", prefix, AlgoAES256SIV, version, base64.StdEncoding.EncodeToString(ciphertext), t, suffix) + return fmt.Sprintf("%s%s,key:%d,data:%s,type:%s%s", prefix, AlgoAES256SIVTypeBound, version, base64.StdEncoding.EncodeToString(ciphertext), t, suffix) } // Decode parses a tag and returns just the ciphertext, ignoring its @@ -154,7 +216,13 @@ func Parse(raw string) (Tag, error) { return Tag{}, fmt.Errorf("%w: expected \"ALGO,key:,data:\"", ErrMalformed) } algo := fields[0] - if algo != AlgoAES256SIV { + var typeBound bool + switch algo { + case AlgoAES256SIV: + typeBound = false + case AlgoAES256SIVTypeBound: + typeBound = true + default: return Tag{}, fmt.Errorf("%w: %q", ErrUnsupportedAlgo, algo) } rest := fields[1:] @@ -194,7 +262,14 @@ func Parse(raw string) (Tag, error) { if parsed.Type = Type(value); !parsed.Type.Valid() { return Tag{}, fmt.Errorf("%w: %q", ErrUnsupportedType, value) } + parsed.TypeBound = typeBound rest = rest2 + } else if typeBound { + // AlgoAES256SIVTypeBound exists specifically to bind a type + // field; a whole-file-shaped tag naming it anyway has nothing to + // bind and is malformed, not something to quietly accept as if + // it were the plain AlgoAES256SIV whole-file form. + return Tag{}, fmt.Errorf("%w: %q requires a \"type:\" field", ErrMalformed, AlgoAES256SIVTypeBound) } if len(rest) > 0 { return Tag{}, fmt.Errorf("%w: unexpected trailing field %q", ErrMalformed, rest[0]) diff --git a/internal/tag/tag_test.go b/internal/tag/tag_test.go index fd3fdb8..155efce 100644 --- a/internal/tag/tag_test.go +++ b/internal/tag/tag_test.go @@ -7,6 +7,7 @@ import ( "bytes" "encoding/base64" "errors" + "strings" "testing" ) @@ -171,6 +172,56 @@ func TestParseWholeFileTagHasNoType(t *testing.T) { if parsed.Type != TypeNone { t.Errorf("Type = %q, want TypeNone", parsed.Type) } + if parsed.TypeBound { + t.Error("TypeBound = true for a whole-file tag, want false") + } +} + +// Regression test for the 2026-09-29 audit's P05: EncodeValue must always +// produce a type-bound tag (AlgoAES256SIVTypeBound), so a caller +// reconstructing the AAD for it knows to include the type — otherwise +// Type sits next to the ciphertext as unauthenticated plaintext metadata, +// changeable without the key. +func TestEncodeValueUsesTypeBoundAlgo(t *testing.T) { + encoded := EncodeValue([]byte("ciphertext"), 1, TypeStr) + if !strings.HasPrefix(encoded, "ENC["+AlgoAES256SIVTypeBound+",") { + t.Errorf("EncodeValue() = %q, want it to start with \"ENC[%s,\"", encoded, AlgoAES256SIVTypeBound) + } + parsed, err := Parse(encoded) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if !parsed.TypeBound { + t.Error("TypeBound = false, want true") + } +} + +// A value tag from before this fix — AlgoAES256SIV, not +// AlgoAES256SIVTypeBound — must still parse: there is no in-tool +// migration, so every already-committed tag of this form must keep +// working (see the package doc comment). TypeBound must come back false +// for it, so a caller reconstructs the AAD it was actually encrypted +// under (without the type), not the new formula. +func TestParseLegacyValueTagIsNotTypeBound(t *testing.T) { + parsed, err := Parse("ENC[AES256_SIV,key:1,data:AAAA,type:str]") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if parsed.Type != TypeStr { + t.Errorf("Type = %q, want %q", parsed.Type, TypeStr) + } + if parsed.TypeBound { + t.Error("TypeBound = true for a legacy AES256_SIV value tag, want false") + } +} + +// AlgoAES256SIVTypeBound exists to bind a type field; naming it on a +// whole-file-shaped tag (no type field) has nothing to bind and is +// malformed, not silently equivalent to plain AlgoAES256SIV. +func TestParseRejectsTypeBoundWithoutType(t *testing.T) { + if _, err := Parse("ENC[AES256_SIV_TB,key:1,data:AAAA]"); !errors.Is(err, ErrMalformed) { + t.Errorf("Parse() error = %v, want %v", err, ErrMalformed) + } } // AC-3.8, extended to the type field: a tag naming a type this build can't diff --git a/specs/03-value-encoding.md b/specs/03-value-encoding.md index a62ab14..897324b 100644 --- a/specs/03-value-encoding.md +++ b/specs/03-value-encoding.md @@ -6,6 +6,15 @@ project key version (spec 04, spec 11) produced this ciphertext — see REQUIREMENTS.md § Key rotation for why one project can have values under several versions at once. +`ALGO` is `AES256_SIV_TB` for every value tag this build produces +(`AES256_SIV` remains the whole-file form, spec 06) — `type`'s value is +bound into the ciphertext's AAD (spec 02), so changing it needs the key, +not just write access to the file it's stored in (AC-3.10). A value tag +using plain `AES256_SIV` is the pre-AC-3.10 form: still accepted, since +there is no in-tool migration (see spec 11 and this repo's +`internal/tag` package doc comment), but its `type` field is not +authenticated the same way. + ## Acceptance criteria - **AC-3.1 Encode string**: encoding a string value under key version `V` @@ -37,3 +46,23 @@ several versions at once. needs to know which version is "current" — the tag names its own version explicitly, and the caller (spec 06) is responsible for finding the matching key in the local keyring. +- **AC-3.10 Type is authenticated**: a value tag's `type` field is bound + into its ciphertext's AAD (`ALGO` = `AES256_SIV_TB`), so changing + `type:str` to `type:bool` (or any other type) on an otherwise-untouched + tag fails authentication instead of silently changing how the decrypted + value renders — closing the gap where `type` sat next to the + ciphertext as unauthenticated plaintext metadata, changeable by anyone + with write access to the file, without the key (2026-09-29 audit, P05). + A value tag from before this fix (`ALGO` = `AES256_SIV`) has no `type` + binding and keeps decrypting exactly as it always did — there is no + in-tool migration for already-committed tags; a value converges onto + the bound form the next time anything re-encrypts it. That + convergence only protects the current, committed tag: an attacker + with write access can still resurrect an old `AES256_SIV` blob from + git history (or a stale branch or clone) with its `type` re-flipped, + and it authenticates exactly as it did when first committed, since + its key version stays registered indefinitely (spec 11 AC-11.5). + Closing that fully would mean actively revoking the old key version, + which rotation does not do. `internal/filterop`'s `validTypeLiteral` + narrows what a successful flip against a legacy tag can render to a + genuine literal of the new type, but does not authenticate it.