Repository navigation
feat(min): sign in to GitHub under the published App, kept in the host keychain - #2364
norrietaylor merged 12 commits into
Conversation
One host keychain item holds the sign-in's token and refresh material, with its metadata (account, instance identifier, flow, expiries) kept apart so status reads no secret. A sign-in with no held item or to a different account gets a new instance identifier; the same account keeps it (BEP-002).
min auth login runs the browser PKCE flow under the Minimal GitHub App when the build embeds the client secret, and the device flow under --device or without it, then stores the sign-in in the keychain item. min auth status reports the account and the token expiry, and never the token (BEP-001, BEP-004).
|
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 |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. The only change to existing lines is a reorder of |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The sign-in flows, the keychain item and min auth status follow Gatehouse §6.10. Two paths break the rule that a same-account login keeps the instance identifier (line 839). store_sign_in reads a keychain refusal as no held item and mints a new instance. The Secret Service backend deletes the item before it stores the new one. Both fail open and revoke every running box's references. Fix them with C1 and C2. Add the App-installation notice at first sign-in with C3.
Ruled at 9b5e516 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
A keychain refusal no longer reads as no item held, so it mints no new instance. On Linux the metadata moves to the label and the one attribute is service, so secret-tool store replaces the item in place without a clear first. The search output is zeroized after the label is parsed. min auth login now says on stderr, at a new sign-in, that the token reaches only where the App is installed. The browser callback reads each connection in its own task under a 10 second limit.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to Gatehouse §6.10, the un-enrolled bullet at line 839. This head fixes the defects of the earlier ruling. A refused keychain read now stops the sign-in before the write. The Linux store replaces the item in place. The install note prints at first sign-in. A same-account login keeps the instance identifier, and a login that finds no item gets a new one. Nobody checked the macOS backend on a Mac. Run one min auth login on a Mac before release.
Ruled at 94e52e8 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Revise round 1. The Linux keychain path (secret-tool) is not on this box. So the new label-based metadata and the in-place replace are checked by clippy only, and not against a real Secret Service. This round touched no files outside the layers. The thread on the embedded client secret is judged invalid: the spec records the embedded public client secret as a deliberate decision (BEP-001 and BEP-069 notes, Gatehouse §6.10). The release step that sets MINIMAL_GITHUB_APP_CLIENT_SECRET lives in the release pipeline under .github/, which is outside this task and a protected path. Until it is set, a build takes the device flow. |
|
/revise The native lane is red on 94e52e8: The encoder is right and the test's constant is wrong. In Generated by Claude Code |
The verifier constant read mJ92 where RFC 7636 appendix B reads mB92. The encoder is unchanged.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change since the last PASS corrects one test vector. The PKCE test now uses the code_verifier from RFC 7636 Appendix B, and that verifier hashes to the asserted challenge. The encoder and the assertion did not change. The ruling on 94e52e8 stands for the remainder of the change under §6.10 line 839.
Ruled at e029b5b against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 13 unresolved threads — minimal-arch[bot] crates/bep/src/signin.rs, minimal-arch[bot] crates/bep/src/signin.rs, minimal-arch[bot] crates/minimal/src/cmd/auth.rs:141, minimal-foundry[bot] crates/minimal/src/cmd/auth.rs:747, minimal-foundry[bot] crates/minimal/src/cmd/auth.rs:470, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
Round 2 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
Notes from the agent: The revise note said to fix the one character in the RFC 7636 verifier and change nothing else. I changed only that character (mJ92 to mB92) on line 965 of crates/minimal/src/cmd/auth.rs. Four review threads hold against the code: the device poll aborts on a transient error, the callback page says signed in too early, the keychain errors give no next step, and the secret-tool stdout is not zeroized. I did not fix them this round because the note limits the round to the test. Each one is a small follow-up inside this task's layers. The Cargo.toml thread needs a change to the root Cargo.toml, which is outside the layers. |
|
/revise Round 2 fixed the test. This round takes the four runtime findings that still hold on e029b5b (the earlier revise note scoped them out by mistake; they are in scope now). Arch rules PASS on this head, so change nothing else.
Then reply on each of the four threads with the commit, as before. The two threads on Generated by Claude Code |
There was a problem hiding this comment.
Babysitter review of e029b5b (T12: min auth login and min auth status, the sign-in item in bep::signin). Read as a COMMENT review so it starts no coder round.
What I checked. The item is one keychain entry with the material and the metadata as separate parts; status reads only the metadata. store_sign_in keeps the instance identifier on a same-account login, mints one on a first login or an account switch, and returns before any write on a refused read. The macOS backend writes through security -i on stdin so the secret never reaches a command line, and reads the metadata back after the write. The Secret Service backend stores on the single service attribute with no prior clear and zeroizes the captured search output. The browser flow carries PKCE S256 with a 64-hex verifier, checks the state, serves each callback connection in its own task under a 10 s read limit, and exchanges the code with the client secret. The device flow honours slow_down and the code's expiry. Tokens are validated to the character set every keychain command carries unquoted. The tests cover both flows against a fake GitHub, the keychain-only storage, the instance rules, the refused-read case, the status texts and the RFC 7636 vector. No blocking finding from me.
Findings, none blocking. The four runtime findings that still hold (device poll retries, callback page text, error next steps, zeroizing the remaining copies) are in the round-3 revise. One more of the same class is folded into that revise: post_form keeps the raw token JSON in a String that is not zeroized.
For the epic owner. The two open threads on BUILT_IN_SECRET (HIGH and MEDIUM, embedded App client secret in the shipped binary) are disputed by the coder as a recorded decision, and minimal-arch's PASS ruling on this head names "the embedded public secret as a recorded decision". A security thread stays open until the owner rules. Nothing else holds the PR once round 3 lands and the arch re-passes.
Generated by Claude Code
A damaged item now says to run min auth login to replace it. A token in an unexpected form gets its own error that says nothing was saved. A missing secret-tool or /usr/bin/security names the tool and what to install. Both keychain reads build Material from the captured output and zeroize it, and the macOS write zeroizes its command line.
A network error, a timeout or a 5xx on a device-flow poll no longer ends the sign-in while the code is live. The poll retries on the next interval. The callback page no longer says the sign-in succeeded before the exchange and the keychain write run. Token endpoint replies are zeroized after they parse.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This push conforms to Gatehouse §6.10 line 839. It does not change the earlier PASS. The device poll retries only until the code expires, so the sign-in still fails closed. A token that the item cannot store is refused before the write, so the held sign-in stays the same. The wipes of token copies in memory go further than the design requires, and the design does not forbid them. No conditions.
Ruled at 54c42fb against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 3 of 10 on a /revise note, pushed as
Verify
Notes from the agent: Round 3 addresses the four findings in the /revise note. (1) Device poll retries: commit 54c42fb (thread PRRT_kwDOPdfcxM6uHH43). A network error, a timeout or a 5xx on a poll is retried on the next interval until the deadline. A 4xx whose body carries an OAuth |
|
/revise Round 3 is verified. minimal-arch rules PASS on 54c42fb. This round takes the two runtime findings raised on that head and nothing else.
Reply on both threads with the commit. The two Generated by Claude Code |
valid_account now accepts `_`, so an Enterprise Managed User login (handle_shortcode) signs in and its metadata decodes. On the Secret Service, the metadata moves to a second item under dev.minimal.github-sign-in.meta whose secret is the encoded metadata. min auth status reads it with secret-tool lookup, so no token enters the process. The macOS backend is unchanged.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The Linux layout now keeps the instance identifier only in the META_SERVICE item. The token item holds no identifier. host::write stores the token item first and the metadata item second. If the second store fails on a different-account login, the old instance names the new account's token. Section 6.10 line 839 says the keychain item carries the per-instance identifier and a new account is a new instance that no outstanding reference names. Make the change in C1. Then the change conforms.
Ruled at 4dc1dfd against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 4 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
Notes from the agent: Round 4 addresses the two runtime findings in commit 4dc1dfd. (1) valid_account accepts |
|
/revise minimal-arch ruled CONDITIONS on 4dc1dfd (check run 118386649812): one condition, and the runtime raised the same point on C1, Reply on the two arch threads (signin.rs lines 433 and 671) and the runtime's Generated by Claude Code |
…-github-with-min-auth-and-keep-the-sig
The token item now stores v1 <instance> <token> <refresh token>, and a read returns the instance in Material::instance. A torn Linux write can no longer put a new account's token under an instance that an outstanding reference binds.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare since 4dc1dfd has one PR commit. It puts the instance identifier into the token item, so the item that holds the material names its own instance. Section 6.10 line 839 says the keychain item carries a per-instance identifier and references bind it. A torn Linux write can no longer put a new account's token under an old instance. The rest of the change keeps its earlier status. The change conforms.
Ruled at 0d3334d against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 5 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
Notes from the agent: This round addresses only C1, as the directive asks. encode_secret takes the Metadata and writes |
There was a problem hiding this comment.
Approving for merge: T12 after the person's secret ruling and the arch PASS on 0d3334d. Gate passes on 0d3334d (arch PASS, review approved, lanes green, squash subject lint-clean).
Generated by Claude Code
|
/revise Base merge only. The merge queue built 0d3334d on top of
Generated by Claude Code |
…-github-with-min-auth-and-keep-the-sig
The merge with main carried bep twice in the manifest, and cargo refused the workspace. Keep one line and fold both uses into its comment.
…-github-with-min-auth-and-keep-the-sig
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare since 0d3334d5 merges main twice and removes a duplicate bep line from crates/minimal/Cargo.toml. The sign-in code (signin.rs, auth.rs, cli.rs) has no change. Gatehouse §6.10 line 839 still governs it, and the PASS on 0d3334d5 stands. The change conforms. There are no conditions.
Ruled at b7145f6 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 6 of 10 on a /revise note, pushed as
Verify
Notes from the agent: Base merge round. The merge commit c367d6a with origin/main (767c59f) was already on the branch. It left |
There was a problem hiding this comment.
Own review carried to b7145f6. This head is round 6 on the base-merge directive: two merges of main (767c59f then abbdb31) and one commit, 178726c, that drops the duplicate bep = { path = "../bep" } line from crates/minimal/Cargo.toml (one line remains, at 74, with the min auth comment). The branch's own diff against main is the same 7 files as at 0d3334d. minimal-arch ruled PASS on this head. The runtime review's one finding (LOW, the embedded App secret) is the point the epic owner already ruled on; resolved with that ruling. Approval follows once the KVM and macOS lanes are green.
Generated by Claude Code
There was a problem hiding this comment.
Approving for merge: T12 on the round-6 base-merge head b7145f6 (arch PASS 118448851499, duplicate bep dependency dropped). Gate passes on b7145f6 (arch PASS, review concluded with no open threads, lanes green, squash subject lint-clean).
Generated by Claude Code
T12 — Sign in to GitHub with min auth and keep the sign-in in one keychain item
slice: S0
kind: task
covers: BEP-001, BEP-002, BEP-004
size: L
est: 410 code, 480 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics:
min auth statusprints "Signed in to GitHub as . The token expires at ." or, with expired token and live refresh material, "Signed in to GitHub as . The token renews at the proxy on the next request that uses it.", never a token, pinned by auth_status_reports_expiry_without_tokenobservability: one info log line per sign-in naming the account, the flow and whether the instance is new
layers:
- cli:
- crates/minimal/src/cli.rs:
min auth login [--device],min auth status- crates/minimal/src/cmd/mod.rs: dispatch and module line
- crates/minimal/src/cmd/auth.rs (new): device and PKCE browser flows under the App (client id
Iv23liv6KetAFJp4Eb5K, callbackhttp://127.0.0.1/callback, any port), browser by default; the secret isoption_env!("MINIMAL_GITHUB_APP_CLIENT_SECRET"), and a build without it takes the device flow; status from the item's metadata- crates/minimal/Cargo.toml: the bep crate
- Cargo.lock: the new dependency edge
test: cargo nextest run -p minimal auth_login_completes_device_flow_and_reports_account
test: cargo nextest run -p minimal auth_login_completes_browser_pkce_flow
test: cargo nextest run -p minimal auth_login_without_built_in_secret_takes_device_flow
test: cargo nextest run -p minimal auth_status_reports_expiry_without_token
test: cargo nextest run -p minimal auth_status_reports_renewal_of_expired_sign_in
- item:
- crates/bep/src/signin.rs (new): the one sign-in keychain item and its instance identifier (BEP-002)
- crates/bep/src/lib.rs: module line
test: cargo nextest run -p minimal auth_login_stores_material_in_keychain_only
test: cargo nextest run -p minimal a_sign_in_with_no_held_item_is_a_new_instance
test: cargo nextest run -p minimal re_login_to_the_same_account_keeps_the_instance
produces:
- crates/bep/src/signin.rs
consumes: none
proof:
min auth logincompletes the device flow and names the account, andmin auth statusshows the expiry and no token.depends on:
- blocked by #2258
Proof
min auth login --deviceprints a code to enter at github.com. After the code is entered, it printsSigned in to GitHub as <account>.and keeps the token and refresh token in one host keychain item. In a build with no embedded client secret, the baremin auth logintakes the same device flow.min auth statusprintsSigned in to GitHub as <account>. The token expires at <time>.and prints no token. The tests show this against a local stand-in for GitHub.Notes from the agent: The tests use a local fake GitHub server and an in-memory keychain (bep::signin::MemoryKeychain). No test ran against real GitHub or a real host keychain. The host keychain backend calls
/usr/bin/securityon macOS. On other hosts it callssecret-tool(Secret Service), because the spec leaves the Linux store open. The macOS backend did not run on this Linux box. It needs a check on a Mac: the write throughsecurity -i, and the parse of theicmtcomment attribute that holds the metadata. bep has no Cargo.toml change: the backends use std::process only. minimal depends on bep throughbep = { path = "../bep" }, because the root Cargo.toml (not in layers) has no workspace entry for bep. The PKCE verifier and the state use bep::signin::new_instance_id, and the base64url encoder is local, so minimal has no new crates. The hiddenmin loginverb in cmd/admin.rs is unchanged. docs/reference/cli-min.md does not yet documentmin auth(not in layers). The status line for an expired token copies the wording from the task's diagnostics line, which uses the word "proxy". The status time is printed in UTC.auth_login_stores_material_in_keychain_onlyproves the material goes to the keychain item and not to the metadata. The code writes no file, but the test does not scan the project directory for the token.just clippy-strictandjust ciwere not run.Tests
cargo nextest run -p minimal auth_login_completes_device_flow_and_reports_accountcargo nextest run -p minimal auth_login_completes_browser_pkce_flowcargo nextest run -p minimal auth_login_without_built_in_secret_takes_device_flowcargo nextest run -p minimal auth_status_reports_expiry_without_tokencargo nextest run -p minimal auth_status_reports_renewal_of_expired_sign_incargo nextest run -p minimal auth_login_stores_material_in_keychain_onlycargo nextest run -p minimal a_sign_in_with_no_held_item_is_a_new_instancecargo nextest run -p minimal re_login_to_the_same_account_keeps_the_instanceVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.97.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Closes #2263