Repository navigation
feat(bep): add the bep crate with the GitHub module and the sealed envelope - #2346
norrietaylor merged 10 commits into
Conversation
The GitHub module carries its v1 host set at port 443, its identifier, its host-set version and the fixed deny set. A member is sealed to the proxy key (X25519, HKDF-SHA256, ChaCha20-Poly1305) with the box id, host, module, host-set version, mode and expiry as associated data, and the breadth in the plaintext. A failed open names the bound field that differed, never its value. The printed form, minseal1_ plus base64url, matches no GitHub token form, and a network test shows api.github.com answers 401 to it.
|
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 |
|
/arch-review |
|
Add Carrot credits or activate Agent usage billing to use Autofix |
|
Reviewed: no findings; approval
8000
withheld, 13 threads unresolved at APPROVE withheld: 13 unresolved threads — minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs:111, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, …. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none, because the crate is new. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 8000 5 conditions · the full ruling
This PR adds the bep crate in the right place, and the GitHub v1 host set matches §6.10 line 851. The sealed value does not use the envelope that §6.10 line 817 names. It is a custom minseal1_ X25519 format, and the design names a compact JWE gatehouse-sealed-secret+jwe with ECDH-ES P-256 (C1). The bound context omits iat and kid (C2), and the plaintext is not the member list (C3). open() requires exact matches for mode, host-set version and expiry. The design makes mode audit-only, admits the intersection of the host sets, and checks expiry against a clock (C4). Remove the guessed DENY_SET until the escalated design question has an answer (C5).
Ruled at 0c010b4 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter review of 0c010b43 against the T1 plan block (#2258) and its two test lines.
Matches the plan. crates/bep is a new workspace member with module.rs and seal.rs. The GitHub v1 host set is the four GitHub authorities at port 443. The sealed value binds box id, host, module, host-set version, mode and expiry in the AEAD's associated data, and the breadth rides the plaintext (BEP-006). The printed form matches no GitHub token shape, and the network test checks the three credential forms (BEP-027). The task status comment shows both plan test lines at exit 0, and every lane aggregator is green on this head.
Defects.
crates/bep/proptest-regressions/seal.txtis outside the task'slayers:. The PR body still says the PR is a draft for it, and the PR left draft at 08:07Z with the file in place. Remove the file from the PR. Proptest regenerates its seeds locally.- The doc comments in
crates/bep/src/lib.rs(lines 4 to 6) andcrates/bep/src/module.rs(lines 5 to 7) say a member sealed under an old host-set version stops opening. That contradicts the arch ruling's C4. Change them with the C4 change.
The design findings are the arch reviewer's (check run 117956813105, verdict CONDITIONS, C1 to C5). The /revise that follows carries them in substance. I did not compile or run the tests here. The lanes and the task status comment did.
Generated by Claude Code
…-bep-crate-with-the-github-module-and
|
/revise The arch reviewer ruled CONDITIONS on C1. The envelope is a compact JWE ( C2. Add C3. The plaintext is the member list ( C4. C5. Remove the deny set ( Review defect. Keep the Generated by Claude Code |
|
That Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
Follow the arch review of the sealed envelope (C1 to C5):
- The sealed value is a compact JWE with typ
gatehouse-sealed-secret+jwe, ECDH-ES on P-256 and A256GCM, in place of
the X25519 minseal1_ envelope.
- The protected header carries the context, now with iat and kid. open()
refuses a kid that is not this proxy key's fingerprint.
- The plaintext is the JSON member list {members, default_owner}.
- open() compares only box id, host and module, and returns the carried
context. A change to any header field still breaks the tag.
- Remove the deny set until its contents are settled, and the proptest
regressions file that was outside the task's layers.
|
Round 1 of 8 on a review requesting changes, pushed as
Verify Review threads the coder judged:
A sealed member is now a compact JWE (typ gatehouse-sealed-secret+jwe, ECDH-ES on P-256, A256GCM). It fails to open when a request comes from another box, host or module, and when any field of its protected header changes: box id, host, module, host-set version, mode, iat, expiry or kid. api.github.com rejects it with 401 in all three credential forms. Notes from the agent: Revise round 1. The proptest regressions file is deleted. C4 changes the diagnostics contract. open() now names only box id, host or module as the field that differs. A changed host-set version, mode or expiry in the header fails the AEAD tag with NotOpenable, and that error cannot name a field. The expiry check against a clock and the host-set intersection belong to the redemption path, not to this crate. C5 removes the fixed deny set, which the task's module.rs layer note named. A later task must add it when the escalation settles its contents. seal() refuses a Context whose kid is not the recipient key's fingerprint. cargo nextest run -p bep: 5 passed, 1 skipped. I did not run just ci or just clippy-strict. |
Finish the arch review conditions C2 to C4 on the sealed envelope: - open() returns NotOpenable for a kid that is not this proxy key's fingerprint, before it decrypts. OtherKey is gone. - A member carries breadth, exp and its credential. seal() and open() refuse a list that does not hold exactly one member, and open() returns UnknownBreadth for a missing or unknown breadth. - open() returns the carried context with the one member. - Add Context::is_expired: the expiry is a clock check. - The crate and Context docs no longer say a host-set version change stops a member from opening.
|
Round 2 of 8 on a /revise note, pushed as
Verify Review threads the coder judged:
A sealed member is a compact JWE with typ gatehouse-sealed-secret+jwe. It fails to open when any carried header field changes, because the tag fails. A request from another box, host or module gets an error that names that field. api.github.com answers 401 to the sealed value as a Bearer token, as a token credential and as basic auth. Notes from the agent: Commit e604b96 from the previous round already did most of the directive: the JWE form, P-256 ECDH-ES, iat and kid, the JSON member list, the compare on box_id, host and module only, the removal of the deny set, and the removal of proptest-regressions/seal.txt. This round (53a5579) closes the remaining gaps against the directive. open() now returns OpenError::NotOpenable for a kid mismatch before it decrypts, and OtherKey is removed. Member now has breadth, exp and credential. seal() and open() refuse a list that does not hold exactly one member (SealError::MemberCount, OpenError::BadMembers). open() returns OpenError::UnknownBreadth for a missing or unknown breadth. Opened now holds the context, the one member and default_owner. Context::is_expired(now) is added. The lib.rs crate doc and the Context doc no longer say that a change to the host-set version, mode or expiry stops the value from opening. Per the ruling, the diagnostics line yields to Gatehouse §6.10: a named mismatch is reported only for box id, host and module. A change to the other carried fields is reported as NotOpenable. I did not run just clippy-strict. |
open() takes the proxy clock in Binding::now and returns OpenError::Expired when the carried expiry is at or before it. A caller can no longer skip the expiry check. The mode stays audit-only and the host-set version stays an admission check, after Gatehouse §6.10.
|
Round 3 of 8 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A sealed member fails to open when its box id, host or module differs from the request, or when the request's clock is at or past its expiry, and the error names that field with no value. A change to any other bound field (host-set version, mode, issue time) breaks the tag. api.github.com answers 401 to the sealed value as a Bearer token, a token header and basic auth. Notes from the agent: This round: open() now takes the proxy clock in Binding::now and refuses an expired value with OpenError::Expired ("the sealed value is past its expiry"). The mode is not compared, because Gatehouse §6.10 makes it audit-only (arch review C4). The host-set version is not compared, because an old-version member still opens and is admitted per authority by the intersection of the carried and current sets. That admission check belongs to a later task. So the diagnostics line holds for box id, host, module and expiry, and a change to the host-set version or the mode shows as NotOpenable, because the tag fails. The fixed deny set is not in module.rs. The arch review (C5) asked to remove it because its contents do not match §6.10 line 849. It moves to the BEP-013 task. The PR body must say so. |
open checked only the header expiry. A member whose own exp was earlier opened and was returned to the caller. open now refuses it with Expired, and a test covers the case.
|
Round 4 of 8 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A sealed member fails to open when the box id, host or module differs, or when the value or the member is past its expiry, and the error names the field that differed. api.github.com answers 401 to the sealed value when it is presented as a credential. Notes from the agent: This round fixes one finding: open now refuses a member whose own exp is at or before Binding::now, with OpenError::Expired. The seven older threads were fixed in earlier rounds and still hold on this branch. The full bep suite (8 tests) also passes. |
seal() needs only the public key, so the caller must verify the member's signed credential (BEP-005, BEP-064). The module docs also say that the fixed deny set lands with the BEP-013 task.
|
Round 5 of 8 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A sealed member fails to open when the box id, host or module differs, or when it is past its expiry. The error names the field and not its value. The seal is a five-segment compact JWE, and api.github.com answers 401 when it is presented directly. Notes from the agent: This round changes doc comments only. seal.rs now says that the envelope proves no origin, so the caller must verify the member's signed credential (BEP-005, BEP-064). module.rs now says that the fixed deny set is not in this file and lands with the BEP-013 task. That deny set was removed in an earlier round on arch review C5. The PR body still says module.rs holds the deny set, so the body must say that it moves to the BEP-013 task. |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 10 unresolved threads — minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs:97, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
/arch-review |
|
@CodeRabbit review |
|
There was a problem hiding this comment.
arch-review: CONDITIONS · 4 conditions · the full ruling
The head now seals a compact JWE with the Gatehouse typ, and the shape conforms to Gatehouse §6.10 line 817. The wire names do not. Line 817 names the context members sub, node_id and upstream. The head writes box_id, host, module and host_set_version (C1, C2). Section 6.3.2 line 672 gives the modes user and installation, but the head serialises app (C3). Line 817 gives the un-enrolled member as {kind: "signin", breadth: "full", jws}. The head requires credential and exp, so it refuses a member of the design's shape (C4). BEP-027 conforms. One escalation asks what td an un-enrolled envelope carries.
Ruled at d1c16ce against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
/revise address arch review concerns! |
|
That What the runner saidcould not start the coder: Error: runner refused the job: HTTP 400 {"error": "runtime_sha does not match this image", "job_runtime_sha": "c64d7778180a4f8f895883dd56236c62c9807cfc", "image_runtime_sha": "338c982c14bccfc7b365085f5dc4834ba9ddcc67"} Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
The protected header names the box id sub and the host node_id, and
binds one upstream member {module, host_set_version}. The mode value
app becomes installation. A member is {kind, breadth, jws, exp?}: open
refuses an unknown kind and checks exp only when it is present.
|
Round 6 of 8 on a /revise note, pushed as
Verify Review threads the coder judged:
A sealed value is a compact JWE whose protected header carries sub, node_id, upstream {module, host_set_version}, mode, iat, exp and kid. It fails to open when the box id, host or module differs, and the error names that field and never its value. It also fails to open when a carried field is rewritten or the value is past its expiry. api.github.com answers 401 when the value is presented as a credential. Notes from the agent: This round addresses the four new arch threads in crates/bep/src/seal.rs. The other ten threads were fixed in earlier rounds, and this branch still has those fixes. The arch document is not in the spec excerpt, so I could not check the header names sub, node_id and upstream against it. Spec line 161 confirms {kind: "signin", breadth: "full", jws}, and BEP-056 confirms mode = "installation". The full bep test suite passed: 10 tests run, 1 ignored. |
open() refuses a value longer than MAX_SEALED_LEN before it decodes anything. The aes-gcm, getrandom, p256 and zeroize versions move to the workspace dependencies.
|
Round 7 of 8 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A sealed member (a compact JWE with typ gatehouse-sealed-secret+jwe) fails to open with ContextMismatch naming the field when the box id (sub), host (node_id) or module differs. It fails with Expired when the value or member is past its expiry. A changed mode, host-set version, iat or kid fails as NotOpenable, because the protected header is the AEAD's associated data. api.github.com answers 401 when the sealed value is sent as a credential. Notes from the agent: Round 7. Added MAX_SEALED_LEN (16 KiB). open() refuses a longer value as Malformed before it decodes anything, and a new test covers this. The aes-gcm, getrandom, p256 and zeroize pins moved to [workspace.dependencies]. Cargo.lock did not change. Correction to the earlier PR body: open() does not compare mode or host-set version. The C4 finding asked for this, per Gatehouse 6.10. A changed mode or version fails as NotOpenable through the AEAD tag. There is no DENY_SET and no Mode::App. The mode variant is Mode::Installation. |
open() compares the box id, host and module. The other header fields fail through the tag, so the test rewrite uses its own HeaderField.
|
Round 8 of 8 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A sealed member fails to open when its box id, host or module differs from the request, and the error names that field and no value. A change to its mode, host-set version, issue time or expiry breaks the AEAD tag and fails to open. A member at or past its expiry fails as expired. api.github.com answers 401 to the sealed value. Notes from the agent: This round narrows BoundField to BoxId, Host and Module, the only fields open() compares. The variants open() never returns (HostSetVersion, Mode, Iat, Expiry, Kid) are removed. The test header rewrite now uses a test-only HeaderField enum. The mode and host-set version do not bind by comparison, per Gatehouse 6.10 and thread C4: a change to one fails as NotOpenable through the tag. So the diagnostics line's 'host-set version, mode' naming is not met as written. The PR body must say this. The test name sealed_context_binds_box_host_module_version_expiry is the plan's name and is kept. The full bep suite passes (11 passed, 1 ignored). |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 13 unresolved threads — minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs:111, minimal-arch[bot] crates/bep/src/seal.rs, minimal-arch[bot] crates/bep/src/seal.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
/arch-review Generated by Claude Code |
There was a problem hiding this comment.
Babysitter review of fde991ef against the T1 plan block (#2258), its two test lines, and the two arch rulings (check runs 117956813105 and 118110933688).
Matches the plan and the rulings. seal() emits a compact JWE: five segments, the second empty, typ gatehouse-sealed-secret+jwe, alg ECDH-ES in direct mode on P-256, enc A256GCM (crates/bep/src/seal.rs lines 369 to 402). The Concat KDF follows RFC 7518 §4.6.2 with the round counter, the length-prefixed A256GCM, empty apu and apv, and the 256-bit length (lines 525 to 537), so a JOSE library derives the same key. The protected header is the AAD and carries sub, node_id, upstream {module, host_set_version}, mode, iat, exp and kid (lines 588 to 602). open() refuses a foreign kid before it decrypts, compares only box id, host and module, treats expiry as a clock check, and checks a member's own exp when present (lines 418 to 502). The plaintext is the member list with a required kind and a fail-closed breadth (lines 722 to 747). The deny set is gone from module.rs. The out-of-layers proptest regressions file is gone. Both plan test lines ran at exit 0 in round 8, and every lane aggregator is green on this head.
Findings. Neither is a code defect, and neither holds the merge on its own.
rewrite_header(seal.rslines 819 to 841) decodes the header intoserde_json::Valueand re-encodes it.Valuesorts keys, and the header is written in struct order, so the re-encoded AAD differs from the original for every field. TheNotOpenableassertion at line 908 then passes whichever field the test picks, and proves less than its comment says. The binding still holds by construction, because the whole protected header is the AAD. A follow-up can edit the base64 text in place or enableserde_json'spreserve_order. The runtime's review raised this (thread 4314431908) and the thread closed without a change.- The PR body describes the head of round 0:
minseal1_plus base64url,module::DENY_SET, aMode::Appvariant, and "this PR is a draft" for a file that is no longer in the diff. The code at this head has none of them. The runtime's review raised it twice (threads 4315880373 and 4315956795). No coder round owns the body.
Not checked. I did not compile or run the tests. The lanes and the runtime's round comments did.
Generated by Claude Code
|
/arch-review |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The seal conforms to Gatehouse §6.10 at this head. The header uses the §6.10 member names, and AAD binds the whole context. open() compares only box, host and module, and checks the expiry. The member is the un-enrolled {kind: "signin", breadth: "full", jws} form, and the check fails closed on an unknown kind or breadth. One condition remains. The PR body still describes the old minseal1_ envelope, an App mode and a DENY_SET. Rewrite it to match this head, as C1 says.
Ruled at fde991e against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
There was a problem hiding this comment.
Approved on fde991ef by the babysitter on the operator's instruction: every lane aggregator green, minimal-review with no findings, all 23 review threads resolved and re-verified, arch ruling 118194271545 PASS_WITH_CONDITIONS with its one condition (the PR body) now met.
Generated by Claude Code
T1 — Create the bep crate with the GitHub module and the sealed envelope
slice: S0
kind: task
covers: BEP-006, BEP-027
size: M
est: 300 code, 160 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: an envelope that fails to open names the bound field that differed (box id, host, module, host-set version, mode or expiry) in the error the proxy logs, never the value
observability: a failed open names the bound field that differed, which the proxy logs per failed redemption, never the value
layers:
- crate:
- Cargo.toml: adds
crates/bepto the members- Cargo.lock: the new crate and its dependencies
- crates/bep/Cargo.toml (new): the crate, its seal dependencies and
proptestas a dev-dependency- crates/bep/src/lib.rs (new): module lines
- crates/bep/src/module.rs (new): the GitHub module's v1 host set at port 443, identifier, version and the fixed deny set
- crates/bep/src/seal.rs (new): the envelope of BEP-006, in a form GitHub never accepts as a token
test: cargo nextest run -p bep sealed_context_binds_box_host_module_version_expiry
test: cargo nextest run -p bep --run-ignored ignored-only sealed_value_rejected_by_github
produces:
- crates/bep/src/module.rs
- crates/bep/src/seal.rs
consumes: none
proof: A sealed member fails to open when any bound field differs, and
api.github.comrejects it as a credential.Proof
In crates/bep, seal() binds a member list to the box id, host, module, host-set version, mode, issue time, expiry and the proxy key's fingerprint. The sealed value is a compact JWE: direct ECDH-ES on P-256 with A256GCM, typ gatehouse-sealed-secret+jwe, with the context in the protected header under the Gatehouse §6.10 names (sub, node_id, upstream {module, host_set_version}, mode, iat, exp, kid). The header 8000 is the AEAD's associated data. open() refuses a value whose kid is not this proxy key's fingerprint before it decrypts, compares only the box id, host and module with the request and names the one that differs (for example 'the sealed value is bound to a different host'), and refuses a value at or after its expiry, or a member past its own exp, as Expired. A rewrite of any other header field (mode, host-set version, iat) breaks the tag and fails as NotOpenable. The plaintext is the JSON member list {members: [{kind: "signin", breadth: "full", jws, exp?}], default_owner?}, with exactly one member in v1; a missing or unknown kind or breadth is refused. A key from another host does not open the value. api.github.com answers 401 when a box sends the value as a Bearer token, as a 'token' credential or as basic auth.
Notes from the agent: Mode has two variants, User and Installation, after Gatehouse §6.3.2; the un-enrolled host mints only User. The mode and the host-set version are carried and authenticated but not compared: the mode is for the audit record, and the redeemer admits an authority only when it is in both the carried host-set version's set and the current set (Gatehouse §6.10 Module host sets, BEP-021). The task's diagnostics line lists mode and expiry as fields a failed open names; the design makes them a clock check and an audit field, so the plan text needs amending, not the code. module.rs carries no deny set: the fixed deny set lands with the BEP-013 task once the design question in the first arch ruling has an answer. The envelope gives confidentiality and binding, not origin: authority comes from the member's jws, a handle signed under the client's local key that the caller verifies (BEP-005, BEP-064). The crate has unit tests beyond the two plan test lines (the JWE shape and typ, the Gatehouse header names, a forged kid, expiry as a clock check, a member past its own exp, the length cap, and the member-list refusals). cargo clippy -p bep --all-targets and cargo fmt --check are clean in each round's verify step.
Tests
cargo nextest run -p bep sealed_context_binds_box_host_module_version_expirycargo nextest run -p bep --run-ignored ignored-only sealed_value_rejected_by_githubVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2258
Generated by Claude Code