diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a66e8..9afeba5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,12 @@ ### Fixes +- **Fail closed on an unmanaged `CODEX_HOME` in ChatGPT auth mode** (#441) — + The guest wrapper now refuses an explicitly set `CODEX_HOME` when coop's + managed `~/.codex/config.toml` selects keyring storage. This prevents `codex + login` from silently writing a plaintext refresh token to the alternate + directory, including a workspace path that syncs back to the host. + - **Install Codex's complete runtime package** (#442) — Recent Codex releases use a companion `codex-code-mode-host` executable, but coop installed only the raw `codex` binary, causing Code Mode to fail closed at startup. Image diff --git a/docs/codex-integration.md b/docs/codex-integration.md index a8c9198..d90cfbd 100644 --- a/docs/codex-integration.md +++ b/docs/codex-integration.md @@ -127,6 +127,11 @@ Security and billing guardrails in this mode: --env`. - `auth.json` from the host Codex config directory is not copied into the guest. Account tokens are stored in the guest OS credential store instead. +- `CODEX_HOME` cannot redirect Codex around keyring storage in this mode. When + coop's managed config selects the keyring, the guest wrapper refuses any + explicitly set `CODEX_HOME`, preventing Codex from writing account + credentials to an unmanaged `auth.json`; unset `CODEX_HOME` when using + ChatGPT account auth. - `[proxy.openai]` is rejected with `auth = "chatgpt"`, because the proxy path uses an OpenAI API key and would switch Codex back to API billing. diff --git a/docs/trust-model.md b/docs/trust-model.md index 2fd27f9..8c41cf7 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -126,7 +126,10 @@ user `env_forward` entries, and the VM SSH key. The invariants: from the staged set only stops coop *copying* one, it removes nothing), and `coop codex` refuses to launch when the guest config does not actually select the keyring store — otherwise the wrapper would pass through to plain - Codex and write the token in the clear. + Codex and write the token in the clear. The wrapper also fails closed when a + session-level `CODEX_HOME` is set while coop's managed + `~/.codex/config.toml` selects keyring mode; coop does not otherwise stage or + maintain an alternate Codex home. ## SSH boundary diff --git a/scripts/guest/codex-account.sh b/scripts/guest/codex-account.sh index 0d3b35b..eb116e0 100644 --- a/scripts/guest/codex-account.sh +++ b/scripts/guest/codex-account.sh @@ -22,11 +22,16 @@ die() { # credentials from auth.json, where the D-Bus/keyring session is pure overhead # (and its password prompt is an outright regression). Gating on the config the # guest actually has lets every Codex entry point route through this wrapper. +keyring_mode_in() { + local config=$1 + local first_line="" + [ -r "$config" ] \ + && IFS= read -r first_line < "$config" \ + && [ "$first_line" = 'cli_auth_credentials_store = "keyring"' ] +} + keyring_mode() { - [ -r "$CODEX_CONFIG" ] \ - && grep -Eq \ - '^[[:space:]]*cli_auth_credentials_store[[:space:]]*=[[:space:]]*"keyring"' \ - "$CODEX_CONFIG" + keyring_mode_in "$CODEX_CONFIG" } keyring_exists() { @@ -116,6 +121,17 @@ if [ ! -x "$CODEX_BIN" ]; then die "$CODEX_BIN is missing; rebuild the coop image" fi +# coop stages and maintains Codex state only in ~/.codex. In ChatGPT account +# mode, an explicit CODEX_HOME could put auth.json outside coop's cleanup path +# (including under /workspace, which is pulled back to the host). Refuse the +# unsupported override before inspecting its config. coop writes the managed +# credential-store key as the first line, so a same-named nested key cannot +# trigger this guard. +if [ -n "${CODEX_HOME:-}" ] \ + && keyring_mode_in "$HOME/.codex/config.toml"; then + die "CODEX_HOME is set, but coop manages ~/.codex and it selects the keyring credential store; unset CODEX_HOME for Codex ChatGPT account auth" +fi + if ! keyring_mode; then exec "$CODEX_BIN" "$@" fi diff --git a/src/backend.rs b/src/backend.rs index 9cae856..dfe874f 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -1810,8 +1810,8 @@ pub fn codex_keyring_not_configured_message() -> &'static str { /// enabling the mode against a running VM reaches exactly this gap. pub fn ensure_codex_keyring_configured(target: &SshTarget) -> Result<()> { let configured = target.exec_ok(RemoteCommand::new().literal( - "grep -Eq '^[[:space:]]*cli_auth_credentials_store[[:space:]]*=[[:space:]]*\"keyring\"' \ - ~/.codex/config.toml", + "IFS= read -r first_line < ~/.codex/config.toml \ + && [ \"$first_line\" = 'cli_auth_credentials_store = \"keyring\"' ]", )); if configured { return Ok(()); @@ -2771,18 +2771,11 @@ fn stage_codex_files( let TomlValue::Table(root) = &mut config else { bail!("Codex {CODEX_CONFIG_FILE} must deserialize to a TOML table"); }; - if auth.uses_chatgpt_account() { - root.insert( - "cli_auth_credentials_store".to_string(), - TomlValue::String("keyring".to_string()), - ); - } else { - // Explicit, not incidental: the host's own config.toml may set - // this (the user may use keyring storage on the host too), and - // copying it into an `api_key` guest would make the wrapper - // demand a keyring password that mode does not need. - root.remove("cli_auth_credentials_store"); - } + // This setting is coop-owned in both modes. ChatGPT mode prepends the + // managed value during serialization below; API-key mode leaves it + // absent so a host setting cannot make the guest wrapper demand an + // unnecessary keyring password. + root.remove("cli_auth_credentials_store"); } // The Codex CLI records installed marketplaces under `[marketplaces.*]` @@ -2824,12 +2817,15 @@ fn stage_codex_files( ); if should_write_config { - std::fs::write( - staging.path().join(CODEX_CONFIG_FILE), - toml::to_string(&config) - .with_context(|| format!("Failed to serialize Codex {CODEX_CONFIG_FILE}"))?, - ) - .with_context(|| format!("Failed to stage Codex {CODEX_CONFIG_FILE}"))?; + let serialized = toml::to_string(&config) + .with_context(|| format!("Failed to serialize Codex {CODEX_CONFIG_FILE}"))?; + let serialized = if auth.uses_chatgpt_account() { + format!("cli_auth_credentials_store = \"keyring\"\n{serialized}") + } else { + serialized + }; + std::fs::write(staging.path().join(CODEX_CONFIG_FILE), serialized) + .with_context(|| format!("Failed to stage Codex {CODEX_CONFIG_FILE}"))?; } Ok(staging) @@ -4040,9 +4036,15 @@ Filesystem 1M-blocks Used Available Use% Mounted on } #[test] - fn stage_codex_files_chatgpt_mode_writes_config_without_source() { + fn stage_codex_files_chatgpt_mode_writes_keyring_setting_first() { + let src = tempfile::TempDir::new().unwrap(); + std::fs::write( + src.path().join("config.toml"), + "approval_policy = \"never\"\ninstructions = \"\"\"\n[not-a-table]\n\"\"\"\n", + ) + .unwrap(); let staging = stage_codex_files( - None, + Some(src.path()), &std::collections::HashMap::new(), None, false, @@ -4054,7 +4056,16 @@ Filesystem 1M-blocks Used Available Use% Mounted on .unwrap(); let config = std::fs::read_to_string(staging.path().join("config.toml")).unwrap(); - assert_eq!(config.trim(), "cli_auth_credentials_store = \"keyring\""); + assert!( + config.starts_with("cli_auth_credentials_store = \"keyring\"\n"), + "the shell guards require the coop-managed setting first: {config:?}", + ); + let parsed = toml::from_str::(&config).unwrap(); + assert_eq!( + parsed["cli_auth_credentials_store"].as_str(), + Some("keyring"), + ); + assert_eq!(parsed["instructions"].as_str(), Some("[not-a-table]\n")); } #[test] diff --git a/src/guest.rs b/src/guest.rs index 0f2853a..0668466 100644 --- a/src/guest.rs +++ b/src/guest.rs @@ -693,16 +693,30 @@ mod tests { } #[test] - fn codex_account_script_passes_through_without_keyring_mode() { + fn codex_account_script_guards_alternate_home_and_otherwise_passes_through() { // Every Codex entry point routes through the wrapper, so it must be a // transparent exec unless the guest config asks for keyring storage. + // The exception prevents session-level CODEX_HOME from bypassing + // coop's managed keyring config and its plaintext-token cleanup path. assert!( SCRIPT_CODEX_ACCOUNT.contains("cli_auth_credentials_store"), "wrapper should gate on the guest Codex credential-store setting", ); assert!( SCRIPT_CODEX_ACCOUNT - .contains("if ! keyring_mode; then\n exec \"$CODEX_BIN\" \"$@\""), + .contains("[ \"$first_line\" = 'cli_auth_credentials_store = \"keyring\"' ]"), + "credential-store detection should require coop's first-line setting", + ); + assert!( + SCRIPT_CODEX_ACCOUNT.contains( + "if [ -n \"${CODEX_HOME:-}\" ] \\\n && keyring_mode_in \"$HOME/.codex/config.toml\"; then" + ), + "wrapper should reject an explicit CODEX_HOME when coop's config uses the keyring", + ); + assert!( + SCRIPT_CODEX_ACCOUNT.contains( + "unset CODEX_HOME for Codex ChatGPT account auth\"\nfi\n\nif ! keyring_mode; then\n exec \"$CODEX_BIN\" \"$@\"" + ), "wrapper should exec Codex directly when keyring mode is off", ); } diff --git a/tests/integration.sh b/tests/integration.sh index 881199c..b0adf32 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1280,13 +1280,21 @@ test_codex_account_auth_support() { fi done - # The primary guest config may inherit the host's Codex credential-store - # setting. Use an explicit empty config so this assertion isolates the - # wrapper's non-keyring branch from the machine running the suite. - local account_probe_dir="/tmp/coop-codex-account-probe" - if ! guest_exec sh -c 'mkdir -p "$1" && : > "$1/config.toml"' \ - sh "$account_probe_dir"; then + # Give the probe an explicit managed config with no credential-store + # setting. The wrapper checks $HOME/.codex/config.toml before consulting an + # explicit $CODEX_HOME, so both locations must be isolated for this test. + local account_probe_root="/tmp/coop-codex-account-probe" + local account_probe_home="$account_probe_root/home" + local account_probe_codex_home="$account_probe_root/codex-home" + if ! guest_exec sh -c ' + set -eu + rm -rf "$1" + mkdir -p "$1/home/.codex" "$1/codex-home" + : > "$1/home/.codex/config.toml" + : > "$1/codex-home/config.toml" + ' sh "$account_probe_root"; then fail "prepare codex-account probe config" "stderr: $(guest_stderr)" + guest_exec rm -rf "$account_probe_root" || true return fi @@ -1294,7 +1302,8 @@ test_codex_account_auth_support() { # D-Bus session, keyring, or password prompt. A hang here would mean it # tried to unlock a keyring it should have skipped. local version - if version=$(coop_exec env CODEX_HOME="$account_probe_dir" \ + if version=$(coop_exec env HOME="$account_probe_home" \ + CODEX_HOME="$account_probe_codex_home" \ /usr/local/bin/codex-account --version); then pass "codex-account passes through to codex without keyring mode ($version)" else @@ -1302,20 +1311,26 @@ test_codex_account_auth_support() { "stderr: $(guest_stderr)" fi - # Drive the keyring branch without reconfiguring the VM: the wrapper reads - # $CODEX_HOME, so a scratch config selects keyring mode for one call. This - # is the only place the `cli_auth_credentials_store` grep, the D-Bus - # re-exec, the tool guards and the TTY guard actually execute — the - # assertions above all run on the passthrough branch. + # Select keyring mode explicitly in the scratch CODEX_HOME while retaining + # the isolated managed config. This is the only place the + # `cli_auth_credentials_store` check, the D-Bus re-exec, the tool guards and + # the TTY guard actually execute — the assertions above all run on the + # passthrough branch. # # `coop exec` is not a TTY, so the wrapper must refuse rather than block on # a password prompt. A hang here is the failure this asserts against. - guest_exec sh -c 'printf "cli_auth_credentials_store = \"keyring\"\n" \ - > "$1/config.toml"' sh "$account_probe_dir" + if ! guest_exec sh -c 'printf "cli_auth_credentials_store = \"keyring\"\n" \ + > "$1/config.toml"' sh "$account_probe_codex_home"; then + fail "prepare codex-account keyring probe config" \ + "stderr: $(guest_stderr)" + guest_exec rm -rf "$account_probe_root" || true + return + fi # "$1/config.toml"' \ + sh "$alternate_codex_home" + if chatgpt_exec env CODEX_HOME="$alternate_codex_home" \ + /usr/local/bin/codex-account --version; then + fail "chatgpt mode rejects an unmanaged CODEX_HOME" \ + "wrapper honored CODEX_HOME instead of coop's managed config" + elif guest_stderr | grep -q "unset CODEX_HOME"; then + pass "chatgpt mode rejects an unmanaged CODEX_HOME" + else + fail "chatgpt mode rejects an unmanaged CODEX_HOME" \ + "stderr: $(guest_stderr)" + fi + chatgpt_exec rm -rf "$alternate_codex_home" + + # Positive witness for the supported path: the CODEX_HOME guard must not + # reject a normal ChatGPT launch. This non-TTY call should get past that + # guard and reach the existing keyring prompt check. + if chatgpt_exec env -u CODEX_HOME \ + /usr/local/bin/codex-account --version