diff --git a/USER_INTERACTIONS.md b/USER_INTERACTIONS.md index 3c8bedf..63f0a2a 100644 --- a/USER_INTERACTIONS.md +++ b/USER_INTERACTIONS.md @@ -101,6 +101,22 @@ $ nebel add file "secrets/*.pem" Added rule: secrets/*.pem (mode: file) ``` +If `secrets/*.pem` already matches files that were tracked before this +rule existed, they're encrypted right away, not left for the next edit to +pick up: + +``` +$ nebel add file "secrets/*.pem" +Added rule: secrets/*.pem (mode: file) +Encrypted and staged 2 files already tracked and matching this rule: + secrets/prod.pem + secrets/staging.pem + +⚠ This only protects future commits — the old plaintext is still + recoverable from git history. If this repo has been pushed anywhere, + change the underlying secret(s), not just the file. +``` + ### `add field [path...]` — per-value mode For config you still want to read and diff in git. Paths are dot notation @@ -138,8 +154,16 @@ Added to rule config/staging.yaml (mode: value): - Re-running with the same paths is a no-op. - Supported formats: YAML and JSON. `.env` and `.properties` are not implemented yet. -- Both subcommands also add the pattern to `.gitattributes`. After that the - normal `git add`/`git commit` flow picks the rule up — no further step. +- Both subcommands also add the pattern to `.gitattributes`, then + immediately re-encrypt any file the new rule already matches among + files git currently tracks — no further step, even for a file that + predates the rule and hasn't been touched since. (A brand new rule + commonly matches nothing yet — that's not an error, just nothing to + report.) Anything it re-encrypts was, by definition, still readable in + plaintext form in git history before this command ran; if the repo has + been pushed anywhere, treat the underlying secret as exposed and change + it (not `nebel rotate`, which only changes the shared password — see § + 4), the same as after any other accidental plaintext commit. ## 3. `nebel status` diff --git a/cmd/nebel/add.go b/cmd/nebel/add.go index c4289c3..90e5591 100644 --- a/cmd/nebel/add.go +++ b/cmd/nebel/add.go @@ -76,7 +76,10 @@ func runAddFile(args []string) error { return fmt.Errorf("%q is already registered with mode: %s — mode conflict", glob, r.Mode) } fmt.Printf("Rule already exists: %s (mode: file)\n", glob) - return addGitattributesPattern(root, glob) // idempotent; covers a hand-edited config missing the .gitattributes line + if err := addGitattributesPattern(root, glob); err != nil { // idempotent; covers a hand-edited config missing the .gitattributes line + return err + } + return renormalizeAndReport(root, cfg, glob) } cfg.Rules = append(cfg.Rules, config.Rule{Files: glob, Mode: config.ModeFile}) @@ -88,7 +91,7 @@ func runAddFile(args []string) error { } fmt.Printf("Added rule: %s (mode: file)\n", glob) - return nil + return renormalizeAndReport(root, cfg, glob) } // runAddField implements `nebel add field [path...]` (AC-8.2). @@ -134,7 +137,10 @@ func runAddField(args []string) error { } if len(added) == 0 { fmt.Printf("No new fields to add for %s.\n", target) - return addGitattributesPattern(root, target) + if err := addGitattributesPattern(root, target); err != nil { + return err + } + return renormalizeAndReport(root, cfg, target) } if err := applyFieldRule(cfg, target, added); err != nil { @@ -151,7 +157,7 @@ func runAddField(args []string) error { for _, path := range added { fmt.Printf(" %s\n", path) } - return nil + return renormalizeAndReport(root, cfg, target) } // existingFields returns the paths already configured for pattern. @@ -203,6 +209,68 @@ func applyFieldRule(cfg *config.Config, pattern string, added []string) error { return nil } +// filesMatchingNewRule returns every currently tracked file that +// cfg.MatchRule resolves to the rule named pattern — the one this `add` +// call just wrote or reconfirmed — rather than whatever a raw pathspec +// match on pattern would return. That distinction matters twice over: +// spec 08's glob and .gitattributes' pattern can disagree on what they +// match (audit finding, cross-directory `*.env` semantics), and `git add +// --renormalize`'s own pathspec form errors outright when pattern +// matches no tracked file yet — which a brand-new glob commonly does, +// before anything matching it has ever been committed. +func filesMatchingNewRule(root string, cfg *config.Config, pattern string) ([]string, error) { + tracked, err := gitutil.TrackedFiles(root) + if err != nil { + return nil, err + } + var matched []string + for _, f := range tracked { + if r, ok := cfg.MatchRule(f); ok && r.Files == pattern { + matched = append(matched, f) + } + } + return matched, nil +} + +// renormalizeAndReport brings every already-tracked file matching +// pattern's rule under that rule immediately, instead of leaving it +// exactly as it was until something unrelated later edits it. +// +// Without this, a file that was already tracked and unchanged before +// `add` registered a rule for it can sit unencrypted indefinitely +// afterward: git only re-invokes the clean filter for a path when it +// can't trust its own cached stat info for that path, and a settled, +// already-tracked file commonly gives it no reason not to — a new +// `.gitattributes` line routing the path through nebel notwithstanding. +// `add`'s own documented "no further step" promise depends on this. +func renormalizeAndReport(root string, cfg *config.Config, pattern string) error { + files, err := filesMatchingNewRule(root, cfg, pattern) + if err != nil { + return err + } + if len(files) == 0 { + return nil + } + + migrated, err := gitutil.Renormalize(root, files) + if err != nil { + return fmt.Errorf("encrypting already-tracked files matching %s (fix by hand with `git add --renormalize -- %s`): %w", pattern, strings.Join(files, " "), err) + } + if len(migrated) == 0 { + return nil + } + + fmt.Printf("Encrypted and staged %s already tracked and matching this rule:\n", plural(len(migrated), "file")) + for _, path := range migrated { + fmt.Printf(" %s\n", path) + } + fmt.Println() + fmt.Println("⚠ This only protects future commits — the old plaintext is still") + fmt.Println(" recoverable from git history. If this repo has been pushed anywhere,") + fmt.Println(" change the underlying secret(s), not just the file.") + return nil +} + // ensureGitattributes creates an empty .gitattributes file at bootstrap if // one doesn't exist yet, so it's ready to commit even before any rule is // added. diff --git a/cmd/nebel/add_existing_files_test.go b/cmd/nebel/add_existing_files_test.go new file mode 100644 index 0000000..acd59ec --- /dev/null +++ b/cmd/nebel/add_existing_files_test.go @@ -0,0 +1,175 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// commitTrackedAndSettle writes content at repo-relative relPath, backdates +// its mtime well into the past, and commits it — reproducing a file that +// was already tracked and settled in git's index long before any nebel +// rule ever mentioned it. +// +// The backdating matters: without it, the file's mtime sits close enough +// to "now" that git's own protection against racy stat comparisons (a +// path whose mtime isn't safely older than the index's own mtime can't be +// trusted from cached stat info alone) forces a full content re-check on +// every subsequent git operation anyway — which would mask exactly the +// bug these tests exist to catch. Once a path's stat info is unambiguously +// old, git trusts it blindly and skips re-invoking any filter for that +// path — a new .gitattributes line notwithstanding — until something +// actually touches the file again. +func commitTrackedAndSettle(t *testing.T, repo, pathEnv, relPath string, content []byte) { + t.Helper() + full := filepath.Join(repo, relPath) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, content, 0o644); err != nil { + t.Fatal(err) + } + old := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) + if err := os.Chtimes(full, old, old); err != nil { + t.Fatal(err) + } + runIn(t, repo, pathEnv, "git", "add", relPath) + runIn(t, repo, pathEnv, "git", "commit", "-q", "-m", "commit "+relPath+" before any nebel rule exists") +} + +// `add file` used to only register a rule: git's own index would keep +// treating an already-tracked, already-settled file as clean, and it +// would stay plaintext in the repository until something unrelated later +// edited it — contradicting the documented "no further step" promise. +func TestAddFileEncryptsAlreadyTrackedUnchangedFile(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + initWithPassword(t, repo, pathEnv, "test-password-123") + + plaintext := []byte("-----BEGIN KEY-----\ntotally-secret\n-----END KEY-----\n") + commitTrackedAndSettle(t, repo, pathEnv, "secrets/prod.pem", plaintext) + + out := runIn(t, repo, pathEnv, "nebel", "add", "file", "secrets/*.pem") + if !strings.Contains(out, "Encrypted and staged 1 file") { + t.Errorf("add file did not report re-encrypting the already-tracked file:\n%s", out) + } + + staged := runIn(t, repo, pathEnv, "git", "show", ":secrets/prod.pem") + if !strings.HasPrefix(staged, "ENC[") { + t.Fatalf("already-tracked file was not encrypted by `add file` alone (no manual git add/renormalize ran): got %q", staged) + } + + // Re-running is a no-op: nothing left to re-encrypt. + out = runIn(t, repo, pathEnv, "nebel", "add", "file", "secrets/*.pem") + if strings.Contains(out, "Encrypted") { + t.Errorf("re-running add file re-encrypted an already-converged file:\n%s", out) + } + + runIn(t, repo, pathEnv, "git", "add", ".") + runIn(t, repo, pathEnv, "git", "commit", "-q", "-m", "commit the rule and its ciphertext") + onDisk, err := os.ReadFile(filepath.Join(repo, "secrets", "prod.pem")) + if err != nil { + t.Fatal(err) + } + if string(onDisk) != string(plaintext) { + t.Errorf("working tree changed unexpectedly: got %q, want %q", onDisk, plaintext) + } +} + +// A glob that doesn't yet match any tracked file is the common case for a +// brand new rule — `add file` must not error just because there's nothing +// to renormalize yet (a raw `git add --renormalize -- ` does error +// in this case). +func TestAddFileWithNoExistingMatchesSucceeds(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + initWithPassword(t, repo, pathEnv, "test-password-123") + + out := runIn(t, repo, pathEnv, "nebel", "add", "file", "secrets/*.pem") + if !strings.Contains(out, "Added rule") { + t.Errorf("add file with no matching tracked files did not succeed:\n%s", out) + } +} + +// Same bug, mode: value: `add field` on a file that already exists, +// tracked and unchanged, must encrypt the named field immediately. +func TestAddFieldEncryptsAlreadyTrackedUnchangedFile(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + initWithPassword(t, repo, pathEnv, "test-password-123") + + commitTrackedAndSettle(t, repo, pathEnv, "config/staging.yaml", []byte(stagingYAML)) + + out := runIn(t, repo, pathEnv, "nebel", "add", "field", "config/staging.yaml", "database.password") + if !strings.Contains(out, "Encrypted and staged 1 file") { + t.Errorf("add field did not report re-encrypting the already-tracked file:\n%s", out) + } + + staged := runIn(t, repo, pathEnv, "git", "show", ":config/staging.yaml") + if strings.Contains(staged, "s3cr3t") { + t.Errorf("already-tracked file still has the plaintext secret staged:\n%s", staged) + } + if !strings.Contains(staged, "ENC[") { + t.Errorf("field was not encrypted:\n%s", staged) + } +} + +// Appending a second field to a rule that already applies to an +// already-tracked, already-converged file must encrypt the newly added +// field too, without disturbing the field the rule already covered. +func TestAddFieldAppendEncryptsNewFieldOnAlreadyTrackedFile(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + initWithPassword(t, repo, pathEnv, "test-password-123") + + commitTrackedAndSettle(t, repo, pathEnv, "config/staging.yaml", []byte(stagingYAML)) + 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 password field") + + out := runIn(t, repo, pathEnv, "nebel", "add", "field", "config/staging.yaml", "api.keys[0]") + if !strings.Contains(out, "Encrypted and staged 1 file") { + t.Errorf("appending a field to an existing rule did not re-encrypt the already-tracked file:\n%s", out) + } + + staged := runIn(t, repo, pathEnv, "git", "show", ":config/staging.yaml") + if strings.Contains(staged, "alpha") { + t.Errorf("newly added field is still plaintext:\n%s", staged) + } + if strings.Contains(staged, "s3cr3t") { + t.Errorf("previously encrypted field regressed to plaintext:\n%s", staged) + } + if strings.Count(staged, "ENC[") != 2 { + t.Errorf("want exactly 2 encrypted fields staged, got:\n%s", staged) + } +} + +// A non-ASCII filename must not be silently dropped from renormalize's +// scope: git's default output quotes it as a C-style escaped string +// ("secrets/zug\303\244nge.pem" for an actual "ü"), which would never +// match the same path returned by cfg.MatchRule if that quoting weren't +// undone (gitutil's ls-files/ls-files -s calls use -z for exactly this +// reason). +func TestAddFileEncryptsAlreadyTrackedFileWithNonASCIIName(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := newTestRepo(t, pathEnv) + initWithPassword(t, repo, pathEnv, "test-password-123") + + plaintext := []byte("-----BEGIN KEY-----\ntotally-secret\n-----END KEY-----\n") + commitTrackedAndSettle(t, repo, pathEnv, "secrets/zugänge.pem", plaintext) + + out := runIn(t, repo, pathEnv, "nebel", "add", "file", "secrets/*.pem") + if !strings.Contains(out, "Encrypted and staged 1 file") { + t.Errorf("add file did not report re-encrypting the already-tracked non-ASCII-named file:\n%s", out) + } + + staged := runIn(t, repo, pathEnv, "git", "show", ":secrets/zugänge.pem") + if !strings.HasPrefix(staged, "ENC[") { + t.Fatalf("already-tracked non-ASCII-named file was not encrypted by `add file`: got %q", staged) + } +} diff --git a/internal/gitutil/gitutil.go b/internal/gitutil/gitutil.go index 9752837..8ecb57e 100644 --- a/internal/gitutil/gitutil.go +++ b/internal/gitutil/gitutil.go @@ -87,19 +87,55 @@ func Add(repoRoot string, paths ...string) error { return nil } -// RenormalizeAll force-reencrypts every nebel-managed file/field under the -// config's *current* key version, regardless of what version each -// previously carried, and stages the result — the `git add --renormalize` -// escape hatch (spec 11 AC-11.10), run automatically by `nebel rotate` so -// existing content doesn't need a manual follow-up to actually move onto -// the version rotate just minted. +// indexBlobs maps each of paths to its current staged blob hash, via +// `git ls-files -s -z`. A path with nothing tracked at it is simply +// absent from the result rather than being an error. +// +// The -z form is not cosmetic: git's default output quotes any path +// containing a non-ASCII byte (core.quotePath) as a C-style escaped +// string — "secrets/zug\303\244nge.pem" for an actual "ü" — which would +// never match the same path as returned by cfg.MatchRule, silently +// dropping that file from renormalize's scope. +func indexBlobs(repoRoot string, paths []string) (map[string]string, error) { + blobs := map[string]string{} + if len(paths) == 0 { + return blobs, nil + } + args := append([]string{"ls-files", "-s", "-z", "--"}, paths...) + cmd := exec.Command("git", args...) + cmd.Dir = repoRoot + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("gitutil: git ls-files -s: %w", wrapExitErr(err)) + } + for _, record := range strings.Split(strings.TrimRight(string(out), "\x00"), "\x00") { + if record == "" { + continue + } + meta, path, ok := strings.Cut(record, "\t") + if !ok { + continue + } + fields := strings.Fields(meta) + if len(fields) < 2 { + continue + } + blobs[path] = fields[1] // " " + } + return blobs, nil +} + +// renormalize force-reencrypts files under the config's *current* key +// version, regardless of what version each previously carried, and +// stages the result — the `git add --renormalize` escape hatch (spec 11 +// AC-11.10). // // This only ever needs the *current* version's key: clean always encrypts -// under it regardless of a file's prior tag (spec 06 AC-6.12), and rotate -// has just derived and registered that key locally before calling this — -// so, unlike CheckoutAll's smudge (which needs whichever version's key a +// under it regardless of a file's prior tag (spec 06 AC-6.12) — so, +// unlike CheckoutAll's smudge (which needs whichever version's key a // value's own tag names), the `git add --renormalize` call itself is -// expected to always succeed. +// expected to always succeed as long as the caller already holds that +// key locally. // // It can still be a no-op for a given path even so: a mode: value field, // or a mode: file whole file, that's still sitting as ciphertext @@ -107,17 +143,22 @@ func Add(repoRoot string, paths ...string) error { // first place (e.g. it names a version this clone hasn't fetched via // `nebel init --version N`) looks the same to clean as any other // already-encrypted value (AC-6.4) and is left untouched. The returned -// slice reflects only the paths that actually ended up staged with new -// content, so a caller reporting "N re-encrypted" doesn't overcount those. -func RenormalizeAll(repoRoot string) ([]string, error) { - files, err := ManagedFiles(repoRoot) - if err != nil { - return nil, err - } +// slice reflects only the paths whose staged blob hash actually changed, +// compared before and after — not every path passed in, and not every +// path that already differed from HEAD for some unrelated reason (which +// diffing the result against HEAD, instead, would wrongly count as this +// call's doing) — so a caller reporting "N re-encrypted" doesn't +// overcount either way. +func renormalize(repoRoot string, files []string) ([]string, error) { if len(files) == 0 { return nil, nil } + before, err := indexBlobs(repoRoot, files) + if err != nil { + return nil, err + } + args := append([]string{"add", "--renormalize", "--"}, files...) cmd := exec.Command("git", args...) cmd.Dir = repoRoot @@ -125,18 +166,39 @@ func RenormalizeAll(repoRoot string) ([]string, error) { return nil, fmt.Errorf("gitutil: git add --renormalize: %w: %s", wrapExitErr(err), strings.TrimSpace(string(out))) } - diffArgs := append([]string{"diff", "--cached", "--name-only", "--"}, files...) - diffCmd := exec.Command("git", diffArgs...) - diffCmd.Dir = repoRoot - out, err := diffCmd.Output() + after, err := indexBlobs(repoRoot, files) if err != nil { - return nil, fmt.Errorf("gitutil: git diff --cached: %w", wrapExitErr(err)) + return nil, err } - trimmed := strings.TrimSpace(string(out)) - if trimmed == "" { - return nil, nil + + var changed []string + for _, f := range files { + if before[f] != after[f] { + changed = append(changed, f) + } } - return strings.Split(trimmed, "\n"), nil + return changed, nil +} + +// RenormalizeAll runs renormalize over every nebel-managed file/field in +// the repository — run automatically by `nebel rotate` so existing +// content doesn't need a manual follow-up to actually move onto the +// version rotate just minted. +func RenormalizeAll(repoRoot string) ([]string, error) { + files, err := ManagedFiles(repoRoot) + if err != nil { + return nil, err + } + return renormalize(repoRoot, files) +} + +// Renormalize runs renormalize over a caller-chosen subset of +// already-tracked, repo-relative paths — used by `nebel add` to bring +// files that already existed, unchanged, under a rule just registered +// for them, without touching every other nebel-managed file in the repo +// the way RenormalizeAll does. +func Renormalize(repoRoot string, files []string) ([]string, error) { + return renormalize(repoRoot, files) } // Show returns path's content as committed at commit (e.g. "HEAD" or a @@ -259,19 +321,39 @@ func CheckoutAll(repoRoot string) (decrypted int, failures []CheckoutFailure, er return decrypted, failures, nil } -// ManagedFiles returns the repo-relative paths of every tracked file -// whose "filter" gitattribute is "nebel". -func ManagedFiles(repoRoot string) ([]string, error) { - lsCmd := exec.Command("git", "ls-files") +// TrackedFiles returns the repo-relative paths of every file `git +// ls-files` reports for repoRoot — every file git currently tracks, +// regardless of any nebel rule. +// +// -z, not plain `ls-files`: git's default output quotes any path +// containing a non-ASCII byte (core.quotePath) as a C-style escaped +// string, which would never match the same path as returned by +// cfg.MatchRule — silently dropping that file from every caller that +// filters this list by rule, notably `nebel add`'s renormalize step. +func TrackedFiles(repoRoot string) ([]string, error) { + lsCmd := exec.Command("git", "ls-files", "-z") lsCmd.Dir = repoRoot lsOut, err := lsCmd.Output() if err != nil { return nil, fmt.Errorf("gitutil: git ls-files: %w", wrapExitErr(err)) } - tracked := strings.Split(strings.TrimSpace(string(lsOut)), "\n") + tracked := strings.Split(strings.TrimRight(string(lsOut), "\x00"), "\x00") if len(tracked) == 1 && tracked[0] == "" { return nil, nil } + return tracked, nil +} + +// ManagedFiles returns the repo-relative paths of every tracked file +// whose "filter" gitattribute is "nebel". +func ManagedFiles(repoRoot string) ([]string, error) { + tracked, err := TrackedFiles(repoRoot) + if err != nil { + return nil, err + } + if len(tracked) == 0 { + return nil, nil + } args := append([]string{"check-attr", "filter", "--"}, tracked...) attrCmd := exec.Command("git", args...) diff --git a/specs/08-cli-add.md b/specs/08-cli-add.md index c4f4abe..bd4ebd3 100644 --- a/specs/08-cli-add.md +++ b/specs/08-cli-add.md @@ -43,3 +43,17 @@ diff. A bare `add ` names neither and is refused with usage. - **AC-8.10**: after `add` succeeds, the new rule is immediately visible to `nebel status` (spec 09) and to the git filter driver (spec 06) — no separate reload step needed. +- **AC-8.11**: after writing the rule, `add` re-encrypts every file + already tracked and matching it — a `git add --renormalize` equivalent, + scoped to files the new rule (or an existing rule it just added a field + to) resolves to, per spec 04's `MatchRule` precedence rather than a raw + pathspec match on the glob itself. Without this, a file that was + already tracked and unchanged in the working tree stays plaintext + indefinitely: git only re-invokes the clean filter for a path when it + can't trust its own cached stat info for that path, and an + already-settled, already-tracked file commonly gives it no reason not + to, a new `.gitattributes` line notwithstanding. A pattern matching no + tracked file yet — the common case for a brand new rule — is not an + error. Files it does re-encrypt are reported, along with a reminder + that this only protects future commits: content already committed in + plaintext is still recoverable from history.