feat(oauth-capture): persist tokens to macOS Keychain instead of a plaintext file - #1604
Conversation
PR Review SummarySize
Affected crates
Blast radius — ModerateThis PR touches: source code,configuration / policy files Updated automatically on each push to this PR. |
414a855 to
5437b7c
Compare
christine-at-datadog
changed the title
![nogent-nolabs-ai[bot]](https://avatars.githubusercontent.com/in/4165202?s=60&v=4){kind=small}
There was a problem hiding this comment.
nogent code review
1 design concern regarding unintended activation of the tool-sandbox subsystem on macOS.
Findings (not tied to a changed line):
- 🏗️ [LOW · design]
crates/nono-cli/data/nono-profile.schema.json:1— The profile JSON schemacrates/nono-cli/data/nono-profile.schema.jsonis missing the newly introducedoauth_capture_store_backendtop-level property. Because the schema definesadditionalProperties: falseat the top level, any profile utilizing this field to configure the persistence backend will fail schema validation. Addoauth_capture_store_backendwith typestringand enum["auto", "file", "keychain"]to the schema properties.
Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.
| )) | ||
| }; | ||
|
|
||
| let security = command_policies |
There was a problem hiding this comment.
Warning
🏗️ [MEDIUM · design] If keychain_active is true but command_policies is None or does not define a security policy, derive_oauth_capture_security_mediation returns a hard ProfileParse error. Forcing the user to define a dummy security command to satisfy this check has the unintended side-effect of activating the entire tool-sandboxing subsystem (since command_policies.is_active() becomes true), which then blocks all other unconfigured commands. The function should gracefully handle cases where the tool-sandbox is inactive or allow injecting the mediation rule without enabling full tool-sandboxing.
There was a problem hiding this comment.
Updated: Turning on the Keychain backend no longer forces the user to add a security rule that would accidentally switch on the whole command-sandbox and block everything else. Now it skips that step when the sandbox isn't already on (relying on the Keychain's own access control instead).
There was a problem hiding this comment.
nogent code review
1 high-severity bug in the macOS Keychain persistence layer; optional token persistence failure should not abort core sandbox execution.
Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.
| //! locked-keychain handling) is ported from the closed, unmerged | ||
| //! `nolabs-ai/nono#1267` ("oauth-capture routes with phantom-token broker"), | ||
| //! `crates/nono-cli/src/tool-sandbox/broker_store.rs`. That PR was closed in | ||
| //! favor of the simpler flat-file design that shipped as `#1343`, not |
There was a problem hiding this comment.
Caution
🐛 [HIGH · bug] The binary-path/staleness check in load_persisted_tokens has a major logical flaw and causes fatal sandbox startup crashes in headless/SSH environments. To read the saved nono_path inside the generic-password envelope and compare it, the code first calls load_in_process which uses find_generic_password. Because the custom SecAccess ACL restricts silent decryption to only the binary path/signature used at save time, securityd will trigger an ACL check. In headless/SSH sessions, macOS cannot present the GUI prompt and immediately returns errSecInteractionNotAllowed (-25308). This error is propagated up as fatal, preventing the entire sandbox (nono-proxy / supervisor) from launching. In GUI sessions, it forces an intrusive prompt, only to delete the item immediately after the user approves. Change the load path to gracefully handle decryption/access errors (such as -25308 and -25293 / errSecAuthFailed) by treating the cache as empty (returning Ok(HashMap::new())) and logging a warning, instead of failing closed and aborting proxy/sandbox execution.
There was a problem hiding this comment.
Updated to resolve this bug: locked/inaccessible-keychain statuses on the load path now degrade to an empty store with a warning instead of propagating a fatal error, so a headless/SSH launch no longer aborts sandbox startup. The write path still fails closed.
| return Err(missing()); | ||
| } | ||
|
|
||
| let rule = InterceptRuleConfig { |
There was a problem hiding this comment.
Warning
🔒 [MEDIUM · security] The subprocess mediation rule (Layer 1) for security can be easily bypassed by the sandboxed child, allowing it to trigger a social-engineering system prompt to read the OAuth token store. The intercept rule only matches when argv contains both "-a" and "oauth_capture_store". However, a child can call security find-generic-password -s nono -w (omitting "-a" and "oauth_capture_store" entirely). Since the service name is nono, this query still resolves to the same generic password item. Because the contains rule does not match, the mediation layer is bypassed and a Keychain dialog is presented to the user. Fix this by making the intercept rule match any security find-generic-password or security delete-generic-password invocation targeting either the service nono (-s nono) or the account oauth_capture_store (-a oauth_capture_store).
There was a problem hiding this comment.
Updated this part: Mediation now installs a respond-empty intercept rule per selector: account (-a oauth_capture_store) and service (-s nono). security find-generic-password -s nono -w (which omits -a but still resolves the item by service name) cannot fall through to the ACL prompt.
|
Also worth handling here: |
…intext file On macOS, persist the OAuth-capture phantom-token map to an ACL-restricted macOS Keychain item (service `nono`, account `oauth_capture_store`) instead of the plaintext `~/.local/state/nono/oauth-capture/providers.json`. The file backend (`persist.rs`) is unchanged and remains the backend on Linux and other platforms. The Keychain item is written via raw security-framework FFI with a `SecAccess` ACL listing only the running nono binary, and stamps the binary path for staleness detection. Also in this change: - Locked/unwritable keychain fails closed (no in-memory fallback); the load and save paths return ProxyError::Keystore with an actionable message. Docs and the user-facing message describe the fail-closed behaviour accurately. - New managed, profile-wide scalar `oauth_capture_store_backend` (auto|file|keychain, default auto) lets operators force the plaintext file backend on macOS (e.g. headless/SSH hosts). Threaded Profile -> PreparedSandbox -> ProxyLaunchOptions -> ProxyConfig. - When the Keychain backend is active, nono-cli derives a `security` command mediation that returns empty output for reads of the oauth_capture_store account, so enabling capture can never leave the item readable by a bare `security find-generic-password` from the sandbox. The account name is a single shared constant so the ACL entry and the deny rule cannot drift. A mediated command needs a session sandbox, so derivation fails closed with an actionable error if the profile enables the Keychain backend without one (opt out with oauth_capture_store_backend = "file"). - Docs: 'Configuring the persistence backend' section in the OAuth capture manual-QA doc with keychain/file examples and the fail-closed error strings. Signed-off-by: christine.le <christine.le@datadoghq.com>
…ence - Skip security-mediation synthesis when tool-sandboxing isn't already active, instead of forcing the whole command_policies subsystem on. - Zeroize plaintext token copies held by encode_tokens during persistence instead of leaving them on the heap. - Canonicalize current_exe() before using it for Keychain ACL/identity checks so symlinked invocations resolve consistently. - Add oauth_capture_store_backend to the profile JSON schema so profiles setting it pass validation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merging main brought in test_schema_top_level_profile_matches_rust_model (added in nolabs-ai#1607), whose hardcoded expected top-level /properties list predates this branch's oauth_capture_store_backend field. The field is a legitimate cross-platform profile scalar (file backend everywhere; auto resolves to Keychain on macOS), so add it to the snapshot's expected list rather than gating it by platform. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nono mediation bypass Addresses two nogent review findings on the Keychain persistence layer. HIGH (keychain_persist.rs): the load-path staleness check reads the entry via find_generic_password, which hits the item's nono-only SecAccess ACL. In headless/SSH sessions macOS cannot present the ACL prompt and returns errSecInteractionNotAllowed (-25308) / errSecAuthFailed (-25293); the old code propagated that as fatal, crashing proxy/sandbox startup on every launch. OAuth-capture persistence is an optional cache, so the load path now degrades to an empty store and warns (via a pure, unit-tested classify_load_status helper) instead of failing closed. The write path (save_with_nono_acl) still fails closed and never falls back to plaintext. MEDIUM (command_policy.rs): the subprocess `security` mediation matched only argv containing both `-a` and `oauth_capture_store`, so a child could bypass it with `security find-generic-password -s nono -w` (the item also resolves by its service name) and trigger the ACL's Allow/Deny dialog. Mediation now installs one respond-empty rule per selector — account (`-a oauth_capture_store`) and service (`-s nono`) — so reads and deletes reaching the item by either selector are neutralised. Rules stay idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
14c1658 to
66dbe73
Compare
Signed-off-by: christine.le <christine.le@datadoghq.com>
# Conflicts: # crates/nono-proxy/src/oauth_capture/mod.rs
|
hi @christine-at-datadog , just one more thing noted, any existing plaintext providers.json files are not migrated or removed Existing plaintext providers.json files are not migrated or removed - would it make sense to clean these up or you thinking it should be manual? |
3 participants
Closes #1573
Summary
On macOS, persist the OAuth-capture phantom-token map to an ACL-restricted macOS Keychain item instead of the plaintext file at
~/.local/state/nono/oauth-capture/providers.json. The file backend (persist.rs) is unchanged and remains the backend on Linux and other platforms.The Keychain item stores the same JSON shape the file backend uses (
encode_tokens/decode_tokens), under servicenono/ accountoauth_capture_store.nono writes the entry with a
SecAccessACL trusting only the running nono binary — intrinsic to nono's write path (save_with_nono_acl) and not adjustable from a profile — so any other process'ssecurity find-generic-passwordread ofoauth_capture_storecannot silently succeed and instead triggers a visible macOS Allow/Deny prompt.Configuring the backend
The backend is a managed, profile-wide scalar
oauth_capture_store_backend(autodefault → Keychain on macOS, file elsewhere;fileforces the plaintextproviders.jsonon every platform). When the Keychain backend is active, nono-cli derives asecuritymediation that refuses subprocess reads of theoauth_capture_storeaccount, failing closed if the profile has nosecuritysession sandbox.See
docs/cli/development/oauth-capture-manual-qa.mdx→ "Configuring the persistence backend" for keychain vsproviders.jsonprofile examples and the fail-closed error output.Data flow: Keychain vs
providers.jsonBoth backends share
encode_tokens/decode_tokens(same JSON shape, sameZeroizingscrubbing) and differ only in where the encoded bytes land;oauth_capture_store_backendselects the branch.flowchart TD A["Real OAuth token captured by proxy"] --> B["encode_tokens() — shared<br/>JSON payload, Zeroizing-scrubbed"] B --> C{"oauth_capture_store_backend"} C -->|"file — all platforms; auto off-macOS"| D[("~/.local/state/nono/oauth-capture/<br/>providers.json (0600)")] C -->|"keychain — macOS; auto or keychain"| E["save_with_nono_acl():<br/>SecItemAdd with SecAccess ACL<br/>trusting only the running nono binary"] E --> F[("macOS login Keychain<br/>svce=nono acct=oauth_capture_store")]This PR was prepared with the assistance of an AI agent.
Agent Compliance Check