From 19ab9ca9792faded2f8bed046601fc8a6a68997d Mon Sep 17 00:00:00 2001 From: Marwin Moellers Date: Thu, 1 Oct 2026 11:44:11 +0200 Subject: [PATCH 1/5] Fix R05 follow-up: LooksEncrypted accepted malformed tags as encrypted The 2026-09-30 reanalysis (finding 4.2) found that LooksEncrypted validated each ENC[...] field independently instead of sharing Parse's grammar, so a plaintext value shaped like one of these was wrongly treated as already-encrypted and left unprotected by Clean: ENC[AES256_SIV,data:] ENC[AES256_SIV,data:AAAA,data:AAAA] ENC[AES256_SIV_TB,data:AAAA] LooksEncrypted now delegates to the same positional field-parsing core Parse uses (rejecting duplicate/reordered/trailing fields and an AES256_SIV_TB tag missing its mandatory key field automatically), with one documented, narrow exception: a keyless pre-rotation AES256_SIV tag is still recognized as genuine ciphertext rather than double-wrapped. It also now rejects a "data:" field that decodes to fewer bytes than real AES-256-SIV output can ever be (internal/siv.MinCiphertextLen). Several existing test fixtures used placeholder ciphertext shorter than that minimum; bumped them to realistic lengths rather than loosening the new check to keep them passing. Co-Authored-By: Claude Sonnet 5 --- internal/filterop/filterop_test.go | 6 +- internal/siv/siv.go | 9 +++ internal/tag/tag.go | 115 ++++++++++++++--------------- internal/tag/tag_test.go | 22 +++++- 4 files changed, 85 insertions(+), 67 deletions(-) diff --git a/internal/filterop/filterop_test.go b/internal/filterop/filterop_test.go index c479545..2ff3e7e 100644 --- a/internal/filterop/filterop_test.go +++ b/internal/filterop/filterop_test.go @@ -52,7 +52,7 @@ func TestCleanIsIdempotent(t *testing.T) { // AC-6.4: clean run on input that is already a well-formed ENC[...] tag // leaves it unchanged rather than double-encrypting. func TestCleanSkipsAlreadyEncrypted(t *testing.T) { - already := tag.Encode([]byte("some-ciphertext"), 1) + already := tag.Encode([]byte("some-ciphertext-16-bytes-plus"), 1) got, err := Clean(testConfig(), testKeyring, "secrets/prod.pem", []byte(already)) if err != nil { t.Fatalf("Clean: %v", err) @@ -85,7 +85,7 @@ func TestCleanEncryptsPlaintextStartingWithEncPrefix(t *testing.T) { // double-wrapped: LooksEncrypted is deliberately more lenient than a bare // prefix check specifically so this legacy shape still counts as "ours". func TestCleanLeavesLegacyWholeFileTagAlone(t *testing.T) { - legacy := "ENC[AES256_SIV,data:c29tZXRoaW5n]" + legacy := "ENC[AES256_SIV,data:c2l4dGVlbi1ieXRlLWNpcGhlcnRleHQtcGxhY2Vob2xkZXI=]" got, err := Clean(testConfig(), testKeyring, "secrets/prod.pem", []byte(legacy)) if err != nil { t.Fatalf("Clean: %v", err) @@ -103,7 +103,7 @@ func TestCleanLeavesLegacyWholeFileTagAlone(t *testing.T) { // from hard-failing on a clone that's fallen behind a rotation it hasn't // rejoined yet. func TestCleanWholeFileNoKeyNeededWhenAlreadyEncrypted(t *testing.T) { - already := tag.Encode([]byte("some-ciphertext"), 1) + already := tag.Encode([]byte("some-ciphertext-16-bytes-plus"), 1) got, err := Clean(configAtVersion(2), Keyring{}, "secrets/prod.pem", []byte(already)) if err != nil { t.Fatalf("Clean() with no keys registered: want success (nothing to encrypt), got %v", err) diff --git a/internal/siv/siv.go b/internal/siv/siv.go index 408ca3f..0f3e39a 100644 --- a/internal/siv/siv.go +++ b/internal/siv/siv.go @@ -21,6 +21,15 @@ import ( // 256-bit subkeys, derived from this single 512-bit key. const KeySize = 64 +// MinCiphertextLen is the smallest possible output of Encrypt: RFC 5297's +// synthetic IV is itself 16 bytes, prepended to the (possibly zero-length, +// for an empty plaintext) encrypted data. No genuine ciphertext this +// package ever produces can be shorter — a decoded "data:" field below +// this length in an ENC[...] tag cannot be real SIV output (internal/tag's +// LooksEncrypted relies on this to reject tags shaped like an +// already-encrypted value around implausibly short or empty payloads). +const MinCiphertextLen = 16 + // Mode is the encryption granularity a rule applies to a file. type Mode string diff --git a/internal/tag/tag.go b/internal/tag/tag.go index b953e4d..0bffde7 100644 --- a/internal/tag/tag.go +++ b/internal/tag/tag.go @@ -72,6 +72,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/MuffinUser/nebel/internal/siv" ) // AlgoAES256SIV is the algorithm identifier for a whole-file tag, and for @@ -170,25 +172,29 @@ func IsEncrypted(raw []byte) bool { return strings.HasPrefix(string(raw), prefix) } -// LooksEncrypted reports whether raw is shaped like one of this package's -// ENC[...] tags: delimiters present, the first comma-separated field -// naming a recognized algorithm, a genuine "data:" field whose payload -// actually decodes as base64, and — for any other field present — a -// recognized field name with a syntactically valid value. Unlike Parse, -// the "key:" field is not required (see below), but every field that IS -// present must be well-formed; an unrecognized or malformed field, or the -// literal absence of any "data:" field at all, means raw is not a tag. -// -// This intentionally stops short of a full Parse in exactly one respect: -// a pre-rotation tag (this package's doc comment) has no "key:" field at -// all and fails Parse for that reason alone, yet is genuine, -// already-encrypted content with its own documented migration path -// (README's "Upgrading from a pre-rotation repository"), not plaintext to -// encrypt over. LooksEncrypted still recognizes it as "ours" so -// Clean/cleanValues leave it alone (AC-6.4) instead of double-wrapping it. +// LooksEncrypted reports whether raw is a tag this package would itself +// produce or still accept: it shares Parse's exact grammar — algorithm, +// then an ordered key/data/type field sequence with no duplicates, no +// trailing garbage, and a "data:" payload that both decodes as base64 and +// is long enough to be genuine SIV output (internal/siv.MinCiphertextLen) +// — relaxed in exactly one respect: a pre-rotation tag (this package's doc +// comment) has no "key:" field at all, fails Parse for that reason alone, +// and is still genuine, already-encrypted content with its own documented +// migration path (README's "Upgrading from a pre-rotation repository"), +// not plaintext to encrypt over. LooksEncrypted still recognizes it as +// "ours" so Clean/cleanValues leave it alone (AC-6.4) instead of +// double-wrapping it. AlgoAES256SIVTypeBound postdates key rotation (it +// didn't exist before rotation introduced the "key:" field), so that +// relaxation applies only to AlgoAES256SIV — a type-bound tag always needs +// one. // -// Beyond that one deliberate exception, LooksEncrypted requires the same -// field shapes Parse does: a value merely starting with "ENC[" plus a +// Sharing Parse's grammar (rather than re-validating each field +// independently) is what rejects a duplicated or reordered field, a +// "data:" field decoding to too little ciphertext to be real, or a +// type-bound tag with no type field — forms the previous, per-field +// validation accepted as already-encrypted even though they were never a +// tag this package could have produced or could ever decrypt (audit +// 2026-09-30, reanalysis 4.2). A value merely starting with "ENC[" plus a // recognized algorithm name, but otherwise malformed — e.g. // "ENC[AES256_SIV,not-a-real-tag]", which has no "data:" field at all — is // not treated as already-encrypted (audit 2026-09-30, R05). This is @@ -196,43 +202,11 @@ func IsEncrypted(raw []byte) bool { // also (wrongly) call a plaintext value that merely starts with "ENC[" // already-encrypted (audit 2026-09-29, P08). func LooksEncrypted(raw []byte) bool { - s := string(raw) - if !strings.HasPrefix(s, prefix) || !strings.HasSuffix(s, suffix) { - return false - } - inner := s[len(prefix) : len(s)-len(suffix)] - fields := strings.Split(inner, ",") - if len(fields) < 2 || fields[0] == "" { - return false - } - switch fields[0] { - case AlgoAES256SIV, AlgoAES256SIVTypeBound: - default: + parsed, err := parse(string(raw), true) + if err != nil { return false } - - hasData := false - for _, field := range fields[1:] { - switch { - case strings.HasPrefix(field, "key:"): - version, err := strconv.Atoi(strings.TrimPrefix(field, "key:")) - if err != nil || version < 1 { - return false - } - case strings.HasPrefix(field, "data:"): - if _, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(field, "data:")); err != nil { - return false - } - hasData = true - case strings.HasPrefix(field, "type:"): - if !Type(strings.TrimPrefix(field, "type:")).Valid() { - return false - } - default: - return false - } - } - return hasData + return len(parsed.Ciphertext) >= siv.MinCiphertextLen } // Encode wraps ciphertext produced for a whole file into an ENC[...] tag, @@ -278,6 +252,17 @@ func Decode(raw string) ([]byte, error) { // tag names its own version explicitly, and the caller (spec 06) is // responsible for finding the matching key in the local keyring. func Parse(raw string) (Tag, error) { + return parse(raw, false) +} + +// parse implements the grammar both Parse and LooksEncrypted share. +// keyOptional relaxes only the "key:" field requirement, and only for +// AlgoAES256SIV — see LooksEncrypted's doc comment for why. Every other +// rule (field order, no duplicates, no trailing fields, a well-formed +// "data:" payload) applies identically either way, so a tag shape +// LooksEncrypted accepts is always one Parse would too, once it also +// carries a "key:" field. +func parse(raw string, keyOptional bool) (Tag, error) { if !strings.HasPrefix(raw, prefix) || !strings.HasSuffix(raw, suffix) { return Tag{}, fmt.Errorf("%w: missing %q...%q delimiters", ErrMalformed, prefix, suffix) } @@ -303,15 +288,25 @@ func Parse(raw string) (Tag, error) { } rest := fields[1:] - keyField, rest := rest[0], rest[1:] - keyValue, ok := strings.CutPrefix(keyField, "key:") - if !ok { + // AlgoAES256SIVTypeBound postdates key rotation (it didn't exist + // before "key:" did), so it never qualifies for the keyless + // pre-rotation exception below, whatever the caller asked for. + var version int + if len(rest) > 0 && strings.HasPrefix(rest[0], "key:") { + keyValue := strings.TrimPrefix(rest[0], "key:") + v, err := strconv.Atoi(keyValue) + if err != nil || v < 1 { + return Tag{}, fmt.Errorf("%w: invalid key version %q", ErrMalformed, keyValue) + } + version = v + rest = rest[1:] + } else if !keyOptional || typeBound { + keyField := "" + if len(rest) > 0 { + keyField = rest[0] + } return Tag{}, fmt.Errorf("%w: expected a \"key:\" field, got %q", ErrMalformed, keyField) } - version, err := strconv.Atoi(keyValue) - if err != nil || version < 1 { - return Tag{}, fmt.Errorf("%w: invalid key version %q", ErrMalformed, keyValue) - } if len(rest) == 0 { return Tag{}, fmt.Errorf("%w: missing \"data:\" field", ErrMalformed) diff --git a/internal/tag/tag_test.go b/internal/tag/tag_test.go index adc5219..e36c98d 100644 --- a/internal/tag/tag_test.go +++ b/internal/tag/tag_test.go @@ -256,6 +256,17 @@ func TestLooksEncryptedRejectsMalformedTagShapes(t *testing.T) { {"unrecognized algo", "ENC[ROT13,key:1,data:AA==]"}, {"missing suffix", "ENC[AES256_SIV,key:1,data:AA=="}, {"missing prefix", "AES256_SIV,key:1,data:AA==]"}, + + // Regression tests for the 2026-09-30 reanalysis's finding 4.2: + // the old per-field validation accepted all four of these as + // already-encrypted, even though none of them is a tag this + // package ever produced or could decrypt. A plaintext value that + // happened to look like one of these would have been committed + // unencrypted. + {"empty data field", "ENC[AES256_SIV,data:]"}, + {"duplicate data field", "ENC[AES256_SIV,data:AAAA,data:AAAA]"}, + {"type-bound tag missing its mandatory key field", "ENC[AES256_SIV_TB,data:AAAA]"}, + {"data too short to be genuine SIV output", "ENC[AES256_SIV,key:1,data:AAAA]"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -276,10 +287,13 @@ func TestLooksEncryptedAcceptsWellFormedTagShapes(t *testing.T) { name string raw string }{ - {"whole-file tag", "ENC[AES256_SIV,key:1,data:c29tZXRoaW5n]"}, - {"pre-rotation whole-file tag (no key field)", "ENC[AES256_SIV,data:c29tZXRoaW5n]"}, - {"legacy value tag", "ENC[AES256_SIV,key:1,data:c29tZXRoaW5n,type:str]"}, - {"type-bound value tag", "ENC[AES256_SIV_TB,key:1,data:c29tZXRoaW5n,type:int]"}, + // The base64 below decodes to 36 bytes — long enough to pass as + // genuine SIV output (internal/siv.MinCiphertextLen), unlike a + // short placeholder such as "something". + {"whole-file tag", "ENC[AES256_SIV,key:1,data:c2l4dGVlbi1ieXRlLWNpcGhlcnRleHQtcGxhY2Vob2xkZXI=]"}, + {"pre-rotation whole-file tag (no key field)", "ENC[AES256_SIV,data:c2l4dGVlbi1ieXRlLWNpcGhlcnRleHQtcGxhY2Vob2xkZXI=]"}, + {"legacy value tag", "ENC[AES256_SIV,key:1,data:c2l4dGVlbi1ieXRlLWNpcGhlcnRleHQtcGxhY2Vob2xkZXI=,type:str]"}, + {"type-bound value tag", "ENC[AES256_SIV_TB,key:1,data:c2l4dGVlbi1ieXRlLWNpcGhlcnRleHQtcGxhY2Vob2xkZXI=,type:int]"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 099a326e1ef3dd0f2426e68dac51ac1602f67012 Mon Sep 17 00:00:00 2001 From: Marwin Moellers Date: Thu, 1 Oct 2026 11:45:30 +0200 Subject: [PATCH 2/5] Fix R06 follow-up: legacy value tags could flip to invalid-literal types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-09-30 reanalysis (finding 4.4) found that validTypeLiteral used strconv's own parsers to decide whether a decrypted value was a genuine literal of its claimed type, but strconv is more permissive than either output format: strconv.ParseFloat accepts "NaN"/"Inf", and strconv.ParseBool accepts "1"/"t" and more. Neither reads back as the claimed type in JSON or this project's YAML handling, so an attacker with write access who flips a legacy (pre-Package-F, unauthenticated) tag's type field onto one of these could make Clean/Smudge render invalid JSON, or a silently wrong-typed value. Added Handler.RoundTrip (internal/format): it renders a value as its claimed type, then re-reads that rendered text back through the same Locate function Clean/Smudge already use, requiring the exact same value and type to come back out. validTypeLiteral now uses this for TypeFloat/TypeBool instead of strconv directly. TypeInt keeps its existing strconv range check, since JSON has no defined integer range for RoundTrip to enforce on its own (the deliberate int64-overflow limit from the original R06 fix). Because Render and Locate are the same pair of functions a field's type was originally classified by, anything that actually went through Clean normally round-trips by construction — this only ever rejects a value whose claimed type doesn't match what a parser would otherwise infer for it, i.e. exactly the adversarial case. Co-Authored-By: Claude Sonnet 5 --- internal/filterop/filterop.go | 36 ++++++++++++------- internal/filterop/value_test.go | 64 +++++++++++++++++++++++++++++++++ internal/format/format.go | 19 ++++++++++ internal/format/format_test.go | 41 +++++++++++++++++++++ internal/format/json.go | 23 ++++++++++++ internal/format/yaml.go | 24 +++++++++++++ 6 files changed, 195 insertions(+), 12 deletions(-) diff --git a/internal/filterop/filterop.go b/internal/filterop/filterop.go index 0608c7a..0bb5838 100644 --- a/internal/filterop/filterop.go +++ b/internal/filterop/filterop.go @@ -211,7 +211,7 @@ func cleanValues(rule config.Rule, version int, key []byte, haveKey bool, filePa // `nebel add`/`git add`/renormalize instead of ciphertext already // committed to git that no future smudge can turn back into the // original value (audit 2026-09-30, R06). - if err := validTypeLiteral(span.Value, span.Type); err != nil { + if err := validTypeLiteral(handler, span.Value, span.Type); err != nil { return nil, fmt.Errorf("filterop: clean %s at %s: value cannot be safely restored later: %w", filePath, field, err) } if _, err := handler.Render(span.Value, span.Type); err != nil { @@ -308,7 +308,7 @@ func smudgeValues(rule config.Rule, keyring Keyring, filePath string, input []by // 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 { + if err := validTypeLiteral(handler, string(plaintext), parsed.Type); err != nil { return nil, nil, fmt.Errorf("filterop: smudge %s at %s: %w", filePath, field, err) } rendered, err := handler.Render(string(plaintext), parsed.Type) @@ -330,18 +330,30 @@ var ErrInvalidTypeLiteral = errors.New("filterop: decrypted value is not a valid // 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 +// +// TypeInt additionally enforces signed 64-bit range via strconv, a scope +// limit this tool chooses rather than one either output format imposes +// (audit 2026-09-30, R06's int64-overflow case) — handler.RoundTrip alone +// wouldn't catch it, since JSON has no defined integer range and this +// package's YAML handler classifies by the lexer's own token type, not a +// numeric range check. +// +// TypeFloat and TypeBool are checked by handler.RoundTrip instead of +// strconv directly: strconv.ParseFloat accepts "NaN"/"Inf"/hex floats, +// and strconv.ParseBool accepts "1"/"t"/"T" and more, none of which +// either output format reads back as the claimed type (audit 2026-09-30, +// reanalysis 4.4 — see the Handler.RoundTrip doc comment for the concrete +// examples this closes). +func validTypeLiteral(handler format.Handler, value string, t tag.Type) 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) + if _, err := strconv.ParseInt(value, 10, 64); err != nil { + return fmt.Errorf("%w: %q is not a valid %s: %v", ErrInvalidTypeLiteral, value, t, err) + } + case tag.TypeFloat, tag.TypeBool: + if err := handler.RoundTrip(value, t); err != nil { + return fmt.Errorf("%w: %q is not a valid %s: %v", ErrInvalidTypeLiteral, value, t, err) + } } return nil } diff --git a/internal/filterop/value_test.go b/internal/filterop/value_test.go index 144359e..b568a89 100644 --- a/internal/filterop/value_test.go +++ b/internal/filterop/value_test.go @@ -203,6 +203,34 @@ func TestValueRoundTripPreservesType(t *testing.T) { } } +// Regression/confirmation test for the 2026-09-30 reanalysis's finding +// 4.4: validTypeLiteral now checks TypeFloat and TypeBool via +// handler.RoundTrip instead of strconv directly (internal/format). +// Legitimate float and bool values — the ones this test actually +// round-trips — must still clean and smudge back out unchanged: RoundTrip +// re-uses the exact same Locate/Render pair Clean and Smudge already +// call, so anything Locate itself classified as float/bool round-trips by +// construction, whatever literal forms that same lexer happens to accept. +func TestCleanValuesRoundTripsGenuineFloatAndBool(t *testing.T) { + doc := "database:\n active: true\n rate: 3.14\n" + cfg := valueConfig("database.active", "database.rate") + + cleaned, err := Clean(cfg, testKeyring, "config/staging.yaml", []byte(doc)) + if err != nil { + t.Fatalf("Clean: %v", err) + } + if !strings.Contains(string(cleaned), "type:bool") || !strings.Contains(string(cleaned), "type:float") { + t.Errorf("bool/float type not recorded in the tag:\n%s", cleaned) + } + smudged, _, err := Smudge(cfg, testKeyring, "config/staging.yaml", cleaned) + if err != nil { + t.Fatalf("Smudge: %v", err) + } + if string(smudged) != doc { + t.Errorf("round trip changed the document:\n got = %q\n want = %q", smudged, doc) + } +} + // AC-6.3: clean is deterministic, so re-staging an unchanged file produces // no diff. func TestCleanValuesIsDeterministic(t *testing.T) { @@ -510,3 +538,39 @@ func TestSmudgeValuesRejectsNonLiteralAfterLegacyTypeFlip(t *testing.T) { t.Errorf("Smudge() error = %v, want %v", err, ErrInvalidTypeLiteral) } } + +// Regression tests for the 2026-09-30 reanalysis's finding 4.4: Go's own +// strconv parsers accept strings neither output format reads back as the +// claimed type, so a legacy tag's unauthenticated type field could be +// flipped onto one of these and still pass the old strconv-only check — +// "NaN" is accepted by strconv.ParseFloat but isn't valid JSON/YAML float +// syntax, and "1" is accepted by strconv.ParseBool but both formats read +// it back as an int. Both must now be refused, the same as +// TestSmudgeValuesRejectsNonLiteralAfterLegacyTypeFlip's ordinary-text +// case, rather than rendered as invalid source or a silently wrong type. +func TestSmudgeValuesRejectsStrconvPermissiveLiteralsAfterLegacyTypeFlip(t *testing.T) { + tests := []struct { + name string + legacyValue string + newType string + }{ + {"NaN flipped to float", "NaN", "float"}, + {"1 flipped to bool", "1", "bool"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.yaml", "database.password") + ciphertext, err := siv.Encrypt(testKey, []byte(tt.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:"+tt.newType+"]", 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 c889573..63b6ea6 100644 --- a/internal/format/format.go +++ b/internal/format/format.go @@ -94,6 +94,25 @@ type Handler interface { // yamlHandler.Render's doc comment for the one case that applies today // (audit 2026-09-29, P10). Render(value string, t tag.Type) (string, error) + + // RoundTrip reports an error unless rendering value as t, then + // reading that rendered text back via Locate, recovers exactly value + // and t again. This is what validTypeLiteral (internal/filterop) + // uses to decide whether a historical, type-flipped legacy value tag + // (see internal/tag's doc comment on AlgoAES256SIV's unauthenticated + // type field) is safe to render: a value accepted by Go's own + // strconv parsers is not necessarily one this format's own parser + // would read back the same way — strconv.ParseFloat accepts "NaN", + // which is not a value either JSON or this package's YAML handler + // recognizes as a float, and strconv.ParseBool accepts "1", which + // both formats read back as an int rather than a bool. Checking + // round-trip fidelity against each format's own parser catches both + // without needing a hand-maintained catalogue of every way a format's + // accepted literals are stricter than Go's (audit 2026-09-30, + // reanalysis 4.4). A value Clean or Smudge actually produced always + // round-trips, by construction, so this never rejects legitimate + // content. + RoundTrip(value string, t tag.Type) error } // Leaf is one scalar in a document, addressable by Path. diff --git a/internal/format/format_test.go b/internal/format/format_test.go index 141155a..ed1dbb2 100644 --- a/internal/format/format_test.go +++ b/internal/format/format_test.go @@ -689,3 +689,44 @@ func TestLeaves(t *testing.T) { }) } } + +// Regression test for the 2026-09-30 reanalysis's finding 4.4: Go's +// strconv parsers accept literal forms ("NaN", "Inf", "1" as a bool) that +// neither JSON nor this package's YAML handler actually reads back as +// the claimed type. RoundTrip is what validTypeLiteral (internal/filterop) +// relies on instead of a standalone strconv check for TypeFloat/TypeBool. +func TestHandlerRoundTrip(t *testing.T) { + tests := []struct { + name string + value string + typ tag.Type + wantErr bool + }{ + {"genuine bool true", "true", tag.TypeBool, false}, + {"genuine bool false", "false", tag.TypeBool, false}, + {"genuine float", "3.14", tag.TypeFloat, false}, + {"genuine negative float", "-0.5", tag.TypeFloat, false}, + {"NaN claimed as float", "NaN", tag.TypeFloat, true}, + {"Inf claimed as float", "Inf", tag.TypeFloat, true}, + {"1 claimed as bool", "1", tag.TypeBool, true}, + {"0 claimed as bool", "0", tag.TypeBool, true}, + {"t claimed as bool", "t", tag.TypeBool, true}, + } + for _, format := range []string{"x.json", "x.yaml"} { + h, err := For(format) + if err != nil { + t.Fatal(err) + } + for _, tt := range tests { + t.Run(format+"/"+tt.name, func(t *testing.T) { + err := h.RoundTrip(tt.value, tt.typ) + if tt.wantErr && err == nil { + t.Errorf("RoundTrip(%q, %s) = nil, want an error", tt.value, tt.typ) + } + if !tt.wantErr && err != nil { + t.Errorf("RoundTrip(%q, %s) = %v, want nil", tt.value, tt.typ, err) + } + }) + } + } +} diff --git a/internal/format/json.go b/internal/format/json.go index 5b1a157..eafb8c8 100644 --- a/internal/format/json.go +++ b/internal/format/json.go @@ -175,6 +175,29 @@ func (jsonHandler) Render(value string, t tag.Type) (string, error) { } } +// RoundTrip embeds value, rendered as t, as the lone field of a minimal +// JSON object and reads it back via Locate — see the Handler interface +// doc comment for why this, rather than a standalone literal syntax +// check, is what catches a historical legacy value tag (internal/tag) +// whose unauthenticated type field was flipped to a type the literal +// text doesn't actually support in JSON (e.g. "NaN" claimed as a float: +// strconv.ParseFloat accepts it, but it isn't valid JSON number syntax). +func (h jsonHandler) RoundTrip(value string, t tag.Type) error { + rendered, err := h.Render(value, t) + if err != nil { + return err + } + doc := []byte(`{"v":` + rendered + `}`) + span, err := h.Locate(doc, "v") + if err != nil { + return fmt.Errorf("%q does not read back as valid JSON: %w", rendered, err) + } + if span.Type != t || span.Value != value { + return fmt.Errorf("%q reads back as %s %q, not %s %q", rendered, span.Type, span.Value, t, value) + } + return nil +} + func delimName(d json.Delim) string { if d == '{' { return "object" diff --git a/internal/format/yaml.go b/internal/format/yaml.go index d8b0d35..18f002d 100644 --- a/internal/format/yaml.go +++ b/internal/format/yaml.go @@ -338,6 +338,30 @@ func (yamlHandler) Render(value string, t tag.Type) (string, error) { } } +// RoundTrip embeds value, rendered as t, as the lone field of a minimal +// YAML mapping and reads it back via Locate — see the Handler interface +// doc comment for why this, rather than a standalone literal syntax +// check, is what catches a historical legacy value tag (internal/tag) +// whose unauthenticated type field was flipped to a type the literal +// text doesn't actually support in YAML (e.g. "1" claimed as a bool: +// strconv.ParseBool accepts it, but YAML 1.1 reads a bare "1" back as an +// int, not one of its recognized boolean literals). +func (h yamlHandler) RoundTrip(value string, t tag.Type) error { + rendered, err := h.Render(value, t) + if err != nil { + return err + } + doc := []byte("v: " + rendered + "\n") + span, err := h.Locate(doc, "v") + if err != nil { + return fmt.Errorf("%q does not read back as valid YAML: %w", rendered, err) + } + if span.Type != t || span.Value != value { + return fmt.Errorf("%q reads back as %s %q, not %s %q", rendered, span.Type, span.Value, t, value) + } + return nil +} + // ErrControlCharacterUnsupported is returned by Render when a decrypted // string contains a control character this build cannot safely render // into a YAML double-quoted scalar — see yamlEscapeDoubleQuoted. From 6aa86f0f2d8552ae69398f7d6b06cfb55853a585 Mon Sep 17 00:00:00 2001 From: Marwin Moellers Date: Thu, 1 Oct 2026 11:46:52 +0200 Subject: [PATCH 3/5] Fix R04 follow-up: cross-process races on the local key file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-09-30 reanalysis (finding 4.3) found that Set's read-modify-write of the local key file was serialized only within one process (setMu): two concurrent `nebel` processes (a racing filter invocation, two concurrent `nebel rotate` runs, ...) could each read the same old content and then each write back a different, equally "complete" update, silently losing whichever lost the race. A corrected repro (the originally reported figure was a test-isolation artifact, not a real measurement) lost 37 of 60 concurrent registrations in a single run. Set now also takes an exclusive, cross-process advisory lock on a dedicated sibling file for the same read-modify-write span: flock on Unix (internal/localkey/filelock_unix.go), LockFileEx on Windows (filelock_windows.go). Locking a sibling file rather than the key file itself matters because writeKeyFile replaces the key file by renaming a temp file onto it (for atomicity against a concurrent reader) — a lock held on the key file's own inode would stop protecting anything the moment the first writer's rename swapped a new inode in underneath it. Also adds: - Remove, to delete a single version's entry (used by cmd/nebel/rotate.go in a later commit to undo its own Set on a failed, reverted rotation). - LockRotation, a second lock serializing an entire `nebel rotate` operation against another concurrent one in the same clone (used in the same later commit) — Set's own lock only protects the key file's bytes, not the higher-level decision of which next version number to mint, so it alone doesn't stop two concurrent rotations from both reading the same current version and each minting the same next one with a different key. golang.org/x/sys was already an indirect dependency (via golang.org/x/term); go.mod now lists it directly, with no version change. Co-Authored-By: Claude Sonnet 5 --- go.mod | 3 +- internal/localkey/crossprocess_test.go | 119 ++++++++++++++++++++++ internal/localkey/filelock_unix.go | 43 ++++++++ internal/localkey/filelock_windows.go | 44 ++++++++ internal/localkey/localkey.go | 135 +++++++++++++++++++++++-- 5 files changed, 334 insertions(+), 10 deletions(-) create mode 100644 internal/localkey/crossprocess_test.go create mode 100644 internal/localkey/filelock_unix.go create mode 100644 internal/localkey/filelock_windows.go diff --git a/go.mod b/go.mod index f59bdd0..dc47203 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/goccy/go-yaml v1.19.2 github.com/tink-crypto/tink-go/v2 v2.8.0 golang.org/x/crypto v0.57.0 + golang.org/x/sys v0.48.0 golang.org/x/term v0.46.0 ) - -require golang.org/x/sys v0.48.0 // indirect diff --git a/internal/localkey/crossprocess_test.go b/internal/localkey/crossprocess_test.go new file mode 100644 index 0000000..f8d00e8 --- /dev/null +++ b/internal/localkey/crossprocess_test.go @@ -0,0 +1,119 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package localkey + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "strconv" + "sync" + "testing" +) + +// TestMain lets this test binary re-exec itself as a bare-bones worker +// process that does nothing but call Set(version, key) once. Real +// separate processes are needed because lockFile's flock/LockFileEx +// already correctly serializes concurrent goroutines within one process +// too (see setMu's doc comment) — only distinct processes, each with +// their own keyring read before any of them writes, can reproduce the +// lost-update race this test is checking is now closed. +func TestMain(m *testing.M) { + if os.Getenv("NEBEL_CROSSPROCESS_HELPER") == "1" { + version, _ := strconv.Atoi(os.Args[1]) + key := []byte(os.Args[2]) + if err := Set(version, key); err != nil { + fmt.Fprintln(os.Stderr, "helper Set failed:", err) + os.Exit(1) + } + os.Exit(0) + } + os.Exit(m.Run()) +} + +// Regression test for the 2026-09-30 reanalysis's finding 4.3: Set's +// read-modify-write of the key file was serialized only within one +// process (setMu); two concurrent `nebel` processes could each read the +// same old content and then each write back a different, equally +// "complete" update, silently losing whichever one lost the race — a real +// run of this test's predecessor, before the lockFile fix, lost 37 of 60 +// concurrent registrations in a single run. +func TestCrossProcessSetDoesNotLoseUpdates(t *testing.T) { + dir := t.TempDir() + runGit(t, dir, "init", "-q") + runGit(t, dir, "config", "user.email", "t@t.com") + runGit(t, dir, "config", "user.name", "t") + + // localkey resolves the key file path relative to the current + // process's git repo (gitCommonDir), so both this parent (which reads + // back the result via All()) and every helper subprocess it launches + // need their working directory set to dir — a helper's cmd.Dir alone + // covers the subprocess, but not this process's own later All() call. + orig, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + t.Cleanup(func() { os.Chdir(orig) }) + if err := os.Chdir(dir); err != nil { + t.Fatalf("Chdir: %v", err) + } + + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable: %v", err) + } + + const n = 60 + var wg sync.WaitGroup + results := make([]error, n) + for i := 0; i < n; i++ { + version := i + 1 + wg.Add(1) + go func(idx, version int) { + defer wg.Done() + cmd := exec.Command(exe, strconv.Itoa(version), fmt.Sprintf("key-for-v%d", version)) + cmd.Env = append(os.Environ(), "NEBEL_CROSSPROCESS_HELPER=1") + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + results[idx] = fmt.Errorf("version %d: %v: %s", version, err, out) + } + }(i, version) + } + wg.Wait() + + for _, err := range results { + if err != nil { + t.Fatalf("helper process itself failed (not the race under test): %v", err) + } + } + + all, err := All() + if err != nil { + t.Fatalf("All: %v", err) + } + var missing []int + for i := 0; i < n; i++ { + version := i + 1 + if _, ok := all[version]; !ok { + missing = append(missing, version) + } + } + if len(missing) > 0 { + t.Errorf("%d/%d versions reported Set() success but are missing from the final key file (lost update across processes): %v", len(missing), n, missing) + } +} + +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + var out bytes.Buffer + cmd.Stdout = &out + cmd.Stderr = &out + if err := cmd.Run(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out.String()) + } +} diff --git a/internal/localkey/filelock_unix.go b/internal/localkey/filelock_unix.go new file mode 100644 index 0000000..1d1fc75 --- /dev/null +++ b/internal/localkey/filelock_unix.go @@ -0,0 +1,43 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +//go:build !windows + +package localkey + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// fileLock holds an exclusive, advisory, cross-process lock on the file it +// was opened for (see lockFile). +type fileLock struct { + f *os.File +} + +// lockFile opens (creating if necessary) the file at path and blocks until +// it holds an exclusive flock on it. flock's lock is released automatically +// if this process dies while holding it (unlike a lock file whose mere +// existence signals "locked" — that kind can't tell a crashed holder from +// a live one, and needs its own stale-lock cleanup), so a `nebel` process +// killed mid-Set never leaves the key file permanently unwritable to +// others. +func lockFile(path string) (*fileLock, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil { + f.Close() + return nil, err + } + return &fileLock{f: f}, nil +} + +// Unlock releases the lock and closes the underlying file handle. +func (l *fileLock) Unlock() error { + defer l.f.Close() + return unix.Flock(int(l.f.Fd()), unix.LOCK_UN) +} diff --git a/internal/localkey/filelock_windows.go b/internal/localkey/filelock_windows.go new file mode 100644 index 0000000..afb7f5e --- /dev/null +++ b/internal/localkey/filelock_windows.go @@ -0,0 +1,44 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +//go:build windows + +package localkey + +import ( + "os" + + "golang.org/x/sys/windows" +) + +// fileLock holds an exclusive, advisory, cross-process lock on the file it +// was opened for (see lockFile). +type fileLock struct { + f *os.File +} + +// lockFile opens (creating if necessary) the file at path and blocks until +// it holds an exclusive LockFileEx lock on it — this platform's equivalent +// of the Unix flock lockFile (filelock_unix.go) takes, released +// automatically if this process dies while holding it. +func lockFile(path string) (*fileLock, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + var overlapped windows.Overlapped + // One byte is enough to lock: this file is never read or written + // through the handle itself, only used as a mutex. + if err := windows.LockFileEx(windows.Handle(f.Fd()), windows.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, &overlapped); err != nil { + f.Close() + return nil, err + } + return &fileLock{f: f}, nil +} + +// Unlock releases the lock and closes the underlying file handle. +func (l *fileLock) Unlock() error { + defer l.f.Close() + var overlapped windows.Overlapped + return windows.UnlockFileEx(windows.Handle(l.f.Fd()), 0, 1, 0, &overlapped) +} diff --git a/internal/localkey/localkey.go b/internal/localkey/localkey.go index da96aa4..cbcf63e 100644 --- a/internal/localkey/localkey.go +++ b/internal/localkey/localkey.go @@ -36,13 +36,34 @@ import ( ) // setMu serializes Set's read-modify-write of the key file against other -// goroutines in this process. It does not extend across processes — a -// separate `nebel` invocation racing this one (a concurrent filter -// process, say) is not covered, since doing so portably (this project -// supports Windows, so a unix-only syscall.Flock is off the table) would -// need a cross-process lock file of its own, with its own stale-lock and -// cleanup questions. Narrowing the race to cross-process only is a -// deliberate, accepted trade-off, not a silent one. +// goroutines in this process. The cross-process lockFile below would +// still correctly serialize two goroutines in this same process too on +// Unix (verified empirically there: a second, independent open-and-flock +// from the same process blocks until the first is released, rather than +// acquiring a separate, non-conflicting lock) — Windows' LockFileEx is +// only compile-checked, not verified the same way, but documented as +// having the same per-open-handle semantics. Either way, setMu isn't +// needed for correctness, only kept as a cheap in-process fast path in +// front of it, avoiding a file open and a syscall for the common case of +// no cross-process contention at all. +// +// The lockFile call inside Set covers a concurrent `nebel` process (a +// racing filter invocation, or another command reading the keyring mid-Set) +// losing or clobbering an update to the key file itself — the lost-update +// race reproduced and fixed in audit 2026-09-30, reanalysis 4.3. It does +// not, by itself, serialize two concurrent full `nebel rotate` operations: +// each could still read the same current version from .nebel.yaml and +// independently decide to mint the same next version with a different +// key before either calls Set — Set's lock then just decides, silently, +// which of the two keys ends up registered for that version. Preventing +// that is cmd/nebel/rotate.go's job (see localkey.LockRotation), not +// this package's: a bare Get/Set caller has no "operation" to serialize +// around, only the one write this function already protects. +// +// Advisory, not mandatory, locking is the right trade-off here: every +// locker is this same package (plus, for LockRotation, cmd/nebel +// directly), so there is no untrusted writer to defend against, only +// ordinary concurrent legitimate use. var setMu sync.Mutex // configKeyPrefix is the local git config variable a version's key was @@ -115,6 +136,62 @@ func keyFilePath() (string, error) { return filepath.Join(dir, keyFileName), nil } +// lockFilePath returns the path of the sibling file Set locks to serialize +// its read-modify-write across processes. A dedicated file, not the key +// file itself: writeKeyFile replaces the key file by renaming a temp file +// onto it (for atomicity against a concurrent reader), so a lock held on +// the key file's own inode would stop protecting anything the moment the +// first writer's rename swaps a new inode in underneath it — the next +// locker would lock the old, now-orphaned inode no one else can still see. +func lockFilePath() (string, error) { + path, err := keyFilePath() + if err != nil { + return "", err + } + return path + ".lock", nil +} + +// LockRotation acquires an exclusive, cross-process lock over a full +// `nebel rotate` operation (cmd/nebel/rotate.go), from before it reads +// the current key version through the point it finishes re-encrypting +// everything under the new one. The returned func releases it; call it +// exactly once, however rotate finishes. +// +// This is a different file from Set's own lockFilePath, deliberately: +// rotate calls Set once as part of the operation this lock already spans, +// and flock/LockFileEx locks conflict against another lock this same +// process already holds via a different open file description (verified +// empirically — see setMu's doc comment) — locking the same file twice +// in one process would block that second call forever, on itself. +// +// Without this, two concurrent `rotate` runs in the same clone could both +// read the same current version from .nebel.yaml and each independently +// decide to mint the same next version with a different key: Set's own +// lock (audit 2026-09-30, reanalysis 4.3) only stops that pair of calls +// from corrupting the key file, not from both proceeding at all — whichever +// one's Save runs last would silently decide the project's actual key for +// a version the other process's keyring now also, wrongly, claims to +// have the key for. This lock serializes the two attempts instead, so the +// second one sees the first's result and mints version N+2, not another +// N+1. +// +// Scoped to one shared git directory — the same scope Set's own keyring +// is shared across (gitCommonDir) — so it serializes every worktree of +// one clone, but not `rotate` running concurrently in two independent +// clones (which have no shared state to protect in the first place; each +// produces its own, independently valid, next version). +func LockRotation() (func() error, error) { + dir, err := gitCommonDir() + if err != nil { + return nil, err + } + lock, err := lockFile(filepath.Join(dir, keyFileName+".rotate.lock")) + if err != nil { + return nil, fmt.Errorf("localkey: acquiring rotation lock: %w", err) + } + return lock.Unlock, nil +} + // 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 @@ -271,14 +348,56 @@ func Get(version int) (key []byte, ok bool, err error) { // the legacy git-config location (see readLegacyEntries), which this // never migrates or removes. func Set(version int, key []byte) error { + return withKeyFileLock(func(entries map[int]string) (map[int]string, error) { + entries[version] = base64.StdEncoding.EncodeToString(key) + return entries, nil + }) +} + +// Remove deletes version's entry from the key file, if present; removing +// an absent version is not an error. Unlike Set, this is not meant for +// ordinary use — Set's own "existing versions are left untouched" +// guarantee (AC-11.5) exists specifically so a clone never loses the +// ability to read something it could read before. Remove exists only for +// a caller undoing a Set it just made itself but no longer wants: a +// failed `nebel rotate` reverting the key version it minted, so that +// version doesn't linger registered under this attempt's key and later +// collide with whatever key a teammate's own successful rotation to the +// same version number eventually registers for it (audit 2026-09-30, +// reanalysis 4.3). +func Remove(version int) error { + return withKeyFileLock(func(entries map[int]string) (map[int]string, error) { + delete(entries, version) + return entries, nil + }) +} + +// withKeyFileLock runs edit under both setMu and the cross-process +// lockFile (see their doc comments), over the same read-modify-write span +// both Set and Remove need: read the current entries, let edit produce +// the next version of the map, write it back. +func withKeyFileLock(edit func(entries map[int]string) (map[int]string, error)) error { setMu.Lock() defer setMu.Unlock() + lockPath, err := lockFilePath() + if err != nil { + return err + } + lock, err := lockFile(lockPath) + if err != nil { + return fmt.Errorf("localkey: locking %s: %w", lockPath, err) + } + defer lock.Unlock() + entries, err := readKeyFile() if err != nil { return err } - entries[version] = base64.StdEncoding.EncodeToString(key) + entries, err = edit(entries) + if err != nil { + return err + } return writeKeyFile(entries) } From 014c39c7d6b72e4f3cc46b4a714f45149a35a6f5 Mon Sep 17 00:00:00 2001 From: Marwin Moellers Date: Thu, 1 Oct 2026 11:48:10 +0200 Subject: [PATCH 4/5] Fix R03 follow-up: wrong rotate recovery advice, and parallel-rotate races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-09-30 reanalysis (finding 4.1) found that rotateRecoveryHint told the user to run `git checkout -- .` to undo a failed rotation, claiming it "fully discards" the attempt. That command restores the working tree from the index, not HEAD — once gitutil.Add had already staged the rotated config (exactly the state when a later re-encryption failure is reported), the rotation stayed staged afterward despite the message's claim. `.` could also discard unrelated uncommitted work elsewhere in the repo. `nebel rotate` now reverts itself automatically instead of printing instructions. Before changing anything, it snapshots .nebel.yaml's exact working-tree bytes and index entry (",", gitutil.IndexEntry), and the local keyring's prior entry (if any) for the version it's about to mint. On any failure, it restores all of that precisely: - The working tree file, always. - The index entry via gitutil.SetIndexEntry rather than re-running `git add` — which would instead stage whatever happens to be in the working tree by then, including a pre-existing unstaged edit to .nebel.yaml that predates this rotate attempt and was never staged in the first place. - The local key registration for the version this attempt minted: Set it back to whatever was there before (almost always nothing, so Remove), rather than leaving this attempt's orphaned key registered. Left behind, it would collide with whatever key a teammate's own later, independent rotation to that same version number eventually registers, turning that clone's next `git pull` into a hard authentication failure instead of the usual graceful "missing key" passthrough. Restoring to the snapshot taken when this attempt started, not to HEAD, matters: AC-11.9 explicitly allows running `rotate` again before a previous rotation is even committed, in which case HEAD is one rotation further back than what a failed retry should revert to. Also closes the other half of reanalysis finding 4.3: two concurrent `nebel rotate` runs in the same clone could each read the same current version and independently mint the same next one with a different key, silently colliding. runRotate now holds localkey.LockRotation for the whole operation, so the second attempt sees the first's committed-to-disk result and mints the version after it instead. Fixed the CLI help text, which claimed rotate "does not touch already-encrypted content" — it eagerly re-encrypts everything. Co-Authored-By: Claude Sonnet 5 --- USER_INTERACTIONS.md | 7 + cmd/nebel/main.go | 5 +- cmd/nebel/rotate.go | 185 +++++++++++-- cmd/nebel/rotate_recovery_e2e_test.go | 365 ++++++++++++++++++++++++++ internal/gitutil/gitutil.go | 40 +++ specs/11-cli-rotate.md | 11 + 6 files changed, 591 insertions(+), 22 deletions(-) create mode 100644 cmd/nebel/rotate_recovery_e2e_test.go diff --git a/USER_INTERACTIONS.md b/USER_INTERACTIONS.md index 07a91b6..0877e16 100644 --- a/USER_INTERACTIONS.md +++ b/USER_INTERACTIONS.md @@ -255,6 +255,13 @@ leaked, change the underlying secrets too, not just the password. `ENC[...]` in the working tree): rotating anyway would silently strand that field on its existing version forever. The error names the stuck path, field, and version, and points at `nebel init --version N`. +- If something fails partway through (disk full while saving the new + config, `git add` itself failing, or a re-encryption failure), `rotate` + automatically restores `.nebel.yaml` to exactly what it held before this + attempt — working tree and, if it was already staged, the index too — + before reporting the error. No managed file is ever at risk: re-encryption + only stages one once it has succeeded for every managed path in the same + step, so a failure there leaves all of them untouched. - Other clones/CI keep reading old content fine with their *current* key right up until they pull the rotation commit — after that, since rotation just re-encrypted everything on this branch, they need the new diff --git a/cmd/nebel/main.go b/cmd/nebel/main.go index 64a902d..caf203a 100644 --- a/cmd/nebel/main.go +++ b/cmd/nebel/main.go @@ -61,8 +61,9 @@ func usage() { encrypt named values inside ; with no paths, choose them interactively nebel rotate [--password-stdin] - mint a new, current key version; does not - touch already-encrypted content + mint a new, current key version and + eagerly re-encrypt everything managed + under it nebel clean (invoked by git) encrypt stdin to stdout nebel smudge (invoked by git) decrypt stdin to stdout nebel version print the build version`) diff --git a/cmd/nebel/rotate.go b/cmd/nebel/rotate.go index 5f726de..b324972 100644 --- a/cmd/nebel/rotate.go +++ b/cmd/nebel/rotate.go @@ -7,6 +7,7 @@ import ( "crypto/subtle" "encoding/base64" "fmt" + "os" "path/filepath" "sort" "strings" @@ -40,6 +41,40 @@ func runRotate(args []string) error { if !config.Exists(configPath) { return fmt.Errorf("no %s found — run `nebel init` first", config.FileName) } + + // Serializes this entire operation against another concurrent `nebel + // rotate` in the same clone (audit 2026-09-30, reanalysis 4.3): without + // it, two runs could both read the same current version below and each + // mint the same next one with a different key. See LockRotation's doc + // comment. Acquired before originalConfigBytes is even read, so a + // concurrent rotate can't change configPath out from under that + // snapshot between reading it and this one finishing. + unlockRotation, err := localkey.LockRotation() + if err != nil { + return err + } + defer unlockRotation() + + // Captured before anything changes, so a failure partway through can + // restore configPath to exactly this — not necessarily HEAD's content: + // AC-11.9 lets `rotate` run again before the previous rotation is even + // committed, in which case this is that still-staged, uncommitted + // result, and HEAD would be one rotation further back than what a + // failed retry needs to revert to (audit 2026-09-30, reanalysis 4.1). + originalConfigBytes, err := os.ReadFile(configPath) + if err != nil { + return fmt.Errorf("reading %s: %w", config.FileName, err) + } + // The index entry alongside it, for the same reason but the index + // half: restoring it later via SetIndexEntry, rather than re-running + // Add against whatever the working tree holds by then, is what keeps + // a revert from staging a pre-existing unstaged edit to configPath + // that predates this rotate attempt entirely (audit 2026-09-30, + // reanalysis 4.1). + originalConfigIndex, haveOriginalConfigIndex, err := gitutil.IndexEntry(root, config.FileName) + if err != nil { + return err + } cfg, err := config.Load(configPath) if err != nil { return err @@ -131,15 +166,38 @@ func runRotate(args []string) error { return fmt.Errorf("internal error: freshly created canary did not verify: %w", err) } + // Captured so a failed attempt can put the keyring back exactly how it + // found it (rotateAttempt.failure): newVersion almost never already + // has a locally-registered key at this point, but if it somehow does + // (e.g. `nebel init --version N` fetched it out of band before this + // clone's own rotate happened to mint the same number), Set below + // would silently overwrite it, and simply deleting it on revert would + // be just as wrong as leaving this attempt's own key behind. + prevKey, hadPrevKey, err := localkey.Get(newVersion) + if err != nil { + return fmt.Errorf("reading local key: %w", err) + } + // AC-11.5: register the new key before saving the config. If the // config write below failed after this, this machine would still be // able to clean under the new version on retry; the reverse order // could leave a saved config this machine can't yet use. + attempt := rotateAttempt{ + root: root, + configPath: configPath, + originalBytes: originalConfigBytes, + originalIndex: originalConfigIndex, + haveOriginalIndex: haveOriginalConfigIndex, + newVersion: newVersion, + prevKey: prevKey, + hadPrevKey: hadPrevKey, + generated: generated, + } if err := localkey.Set(newVersion, newKey); err != nil { return fmt.Errorf("registering local key: %w", err) } if err := next.Save(configPath); err != nil { - return rotateRecoveryHint(err) + return attempt.failure(false, err) } // Stage the config, then eagerly re-encrypt every managed file/field @@ -150,34 +208,121 @@ func runRotate(args []string) error { // reaches everything: nothing left encrypted going in means nothing // comes out still tagged with the old version. if err := gitutil.Add(root, config.FileName); err != nil { - return rotateRecoveryHint(err) + return attempt.failure(false, err) } migrated, err := gitutil.RenormalizeAll(root) if err != nil { - return rotateRecoveryHint(fmt.Errorf("re-encrypting existing content under the new version: %w", err)) + return attempt.failure(true, fmt.Errorf("re-encrypting existing content under the new version: %w", err)) } printRotateSummary(newVersion, migrated) return nil } -// rotateRecoveryHint explains how to undo a rotation that failed partway -// through registering the new config or re-encrypting content under it -// (audit 2026-09-30, R03). Nothing runRotate does reaches this point is -// ever committed — only the working tree and the index change — so a -// plain git command already fully reverts it; no separate rollback -// mechanism is needed. Re-running `nebel rotate` also works without first -// reverting anything: eager re-encryption is idempotent (spec 11 AC-11.10) -// and Set/Save both tolerate being called again with the same or a fresh -// version, so a retry converges the rest instead of leaving anything -// half-migrated — it just mints another version in the process. -func rotateRecoveryHint(err error) error { - return fmt.Errorf("%w\n\nNothing from this rotation has been committed yet — "+ - "`git checkout -- .` (or `git reset --hard HEAD`, if you have no other "+ - "uncommitted changes) fully discards it. Or leave it as-is, fix the "+ - "underlying problem, and re-run `nebel rotate`: re-encryption is "+ - "idempotent, so it converges the rest instead of leaving anything "+ - "half-migrated.", err) +// rotateAttempt carries what a single runRotate call needs to revert +// itself if it fails partway through (audit 2026-09-30, reanalysis 4.1 +// and 4.3). +type rotateAttempt struct { + root, configPath string + + // originalBytes is configPath's exact content the instant runRotate + // started — not necessarily HEAD's: AC-11.9 lets `rotate` run again + // before a previous rotation is even committed, in which case this is + // that still-staged, uncommitted result, and HEAD would be one + // rotation further back than what a failed retry needs to revert to. + originalBytes []byte + + // originalIndex is configPath's index entry (",", see + // gitutil.IndexEntry) at the same instant, restored via + // gitutil.SetIndexEntry rather than re-running Add so a revert never + // stages whatever happens to be in the working tree by the time it + // runs — including a pre-existing unstaged edit to configPath that + // predates this attempt and was never staged in the first place. + originalIndex string + haveOriginalIndex bool + + newVersion int + + // prevKey/hadPrevKey is whatever localkey.Get(newVersion) returned + // right before this attempt's own localkey.Set(newVersion, ...) — + // almost always hadPrevKey == false, since newVersion doesn't exist + // yet anywhere else. Restoring exactly this (Set it back, or Remove + // if there was nothing) rather than unconditionally removing + // newVersion is what keeps a revert from erasing a key some other, + // unrelated process had already legitimately registered for that + // same version number. + prevKey []byte + hadPrevKey bool + + generated bool +} + +// failure reverts configPath and the local keyring back to what they held +// before this attempt, and reports cause alongside what that revert did. +// staged must be true only when this attempt's own gitutil.Add already +// staged configPath before the failure being reported — i.e. only for the +// gitutil.RenormalizeAll failure point, not next.Save's or gitutil.Add's +// own. +// +// Nothing runRotate does before the point this is called from is ever +// committed, and no managed file's working tree or index state is ever +// touched by any of its three failure points: `git add --renormalize` +// only stages a managed file when it succeeds for every path in that +// single invocation (verified empirically — git aborts the whole index +// update on the first filter failure), so a failure there leaves every +// managed file exactly as it was. Only configPath and the local keyring +// entry for newVersion ever need undoing. +// +// Restoring the keyring matters beyond tidiness: without it, this +// attempt's orphaned key for newVersion stays registered after the +// revert. If the user doesn't retry, and a teammate's own, independent +// rotation later succeeds in minting that same version number with a +// *different* key, this clone's next `git pull` finds a version its +// keyring already claims to have a key for — Smudge's AC-6.11 passthrough +// only applies to a version the keyring doesn't hold at all, so instead of +// the usual graceful "missing key" passthrough, decryption fails outright +// with a hard authentication error (audit 2026-09-30, reanalysis 4.3). +func (a rotateAttempt) failure(staged bool, cause error) error { + passwordNote := "" + if a.generated { + passwordNote = " The password shown above was never applied to anything — discard it; a retry generates a new one." + } + if err := os.WriteFile(a.configPath, a.originalBytes, 0o644); err != nil { + if a.generated { + passwordNote = fmt.Sprintf(" %s's working tree copy may still hold the new, uncommitted version the password shown above applies to — keep that password until you've confirmed what %[1]s actually contains.", config.FileName) + } + return fmt.Errorf("%w\n\nAdditionally, restoring %s to what it held before this attempt failed: %v.%s Rewrite it by hand before retrying.", + cause, config.FileName, err, passwordNote) + } + if staged { + if !a.haveOriginalIndex { + return fmt.Errorf("%w\n\n%s's working tree copy was restored, but it was not tracked in the index before this "+ + "attempt started, so the now-stale staged version can't be restored automatically — run `git rm --cached -- %s` "+ + "by hand before retrying.%s", cause, config.FileName, config.FileName, passwordNote) + } + if err := gitutil.SetIndexEntry(a.root, a.originalIndex, config.FileName); err != nil { + return fmt.Errorf("%w\n\n%s's working tree copy was restored, but re-staging its prior version failed: %v — "+ + "run `git add -- %s` by hand before retrying, so the index doesn't still hold the new version (this may "+ + "also re-stage any of your own unrelated pending edit to it).%s", + cause, config.FileName, err, config.FileName, passwordNote) + } + } + if a.hadPrevKey { + if err := localkey.Set(a.newVersion, a.prevKey); err != nil { + return fmt.Errorf("%w\n\n%s was fully reverted, but restoring version %d's previous local key failed: %v — "+ + "run `nebel init --version %d` again by hand before retrying.%s", + cause, config.FileName, a.newVersion, err, a.newVersion, passwordNote) + } + } else if err := localkey.Remove(a.newVersion); err != nil { + return fmt.Errorf("%w\n\n%s was fully reverted, but removing the orphaned local key this attempt registered "+ + "for version %d failed: %v — if a teammate's own rotation later mints that same version with a different "+ + "key, this clone's next `git pull` will hard-fail on it instead of the usual graceful passthrough, until "+ + "that stale entry is removed by hand.%s", cause, config.FileName, a.newVersion, err, passwordNote) + } + return fmt.Errorf("%w\n\nThis rotation attempt was automatically reverted: %s and the local keyring are both "+ + "back to what they held before this attempt, and no managed file was ever touched.%s Fix the problem above, "+ + "then re-run `nebel rotate`; it will mint version %[4]d again, with a newly generated key, not this "+ + "attempt's.", cause, config.FileName, passwordNote, a.newVersion) } // printGeneratedPasswordNotice reports an auto-generated password the diff --git a/cmd/nebel/rotate_recovery_e2e_test.go b/cmd/nebel/rotate_recovery_e2e_test.go new file mode 100644 index 0000000..c726a2c --- /dev/null +++ b/cmd/nebel/rotate_recovery_e2e_test.go @@ -0,0 +1,365 @@ +// Copyright (C) 2026 Marwin Moellers +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/MuffinUser/nebel/internal/config" +) + +// setupRotatableValueRepo bootstraps a repo with one mode: value field +// already encrypted and committed under key version 1 — a minimal +// starting point for forcing a failure partway through a later rotate. +func setupRotatableValueRepo(t *testing.T, pathEnv string) string { + t.Helper() + 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") + return repo +} + +// breakStagingPassword overwrites config/staging.yaml's database.password +// value, in the working tree only, with one containing a raw control +// character — the same value TestCleanValuesRejectsUnrenderableControlCharacter +// (internal/filterop) proves Clean refuses, via format.ErrControlCharacterUnsupported. +// Used here as a fault seam: checkFullyDecryptable doesn't flag it (it +// isn't an ENC[...] tag), so rotate proceeds past its own pre-flight +// check and fails only once gitutil.RenormalizeAll actually tries to +// re-encrypt it. +func breakStagingPassword(t *testing.T, repo string) { + t.Helper() + path := filepath.Join(repo, "config", "staging.yaml") + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + broken := strings.Replace(string(data), `password: "s3cr3t"`, "password: \"a\x01b\"", 1) + if broken == string(data) { + t.Fatal("test setup: database.password literal not found to break") + } + if err := os.WriteFile(path, []byte(broken), 0o644); err != nil { + t.Fatal(err) + } +} + +// Regression test for the 2026-09-30 reanalysis's finding 4.1: the +// previous rotateRecoveryHint claimed `git checkout -- .` "fully +// discards" a failed rotation, but that command restores the working +// tree from the index, not HEAD — once gitutil.Add has already staged +// the rotated config (exactly the state at the point RenormalizeAll can +// fail), the rotation stayed staged afterward. rotate now reverts +// config.FileName itself instead of telling the user to run a command. +func TestRotateAutomaticallyRevertsOnRenormalizeFailure(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + beforeConfig := readConfig(t, repo) + beforeStagingCommitted := runIn(t, repo, pathEnv, "git", "show", "HEAD:config/staging.yaml") + + breakStagingPassword(t, repo) + + out, err := runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password"}, "") + if err == nil { + t.Fatalf("rotate with an unrenderable field: want an error, got success:\n%s", out) + } + if !strings.Contains(out, "automatically reverted") { + t.Errorf("rotate error does not confirm an automatic revert:\n%s", out) + } + if strings.Contains(out, "git checkout") || strings.Contains(out, "git reset") { + t.Errorf("rotate error still tells the user to run a git command by hand:\n%s", out) + } + + afterConfig := readConfig(t, repo) + if afterConfig != beforeConfig { + t.Errorf("config.FileName not fully reverted:\n got = %q\n want = %q", afterConfig, beforeConfig) + } + + staged := strings.TrimSpace(runIn(t, repo, pathEnv, "git", "diff", "--cached", "--name-only")) + if staged != "" { + t.Errorf("something is still staged after the automatic revert: %q", staged) + } + + afterStagingCommitted := runIn(t, repo, pathEnv, "git", "show", "HEAD:config/staging.yaml") + if afterStagingCommitted != beforeStagingCommitted { + t.Error("the managed file's committed content changed even though rotate never staged it") + } + + if _, ok := registeredKeyValue(t, repo, pathEnv, 2); ok { + t.Error("version 2's orphaned local key was not removed by the automatic revert (audit 2026-09-30, reanalysis 4.3: a teammate's later, independent rotation to the same version number would collide with it)") + } +} + +// Companion to the above: a second `nebel rotate` run before the first +// one is committed (AC-11.9 explicitly allows this) must, on its own +// failure, revert to the still-staged result of the *first* rotation — +// not to HEAD, which is one rotation further back. An earlier draft of +// this fix restored from HEAD unconditionally and would have silently +// discarded the first rotation's legitimate staged work here. +func TestRotateRevertsToPriorStagedRotationNotHEAD(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + out, err := runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password-1"}, "") + if err != nil { + t.Fatalf("first nebel rotate: %v\n%s", err, out) + } + stagedAfterFirstRotate := readConfig(t, repo) + if !strings.Contains(stagedAfterFirstRotate, "key_version: 2") { + t.Fatalf("test setup: first rotate did not reach version 2:\n%s", stagedAfterFirstRotate) + } + stagedPathsAfterFirstRotate := strings.TrimSpace(runIn(t, repo, pathEnv, "git", "diff", "--cached", "--name-only")) + + // An edit made after the first rotate, still unstaged — plausible if + // someone touched the file before running rotate again. + breakStagingPassword(t, repo) + + out, err = runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password-2"}, "") + if err == nil { + t.Fatalf("second rotate with an unrenderable field: want an error, got success:\n%s", out) + } + + afterFailedSecondRotate := readConfig(t, repo) + if afterFailedSecondRotate != stagedAfterFirstRotate { + t.Errorf("second rotate's failure did not revert to the first rotate's staged result:\n got = %q\n want = %q", afterFailedSecondRotate, stagedAfterFirstRotate) + } + if strings.Contains(afterFailedSecondRotate, "key_version: 1") { + t.Error("second rotate's failure reverted all the way back to the pre-rotation HEAD, discarding the first rotation's legitimate staged work") + } + + staged := strings.TrimSpace(runIn(t, repo, pathEnv, "git", "diff", "--cached", "--name-only")) + if staged != stagedPathsAfterFirstRotate { + t.Errorf("staged files after the failed second rotate = %q, want unchanged from after the first rotate %q", staged, stagedPathsAfterFirstRotate) + } +} + +// A generated password is shown before any state changes (audit +// 2026-09-30, R03) — but once a later failure auto-reverts the attempt +// that generated it, that password no longer applies to anything. The +// error must say so, rather than leaving the earlier "store it now" +// notice as the last word on it. +func TestRotateFailureDiscardsGeneratedPasswordAfterSuccessfulRevert(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + beforeConfig := readConfig(t, repo) + breakStagingPassword(t, repo) + + // No password supplied: rotate generates and prints one before it + // fails, exactly the sequence that makes the "discard it" note matter. + out, err := runRotateWith(t, repo, pathEnv, nil, "") + if err == nil { + t.Fatalf("rotate with an unrenderable field: want an error, got success:\n%s", out) + } + if !strings.Contains(out, "Generated new password:") { + t.Fatalf("test setup: rotate did not generate a password:\n%s", out) + } + if !strings.Contains(out, "discard it") { + t.Errorf("rotate error does not tell the user to discard the now-unused generated password:\n%s", out) + } + + if afterConfig := readConfig(t, repo); afterConfig != beforeConfig { + t.Errorf("config.FileName not fully reverted:\n got = %q\n want = %q", afterConfig, beforeConfig) + } +} + +// Regression test for a gap in the first version of this fix's revert +// logic: it re-staged config.FileName unconditionally after restoring it, +// which would have swept up any pre-existing *unstaged* edit to it (made +// before this rotate attempt even started) into the index — something +// this attempt never did and has no business doing. Re-staging must only +// happen when this attempt's own gitutil.Add already staged config.FileName +// (the RenormalizeAll failure point) — not when gitutil.Add itself is what +// failed, forced here via a stale .git/index.lock. +func TestRotateFailureDoesNotStageUnrelatedPreexistingConfigEdit(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + // An edit to .nebel.yaml that predates this rotate attempt and was + // never staged — config.Load/Save round-trips it into whatever rotate + // saves, so it must survive untouched in both the working tree (with + // the edit) and the index (without it) if this attempt fails. + configPath := filepath.Join(repo, config.FileName) + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + preexistingEdit := strings.Replace(string(before), "rules:\n", "rules:\n- files: \"*.unrelated\"\n mode: file\n", 1) + if preexistingEdit == string(before) { + t.Fatal("test setup: rules literal not found to extend") + } + if err := os.WriteFile(configPath, []byte(preexistingEdit), 0o644); err != nil { + t.Fatal(err) + } + indexBefore := runIn(t, repo, pathEnv, "git", "show", ":"+config.FileName) + + lockPath := filepath.Join(repo, ".git", "index.lock") + if err := os.WriteFile(lockPath, nil, 0o644); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Remove(lockPath) }) + + out, err := runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password"}, "") + if err == nil { + t.Fatalf("rotate with a stale index.lock: want an error, got success:\n%s", out) + } + if !strings.Contains(out, "automatically reverted") { + t.Errorf("rotate error does not confirm an automatic revert:\n%s", out) + } + + afterWorktree, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(afterWorktree) != preexistingEdit { + t.Errorf("the pre-existing unstaged edit was not preserved in the working tree:\n got = %q\n want = %q", afterWorktree, preexistingEdit) + } + + os.Remove(lockPath) // `git show :path` below needs the index usable again + indexAfter := runIn(t, repo, pathEnv, "git", "show", ":"+config.FileName) + if indexAfter != indexBefore { + t.Errorf("the pre-existing unstaged edit was swept into the index by the failed rotate's revert:\n got = %q\n want = %q", indexAfter, indexBefore) + } +} + +// Regression test for the second part of the 2026-09-30 reanalysis's +// finding 4.3: two concurrent `nebel rotate` processes in the same clone +// used to be able to both read key_version: 1 and each independently +// mint version 2 with a different key — Set's own lost-update fix only +// decided which of the two keys silently won, not whether both proceeded +// at all. localkey.LockRotation (acquired by runRotate) now serializes +// the two attempts, so the second one mints version 3 instead of also +// minting version 2. +// +// Subprocess output/errors are collected into slices rather than +// t.Fatal'd from inside the goroutines, since testing.T methods other +// than Log/Logf are not safe to call from a non-test goroutine. +func TestConcurrentRotatesDoNotCollideOnTheSameVersion(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + passwords := []string{"rotated-password-a", "rotated-password-b"} + outs := make([]string, len(passwords)) + errs := make([]error, len(passwords)) + + var wg sync.WaitGroup + for i, pw := range passwords { + wg.Add(1) + go func(i int, pw string) { + defer wg.Done() + cmd := exec.Command(filepath.Join(binDir, binName), "rotate") + cmd.Dir = repo + cmd.Env = append(os.Environ(), "PATH="+pathEnv, passwordEnv+"="+pw) + out, err := cmd.CombinedOutput() + outs[i], errs[i] = string(out), err + }(i, pw) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("concurrent rotate #%d failed: %v\n%s", i, err, outs[i]) + } + } + + finalConfig := readConfig(t, repo) + if !strings.Contains(finalConfig, "key_version: 3") { + t.Errorf("two concurrent rotates did not both proceed to a distinct version (want key_version: 3):\n%s", finalConfig) + } + + for _, v := range []int{1, 2, 3} { + if _, ok := registeredKeyValue(t, repo, pathEnv, v); !ok { + t.Errorf("version %d has no registered local key after two concurrent rotates", v) + } + } + + // A silent collision (both processes reading key_version: 1 and each + // minting version 2 with a different key) wouldn't necessarily show up + // as key_version: 3 above — the losing process's Save could still run + // last and leave key_version: 2, while the keyring ends up with the + // *other* process's key for it. A third, sequential rotate's own + // AC-11.1 canary check — requiring the local key for the *current* + // version to verify against the *current* canary — catches exactly + // that mismatch, which a bare version-number check alone could miss. + out, err := runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password-c"}, "") + if err != nil { + t.Fatalf("third, sequential rotate: %v\n%s", err, out) + } + if finalConfig := readConfig(t, repo); !strings.Contains(finalConfig, "key_version: 4") { + t.Errorf("third rotate did not reach version 4 (canary/key mismatch from the concurrent pair?):\n%s", finalConfig) + } +} + +// Regression test for a gap in an earlier draft of this fix: at the +// RenormalizeAll failure point (staged == true), the revert used to +// re-run `git add` against whatever the working tree held by then — which +// is correct for the RESTORED content in the common case, but wrong if +// config.FileName also carried its own pre-existing *unstaged* edit from +// before this rotate attempt even started: `git add` would stage that +// edit too, something this attempt never did. Restoring the index via its +// captured mode+sha (gitutil.SetIndexEntry) instead of re-adding fixes +// this, independent of whatever the working tree holds by the time the +// revert runs. +// +// Unlike TestRotateFailureDoesNotStageUnrelatedPreexistingConfigEdit +// (which forces the gitutil.Add failure point, staged == false, where +// nothing ever gets re-staged at all — a stale index.lock prevents any +// index write, so that test cannot distinguish a correct revert from a +// merely absent one), this one forces the RenormalizeAll failure point +// specifically, so a revert that (incorrectly) re-runs `git add` here +// actually has the opportunity to stage the pre-existing edit, and this +// test can tell the difference. +func TestRotateFailureRestoresExactIndexAtRenormalizeFailure(t *testing.T) { + pathEnv := pathEnvWithBin(t) + repo := setupRotatableValueRepo(t, pathEnv) + + configPath := filepath.Join(repo, config.FileName) + committed, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + indexBefore := runIn(t, repo, pathEnv, "git", "show", ":"+config.FileName) + + preexistingEdit := strings.Replace(string(committed), "rules:\n", "rules:\n- files: \"*.unrelated\"\n mode: file\n", 1) + if preexistingEdit == string(committed) { + t.Fatal("test setup: rules literal not found to extend") + } + if err := os.WriteFile(configPath, []byte(preexistingEdit), 0o644); err != nil { + t.Fatal(err) + } + + // Forces the RenormalizeAll failure point (staged == true): rotate's + // own Add succeeds and stages this attempt's rotated config (which + // incorporates the pre-existing edit above, since config.Load reads + // whatever's currently on disk) before RenormalizeAll fails on the + // broken field. + breakStagingPassword(t, repo) + + out, err := runRotateWith(t, repo, pathEnv, []string{passwordEnv + "=rotated-password"}, "") + if err == nil { + t.Fatalf("rotate with an unrenderable field: want an error, got success:\n%s", out) + } + if !strings.Contains(out, "automatically reverted") { + t.Errorf("rotate error does not confirm an automatic revert:\n%s", out) + } + + afterWorktree, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(afterWorktree) != preexistingEdit { + t.Errorf("the pre-existing unstaged edit was not preserved in the working tree:\n got = %q\n want = %q", afterWorktree, preexistingEdit) + } + + indexAfter := runIn(t, repo, pathEnv, "git", "show", ":"+config.FileName) + if indexAfter != indexBefore { + t.Errorf("the pre-existing unstaged edit was swept into the index by the failed rotate's revert:\n got = %q\n want = %q (the original committed content, never staged by this attempt)", indexAfter, indexBefore) + } +} diff --git a/internal/gitutil/gitutil.go b/internal/gitutil/gitutil.go index 16f459c..9ffbb9a 100644 --- a/internal/gitutil/gitutil.go +++ b/internal/gitutil/gitutil.go @@ -87,6 +87,46 @@ func Add(repoRoot string, paths ...string) error { return nil } +// IndexEntry returns path's current index entry as "," — enough +// for SetIndexEntry to restore it exactly later — and ok=false if path +// isn't currently tracked in the index at all. Unlike indexBlobs (which +// renormalize's before/after diff only needs the hash from, to detect a +// change), a full restoration via `git update-index --cacheinfo` needs +// the mode too. +func IndexEntry(repoRoot, path string) (modeSHA string, ok bool, err error) { + cmd := exec.Command("git", "ls-files", "-s", "-z", "--", path) + cmd.Dir = repoRoot + out, err := cmd.Output() + if err != nil { + return "", false, fmt.Errorf("gitutil: git ls-files -s: %w", wrapExitErr(err)) + } + record := strings.TrimRight(string(out), "\x00") + if record == "" { + return "", false, nil + } + meta, _, cut := strings.Cut(record, "\t") + fields := strings.Fields(meta) + if !cut || len(fields) < 2 { + return "", false, fmt.Errorf("gitutil: unexpected `git ls-files -s` output for %s: %q", path, record) + } + return fields[0] + "," + fields[1], true, nil +} + +// SetIndexEntry restores path's index entry to modeSHA (as returned by +// IndexEntry), independent of path's current working tree content — +// unlike Add, which stages whatever the working tree currently holds. +// Used by `nebel rotate` to undo its own Add on a later failure without +// disturbing an unrelated edit already sitting in the working tree when +// rotate started (audit 2026-09-30, reanalysis 4.1). +func SetIndexEntry(repoRoot, modeSHA, path string) error { + cmd := exec.Command("git", "update-index", "--cacheinfo", modeSHA+","+path) + cmd.Dir = repoRoot + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("gitutil: git update-index --cacheinfo: %w: %s", wrapExitErr(err), strings.TrimSpace(string(out))) + } + return nil +} + // 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. diff --git a/specs/11-cli-rotate.md b/specs/11-cli-rotate.md index 7686851..249f812 100644 --- a/specs/11-cli-rotate.md +++ b/specs/11-cli-rotate.md @@ -106,3 +106,14 @@ up front or to pick up that lazy convergence afterward. leaves the committed config untouched. Minting a new version while silently leaving part of the repo unreadable under it is exactly the outcome AC-11.4 exists to prevent. +- **AC-11.12 A failure partway through reverts itself**: `rotate` captures + `.nebel.yaml`'s exact content before changing anything. If it then fails + while writing the new config, staging it, or re-encrypting content under + it (AC-11.2/AC-11.6), it automatically restores that captured content — + to the working tree, and to the index if it had already been staged — + before reporting the error, so no managed file and no leftover + half-rotated config is left behind. It restores to what `.nebel.yaml` + held when this `rotate` invocation started, not necessarily `HEAD`: + AC-11.9 allows running `rotate` again before a previous rotation is + committed, and in that case `HEAD` is one rotation further back than + what a failed retry should revert to (audit 2026-09-30, reanalysis 4.1). From 1ef5be850a241703de9261c1aaccae25de77ddc0 Mon Sep 17 00:00:00 2001 From: Marwin Moellers Date: Thu, 1 Oct 2026 14:25:57 +0200 Subject: [PATCH 5/5] Close 4.4's int case: round-trip ints, not just strconv.ParseInt The previous commit closed reanalysis finding 4.4 for floats and bools, but left ints on strconv.ParseInt alone. That accepts a leading "+" and leading zeros: "+5" isn't valid JSON number syntax, and "007" is neither valid JSON nor read back as an int by the YAML handler. A legacy value tag whose unauthenticated type field is flipped from str to int could therefore still render invalid JSON, or a silently changed type. validTypeLiteral now runs handler.RoundTrip for ints too. The strconv check stays alongside it for the deliberate int64 range limit, which RoundTrip can't enforce on its own. Co-Authored-By: Claude Opus 5.5 --- internal/filterop/filterop.go | 33 ++++++++++++++++++--------------- internal/filterop/value_test.go | 24 ++++++++++++++++++++++++ internal/format/format_test.go | 3 +++ 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/internal/filterop/filterop.go b/internal/filterop/filterop.go index 0bb5838..d8a2342 100644 --- a/internal/filterop/filterop.go +++ b/internal/filterop/filterop.go @@ -331,29 +331,32 @@ var ErrInvalidTypeLiteral = errors.New("filterop: decrypted value is not a valid // passes: Render quotes it, so whatever bytes it contains stay inert data // rather than becoming unquoted source syntax. // -// TypeInt additionally enforces signed 64-bit range via strconv, a scope -// limit this tool chooses rather than one either output format imposes -// (audit 2026-09-30, R06's int64-overflow case) — handler.RoundTrip alone +// Every non-string type is checked by handler.RoundTrip rather than by +// strconv alone: strconv.ParseFloat accepts "NaN"/"Inf"/hex floats, +// strconv.ParseBool accepts "1"/"t"/"T" and more, and strconv.ParseInt +// accepts a leading "+" or leading zeros ("+5", "007") — none of which +// both output formats read back as the claimed type (audit 2026-09-30, +// reanalysis 4.4; see the Handler.RoundTrip doc comment). +// +// TypeInt additionally keeps its strconv check, for signed 64-bit range: a +// scope limit this tool chooses rather than one either output format +// imposes (audit 2026-09-30, R06's int64-overflow case). RoundTrip alone // wouldn't catch it, since JSON has no defined integer range and this // package's YAML handler classifies by the lexer's own token type, not a // numeric range check. -// -// TypeFloat and TypeBool are checked by handler.RoundTrip instead of -// strconv directly: strconv.ParseFloat accepts "NaN"/"Inf"/hex floats, -// and strconv.ParseBool accepts "1"/"t"/"T" and more, none of which -// either output format reads back as the claimed type (audit 2026-09-30, -// reanalysis 4.4 — see the Handler.RoundTrip doc comment for the concrete -// examples this closes). func validTypeLiteral(handler format.Handler, value string, t tag.Type) error { switch t { - case tag.TypeInt: + case tag.TypeInt, tag.TypeFloat, tag.TypeBool: + default: + return nil + } + if t == tag.TypeInt { if _, err := strconv.ParseInt(value, 10, 64); err != nil { return fmt.Errorf("%w: %q is not a valid %s: %v", ErrInvalidTypeLiteral, value, t, err) } - case tag.TypeFloat, tag.TypeBool: - if err := handler.RoundTrip(value, t); err != nil { - return fmt.Errorf("%w: %q is not a valid %s: %v", ErrInvalidTypeLiteral, value, t, err) - } + } + if err := handler.RoundTrip(value, t); err != nil { + return fmt.Errorf("%w: %q is not a valid %s: %v", ErrInvalidTypeLiteral, value, t, err) } return nil } diff --git a/internal/filterop/value_test.go b/internal/filterop/value_test.go index b568a89..a6f9440 100644 --- a/internal/filterop/value_test.go +++ b/internal/filterop/value_test.go @@ -556,6 +556,7 @@ func TestSmudgeValuesRejectsStrconvPermissiveLiteralsAfterLegacyTypeFlip(t *test }{ {"NaN flipped to float", "NaN", "float"}, {"1 flipped to bool", "1", "bool"}, + {"leading zero flipped to int", "007", "int"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -574,3 +575,26 @@ func TestSmudgeValuesRejectsStrconvPermissiveLiteralsAfterLegacyTypeFlip(t *test }) } } + +// Companion to TestSmudgeValuesRejectsStrconvPermissiveLiteralsAfterLegacyTypeFlip +// for a JSON document: strconv.ParseInt accepts a leading "+", and YAML +// reads "+5" back as an int, but it isn't valid JSON number syntax, so a +// legacy tag flipped from str to int must not render it unquoted into a +// JSON document (audit 2026-09-30, reanalysis 4.4). +func TestSmudgeValuesRejectsPlusSignedIntInJSONAfterLegacyTypeFlip(t *testing.T) { + const legacyValue = "+5" + cfg := &config.Config{Rules: []config.Rule{ + {Files: "config/*.json", Mode: config.ModeValue, Encrypt: []string{"secret"}}, + }} + legacyAAD := siv.AAD(siv.ModeValue, "config/staging.json", "secret") + ciphertext, err := siv.Encrypt(testKey, []byte(legacyValue), legacyAAD) + if err != nil { + t.Fatalf("siv.Encrypt: %v", err) + } + tamperedTag := fmt.Sprintf("ENC[%s,key:1,data:%s,type:int]", tag.AlgoAES256SIV, base64.StdEncoding.EncodeToString(ciphertext)) + + doc := `{"secret": "` + tamperedTag + `"}` + if _, _, err := Smudge(cfg, testKeyring, "config/staging.json", []byte(doc)); !errors.Is(err, ErrInvalidTypeLiteral) { + t.Errorf("Smudge() error = %v, want %v", err, ErrInvalidTypeLiteral) + } +} diff --git a/internal/format/format_test.go b/internal/format/format_test.go index ed1dbb2..3881cd2 100644 --- a/internal/format/format_test.go +++ b/internal/format/format_test.go @@ -711,6 +711,9 @@ func TestHandlerRoundTrip(t *testing.T) { {"1 claimed as bool", "1", tag.TypeBool, true}, {"0 claimed as bool", "0", tag.TypeBool, true}, {"t claimed as bool", "t", tag.TypeBool, true}, + {"genuine int", "42", tag.TypeInt, false}, + {"genuine negative int", "-7", tag.TypeInt, false}, + {"leading zero claimed as int", "007", tag.TypeInt, true}, } for _, format := range []string{"x.json", "x.yaml"} { h, err := For(format)