Repository navigation
feat(bep): append one hash-chained audit record per proxy decision - #2361
norrietaylor merged 7 commits into
Conversation
Add the audit module. A record is built from a Decision alone, which has no field for a credential, an injected header or a body (BEP-039, BEP-040). Each record carries previous_hash, the SHA-256 of the previous line, or the all-zero hash first (BEP-048). The log opens append-only with mode 0600 (BEP-041). A failed append logs one warn line that names the path. Add tracing to the bep dependencies for that warn line.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Approved: no findings at Nothing to do. Clobber: none: the file is new. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The record field set matches architecture.md line 501, and the append-only open, the hash chain and the secret exclusion conform. The schema value is wrong. The record writes the bare "min/v1". Line 377 gives every machine object min/v1/<type>, and line 501 puts the audit output under a min/v1/… schema. The chain freezes the bytes of each line, so fix this before the first record exists. Make the edit in C1. Then the change conforms.
Ruled at e5e7e5f against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
Records now carry "min/v1/audit", as other min/v1 documents name their type. A failed append cuts its partial bytes, open cuts a torn tail, and open takes an exclusive lock and refuses a symlink.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Every decision appends one line to the audit log, with schema "min/v1/audit". The line names the box and the upstream authority, chains to the previous line by its SHA-256, and carries no credential, injected header or body. A failed or interrupted append no longer leaves a torn line that the next record joins, and a second writer cannot fork the chain. Notes from the agent: Revise round 1. The architecture.md lines that C1 cites (377, 501) do not exist in this tree, because docs/architecture.md has 251 lines. The min/v1/ convention holds in code (min/v1/error, min/v1/session-policy, min/v1/finalize-install), so I accepted C1. The repair cuts only bytes that never formed a record: the partial tail of a failed append, or a tail without a newline at open. The audit_log_is_opened_append_only test still proves that open leaves a well-formed log byte for byte. The lock uses std File::try_lock (stable since 1.89, and the floor is 1.95), so Cargo.toml needs no new dependency. The symlink refusal is an lstat check before the open. bep has no libc dependency for O_NOFOLLOW, so a narrow race remains. Open does not verify the whole existing chain. That belongs to the reader that |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare closes the open points of the e5e7e5f ruling. Records now carry min/v1/audit, as architecture.md line 377 requires. The writer cuts only bytes that never formed a record, so the log stays append-only (line 501, BEP-041). The exclusive lock refuses a second writer, and the chain stays whole (BEP-048). The four covered requirements conform. No conditions.
Ruled at 7296ce9 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
Check the open file against the path's own entry after the open, so a symlink swapped in before the open is refused. Hash the last line's raw bytes on open, so a non-UTF-8 line chains as it is on disk. State plainly that Decision strings are free text.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Every decision appends one min/v1/audit JSONL record that names the box and the upstream authority and chains to the previous line by its SHA-256. No record carries a credential or a body. The log is opened append-only, refuses a symlink at its path after the open, and chains to the raw bytes of the last line. Notes from the agent: The bep crate has no libc dependency, and crates/bep/Cargo.toml is outside the layers. So the symlink fix does not use O_NOFOLLOW. open() now checks after the open that the open file is a regular file with the same dev and inode as the lstat entry at the path. A swap before or after the open fails that check. One residual: when the path is a dangling symlink, the open with create can create an empty 0600 file at the target before open() refuses. O_NOFOLLOW closes that too, but it needs |
relay_request_body copies exactly Content-Length bytes, or parses the box's chunked framing and writes canonical chunks upstream. Raw chunked bytes never go upstream. forward_requests is the request half of a box connection. It writes one tracing::debug! line per closed connection, with the box id and CloseReason::rule(). The parser also refuses a request with no Host, an absolute-form target that names another authority, and a Connection option that names Content-Length, Transfer-Encoding or Host. Response heads now pass the same framing checks as request heads. The tracing dependency in crates/bep/Cargo.toml (and its Cargo.lock line) is outside this task's layers. The epic owner accepted it as a layer exception, the same line that #2361 adds.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare to 7296ce9 conforms. AuditLog::open now checks the open file and not the path before the open, so it still fails closed and has no race window. The chain now hashes raw line bytes. That does not change a valid record and keeps a corrupt line faithful to the disk (architecture.md line 501). The PASS on 7296ce9 stands for the rest.
Ruled at c1c04e9 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
Check the path with symlink_metadata before the open too, so a symlink target is never created and a FIFO never blocks the open. Cut each record string field to MAX_FIELD_BYTES at a char boundary.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare since c1c04e9 changes only crates/bep/src/audit.rs, and it conforms. Each string field is cut to 512 bytes before the record line is hashed, so the chain stays exact. AuditLog::open now refuses a symlink or a non-regular file before the open, which is fail closed. Architecture line 501 and Gatehouse §6.10 line 836 do not change status. The PASS on c1c04e9 stands for the rest.
Ruled at 974985e against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Every decision appends one hash-chained min/v1/audit record that names the box and the upstream authority. A record never carries a credential or a body. Each string field is now cut to 512 bytes. A symlink or a non-file at the log path is refused before the open. Notes from the agent: Round 3. O_NOFOLLOW and O_NONBLOCK need the libc crate, which bep does not depend on. A new dependency changes Cargo.toml and Cargo.lock, and those are outside the layers. So open now checks symlink_metadata before the open as well, and the check after the open stays. A small race remains: a symlink or FIFO swapped in between the pre-check and the open can still create the link target or block the open. The post-open dev/ino check still refuses a swapped symlink. Adding libc to bep and setting O_NOFOLLOW | O_NONBLOCK closes that race, in a follow-up that is allowed to change Cargo.toml. The field bound does not stop the log growing from many records. That growth is inherent to an append-only log, and rotation belongs elsewhere. |
The head hash now comes from the locked descriptor, not a second open of the path. A field over the bound ends in a marker and the full hash.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Every decision appends one chained min/v1/audit line to the proxy's log. The line names the box and the authority, and never a credential or a body. A field over 512 bytes now shows the cut and the hash of the full value. The head hash comes from the file the proxy holds locked. Notes from the agent: Round 4. On Linux, open reads the log through /proc/self/fd/N of the locked write-only descriptor. The descriptor stays O_WRONLY|O_APPEND, so the append-only test still holds. On other Unix hosts, open reads the path again and refuses a different device or inode. The bep crate has no libc dependency, so O_NOFOLLOW is not used there. bounded keeps a value up to 512 bytes whole. A longer value keeps a prefix, then '…sha256:' and the hex SHA-256 of the full value, all in 512 bytes. just clippy-strict was not run. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head conforms. The one commit since the last PASS changes only crates/bep/src/audit.rs. A cut field now shows the cut and the hash of the full value. The head hash comes from the file the writer holds, not from the path. Both changes keep the records free of credentials and make the hash chain stronger, as architecture.md line 501 requires. There are no conditions.
Ruled at 76af227 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
Open finds the head from the end of the log, so its cost does not grow with the log. Without /proc the reader falls back to the path and the inode check. Created directories get 0700 and an existing log is set to 0600 through the held descriptor.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare from 76af227 changes only how AuditLog::open finds the chain head, plus the file modes. The tail read gives the same head and the same torn-line cut as a whole-file read, so the log stays hash-chained per architecture.md line 501. The /proc fallback still runs the device and inode check, so it fails closed. The 0700 and 0600 modes only narrow access. The earlier PASS stands, and nothing is left to do.
Ruled at a82526f against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Every decision appends one chained min/v1/audit record to the proxy's log. The record names the box and the authority, and never a credential or a body. Open now reads only the end of the log to find the head, and the log and the directories that open creates are private to the proxy. Notes from the agent: This round adds the test audit_log_finds_the_head_past_one_chunk, which covers a last line and a torn tail that both start before the last read chunk. It passed in |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 11 unresolved threads — minimal-arch[bot] crates/bep/src/audit.rs, minimal-arch[bot] crates/bep/src/audit.rs, minimal-foundry[bot] crates/bep/src/audit.rs:475, minimal-foundry[bot] crates/bep/src/audit.rs:422, minimal-foundry[bot] crates/bep/src/audit.rs:382, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
Babysitter review of a82526f (T9: the proxy's audit log, bep::audit). Read as a COMMENT review so it starts no coder round.
What I checked. A record is built from the BEP-039 Decision alone, with act and txn empty on an un-enrolled host, none and module_unmapped as the fixed fill-ins, every free-text field cut to 512 bytes at a char boundary with a marker and the SHA-256 of the full value, and the field order fixed by the struct. The chain is the lowercase hex SHA-256 of the previous line without its newline, zero for the first record, computed over raw bytes so a non-UTF-8 line still chains. open creates missing directories 0700, opens append-only with mode 0600, tightens an existing log to 0600 through the descriptor, refuses a symlink or non-file both before and after the open (the post-open check compares the entry's dev and inode with the open file's), takes an exclusive try_lock, reads the tail through /proc/self/fd/N in 16 KiB chunks with a path fallback that is dev/inode-checked, and cuts a torn last line. append is one write call, a short count is an error, a failed write truncates back to the last complete record or poisons the log. The proptest pins that a credential, an injected header and a body never reach the file and that two requests differing only in those give the same record. The eleven runtime and arch threads all hold on this head; I am resolving them with the line that shows each. No blocking finding from me.
Findings, none blocking
-
openstill follows a symlink swapped in between the pre-check and theOpenOptions::open: the post-open check then refuses, butcreate(true)has already created or opened the link's target for append. Nothing is written to it, so the exposure is an empty 0600 file at the target.O_NOFOLLOWcloses it and needs thelibcdependency T2's branch already adds undercfg(unix); worth taking once T2 merges. -
An existing state directory keeps its mode:
opensets 0700 only on directories it creates, and the log's mode on the file. A wider existing directory lets another local user rename the log away between proxy starts, which the held descriptor does not protect against. T2'spublish.rshas acheck_dirfor the same case; the same check here would make the proxy-only-writer claim hold across restarts, not only within one. -
read_tailprepends each chunk to the tail with a copy, so a torn tail of many chunks costs quadratic copying. Bounded by the log, which the next torn-tail cut shrinks; a nit.
Generated by Claude Code
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of a82526f55352 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
|
/revise Base merge only. main moved to f8431d0 (T2, #2363) and this branch now conflicts with it in Generated by Claude Code |
Keep the bep dependencies and modules from both sides. Cargo regenerated Cargo.lock.
|
Round 6 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify On the merged head, every proxy decision appends one chained min/v1 record that names the box and the authority. The record never holds a credential or a body. The four audit tests pass on top of origin/main. Notes from the agent: Base merge only. The merge commit 3fe7765 resolves the three conflicts. crates/bep/Cargo.toml keeps every dependency from both sides, with one tracing line and the added tracing-subscriber. lib.rs keeps audit, keys, module, publish and seal in alphabetical order. Cargo regenerated Cargo.lock (cargo metadata), and the result is identical to the origin/main lock. audit.rs is unchanged. The other files in the merge (keys.rs, main.rs, publish.rs, seal.rs, justfile) come from origin/main. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head merges main into the branch, and it does not change the audit module. The PASS on a82526f stands. The only conflict was in crates/bep/src/lib.rs, and the resolution keeps pub mod audit; with the new keys and publish modules. The record still matches architecture.md line 501 and Gatehouse §6.10 line 836.
Ruled at 3fe7765 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter review of 3fe7765: this head is a82526f plus one merge of origin/main (f8431d0). The compare against a82526f touches only Cargo.lock, crates/bep/Cargo.toml and crates/bep/src/lib.rs, lib.rs lists audit, keys, module, publish and seal once each, and the manifest keeps both sides' dependencies with one tracing line. audit.rs is unchanged. My review 5579450015 of a82526f stands; no new finding.
Generated by Claude Code
There was a problem hiding this comment.
Babysitter approval of 3fe7765 for the epic owner. Gate: all twelve items pass on this head (minimal-arch PASS, runtime review approved with every thread resolved and re-verified, every lane aggregator and the CLA status green, squash subject lints as Conventional Commits). The head is the reviewed a82526f plus the merge of main.
Generated by Claude Code
…and body (gominimal#2362) * feat(bep): parse request heads strictly and forward them canonically Add a strict HTTP/1.1 head parser that closes on the h2 prior-knowledge preface and on ambiguous framing, records a framing_ambiguous audit event, strips an h2c upgrade, and forwards the canonical re-serialization of the parsed head (BEP-074, BEP-077). Response heads lose every Alt-Svc header (BEP-076). Idle upstream connections are kept per box and authority, so no two boxes share one. * fix(bep): refuse a duplicate Host and cap idle upstream connections The request parser now closes on a head with more than one Host field. The upstream pool keeps at most MAX_IDLE_PER_KEY idle connections per box and authority, and removes a key when its idle list is empty. * fix(bep): drop a connection given back after its box is dropped drop_box now moves the box id's epoch on. A connection checked out before drop_box carries the old epoch, and checkin drops it, so a later box with the same id never gets a dead box's connection. * feat(bep): relay request bodies re-framed and log each close relay_request_body copies exactly Content-Length bytes, or parses the box's chunked framing and writes canonical chunks upstream. Raw chunked bytes never go upstream. forward_requests is the request half of a box connection. It writes one tracing::debug! line per closed connection, with the box id and CloseReason::rule(). The parser also refuses a request with no Host, an absolute-form target that names another authority, and a Connection option that names Content-Length, Transfer-Encoding or Host. Response heads now pass the same framing checks as request heads. The tracing dependency in crates/bep/Cargo.toml (and its Cargo.lock line) is outside this task's layers. The epic owner accepted it as a layer exception, the same line that gominimal#2361 adds. * fix(bep): forget a dropped box id in the upstream pool drop_box now removes the box id's generation instead of moving an epoch on, so the pool keeps no entry for a box that ended. Generations come from one counter and are never reused, so a connection checked out before the drop is still refused when it comes back. * fix(bep): refuse a Host that is not a host and optional port parse_request closes with invalid_host on an empty Host, or one with user info, a path, a comma, a space or a bad port, so the upstream and the policy read one authority. * fix(bep): refuse empty framing elements and stop at a protocol switch An empty Content-Length or Transfer-Encoding list element is refused as framing_ambiguous, so the forwarded value cannot be read differently upstream. forward_requests stops after an Upgrade or CONNECT request and returns the unparsed bytes. The pool caps idle connections per box. * docs(bep): require a concurrent response half for Expect forward_requests forwards the head, then blocks on the body. A box that sends Expect: 100-continue waits for the upstream's 100 Continue, so the caller must relay interim 1xx heads concurrently. Pin that Expect goes upstream unchanged, before the body. * fix(bep): refuse a CONNECT target that disagrees with Host A CONNECT tunnel goes to its authority-form target, so the policy that reads Host and the upstream must see the same authority. * fix(bep): hold a buffered chunk line to MAX_CHUNK_LINE_LEN A chunk line that ended inside the buffer was returned at any length. Check the line length before the line is returned. * fix(bep): flush the forwarded head before the request body A box that sends Expect: 100-continue holds its body until the upstream answers. A head held in a buffered upstream writer stalled both sides. Also document why a response gets the strict request framing rules. --------- Co-authored-by: minimal-foundry[bot] <327719912+minimal-foundry[bot]@users.noreply.github.com>
T9 — Append hash-chained audit records the proxy alone writes
slice: S0
kind: task
covers: BEP-039, BEP-040, BEP-041, BEP-048
size: L
est: 300 code, 240 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: the log is plain JSONL under the proxy's state directory, one record per decision, readable with
min box audit <box>once that task landsobservability: the audit log is the record; one warn log line if an append fails, naming the path
layers:
- audit:
- crates/bep/src/audit.rs (new):
min/v1JSONL records of BEP-039 built from the decision alone, chained per BEP-048, opened append-only- crates/bep/src/lib.rs: module line
test: cargo nextest run -p bep every_decision_appends_one_audit_record
test: cargo nextest run -p bep prop_audit_records_never_contain_secrets
test: cargo nextest run -p bep audit_log_is_opened_append_only
test: cargo nextest run -p bep audit_record_carries_previous_hash
produces:
- crates/bep/src/audit.rs
consumes: none
proof: Every decision appends one chained record naming the box and the authority, never a credential or a body.
depends on:
- blocked by #2258
Proof
bep::audit::AuditLog::append writes one min/v1 JSONL line for each Decision. The line names the subject box and the upstream authority, and carries previous_hash, the SHA-256 of the previous line (all zeros for the first line). A Decision has no field for a credential, an injected header value or a body, so a record cannot carry one.
Notes from the agent: Outside the layers: crates/bep/Cargo.toml adds
tracing.workspace = truefor the observability warn line on a failed append, and Cargo.lock changed by that one line. The record field names are mine where BEP-039 does not fix them: schema, kind, subject, act, txn, upstream, identifier, resource, permission, decision, marker, previous_hash. An unmapped request carriesmodule_unmappedin both resource and permissi 8000 on. A request with no identifier carriesnone. marker is null when there is none. Nothing calls AuditLog yet, because the proxy's request path is a later task. The chain helpers (line_hash, head_hash, Record::from_decision, Record::to_line) are pure over bytes, so the BEP-049 verifier and the Kani harness can reuse them. AuditLog::open reads the whole existing file to find the head hash. That is acceptable until BEP-068 segment rotation bounds the file size. The O_APPEND check in audit_log_is_opened_append_only reads /proc/self/fdinfo and runs on Linux only.Tests
cargo nextest run -p bep every_decision_appends_one_audit_recordcargo nextest run -p bep prop_audit_records_never_contain_secretscargo nextest run -p bep audit_log_is_opened_append_onlycargo nextest run -p bep audit_record_carries_previous_hashVerify
min task run agent-verifypassed ([tasks.verify]).Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/bep/Cargo.tomlCloses #2262
Layer exception
Accepted by the epic owner on 2026-10-19:
tracing.workspace = trueincrates/bep/Cargo.tomlis outside the task's planned layers;audit.rsneeds it.Generated by Claude Code