feat(auth): add headless jwt-bearer client (actual mint-token) - #803
Conversation
27951c2 to
21f8500
Compare
Actual Adversarial ReviewKey findings
🛑 APR-001 P1 Parse the issuer before allowing plaintext loopback transportLocation: Evidence and required correctionIssue: The new command signs its assertion before calling the shared transport guard. That guard decides whether HTTP is loopback with string prefixes in Evidence:
Required correction: Parse the issuer as a URL before signing or dispatching. Permit plaintext only when the parsed scheme is HTTP and the parsed host is exactly an intended loopback host or loopback IP address; require HTTPS otherwise. Reject malformed URLs, user-info host confusion, and lookalike hostnames before an assertion can leave the process. Add coverage for the rejected lookalikes and the allowed loopback forms. 🤖 Corrective action — 🛑 APR-001 P1 — Option A: parsed loopback allowlistTradeoff: This preserves HTTP support for local development while making the exception depend on URL semantics rather than a textual prefix. Copy into a coding agent to run. 🧭 ADR option — APR-001: Service-account assertion transport boundary · Policy: JWT-bearer assertions and OAuth credentials may use HTTP only after parsed URL validation establishes an exact loopback destination. — Create · Update Architecture intent🧭 Suggested ADR: Service-account assertion transport boundary — Create · Update
Review metadataImportant One finding remains unresolved at head Compared: upstream |
austinborn
left a comment
There was a problem hiding this comment.
Reviewed the full diff at head 21f85007, focused on assertion forgery, replay, and audience handling.
No blocking findings. Approving.
What holds up
Algorithm confusion is closed at the type level, which is stronger than the usual runtime check. AssertionAlgorithm has exactly two variants, so HS256 and none aren't merely rejected at parse time, they're unrepresentable. No code path can emit a symmetric or unsigned alg.
Replay and lifetime look right. Every call mints a fresh jti from Uuid::new_v4(), exp - iat is clamped to the server's 300-second ceiling with a 60-second default well under it, and iss == sub == service_account_id matches the server's contract.
is_uuid initially looked like a reimplementation of something the already-present uuid crate does. It isn't. Uuid::parse_str accepts any version and variant, while this enforces version 1-5 and variant 8/9/a/b specifically to mirror the server's isUuid, so the client accepts a principal id exactly when the server would. The doc comment says as much. Worth flagging anyway, so nobody "simplifies" it later.
Secret handling matches the module's standard: MintedToken hand-writes a redacting Debug, mint_error surfaces only OAuth error JSON, key-load failures never echo PEM content, and write_token_output keeps stdout to the token alone with all status on stderr.
Non-blocking
1. An explicit --aud combined with a redirected issuer would sign a correctly-audienced assertion and POST it to the wrong host. resolve_audience defaults aud to the resolved issuer, and that default is the safe case: point --issuer at an attacker and the assertion carries aud: https://attacker, which the real server rejects, so what leaks is useless. Passing --aud https://app.actual.ai while ACTUAL_AUTH_URL points somewhere else breaks that coupling. The attacker then holds a valid, real-audience assertion and roughly 60 seconds to replay it for a token carrying the principal's full scope whitelist.
I don't think this is blocking, and I'd rather be explicit about why than leave it implied. It needs a second misconfiguration stacked on the redirect, and anyone who can set ACTUAL_AUTH_URL in an agent's environment can usually read ACTUAL_SERVICE_ACCOUNT_KEY out of that same environment, so the marginal escalation is small. Closing it is cheap, though: warn, or refuse, when an explicit aud is neither the resolved issuer nor <issuer>/api/oauth/token. That's defense in depth for the unattended path, which is where a poisoned env var is most plausible in the first place.
2. resolve_lifetime's comment describes a sentinel that clap never produces. It treats 0 as "the unset sentinel from the default", but assertion_ttl_seconds is default_value_t = 60, so 0 only shows up when someone passes --assertion-ttl-seconds 0 explicitly. That input becomes 60 rather than clamping to 1. Defensible, but it isn't what the comment describes.
3. The key file is read with no permission check. load_key_pem reads whatever --key points at. Given that token_store goes out of its way to write 0600, a warning when a private key is group- or world-readable would round out the story.
Posted by the operator's software factory.
• City:factory-main· Agent:local-core.builder-1
• On behalf of: @austinborn
21f8500 to
7debf81
Compare
wattswolf
left a comment
There was a problem hiding this comment.
Solid, well-tested jwt-bearer client, but the loopback transport check lets a signed assertion go out over plaintext to an attacker-controlled host. Fix the guard before merge.
- The HTTPS bypass keys off
base_url.starts_with("http://localhost")/"http://127.0.0.1", sohttp://localhost.evil.invalid,http://localhost@evil.invalid, andhttp://127.0.0.1.evil.invalidall read as loopback and get plaintext. With--audpointed at the real server, the minted assertion is a replayable bearer sent in the clear —src/auth/oauth.rs:146. Parse the URL and match the exact host (localhost,127.0.0.1,::1), not a prefix. - Same guard is reused by
mint_tokenviabuild_http_client, so this new signed-assertion path inherits the weakness with no local override —src/cli/commands/mint_token.rs:68.
wattswolf
left a comment
There was a problem hiding this comment.
The client implementation looks strong, but I’d hold approval until the integration evidence is complete.
The JWT handling is well designed: RS256/ES256-only signing, bounded claims and lifetime, fresh jti values, redacted debugging, token-only stdout, and substantial unit/E2E coverage. I also checked the #804-then-#803 merge order, and the hardened transport guard is preserved without conflict.
The remaining gaps are integration-related:
-
The current head, 7debf81, predates #804, so there isn’t yet a real post-#804 SHA with green CI.
-
The matching server grant in sprintreview#3267 is still open and conflicting on top of prerequisite #3259.
-
Because the server route has not landed, the complete client/server mint flow could not be verified live.
-
Before approval, I’d like to see #804 land, this PR refreshed onto the resulting main, CI green on the new head, and the #3259/#3267 server rollout confirmed.
One non-blocking hardening suggestion: consider preventing an explicit --aud from differing from --issuer, which would reduce the chance of a valid short-lived assertion being sent to the wrong HTTPS endpoint.
Add `actual mint-token`, a fully-headless RFC 7523 jwt-bearer client: it signs a short-lived service-account assertion with a registered private key (RS256 or ES256) and exchanges it for an access token at the OAuth token endpoint — no browser, no human, no stored long-lived secret. This is the unattended-agent path the existing enrollment commands do not cover. - New `src/auth/jwt_bearer.rs`: build and sign the assertion, mint the token, and the stdout-only-token output contract. - New `src/cli/commands/mint_token.rs`: the headless command handler. - Reuse the existing HTTPS transport guard in `src/auth/oauth.rs`. - Only RS256/ES256 can be emitted; HS*/none are refused. The minted token is redacted in Debug and printed only to stdout; status goes to stderr. Verified with unit and end-to-end tests: a signed assertion verifies under the server's exact validation for both algorithms, forbidden algorithms and malformed inputs fail cleanly, and the real binary mints against a mock token endpoint printing only the token to stdout. Generated by the operator's software factory. On behalf of: @benw5483 Co-Authored-By: Actual Factory Bot <factory-bot@actual.ai.invalid> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Coverage Enforcement gate was red on three files, not the single lib.rs line the first review flagged: lib.rs (the MintToken dispatch arm), auth/jwt_bearer.rs (15 lines), and cli/commands/mint_token.rs (55 lines, essentially the whole exec() body). Root cause: tests/mint_token_cli.rs — the end-to-end client tests that drive exec() and the mint round-trip — was never added to the coverage workflow's --test list, so it never ran under instrumentation. The binary exits via process::exit, which flushes the llvm-cov profile, so these subprocess tests do contribute coverage once they run. Register the test file in coverage.yml, as that file's own reminder requires. That covers exec()'s orchestration. The remaining gaps are branches on their own lines that no end-to-end test reaches, so cover them the way the surrounding code already does — with small, unit-testable seams: - Extract resolve_lifetime() next to resolve_scope/resolve_audience/ resolve_algorithm, and unit-test the ttl==0 default path. - Split unix_seconds(SystemTime) out of now_unix(), so the clock-before-epoch error path is testable with an injected time (the same pattern build_and_sign_assertion already uses for now). Add unit tests for the jwt_bearer.rs error branches that had no coverage: non-UTF-8 and PKCS#1/SEC1/PKCS#8 key-header inference, the is_uuid dash-position and non-hex-digit rejections, the empty-audience guard, and the mint_error fallback for a non-OAuth error body. Add an in-process run() dispatch test for the MintToken arm in lib.rs, matching the sibling per-command dispatch tests. No production behavior changes; the two extractions are pure refactors and the 100% coverage gate is left intact. Generated by the operator's software factory. On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.ai.invalid>
7debf81 to
616a6c6
Compare
Actual first-pass review — Request changesRisk: Moderate · Confidence: High · Commit: SEC1 P-256 keys are recognized as ES256 but cannot be signed by the pinned library, so a common advertised key format deterministically fails. Material risks
Review gaps
Next step: Handle SEC1 explicitly, add a real SEC1 regression test, then rerun CI and the live mint path. A human reviewer owns the final decision. |
`AssertionAlgorithm::infer_from_pem` returned `Es256` for a key whose PEM header said `BEGIN EC PRIVATE KEY`, on the header string alone and with no parser check. That header is SEC1 (RFC 5915), and the pinned jsonwebtoken cannot sign with it: its PEM decoder handles PKCS#1 and PKCS#8 only, and the SEC1 label falls through to `InvalidKeyFormat`. So the inference returned a wrong answer that surfaced later, at signing time, far from its cause. SEC1 is what `openssl ecparam -genkey` writes by default, which makes it a likely shape for a user to arrive with. Reject it at inference instead, naming PKCS#8 and the exact conversion command. The explicit `--alg es256` path skips inference and fails in the key loader, so it gets the same remedy there rather than the generic wrong-key-type message. The guidance rides on a new `Sec1KeyUnsupported` variant that carries its hint, the way `OrgMismatch` already does. That split is load-bearing: the error panel truncates every row to the terminal width, so a conversion command baked into Display is the first thing a user loses. Verified against the real binary — both message and hint now render whole at 80 columns. The RSA branch has the same header-match shape and was checked rather than assumed. It is sound: the decoder maps `BEGIN RSA PRIVATE KEY` to PKCS#1 and `as_rsa_key` returns it directly. A test now pins that asymmetry, so a library change that drops PKCS#1 is caught here instead of at a user's signing call. Tests use genuine SEC1 P-256 and PKCS#1 RSA keys generated at runtime, matching this branch's existing practice of committing no key material. Each mechanism was checked by injection: reverting the inference fix, dropping the loader arm, or moving the command back into the message each reds exactly its own test. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-2 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
The doc comment on mint_token_refuses_a_sec1_ec_key_and_shows_the_conversion_command asserted that its two loop iterations cover the inference and explicit-alg entry points as distinct code paths. They do not, and cannot. infer_from_pem and the EncodingKey loader both answer a SEC1 key with the same sec1_ec_key_error(), so two guards enforce one property and an injection at either site alone is absorbed by the other. Verified by injection rather than reasoned. Restoring the original defect in infer_from_pem (returning Ok(Es256) on the header match) reds only the unit test infer_from_pem_rejects_a_genuine_sec1_ec_key; all six end-to-end tests stay green, including the iteration that omits --alg, because the loader guard then emits identical stderr. Moving the openssl command off the hint and into the message does red the end-to-end test, and reproduces the truncation it exists to catch: the row arrives as "PKCS#8 is required. Co..." with the remedy cut off. The comment now states what the test pins, which is the remedy arriving whole at the terminal, names the injection axis that controls it, which is the shared emitter, and names the unit test that is the real control for the inference fix. Coverage of both entry points is kept and described as coverage. Comment only. The guards, the error text, and every assertion are untouched. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
The PR adds a top-level `actual mint-token` and documented it nowhere, which leaves two stale signposts behind. README's Commands block enumerates the CLI surface and omitted the new command, and the paragraph under it routes anyone reading about "non-interactive (CI / agent) authentication" to docs/AGENT_AUTH.md, a doc that covered only `auth create-token`. This feature's primary audience was being sent to a page that never mentioned it. README gets a Commands line, and the signpost paragraph now names both headless paths rather than only the PAT one. docs/AGENT_AUTH.md gets a "Two headless paths" orientation near the top and a "Service-account keys" section at the end covering the flow end to end: the capture contract, where the key comes from and why no flag accepts key material, the PKCS#8 requirement with the SEC1 refusal and its openssl conversion, the algorithm/lifetime/audience flags, and a CI step that masks the minted token. Its existing "Endpoint" heading is now "Endpoint (create-token)", since the doc describes two endpoints from here on. Documentation only. No behavior change, and `auth create-token` is the exact precedent: it shipped with both a README line and this doc. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
Two descriptions of the same flag disagreed with the code, in opposite directions. resolve_lifetime's doc comment called `0` "the unset sentinel from the default". The flag carries clap's own `default_value_t = 60`, so an omitted flag arrives as `60` and never as `0`; the only way into the `0` arm is to pass `--assertion-ttl-seconds 0` explicitly. The inline comment in resolve_lifetime_uses_default_only_for_zero repeated the same false framing. Both now say what actually reaches the function. The flag's own help said "clamped to 1..=300", which is true of every value except the one a reader is most likely to try: `0` resolves to 60, not 1, because resolve_lifetime intercepts it before build_and_sign_assertion's `.clamp(1, MAX_ASSERTION_LIFETIME_SECONDS)`. The help now says so. The copy moved, not the behavior, since changing what `0` does is a product call rather than a doc fix. Comment and help text only. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
@wattswolf This has been addressed. |
wattswolf
left a comment
There was a problem hiding this comment.
Re-reviewed 05c6516f5ca8. The SEC1 issue is resolved in both inference and explicit ES256 paths with genuine SEC1 regression coverage. Build, tests, lint, coverage, and CodeQL pass. No blocking code findings remain. The only remaining gap is a live client/server mint, which this PR enables.”
Actual first-pass review — Needs additional reviewRisk: Unknown · Confidence: High · Commit: No blocking code defect was identified. The SEC1 failure is now handled at inference and explicit ES256 loading with real-key regression coverage; exact-head CI is green and both server prerequisites are merged. The real client/server mint remains unverified. Material risksNone identified. Review gaps
Next step: Proceed with human review. A human reviewer owns the final decision. Review evidenceFull findings
Open questions and missing evidence
Skipped or inconclusive checks
Coverage and verification ledger
|
Summary
Adds
actual mint-token, a fully-headless RFC 7523 jwt-bearer client: it signs a short-lived service-account assertion with a registered private key and exchanges it for an access token — no browser, no human, no stored long-lived secret. This is the unattended-agent path that the existing enrollment commands (login, andauth create-token/login --devicein #801 / #802) do not cover: those establish a human-delegated session, whereas an autonomous agent on a dev box or in CI holds its own key and self-issues an assertion.The command mirrors the authorization server's jwt-bearer grant exactly:
{ alg: RS256 | ES256, kid, typ: JWT }.HS*andnonecannot be represented, let alone emitted — closing alg-confusion on the client the same way the server closes it.iss == sub == <service-account-id>(validated as a UUID before signing),auddefaulting to the issuer origin, a fresh uniquejtiper call,iat = now, andexpclamped to at most 300s afteriat.POST <issuer>/api/oauth/tokenwithgrant_type=urn:ietf:params:oauth:grant-type:jwt-bearer,assertion=<JWS>, and an optional space-delimitedscope.Headless usage
Every input comes from a flag, an environment variable, or a file:
--key <PATH>(orACTUAL_SERVICE_ACCOUNT_KEY_FILE) reads the key from a file instead. The algorithm is inferred from the key (RSA → RS256, EC P-256 → ES256) unless--algis given.--jsonswaps stdout for the full token response.Output contract: the raw access token is the only thing on stdout, so
TOKEN=$(actual mint-token …)captures exactly the token; all status goes to stderr. This matches the capture contract used by the other auth commands.Scope decisions
A few calls the spec left open, surfaced here for review:
mint-tokencommand, not anauthsubcommand. feat(auth): addactual auth create-tokenfor non-interactive auth #801 and feat(auth): addactual login --devicefor browserless sign-in #802 are still open and both restructure theauthsurface; adding a siblingauthsubcommand now would collide with them. A standalone command keeps this change independent. It can fold underauthonce those land and that surface settles.SEC1 EC keys: rejected with the conversion command, not converted
First-pass review asked that SEC1 keys (
BEGIN EC PRIVATE KEY, RFC 5915) either be supported or be rejected with PKCS#8 guidance. Inferring ES256 from that header alone doesn't fail at inference time, which is the problem: it fails later, at signing, far from its cause.This branch rejects them.
The premise was checked against the pinned
jsonwebtokenrather than taken on report, since a version bump could have changed the answer. Its PEM decoder has arms for PKCS#1 and PKCS#8 only, the SEC1 label falls through toInvalidKeyFormat, andas_ec_private_keyis documented there as "Can only be PKCS8". A test pins that premise, so if a later version does learn SEC1 it goes red, and the rejection doesn't quietly outlive its reason.Converting was the other option the review allowed, and it's turned down here on dependency surface. The crates that can re-encode a key are dev-dependencies on this branch by design, so converting would promote a key re-encoder onto the credential path to fix what's really a wrong error message. Rejecting keeps the shipped binary's key handling at "read the PEM, hand it to the signer".
Both entry points give the guidance, not just the reported one. Omitting
--alggoes through inference; passing--alg es256skips inference and fails at the key loader instead. Either way you get the same error: the message names the encoding and the target, and the hint carries the exactopenssl pkcs8 -topk8 -nocryptcommand. Converting re-encodes the same key pair, so the registered public key, and therefore--kid, isn't touched.flowchart TD K["SEC1 EC private key"] --> A{"--alg given?"} A -->|"no"| I["infer_from_pem"] A -->|"es256"| L["EncodingKey::from_ec_pem"] I -->|"SEC1 header"| E["sec1_ec_key_error()"] L -->|"load fails and key is SEC1"| E E --> S["stderr: message names PKCS#8, hint carries the openssl command"]The RSA branch next door is correct, and it's now pinned either way.
BEGIN RSA PRIVATE KEYis PKCS#1, which the library accepts directly, so answering RS256 on that header alone is safe where the EC one wasn't. That asymmetry is load-bearing and easy to tidy into a bug, so a test signs with a genuine PKCS#1 RSA key rather than leaving the claim in a comment.What the tests control, and what they only cover
Each mechanism was checked by injection. One result is worth stating plainly, because the obvious reading of the end-to-end test is wrong:
infer_from_pemreturnsEs256on the SEC1 header again, restoring the reported defectinfer_from_pem_rejects_a_genuine_sec1_ec_key, and nothing elseopensslcommand moves off the hint and into the messagemint_token_refuses_a_sec1_ec_key_and_shows_the_conversion_commandThe end-to-end test is coverage of the user-visible surface. It isn't the regression control for the inference fix, and it can't be: two guards enforce one property, so restoring the original defect leaves the loader guard emitting identical stderr and every end-to-end test stays green, including the iteration that omits
--alg. The control for that defect is the unit test named above, which drives inference directly.Delivery is the thing it does control. The panel truncates every row to the terminal width, so moving the command into the message reds it with the remedy visibly cut off partway through. Its doc comment now says exactly that, so the next reader doesn't trust it for more than it does.
Live E2E at this head
The review also noted that no live client/server mint was exercised, and that the Live E2E job was skipped at the exact head. It can't be run there. The job is gated on
github.event_name == 'merge_group', so it runs from the merge queue and never on apull_requestevent, and the workflow carries noworkflow_dispatchto force one.Running it wouldn't close that gap on its own either.
scripts/e2e.shhas nomint-tokenor service-account scenario today, so a live mint needs a new scenario plus a service-account key registered in the E2E environment. That's separate work, flagged here for a decision rather than picked up in this change.Documentation
mint-tokenis a new top-level command, so it needed a home in the docs this repo already points readers at.auth create-tokenis the exact precedent: it shipped with a README Commands line and a section indocs/AGENT_AUTH.md, and this follows it.docs/AGENT_AUTH.mdnow names both headless paths rather than only the PAT one. That paragraph was the stale signpost. It was sending this feature's primary audience to a doc that never mentioned the feature.docs/AGENT_AUTH.mdgains a "Two headless paths" orientation near the top, and a "Service-account keys" section covering the flow end to end: the stdout capture contract, the two key-injection environment variables and why no flag accepts key material, the PKCS#8 requirement with the SEC1 refusal and itsopenssl pkcs8 -topk8 -nocryptconversion, the algorithm/lifetime/audience flags, and a CI step that masks the minted token before it can reach a log. Its existing## Endpointheading is now## Endpoint (create-token), since the doc describes two endpoints from here on.Two doc-comment corrections ride along, both the same defect class caught once already this round.
resolve_lifetimedescribed0as "the unset sentinel from the default", but clap'sdefault_value_t = 60means an omitted flag arrives as60, and0only ever arrives explicitly. The flag's own help said "clamped to 1..=300", which holds for every value except0, the one a reader is most likely to try. Both now describe what the code actually does. The copy moved and the behavior did not, because what0should mean is a product call rather than a doc fix.Test plan
Unit tests (
src/auth/jwt_bearer.rs,src/cli/commands/mint_token.rs) and end-to-end binary tests (tests/mint_token_cli.rs):exp), and carriesiss == sub ==UUID, an acceptedaud,exp - iat <= 300, a headeralg ∈ {RS256, ES256}+kid, and a fresh uniquejtiper call.HS256,none,RS512, …) are refused; a non-UUID principal and a wrong-key-for-alg fail cleanly (no panic, no key material in the message).grant_type,assertion,scope); a serverinvalid_grantsurfaces cleanly with no stack trace or secret leakage.--jsonstays machine-parseable; a non-HTTPS non-loopback issuer is refused before anything is sent; a non-UUID principal exits non-zero with an empty stdout.cargo fmt --check,cargo clippy -- -D warnings,cargo test,cargo build --releaseall green.A live end-to-end mint against the running server is not included: the server-side grant is not yet reachable from this repo's test environment (it needs a full app + database stack and a pre-registered key). The client is instead proven to produce a spec-compliant, server-verifiable assertion (the unit tests verify the signature under the server's exact validation) and to capture the token correctly.
Security notes
Debugimpl redacts the token; nothing logs key or token material; the private key is read from a file or env, never a CLI arg.jtiper call and a short (default 60s, ≤300s) lifetime bound the replay/leak window; the server anti-replays on thejti.Test plan for a reviewer
cargo test(unit +tests/mint_token_cli.rs)cargo clippy -- -D warningsandcargo fmt --checkactual mint-token --helpreads clearly for a headless caller