feat(credential-broker): add macos keychain credential broker#1034
feat(credential-broker): add macos keychain credential broker#1034lukehinds wants to merge 2 commits into
Conversation
Introduces a new system for brokering macOS Keychain access for sandboxed applications. - This feature allows `nono` profiles to define granular permissions for `security` CLI interactions. - A new `credential-helper` internal subcommand processes credential requests via supervisor IPC. - On macOS, a `security` shim is installed in `PATH` when `NONO_EXPERIMENTAL_KEYCHAIN_BROKER` is set, redirecting keychain operations through `nono`. - Profiles can now specify `credential_access` grants, including `provider`, `classes`, `operations`, `services`, `service_prefixes`, and `accounts` to control access. Signed-off-by: Luke Hinds <lukehinds@gmail.com>
PR Review SummarySize
Affected crates
Blast radius — ContainedThis PR touches: source code Updated automatically on each push to this PR. |
There was a problem hiding this comment.
Code Review
This pull request introduces a supervisor-brokered credential helper for macOS Keychain access, adding a new credential-helper subcommand, a credential_broker module to parse and authorize macOS security CLI arguments, and support for creating a security PATH shim to intercept keychain requests inside the supervised sandbox. The code review feedback highlights several important improvements: writing raw bytes directly to stdout instead of using String::from_utf8 and println! to avoid restricting secrets to UTF-8 and appending unwanted newlines; using a symlink or hard link instead of copying the entire nono executable to reduce I/O overhead; checking if stdin is a TTY before calling read_to_string to prevent indefinite blocking; correctly handling passwords starting with a dash for the -w flag; and adding support for the -g flag in security find-generic-password to improve compatibility with existing scripts.
| "-w" => { | ||
| if matches!(args.get(i + 1), Some(value) if !value.starts_with('-')) { | ||
| i += 1; | ||
| flags.secret = Some(args[i].as_bytes().to_vec()); | ||
| } else { | ||
| flags.print_password = true; | ||
| } |
There was a problem hiding this comment.
In security add-generic-password, the -w flag always requires a value (the password to add). However, the generic parse_security_flags parser treats -w as a boolean flag if the next argument starts with - (e.g., if the password itself starts with a dash). This will cause passwords starting with a dash to be parsed incorrectly.
Consider passing a context or boolean parameter to parse_security_flags indicating whether -w requires a value, or separate the parsing logic for add and find commands.
| "-i" => return Err("security -i must be the top-level invocation".to_string()), | ||
| other => return Err(format!("unsupported security flag: {other}")), |
There was a problem hiding this comment.
The standard macOS security find-generic-password command historically uses the -g flag to display/print the password. Many existing scripts rely on -g rather than -w.
Since -g is currently unhandled, any invocation using -g will fail with an unsupported security flag error. Consider adding support for -g (mapping it to print_password = true) to improve compatibility with existing scripts.
"-g" => {
flags.print_password = true;
}
"-i" => return Err("security -i must be the top-level invocation".to_string()),
other => return Err(format!("unsupported security flag: {other}")),|
Still need some work, but pleased to see no dreaded red labels from gemmy. |
…ling The `SecretBytes` type (an alias for `Zeroizing<Vec<u8>>`) is now used for all secret material passed through the supervisor IPC and handled by the credential broker. This change ensures that sensitive credential data is securely wiped from memory when the `SecretBytes` object is dropped, reducing the risk of accidental secret leakage. Additionally, the `nono credential` command now writes raw binary secret data directly to stdout, supporting non-UTF8 secrets and preventing potential issues with string conversions. Signed-off-by: Luke Hinds <lukehinds@gmail.com>
|
Ran some tests as we discussed in Discord. See: https://github.com/TBeijen/nono-pocs/blob/main/broker-test/RESULT-20260531-0820CEST.txt (and the TL;DR:
In its current form could work, by allowing only access to claude credentials (via cli shim) while blocking access to Keychain in general. As long as Claude doesn't start using I have no SSO account for Claude to actually test its way of storing those credentials. (🤖 / 👨): No originally Claude believed claude used keytar to access Keychain in a similar way to gh, based on it diggin through itself using |
|
If we phrase the goal as:
Then there might be some pointers in thise OpenShell issue which tackles the same problem: NVIDIA/OpenShell#620 |
Introduces a new system for brokering macOS Keychain access, that allows
nonoprofiles to define granular permissions forsecurityCLI interactions. This brings in a newcredential-helperinternal subcommand processes credential requests via supervisor IPC. On macOS, asecurityshim is installed inPATHwhenNONO_EXPERIMENTAL_KEYCHAIN_BROKERis set, redirecting keychain operations throughnono. Profiles can now specifycredential_accessgrants, includingprovider,classes,operations,services,service_prefixes, andaccountsto control access.