[Auth] Choose which auth storage to use based on config #5792
Conversation
celia-oai
force-pushed
the
dev/cc/set-auth-storage-based-on-config
branch
6 times, most recently
from
ebf396f to
ff93451
Compare
celia-oai
changed the title
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with ��. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| /// keyring: Use an OS-specific keyring service. | ||
| /// file: Use a file in the Codex home directory. | ||
| /// auto (default): Use the OS-specific keyring service if available, otherwise use a file. | ||
| pub cli_auth_credentials_store_mode: AuthCredentialsStoreMode, |
There was a problem hiding this comment.
Why do we need a setting?
There was a problem hiding this comment.
I think we want to keep the setting as file as default for now so it is compatible with current behavior (esp. in headless and sandboxed setups). This config gives users with higher security requirements the ability to config keyring-only mode. I don't expect keyring-only to ever be the default because of the diverse environments Codex is run in, so there will always be environments where keyring-only won't work.
celia-oai
force-pushed
the
dev/cc/auth-keyring-support
branch
5 times, most recently
from
2b06a08 to
72ccf34
Compare
celia-oai
force-pushed
the
dev/cc/set-auth-storage-based-on-config
branch
3 times, most recently
from
1d38545 to
d849e28
Compare
celia-oai
force-pushed
the
dev/cc/set-auth-storage-based-on-config
branch
2 times, most recently
from
c9b7ecb to
e6b55bf
Compare
| - `keyring` – Store credentials in the operating system keyring via the [`keyring` crate](https://crates.io/crates/keyring); the CLI reports an error if secure storage is unavailable. Backends by OS: | ||
| - macOS: macOS Keychain | ||
| - Windows: Windows Credential Manager | ||
| - Linux: DBus‑based Secret Service, the kernel keyutils, or a combination |
There was a problem hiding this comment.
yup, if you open the link of doc it has a section talking about this:
Platforms
This crate provides built-in implementations of the following platform-specific credential stores:Linux: The DBus-based Secret Service, the kernel keyutils, and a combo of the two.
FreeBSD, OpenBSD: The DBus-based Secret Service.
macOS, iOS: The local keychain.
Windows: The Windows Credential Manager.
There was a problem hiding this comment.
actually, let me also turn on the features in Cargo.toml that correspond to this doc
| } | ||
| SlashCommand::Logout => { | ||
| if let Err(e) = codex_core::auth::logout(&self.config.codex_home) { | ||
| if let Err(e) = codex_core::auth::logout( |
There was a problem hiding this comment.
surprised this is not an auth_manager call
There was a problem hiding this comment.
I could maybe do a sweep of tech debt like this at some point as a follow-up to unify auth workflow. Right now it's a bit painful to do the refactor because there are many custom workflows like this.
| error: None, | ||
| sign_in_state: Arc::new(RwLock::new(SignInState::PickMode)), | ||
| codex_home: codex_home.clone(), | ||
| cli_auth_credentials_store_mode, |
There was a problem hiding this comment.
Who else needs this? Would be nice to encapsulate all logic in AuthManager
There was a problem hiding this comment.
Agreed we can refactor this to use AuthManager as a follow up. This is needed at a few places rn unfortunately:
celia-oai
force-pushed
the
dev/cc/set-auth-storage-based-on-config
branch
from
8f241a1 to
8e5f8f2
Compare
celia-oai
force-pushed
the
dev/cc/set-auth-storage-based-on-config
branch
from
6fd7241 to
b752a20
Compare
This PR is a follow-up to #5591. It allows users to choose which auth storage mode they want by using the new
cli_auth_credentials_store_modeconfig.