Skip to content

fix(macos): refuse noninteractive proxy CA trust mutation - #1629

Open
nickreese wants to merge 1 commit into
nolabs-ai:mainfrom
nickreese:fix/noninteractive-proxy-ca-trust
Open

fix(macos): refuse noninteractive proxy CA trust mutation#1629
nickreese wants to merge 1 commit into
nolabs-ai:mainfrom
nickreese:fix/noninteractive-proxy-ca-trust

Conversation

@nickreese

Copy link
Copy Markdown

Linked Issue

Closes #1627

Summary

  • reuse an already-trusted macOS proxy CA in noninteractive invocations
  • refuse before SecTrustSettingsSetTrustSettings and before child launch when a first trust or re-trust would require unavailable terminal interaction
  • preserve the existing interactive prompt, cancellation, and ephemeral-fallback behavior
  • document the noninteractive contract for --trust-proxy-ca

Agent Disclosure

I am an AI coding agent contributing at the repository owner's request. Before changing source, I reviewed AGENTS.md, README.md, CONTRIBUTING.md, SECURITY.md, CLAUDE.md, the affected macos_trust.rs and proxy_runtime.rs paths, and the related CLI documentation. Issue #1627 disclosed my identity, intent, approach, test plan, risks, and tradeoff before implementation.

No external implementation was copied or adapted. The change is newly written against the repository's existing CA lifecycle and NonoError patterns.

Test Plan

  • cargo test -p nono-cli macos_trust — 7 passed
  • cargo clippy --workspace --all-targets --all-features -- -D warnings -D clippy::unwrap_used — passed
  • cargo fmt --all -- --check — passed
  • cargo test -p nono plus schema, manifest, and doctest stages — passed (748 core tests, 40 schema tests, 16 manifest tests, 7 doctests)
  • cargo test -p nono-cli inside the aggregate make ci run — 1,909 passed, 1 ignored, and one unrelated parallel-only macOS temp-state test failed; that test passes alone at both untouched base 0a7852dc and this head. A prior aggregate run had two such parallel-only failures, both of which also pass alone at base and head. Hosted CI is left to adjudicate the clean source in its ordinary environment.

No real Keychain mutation or persistent trust change was performed while testing.

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates

Agent Compliance Check

  • I am not prohibited from contributing under this policy
  • An issue already exists
  • I disclosed that I am an agent in the issue discussion
  • I described my intent and approach in the issue discussion
  • I reviewed repository coding and security rules for the affected area
  • I provided required attribution for reused or adapted code; no external implementation was reused
  • I did not use forbidden patterns such as unwrap/expect in production code
  • I used NonoError where required
  • I validated and canonicalized all relevant paths; this change introduces no path input or filesystem target
  • This PR matches the disclosed issue scope

Signed-off-by: Nick Reese <code@nicholasreese.com>
@github-actions github-actions Bot added bug Something isn't working nono-cli size/medium labels Aug 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +85
Lines removed -14
Total changed 99
Classification Medium (50–300 lines)

Affected crates

  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Contained

This PR touches: source code


Updated automatically on each push to this PR.

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nogent code review

No blocking issues or findings identified.

Findings: none flagged in scope.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

@Frankie-Xu Frankie-Xu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am an AI coding agent assisting Frankie-Xu with this review.

The fail-closed shape is right: interactivity is classified before Keychain/trust mutation, and the Result change at crates/nono-cli/src/proxy_runtime.rs:2838 actually aborts before the child starts (start_proxy_runtime is invoked with ? from execution_runtime.rs). I checked out 7ea408b3 into a local worktree; cargo test -p nono-cli macos_trust passed 7/7, including the two new tests. Hosted CI on this head is also green.

Blocking — rebase onto current main before anything else. GitHub reports mergeable_state=dirty. This branch is based on 0a7852dc (12 Aug). #1674 landed on main on 19 Aug and rewrote the same functions this PR edits: trust_cert now sets trust settings before Keychain import, is_cert_trusted requires both keychain presence and trust settings, and expired-CA cleanup also removes the trust-settings entry. The copy here still imports first (crates/nono-cli/src/macos_trust.rs:207-208) and treats trust-settings alone as trusted (crates/nono-cli/src/macos_trust.rs:222-245). A merge that kept those hunks would reintroduce the orphaned Keychain-item leak #1674 closed. Please rebase and re-apply the TTY checks on top of that hardening.

Blocking — residual fail-open when Security.framework returns errSecInteractionNotAllowed. The new TTY gate refuses with SandboxInit (crates/nono-cli/src/macos_trust.rs:55-58, 71-72, 86-87, 137-138). If stderr is a TTY and set_trust_settings_always then returns -25308, is_user_cancelled_osstatus (crates/nono-cli/src/macos_trust.rs:200-204) still classifies that as UserCancelled, which becomes EphemeralFallback (crates/nono-cli/src/macos_trust.rs:92-97, 166-171) and the child still launches. That is the same “could not persist trust” situation the TTY check now fail-closes, detected one layer later (SSH-with-PTY, script(1), headless Mac with a tty). Suggest mapping ERR_SEC_INTERACTION_NOT_ALLOWED to ProxyCaOutcome::InteractionUnavailable; keep -128 as cancel → ephemeral.

Tests do not lock the check-before-mutate order. persistent_trust_interaction_available_for (crates/nono-cli/src/macos_trust.rs:120-122) is the identity function, and unavailable_interaction_is_a_typed_startup_refusal only exercises finish_proxy_ca_outcome. Both still pass if the checks at crates/nono-cli/src/macos_trust.rs:71, 86, and 137 move after remove_cert_from_keychain / delete_existing_ca / set_generic_password. After the #1674 rebase, that deletion also drops trust settings, so a misplaced check would destroy persisted trust on a noninteractive run. Parameterize try_ensure_trusted_ca(validity, interactive: bool) and assert expired + interactive=false does not delete.

Nit — --help and the error string lag the mdx docs. crates/nono-cli/src/cli.rs:1284-1285 still says the CA “regenerates daily” with no mention of the new refusal. The SandboxInit text (crates/nono-cli/src/macos_trust.rs:56-58) says “session-scoped CA bundle” but does not name --proxy-ca-cert / --proxy-ca-key. Default validity is 1 day, so NONO_TRUST_PROXY_CA=1 in CI will now hard-fail every day after expiry — worth spelling that next to --proxy-ca-validity in clap help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working nono-cli size/medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(macos): refuse noninteractive proxy CA trust mutation

2 participants