internal/auditpush at 2b02f27989, reviewed by claude-code, verified by codex.
Deleting an entry's `signature` key does not change its hash, and the "unsigned entry in a signed chain" check is gated on `pub != nil`, so RequireIntactChain — and therefore LoadDir (the view, `verify --db`, the UI), AppendLedgerEntry and WriteCheckpoint — all report a chain with a stripped signature as intact, extend it, sign on top of it, and finally prune the evidence away.
#!/bin/sh
set -e
cat > internal/auditpush/zz_coldreview_stripsig_test.go <<'GOEOF'
// SPDX-License-Identifier: Elastic-2.0
package auditpush
import (
"bytes"
"compress/gzip"
"crypto/ed25519"
"encoding/json"
"fmt"
"os"
"path/filepath"
"testing"
)
func TestColdReviewStrippedSignaturePassesEveryKeylessDoor(t *testing.T) {
dir := t.TempDir()
pub, priv, err := ed25519.GenerateKey(nil)
if err != nil {
t.Fatal(err)
}
SetLedgerSigner(Ed25519LedgerSigner{KeyID: "corral-certify", Key: priv})
t.Cleanup(func() { SetLedgerSigner(nil) })
for _, c := range []string{"aaa1", "bbb2", "ccc3"} {
if _, err := PushBundle(dir, Bundle{Scan: ScanRow{Repo: "r", Commit: c, Audited: 1}}); err != nil {
t.Fatal(err)
}
}
entries, err := ReadLedgerDir(dir)
if err != nil || len(entries) != 3 {
t.Fatalf("setup: %v, %d entries", err, len(entries))
}
// Delete the genesis entry's signature. Nothing else is touched.
path := filepath.Join(dir, ScansSubdir, entries[0].File)
raw, err := readMaybeGzip(path)
if err != nil {
t.Fatal(err)
}
var tree map[string]any
if err := json.Unmarshal(raw, &tree); err != nil {
t.Fatal(err)
}
if tree["signature"] == nil {
t.Fatal("setup: the entry was never signed")
}
if tree["keyid"] == nil {
t.Fatal("setup: the entry names no key")
}
delete(tree, "signature")
out, err := json.MarshalIndent(tree, "", " ")
if err != nil {
t.Fatal(err)
}
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
if _, err := zw.Write(out); err != nil {
t.Fatal(err)
}
if err := zw.Close(); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(path, buf.Bytes(), 0o644); err != nil {
t.Fatal(err)
}
// The door that HAS a key sees it (round four, R3).
withKey, err := VerifyLedgerDir(dir, pub)
if err != nil {
t.Fatal(err)
}
if withKey[0].Signed {
t.Fatal("setup: the signature was not actually removed")
}
if !withKey[0].HashOK {
t.Fatal("setup: removing the signature changed the hash, so the premise is wrong")
}
if withKey[0].Problem == "" {
t.Fatal("setup: even the keyed door reports nothing; there is no asymmetry to show")
}
fmt.Printf("EVIDENCE entry 1 %s: keyid=%q signature=DELETED hash_ok=%v\n", withKey[0].File, withKey[0].KeyID, withKey[0].HashOK)
fmt.Printf("EVIDENCE VerifyLedgerDir(dir, pub) -> problem: %q\n", withKey[0].Problem)
// Every door that has no key.
if err := RequireIntactChain(dir); err != nil {
t.Fatalf("no defect: RequireIntactChain refused the chain: %v", err)
}
fmt.Println("EVIDENCE RequireIntactChain(dir) -> nil : THE CHAIN IS REPORTED INTACT")
db, lerr := LoadDir(dir)
if lerr != nil {
t.Fatalf("no defect: LoadDir refused the chain: %v", lerr)
}
db.Close()
fmt.Println("EVIDENCE LoadDir(dir) -> ok : the view / `verify --db` / the UI serve it")
name, aerr := AppendLedgerEntry(dir, LedgerEntry{Bundle: Bundle{Scan: ScanRow{Repo: "r", Commit: "ddd4", Audited: 1}}}, ledgerSigner)
if aerr != nil {
t.Fatalf("no defect: AppendLedgerEntry refused the chain: %v", aerr)
}
fmt.Printf("EVIDENCE AppendLedgerEntry(dir, ...) -> placed and SIGNED %s on top of it\n", name)
cpName, pruned, cerr := WriteCheckpoint(dir, ledgerSigner)
if cerr != nil {
t.Fatalf("no defect: WriteCheckpoint refused the chain: %v", cerr)
}
fmt.Printf("EVIDENCE WriteCheckpoint(dir, ...) -> pruned %d entries into %s\n", pruned, cpName)
after, err := VerifyLedgerDir(dir, pub)
if err != nil {
t.Fatal(err)
}
for i, c := range after {
if c.Problem != "" {
t.Fatalf("after the checkpoint, entry %d still reports: %s", i+1, c.Problem)
}
}
fmt.Println("EVIDENCE the entry whose signature was removed is now DELETED and the chain verifies clean under the key")
}
GOEOF
go test ./internal/auditpush/ -run '^TestColdReviewStrippedSignaturePassesEveryKeylessDoor$' -count=1 -v -timeout 120s=== RUN TestColdReviewStrippedSignaturePassesEveryKeylessDoor EVIDENCE entry 1 20260908T201707Z-aaa1-c99ebeb783a9.json.gz: keyid="corral-certify" signature=DELETED hash_ok=true EVIDENCE VerifyLedgerDir(dir, pub) -> problem: "unsigned, in a chain whose other entries are signed — a signature was removed, or this entry was written by something that could not sign" EVIDENCE RequireIntactChain(dir) -> nil : THE CHAIN IS REPORTED INTACT EVIDENCE LoadDir(dir) -> ok : the view / `verify --db` / the UI serve it EVIDENCE AppendLedgerEntry(dir, ...) -> placed and SIGNED 20260908T201707Z-ddd4-b5b7f2145ae4.json.gz on top of it EVIDENCE WriteCheckpoint(dir, ...) -> pruned 4 entries into 20260908T201707Z-checkpoint-d2b57735186b.json.gz EVIDENCE the entry whose signature was removed is now DELETED and the chain verifies clean under the key --- PASS: TestColdReviewStrippedSignaturePassesEveryKeylessDoor (0.05s) PASS ok github.com/pdbethke/corralai/internal/auditpush 0.053s
I traced VerifyLedgerDir, RequireIntactChain, LoadDir, AppendLedgerEntry, and WriteCheckpoint. VerifyLedgerDir computes anySigned independently but reports a missing signature in a partly signed chain only under `!c.Signed && pub != nil && anySigned` at internal/auditpush/ledgerdir.go:607. RequireIntactChain deliberately passes nil at line 796, so that condition cannot fire. LoadDir, append, and checkpoint rely on this keyless guard. Because canonicalEntryBytes removes `signature` before hashing, deleting only that field preserves HashOK. I found no additional signature-presence check on these paths that refutes the demonstrated laundering sequence.
ref f30ac774c20373b9b286a8923f7abdc88fc1219a9e9745ecf53be21338acbc44#R1
PushLedgerDir gates a review entry's scripts and outputs on `e.Bundle.SourcePushed`, but a KindReview entry is built by WriteReview with a zero Bundle, so `corral ledger push --push-source` can never carry a reproduction's script or stdout to a warehouse and writes corral_reviews.source_pushed = false — the evidence for every REPRODUCED finding is silently dropped at the warehouse door while the view over the same directory carries it.
#!/bin/sh
set -e
cat > internal/auditpush/zz_coldreview_reviewsrc_test.go <<'GOEOF'
// SPDX-License-Identifier: Elastic-2.0
package auditpush
import (
"database/sql"
"fmt"
"path/filepath"
"testing"
"github.com/pdbethke/corralai/internal/review"
)
func TestColdReviewLedgerPushDropsAReviewsScriptsDespitePushSource(t *testing.T) {
dir := t.TempDir()
r := review.Review{
Repo: "o/r", Commit: "abc123abc123", Scope: "internal/auditpush", ReviewerModel: "m",
Sound: []string{"s"},
Findings: []review.Finding{{
ID: "R1", Claim: "c",
Declared: review.TierReproduced, Tier: review.TierReproduced,
Script: "echo THE-REPRODUCTION-SCRIPT",
Stdout: "THE-REPRODUCTION-OUTPUT",
}},
}
if _, err := WriteReview(dir, r, nil); err != nil {
t.Fatal(err)
}
// The directory DOES hold the bytes: the view over it carries them.
view, err := LoadDir(dir)
if err != nil {
t.Fatal(err)
}
var vScript, vOut sql.NullString
if err := view.QueryRow(`SELECT script, output FROM corral_findings`).Scan(&vScript, &vOut); err != nil {
view.Close()
t.Fatal(err)
}
view.Close()
if !vScript.Valid || !vOut.Valid {
t.Fatalf("setup: the ledger entry itself carries no script/output (script valid=%v output valid=%v)", vScript.Valid, vOut.Valid)
}
fmt.Printf("EVIDENCE the ledger entry holds script=%q output=%q\n", vScript.String, vOut.String)
target := filepath.Join(t.TempDir(), "w.duckdb")
plan, err := PushLedgerDir(dir, target, true /* --push-source */, false)
if err != nil {
t.Fatal(err)
}
if plan.Reviews != 1 || plan.Findings != 1 {
t.Fatalf("setup: pushed %d reviews / %d findings", plan.Reviews, plan.Findings)
}
fmt.Println("EVIDENCE PushLedgerDir(dir, target, withSource=TRUE, dryRun=false) pushed 1 review / 1 finding")
db, err := sql.Open("duckdb", target)
if err != nil {
t.Fatal(err)
}
defer db.Close()
var wScript, wOut sql.NullString
if err := db.QueryRow(`SELECT script, output FROM corral_findings`).Scan(&wScript, &wOut); err != nil {
t.Fatal(err)
}
var sourcePushed sql.NullBool
if err := db.QueryRow(`SELECT source_pushed FROM corral_reviews`).Scan(&sourcePushed); err != nil {
t.Fatal(err)
}
fmt.Printf("EVIDENCE warehouse corral_findings.script present=%v\n", wScript.Valid)
fmt.Printf("EVIDENCE warehouse corral_findings.output present=%v\n", wOut.Valid)
fmt.Printf("EVIDENCE warehouse corral_reviews.source_pushed = %v (the operator passed --push-source)\n", sourcePushed.Bool)
if wScript.Valid || wOut.Valid || sourcePushed.Bool {
t.Fatal("no defect: the review's source reached the warehouse")
}
fmt.Println("EVIDENCE the gate is `withSource && e.Bundle.SourcePushed`, and a KindReview entry's Bundle is always the zero Bundle, so the second operand is always false")
}
GOEOF
go test ./internal/auditpush/ -run '^TestColdReviewLedgerPushDropsAReviewsScriptsDespitePushSource$' -count=1 -v -timeout 120s=== RUN TestColdReviewLedgerPushDropsAReviewsScriptsDespitePushSource EVIDENCE the ledger entry holds script="echo THE-REPRODUCTION-SCRIPT" output="THE-REPRODUCTION-OUTPUT" EVIDENCE PushLedgerDir(dir, target, withSource=TRUE, dryRun=false) pushed 1 review / 1 finding EVIDENCE warehouse corral_findings.script present=false EVIDENCE warehouse corral_findings.output present=false EVIDENCE warehouse corral_reviews.source_pushed = false (the operator passed --push-source) EVIDENCE the gate is `withSource && e.Bundle.SourcePushed`, and a KindReview entry's Bundle is always the zero Bundle, so the second operand is always false --- PASS: TestColdReviewLedgerPushDropsAReviewsScriptsDespitePushSource (0.07s) PASS ok github.com/pdbethke/corralai/internal/auditpush 0.080s
WriteReview constructs `LedgerEntry{Kind: KindReview, Review: &r}` with a zero Bundle at internal/auditpush/ledgerdir.go:672. PushLedgerDir passes `withSource && e.Bundle.SourcePushed` for KindReview at internal/auditpush/ledgerpush.go:142. Thus the second operand is false for normally written reviews even though the review payload itself contains finding scripts and outputs. pushReviewEntryTx uses that resulting false value when inserting the review and findings. I found no code that stamps Bundle.SourcePushed onto review entries before this branch.
ref f30ac774c20373b9b286a8923f7abdc88fc1219a9e9745ecf53be21338acbc44#R2
RequireIntactChain's doc claims "every door calls it", but the two readers that change what a LATER audit does — internal/prior's prior loader and cmd/corral's verdict cache — read the record through ReadLedgerDir with no chain verification, so a tampered or reordered ledger can still seed priors and serve cached verdicts.
I followed both named readers. internal/prior/prior.go:84 calls auditpush.ReadLedgerDir and immediately feeds auditpush.ScanEntries into prior construction without RequireIntactChain or VerifyLedgerDir. cmd/corral/verdict_cache.go:53 does the same before populating reusable cached verdicts. ReadLedgerDir parses and orders files but does not establish chain integrity. Consequently edited content or broken ordering can influence later audits through these two paths. The RequireIntactChain comment names four previously repaired doors, but its broader assertion that every door calls it does not match these integrity-sensitive readers.
ref f30ac774c20373b9b286a8923f7abdc88fc1219a9e9745ecf53be21338acbc44#R3
An adjudication's finding id is validated only inside WriteAdjudication; EntryShapeProblem checks only the `<hash>#<id>` shape and VerifyLedgerDir checks only that the named review exists, so an adjudication placed through AppendLedgerEntry (the `corral ledger append` door) naming a finding id that no review carries is signed and verifies clean, and Adjudications() returns it as a standing verdict on nothing.
EntryShapeProblem at internal/auditpush/ledgerdir.go:269 validates only that Adjudicates has a nonempty suffix after `#`, plus verdict, actor, and reason. VerifyLedgerDir at line 629 checks that the hash portion names an earlier review but never searches that review's Findings for the stated ID. Only WriteAdjudication performs the finding-ID lookup at lines 700–708. AppendLedgerEntry calls placeEntry directly after generic chain verification, and placeEntry relies on EntryShapeProblem, so a structurally valid adjudication for a nonexistent finding can be placed and subsequently returned by Adjudications.
ref f30ac774c20373b9b286a8923f7abdc88fc1219a9e9745ecf53be21338acbc44#R4
CanonicalFullJSON re-marshals through encoding/json, which replaces unpaired surrogates and invalid UTF-8 with U+FFFD, so distinct entry byte-strings canonicalize — and therefore hash and verify — identically, contradicting the surrounding claim that the entry's hash is over its bytes as written.
CanonicalFullJSON decodes into Go strings and then marshals the decoded tree at internal/auditpush/canonical.go:126–133. Go's encoding/json replaces invalid UTF-8 and unpaired UTF-16 surrogate escapes with U+FFFD during decoding, so distinct raw spellings such as different unpaired surrogate escapes lose their distinction before hashing. canonicalEntryBytes uses this function, and EntryHash uses canonicalEntryBytes for current ledger formats. The nearby documentation does disclose re-marshalling, making “over its bytes as written” imprecise rather than a literal raw-byte promise, but that wording nuance does not refute the claimed canonicalization collision or identical verification result.
ref f30ac774c20373b9b286a8923f7abdc88fc1219a9e9745ecf53be21338acbc44#R5
Recorded so that "found nothing here" can be told apart from "never looked here".
This package has clearly been through several rounds already, and most of the obvious laundering paths are closed and pinned by tests. The two things I found are both instances of the shape the package's own comments keep naming and keep re-committing: a rule enforced at one door and not the other. Finding 1 is the more serious — round four added "an unsigned entry in a signed chain is a problem", but gated it on `pub != nil`, and `RequireIntactChain` (the single function whose whole purpose is to be called by every door) passes `nil`. So the one check that needs no key is the one check no keyless door performs: a signature can be deleted from a placed entry without changing its hash (`canonicalEntryBytes` deletes the `signature` key before hashing), and `LoadDir`, `AppendLedgerEntry` and `WriteCheckpoint` all then report the chain intact — the checkpoint deleting the evidence outright, which is exactly the laundering `WriteCheckpoint`'s own R1 comment was written to prevent. Finding 2 is a dead custody switch: `ledgerpush.go:142` gates a review entry's scripts and outputs on `e.Bundle.SourcePushed`, and a review entry never has a Bundle, so `corral ledger push --push-source` provably cannot carry a reproduction's script or stdout to a warehouse and records `source_pushed = false` for every review — the fix for a one-door mistake written one door short, in the same commit that says so. Finding 3 is the claim-versus-code half of finding 1: `RequireIntactChain`'s doc says "every door calls it", and the prior loader and the verdict cache — the two readers that change what a LATER audit does — do not. Findings 4 and 5 are smaller and I did not execute them.
The review stands in full. R1 and R2 are directly supported by the supplied reproductions and the corresponding code paths. R3 identifies two integrity-sensitive readers that bypass the shared verifier. R4 finds a genuine referential-integrity gap between adjudication construction and generic placement. R5 correctly identifies a non-injective canonicalization case despite slightly imprecise documentation language about bytes “as written.”