From af3d79872f463a115dd82f3ffdd750c1ea724b91 Mon Sep 17 00:00:00 2001 From: hbrodin <90325907+hbrodin@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:42:22 +0200 Subject: [PATCH 1/4] fix: keep terminal Codex auth on the unlocked keyring session --- .github/workflows/ci.yml | 3 + docs/codex-integration.md | 9 ++ docs/testing.md | 16 +++ scripts/guest/codex-account.sh | 5 +- tests/test-codex-account.py | 209 +++++++++++++++++++++++++++++++++ 5 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 tests/test-codex-account.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4fccc47..45cac1c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,6 +56,9 @@ jobs: - name: Integration probe regression tests run: python3 tests/test-integration-probes.py + - name: Codex account wrapper regression tests + run: python3 tests/test-codex-account.py + - name: Integration test — proxy reverse forwarding run: ./tests/integration-proxy-forward.sh diff --git a/docs/codex-integration.md b/docs/codex-integration.md index 2285122..616e03e 100644 --- a/docs/codex-integration.md +++ b/docs/codex-integration.md @@ -16,6 +16,15 @@ unlocks GNOME Keyring, and then runs the real Codex binary. In keyring mode each launch gets a fresh private D-Bus session, so the wrapper asks for the guest keyring password every time before Codex starts. +The wrapper also supplies `-c 'cli_auth_credentials_store="keyring"'`. +In Codex 0.153.0 and 0.154.0, this override prevents implicit reuse of a desktop +app-server whose D-Bus session may have an unusable keyring. Terminal sign-in +then uses the session unlocked by the wrapper, including through `codex-yolo`. +Caller arguments follow this default and retain their precedence; explicitly +selecting a remote app-server still selects that server and its auth session. +API-key mode remains a passthrough. This daemon-selection behavior is +version-dependent and should be rechecked when updating Codex. + The in-guest `codex-yolo` shortcut routes through the same wrapper, so it works in either auth mode. Running the bare `codex` binary from `coop shell` does not: it has no D-Bus session, and `keyring` credential storage has no diff --git a/docs/testing.md b/docs/testing.md index 4183658..a5c34f0 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -54,6 +54,22 @@ Linux CI runs them. The full VM suite additionally checks these probes against real guests. A host FORWARD policy other than ACCEPT still causes an explicit skip of the routed guest-isolation probe, since it would mask the coop rule. +Run `python3 tests/test-codex-account.py` for the account wrapper's argument, +login/logout, API-key passthrough, and `codex-yolo` regressions (also in Linux +CI). To additionally test implicit daemon reuse with a real Linux Codex binary: + +```bash +COOP_TEST_CODEX="$(command -v codex)" python3 tests/test-codex-account.py +``` + +This requires `dbus-run-session`, `gnome-keyring-daemon`, `secret-tool`, and +`strace`. It uses temporary homes and disposable keyring passwords, starts a +real app-server on a separate unusable keyring session, and observes terminal +socket connections. It checks that sign-in is reached without reusing that +server and that removing the wrapper override restores reuse. No account login +or real tokens are needed. Run it when upgrading Codex: daemon selection is +version-dependent. This opt-in test does not replace either VM backend gate. + The full Codex update tests install native release `0.153.0` before running `codex update` as the guest user, and require the installed version to change. They compare the actual `config.toml` contents across host updates, self-updates, diff --git a/scripts/guest/codex-account.sh b/scripts/guest/codex-account.sh index eb116e0..167f26b 100644 --- a/scripts/guest/codex-account.sh +++ b/scripts/guest/codex-account.sh @@ -171,6 +171,9 @@ else export COOP_CODEX_ACCOUNT_UNLOCKED=1 fi -exec "$CODEX_BIN" "$@" +# Codex 0.154.0 can reuse a desktop daemon on another D-Bus session when +# there are no explicit config overrides. Keep terminal auth on the keyring +# we just unlocked. Prepend the default so caller overrides retain precedence. +exec "$CODEX_BIN" -c 'cli_auth_credentials_store="keyring"' "$@" CODEXACCOUNTEOF chmod 755 /usr/local/bin/codex-account diff --git a/tests/test-codex-account.py b/tests/test-codex-account.py new file mode 100644 index 0000000..39c1022 --- /dev/null +++ b/tests/test-codex-account.py @@ -0,0 +1,209 @@ +#!/usr/bin/env python3 +"""Wrapper regressions; set COOP_TEST_CODEX to also test a real Linux Codex CLI.""" +import json +import os +from pathlib import Path +import pty +import select +import shutil +import signal +import struct +import subprocess +import sys +import tempfile +import time +import unittest +import fcntl +import termios + +ROOT = Path(__file__).resolve().parent.parent +SOURCE = (ROOT / 'scripts/guest/codex-account.sh').read_text() +WRAPPER = SOURCE.split("<<'CODEXACCOUNTEOF'\n", 1)[1].split('\nCODEXACCOUNTEOF', 1)[0] +OVERRIDE = ['-c', 'cli_auth_credentials_store="keyring"'] + + +def executable(path, source): + path.write_text(source) + path.chmod(0o755) + + +def isolated_env(root): + # Do not let host credentials or launch overrides affect the fixture. + env = {k: v for k, v in os.environ.items() + if not k.startswith(('CODEX_', 'COOP_CODEX_', 'OPENAI_', 'XDG_', 'DBUS_', 'GNOME_KEYRING'))} + env.update(HOME=str(root), XDG_DATA_HOME=str(root / 'data'), + XDG_RUNTIME_DIR=str(root / 'run'), TERM='xterm-256color') + (root / '.codex').mkdir() + (root / 'run').mkdir(mode=0o700) + return env + + +def stop(process): + # dbus-run-session may exit before Codex; waiting only for the launcher + # races children still writing into the temporary home. + for sig in [signal.SIGTERM, signal.SIGKILL]: + try: + os.killpg(process.pid, sig) + except ProcessLookupError: + process.wait(timeout=5) + return + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + process.poll() + try: + os.killpg(process.pid, 0) + except ProcessLookupError: + process.wait(timeout=5) + return + time.sleep(0.05) + raise RuntimeError(f'fixture process group {process.pid} did not exit') + + +class WrapperTests(unittest.TestCase): + def test_cleanup_waits_for_launcher_children(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + child = root / 'child.py' + child.write_text(''' +import signal, sys, time +from pathlib import Path +def finish(*args): + time.sleep(0.2) + Path(sys.argv[1], 'finished').touch() + sys.exit(0) +signal.signal(signal.SIGTERM, finish) +Path(sys.argv[1], 'ready').touch() +time.sleep(60) +''') + parent = subprocess.Popen([sys.executable, '-c', ''' +import subprocess, sys, time +subprocess.Popen([sys.executable, sys.argv[1], sys.argv[2]]) +time.sleep(60) +''', str(child), str(root)], start_new_session=True) + try: + deadline = time.monotonic() + 5 + while not (root / 'ready').exists() and time.monotonic() < deadline: + time.sleep(0.01) + self.assertTrue((root / 'ready').exists()) + finally: + stop(parent) + self.assertTrue((root / 'finished').exists(), 'cleanup returned before child exit') + + def test_arguments_and_passthrough(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + env = isolated_env(root) + binary = root / 'codex' + executable(binary, '#!/usr/bin/env python3\nimport json,sys\nprint(json.dumps(sys.argv[1:]))\nsys.exit(23)\n') + wrapper = root / 'codex-account' + executable(wrapper, WRAPPER.replace('/usr/local/bin/codex', str(binary))) + for tool in ['secret-tool', 'gnome-keyring-daemon']: + executable(root / tool, '#!/bin/sh\nexit 0\n') + env['PATH'] = str(root) + ':' + env['PATH'] + env.update(COOP_CODEX_ACCOUNT_DBUS='1', COOP_CODEX_ACCOUNT_UNLOCKED='1') + config = root / '.codex/config.toml' + for keyring in [False, True]: + config.write_text('cli_auth_credentials_store = "keyring"\n' if keyring else '') + for args in [[], ['login', '--device-auth'], ['logout'], + ['--', 'a prompt with spaces; $(false)'], + ['-c', 'model="example"', '--model', 'explicit'], + ['-c', 'cli_auth_credentials_store="file"'], + ['--remote', 'unix:///explicit.sock']]: + with self.subTest(keyring=keyring, args=args): + result = subprocess.run([str(wrapper), *args], env=env, + capture_output=True, text=True, timeout=10) + self.assertEqual(result.returncode, 23, result.stderr) + self.assertEqual(json.loads(result.stdout), (OVERRIDE if keyring else []) + args) + # Exercise the actual provisioned yolo command, including its bypass flag. + lima = (ROOT / 'src/lima.rs').read_text() + yolo = lima.split("cat > /usr/local/bin/codex-yolo <<'YOLOEOF'\n", 1)[1].split('\nYOLOEOF', 1)[0] + shortcut = root / 'codex-yolo' + executable(shortcut, yolo.replace('/usr/local/bin/codex-account', str(wrapper))) + result = subprocess.run([str(shortcut), 'hello world'], env=env, + capture_output=True, text=True, timeout=10) + self.assertEqual(result.returncode, 23, result.stderr) + self.assertEqual(json.loads(result.stdout), OVERRIDE + + ['--dangerously-bypass-approvals-and-sandbox', 'hello world']) + + +@unittest.skipUnless(os.environ.get('COOP_TEST_CODEX'), 'set COOP_TEST_CODEX for real daemon regression') +class RealDaemonTests(unittest.TestCase): + def test_terminal_avoids_daemon_with_unusable_keyring(self): + binary = str(Path(os.environ['COOP_TEST_CODEX']).resolve()) + for tool in ['dbus-run-session', 'gnome-keyring-daemon', 'secret-tool', 'strace']: + self.assertIsNotNone(shutil.which(tool), f'missing prerequisite: {tool}') + print(subprocess.check_output([binary, '--version'], text=True).strip(), flush=True) + with tempfile.TemporaryDirectory(prefix='c481-') as directory: + root = Path(directory) + env = isolated_env(root) + # Disable unrelated plugin downloads without a CLI override, which would + # itself prevent the control launch from reusing the daemon. + (root / '.codex/config.toml').write_text( + 'cli_auth_credentials_store = "keyring"\n[features]\nplugins = false\n') + wrapper = root / 'codex-account' + executable(wrapper, WRAPPER.replace('/usr/local/bin/codex', binary)) + socket = root / '.codex/app-server-control/app-server-control.sock' + # The existing server gets a different bus and an empty keyring directory. + daemon_env = {**env, 'XDG_DATA_HOME': str(root / 'desktop-data')} + with (root / 'daemon.log').open('w') as log: + daemon = subprocess.Popen(['dbus-run-session', '--', 'bash', '-c', ''' + printf '%s' "$DBUS_SESSION_BUS_ADDRESS" > "$HOME/desktop-bus" + exec "$1" app-server --listen unix:// + ''', 'bash', binary], env=daemon_env, stdout=log, stderr=log, start_new_session=True) + try: + deadline = time.monotonic() + 20 + while not socket.exists() and daemon.poll() is None and time.monotonic() < deadline: + time.sleep(0.05) + self.assertTrue(socket.exists(), (root / 'daemon.log').read_text()) + bad_env = {**daemon_env, 'DBUS_SESSION_BUS_ADDRESS': (root / 'desktop-bus').read_text()} + probe = subprocess.run(['timeout', '5', 'secret-tool', 'store', '--label=probe', + 'service', 'coop-regression'], input='disposable', text=True, + capture_output=True, env=bad_env, timeout=10) + self.assertNotEqual(probe.returncode, 0, 'desktop keyring unexpectedly writable') + # Positive witness: removing only the fix must connect to that daemon. + mutant = root / 'codex-account-mutant' + executable(mutant, wrapper.read_text().replace( + "-c 'cli_auth_credentials_store=\"keyring\"' ", '')) + old_trace = self.launch(root, env, mutant, 'old') + self.assertIn(str(socket), old_trace, 'control never reused the existing daemon') + new_trace = self.launch(root, env, wrapper, 'new') + self.assertNotIn(str(socket), new_trace, 'terminal reused the desktop daemon') + self.assertIsNone(daemon.poll(), 'test must leave the existing server running') + self.assertFalse((root / '.codex/auth.json').exists()) + finally: + stop(daemon) + + def launch(self, root, env, wrapper, label): + trace = root / (label + '.trace') + master, slave = pty.openpty() + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack('HHHH', 40, 120, 0, 0)) + process = subprocess.Popen(['strace', '-f', '-e', 'connect', '-s', '256', '-o', str(trace), + str(wrapper)], env=env, cwd=root, stdin=slave, stdout=slave, + stderr=slave, start_new_session=True) + os.close(slave) + output = b'' + deadline = time.monotonic() + 30 + try: + while time.monotonic() < deadline and process.poll() is None: + if not select.select([master], [], [], 0.1)[0]: + continue + try: + chunk = os.read(master, 65536) + except OSError: + break + output += chunk + if b'\x1b[6n' in chunk: + os.write(master, b'\x1b[1;1R') + if b'keyring password: ' in chunk: + os.write(master, b'coop-test-password\n') + if b'Sign in with ChatGPT' in output: + break + self.assertIn(b'Sign in with ChatGPT', output, output.decode(errors='replace')) + return trace.read_text() + finally: + stop(process) + os.close(master) + + +if __name__ == '__main__': + unittest.main() From 864f7174274aa61f603b7dd911b7aa43f6b8db2f Mon Sep 17 00:00:00 2001 From: hbrodin <90325907+hbrodin@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:23:19 +0200 Subject: [PATCH 2/4] feat: share guest keyring for Codex desktop authentication --- .github/workflows/ci.yml | 5 +- docs/ARCHITECTURE.md | 1 + docs/codex-integration.md | 109 ++-- docs/commands.md | 20 + docs/configuration.md | 3 +- .../issue-480-desktop-auth-implementation.md | 272 ++++++++ .../design/issue-480-desktop-auth-research.md | 521 +++++++++++++++ docs/images-and-profiles.md | 5 +- docs/index.md | 3 + docs/testing.md | 26 +- scripts/guest/codex-account.sh | 128 +--- scripts/guest/codex-keyring-migrate.sh | 21 + scripts/guest/codex-keyring-pam.c | 113 ++++ scripts/guest/codex-keyring-setup.sh | 31 + scripts/guest/codex-keyring.py | 435 +++++++++++++ src/backend.rs | 15 +- src/commands/codex.rs | 51 ++ src/commands/mod.rs | 2 + src/guest.rs | 176 +---- src/lib.rs | 25 + src/lima.rs | 1 + src/setup.rs | 1 + src/ssh.rs | 16 + tests/codex-keyring-pam-probe.c | 44 ++ tests/integration.sh | 20 +- tests/test-codex-account.py | 5 +- tests/test-codex-desktop-prototype.py | 615 ++++++++++++++++++ tests/test-codex-keyring-systemd.py | 288 ++++++++ tests/test-codex-keyring.py | 406 ++++++++++++ 29 files changed, 3003 insertions(+), 355 deletions(-) create mode 100644 docs/design/issue-480-desktop-auth-implementation.md create mode 100644 docs/design/issue-480-desktop-auth-research.md create mode 100644 scripts/guest/codex-keyring-migrate.sh create mode 100644 scripts/guest/codex-keyring-pam.c create mode 100644 scripts/guest/codex-keyring-setup.sh create mode 100644 scripts/guest/codex-keyring.py create mode 100644 src/commands/codex.rs create mode 100644 tests/codex-keyring-pam-probe.c create mode 100644 tests/test-codex-desktop-prototype.py create mode 100644 tests/test-codex-keyring-systemd.py create mode 100644 tests/test-codex-keyring.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 45cac1c..97ea073 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,7 +48,7 @@ jobs: run: cargo test --workspace - name: Install network test tools - run: sudo apt-get update && sudo apt-get install -y iproute2 iptables iputils-ping util-linux openssh-client openssh-server + run: sudo apt-get update && sudo apt-get install -y iproute2 iptables iputils-ping util-linux openssh-client openssh-server python3-dbus - name: Release preflight regression tests run: python3 tests/test-preflight-release.py @@ -59,6 +59,9 @@ jobs: - name: Codex account wrapper regression tests run: python3 tests/test-codex-account.py + - name: Codex shared keyring readiness tests + run: /usr/bin/python3 tests/test-codex-keyring.py + - name: Integration test — proxy reverse forwarding run: ./tests/integration-proxy-forward.sh diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4ca6132..30525a1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -132,6 +132,7 @@ The `commands/` submodules own the domains: `lifecycle.rs` (up/start/shell/ exec/stop/destroy/status/list/resize/commit/restore), `quickstart.rs`, `devcontainer.rs`, `profiles.rs` (+ images), `agent.rs` (`coop agent update`), `model.rs` (`coop model`), `proxy.rs` (`coop proxy`), `github.rs`, +`codex.rs` (shared guest keyring installation and `coop codex-unlock`), `admin.rs` (init/validate/uninstall), and `json.rs` (machine-readable `--json` output types). `commands/mod.rs` re-exports the dispatch surface and holds cross-domain helpers diff --git a/docs/codex-integration.md b/docs/codex-integration.md index 616e03e..26f633b 100644 --- a/docs/codex-integration.md +++ b/docs/codex-integration.md @@ -10,31 +10,55 @@ coop codex [instance-name] [-- extra-args...] This SSHes into the guest and runs the `codex` CLI. By default coop passes `--dangerously-bypass-approvals-and-sandbox`, so Codex runs without its sandbox or approval prompts — parity with how `coop claude` runs unrestricted. The VM is the isolation boundary, so Codex's own sandbox is redundant; it also does not work in the guest, which lacks a functioning bubblewrap, so leaving it enabled makes every shell command Codex runs fail. -When `[codex] auth = "chatgpt"` is enabled, `coop codex` launches a small -guest wrapper (`/usr/local/bin/codex-account`) that starts a D-Bus session, -unlocks GNOME Keyring, and then runs the real Codex binary. In keyring mode -each launch gets a fresh private D-Bus session, so the wrapper asks for the -guest keyring password every time before Codex starts. - -The wrapper also supplies `-c 'cli_auth_credentials_store="keyring"'`. -In Codex 0.153.0 and 0.154.0, this override prevents implicit reuse of a desktop -app-server whose D-Bus session may have an unusable keyring. Terminal sign-in -then uses the session unlocked by the wrapper, including through `codex-yolo`. -Caller arguments follow this default and retain their precedence; explicitly -selecting a remote app-server still selects that server and its auth session. -API-key mode remains a passthrough. This daemon-selection behavior is -version-dependent and should be rechecked when updating Codex. - -The in-guest `codex-yolo` shortcut routes through the same wrapper, so it works -in either auth mode. Running the bare `codex` binary from `coop shell` does -not: it has no D-Bus session, and `keyring` credential storage has no -`auth.json` fallback, so Codex will not find its credentials. Inside the guest, -run `codex-account` (or `codex-yolo`) instead of `codex`. The wrapper is a -transparent passthrough unless the guest `~/.codex/config.toml` asks for -keyring storage, so it is safe to use in either mode. It gates on the guest -file rather than on coop's `auth` setting, which is what keeps `codex-yolo` -working from inside the guest; coop keeps that file in step when you switch -modes, rewriting it on the next `coop start` to drop the keyring setting. +With `[codex] auth = "chatgpt"`, the guest wrapper (`codex-account`) verifies +and unlocks the guest user's shared GNOME Keyring service before launching +Codex. The systemd user bus and keyring survive closing the terminal; a VM +restart locks the keyring again. Nested launches reuse the unlocked service. + +The wrapper supplies `-c 'cli_auth_credentials_store="keyring"'` to keep the +terminal app-server independent of the desktop app-server (#481). Both read +one guest credential store. Caller arguments retain their precedence, +including an explicitly selected remote app-server. API-key mode passes through. + +Use `codex-account` or `codex-yolo` inside the guest for automatic readiness +checks. After unlock, ordinary SSH sessions can also run bare `codex` against +the standard user bus. The wrapper gates on the managed guest configuration; +coop updates that configuration when you change authentication modes and start +the VM again. + +### Desktop over SSH + +Desktop authentication is implemented for validation with GNOME Keyring 46.1 +and native Codex 0.154.0. Actual desktop UI connection and real-account OAuth +validation remain release gates; see the [implementation record](design/issue-480-desktop-auth-implementation.md). + +```bash +coop ssh-config my-project +ssh coop-my-project codex --version +coop codex-unlock my-project +``` + +Then enable the SSH host in the desktop app's Settings → Connections, select +`/workspace`, and complete Codex login inside the guest. Desktop sign-in, SSH +access, the keyring password and guest account login are separate steps. Close +the unlock terminal and reconnect to verify that credentials remain available. + +On an existing VM, the first `codex-unlock` installs shared service support in +place. Stop and start that VM before running unlock again. This retires old +private keyring daemons and updaters together. Installation preserves the Codex +home and encrypted credentials. Multiple running keyring daemons must be +resolved before migration so their cached credential histories are not silently +selected at reboot. Conflicting keyring files, unsupported formats +and plaintext stores require explicit migration; coop never selects a history +or replaces them automatically. Back up the files inside the guest before +resolving conflicts or reauthenticating. + +After a keyring crash or locked-to-unlocked transition, rerun `codex-unlock` +and reconnect the desktop. Recovery retires the desktop server through Codex's +native `daemon stop`; the desktop owns its next startup and updater. Closing or +locking the keyring cannot erase credentials already cached in running clients. +Concurrent OAuth refresh and logout across clients require real-account testing. +Desktop SSH does not use coop's API-key/proxy secret forwarding. To keep Codex's sandbox and approval prompts for a single session, pass `--ask`. coop then launches `codex` with no bypass flag, so Codex applies its normal defaults: @@ -120,14 +144,13 @@ agent session, so there is nothing to sandbox — and no `--ask` is needed.) A fresh VM has no keyring, so the first prompt is *choosing* a password, not entering one. The wrapper says so and asks for confirmation. That password encrypts the Codex account credentials at rest inside the guest and is -requested again on later launches; it is unrelated to your ChatGPT or host +requested again after a VM restart or keyring lock; it is unrelated to your ChatGPT or host credentials. Because it is per-guest, `coop destroy` discards it along with the cached login. -The prompt needs a terminal. `coop codex` provides one. Anything that runs -the wrapper without one — invoking `codex-account` yourself through -`coop exec`, or a `post_start` script — fails with a clear message rather than -hanging. +Unlocking needs a terminal, supplied by `coop codex` and `coop codex-unlock`. +Noninteractive launches work after successful unlock in the current service +generation; otherwise they fail with guidance to run an interactive unlock. Security and billing guardrails in this mode: @@ -150,28 +173,10 @@ own state in it — installed marketplaces and plugins, and the `[projects.*]` workspace-trust records — is read back and preserved across the rewrite, so you are not re-approving workspace trust after each restart. -Images built before this support existed need a rebuild: - -```bash -coop setup --rebuild -``` - -A rebuild only changes the golden image. An existing VM keeps its own guest -disk across `coop stop` / `coop start`, so it will not pick up the new guest -packages. Swap the rebuilt image in without losing the instance: - -```bash -coop restore my-project --image default --reprovision -``` - -[`--reprovision`](commands.md#--reprovision) keeps the instance's name, index, -IP, and workspace association, accepts a running instance, and leaves it -running. It provisions the replaced disk as a first boot, so `/workspace` is -restored and the agent plugins are reinstalled — a plain `restore` here would -leave both empty, because the base image carries neither. Both reprovisioning -and destroying/recreating replace the guest disk. Save -guest-only work first (for example with `coop pull`); the replacement also -discards any guest keyring and cached account login. +For existing guests, use `coop codex-unlock ` to install shared keyring +support in place, then stop and start the VM. To include support in future +VMs, rebuild the golden image with `coop setup --rebuild`. Rebuilding an image +does not change an existing guest disk. ### GitHub auth diff --git a/docs/commands.md b/docs/commands.md index 3a15e66..43c4be6 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -359,6 +359,26 @@ coop codex my-project -- --model gpt-5 coop codex my-project -- login --device-auth ``` +### `codex-unlock` + +```text +coop codex-unlock [NAME] +``` + +Unlock the guest's shared encrypted keyring for ChatGPT account authentication. +Requires `[codex] auth = "chatgpt"` and a running VM with the managed guest +configuration. First use confirms a nonempty password; later unlocks reuse the +same store. The keyring stays unlocked after SSH logout and locks at VM restart. + +Existing guests receive support files and packages in place on the first call; +stop and start the VM before retrying. Invalid, plaintext, unsupported or +conflicting stores are preserved and refused. Successful recovery retires the +native desktop server's cached authentication; reconnect the desktop afterward. +See [desktop SSH setup](codex-integration.md#desktop-over-ssh) for validation +status and the account login steps. + +`coop codex unlock` still launches Codex in a VM named `unlock`. + ### `exec` Run a command in the VM and print its output. No PTY is allocated and stdin is not forwarded; use `shell` for interactive work. diff --git a/docs/configuration.md b/docs/configuration.md index b2b97e4..64afb23 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -355,7 +355,8 @@ config means no proxy — credentials are forwarded into the guest exactly as before. Every golden image installs the Secret Service packages this mode needs -(`dbus-user-session`, `gnome-keyring`, `libsecret-tools`) regardless of the +(`dbus-user-session`, `gnome-keyring`, `libpam-gnome-keyring`, +`libpam0g-dev`, `libpam-systemd`, `python3`, `python3-dbus`, `libsecret-tools`) regardless of the `auth` setting, because the image is built once and reused across configs — gating them would let a later `auth = "chatgpt"` edit meet an image that cannot serve it. diff --git a/docs/design/issue-480-desktop-auth-implementation.md b/docs/design/issue-480-desktop-auth-implementation.md new file mode 100644 index 0000000..8bf1382 --- /dev/null +++ b/docs/design/issue-480-desktop-auth-implementation.md @@ -0,0 +1,272 @@ +# Issue #480: desktop guest authentication implementation guide + +Status: implementation branch for validation, based on PR #482. Desktop UI, +real-account OAuth, and both VM lifecycle gates remain required before release. +Updated: 2026-09-15. Evidence baseline: Ubuntu 24.04, systemd 255, +GNOME Keyring 46.1, native Codex 0.154.0. + +This is the implementation direction for #480. The +[research record](issue-480-desktop-auth-research.md) contains reproductions, +upstream sources, prototype commands, and the adversarial review findings. +Use this guide for decisions and that record for supporting evidence. + +## Outcome and scope + +A user unlocks their guest keyring, connects the Codex desktop app over SSH, +closes the unlock terminal, and can reconnect without unlocking again. A VM +restart requires another unlock. Both desktop and terminal use the same guest +credentials while retaining separate Codex app-servers. + +Implement ChatGPT guest authentication for both Linux/Firecracker and +macOS/Lima. Both backends run Linux guests. API-key/proxy forwarding is a +separate design; ordinary desktop SSH bypasses coop's session secret injection. + +## Architecture and ownership + +| Component | Owner and contract | +| --- | --- | +| User D-Bus | systemd user manager; stable `/run/user//bus`. | +| Secret Service | One packaged GNOME Keyring service/socket per guest UID; one persistent encrypted login store. | +| Lifetime after logout | Enable user lingering and explicit headless service startup. | +| Interactive unlock | Dedicated guest PAM helper; password exists only during the operation. | +| Desktop app-server and updater | Codex's native daemon interfaces own their lifecycle. | +| Readiness and recovery | coop's unlock operation checks authentication and performs native recovery while it runs. | +| Terminal app-server | Remains independent, preserving #481's explicit configuration override. | + +Use the ordinary SSH login environment supplied by PAM/systemd. Never read a +terminal process's environment or introduce a separate desktop bus. Configure +the packaged keyring service for `default.target`; Ubuntu's graphical install +target alone is insufficient on a headless guest. + +Replace the terminal wrapper's private keyring sessions with access to this +service. Preserve its explicit launch argument: + +```text +-c 'cli_auth_credentials_store="keyring"' +``` + +Do not create multiple keyring daemons writing the same files: this reproduced +stale reads and resurrection of deleted credentials after an unrelated write. +Do not introduce separate desktop credentials, a custom OAuth broker, private +GNOME unlock protocol code, or a competing app-server PID manager. + +## Unlock operation + +The implemented command is `coop codex-unlock `. The existing +`coop codex unlock` spelling continues to select a VM named `unlock`. + +Recovery uses native `daemon stop` to retire cached server authentication. +Coop does not call `start`, `restart`, or `bootstrap`: the desktop owns its next +bootstrap and updater. This avoids introducing failed-start children that +cannot be rolled back conditionally through native 0.154.0 interfaces. A pidfd +observes termination of the server present before stop; coop never signals a +PID or edits native process records. Concurrent native bootstrap still requires +an acceptance test with the actual desktop. + +1. Check managed ChatGPT mode and the existing `~/.codex` keyring policy. + Preserve guards against plaintext fallback and unmanaged `CODEX_HOME`. +2. Acquire a bounded per-user operation lock. Start or verify the systemd user + bus and keyring service. Derive paths from the guest user's runtime directory. +3. Classify the existing store **before** prompting, probing writes, or taking + an already-unlocked shortcut. Apply the adoption rules below. +4. For genuinely new storage, prompt without echo for a nonempty password and + confirmation. For a locked existing encrypted collection, prompt to unlock. +5. Use a dedicated PAM service and `pam_gnome_keyring.so` to create/unlock the + running daemon through its control socket. Omit `auto_start`: PAM does not + own daemon lifetime. Do not use standalone `gnome-keyring-daemon --unlock` + against the running service; it can create competing processes. +6. Resolve `ReadAlias("default")`; verify it names the intended persistent login + collection and that the collection is unlocked. Do not redirect an unrelated + default or accept a session-only collection. +7. Write a uniquely named disposable non-secret item, read it back, and delete + it. Report cleanup failures. An exit-zero or PAM-success result alone is + insufficient. +8. Perform required native server recovery, then record the successfully + recovered service generation. Recheck the generation before reporting ready. + +Pass the password only over a guest TTY/private pipe, never argv, environment, +files, or logs. Implement cancellation, bounded noninteractive operations, and +secret-memory cleanup. The existing test PAM client is not a production helper. + +The prototype PAM stack collects the token with +`pam_exec.so expose_authtok /usr/bin/true`, followed by the GNOME module. +Verify the chosen production stack and package dependencies. Ubuntu's PAM +package runs `pam-auth-update` and adds a global password-management hook by +default; provisioning must explicitly account for that side effect. + +## Storage adoption and encryption + +An unlocked, writable login collection may store plaintext. Never infer +encryption from its name, permissions, `Locked` property, or successful writes. + +| Existing state | Required action | +| --- | --- | +| No store and no conflicting live collection | Allow first-use creation with a confirmed nonempty password; verify format afterward. | +| Supported encrypted-format candidate | Require the fresh daemon to load the intended collection, then unlock through PAM. | +| Plaintext or unsupported format | Refuse before PAM or writes; explain explicit migration and reauthentication. | +| Existing file that daemon cannot load | Refuse as invalid storage; do not treat it as first use or overwrite it. | +| Encrypted candidate that cannot unlock | Preserve it; report unlock failure without asserting an incorrect password. | + +GNOME 46.1's binary prefix and version fields identify an encrypted-format +candidate, not its integrity. A valid prefix can survive truncation or +ciphertext damage. The fresh daemon's parser and successful unlock provide +additional evidence. A wrong password and damaged ciphertext can produce the +same unlock failure. + +Initial adoption requires a fresh service generation; cached state in an +already-unlocked daemon cannot validate a changed backing file. The implementation accepts GNOME Keyring 46.1 and refuses other versions until +their format/adoption behavior is tested. Do not +claim that a header check is a general integrity validator. Do not silently +convert, delete, or replace existing plaintext/unknown stores. + +## Recovery, concurrency, and cleanup + +The first implementation uses **user-triggered recovery**. Systemd keeps the +keyring alive. No coop process remains after unlock to retire a native server +automatically when the keyring fails. The recovery instruction is to rerun +unlock and reconnect the desktop. Running clients may retain cached credentials +until they restart; locking the keyring cannot erase those copies. + +Record only non-secret runtime state: boot ID, D-Bus server ID, and the +keyring's unique bus owner. Compare this generation with the last successfully +recovered generation. A missing/changed record, or a locked-to-unlocked +transition, requires native server recovery even if another client already +unlocked the replacement service. Publish success only after all probes and +recovery pass. Failure must leave the next invocation eligible to retry. + +Repeated unlock can avoid server churn only when storage, collection readiness, +and generation checks pass. Serialize coop operations, but do not assume that +this lock also serializes desktop or updater operations. Use native lifecycle +commands for their own locks and ownership checks. + +Native 0.154.0 imposes these constraints: + +- `start` reuses a responsive socket without checking its keyring environment. +- `bootstrap` replaces the server and starts/replaces an updater; it is not an + idempotent attachment operation. `stop` does not also stop the updater. +- Server readiness does not establish keyring readiness. Failed readiness can + leave detached processes requiring cleanup; an outer timeout is insufficient. +- A coop lock cannot prevent desktop-owned bootstrap/restart. Verify the real + command sequence and updater races before claiming reliable recovery. + +For intentional keyring replacement, retire the associated desktop server first. +For unexpected crashes, apply the next-unlock recovery contract. Preserve the +original failure when rollback also fails. Bound lock waits, D-Bus calls, native +startup, and rollback independently. Clean only resources owned by the failed +operation; cancellation must not tear down a shared healthy service. Validate +real process termination, not just removal of sockets or PID records. + +Keep distinct errors for missing/uninitialized collection, locked collection, +unavailable service, invalid default alias, plaintext/unsupported or invalid +storage, unlock failure, probe-write failure, probe-cleanup failure, busy +startup, server ownership conflict, and server readiness timeout. + +## Implementation order and repository touchpoints + +1. **Guest service and helper:** package dependencies and embedded scripts in + `src/guest.rs`, shared guest provisioning, `src/lima.rs`, and `scripts/guest/`. + Add the PAM helper and service setup with isolated lifecycle tests. +2. **Storage/readiness contract:** implement typed failure states, adoption, + collection checks, bounded operations, and discriminating tests. +3. **Terminal migration:** update `scripts/guest/codex-account.sh` to use the + singleton while preserving #481 behavior, nesting, and secret-store policy. +4. **Host command and native recovery:** add compatible parsing/dispatch in + `src/lib.rs` and shared command code; preserve host/guest trust boundaries. +5. **Existing guests:** support in-place installation, then require a VM restart + before singleton activation to retire old private daemons. Preserve valid + encrypted credentials and Codex home; do not silently select between stale + conflicting credential histories. +6. **User docs and gates:** update command/configuration references, examples, + guest dependency checks, and relevant mutation scope together. + +Before enabling the feature, prove the actual desktop sequence through native +interfaces: **unlock → connect → close terminal → reconnect → crash server → +reconnect**. Do not modify private desktop databases or replace native binaries +to force attachment. Resolve native lifecycle blockers before expanding scope. + +## Discovery and user workflow + +1. Start the VM and generate its alias with `coop ssh-config `. +2. Verify `ssh coop-` works and the remote login shell finds `codex`. +3. Run the implemented unlock operation. +4. Enable the SSH host in desktop Settings → Connections and select the guest + project, normally `/workspace`. +5. Complete guest Codex login and run a real task. + +Concrete SSH aliases match documented discovery; actual UI discovery/refresh +remains untested. Desktop sign-in, SSH authentication, keyring unlock, guest +Codex login, and project selection are separate steps. Test changed Lima ports, +VM recreation, stale saved connections, and removal of coop-owned SSH entries. + +## Validation and release gates + +| Gate | Evidence/status | +| --- | --- | +| Shared service coherence and #481 isolation | Local real-process tests pass, including an override-removal mutant. | +| PAM creation/unlock, same service PID | Local tests pass; replacing PAM with unconditional success fails the test. | +| Storage adoption cases | Valid, plaintext, unknown, truncated, unsupported-version, and damaged-ciphertext files tested locally without modifying existing files. Production candidate classification and fresh-service adoption are implemented; the parser and successful PAM unlock provide the additional evidence. | +| Logout vs lookup failure | Helper now verifies absence; unavailable-bus regression fails with old behavior restored. | +| Real systemd and SSH logout/reconnect | Passed with a disposable user and packaged services. | +| Keyring crash and user-manager restart | Passed locally; replacement starts locked and unlock restores credentials. This is not a VM reboot test. | +| Native duplicate start, crash, failed-start cleanup | Local prototype passes. Actual desktop/bootstrap/updater races remain untested. | +| Actual desktop terminal-close/reconnect | Required on macOS/Lima; not run. | +| Fresh browser/device-code login, logout/relogin, account switching | Real accounts required; not run. | +| Concurrent CLI/desktop OAuth refresh and logout | Not run. Shared storage does not guarantee atomic cross-process refresh or cache invalidation. | +| VM reboot, stop/destroy/recreate, migration | Required on both backends; not run. | +| Duplicate unlock, cancellation, wrong password, retry, cleanup | Production PAM/systemd/SSH fixture passes; direct probe tests cover read/write and cleanup failures. | + +The research baseline had 15 passing prototype tests; it is characterization +evidence, not completed-feature CI coverage. The historical private-bus terminal +test is superseded by the production shared-service terminal test. The user will run the pushed branch +on macOS/Lima and Linux/Firecracker. Apply repository build, formatting, lint, +unit, mutation, integration, and closeout-review requirements to the actual +implementation. Report unrun gates explicitly. + +If real concurrent OAuth use exposes a race, reproduce it at Codex's shared +credential-store boundary. Do not assume singleton keyring ownership fixes +application memory caches or refresh transactions. API-key/proxy mode needs +its own credential-delivery, rotation, and route-compatibility investigation. + +## Implementation validation record + +The shared provisioning path embeds `codex-keyring.py`, compiles the dedicated +unprivileged PAM client, installs its isolated PAM service, removes Ubuntu's +global GNOME password hook, and enables headless startup and lingering. In-place +installation records the installation boot and refuses activation until reboot. + +The operation classifies storage before prompts or writes, requires fresh-service +adoption when the recovered generation is missing or changed, checks the default +alias and lock state, and creates/reads/deletes a uniquely named non-secret item. +Only successful native retirement and final generation checks publish readiness. +Explicit retries reset systemd's start-limit failure before a bounded startup. +Passwords stay in the C helper; its locked buffers are cleared, core dumps are +disabled, and signal/timeout handling restores TTY echo. + +The production systemd/SSH fixture has passed first-use confirmation, encrypted +storage, repeated unlock without service churn, last-terminal closure and fresh +SSH access, wrong-password refusal, native bootstrap/updater presence, native +server crash/restart, keyring crash recovery, and refusal of plaintext, unknown, unsupported-version, +truncated and damaged-ciphertext stores. It also tests concurrent native +bootstrap/unlock and the terminal override-removal mutant. +These tests use disposable fixture values, not real account credentials. + +Release remains blocked on actual desktop attachment/reconnect, concurrent OAuth, +VM reboot and migration on both Lima and Firecracker. This branch is intended +for those validation runs; local Linux service tests do not establish them. + +The 28 host-only tests exercise storage and collection decisions, operation +retry/generation handling, migration refusal, and the production disposable-item +probe. Removing probe writes, deletion, value verification, recovery or format +checks makes these tests fail. Replacing PAM authentication with unconditional +success also fails the real-service fixture. + +Workspace build, format, clippy, unit tests and TOML formatting have passed. +`cargo deny` reports inherited advisory RUSTSEC-2026-0285 for the unchanged +`rustls 0.23.43` dependency; that is a separate release blocker. + +The final cargo-mutants full-file sweep of `src/lib.rs` and `src/guest.rs` +tested 66 mutants: 54 caught, 12 unviable, zero missed and zero timeouts. +The new host command is covered by the existing `cmd_*` IO exclusion; +`.cargo/mutants.toml` needed no change. Final pre-commit hooks passed. +Closeout review found no remaining code blockers; release readiness remains +blocked on the explicit external gates and dependency advisory above. diff --git a/docs/design/issue-480-desktop-auth-research.md b/docs/design/issue-480-desktop-auth-research.md new file mode 100644 index 0000000..388c463 --- /dev/null +++ b/docs/design/issue-480-desktop-auth-research.md @@ -0,0 +1,521 @@ +# Desktop guest authentication: issue #480 research + +Implementation entrypoint: +[`issue-480-desktop-auth-implementation.md`](issue-480-desktop-auth-implementation.md). +That guide consolidates the current decisions; this file retains evidence and +experimental detail. + +Research date: 2026-09-15. Repository: `f166413`. Locally inspected CLI: +`codex-cli 0.154.0`; upstream source inspected at `rust-v0.154.0`. + +Status: a real-process guest-side prototype is implemented in +[`tests/test-codex-desktop-prototype.py`](../../tests/test-codex-desktop-prototype.py). +It does not yet implement the coop command or persistent guest service. The +recommended design is one shared user keyring with separate Codex app-servers. +No real account login, desktop connection, or VM lifecycle test was run. +#481's production code is unchanged; its server isolation has also been tested +against the proposed shared-keyring arrangement. + +## Recommended architecture + +Use the ordinary per-user Secret Service architecture: **one GNOME Keyring +daemon, one encrypted store, multiple application clients**. Keep terminal +Codex's explicit configuration override so it continues using its own server. +Sharing the credential service does not require sharing the Codex app-server. + +```mermaid +flowchart LR + T[Terminal Codex: independent server] --> K[One guest user Secret Service] + D[Desktop Codex app-server] --> K + K --> S[One encrypted login keyring] +``` + +This is an established problem area, not a new storage abstraction to invent. +The same concurrent-daemon concern was raised on +[GNOME Discourse in 2020](https://discourse.gnome.org/t/are-concurrent-gnome-keyring-daemon-processes-safe-to-run/3601). +That thread has no technical answer; it establishes precedent only. The design +is supported by the [Secret Service architecture](https://specifications.freedesktop.org/secret-service/latest/ch01.html), +GNOME's [PAM integration](https://wiki.gnome.org/Projects/GnomeKeyring/Pam), and +the real-process comparisons below. + +| Approach | Assessment | +| --- | --- | +| One user keyring, separate Codex servers | Recommended. Coherent storage and a shared login without giving up #481's server isolation. | +| Multiple keyring daemons writing one directory | Rejected. Reproduced stale reads and resurrection after an unrelated write. | +| Separate desktop and terminal credential directories | Avoids the file conflict but introduces unnecessary separate login state. | +| Share the desktop app-server with terminal Codex | Outside this design; preserve #481's explicit no-reuse behavior. | +| Implement GNOME's private unlock protocol | Unnecessary. Its packaged PAM module already implements that operation. | + +**Headless unlock can use PAM without restarting the keyring.** A dedicated +keyring-only PAM service can obtain a password through an explicit helper's +conversation callback and pass it to `pam_gnome_keyring.so`. The module talks +to the running daemon through its existing control socket. This is the same +mechanism GNOME uses at login; coop need not implement its private protocol. +It does not require changing SSH authentication or setting a Linux account +password to match the keyring password. + +The isolated proof uses `pam_exec.so expose_authtok /usr/bin/true` to collect +the PAM authentication token, followed by `auth required pam_gnome_keyring.so` +without `auto_start`. `/usr/bin/true` emits nothing. This is a prototype PAM +stack, not a proposed replacement for the machine's login stack. The production +helper still needs TTY handling, first-use confirmation, bounded operations, +secret-memory cleanup, and post-unlock collection/write checks. +Guest provisioning must also account for package installation effects: +Ubuntu's `libpam-gnome-keyring` post-install script runs `pam-auth-update`, and +its default profile adds a password-management hook. Installing the module is +not equivalent to leaving the global PAM configuration unchanged. Audit that +profile explicitly; the unlock helper must use its dedicated PAM service. + +Why this works: the GNOME module reads `PAM_AUTHTOK` and performs an unlock +operation on the existing daemon. A bare `gnome-keyring-daemon --unlock` takes +a different startup path. `--start --unlock` is explicitly incompatible in +46.1, so combining those CLI flags is not a fix. Sources: +[GNOME PAM module, 46.1](https://github.com/GNOME/gnome-keyring/blob/46.1/pam/gkr-pam-module.c), +[daemon option handling, 46.1](https://github.com/GNOME/gnome-keyring/blob/46.1/daemon/gkd-main.c), +[Linux-PAM token collection, 1.5.3](https://github.com/linux-pam/linux-pam/blob/v1.5.3/modules/pam_exec/pam_exec.c). + +## Local prototype results + +Run on Ubuntu 24.04, GNOME Keyring 46.1, and native Codex 0.154.0. The fixture +owns private D-Bus/keyring processes, temporary credential storage, and native +daemon lifecycle operations. It uses disposable values only; no model task or +real account authentication occurs. The following observations narrow the +implementation requirements: + +- An explicitly supplied bus address reaches the native daemon. The launch + command exits; a subsequent native start attaches to the same process. + After a server crash, a new native start with that environment reconnects. +- WebSocket JSON-RPC over the native Unix control socket can store an invalid, + disposable API key in the configured keyring, read account state after client + disconnect and server replacement, and log out. No `auth.json` is created. + This exercises persistence, not API-key validity or ChatGPT OAuth. +- Concurrent native starts produce one `started` and one `alreadyRunning` + result at the same socket. Native start from a different bus still reuses + the first server; readiness does not verify authentication-session identity. +- Missing default aliases and locked collections are separately observable. + A wrong password leaves the collection locked. Replacing the owned keyring + process with the correct password restores access to encrypted data. +- Repeated standalone `--unlock` calls against a running service returned + success while leaving it locked and created competing daemon candidates. + During investigation, a candidate took the bus name after the original + daemon exited. The fixture therefore permits `--unlock` only to create its + foreground child and retires that child before trying again. It also disables + automatic service activation on its private bus. For intentional maintenance, + retire the associated app-server before replacing its keyring. Unexpected + service failures follow the user-triggered recovery contract below. +- On this private bus, native app-server readiness can succeed with a locked + keyring, while credential persistence fails. Do not turn every locked-keyring + failure into a server-start timeout diagnostic. +- A deliberately stalled temporary server binary demonstrates that a native + readiness failure leaves process state requiring explicit cleanup. The test + verifies process termination with a Linux PID file descriptor, not just + removal of the daemon's record. +- **Shared backing files are not coherent across separate keyring daemons.** + A second daemon continued returning the old disposable token after the first + refreshed and deleted it; a later **unrelated item write** from the stale + daemon resurrected the deleted credential in a fresh session. This was not + simply an explicit re-login to the deleted account. +- **One service fixes that storage inconsistency.** Independent `secret-tool` + processes immediately observed changes and deletion; an unrelated write did + not resurrect the deleted item, including after the service was restarted. +- Both `secret-tool` and native Codex worked with `DBUS_SESSION_BUS_ADDRESS` + and `GNOME_KEYRING_CONTROL` absent from the client environment. They discovered + the shared bus through `XDG_RUNTIME_DIR/bus`. The test supplies an isolated + runtime directory; PAM/systemd must supply the ordinary directory in guests. +- The existing terminal wrapper can use the same keyring as the desktop daemon + while keeping its embedded server. `strace` verified no desktop control-socket + connection; removing only #481's explicit override made it attach. +- The PAM proof created the missing login collection, rejected a wrong password, + and unlocked the existing collection with the correct password. **The same + keyring PID retained the D-Bus name throughout.** The short-lived PAM helper + exited without terminating the service. No host PAM configuration was changed. +- An existing empty-password login collection can become unlocked and pass + write/read checks while storing the disposable value literally in its file. + Collection identity, `Locked=false`, and successful writes do not prove + encryption. This must be checked before accepting an existing store. +- Native Codex retained its in-memory API-key account after another CLI process + logged out of the shared store. A server restart observed the deletion. + Shared storage therefore does not imply immediate cross-process cache + invalidation. This is an API-key characterization, not an OAuth refresh test. + +Run the prototype on Linux with a native Codex installation: + +```bash +sudo apt-get install python3-dbus python3-websocket dbus gnome-keyring libsecret-tools strace +COOP_TEST_CODEX="$(command -v codex)" python3 tests/test-codex-desktop-prototype.py -v +``` + +These are opt-in characterization tests for the inspected versions, not CI +coverage of a completed feature. Several assertions intentionally pin upstream +limitations and should be revisited when those limitations change. The package +symlink is created only under the temporary home; the installed package is not +modified. The stalled-server test replaces only that temporary symlink. +The existing #481 real-daemon regression also passes with Codex 0.154.0. +Validation: all fifteen prototype tests passed, including the optional PAM +test. The three original #481 tests passed in the preceding prototype run. +Changing the prototype's credential-store configuration to `file` deliberately +failed the login persistence assertion; restoring `keyring` passed. This checks +for plaintext immediately after login, before logout could hide the file. +An adversarial review also found that the original lookup helper treated bus +failures as missing credentials. It now requires a healthy unlocked collection +and an empty Secret Service search before accepting a failed lookup as absence. +The unavailable-bus regression passes; restoring the old helper makes it fail. +Replacing the plaintext fixture's startup with nonempty-password initialization +also makes its plaintext assertion fail, distinguishing encrypted storage from +an unlocked, writable collection. + +To include the PAM proof on a disposable Linux guest: + +```bash +sudo apt-get install libpam0g-dev libpam-gnome-keyring +cc -Wall -Wextra -Werror tests/codex-keyring-pam-probe.c -lpam -o /tmp/coop-keyring-pam-probe +COOP_TEST_CODEX="$(command -v codex)" \ +COOP_TEST_PAM_PROBE=/tmp/coop-keyring-pam-probe \ +python3 tests/test-codex-desktop-prototype.py -v +``` + +The local investigation extracted the PAM package into a temporary directory +instead of installing it into the host's authentication stack. The optional +`COOP_TEST_PAM_MODULE` selects that extracted module. Tests use +`pam_start_confdir` with a temporary configuration directory. Replacing the +GNOME module with `pam_permit.so` produced PAM success but failed the collection +assertion, confirming that the proof tests the actual unlock, not an exit code. + +## Conclusion + +### Follow-up: real systemd ownership and storage adoption + +Tested locally on the same Ubuntu 24.04 host using a disposable Linux user, +its ordinary systemd user manager, and fresh public-key SSH connections to the +existing local SSH server. The test enabled lingering, added the packaged +`gnome-keyring-daemon.service` to `default.target`, and started its packaged +socket/service. It used a private PAM configuration, without changing global +PAM. The disposable user, lingering setting, runtime services, SSH key, and +home were removed afterward. + +Observed results: + +- Noninteractive SSH supplied `/run/user/` as `XDG_RUNTIME_DIR`. +- PAM created an encrypted login collection without changing the service PID. + Independent `secret-tool` processes stored and read a disposable value with + `DBUS_SESSION_BUS_ADDRESS` and `GNOME_KEYRING_CONTROL` removed. +- After all SSH sessions exited, `loginctl` reported zero sessions while the + user manager remained active. A new SSH connection found the same unlocked + keyring PID. +- Killing the keyring service's main process caused systemd to replace it with + a locked service. PAM unlock restored access to the persisted value. +- Stopping and starting the user manager brought up the keyring through the + headless target, locked again. Another unlock restored the value. This is a + user-manager restart test, not a VM reboot test. + +A separate isolated-daemon experiment checked storage adoption against actual +GNOME 46.1 files. The binary format begins with `GnomeKeyring\n\r\0\n` +and four zero bytes identifying its supported version/algorithms. That prefix +identifies an **encrypted-format candidate**, not file integrity. See the +[versioned binary reader](https://github.com/GNOME/gnome-keyring/blob/46.1/pkcs11/secret-store/gkm-secret-binary.c) +and [plaintext format](https://github.com/GNOME/gnome-keyring/blob/46.1/pkcs11/secret-store/gkm-secret-textual.c). + +| Existing file | Fresh daemon result | Adoption result | +| --- | --- | --- | +| Valid encrypted store | Login collection locked | Correct-password PAM unlock succeeds. | +| Plaintext store | Login collection present | Rejected by format check before PAM or writes. | +| Unknown format | Login collection missing | Rejected; existing file must not be treated as first use. | +| Truncated binary with valid prefix | Login collection missing | Rejected; prefix alone is insufficient. | +| Unsupported binary version | Login collection missing | Rejected before PAM. | +| Damaged ciphertext with valid prefix | Login collection locked | PAM unlock fails; collection stays locked. | + +All six existing files remained byte-for-byte unchanged by these checks. +The adoption sequence is therefore: classify format, require the fresh daemon +to load an existing collection, then unlock through PAM and verify the intended +collection. Only a genuinely absent store may enter first-use initialization. +Do not try PAM creation to repair a file the daemon failed to load. A damaged +ciphertext and an incorrect password are not distinguishable from this unlock +result; report failure without inventing the cause. + +Initial adoption must validate through a fresh service generation; an already +unlocked daemon can hold cached state that says nothing about a subsequently +damaged file. This experiment is not a general integrity validator for an +already-unlocked store. Production adoption checks and their error states still +need implementation. The remaining external gates are actual desktop/native +updater behavior, real OAuth concurrency, and the two VM backends. + +Prefer the standard user bus over a desktop-specific bus. Native launches can +then discover the same service from their normal login environment; coop does +not need to inject the environment of another process into a desktop daemon. +The first gate should remain **unlock → desktop connects → terminal exits → +desktop reconnects**, followed immediately by a server-crash reconnect. + +The user will run the branch on macOS/Lima and Linux/Firecracker after it is +pushed. Implement toward this standard service arrangement, and treat those +tests as required acceptance gates before claiming desktop support. Native +bootstrap/updater behavior and failed-start cleanup still need verification in +the actual desktop command sequence. + +## What is established + +### Existing coop behavior + +- [#480](https://github.com/trailofbits/coop/issues/480) records successful SSH + connection and failed guest OAuth persistence. Its missing collection and + stalled startup observations are separate failure states. +- [#481](https://github.com/trailofbits/coop/issues/481)'s fix is present in + `scripts/guest/codex-account.sh`: terminal launches explicitly pass + `-c 'cli_auth_credentials_store="keyring"'`. Preserve that server-isolation + override. The proposed change replaces per-terminal keyring sessions with + the managed user service; it does not restore terminal app-server reuse. +- The wrapper already prompts without echo, rejects empty passwords, confirms + first-use passwords, and probes a disposable write. Its `*.keyring` existence + check does not verify the live login collection or default alias. Its error + classification is insufficient for the desktop service. +- Native installation is already implemented in `scripts/guest/codex.sh`. + Existing disks still need migration; a golden-image rebuild alone does not + update an existing VM. See `docs/codex-integration.md`. + +### Native Codex lifecycle, version 0.154.0 + +The installed CLI exposes `app-server daemon start`, `restart`, `stop`, +`version`, `bootstrap`, and `app-server proxy [--sock PATH]`. + +Source inspection establishes: + +| Interface | Behavior relevant to coop | +| --- | --- | +| `daemon start` | Uses the native managed binary, not an arbitrary PATH wrapper. Returns JSON after a control-socket initialize probe. Reuses a responding socket without checking its D-Bus session. | +| Detached launch | Inherits the launching process environment; Unix launch uses `setsid` and redirects standard streams. No explicit environment-file option appears in CLI help. | +| `daemon restart` | Starts the replacement from the restart caller's environment. An ordinary SSH restart could therefore lose the managed bus address. | +| `daemon bootstrap` | Stops an existing managed server, starts another, and starts/replaces a detached updater. It is not an idempotent attach operation. | +| Native serialization | Lifecycle mutations take a per-`CODEX_HOME` operation lock; process publication has additional reservation locking and process-start identity checks. | +| Readiness failures | Nominal readiness deadline is 10 seconds; operation-lock deadline is 75 seconds. `start` and `bootstrap` propagate readiness errors without a corresponding rollback in those paths. An outer timeout alone does not clean up detached children. | +| `daemon stop` | Stops the app-server; the inspected path does not also stop the bootstrap updater. | + +Sources: upstream [daemon implementation](https://github.com/openai/codex/blob/rust-v0.154.0/codex-rs/app-server-daemon/src/lib.rs), +[process launch](https://github.com/openai/codex/blob/rust-v0.154.0/codex-rs/app-server-daemon/src/backend/pid_start.rs), +[managed binary resolution](https://github.com/openai/codex/blob/rust-v0.154.0/codex-rs/app-server-daemon/src/managed_install.rs), +and [daemon lifecycle contract](https://github.com/openai/codex/blob/rust-v0.154.0/codex-rs/app-server-daemon/README.md). +These interfaces are experimental; verify the same assumptions after upgrades. + +Consequences: + +- Prefer `start` for the first experiment. Determine whether the desktop uses + `start`, `bootstrap`, `restart`, or another launch path on each connection. +- A successful `start` is not evidence that the server uses the intended bus. + Verify session identity and credential persistence separately. +- A launcher-only environment assignment is insufficient if later SSH commands + or an existing updater can replace the server outside that environment. +- Do not assume native bootstrap installs a systemd service. The inspected + implementation uses detached PID-managed processes and an updater. + +## Proposed guest service + +One authentication service per guest UID, used by terminal and desktop clients: + +- Use the normal user bus at `/run/user//bus`, managed by systemd, and the + packaged GNOME Keyring service/control socket. GNOME already ships a + [foreground service unit](https://github.com/GNOME/gnome-keyring/blob/46.1/daemon/gnome-keyring-daemon.service.in). + Ubuntu's installed unit differs in its install target, so explicitly arrange + headless startup instead of assuming a graphical-session target will run. +- Enable lingering for the guest user so the user manager survives the last + SSH logout. This is systemd's supported mechanism for long-running user + services ([systemd 255 documentation](https://github.com/systemd/systemd/blob/v255/man/loginctl.xml)). + The keyring starts locked after reboot; lingering does not retain its password. +- Let ordinary SSH login sessions discover the standard bus. Verify that + `pam_systemd` supplies `XDG_RUNTIME_DIR` in both guest images, including + noninteractive SSH. Do not import another process's environment. +- Make the terminal wrapper check/unlock this service, retaining the explicit + keyring configuration override. It must not launch another private keyring + against the same files. Nested terminal invocations use the same service. +- Keep Codex's native daemon commands responsible for server PID/socket + ownership. Coop owns authentication readiness and recovery. Avoid a second + competing app-server PID manager; audit the native updater separately. +- An app-server started while the keyring was locked may cache missing auth. + After an unlock transition, recover that server through the native restart + interface. Repeated unlock may be a no-op only after encryption, collection + readiness, and the recorded service generation have all been verified. + Bus/keyring failures need explicit server recovery, not just a readiness flag. +- No persistent password, plaintext auth fallback, or host credential copy. + Keep the existing managed `~/.codex` credential-store policy. + +Upgrade guest support in place where possible, then restart the VM to retire +old per-terminal keyring daemons before enabling the singleton. The existing +encrypted collection and Codex home can remain in place; separate desktop +credentials or a new keyring password are not inherent requirements. + +### Recovery ownership after coop exits + +The proposed first implementation uses **user-triggered recovery**. Systemd +owns the bus and keyring lifetime; `coop codex unlock` owns the serialized +readiness check and native server recovery while that command runs. No coop +process remains to retire a native server automatically after a keyring crash. +An already running Codex client may retain cached credentials during that gap. + +Record a non-secret service generation in the guest runtime directory: boot ID, +D-Bus server ID, and the keyring's unique bus owner. On each unlock operation, +compare it with the last successfully recovered generation. A change or missing +record requires native server recovery even if another client already unlocked +the replacement keyring. Publish the new record only after the collection probe +and native recovery succeed; retry after failure must not take the no-op path. +Use Codex's lifecycle lock through its native commands, not direct PID control. + +After a bus/keyring failure, the documented action is to rerun unlock and then +reconnect the desktop. Automatic retirement is not part of this contract. +Desktop-owned starts and the updater do not take coop's lock, so their races +with this recovery remain an explicit acceptance gate. If native interfaces +cannot provide bounded recovery in that sequence, this design is not ready to +ship; a successful local keyring probe is insufficient. + +### Interactive unlock contract + +`coop codex unlock ` is a proposed spelling, not an existing command. The +current CLI accepts a VM name at that position; preserve existing parsing and +consider the ambiguity of a VM named `unlock` before choosing the public syntax. + +Serialize create/unlock/start as one coop operation. Read the password on the +guest TTY; pass it through stdin or a private pipe, never argv, an environment +variable, a temporary file, or logs. Clear in-process references promptly. + +Start or verify the packaged user service, then use the dedicated PAM helper +to create/unlock its login collection. Supply its control directory from the +known user runtime path. Leave `auto_start` out of the PAM stack so the helper +does not acquire daemon lifetime ownership. Inspect the collection before +attempting a write that could invoke a graphical prompt. Neither PAM success +nor process exit status alone establishes that the collection is ready. + +Before prompting, writing a probe, or taking an already-unlocked shortcut, +verify that an existing login store uses the supported encrypted GNOME format. +Reject plaintext and unrecognized formats with a distinct diagnostic. The +Secret Service API has no portable at-rest encryption property; the production +implementation needs a version-tested GNOME storage check, not an inference +from the collection name, `Locked`, file permissions, or PAM success. Creating +a new collection requires a confirmed nonempty password and the same encryption +check afterward. The prototype characterizes this gap; it does not yet implement +the production format check. Do not overwrite or silently convert an existing +plaintext/unknown store. Require explicit migration to a new encrypted +collection and reauthentication before enabling managed desktop use. + +After initialization, resolve `ReadAlias("default")`, verify the target object +exists and is the intended persistent login collection, then check its `Locked` +property. `/` means the alias is absent. Create a unique disposable non-secret +item, read it back, and delete it. Report cleanup failure. Do not silently +accept a session-only collection or redirect an unrelated existing default. +See the [Secret Service API](https://specifications.freedesktop.org/secret-service/latest-single/). + +Return distinct states: collection missing/uninitialized; collection locked; +service unavailable; invalid/dangling default alias; write or cleanup failed; +unencrypted/unsupported store; startup busy; server ownership conflict; +server readiness timeout. A failed +probe alone does not prove the password was wrong. Bound noninteractive D-Bus +calls, lock acquisition, startup, and rollback separately. Prompt cancellation +must also release ownership and clean up newly started processes. + +### Remaining Codex cache and refresh behavior + +One keyring daemon fixes divergent keyring-file state. It does not invalidate +credentials already copied into a running application's memory. The API-key +probe above demonstrates that distinction. Do not promise immediate global +logout or immediate account switching across all running Codex processes. + +Codex 0.154.0's `AuthManager` caches credentials, reloads the active store before +its guarded ChatGPT refresh, skips refresh if another writer already changed +the stored credentials, and rejects the guarded refresh if the account no +longer matches. Its refresh semaphore is process-local; these guards do not +establish an atomic transaction across independent Codex servers. +See [the versioned auth manager](https://github.com/openai/codex/blob/rust-v0.154.0/codex-rs/login/src/auth/manager.rs). + +Real ChatGPT concurrent refresh, logout/relogin, and account switching remain +acceptance tests. Restart affected clients after externally changing login +state when their cache does not update. If simultaneous OAuth refresh exposes +a remaining race, reproduce it at Codex's shared-store boundary and address it +there; do not introduce a custom OAuth broker or duplicate login stores as a +premature workaround. A keyring lock also cannot erase secrets clients already +hold in memory; VM reboot does terminate those guest processes. + +## Prototype procedure and decision gate + +Use a disposable supported guest and record guest OS, GNOME Keyring version, +CLI version, desktop build, and native-install layout. + +1. Establish the standard user bus/keyring service and initialize/unlock its encrypted + login collection. Verify the default alias and disposable item lifecycle. +2. Launch native `daemon start` through an ordinary fresh SSH session. Preserve + the usual user-bus and Codex home/socket discovery. Record bounded, non-secret readiness + results and the service identity, not credentials or full environments. +3. Connect through the actual desktop app. Observe its launch commands using + narrowly scoped, non-secret instrumentation in the disposable guest. Prove + which daemon and Secret Service instance handle the connection. +4. Complete browser login and a real guest task. Close the unlock terminal, + disconnect the desktop, then reconnect and run another task. +5. Crash just the server and reconnect. Check that any replacement inherits the + same session. Repeat with simultaneous connection attempts. +6. Reboot. Connection must not bypass the required new unlock or hang on a GUI + prompt. Unlock again and reconnect using the persisted encrypted credentials. + +Pass only if native desktop attachment and all replacement paths reach the +same user service. In particular, test the desktop reconnecting after boot +while the keyring is still locked: coop can provide precise preflight/status +errors, but the desktop's own error presentation must be observed. Retain +failed-start cleanup checks even though bus discovery is simpler. Do not edit +private desktop databases or replace packaged binaries to force attachment. + +## Discovery and remaining user steps + +`src/workspace.rs::ssh_config_block` writes `Host coop-` directly into +`~/.ssh/config`, including host, port, user, and identity. This matches the +documented discovery input: concrete aliases resolved through OpenSSH. +Discovery compatibility is established by code/documentation inspection; +actual appearance and refresh in the desktop UI remain to be tested. +See [official SSH connection setup](https://learn.chatgpt.com/docs/remote-connections#connect-to-an-ssh-host). + +After authentication support is proven, document this sequence: + +1. Start the VM and run `coop ssh-config ` on the desktop machine. +2. Verify `ssh coop-` works and `codex` is on the remote login-shell PATH. +3. Run the eventual explicit unlock operation. +4. In Settings → Connections, add or enable the discovered SSH host and select + `/workspace` or the intended guest project directory. +5. Complete guest Codex authentication and verify a task runs in the guest. + +Desktop account sign-in, SSH authentication, guest keyring unlock, guest Codex +login, and project selection are distinct steps. No supported SSH-project +registration CLI/deep link was established in the documentation inspected. +Existing coop code refreshes managed SSH configuration on lifecycle operations +and removes its own blocks on destroy; verify desktop behavior for changed Lima +ports, recreation, repeated setup, app absence, and stale saved connections. + +## API-key and proxy mode: separate investigation + +Desktop API-key support in general does not establish remote proxy compatibility. +The ordinary desktop SSH connection bypasses coop's per-session environment +assembly. `src/commands/lifecycle.rs` supplies the proxy capability token as the +configured provider's environment key; `src/workspace.rs`'s alias does not supply +that token. Raw API-key forwarding also must not be assumed to happen here. + +Test how a persistent daemon receives and refreshes the intended provider +credential, including proxy restart/token rotation and model-mode switches. +Check desktop startup/discovery calls against the proxy route allowlist +(`docs/credential-proxy.md`). Keep ChatGPT auth and `[proxy.openai]` mutually +exclusive. Do not forward a new secret or persist the capability token merely +to make the desktop launch work without a separate design decision. + +## Required validation after the prototype + +| Case | Required observation | +| --- | --- | +| Fresh login | Browser and device-code login each persist credentials in the intended encrypted collection; no fallback auth file. | +| Terminal closes / desktop reconnects | Session survives; server attaches correctly; real remote task completes. | +| Server crash | Replacement uses the owned session, with one socket owner. | +| Bus/keyring crash | Next unlock detects the changed service generation and recovers the native server; reconnect succeeds. Record actual desktop behavior before that explicit recovery. | +| Existing plaintext/unknown keyring | Refused before the already-unlocked shortcut or any credential/probe write; no silent conversion. | +| VM restart | Runtime ownership resets; explicit unlock is required; encrypted login survives. | +| Logout/relogin and token refresh | No stale resurrection or lost writes; test CLI and desktop in both orders. | +| Simultaneous CLI/desktop | #481 isolation remains; test concurrent refresh and writes to backing storage. | +| Duplicate connections/unlocks | One lifecycle owner; bounded waits; no duplicate keyring/server/updater. | +| Failed starts/cancel/wrong password | Preserve failure reason; clean up newly owned processes, sockets, and probe items; retry succeeds. | +| Existing wrong-session server | Reject or deliberately replace under ownership; never report ready based solely on socket response. | +| Bootstrap/update/restart | Every replacement retains the intended environment and lifetime ownership. | +| Stop/destroy/recreate | No owned runtime processes survive; handle stale desktop connections without deleting unrelated configuration. | + +Run desktop-to-Lima end to end and backend-shared authentication/lifecycle +integration on Firecracker. Validate new tripwires by removing the promised +behavior. The local prototype covers only the process-level observations above; +both platform suites, interactive TTY handling, real ChatGPT login/refresh, and +actual desktop connection/reconnection remain unrun. diff --git a/docs/images-and-profiles.md b/docs/images-and-profiles.md index e6bec99..54811a2 100644 --- a/docs/images-and-profiles.md +++ b/docs/images-and-profiles.md @@ -18,7 +18,7 @@ coop stores the result under `~/.coop/images//`. When creating an instance Every template installs these packages regardless of profile selection. -**Base packages:** `openssh-server`, `dbus-user-session`, `curl`, `wget`, `git`, `build-essential`, `ca-certificates`, `gnupg`, `lsb-release`, `sudo`, `iproute2`, `iptables`, `kmod`, `procps`, `util-linux`, `jq`, `rsync`, `unzip`, `zip`, `file`, `gnome-keyring`, `less`, `libsecret-tools` +**Base packages:** `openssh-server`, `dbus-user-session`, `curl`, `wget`, `git`, `build-essential`, `ca-certificates`, `gnupg`, `lsb-release`, `sudo`, `iproute2`, `iptables`, `kmod`, `procps`, `util-linux`, `jq`, `rsync`, `unzip`, `zip`, `file`, `gnome-keyring`, `libpam-gnome-keyring`, `libpam0g-dev`, `libpam-systemd`, `python3`, `python3-dbus`, `less`, `libsecret-tools` **Docker:** `docker-ce`, `docker-ce-cli`, `containerd.io`, `docker-buildx-plugin`, `docker-compose-plugin` @@ -34,7 +34,8 @@ user's home directory. `~/.local/bin/codex` is the native launcher; The image also installs `/usr/local/bin/codex-account`, a wrapper used by `[codex] auth = "chatgpt"` to run Codex with a D-Bus session and guest Linux Secret Service storage. The wrapper and its three supporting packages -(`dbus-user-session`, `gnome-keyring`, `libsecret-tools`) are installed in every +(`dbus-user-session`, `gnome-keyring`, `libpam-gnome-keyring`, +`libpam0g-dev`, `libpam-systemd`, `python3`, `python3-dbus`, `libsecret-tools`) are installed in every image, not gated on the `auth` setting: an image is built once and reused across configs, so gating them would let a later `auth = "chatgpt"` edit meet an image that cannot serve it. When that mode is not configured the wrapper simply execs diff --git a/docs/index.md b/docs/index.md index 5b6acfc..2cb4c1a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -17,6 +17,9 @@ short navigational entrypoint; durable detail lives here. - [`platform-notes.md`](platform-notes.md) — Firecracker CI-kernel workarounds, Docker networking, scp `~` caveat, tracing-to-stderr. +- [Desktop authentication implementation](design/issue-480-desktop-auth-implementation.md) + — shared guest service, adoption policy, recovery and remaining release gates. + ## For users - [`getting-started.md`](getting-started.md) — install and first VM. diff --git a/docs/testing.md b/docs/testing.md index a5c34f0..e88cc50 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -56,19 +56,27 @@ skip of the routed guest-isolation probe, since it would mask the coop rule. Run `python3 tests/test-codex-account.py` for the account wrapper's argument, login/logout, API-key passthrough, and `codex-yolo` regressions (also in Linux -CI). To additionally test implicit daemon reuse with a real Linux Codex binary: +CI). Shared guest readiness and storage decisions run with: ```bash -COOP_TEST_CODEX="$(command -v codex)" python3 tests/test-codex-account.py +/usr/bin/python3 tests/test-codex-keyring.py ``` -This requires `dbus-run-session`, `gnome-keyring-daemon`, `secret-tool`, and -`strace`. It uses temporary homes and disposable keyring passwords, starts a -real app-server on a separate unusable keyring session, and observes terminal -socket connections. It checks that sign-in is reached without reusing that -server and that removing the wrapper override restores reuse. No account login -or real tokens are needed. Run it when upgrading Codex: daemon selection is -version-dependent. This opt-in test does not replace either VM backend gate. +The production helper's PAM, systemd, native server recovery and fresh SSH +connections are exercised on Linux with a disposable user: + +```bash +sudo COOP_TEST_KEYRING_SYSTEMD=1 COOP_TEST_CODEX=/absolute/native/bin/codex \ + /usr/bin/python3 tests/test-codex-keyring-systemd.py -v +``` + +Requires a running systemd and localhost SSH server, `python3-dbus`, `python3-websocket`, +`strace`, a C +compiler, PAM development headers and `libpam-gnome-keyring`. The fixture refuses +an existing `/etc/pam.d/coop-codex-keyring`, creates its own dedicated PAM entry, +and removes it and the disposable user afterward. It never uses real account +credentials. This gate does not replace desktop UI/OAuth or VM lifecycle tests +on both backends. The host-only readiness test runs in Linux CI. The full Codex update tests install native release `0.153.0` before running `codex update` as the guest user, and require the installed version to change. diff --git a/scripts/guest/codex-account.sh b/scripts/guest/codex-account.sh index 167f26b..3912888 100644 --- a/scripts/guest/codex-account.sh +++ b/scripts/guest/codex-account.sh @@ -8,10 +8,6 @@ set -euo pipefail CODEX_BIN="/usr/local/bin/codex" CODEX_CONFIG="${CODEX_HOME:-$HOME/.codex}/config.toml" -KEYRING_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/keyrings" -PROBE_SERVICE="coop-codex" -PROBE_ACCOUNT="keyring-probe" - die() { echo "codex-account: $*" >&2 exit 1 @@ -34,89 +30,6 @@ keyring_mode() { keyring_mode_in "$CODEX_CONFIG" } -keyring_exists() { - compgen -G "$KEYRING_DIR/*.keyring" >/dev/null 2>&1 -} - -# secret-tool's stderr from the last probe, so the failure path can report the -# real cause instead of only guessing at the password. -PROBE_ERROR="" - -# Writing proves the collection is both reachable and unlocked; a lookup can -# succeed against a locked collection. The probe item is cleared again so -# repeated launches do not accumulate junk in the user's keyring. -probe_keyring() { - PROBE_ERROR="$(printf 'ok' \ - | timeout 5 secret-tool store \ - --label="coop Codex keyring probe" \ - service "$PROBE_SERVICE" \ - account "$PROBE_ACCOUNT" \ - 2>&1 >/dev/null)" || return 1 - timeout 5 secret-tool clear \ - service "$PROBE_SERVICE" \ - account "$PROBE_ACCOUNT" \ - >/dev/null 2>&1 || true -} - -unlock_keyring() { - local creating=0 password confirm output - keyring_exists || creating=1 - - if [ ! -t 0 ]; then - die "Codex ChatGPT auth needs an interactive TTY to unlock the guest keyring" - fi - - # On a fresh guest there is no keyring yet, so this prompt is choosing a - # password rather than entering one. Say so, and confirm it — an - # unnoticed typo would otherwise lock the credentials behind a password - # the user cannot reproduce on the next launch. - if [ "$creating" = 1 ]; then - printf '%s\n' \ - 'codex-account: this VM has no guest keyring yet.' \ - 'Choose a password to create one. It encrypts the Codex account' \ - 'credentials stored inside the guest, and later `coop codex` runs' \ - 'ask for it again. It is not your ChatGPT or host password.' >&2 - fi - - if ! IFS= read -rsp "Codex keyring password: " password; then - echo >&2 - die "failed to read keyring password" - fi - echo >&2 - - if [ -z "$password" ]; then - die "keyring password must not be empty" - fi - - if [ "$creating" = 1 ]; then - if ! IFS= read -rsp "Confirm keyring password: " confirm; then - echo >&2 - die "failed to read keyring password" - fi - echo >&2 - if [ "$password" != "$confirm" ]; then - die "passwords did not match; no keyring was created" - fi - fi - - # Let the daemon's stderr reach the terminal. Once it forks it redirects - # its own fd 2, so this surfaces only its startup diagnostics — the probe - # below is what reports a wrong password. - output="$(printf '%s' "$password" \ - | timeout 30 gnome-keyring-daemon --unlock --components=secrets)" \ - || die "failed to unlock the guest keyring" - - # The daemon prints its session env as `NAME=value` lines. Take only - # those, and export them: an unfiltered `eval` would execute any GLib - # diagnostic the daemon puts on stdout as a command, and a bare - # assignment would not reach Codex anyway. - while IFS= read -r line; do - case "$line" in - [A-Za-z_]*=*) export "${line%%=*}=${line#*=}" ;; - esac - done <<<"$output" -} - if [ ! -x "$CODEX_BIN" ]; then die "$CODEX_BIN is missing; rebuild the coop image" fi @@ -136,44 +49,15 @@ if ! keyring_mode; then exec "$CODEX_BIN" "$@" fi -# The keyring speaks D-Bus, and a headless SSH session has no session bus. -# Re-exec under one, using the env guard to avoid recursing forever. -if [ "${COOP_CODEX_ACCOUNT_DBUS:-0}" != "1" ]; then - command -v dbus-run-session >/dev/null 2>&1 \ - || die "dbus-run-session is missing; install dbus-user-session or rebuild the coop image" - export COOP_CODEX_ACCOUNT_DBUS=1 - exec dbus-run-session -- "$0" "$@" -fi - -for tool in gnome-keyring-daemon secret-tool timeout; do - command -v "$tool" >/dev/null 2>&1 \ - || die "$tool is missing; rebuild the coop image with Codex account-auth support" -done - -# A nested codex-account — an in-guest agent shelling out to `codex-account` or -# `codex-yolo` — inherits this bus and its already-unlocked keyring. Unlocking -# again there would fail, for the same reason the ordering below matters, so -# reuse the session instead of prompting a second time. -if [ "${COOP_CODEX_ACCOUNT_UNLOCKED:-0}" = "1" ]; then - probe_keyring \ - || die "inherited guest Secret Service session is unusable${PROBE_ERROR:+: $PROBE_ERROR}" -else - # Unlock before anything else touches the bus. `gnome-keyring-daemon - # --unlock` only creates and unlocks the login collection when it is the - # process that starts the daemon; once any daemon owns - # `org.freedesktop.secrets` it hands the unlock to the graphical - # gcr-prompter, which cannot render on a headless guest and exits - # immediately. That includes a daemon the probe itself would D-Bus-activate, - # so there is no cheap "is it already unlocked?" check to make first. - unlock_keyring - probe_keyring \ - || die "guest Secret Service is unavailable${PROBE_ERROR:+: $PROBE_ERROR}; check the keyring password" - export COOP_CODEX_ACCOUNT_UNLOCKED=1 -fi +# Every terminal, nested invocation and desktop uses the PAM/systemd user bus. +# The helper ignores old COOP_CODEX_ACCOUNT_* markers and verifies live state. +/usr/local/bin/codex-keyring >/dev/null +export DBUS_SESSION_BUS_ADDRESS="unix:path=${XDG_RUNTIME_DIR:?}/bus" +export GNOME_KEYRING_CONTROL="$XDG_RUNTIME_DIR/keyring" # Codex 0.154.0 can reuse a desktop daemon on another D-Bus session when # there are no explicit config overrides. Keep terminal auth on the keyring -# we just unlocked. Prepend the default so caller overrides retain precedence. +# shared by the guest user. Prepend the default so caller overrides retain precedence. exec "$CODEX_BIN" -c 'cli_auth_credentials_store="keyring"' "$@" CODEXACCOUNTEOF chmod 755 /usr/local/bin/codex-account diff --git a/scripts/guest/codex-keyring-migrate.sh b/scripts/guest/codex-keyring-migrate.sh new file mode 100644 index 0000000..5ff25a4 --- /dev/null +++ b/scripts/guest/codex-keyring-migrate.sh @@ -0,0 +1,21 @@ +set -euo pipefail +: "${GUEST_USER:?GUEST_USER must be set by the orchestrator}" +# This check runs only inside the live guest, never in a host-side chroot. +# Multiple private daemons may hold different versions of the same keyring. +if keyring_pids=$(timeout 10 pgrep --uid "$GUEST_USER" --full '(^|/)gnome-keyring-daemon( |$)'); then + if [[ "$keyring_pids" == *$'\n'* ]]; then + echo 'Multiple keyring daemons may hold conflicting credentials. Resolve those histories before migration.' >&2 + exit 1 + fi +else + status=$? + if [ "$status" -ne 1 ]; then + echo 'Could not enumerate guest keyring daemons; migration was not started.' >&2 + exit 1 + fi +fi +export DEBIAN_FRONTEND=noninteractive +apt-get update -qq +apt-get install -y --no-install-recommends build-essential \ + dbus-user-session gnome-keyring libpam-gnome-keyring libpam0g-dev \ + libpam-systemd libsecret-tools python3 python3-dbus diff --git a/scripts/guest/codex-keyring-pam.c b/scripts/guest/codex-keyring-pam.c new file mode 100644 index 0000000..b107e0b --- /dev/null +++ b/scripts/guest/codex-keyring-pam.c @@ -0,0 +1,113 @@ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* Unprivileged PAM client: unlocks this UID's existing service, never authenticates + * a login or starts a daemon. The parent verifies storage and Secret Service. + * Keep passwords out of the Python coordinator, argv, environment and files. */ +static volatile sig_atomic_t cancelled; +static char password[512]; +static char confirmation[512]; + +static void cancel(int sig) { cancelled = sig; } + +static bool prompt(const char *label, char *out, size_t capacity) { + struct termios saved, hidden; + if (tcgetattr(STDIN_FILENO, &saved) != 0) return false; + hidden = saved; + hidden.c_lflag &= ~(ECHO | ECHONL); + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &hidden) != 0) return false; + fputs(label, stderr); + fflush(stderr); + size_t used = 0; + bool ok = false; + while (!cancelled) { + char c = 0; + ssize_t n = read(STDIN_FILENO, &c, 1); + if (n != 1) break; + if (c == '\n') { ok = used > 0; break; } + if (c == '\0' || used + 1 >= capacity) break; + out[used++] = c; + } + out[used] = '\0'; + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &saved) != 0) ok = false; + fputc('\n', stderr); + return ok && !cancelled; +} + +static int converse(int count, const struct pam_message **messages, + struct pam_response **out, void *unused) { + (void)unused; + if (cancelled || count != 1 || messages[0]->msg_style != PAM_PROMPT_ECHO_OFF) + return PAM_CONV_ERR; + struct pam_response *responses = calloc(1, sizeof(*responses)); + if (!responses) return PAM_BUF_ERR; + responses[0].resp = strdup(password); + if (!responses[0].resp) { free(responses); return PAM_BUF_ERR; } + /* libpam owns and wipes this copy when it releases the response/token. */ + *out = responses; + return PAM_SUCCESS; +} + +int main(int argc, char **argv) { + bool creating = argc == 2 && strcmp(argv[1], "--create") == 0; + if (argc != 1 && !creating) return 2; + if (geteuid() != getuid() || getuid() == 0 || !isatty(STDIN_FILENO)) { + fputs("keyring unlock requires the guest user's interactive TTY\n", stderr); + return 2; + } + if (prctl(PR_SET_DUMPABLE, 0) != 0 || + mlock(password, sizeof(password)) != 0 || + mlock(confirmation, sizeof(confirmation)) != 0) return 2; + struct sigaction action = {.sa_handler = cancel}; + sigemptyset(&action.sa_mask); + const int signals[] = {SIGINT, SIGTERM, SIGHUP, SIGALRM}; + for (size_t i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) + if (sigaction(signals[i], &action, NULL) != 0) return 2; + alarm(120); + int result = 1; + pam_handle_t *handle = NULL; + if (creating) + fputs("Choose a nonempty password for the guest's encrypted keyring.\n", stderr); + if (!prompt("Codex keyring password: ", password, sizeof(password))) goto cleanup; + if (creating) { + if (!prompt("Confirm keyring password: ", confirmation, sizeof(confirmation))) goto cleanup; + if (strcmp(password, confirmation) != 0) { + fputs("Passwords did not match; no keyring was created.\n", stderr); + goto cleanup; + } + } + explicit_bzero(confirmation, sizeof(confirmation)); + struct passwd *user = getpwuid(getuid()); + if (!user) goto cleanup; + struct pam_conv conv = {converse, NULL}; + int status = pam_start("coop-codex-keyring", user->pw_name, &conv, &handle); + if (status != PAM_SUCCESS) goto cleanup; + char control[128]; + int length = snprintf(control, sizeof(control), "GNOME_KEYRING_CONTROL=/run/user/%lu/keyring", + (unsigned long)getuid()); + if (length < 0 || (size_t)length >= sizeof(control)) goto cleanup; + status = pam_putenv(handle, control); + alarm(30); + if (status == PAM_SUCCESS && !cancelled) status = pam_authenticate(handle, 0); + if (status == PAM_SUCCESS && !cancelled) result = 0; +cleanup: + if (handle) pam_end(handle, result == 0 ? PAM_SUCCESS : PAM_ABORT); + explicit_bzero(password, sizeof(password)); + explicit_bzero(confirmation, sizeof(confirmation)); + munlock(password, sizeof(password)); + munlock(confirmation, sizeof(confirmation)); + alarm(0); + if (cancelled) fputs("Keyring unlock cancelled.\n", stderr); + return cancelled ? 128 + cancelled : result; +} diff --git a/scripts/guest/codex-keyring-setup.sh b/scripts/guest/codex-keyring-setup.sh new file mode 100644 index 0000000..7a51cd7 --- /dev/null +++ b/scripts/guest/codex-keyring-setup.sh @@ -0,0 +1,31 @@ +# Runs after the embedded helper sources have been staged in KEYRING_BUILD. +# This script installs files only. Activation waits for the next guest boot, +# retiring old private buses/keyrings and their native updaters together. +: "${GUEST_USER:?GUEST_USER must be set by the orchestrator}" +cc -std=c11 -O2 -Wall -Wextra -Werror -fstack-protector-strong \ + -D_FORTIFY_SOURCE=2 -Wl,-z,relro,-z,now \ + "$KEYRING_BUILD/pam.c" -lpam -o "$KEYRING_BUILD/pam" +install -d -m 755 /usr/local/libexec /var/lib/coop +install -m 755 "$KEYRING_BUILD/pam" /usr/local/libexec/coop-codex-keyring-pam +install -m 755 "$KEYRING_BUILD/keyring.py" /usr/local/bin/codex-keyring +cat >/etc/pam.d/coop-codex-keyring <<'PAMEOF' +# Dedicated unprivileged keyring operation; never used to authenticate a login. +# pam_exec collects PAM_AUTHTOK; pam_gnome_keyring passes it to the existing +# control socket. No auto_start: the packaged systemd service owns the daemon. +auth required pam_exec.so expose_authtok /usr/bin/true +auth required pam_gnome_keyring.so +PAMEOF +chmod 644 /etc/pam.d/coop-codex-keyring +# Ubuntu's package enables a common-password hook. Coop uses only the dedicated +# service above; explicitly remove that global hook after package installation. +DEBIAN_FRONTEND=noninteractive pam-auth-update --package --remove gnome-keyring +# Offline-safe equivalent of enabling linger; works while building in a chroot. +install -d -m 755 /var/lib/systemd/linger +touch "/var/lib/systemd/linger/$GUEST_USER" +systemctl --global add-wants default.target gnome-keyring-daemon.service +systemctl --global enable gnome-keyring-daemon.socket +# Never overwrite a migration barrier on repeated installs in the same boot. +if [ ! -f /var/lib/coop/codex-keyring-install-boot ]; then + cat /proc/sys/kernel/random/boot_id >/var/lib/coop/codex-keyring-install-boot +fi +chmod 644 /var/lib/coop/codex-keyring-install-boot diff --git a/scripts/guest/codex-keyring.py b/scripts/guest/codex-keyring.py new file mode 100644 index 0000000..46c6fca --- /dev/null +++ b/scripts/guest/codex-keyring.py @@ -0,0 +1,435 @@ +#!/usr/bin/python3 +"""Guest-only singleton Secret Service readiness and user-triggered recovery. + +Never reads a password or takes over native Codex process ownership. Recovery +retires cached server authentication with the native stop command; the desktop +owns the next bootstrap. All state here is non-secret and scoped to this boot. +""" +import contextlib +import enum +import fcntl +import json +import os +from pathlib import Path +import select +import signal +import stat +import subprocess +import sys +import tempfile +import time + +import dbus + +SERVICE = 'org.freedesktop.Secret.Service' +COLLECTION = 'org.freedesktop.Secret.Collection' +ITEM = 'org.freedesktop.Secret.Item' +PROPERTIES = 'org.freedesktop.DBus.Properties' +ROOT = '/org/freedesktop/secrets' +LOGIN = ROOT + '/collection/login' +PREFIX = b'GnomeKeyring\n\r\0\n' + bytes(4) +CODEX = '/usr/local/bin/codex' +PAM = '/usr/local/libexec/coop-codex-keyring-pam' +MIGRATION = Path('/var/lib/coop/codex-keyring-install-boot') + + +class Candidate(enum.Enum): + ENCRYPTED = 'supported encrypted-format candidate' + + +class Failure(enum.Enum): + POLICY = 'managed ChatGPT keyring policy is required; restart with auth = "chatgpt"' + MIGRATION = 'guest support was installed this boot; restart the VM before unlocking' + BUS = 'user bus or packaged keyring service is unavailable' + BUSY = 'another unlock operation is busy; retry shortly' + FORMAT = 'plaintext or unsupported keyring storage; explicitly migrate and reauthenticate' + INVALID = 'existing keyring storage could not be loaded; preserve it for recovery' + CONFLICT = 'conflicting keyring storage or live collections; resolve histories explicitly' + MISSING = 'persistent login collection is missing or uninitialized; run coop codex-unlock in an interactive terminal' + ADOPTION = 'keyring storage needs interactive verification; run coop codex-unlock in an interactive terminal' + VERSION = 'installed GNOME Keyring version is unsupported; this helper requires 46.1' + LOCKED = 'login collection is locked; run coop codex-unlock in an interactive terminal' + ALIAS = 'default alias does not name the persistent login collection' + UNLOCK = 'keyring unlock failed; the password or encrypted storage may be invalid' + WRITE = 'disposable keyring write/read probe failed' + CLEANUP = 'disposable keyring item cleanup failed; rerun unlock after restoring the service' + CANCELLED = 'keyring operation cancelled' + GENERATION = 'keyring service changed during unlock; reconnect and retry' + NATIVE_BUSY = 'native Codex lifecycle lock or stop timed out; retry after desktop startup finishes' + NATIVE_OWNER = 'native Codex server ownership conflict; resolve it with the native daemon commands' + NATIVE = 'native Codex server recovery failed; no ready state was recorded' + TERMINATION = 'native stop did not retire the observed server; retry after concurrent startup finishes' + + +class Error(Exception): + def __init__(self, kind, secondary=None): + self.kind = kind + self.secondary = secondary + super().__init__(kind.value) + + +def run(args, timeout=20): + return subprocess.run(args, stdin=subprocess.DEVNULL, capture_output=True, + timeout=timeout, check=False) + + +def policy(home): + if os.environ.get('CODEX_HOME') or os.environ.get('XDG_DATA_HOME', str(home / '.local/share')) != str(home / '.local/share'): + raise Error(Failure.POLICY) + try: + with (home / '.codex/config.toml').open('rb') as config: + first = config.readline(128) + if first != b'cli_auth_credentials_store = "keyring"\n': + raise Error(Failure.POLICY) + if (home / '.codex/auth.json').exists(): + raise Error(Failure.POLICY) + except OSError as error: + raise Error(Failure.POLICY) from error + + +def storage(home): + """Classify storage without claiming ciphertext integrity.""" + directory = home / '.local/share/keyrings' + if directory.is_symlink(): + raise Error(Failure.CONFLICT) + if directory.exists(): + names = {p.name for p in directory.glob('*.keyring')} + if names - {'login.keyring'}: + raise Error(Failure.CONFLICT) + alias = directory / 'default' + if alias.is_symlink(): + raise Error(Failure.ALIAS) + if alias.exists(): + try: + with alias.open('rb') as stream: + if stream.read(7) not in (b'login', b'login\n'): + raise Error(Failure.ALIAS) + except OSError as error: + raise Error(Failure.ALIAS) from error + path = directory / 'login.keyring' + if path.is_symlink(): + raise Error(Failure.CONFLICT) + if not path.exists(): + return None + if not path.is_file(): + raise Error(Failure.INVALID) + if path.stat().st_size > 16 * 1024 * 1024: + raise Error(Failure.INVALID) + with path.open('rb') as stream: + if stream.read(len(PREFIX)) != PREFIX: + raise Error(Failure.FORMAT) + return Candidate.ENCRYPTED + + +@contextlib.contextmanager +def operation_lock(directory): + directory.mkdir(mode=0o700, exist_ok=True) + info = directory.lstat() + if not stat.S_ISDIR(info.st_mode) or info.st_uid != os.getuid() or info.st_mode & 0o077: + raise Error(Failure.BUS) + fd = os.open(directory / 'operation.lock', os.O_CREAT | os.O_RDWR | os.O_NOFOLLOW, 0o600) + try: + deadline = time.monotonic() + 15 + while True: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + break + except BlockingIOError: + if time.monotonic() >= deadline: + raise Error(Failure.BUSY) + time.sleep(0.1) + yield + finally: + os.close(fd) + + +def systemctl(operation): + try: + # An explicit retry may follow several failures inside systemd's start + # limit window. Reset the failure counter once; startup stays bounded. + reset = run(['/usr/bin/systemctl', '--user', 'reset-failed', + 'gnome-keyring-daemon.service'], timeout=10) + if reset.returncode: + raise Error(Failure.BUS) + result = run(['/usr/bin/systemctl', '--user', operation, + 'gnome-keyring-daemon.service'], timeout=20) + except subprocess.TimeoutExpired as error: + raise Error(Failure.BUS) from error + if result.returncode: + raise Error(Failure.BUS) + + +class Keyring: + def __init__(self, runtime): + self.bus = None + try: + self.bus = dbus.bus.BusConnection('unix:path=' + str(runtime / 'bus')) + bus_object = self.bus.get_object('org.freedesktop.DBus', '/org/freedesktop/DBus', introspect=False) + self.server = str(bus_object.GetId(dbus_interface='org.freedesktop.DBus', timeout=5)) + deadline = time.monotonic() + 5 + while not bus_object.NameHasOwner('org.freedesktop.secrets', dbus_interface='org.freedesktop.DBus', timeout=5): + if time.monotonic() >= deadline: + raise Error(Failure.BUS) + time.sleep(0.05) + self.owner = str(bus_object.GetNameOwner('org.freedesktop.secrets', dbus_interface='org.freedesktop.DBus', timeout=5)) + owner_pid = int(bus_object.GetConnectionUnixProcessID(self.owner, dbus_interface='org.freedesktop.DBus', timeout=5)) + managed = run(['/usr/bin/systemctl', '--user', 'show', 'gnome-keyring-daemon.service', '-p', 'MainPID', '--value']) + if managed.returncode or managed.stdout.strip() != str(owner_pid).encode(): + raise Error(Failure.BUS) + except (dbus.DBusException, Error) as error: + if self.bus is not None: + self.bus.close() + raise Error(Failure.BUS) from error + + def call(self, path, interface, method, *args): + # Bind to the observed unique owner: never auto-activate a replacement + # halfway through a probe or accidentally delete its items. + obj = self.bus.get_object(self.owner, path, introspect=False) + return obj.get_dbus_method(method, interface)(*args, timeout=5) + + def generation(self, boot): + if str(self.bus.get_name_owner('org.freedesktop.secrets')) != self.owner: + raise Error(Failure.GENERATION) + return [boot, self.server, self.owner] + + def collection(self, candidate): + collections = self.call(ROOT, PROPERTIES, 'Get', SERVICE, 'Collections') + persistent = set(map(str, collections)) - {ROOT + '/collection/session'} + if persistent - {LOGIN}: + raise Error(Failure.CONFLICT) + alias = str(self.call(ROOT, SERVICE, 'ReadAlias', 'default')) + if alias not in ('/', LOGIN): + raise Error(Failure.ALIAS) + if LOGIN not in persistent: + if candidate is not None: + raise Error(Failure.INVALID) + if alias != '/': + raise Error(Failure.ALIAS) + return None + if candidate is None: + raise Error(Failure.CONFLICT) + if alias != LOGIN: + raise Error(Failure.ALIAS) + return bool(self.call(LOGIN, PROPERTIES, 'Get', COLLECTION, 'Locked')) + + def probe(self): + import uuid + attributes = dbus.Dictionary({'service': 'coop-codex-readiness', 'operation': uuid.uuid4().hex}, signature='ss') + session = None + attempted = False + original = None + try: + _, session = self.call(ROOT, SERVICE, 'OpenSession', 'plain', dbus.String('', variant_level=1)) + properties = dbus.Dictionary({ + ITEM + '.Label': 'coop disposable readiness probe', + ITEM + '.Attributes': attributes, + }, signature='sv') + secret = dbus.Struct((session, dbus.ByteArray(b''), dbus.ByteArray(b'coop-probe'), 'text/plain'), signature='oayays') + attempted = True + item, prompt = self.call(LOGIN, COLLECTION, 'CreateItem', properties, secret, False) + if str(prompt) != '/' or str(item) == '/': + raise Error(Failure.WRITE) + read = self.call(item, ITEM, 'GetSecret', session) + if bytes(read[2]) != b'coop-probe': + raise Error(Failure.WRITE) + except (dbus.DBusException, Error, KeyboardInterrupt) as error: + original = error + finally: + try: + if attempted: + # Also cleans a successfully created item after a lost reply. + unlocked, locked = self.call(ROOT, SERVICE, 'SearchItems', attributes) + for item in [*unlocked, *locked]: + prompt = self.call(item, ITEM, 'Delete') + if str(prompt) != '/': + raise Error(Failure.CLEANUP) + unlocked, locked = self.call(ROOT, SERVICE, 'SearchItems', attributes) + if unlocked or locked: + raise Error(Failure.CLEANUP) + if session is not None: + self.call(session, 'org.freedesktop.Secret.Session', 'Close') + except (dbus.DBusException, Error) as cleanup: + if original: + # Preserve the first failure as well as the cleanup failure. + primary = Failure.CANCELLED if isinstance(original, KeyboardInterrupt) else Failure.WRITE + raise Error(primary, Failure.CLEANUP) from cleanup + raise Error(Failure.CLEANUP) from cleanup + if isinstance(original, KeyboardInterrupt): + raise original + if original: + raise Error(Failure.WRITE) from original + + +def retire_server(home): + """Use native ownership checks; pidfd observes termination but never signals. + + Do not start/bootstrap here: native failed-start rollback cannot be scoped + to our invocation when the desktop also bootstraps. Retirement alone clears + the stale cache; the desktop owns subsequent startup and updater lifecycle. + """ + descriptor = None + try: + record = home / '.codex/app-server-daemon/app-server.pid' + if record.exists(): + try: + pid = json.loads(record.read_text())['pid'] + if not isinstance(pid, int) or pid <= 0: + raise Error(Failure.NATIVE_OWNER) + descriptor = os.pidfd_open(pid) + except ProcessLookupError: + pass + except (ValueError, KeyError, OSError) as error: + raise Error(Failure.NATIVE_OWNER) from error + try: + result = run([CODEX, 'app-server', 'daemon', 'stop'], timeout=90) + except subprocess.TimeoutExpired as error: + raise Error(Failure.NATIVE_BUSY) from error + if result.returncode: + if b'not managed by codex' in result.stderr: + raise Error(Failure.NATIVE_OWNER) + if b'operation lock' in result.stderr: + raise Error(Failure.NATIVE_BUSY) + raise Error(Failure.NATIVE) + try: + status = json.loads(result.stdout)['status'] + except (ValueError, KeyError) as error: + raise Error(Failure.NATIVE) from error + if status not in ('stopped', 'notRunning'): + raise Error(Failure.NATIVE) + if descriptor is not None and not select.select([descriptor], [], [], 5)[0]: + raise Error(Failure.TERMINATION) + finally: + if descriptor is not None: + os.close(descriptor) + + +def pam_unlock(creating): + args = [PAM] + (['--create'] if creating else []) + process = subprocess.Popen(args) + try: + if process.wait(timeout=160): + raise Error(Failure.UNLOCK) + finally: + if process.poll() is None: + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait(timeout=5) + + +def record_success(path, value): + fd, temporary = tempfile.mkstemp(dir=path.parent, prefix='.ready-') + try: + with os.fdopen(fd, 'w') as stream: + json.dump(value, stream) + stream.flush() + os.fsync(stream.fileno()) + os.replace(temporary, path) + finally: + Path(temporary).unlink(missing_ok=True) + + +def unlock(home, runtime, boot): + policy(home) + if not MIGRATION.exists() or MIGRATION.read_text().strip() == boot: + raise Error(Failure.MIGRATION) + with operation_lock(runtime / 'coop-codex'): + path = runtime / 'coop-codex/ready.json' + keyring = None + try: + try: + with path.open() as stream: + prior = json.loads(stream.read(1024)) + except (OSError, ValueError): + prior = None + candidate = storage(home) # Before service activation, prompts or writes. + version = run(['/usr/bin/gnome-keyring-daemon', '--version']) + if version.returncode or version.stdout.splitlines()[0:1] != [b'gnome-keyring-daemon: 46.1']: + raise Error(Failure.VERSION) + systemctl('start') + keyring = Keyring(runtime) + generation = keyring.generation(boot) + adopted = prior == generation + if not adopted: + if not sys.stdin.isatty(): + raise Error(Failure.MISSING if candidate is None else Failure.ADOPTION) + # Never let an old success survive any failed adoption/recovery. + path.unlink(missing_ok=True) + # Reject conflicting live state before replacing the service. + try: + keyring.collection(candidate) + except Error as error: + # Only the fresh daemon can classify an existing file as + # unloadable; the current service may predate its adoption. + if error.kind is not Failure.INVALID: + raise + retire_server(home) + keyring.bus.close() + systemctl('restart') + keyring = Keyring(runtime) + generation = keyring.generation(boot) + locked = keyring.collection(candidate) + if locked is not False: + path.unlink(missing_ok=True) + if not sys.stdin.isatty(): + raise Error(Failure.MISSING if locked is None else Failure.LOCKED) + pam_unlock(candidate is None) + candidate = storage(home) + if candidate is None or keyring.collection(candidate) is not False: + raise Error(Failure.UNLOCK) + keyring.probe() + if not adopted or locked is not False: + retire_server(home) + if keyring.collection(storage(home)) is not False: + raise Error(Failure.LOCKED) + if keyring.generation(boot) != generation: + raise Error(Failure.GENERATION) + record_success(path, generation) + # A concurrent service crash must not leave a successful record. + if keyring.generation(boot) != generation: + path.unlink(missing_ok=True) + raise Error(Failure.GENERATION) + except BaseException: + path.unlink(missing_ok=True) + raise + finally: + if keyring is not None: + keyring.bus.close() + + +def main(): + if len(sys.argv) != 1: + return 2 + os.umask(0o077) + home = Path.home() + runtime = Path('/run/user') / str(os.getuid()) + if os.getuid() == 0 or os.environ.get('XDG_RUNTIME_DIR') != str(runtime): + raise Error(Failure.BUS) + # Standard PAM/systemd environment, never a private terminal bus. + os.environ['DBUS_SESSION_BUS_ADDRESS'] = 'unix:path=' + str(runtime / 'bus') + os.environ['GNOME_KEYRING_CONTROL'] = str(runtime / 'keyring') + boot = Path('/proc/sys/kernel/random/boot_id').read_text().strip() + unlock(home, runtime, boot) + print('Guest keyring ready. Connect or reconnect the desktop over SSH.') + return 0 + + +if __name__ == '__main__': + def cancelled(signum, frame): + raise KeyboardInterrupt + for signum in (signal.SIGTERM, signal.SIGHUP): + signal.signal(signum, cancelled) + try: + sys.exit(main()) + except KeyboardInterrupt: + print('codex-keyring: cancelled; rerun unlock to retry', file=sys.stderr) + sys.exit(130) + except Error as error: + print('codex-keyring: ' + error.kind.value + + ('; ' + error.secondary.value if error.secondary else ''), file=sys.stderr) + sys.exit(130 if error.kind is Failure.CANCELLED else 1) + except (dbus.DBusException, OSError, subprocess.TimeoutExpired): + print('codex-keyring: guest operation failed or timed out; rerun unlock to retry', file=sys.stderr) + sys.exit(1) diff --git a/src/backend.rs b/src/backend.rs index 20bca1c..3cd1282 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -1848,7 +1848,8 @@ pub fn ensure_codex_account_guest_support(target: &SshTarget) -> Result<()> { RemoteCommand::new() .literal("test -x ") .arg(crate::guest::codex_account_bin()) - .literal(" && command -v dbus-run-session >/dev/null 2>&1") + .literal(" && test -x /usr/local/bin/codex-keyring") + .literal(" && test -x /usr/local/libexec/coop-codex-keyring-pam") .literal(" && command -v gnome-keyring-daemon >/dev/null 2>&1") .literal(" && command -v secret-tool >/dev/null 2>&1"), ); @@ -1857,16 +1858,8 @@ pub fn ensure_codex_account_guest_support(target: &SshTarget) -> Result<()> { } bail!( - "Codex ChatGPT account auth requires guest Secret Service support, \ - but this VM image does not have it.\n\ - Rebuild the image with `coop setup --rebuild` (or \ - `coop setup --image --rebuild` for a named image).\n\ - A rebuild does not touch this VM's existing guest disk, and a \ - restart reuses it. To pick up the rebuilt image, either \ - `coop restore --image --reprovision` (in place, \ - keeping the instance), or destroy and recreate the VM. \ - Alternatively, install `dbus-user-session`, `gnome-keyring`, and \ - `libsecret-tools` in the running guest by hand." + "Codex ChatGPT account auth requires shared guest keyring support.\n\ + Run `coop codex-unlock ` to install it in place, then stop and start the VM." ); } diff --git a/src/commands/codex.rs b/src/commands/codex.rs new file mode 100644 index 0000000..3a5701c --- /dev/null +++ b/src/commands/codex.rs @@ -0,0 +1,51 @@ +//! Guest `ChatGPT` keyring installation and interactive unlock. +use anyhow::{Result, bail}; + +use crate::backend::{self, PlatformBackend}; +use crate::commands::open_ssh_session; +use crate::config::{CoopConfig, InstanceName}; +use crate::guest; +use crate::model_state::ModelState; +use crate::remote_command::RemoteCommand; +use crate::ssh; + +pub(crate) fn cmd_codex_unlock( + be: &PlatformBackend, + cfg: &CoopConfig, + name: Option<&InstanceName>, +) -> Result<()> { + if !cfg.codex.auth.uses_chatgpt_account() { + bail!("`coop codex-unlock` requires [codex] auth = \"chatgpt\""); + } + let inst = cfg.resolve_instance(name)?; + let model_state = ModelState::load_or_default(&inst)?; + backend::ensure_codex_remote_auth_consistent(cfg, &inst, &model_state)?; + let session = open_ssh_session(be, cfg, name)?; + backend::ensure_codex_keyring_configured(&session.target)?; + if !session.target.exec_ok(RemoteCommand::new().literal( + "test -x /usr/local/bin/codex-keyring \ + && test -x /usr/local/libexec/coop-codex-keyring-pam \ + && test -f /var/lib/coop/codex-keyring-install-boot", + )) { + // All installer bytes are embedded trusted source. SSH user is a + // validated newtype and crosses the shell boundary via arg(). + let script = format!( + "{}\n{}\n{}", + guest::SCRIPT_CODEX_KEYRING_MIGRATE, + guest::SCRIPT_CODEX_KEYRING, + guest::SCRIPT_CODEX_ACCOUNT, + ); + tracing::info!("Installing shared guest keyring support; a VM restart is required"); + session.target.exec_with_stdin( + RemoteCommand::new() + .literal("sudo env ") + .arg(format!("GUEST_USER={}", session.target.user)) + .literal(" bash -s"), + script.into_bytes(), + )?; + bail!( + "Guest keyring support installed. Stop and start this VM, then rerun `coop codex-unlock`." + ); + } + ssh::run_interactive_checked(&session, &["/usr/local/bin/codex-keyring".into()]) +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 88bb1fd..5b9086c 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -4,6 +4,8 @@ //! re-exports the `pub(crate)` dispatch surface [`crate::run`] consumes //! and holds the cross-domain orchestration helpers the submodules share. +mod codex; +pub(crate) use codex::cmd_codex_unlock; mod admin; mod agent; mod devcontainer; diff --git a/src/guest.rs b/src/guest.rs index 895bf8e..39f6d7c 100644 --- a/src/guest.rs +++ b/src/guest.rs @@ -120,7 +120,7 @@ pub fn codex_bin() -> GuestPath { GuestPath::new("/usr/local/bin/codex") } -/// Wrapper that runs Codex with a guest Linux Secret Service session. +/// Wrapper that runs Codex against the shared guest Secret Service. pub fn codex_account_bin() -> GuestPath { GuestPath::new("/usr/local/bin/codex-account") } @@ -170,7 +170,7 @@ impl From for String { /// build shell commands or inspect the chroot get path semantics for /// free (and the `/usr/bin/docker`/`/usr/bin/gh` entries can't be /// mistaken for host paths). -pub fn required_guest_binaries(user: &GuestUser) -> [GuestPath; 8] { +pub fn required_guest_binaries(user: &GuestUser) -> [GuestPath; 11] { [ GuestPath::new("/usr/bin/docker"), GuestPath::new("/usr/bin/gh"), @@ -179,8 +179,11 @@ pub fn required_guest_binaries(user: &GuestUser) -> [GuestPath; 8] { codex_account_bin(), // The Secret Service stack `codex-account` drives. Checking the // wrapper alone proves nothing — the provision script always writes - // it — so verify the three tools its BASE_PACKAGES entries install. - GuestPath::new("/usr/bin/dbus-run-session"), + // it — so verify its interpreter, compiled helper, and service tools. + GuestPath::new("/usr/bin/systemctl"), + GuestPath::new("/usr/bin/python3"), + GuestPath::new("/usr/local/bin/codex-keyring"), + GuestPath::new("/usr/local/libexec/coop-codex-keyring-pam"), GuestPath::new("/usr/bin/gnome-keyring-daemon"), GuestPath::new("/usr/bin/secret-tool"), ] @@ -230,6 +233,23 @@ pub const SCRIPT_CLAUDE_CODE: &str = include_str!("../scripts/guest/claude-code. pub const SCRIPT_CODEX: &str = include_str!("../scripts/guest/codex.sh"); pub const SCRIPT_CODEX_ACCOUNT: &str = include_str!("../scripts/guest/codex-account.sh"); +pub const SCRIPT_CODEX_KEYRING_MIGRATE: &str = + include_str!("../scripts/guest/codex-keyring-migrate.sh"); + +/// Shared headless authentication support, compiled inside either Linux guest. +/// File installation does not start services; migration requires a guest reboot. +pub const SCRIPT_CODEX_KEYRING: &str = concat!( + "(\nset -euo pipefail\nKEYRING_BUILD=$(mktemp -d)\n", + "trap 'rm -rf \"$KEYRING_BUILD\"' EXIT\n", + "cat >\"$KEYRING_BUILD/pam.c\" <<'COOPPAMEOF'\n", + include_str!("../scripts/guest/codex-keyring-pam.c"), + "COOPPAMEOF\ncat >\"$KEYRING_BUILD/keyring.py\" <<'COOPKEYRINGEOF'\n", + include_str!("../scripts/guest/codex-keyring.py"), + "COOPKEYRINGEOF\n", + include_str!("../scripts/guest/codex-keyring-setup.sh"), + ")\n", +); + /// Packages installed into every golden image. /// /// `dbus-user-session`, `gnome-keyring`, and `libsecret-tools` back the @@ -263,6 +283,11 @@ pub const BASE_PACKAGES: &[&str] = &[ "zip", "file", "gnome-keyring", + "libpam-gnome-keyring", + "libpam0g-dev", + "libpam-systemd", + "python3", + "python3-dbus", "less", "libsecret-tools", ]; @@ -484,128 +509,8 @@ pub fn collect_codex_baked_lists(cfg: &CoopConfig) -> (Vec, Vec) mod tests { use super::*; - #[test] - fn codex_account_script_uses_secret_service() { - assert!( - SCRIPT_CODEX_ACCOUNT.contains("cat >/usr/local/bin/codex-account"), - "Codex account wrapper should be installed in the guest image", - ); - for expected in [ - // Anchored on the re-exec itself: a bare "dbus-run-session" also - // matches the `command -v` guard and its die message, so it would - // pass even with the re-exec deleted. - "exec dbus-run-session -- ", - "gnome-keyring-daemon --unlock", - "secret-tool store", - // The probe item must be removed again, not left in the keyring. - "secret-tool clear", - ] { - assert!( - SCRIPT_CODEX_ACCOUNT.contains(expected), - "Codex account wrapper is missing {expected:?}", - ); - } - } - - #[test] - fn codex_account_script_unlocks_before_touching_the_bus() { - // `gnome-keyring-daemon --unlock` only creates and unlocks the login - // collection when it is the process that starts the daemon. Once any - // daemon owns `org.freedesktop.secrets` — including one the probe's - // `secret-tool` would D-Bus-activate — the unlock is handed to the - // graphical gcr-prompter, which cannot run on a headless guest, and - // ChatGPT auth fails on every invocation. So the unlock must be the - // first thing the main flow does. - assert!( - SCRIPT_CODEX_ACCOUNT.contains(" unlock_keyring\n probe_keyring \\"), - "on a bus this wrapper created, the unlock must precede the probe", - ); - assert!( - !SCRIPT_CODEX_ACCOUNT.contains("gnome-keyring-daemon --start"), - "starting the daemon before the unlock is what breaks the unlock", - ); - } - - #[test] - fn codex_account_script_reuses_an_inherited_unlocked_session() { - // A nested codex-account (an in-guest agent shelling out to - // `codex-account` / `codex-yolo`) inherits the bus and its unlocked - // keyring. Unlocking again there hits the very gcr-prompter trap the - // ordering above exists to avoid, so the nested call must probe and - // reuse rather than re-unlock — and the outer call must publish the - // marker that says so. - assert!( - SCRIPT_CODEX_ACCOUNT.contains("export COOP_CODEX_ACCOUNT_UNLOCKED=1"), - "the unlocking call must mark the session as reusable", - ); - assert!( - SCRIPT_CODEX_ACCOUNT.contains("\"${COOP_CODEX_ACCOUNT_UNLOCKED:-0}\" = \"1\""), - "a nested call must branch on the inherited-session marker", - ); - } - - #[test] - fn codex_account_script_guards_against_dbus_reexec_recursion() { - // `exec dbus-run-session -- "$0"` re-runs this same script. Without the - // env guard around it that is an unbounded fork loop inside the guest, - // and no other test would notice. - assert!( - SCRIPT_CODEX_ACCOUNT.contains("\"${COOP_CODEX_ACCOUNT_DBUS:-0}\" != \"1\""), - "the re-exec must be guarded, or it recurses forever", - ); - assert!( - SCRIPT_CODEX_ACCOUNT.contains("export COOP_CODEX_ACCOUNT_DBUS=1"), - "the guard must be set before the re-exec, or it never takes effect", - ); - } - - #[test] - fn codex_account_script_bounds_its_secret_service_calls() { - // A wedged Secret Service must fail the launch, not hang it. The - // tool-presence loop checks for `timeout`; these pin that it is - // actually used on both probe calls. - assert_eq!( - SCRIPT_CODEX_ACCOUNT - .matches("timeout 5 secret-tool") - .count(), - 2, - "both probe secret-tool calls must be time-bounded", - ); - assert!( - SCRIPT_CODEX_ACCOUNT.contains("timeout 30 gnome-keyring-daemon --unlock"), - "the unlock must be time-bounded too: it runs inside a command \ - substitution, which waits for EOF rather than for exit", - ); - } - - #[test] - fn codex_account_script_requires_a_tty_before_prompting() { - // Without this guard `read -rsp` blocks forever on a non-interactive - // session (agent bootstrap, `coop exec`), turning a clear failure into - // a hang. - assert!( - SCRIPT_CODEX_ACCOUNT.contains("[ ! -t 0 ]"), - "the wrapper must refuse to prompt without a TTY", - ); - } - - #[test] - fn codex_account_script_captures_probe_stderr() { - // The redirect order is load-bearing and easy to "correct" wrongly: - // `2>&1 >/dev/null` captures stderr because `2>&1` binds while stdout - // is still the substitution pipe. The tidier-looking `>/dev/null 2>&1` - // sends both to /dev/null, leaving PROBE_ERROR always empty and the - // die message back to guessing at the password. - assert!( - SCRIPT_CODEX_ACCOUNT.contains("2>&1 >/dev/null"), - "the probe must capture secret-tool's stderr, not discard it", - ); - assert!( - SCRIPT_CODEX_ACCOUNT.contains("${PROBE_ERROR:+"), - "the die message must report the captured cause when there is one", - ); - } - + // Runtime wrapper and singleton behavior is tested by the Python suites; + // avoid assertions that merely repeat the shell implementation. #[test] fn codex_account_script_guards_alternate_home_and_otherwise_passes_through() { // Every Codex entry point routes through the wrapper, so it must be a @@ -635,21 +540,6 @@ mod tests { ); } - #[test] - fn codex_account_script_confirms_a_newly_created_keyring_password() { - // A fresh guest has no keyring, so the prompt creates one; an - // unconfirmed typo would lock credentials behind an unreproducible - // password. - assert!( - SCRIPT_CODEX_ACCOUNT.contains("Confirm keyring password: "), - "wrapper should confirm the password when creating a keyring", - ); - assert!( - SCRIPT_CODEX_ACCOUNT.contains("this VM has no guest keyring yet"), - "wrapper should explain that the first prompt chooses a password", - ); - } - #[test] fn collect_baked_lists_merges_global_and_profile_entries() { let mut cfg = CoopConfig::default(); @@ -862,7 +752,9 @@ mod tests { // The wrapper is written unconditionally by the provision script, so // verifying it alone cannot catch the packages failing to install. for tool in [ - "/usr/bin/dbus-run-session", + "/usr/bin/systemctl", + "/usr/local/bin/codex-keyring", + "/usr/local/libexec/coop-codex-keyring-pam", "/usr/bin/gnome-keyring-daemon", "/usr/bin/secret-tool", ] { diff --git a/src/lib.rs b/src/lib.rs index 2a3e2be..d3ef752 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -392,6 +392,15 @@ enum Commands { #[arg(trailing_var_arg = true, allow_hyphen_values = true)] args: Vec, }, + /// Unlock the shared guest keyring for Codex desktop SSH connections + CodexUnlock { + /// Instance name (required if multiple instances exist) + #[arg( + value_parser = config::InstanceName::new, + add = ArgValueCandidates::new(completions::instance_candidates), + )] + name: Option, + }, /// Gracefully stop the VM Stop { /// Instance name (required if multiple instances exist) @@ -1349,6 +1358,7 @@ pub fn run() -> Result<()> { let claude_bin = guest::GuestUser::new(sess.target.user.as_ref())?.claude_bin(); ssh::run_interactive(&sess, &prepend_binary(claude_bin.as_ref(), args)) } + Commands::CodexUnlock { name } => commands::cmd_codex_unlock(&be, &cfg, name.as_ref()), Commands::Codex { name, ask, args } => { let sess = open_ssh_session(&be, &cfg, name.as_ref())?; let args = codex_launch_args(ask, args); @@ -2074,6 +2084,21 @@ token = "test-pat" assert_eq!(args, vec!["--model", "gpt-5"]); } + #[test] + fn codex_unlock_preserves_vm_named_unlock() { + let cli = parse(&["codex", "unlock"]); + let super::Commands::Codex { name, args, .. } = cli.command else { + panic!("expected ordinary Codex launch"); + }; + assert_eq!(name.unwrap().as_str(), "unlock"); + assert!(args.is_empty()); + let cli = parse(&["codex-unlock", "unlock"]); + let super::Commands::CodexUnlock { name } = cli.command else { + panic!("expected CodexUnlock"); + }; + assert_eq!(name.unwrap().as_str(), "unlock"); + } + #[test] fn codex_ask_flag_parses() { let cli = parse(&["codex", "myvm", "--ask", "--", "--model", "gpt-5"]); diff --git a/src/lima.rs b/src/lima.rs index f701f5c..e587733 100644 --- a/src/lima.rs +++ b/src/lima.rs @@ -1493,6 +1493,7 @@ fn compose_provision_script( // Codex CLI (native per-user package with a system compatibility link) s.push_str(SCRIPT_CODEX); s.push('\n'); + s.push_str(crate::guest::SCRIPT_CODEX_KEYRING); s.push_str(SCRIPT_CODEX_ACCOUNT); s.push('\n'); diff --git a/src/setup.rs b/src/setup.rs index d0702f5..122226a 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -888,6 +888,7 @@ fn compose_recipe( s.push_str(SCRIPT_CLAUDE_CODE); // Codex's native installer keeps the full package under the guest user's home. s.push_str(SCRIPT_CODEX); + s.push_str(crate::guest::SCRIPT_CODEX_KEYRING); s.push_str(SCRIPT_CODEX_ACCOUNT); s diff --git a/src/ssh.rs b/src/ssh.rs index 3e115ae..1dce8cd 100644 --- a/src/ssh.rs +++ b/src/ssh.rs @@ -108,6 +108,22 @@ pub fn run_interactive(session: &SshSession, command: &[String]) -> Result<()> { Ok(()) } +/// Interactive authentication must propagate refusal/cancellation to the host. +pub fn run_interactive_checked(session: &SshSession, command: &[String]) -> Result<()> { + let args = interactive_ssh_args(session, render_remote(command)); + let status = Command::new("ssh") + .args(&args) + .envs(session.env.as_envs()) + .env("TERM", guest_term()) + .status() + .context("Failed to launch SSH")?; + if !status.success() { + restore_terminal(); + anyhow::bail!("Guest authentication operation failed ({status})"); + } + Ok(()) +} + /// Run a command non-interactively over SSH (no PTY). /// /// Propagates the remote command's exit code via the process exit code. diff --git a/tests/codex-keyring-pam-probe.c b/tests/codex-keyring-pam-probe.c new file mode 100644 index 0000000..323d8f2 --- /dev/null +++ b/tests/codex-keyring-pam-probe.c @@ -0,0 +1,44 @@ +#define _GNU_SOURCE +#include +#include +#include +#include + +/* Test helper for test-codex-desktop-prototype.py, not a login authenticator. + * Uses a temporary PAM config directory; never changes the system PAM stack. + * Read only disposable fixture passwords from stdin, never argv or env. */ +static int converse(int count, const struct pam_message **messages, + struct pam_response **out, void *unused) { + (void)unused; + if (count != 1 || messages[0]->msg_style != PAM_PROMPT_ECHO_OFF) + return PAM_CONV_ERR; + char *password = NULL; + size_t capacity = 0; + ssize_t length = getline(&password, &capacity, stdin); + if (length < 0) { free(password); return PAM_CONV_ERR; } + if (length && password[length - 1] == '\n') password[--length] = '\0'; + if (!length) { free(password); return PAM_CONV_ERR; } + *out = calloc(1, sizeof(struct pam_response)); + if (!*out) { explicit_bzero(password, capacity); free(password); return PAM_BUF_ERR; } + (*out)->resp = password; + return PAM_SUCCESS; +} + +int main(int argc, char **argv) { + if (argc != 4) return 2; + struct pam_conv conv = {converse, NULL}; + pam_handle_t *handle = NULL; + int result = pam_start_confdir("coop-keyring", argv[1], &conv, argv[2], &handle); + if (result != PAM_SUCCESS) return 3; + char *environment = NULL; + if (asprintf(&environment, "GNOME_KEYRING_CONTROL=%s", argv[3]) < 0) { + pam_end(handle, PAM_BUF_ERR); + return 4; + } + result = pam_putenv(handle, environment); + free(environment); + if (result == PAM_SUCCESS) result = pam_authenticate(handle, 0); + printf("PAM result: %d\n", result); + pam_end(handle, result); + return result == PAM_SUCCESS ? 0 : 1; +} diff --git a/tests/integration.sh b/tests/integration.sh index b3c9811..2df3a65 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1240,7 +1240,7 @@ test_codex_account_auth_support() { # The wrapper is written unconditionally by the provision script, so the # packages behind it are what actually need asserting. local tool - for tool in dbus-run-session gnome-keyring-daemon secret-tool; do + for tool in systemctl gnome-keyring-daemon secret-tool codex-keyring /usr/local/libexec/coop-codex-keyring-pam; do if guest_exec command -v "$tool"; then pass "guest Secret Service tool present: $tool" else @@ -1279,14 +1279,8 @@ test_codex_account_auth_support() { "stderr: $(guest_stderr)" fi - # 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. + # A scratch home selecting keyring mode must fail the managed-home policy + # instead of silently passing through to Codex and writing plaintext auth. 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" \ @@ -1302,10 +1296,10 @@ test_codex_account_auth_support() { /usr/local/bin/codex-account --version + session + unix:tmpdir=/tmp + EXTERNAL + + + + + + +''') + child = subprocess.Popen( + ['dbus-daemon', '--config-file=' + str(config), '--nofork', + '--address=' + self.env['DBUS_SESSION_BUS_ADDRESS']], + env=self.env, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + self.children.append(child) + deadline = time.monotonic() + 5 + while not (self.run / 'bus').exists(): + if child.poll() is not None or time.monotonic() >= deadline: + raise RuntimeError('private D-Bus startup failed') + time.sleep(0.02) + self.bus = dbus.bus.BusConnection(self.env['DBUS_SESSION_BUS_ADDRESS']) + return self + + def unlock(self, password): + if self.keyring is not None: + raise RuntimeError('replace the owned keyring before unlocking again') + args = ['gnome-keyring-daemon', '--unlock', '--components=secrets', + '--control-directory=' + self.env['GNOME_KEYRING_CONTROL']] + self.keyring = subprocess.Popen( + [*args, '--foreground'], env=self.env, stdin=subprocess.PIPE, + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + self.children.append(self.keyring) + self.keyring.stdin.write(password.encode()) + self.keyring.stdin.close() + deadline = time.monotonic() + 5 + while self.state() != 'unlocked': + if self.keyring.poll() is not None: + raise RuntimeError('keyring did not expose its login collection') + if time.monotonic() >= deadline: + if self.state() == 'locked': + return + raise RuntimeError('keyring did not expose its login collection') + time.sleep(0.02) + + def keyring_owner_pid(self): + bus = self.bus.get_object('org.freedesktop.DBus', '/org/freedesktop/DBus', + introspect=False) + return int(bus.GetConnectionUnixProcessID('org.freedesktop.secrets', + dbus_interface='org.freedesktop.DBus', timeout=2)) + + def state(self): + import dbus + if not self.bus.name_has_owner('org.freedesktop.secrets'): + return 'unavailable' + service = self.bus.get_object('org.freedesktop.secrets', + '/org/freedesktop/secrets', introspect=False) + collection = service.ReadAlias('default', + dbus_interface='org.freedesktop.Secret.Service', + timeout=2) + if collection == '/': + return 'missing' + if collection != '/org/freedesktop/secrets/collection/login': + return 'unexpected-default' + try: + obj = self.bus.get_object('org.freedesktop.secrets', collection, + introspect=False) + locked = obj.Get('org.freedesktop.Secret.Collection', 'Locked', + dbus_interface='org.freedesktop.DBus.Properties', timeout=2) + except dbus.DBusException as error: + if error.get_dbus_name() == 'org.freedesktop.DBus.Error.UnknownMethod': + return 'missing' + raise + return 'locked' if locked else 'unlocked' + + def lock(self): + import dbus + service = self.bus.get_object('org.freedesktop.secrets', + '/org/freedesktop/secrets', introspect=False) + service.Lock(dbus.Array(['/org/freedesktop/secrets/collection/login'], signature='o'), + dbus_interface='org.freedesktop.Secret.Service', timeout=2) + + def restart_keyring(self, password): + # A production owner must first retire every associated app-server. + # These fixtures call this only when no app-server is running. + self.keyring.terminate() + self.keyring.wait(timeout=5) + self.keyring = None + self.unlock(password) + + def secret(self, action, value=None, account='disposable'): + args = ['secret-tool', action] + if action == 'store': + args.append('--label=coop desktop disposable probe') + result = subprocess.run( + [*args, 'service', 'coop-desktop-prototype', 'account', account], + env=self.env, input=value, text=True, capture_output=True, timeout=5) + if result.returncode == 0: + return result.stdout.strip() + if action == 'lookup' and result.returncode == 1 and not result.stderr: + # secret-tool also exits nonzero when its bus or collection fails. + # Prove absence against the live service before treating it as logout. + if self.state() != 'unlocked': + raise RuntimeError('lookup failed while the collection was not unlocked') + service = self.bus.get_object('org.freedesktop.secrets', + '/org/freedesktop/secrets', introspect=False) + unlocked, locked = service.SearchItems( + {'service': 'coop-desktop-prototype', 'account': account}, + dbus_interface='org.freedesktop.Secret.Service', timeout=2) + if not unlocked and not locked: + return None + raise RuntimeError(f'disposable {action} failed ({result.returncode}): {result.stderr}') + + def close(self): + if self.bus is not None: + self.bus.close() + for child in reversed(self.children): + if child.poll() is None: + child.terminate() + try: + child.wait(timeout=5) + except subprocess.TimeoutExpired: + child.kill() + child.wait(timeout=5) + + +@unittest.skipUnless(os.environ.get('COOP_TEST_CODEX'), + 'set COOP_TEST_CODEX to run the real-process prototype') +class DesktopPrototypeTests(unittest.TestCase): + def setUp(self): + for tool in ['dbus-daemon', 'gnome-keyring-daemon', 'secret-tool']: + self.assertIsNotNone(shutil.which(tool), f'missing prerequisite: {tool}') + self.temp = tempfile.TemporaryDirectory(prefix='c480-') + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.binary = Path(os.environ['COOP_TEST_CODEX']).resolve() + self.home = self.root / '.codex' + self.home.mkdir() + (self.home / 'config.toml').write_text( + 'cli_auth_credentials_store = "keyring"\n[features]\nplugins = false\n') + # Native daemon management requires the installer layout. Reuse the + # whole package so bundled components resolve beside the executable. + self.assertEqual(self.binary.parent.name, 'bin', 'use a native Codex install') + standalone = self.home / 'packages/standalone' + standalone.mkdir(parents=True) + (standalone / 'current').symlink_to(self.binary.parent.parent, + target_is_directory=True) + + def session(self, name, data=None): + session = Session(self.root, name, data or self.root / 'data') + self.addCleanup(session.close) + return session.open() + + def native(self, session, operation): + result = subprocess.run( + [str(self.binary), 'app-server', 'daemon', operation], + env=session.env, cwd=self.root, text=True, capture_output=True, timeout=30) + self.assertEqual(result.returncode, 0, result.stderr) + return json.loads(result.stdout) + + def stop_native(self, session): + record = self.home / 'app-server-daemon/app-server.pid' + process_fd = None + if record.exists(): + try: + process_fd = os.pidfd_open(json.loads(record.read_text())['pid']) + except ProcessLookupError: + pass + try: + self.native(session, 'stop') + self.assertFalse(record.exists(), 'native stop left its process record') + if process_fd is not None: + self.assertTrue(select.select([process_fd], [], [], 5)[0], + 'native stop removed state but left the process alive') + finally: + if process_fd is not None: + if not select.select([process_fd], [], [], 0)[0]: + signal.pidfd_send_signal(process_fd, signal.SIGKILL) + select.select([process_fd], [], [], 5) + os.close(process_fd) + + def rpc(self, path, method, params, expect_error=False): + import websocket + with socket.socket(socket.AF_UNIX) as connection: + connection.settimeout(5) + connection.connect(path) + # The native control socket carries WebSocket frames, not the + # newline JSON used by `app-server --stdio`. + client = websocket.create_connection('ws://localhost', socket=connection, timeout=5) + try: + def send(message): + client.send(json.dumps(message)) + + def response(request_id): + for _ in range(100): + payload = client.recv() + self.assertTrue(payload, 'app-server closed the connection') + message = json.loads(payload) + if message.get('id') == request_id: + if expect_error and request_id == 1: + self.assertIn('error', message) + return message['error'] + self.assertNotIn('error', message) + return message['result'] + self.fail('app-server sent too many notifications before the response') + + send({'id': 0, 'method': 'initialize', 'params': { + 'clientInfo': {'name': 'coop_desktop_probe', 'version': '0.1.0'}}}) + response(0) + send({'method': 'initialized'}) + send({'id': 1, 'method': method, 'params': params}) + return response(1) + finally: + client.close() + + def test_native_start_reconnect_and_crash(self): + session = self.session('desktop') + self.assertEqual(session.state(), 'unavailable') + session.unlock('disposable-prototype-password') + self.assertEqual(session.state(), 'unlocked') + session.secret('store', 'disposable-token') + self.assertEqual(session.secret('lookup'), 'disposable-token') + session.secret('clear') + + # Register cleanup before startup: failed readiness may leave a child. + self.addCleanup(self.stop_native, session) + first = self.native(session, 'start') + self.assertEqual(first['status'], 'started') + self.rpc(first['socketPath'], 'account/login/start', { + 'type': 'apiKey', 'apiKey': 'sk-coop-disposable-prototype-not-a-real-key'}) + self.assertFalse((self.home / 'auth.json').exists(), + 'login fell back to plaintext credential storage') + self.assertEqual(self.rpc(first['socketPath'], 'account/read', { + 'refreshToken': False})['account']['type'], 'apiKey') + record_path = self.home / 'app-server-daemon/app-server.pid' + first_record = json.loads(record_path.read_text()) + # The launching CLI has exited. Check only the non-secret bus variable, + # never print or copy the server's entire environment. + environment = Path(f'/proc/{first_record["pid"]}/environ').read_bytes().split(b'\0') + bus_variable = ('DBUS_SESSION_BUS_ADDRESS=' + + session.env['DBUS_SESSION_BUS_ADDRESS']).encode() + self.assertIn(bus_variable, environment) + self.assertEqual(self.native(session, 'start')['status'], 'alreadyRunning') + self.assertEqual(json.loads(record_path.read_text()), first_record) + + os.kill(first_record['pid'], signal.SIGKILL) + deadline = time.monotonic() + 5 + while Path(f'/proc/{first_record["pid"]}').exists() and time.monotonic() < deadline: + # A zombie is no longer a running socket owner. + stat = Path(f'/proc/{first_record["pid"]}/stat') + try: + if stat.read_text().split(') ', 1)[1].startswith('Z'): + break + except FileNotFoundError: + break + time.sleep(0.02) + self.assertEqual(self.native(session, 'start')['status'], 'started') + replacement = json.loads(record_path.read_text()) + self.assertNotEqual(replacement['pid'], first_record['pid']) + environment = Path(f'/proc/{replacement["pid"]}/environ').read_bytes().split(b'\0') + self.assertIn(bus_variable, environment) + self.assertEqual(self.rpc(first['socketPath'], 'account/read', { + 'refreshToken': False})['account']['type'], 'apiKey') + self.assertFalse((self.home / 'auth.json').exists()) + self.rpc(first['socketPath'], 'account/logout', None) + self.assertIsNone(self.rpc(first['socketPath'], 'account/read', { + 'refreshToken': False})['account']) + self.assertFalse((self.home / 'auth.json').exists()) + + def test_lock_wrong_password_and_owned_replacement(self): + session = self.session('desktop') + session.unlock('disposable-prototype-password') + session.secret('store', 'disposable-token') + session.lock() + self.assertEqual(session.state(), 'locked') + with self.assertRaisesRegex(RuntimeError, 'replace the owned keyring'): + session.unlock('disposable-prototype-password') + session.restart_keyring('incorrect-disposable-password') + self.assertEqual(session.state(), 'locked') + session.restart_keyring('disposable-prototype-password') + self.assertEqual(session.state(), 'unlocked') + self.assertEqual(session.keyring_owner_pid(), session.keyring.pid) + self.assertEqual(session.secret('lookup'), 'disposable-token') + session.secret('clear') + + def test_duplicate_native_starts(self): + session = self.session('desktop') + session.unlock('disposable-prototype-password') + self.addCleanup(self.stop_native, session) + with ThreadPoolExecutor(max_workers=2) as workers: + results = list(workers.map(lambda _: self.native(session, 'start'), range(2))) + self.assertCountEqual([result['status'] for result in results], + ['started', 'alreadyRunning']) + self.assertEqual(results[0]['socketPath'], results[1]['socketPath']) + + def test_native_reuses_existing_server_on_a_different_bus(self): + first = self.session('first') + first.unlock('disposable-prototype-password') + second = self.session('second', self.root / 'second-data') + second.unlock('another-disposable-password') + self.addCleanup(self.stop_native, first) + self.native(first, 'start') + self.assertEqual(self.native(second, 'start')['status'], 'alreadyRunning') + record = json.loads((self.home / 'app-server-daemon/app-server.pid').read_text()) + environment = Path(f'/proc/{record["pid"]}/environ').read_bytes().split(b'\0') + self.assertIn(('DBUS_SESSION_BUS_ADDRESS=' + + first.env['DBUS_SESSION_BUS_ADDRESS']).encode(), environment) + self.assertNotIn(('DBUS_SESSION_BUS_ADDRESS=' + + second.env['DBUS_SESSION_BUS_ADDRESS']).encode(), environment) + + def test_native_readiness_does_not_prove_keyring_unlocked(self): + session = self.session('desktop') + session.unlock('disposable-prototype-password') + self.addCleanup(self.stop_native, session) + first = self.native(session, 'start') + self.rpc(first['socketPath'], 'account/login/start', { + 'type': 'apiKey', 'apiKey': 'sk-coop-disposable-prototype-not-a-real-key'}) + self.stop_native(session) + session.lock() + self.assertEqual(self.native(session, 'start')['status'], 'started') + self.assertEqual(session.state(), 'locked') + error = self.rpc(first['socketPath'], 'account/login/start', { + 'type': 'apiKey', 'apiKey': 'sk-coop-another-disposable-invalid-key'}, expect_error=True) + self.assertIn('keyring', error['message']) + self.assertFalse((self.home / 'auth.json').exists()) + self.stop_native(session) + session.restart_keyring('disposable-prototype-password') + self.assertEqual(self.native(session, 'start')['status'], 'started') + + def test_failed_native_readiness_requires_explicit_cleanup(self): + session = self.session('desktop') + session.unlock('disposable-prototype-password') + # Deliberately break server readiness while retaining the real native + # lifecycle manager. Replace only the temporary package symlink. + current = self.home / 'packages/standalone/current' + current.unlink() + (current / 'bin').mkdir(parents=True) + stalled = current / 'bin/codex' + stalled.write_text('''#!/bin/sh +if [ "$1" = "--version" ]; then + echo 'codex-cli 0.154.0' + exit 0 +fi +exec sleep 120 +''') + stalled.chmod(0o755) + self.addCleanup(self.stop_native, session) + result = subprocess.run( + [str(self.binary), 'app-server', 'daemon', 'start'], env=session.env, + cwd=self.root, text=True, capture_output=True, timeout=30) + self.assertNotEqual(result.returncode, 0) + self.assertIn('did not become ready', result.stderr) + record_path = self.home / 'app-server-daemon/app-server.pid' + self.assertTrue(record_path.exists(), 'native startup now rolls back; revisit ownership') + self.stop_native(session) + + def test_owner_restart_requires_password(self): + session = self.session('first-boot') + session.unlock('disposable-prototype-password') + session.secret('store', 'persisted-disposable-token') + session.close() + session.bus = None + restarted = self.session('second-boot') + self.assertEqual(restarted.state(), 'unavailable') + restarted.unlock('incorrect-disposable-password') + self.assertEqual(restarted.state(), 'locked') + restarted.restart_keyring('disposable-prototype-password') + self.assertEqual(restarted.state(), 'unlocked') + self.assertEqual(restarted.secret('lookup'), 'persisted-disposable-token') + restarted.secret('clear') + + def test_missing_default_is_distinct_from_locked(self): + import dbus + session = self.session('desktop') + session.unlock('disposable-prototype-password') + service = session.bus.get_object('org.freedesktop.secrets', + '/org/freedesktop/secrets', introspect=False) + service.SetAlias('default', dbus.ObjectPath('/'), + dbus_interface='org.freedesktop.Secret.Service', timeout=2) + self.assertEqual(session.state(), 'missing') + service.SetAlias('default', dbus.ObjectPath('/org/freedesktop/secrets/collection/login'), + dbus_interface='org.freedesktop.Secret.Service', timeout=2) + session.lock() + self.assertEqual(session.state(), 'locked') + + def test_shared_files_have_stale_refresh_and_logout(self): + # Characterization of an implementation blocker, not desired semantics. + # Separate buses alone cannot provide coherent shared credentials. + desktop = self.session('desktop') + desktop.unlock('disposable-prototype-password') + desktop.secret('store', 'version-one') + terminal = self.session('terminal') + terminal.unlock('disposable-prototype-password') + self.assertEqual(terminal.secret('lookup'), 'version-one') + desktop.secret('store', 'version-two') + self.assertEqual(terminal.secret('lookup'), 'version-one') + desktop.secret('clear') + self.assertEqual(terminal.secret('lookup'), 'version-one') + # An unrelated write must not resurrect the deleted account. This + # distinguishes stale whole-file state from deliberately logging in. + terminal.secret('store', 'unrelated-value', account='unrelated') + fresh = self.session('fresh') + fresh.unlock('disposable-prototype-password') + self.assertEqual(fresh.secret('lookup'), 'version-one') + fresh.secret('clear') + + def test_one_service_has_coherent_independent_clients(self): + session = self.session('shared') + session.unlock('disposable-prototype-password') + owner = session.keyring_owner_pid() + # Each secret-tool call creates a separate process and D-Bus client. + # Standard runtime-directory discovery works without borrowed env. + del session.env['DBUS_SESSION_BUS_ADDRESS'] + del session.env['GNOME_KEYRING_CONTROL'] + session.secret('store', 'version-one') + self.assertEqual(session.secret('lookup'), 'version-one') + session.secret('store', 'version-two') + self.assertEqual(session.secret('lookup'), 'version-two') + session.secret('clear') + session.secret('store', 'unrelated-value', account='unrelated') + self.assertIsNone(session.secret('lookup')) + self.assertEqual(session.keyring_owner_pid(), owner) + session.close() + session.bus = None + fresh = self.session('fresh') + fresh.unlock('disposable-prototype-password') + self.assertIsNone(fresh.secret('lookup'), 'unrelated write resurrected deleted credential') + self.assertEqual(fresh.secret('lookup', account='unrelated'), 'unrelated-value') + + def test_lookup_failure_is_not_credential_absence(self): + session = self.session('shared') + session.unlock('disposable-prototype-password') + self.assertIsNone(session.secret('lookup')) + session.secret('store', 'disposable-token') + self.assertEqual(session.secret('lookup'), 'disposable-token') + session.env['DBUS_SESSION_BUS_ADDRESS'] = 'unix:path=' + str(self.root / 'absent-bus') + with self.assertRaisesRegex(RuntimeError, 'disposable lookup failed'): + session.secret('lookup') + + def test_unlocked_writable_login_does_not_prove_encryption(self): + # Minimal GNOME 46.1 empty-password keyring, containing no credentials. + # Ordinary clients can unlock it without a password when they write. + data = self.root / 'data/keyrings' + data.mkdir(parents=True, mode=0o700) + keyring_file = data / 'login.keyring' + keyring_file.write_text( + '[keyring]\ndisplay-name=Login\nctime=1\nmtime=0\n' + 'lock-on-idle=false\nlock-after=false\n') + keyring_file.chmod(0o600) + (data / 'default').write_text('login') + session = self.session('shared') + session.keyring = subprocess.Popen( + ['gnome-keyring-daemon', '--foreground', '--components=secrets', + '--control-directory=' + session.env['GNOME_KEYRING_CONTROL']], + env=session.env, stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + session.children.append(session.keyring) + deadline = time.monotonic() + 5 + while session.state() == 'unavailable': + self.assertIsNone(session.keyring.poll()) + self.assertLess(time.monotonic(), deadline, 'keyring service startup timed out') + time.sleep(0.02) + session.secret('store', 'disposable-plaintext-witness') + self.assertEqual(session.state(), 'unlocked') + self.assertEqual(session.secret('lookup'), 'disposable-plaintext-witness') + self.assertIn(b'disposable-plaintext-witness', keyring_file.read_bytes()) + + def test_codex_shared_store_and_independent_auth_cache(self): + session = self.session('shared') + session.unlock('disposable-prototype-password') + del session.env['DBUS_SESSION_BUS_ADDRESS'] + del session.env['GNOME_KEYRING_CONTROL'] + login = subprocess.run( + [str(self.binary), '-c', 'cli_auth_credentials_store="keyring"', + 'login', '--with-api-key'], input='sk-disposable-invalid-key', + env=session.env, cwd=self.root, text=True, capture_output=True, timeout=10) + self.assertEqual(login.returncode, 0, login.stderr) + self.addCleanup(self.stop_native, session) + desktop = self.native(session, 'start') + params = {'refreshToken': False} + self.assertEqual(self.rpc(desktop['socketPath'], 'account/read', params) + ['account']['type'], 'apiKey') + logout = subprocess.run( + [str(self.binary), '-c', 'cli_auth_credentials_store="keyring"', 'logout'], + env=session.env, cwd=self.root, text=True, capture_output=True, timeout=10) + self.assertEqual(logout.returncode, 0, logout.stderr) + # Shared storage does not invalidate another AuthManager's RAM cache. + # This is Codex API-key cache behavior, not a ChatGPT revocation test. + self.assertEqual(self.rpc(desktop['socketPath'], 'account/read', params) + ['account']['type'], 'apiKey') + self.native(session, 'restart') + self.assertIsNone(self.rpc(desktop['socketPath'], 'account/read', params)['account']) + self.assertFalse((self.home / 'auth.json').exists()) + + @unittest.skip('historical private-bus fixture; production wrapper is covered by test-codex-keyring-systemd.py') + def test_terminal_server_isolation_with_one_shared_keyring(self): + import importlib.util + self.assertIsNotNone(shutil.which('strace'), 'this probe requires strace') + path = Path(__file__).with_name('test-codex-account.py') + spec = importlib.util.spec_from_file_location('account_fixture', path) + account = importlib.util.module_from_spec(spec) + spec.loader.exec_module(account) + session = self.session('shared') + session.unlock('disposable-prototype-password') + self.addCleanup(self.stop_native, session) + desktop = self.native(session, 'start') + wrapper = self.root / 'codex-account' + account.executable(wrapper, account.WRAPPER.replace('/usr/local/bin/codex', str(self.binary))) + env = {**session.env, 'COOP_CODEX_ACCOUNT_DBUS': '1', + 'COOP_CODEX_ACCOUNT_UNLOCKED': '1', 'TERM': 'xterm-256color'} + probe = account.RealDaemonTests() + trace = probe.launch(self.root, env, wrapper, 'shared-keyring') + self.assertNotIn(desktop['socketPath'], trace) + # Positive witness: removing only #481's override attaches to desktop. + mutant = self.root / 'codex-account-mutant' + account.executable(mutant, wrapper.read_text().replace( + "-c 'cli_auth_credentials_store=\"keyring\"' ", '')) + trace = probe.launch(self.root, env, mutant, 'shared-keyring-mutant') + self.assertIn(desktop['socketPath'], trace) + self.assertEqual(session.keyring_owner_pid(), session.keyring.pid) + + @unittest.skipUnless(os.environ.get('COOP_TEST_PAM_PROBE'), + 'compile codex-keyring-pam-probe.c and set COOP_TEST_PAM_PROBE') + def test_pam_creates_and_unlocks_existing_service(self): + import pwd + session = self.session('shared') + session.keyring = subprocess.Popen( + ['gnome-keyring-daemon', '--foreground', '--components=secrets', + '--control-directory=' + session.env['GNOME_KEYRING_CONTROL']], + env=session.env, stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + session.children.append(session.keyring) + deadline = time.monotonic() + 5 + while not session.bus.name_has_owner('org.freedesktop.secrets'): + self.assertIsNone(session.keyring.poll()) + self.assertLess(time.monotonic(), deadline, 'keyring service startup timed out') + time.sleep(0.02) + self.assertEqual(session.state(), 'missing') + owner = session.keyring_owner_pid() + config = self.root / 'pam' + config.mkdir() + module = os.environ.get('COOP_TEST_PAM_MODULE', 'pam_gnome_keyring.so') + (config / 'coop-keyring').write_text( + 'auth required pam_exec.so expose_authtok /usr/bin/true\n' + f'auth required {module}\n') + command = [os.environ['COOP_TEST_PAM_PROBE'], pwd.getpwuid(os.getuid()).pw_name, + str(config), session.env['GNOME_KEYRING_CONTROL']] + + def unlock(password): + result = subprocess.run(command, input=password + '\n', env=session.env, + text=True, capture_output=True, timeout=5) + self.assertNotIn(password, result.stdout + result.stderr) + self.assertEqual(session.keyring_owner_pid(), owner) + return result.returncode + + self.assertEqual(unlock('disposable-password'), 0) + self.assertEqual(session.state(), 'unlocked') + session.secret('store', 'disposable-token') + session.lock() + self.assertNotEqual(unlock('wrong-password'), 0) + self.assertEqual(session.state(), 'locked') + self.assertEqual(unlock('disposable-password'), 0) + self.assertEqual(session.state(), 'unlocked') + self.assertEqual(session.secret('lookup'), 'disposable-token') + session.secret('clear') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test-codex-keyring-systemd.py b/tests/test-codex-keyring-systemd.py new file mode 100644 index 0000000..9eb4f4a --- /dev/null +++ b/tests/test-codex-keyring-systemd.py @@ -0,0 +1,288 @@ +#!/usr/bin/python3 +"""Disposable-user production PAM/systemd/SSH tests (Linux, explicit opt-in). + +sudo COOP_TEST_KEYRING_SYSTEMD=1 COOP_TEST_CODEX=/path/to/native/bin/codex \ + python3 tests/test-codex-keyring-systemd.py -v +Requires OpenSSH on localhost, systemd, python3-dbus, python3-websocket, strace, libpam development headers, +and libpam-gnome-keyring. Optional COOP_TEST_PAM_INCLUDE / COOP_TEST_PAM_MODULE +point to unpacked distribution packages without installing a global PAM hook. +Only disposable fixture passwords and invalid API-key strings are used. +""" +import json +from concurrent.futures import ThreadPoolExecutor +import os +from pathlib import Path +import pwd +import select +import shutil +import signal +import subprocess +import tempfile +import time +import unittest +import uuid + +ROOT = Path(__file__).resolve().parent.parent +ENABLED = os.environ.get('COOP_TEST_KEYRING_SYSTEMD') == '1' + + +def run(args, **kwargs): + return subprocess.run(args, text=True, capture_output=True, timeout=120, check=True, **kwargs) + + +@unittest.skipUnless(ENABLED, 'explicit disposable-user systemd test opt-in required') +class SystemdTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + if os.geteuid() != 0: + raise RuntimeError('run this fixture with sudo') + cls.pam_config = Path('/etc/pam.d/coop-codex-keyring') + if cls.pam_config.exists(): + raise RuntimeError('fixture requires an unused /etc/pam.d/coop-codex-keyring') + cls.root = Path(tempfile.mkdtemp(prefix='coop-keyring-systemd-', dir='/var/tmp')) + cls.root.chmod(0o755) + cls.name = 'coop-kr-' + uuid.uuid4().hex[:8] + cls.uid = None + cls.addClassCleanup(cls.cleanup) + run(['useradd', '--home-dir', str(cls.root / 'home'), '--create-home', '--shell', '/bin/bash', cls.name]) + cls.uid = pwd.getpwnam(cls.name).pw_uid + cls.home = cls.root / 'home' + (cls.home / '.ssh').mkdir(mode=0o700) + run(['ssh-keygen', '-q', '-t', 'ed25519', '-N', '', '-f', str(cls.root / 'id')]) + shutil.copy(cls.root / 'id.pub', cls.home / '.ssh/authorized_keys') + cls.ssh = ['ssh', '-i', str(cls.root / 'id'), '-o', 'BatchMode=yes', + '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', + '-o', 'LogLevel=ERROR', cls.name + '@127.0.0.1'] + cls.binary = Path(os.environ['COOP_TEST_CODEX']).resolve() + standalone = cls.home / '.codex/packages/standalone' + standalone.mkdir(parents=True) + (standalone / 'current').symlink_to(cls.binary.parent.parent) + (cls.home / '.codex/config.toml').write_text('cli_auth_credentials_store = "keyring"\n[features]\nplugins = false\n') + run(['chown', '-R', cls.name + ':' + cls.name, str(cls.home)]) + include = os.environ.get('COOP_TEST_PAM_INCLUDE', '/usr/include') + run(['cc', '-std=c11', '-O2', '-Wall', '-Wextra', '-Werror', '-I' + include, + os.environ.get('COOP_TEST_PAM_SOURCE', str(ROOT / 'scripts/guest/codex-keyring-pam.c')), '-l:libpam.so.0', '-o', str(cls.root / 'pam')]) + module = os.environ.get('COOP_TEST_PAM_MODULE', 'pam_gnome_keyring.so') + cls.pam_config.write_text('auth required pam_exec.so expose_authtok /usr/bin/true\n' + 'auth required ' + module + '\n') + (cls.root / 'old-boot').write_text('disposable-install-boot') + cls.launch = cls.root / 'launch.py' + cls.launch.write_text(f'''import importlib.util,sys,os +from pathlib import Path +spec=importlib.util.spec_from_file_location('keyring', {str(ROOT / 'scripts/guest/codex-keyring.py')!r}) +k=importlib.util.module_from_spec(spec);spec.loader.exec_module(k) +k.PAM={str(cls.root / 'pam')!r} +k.CODEX={str(cls.binary)!r} +k.MIGRATION=Path({str(cls.root / 'old-boot')!r}) +operation=sys.argv[1];sys.argv=sys.argv[:1] +if operation=='unlock': + try:k.main() + except k.Error as error: + print(error.kind.name, file=sys.stderr);sys.exit(1) +elif operation=='state': + keyring=k.Keyring(Path(os.environ['XDG_RUNTIME_DIR'])) + print(keyring.collection(k.storage(Path.home()))) +elif operation=='native-credentials': + import json + prototype_spec=importlib.util.spec_from_file_location('prototype', {str(ROOT / 'tests/test-codex-desktop-prototype.py')!r}) + prototype=importlib.util.module_from_spec(prototype_spec);prototype_spec.loader.exec_module(prototype) + rpc=prototype.DesktopPrototypeTests().rpc + native=json.loads(k.run([k.CODEX,'app-server','daemon','start']).stdout) + rpc(native['socketPath'],'account/login/start',{{'type':'apiKey','apiKey':'sk-coop-disposable-invalid-key'}}) + assert rpc(native['socketPath'],'account/read',{{'refreshToken':False}})['account']['type']=='apiKey' + assert not (Path.home()/'.codex/auth.json').exists() + rpc(native['socketPath'],'account/logout',None) + assert rpc(native['socketPath'],'account/read',{{'refreshToken':False}})['account'] is None +elif operation=='probe-items': + keyring=k.Keyring(Path(os.environ['XDG_RUNTIME_DIR'])) + unlocked,locked=keyring.call(k.ROOT,k.SERVICE,'SearchItems',{{'service':'coop-codex-readiness'}}) + print(len(unlocked)+len(locked)) +elif operation=='lock': + keyring=k.Keyring(Path(os.environ['XDG_RUNTIME_DIR'])) + keyring.call(k.ROOT,k.SERVICE,'Lock',k.dbus.Array([k.LOGIN],signature='o')) +''') + run(['loginctl', 'enable-linger', cls.name]) + cls.remote('systemctl --user add-wants default.target gnome-keyring-daemon.service') + cls.remote('systemctl --user start gnome-keyring-daemon.service') + + @classmethod + def cleanup(cls): + if cls.uid is not None: + # User manager termination kills the fixture's native updater too; + # native stop by itself deliberately does not claim that ownership. + for args in (['loginctl', 'disable-linger', cls.name], + ['loginctl', 'terminate-user', cls.name], + ['systemctl', 'stop', f'user@{cls.uid}.service', f'user-runtime-dir@{cls.uid}.service'], + ['pkill', '-KILL', '-u', str(cls.uid)], + ['userdel', '--remove', cls.name]): + subprocess.run(args, capture_output=True, timeout=20) + cls.pam_config.unlink(missing_ok=True) + shutil.rmtree(cls.root) + + @classmethod + def remote(cls, command, **kwargs): + return run(cls.ssh + [command], **kwargs).stdout.strip() + + def unlock(self, password='disposable-guest-password', confirmation=None, success=True): + # -tt allocates a real remote TTY even though this harness uses pipes. + process = subprocess.Popen(self.ssh[:-1] + ['-tt', self.ssh[-1], + '/usr/bin/python3 ' + str(self.launch) + ' unlock'], + stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + output = b'' + prompted = confirmed = False + deadline = time.monotonic() + 120 + try: + while time.monotonic() < deadline: + ready, _, _ = select.select([process.stdout], [], [], 0.2) + if ready: + chunk = os.read(process.stdout.fileno(), 65536) + if not chunk: + break + output += chunk + if not prompted and b'Codex keyring password: ' in output: + process.stdin.write(password.encode() + b'\n') + process.stdin.flush() + prompted = True + if not confirmed and b'Confirm keyring password: ' in output: + process.stdin.write((confirmation if confirmation is not None else password).encode() + b'\n') + process.stdin.flush() + confirmed = True + code = process.wait(timeout=5) + if success: + self.assertEqual(code, 0, output.decode(errors='replace') + self.remote('systemctl --user show gnome-keyring-daemon.service -p Result -p ActiveState -p NRestarts')) + else: + self.assertNotEqual(code, 0, output.decode(errors='replace')) + if password: + self.assertNotIn(password.encode(), output, 'password was echoed') + return output + finally: + if process.poll() is None: + process.kill() + process.wait(timeout=5) + process.stdin.close() + process.stdout.close() + + def pid(self): + return self.remote('systemctl --user show gnome-keyring-daemon.service -p MainPID --value') + + def state(self): + return self.remote('/usr/bin/python3 ' + str(self.launch) + ' state') + + def test_production_lifecycle(self): + self.unlock(password='', success=False) + self.unlock(confirmation='different-disposable-password', success=False) + self.assertFalse((self.home / '.local/share/keyrings/login.keyring').exists()) + self.unlock() + first = self.pid() + self.assertEqual(self.state(), 'False') + self.remote('secret-tool store --label=disposable service coop-systemd-test', input='disposable-token') + self.assertEqual(self.remote('secret-tool lookup service coop-systemd-test'), 'disposable-token') + raw = (self.home / '.local/share/keyrings/login.keyring').read_bytes() + self.assertTrue(raw.startswith(b'GnomeKeyring\n\r\0\n' + bytes(4))) + self.assertNotIn(b'disposable-token', raw) + self.unlock() + with ThreadPoolExecutor(max_workers=2) as workers: + list(workers.map(lambda _: self.unlock(), range(2))) + self.assertEqual(self.pid(), first, 'repeated/duplicate unlock churned the service') + self.assertEqual(self.remote('/usr/bin/python3 ' + str(self.launch) + ' probe-items'), '0') + # No SSH sessions remain; a fresh login uses the same unlocked service. + time.sleep(1) + self.assertEqual(self.pid(), first) + self.assertEqual(self.remote('env -u DBUS_SESSION_BUS_ADDRESS secret-tool lookup service coop-systemd-test'), 'disposable-token') + # Native bootstrap owns server and updater. Recovery must work while + # that updater exists, without trying to become its process manager. + started = json.loads(self.remote(str(self.binary) + ' app-server daemon bootstrap')) + self.assertEqual(started['status'], 'bootstrapped') + raw = (self.home / '.local/share/keyrings/login.keyring').read_bytes() + self.remote('/usr/bin/python3 ' + str(self.launch) + ' lock') + self.assertEqual(self.state(), 'True') + self.unlock(password='\x03', success=False) + self.assertEqual(self.state(), 'True') + self.unlock(password='incorrect-disposable-password', success=False) + self.assertEqual(self.state(), 'True') + self.assertEqual((self.home / '.local/share/keyrings/login.keyring').read_bytes(), raw) + self.unlock() + self.assertEqual(self.state(), 'False') + self.assertFalse((self.home / '.codex/app-server-daemon/app-server.pid').exists()) + self.assertTrue((self.home / '.codex/app-server-daemon/app-server-updater.pid').exists()) + # A desktop-style bootstrap uses its native lock while unlock takes + # coop's independent lock. Race both against the locked collection. + self.remote('/usr/bin/python3 ' + str(self.launch) + ' lock') + with ThreadPoolExecutor(max_workers=2) as workers: + bootstrapped = workers.submit(self.remote, str(self.binary) + ' app-server daemon bootstrap') + unlocked = workers.submit(self.unlock) + self.assertEqual(json.loads(bootstrapped.result())['status'], 'bootstrapped') + unlocked.result() + self.assertEqual(self.state(), 'False') + self.remote('/usr/bin/python3 ' + str(self.launch) + ' native-credentials') + # Reconnect via the real native bootstrap, then crash its server. + self.remote(str(self.binary) + ' app-server daemon bootstrap') + record = json.loads((self.home / '.codex/app-server-daemon/app-server.pid').read_text()) + os.kill(record['pid'], signal.SIGKILL) + self.assertEqual(json.loads(self.remote(str(self.binary) + ' app-server daemon start'))['status'], 'started') + # Unexpected keyring crash requires another unlock and retirement. + self.remote('systemctl --user kill --signal=KILL --kill-who=main gnome-keyring-daemon.service') + deadline = time.monotonic() + 10 + while (self.pid() in ('0', first)) and time.monotonic() < deadline: + time.sleep(0.2) + self.assertEqual(self.state(), 'True') + self.unlock() + self.assertEqual(self.remote('secret-tool lookup service coop-systemd-test'), 'disposable-token') + self.assertFalse((self.home / '.codex/auth.json').exists()) + # Run the production terminal wrapper as the guest, using the existing + # PTY/strace witness from #481. Removing just its override must attach. + wrapper_source = (ROOT / 'scripts/guest/codex-account.sh').read_text().split("<<'CODEXACCOUNTEOF'\n", 1)[1].split('\nCODEXACCOUNTEOF', 1)[0] + helper = self.root / 'codex-keyring' + helper.write_text('#!/bin/sh\nexec /usr/bin/python3 ' + str(self.launch) + ' unlock\n') + helper.chmod(0o755) + wrapper = self.root / 'codex-account' + wrapper.write_text(wrapper_source.replace('/usr/local/bin/codex-keyring', str(helper)).replace('/usr/local/bin/codex', str(self.binary))) + wrapper.chmod(0o755) + mutant = self.root / 'codex-account-mutant' + mutant.write_text(wrapper.read_text().replace("-c 'cli_auth_credentials_store=\"keyring\"' ", '')) + mutant.chmod(0o755) + desktop = json.loads(self.remote(str(self.binary) + ' app-server daemon bootstrap')) + terminal_probe = self.root / 'terminal-probe.py' + terminal_probe.write_text(f'''import importlib.util,os +from pathlib import Path +spec=importlib.util.spec_from_file_location('account', {str(ROOT / 'tests/test-codex-account.py')!r}) +a=importlib.util.module_from_spec(spec);spec.loader.exec_module(a) +probe=a.RealDaemonTests() +env={{**os.environ, 'TERM':'xterm-256color'}} +trace=probe.launch(Path.home(),env,Path({str(wrapper)!r}),'shared') +assert {desktop['socketPath']!r} not in trace +trace=probe.launch(Path.home(),env,Path({str(mutant)!r}),'mutant') +assert {desktop['socketPath']!r} in trace +''') + self.remote('/usr/bin/python3 ' + str(terminal_probe)) + # Initial adoption may not trust a cached unlocked daemon after the + # backing file changes. Unsupported formats refuse before touching it; + # a valid prefix still needs the fresh parser and successful PAM unlock. + ready = Path(f'/run/user/{self.uid}/coop-codex/ready.json') + store = self.home / '.local/share/keyrings/login.keyring' + raw = store.read_bytes() + for invalid in (b'[keyring]\ndisplay-name=Login\n', b'unknown-format', + raw[:16] + b'\x01' + raw[17:]): + with self.subTest(storage='unsupported'): + before = self.pid() + store.write_bytes(invalid) + self.unlock(success=False) + self.assertEqual(store.read_bytes(), invalid) + self.assertEqual(self.pid(), before) + self.assertFalse(ready.exists()) + for invalid in (raw[:20], raw[:-1] + bytes([raw[-1] ^ 0x80])): + with self.subTest(storage='invalid-encrypted-candidate'): + store.write_bytes(invalid) + self.unlock(success=False) + self.assertEqual(store.read_bytes(), invalid) + self.assertFalse(ready.exists()) + # A repaired valid file must be loaded by a fresh daemon, even if the + # current daemon never loaded the previous truncated collection. + store.write_bytes(raw) + self.unlock() + self.assertEqual(self.remote('secret-tool lookup service coop-systemd-test'), 'disposable-token') + + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test-codex-keyring.py b/tests/test-codex-keyring.py new file mode 100644 index 0000000..e6889e1 --- /dev/null +++ b/tests/test-codex-keyring.py @@ -0,0 +1,406 @@ +#!/usr/bin/python3 +"""Production readiness decision tests. Real PAM/SSH tests live in the systemd suite.""" +import importlib.util +import json +import os +from pathlib import Path +import subprocess +import tempfile +import unittest +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parent.parent +spec = importlib.util.spec_from_file_location('keyring', ROOT / 'scripts/guest/codex-keyring.py') +k = importlib.util.module_from_spec(spec) +spec.loader.exec_module(k) + + +class StorageTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.home = Path(self.temp.name) + self.directory = self.home / '.local/share/keyrings' + self.directory.mkdir(parents=True) + self.store = self.directory / 'login.keyring' + + def test_absent_is_only_new_store(self): + self.assertIsNone(k.storage(self.home)) + for content in (b'', b'[keyring]\ndisplay-name=Login\n', b'unknown', + k.PREFIX[:-1] + b'\1'): + with self.subTest(content=content): + self.store.write_bytes(content) + with self.assertRaises(k.Error) as raised: + k.storage(self.home) + self.assertEqual(raised.exception.kind, k.Failure.FORMAT) + self.assertEqual(self.store.read_bytes(), content) + + def test_prefix_is_candidate_not_integrity(self): + self.store.write_bytes(k.PREFIX) + self.assertIsNotNone(k.storage(self.home)) + self.assertEqual(self.store.read_bytes(), k.PREFIX) + + def test_alias_conflict_and_other_stores(self): + (self.directory / 'default').write_text('session') + with self.assertRaises(k.Error) as error: + k.storage(self.home) + self.assertEqual(error.exception.kind, k.Failure.ALIAS) + (self.directory / 'default').unlink() + (self.directory / 'other.keyring').write_bytes(k.PREFIX) + with self.assertRaises(k.Error) as error: + k.storage(self.home) + self.assertEqual(error.exception.kind, k.Failure.CONFLICT) + + def test_symlink_is_never_new_storage(self): + self.store.symlink_to(self.home / 'absent') + with self.assertRaises(k.Error): + k.storage(self.home) + + def test_policy_rejects_overrides_and_plaintext(self): + config = self.home / '.codex/config.toml' + config.parent.mkdir() + config.write_text('cli_auth_credentials_store = "keyring"\n') + with patch.dict(os.environ, {}, clear=True): + k.policy(self.home) + with patch.dict(os.environ, {'CODEX_HOME': str(self.home / 'elsewhere')}): + with self.assertRaises(k.Error): + k.policy(self.home) + (config.parent / 'auth.json').write_text('{}') + with self.assertRaises(k.Error): + k.policy(self.home) + + +class FakeKeyring: + locked = False + generation_value = ['boot', 'bus', ':1.2'] + events = [] + collection_error = None + probe_error = None + generation_error = False + + def __init__(self, runtime): + self.bus = self + + def close(self): + pass + + def generation(self, boot): + if self.generation_error and 'probe' in self.events: + raise k.Error(k.Failure.GENERATION) + return self.generation_value + + def collection(self, candidate): + self.events.append('collection') + if self.collection_error: + raise k.Error(self.collection_error) + return self.locked + + def probe(self): + self.events.append('probe') + if self.probe_error: + raise k.Error(self.probe_error) + + +class OperationTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.marker = self.root / 'install-boot' + self.marker.write_text('old-boot') + self.state = self.root / 'coop-codex/ready.json' + FakeKeyring.events = [] + FakeKeyring.locked = False + FakeKeyring.collection_error = None + FakeKeyring.probe_error = None + FakeKeyring.generation_error = False + self.patches = [ + patch.object(k, 'MIGRATION', self.marker), + patch.object(k, 'policy'), + patch.object(k, 'storage', return_value='encrypted-candidate'), + patch.object(k, 'Keyring', FakeKeyring), + patch.object(k, 'run', return_value=subprocess.CompletedProcess([], 0, b'gnome-keyring-daemon: 46.1\n', b'')), + patch.object(k, 'systemctl', side_effect=lambda op: FakeKeyring.events.append(op)), + patch.object(k, 'retire_server', side_effect=lambda home: FakeKeyring.events.append('retire')), + patch.object(k.sys.stdin, 'isatty', return_value=True), + ] + for item in self.patches: + item.start() + self.addCleanup(item.stop) + + def invoke(self): + k.unlock(self.root, self.root, 'boot') + + def test_first_adoption_retires_before_fresh_service_and_after_probe(self): + self.invoke() + events = FakeKeyring.events + self.assertLess(events.index('retire'), events.index('restart')) + self.assertLess(events.index('restart'), events.index('probe')) + self.assertEqual(events.count('retire'), 2) + self.assertEqual(json.loads(self.state.read_text()), FakeKeyring.generation_value) + + def test_repeated_operation_does_not_churn(self): + self.invoke() + FakeKeyring.events.clear() + self.invoke() + self.assertNotIn('restart', FakeKeyring.events) + self.assertNotIn('retire', FakeKeyring.events) + self.assertIn('probe', FakeKeyring.events) + + def test_changed_owner_even_already_unlocked_requires_recovery(self): + self.invoke() + self.state.write_text(json.dumps(['boot', 'bus', ':1.old'])) + FakeKeyring.events.clear() + self.invoke() + self.assertIn('restart', FakeKeyring.events) + self.assertEqual(FakeKeyring.events.count('retire'), 2) + + def test_locked_transition_requires_pam_and_recovery(self): + self.invoke() + FakeKeyring.events.clear() + FakeKeyring.locked = True + def pam(creating): + self.assertFalse(creating) + FakeKeyring.locked = False + FakeKeyring.events.append('pam') + with patch.object(k, 'pam_unlock', side_effect=pam): + self.invoke() + self.assertIn('pam', FakeKeyring.events) + self.assertEqual(FakeKeyring.events.count('retire'), 1) + + def test_migration_barrier_precedes_service_or_probe(self): + self.marker.write_text('boot') + with self.assertRaises(k.Error) as error: + self.invoke() + self.assertEqual(error.exception.kind, k.Failure.MIGRATION) + self.assertEqual(FakeKeyring.events, []) + + def test_invalid_storage_requires_fresh_parser_and_refuses_before_pam(self): + FakeKeyring.collection_error = k.Failure.INVALID + with patch.object(k, 'pam_unlock') as pam: + with self.assertRaises(k.Error): + self.invoke() + pam.assert_not_called() + self.assertIn('restart', FakeKeyring.events) + self.assertFalse(self.state.exists()) + + def test_failure_cannot_publish_or_preserve_success(self): + for failure in (k.Failure.WRITE, k.Failure.CLEANUP): + with self.subTest(failure=failure): + FakeKeyring.probe_error = None + self.invoke() + FakeKeyring.probe_error = failure + with self.assertRaises(k.Error): + self.invoke() + self.assertFalse(self.state.exists()) + + def test_service_change_during_probe_invalidates_record(self): + FakeKeyring.generation_error = True + with self.assertRaises(k.Error) as error: + self.invoke() + self.assertEqual(error.exception.kind, k.Failure.GENERATION) + self.assertFalse(self.state.exists()) + + def test_recovery_failure_leaves_retry_eligible(self): + with patch.object(k, 'retire_server', side_effect=k.Error(k.Failure.NATIVE_BUSY)): + with self.assertRaises(k.Error): + self.invoke() + self.assertFalse(self.state.exists()) + self.invoke() + self.assertTrue(self.state.exists()) + + def test_noninteractive_adoption_does_not_replace_service(self): + with patch.object(k.sys.stdin, 'isatty', return_value=False): + with self.assertRaises(k.Error): + self.invoke() + self.assertNotIn('restart', FakeKeyring.events) + self.assertNotIn('retire', FakeKeyring.events) + + + def test_unadopted_service_does_not_invent_a_locked_state(self): + FakeKeyring.locked = False + with patch.object(k.sys.stdin, 'isatty', return_value=False): + with self.assertRaises(k.Error) as error: + self.invoke() + self.assertEqual(error.exception.kind, k.Failure.ADOPTION) + with patch.object(k, 'storage', return_value=None): + with self.assertRaises(k.Error) as error: + self.invoke() + self.assertEqual(error.exception.kind, k.Failure.MISSING) + + def test_unsupported_daemon_version_is_not_a_storage_format_error(self): + with patch.object(k, 'run', return_value=subprocess.CompletedProcess([], 0, b'gnome-keyring-daemon: 99.0\n', b'')): + with self.assertRaises(k.Error) as error: + self.invoke() + self.assertEqual(error.exception.kind, k.Failure.VERSION) + self.assertEqual(FakeKeyring.events, []) + + +class ProbeBus: + def __init__(self, fail=None): + self.fail = fail + self.items = [] + self.calls = [] + self.attributes = None + + def call(self, path, interface, method, *args): + self.calls.append(method) + if method == 'OpenSession': + return '', k.dbus.ObjectPath('/session/one') + if method == 'CreateItem': + self.attributes = args[0][k.ITEM + '.Attributes'] + self.items.append(k.dbus.ObjectPath('/item/one')) + if self.fail == 'lost-create-reply': + raise k.dbus.DBusException('fixture lost reply') + return self.items[0], k.dbus.ObjectPath('/') + if method == 'GetSecret': + if self.fail in ('cancelled', 'cancelled-and-cleanup'): + raise KeyboardInterrupt + if self.fail in ('read', 'read-and-cleanup'): + raise k.dbus.DBusException('fixture read failure') + value = b'wrong-value' if self.fail == 'wrong-value' else b'coop-probe' + return '', b'', value, 'text/plain' + if method == 'SearchItems': + assert args[0] == self.attributes + return list(self.items), [] + if method == 'Delete': + if self.fail in ('cleanup', 'read-and-cleanup', 'cancelled-and-cleanup'): + raise k.dbus.DBusException('fixture cleanup failure') + if self.fail == 'delete-prompt': + return k.dbus.ObjectPath('/prompt/one') + if self.fail != 'residual-item': + self.items.remove(path) + return k.dbus.ObjectPath('/') + if method == 'Close': + return None + raise AssertionError(method) + + +class ProductionProbeTests(unittest.TestCase): + def probe(self, bus): + keyring = k.Keyring.__new__(k.Keyring) + keyring.call = bus.call + keyring.probe() + + def test_writes_reads_deletes_and_checks_absence(self): + bus = ProbeBus() + self.probe(bus) + self.assertEqual(bus.calls, ['OpenSession', 'CreateItem', 'GetSecret', + 'SearchItems', 'Delete', 'SearchItems', 'Close']) + self.assertEqual(bus.items, []) + first_operation = bus.attributes['operation'] + self.probe(bus) + self.assertNotEqual(first_operation, bus.attributes['operation']) + + def test_lost_create_reply_still_deletes_its_item(self): + bus = ProbeBus('lost-create-reply') + with self.assertRaises(k.Error) as error: + self.probe(bus) + self.assertEqual(error.exception.kind, k.Failure.WRITE) + self.assertEqual(bus.items, []) + self.assertIn('Delete', bus.calls) + + def test_read_failure_and_wrong_value_still_clean_up(self): + for failure in ('read', 'wrong-value'): + with self.subTest(failure=failure): + bus = ProbeBus(failure) + with self.assertRaises(k.Error) as error: + self.probe(bus) + self.assertEqual(error.exception.kind, k.Failure.WRITE) + self.assertEqual(bus.items, []) + + def test_cleanup_failure_prompt_and_residual_item_fail(self): + for failure in ('cleanup', 'delete-prompt', 'residual-item'): + with self.subTest(failure=failure): + bus = ProbeBus(failure) + with self.assertRaises(k.Error) as error: + self.probe(bus) + self.assertEqual(error.exception.kind, k.Failure.CLEANUP) + self.assertTrue(bus.items) + + def test_original_and_cleanup_failures_are_both_preserved(self): + bus = ProbeBus('read-and-cleanup') + with self.assertRaises(k.Error) as error: + self.probe(bus) + self.assertEqual(error.exception.kind, k.Failure.WRITE) + self.assertEqual(error.exception.secondary, k.Failure.CLEANUP) + + def test_cancellation_still_cleans_up_and_preserves_cleanup_failure(self): + bus = ProbeBus('cancelled') + with self.assertRaises(KeyboardInterrupt): + self.probe(bus) + self.assertEqual(bus.items, []) + bus = ProbeBus('cancelled-and-cleanup') + with self.assertRaises(k.Error) as error: + self.probe(bus) + self.assertEqual(error.exception.kind, k.Failure.CANCELLED) + self.assertEqual(error.exception.secondary, k.Failure.CLEANUP) + + +class CollectionTests(unittest.TestCase): + def collection(self, paths, alias, locked, candidate): + keyring = k.Keyring.__new__(k.Keyring) + def call(path, interface, method, *args): + if method == 'ReadAlias': + return alias + if args[-1] == 'Collections': + return paths + if args[-1] == 'Locked': + return locked + raise AssertionError((method, args)) + keyring.call = call + return keyring.collection(candidate) + + def test_locked_and_unlocked_persistent_login(self): + for locked in (False, True): + self.assertIs(self.collection([k.LOGIN], k.LOGIN, locked, k.Candidate.ENCRYPTED), locked) + + def test_missing_new_store_vs_invalid_existing_store(self): + self.assertIsNone(self.collection([], '/', False, None)) + with self.assertRaises(k.Error) as error: + self.collection([], '/', False, k.Candidate.ENCRYPTED) + self.assertEqual(error.exception.kind, k.Failure.INVALID) + + def test_session_alias_and_missing_default_are_refused(self): + for alias in ('/', k.ROOT + '/collection/session', '/other'): + with self.subTest(alias=alias), self.assertRaises(k.Error) as error: + self.collection([k.LOGIN], alias, False, k.Candidate.ENCRYPTED) + self.assertEqual(error.exception.kind, k.Failure.ALIAS) + + def test_conflicting_live_collection_is_not_first_use(self): + with self.assertRaises(k.Error) as error: + self.collection([k.LOGIN], k.LOGIN, False, None) + self.assertEqual(error.exception.kind, k.Failure.CONFLICT) + with self.assertRaises(k.Error) as error: + self.collection([k.LOGIN, '/other'], k.LOGIN, False, k.Candidate.ENCRYPTED) + self.assertEqual(error.exception.kind, k.Failure.CONFLICT) + + + +class MigrationTests(unittest.TestCase): + def test_conflicting_daemons_and_probe_errors_precede_installation(self): + source = ROOT / 'scripts/guest/codex-keyring-migrate.sh' + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + pgrep = root / 'pgrep' + apt = root / 'apt-get' + marker = root / 'apt-ran' + apt.write_text('#!/bin/sh\ntouch "$APT_MARKER"\n') + apt.chmod(0o755) + env = {**os.environ, 'PATH': str(root) + ':' + os.environ['PATH'], + 'GUEST_USER': 'fixture', 'APT_MARKER': str(marker)} + for output, status, allowed in (('', 1, True), ('123', 0, True), + ('123\n456', 0, False), ('', 2, False), + ('', 124, False)): + with self.subTest(output=output, status=status): + marker.unlink(missing_ok=True) + pgrep.write_text('#!/bin/sh\nprintf \"%s\\n\" \"' + output + '\"\nexit ' + str(status) + '\n') + pgrep.chmod(0o755) + result = subprocess.run(['bash', str(source)], env=env, + capture_output=True, timeout=5) + self.assertEqual(result.returncode == 0, allowed) + self.assertEqual(marker.exists(), allowed) + + +if __name__ == '__main__': + unittest.main() From fc5209007eb120c03e92ca0f209b4f684f8438a2 Mon Sep 17 00:00:00 2001 From: hbrodin <90325907+hbrodin@users.noreply.github.com> Date: Tue, 15 Sep 2026 21:48:01 +0200 Subject: [PATCH 3/4] Default guest Codex sessions to VM-isolated full access --- .github/workflows/ci.yml | 3 + docs/codex-integration.md | 27 ++++++- docs/commands.md | 2 +- docs/testing.md | 7 ++ docs/trust-model.md | 7 ++ scripts/guest/codex-permissions.sh | 19 +++++ src/backend.rs | 6 ++ src/commands/lifecycle.rs | 51 ++++++++++++- src/guest.rs | 3 + src/lib.rs | 2 +- src/lima.rs | 1 + src/setup.rs | 1 + tests/integration.sh | 12 +++ tests/run-integration.sh | 3 +- tests/test-codex-permissions.py | 117 +++++++++++++++++++++++++++++ 15 files changed, 253 insertions(+), 8 deletions(-) create mode 100644 scripts/guest/codex-permissions.sh create mode 100644 tests/test-codex-permissions.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97ea073..c323fce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,9 @@ jobs: - name: Codex account wrapper regression tests run: python3 tests/test-codex-account.py + - name: Codex permission defaults regression tests + run: python3 tests/test-codex-permissions.py + - name: Codex shared keyring readiness tests run: /usr/bin/python3 tests/test-codex-keyring.py diff --git a/docs/codex-integration.md b/docs/codex-integration.md index 26f633b..5a650f7 100644 --- a/docs/codex-integration.md +++ b/docs/codex-integration.md @@ -60,7 +60,32 @@ locking the keyring cannot erase credentials already cached in running clients. Concurrent OAuth refresh and logout across clients require real-account testing. Desktop SSH does not use coop's API-key/proxy secret forwarding. -To keep Codex's sandbox and approval prompts for a single session, pass `--ask`. coop then launches `codex` with no bypass flag, so Codex applies its normal defaults: +#### Desktop execution permissions + +Select **Full access** in the desktop thread's permissions control when using +the VM as the isolation boundary. An explicit desktop selection overrides +guest defaults, including when continuing an existing thread. Auto mode uses +the Linux workspace sandbox and may fail to initialize on guests without +working bubblewrap/user-namespace support. Changing authentication or unlocking +the keyring does not change a thread's permissions. + +Image provisioning and agent bootstrap install `/etc/codex/config.toml` when +that file does not already exist, in both authentication modes: + +```toml +approval_policy = "never" +default_permissions = ":danger-full-access" +``` + +These are system defaults for Codex 0.154.0, below user/project configuration +and explicit thread selections. Existing system configuration is preserved. +After upgrading an existing VM, restart it with agent bootstrap enabled to +install the defaults, then reconnect the desktop. `--no-agents` skips this +installation on existing images. The defaults do not disable separate app/MCP +approval policies or organization requirements. + +To restore workspace sandboxing and on-request approval prompts for a single +session, pass `--ask`. coop explicitly overrides the unrestricted guest defaults: ```bash coop codex --ask diff --git a/docs/commands.md b/docs/commands.md index 43c4be6..2cd39ab 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -340,7 +340,7 @@ coop ca my-project -- --cwd /workspace ### `codex` -Launch Codex inside the VM. By default coop passes `--dangerously-bypass-approvals-and-sandbox`, so Codex runs without its sandbox or approval prompts — parity with `coop claude`. The VM is the isolation boundary, and Codex's own Linux sandbox does not work in the guest (no functioning bubblewrap), so leaving it enabled makes every shell command Codex runs fail. Use `--ask` to keep Codex's sandbox and approval prompts for that session. With `[codex] auth = "chatgpt"`, `coop codex` launches through the guest keyring wrapper. The `login` and `logout` subcommands are always launched without the bypass flag: they never start an agent session, so there is nothing to sandbox. +Launch Codex inside the VM. By default coop passes `--dangerously-bypass-approvals-and-sandbox`, so Codex runs without its sandbox or approval prompts — parity with `coop claude`. The VM is the isolation boundary, and Codex's own Linux sandbox does not work in the guest (no functioning bubblewrap), so leaving it enabled makes every shell command Codex runs fail. Use `--ask` to explicitly restore workspace sandboxing and on-request approvals for that session, overriding the guest's full-access defaults. Caller arguments can override those settings. For desktop permission selection and system defaults, see [Codex integration](codex-integration.md#desktop-execution-permissions). With `[codex] auth = "chatgpt"`, `coop codex` launches through the guest keyring wrapper. The `login` and `logout` subcommands are always launched without the bypass flag: they never start an agent session, so there is nothing to sandbox. ``` coop codex [NAME] [FLAGS] [ARGS...] diff --git a/docs/testing.md b/docs/testing.md index e88cc50..986489a 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -62,6 +62,13 @@ CI). Shared guest readiness and storage decisions run with: /usr/bin/python3 tests/test-codex-keyring.py ``` +Run `python3 tests/test-codex-permissions.py` for system-default installation +and preservation tests. In a provisioned Linux guest, set +`COOP_TEST_CODEX=/usr/local/bin/codex` to also test the real app-server's system +defaults, user overrides, terminal `--ask` settings, desktop thread selections, +and unrestricted command execution. The real test uses temporary Codex homes +without account credentials; it requires the installed system defaults. + The production helper's PAM, systemd, native server recovery and fresh SSH connections are exercised on Linux with a disposable user: diff --git a/docs/trust-model.md b/docs/trust-model.md index 0167888..348056c 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -24,6 +24,13 @@ tool is to run AI coding agents (Claude Code, Codex) with broad autonomy `--dangerously-bypass-approvals-and-sandbox` / `--dangerously-skip-permissions` unless the user passes `--ask`. +Codex image provisioning and agent bootstrap also install unrestricted system +defaults in `/etc/codex/config.toml` if absent. These apply to desktop SSH and +bare Codex launches, independently of authentication mode. Existing system +configuration, user/project overrides and explicit desktop thread choices are +preserved. `coop codex --ask` explicitly restores workspace sandboxing and +on-request approvals; it does not rely on the unrestricted system defaults. + This is intentional and correct: there is **no privilege boundary inside the guest to protect** — the whole VM is the blast radius. The security model is "anything the agent does stays in the VM." Every rule below exists to keep that diff --git a/scripts/guest/codex-permissions.sh b/scripts/guest/codex-permissions.sh new file mode 100644 index 0000000..cf39058 --- /dev/null +++ b/scripts/guest/codex-permissions.sh @@ -0,0 +1,19 @@ +# Default only: user/project config and desktop thread selections take precedence. +# An administrator-owned system config is never replaced by provisioning. +( +set -eu +if [ ! -e /etc/codex/config.toml ] && [ ! -L /etc/codex/config.toml ]; then + install -d -m 755 /etc/codex + temporary=$(mktemp /etc/codex/.coop-permissions.XXXXXX) + trap 'rm -f "$temporary"' EXIT + cat > "$temporary" <<'CODEXPERMISSIONSEOF' +# coop: the guest VM is the isolation boundary. +approval_policy = "never" +default_permissions = ":danger-full-access" +CODEXPERMISSIONSEOF + chmod 644 "$temporary" + # Publish complete contents without replacing a concurrently created file. + ln "$temporary" /etc/codex/config.toml \ + || test -e /etc/codex/config.toml || test -L /etc/codex/config.toml +fi +) diff --git a/src/backend.rs b/src/backend.rs index 3cd1282..c1d0e43 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -1635,6 +1635,12 @@ fn bootstrap_codex( ) -> Result<()> { let mut model_state = ModelState::load_or_default(inst)?; ensure_codex_remote_auth_consistent(cfg, inst, &model_state)?; + // Refresh existing images too, including API-key guests without copied + // Codex config. This system layer leaves explicit user choices intact. + session.target.exec_with_stdin( + RemoteCommand::new().literal("sudo sh -s"), + crate::guest::SCRIPT_CODEX_PERMISSIONS.as_bytes().to_vec(), + )?; // Proxy mode (issue #411): in remote mode with `[proxy.openai]` (or a // per-VM override) configured, start the host-side injecting proxy and // point Codex at it. The guest holds only the capability token; the real diff --git a/src/commands/lifecycle.rs b/src/commands/lifecycle.rs index a489219..2e68347 100644 --- a/src/commands/lifecycle.rs +++ b/src/commands/lifecycle.rs @@ -1510,7 +1510,9 @@ const CODEX_AUTH_SUBCOMMANDS: &[&str] = &["login", "logout"]; /// The VM is the isolation boundary, so Codex's own sandbox is redundant — and /// broken in the guest, which lacks a working bubblewrap. Bypassing by default /// gives `coop codex` parity with `coop claude`'s `bypassPermissions`. `ask` -/// (from `--ask`) keeps Codex's sandbox and approval prompts. +/// (from `--ask`) explicitly restores workspace sandboxing and approvals even +/// when the guest's system config defaults to full access. Config overrides +/// precede caller arguments so explicit caller settings still take precedence. /// /// `codex login` / `codex logout` never start a session, so there is nothing /// to sandbox and the flag is dropped regardless of `ask`. Codex does accept it @@ -1522,8 +1524,21 @@ pub(crate) fn codex_launch_args(ask: bool, mut args: Vec) -> Vec let is_auth_subcommand = args .first() .is_some_and(|arg| CODEX_AUTH_SUBCOMMANDS.contains(&arg.as_str())); - if !ask && !is_auth_subcommand { - args.insert(0, CODEX_BYPASS_FLAG.to_string()); + if !is_auth_subcommand { + if ask { + args.splice( + 0..0, + [ + "-c", + "sandbox_mode=\"workspace-write\"", + "-c", + "approval_policy=\"on-request\"", + ] + .map(str::to_owned), + ); + } else { + args.insert(0, CODEX_BYPASS_FLAG.to_string()); + } } args } @@ -2701,7 +2716,35 @@ mod tests { #[test] fn codex_launch_args_with_ask_keeps_sandbox() { let args = super::codex_launch_args(true, vec!["--model".into(), "gpt-5".into()]); - assert_eq!(args, vec!["--model", "gpt-5"]); + assert_eq!( + args, + vec![ + "-c", + "sandbox_mode=\"workspace-write\"", + "-c", + "approval_policy=\"on-request\"", + "--model", + "gpt-5" + ] + ); + } + + #[test] + fn codex_launch_args_ask_preserves_caller_overrides_and_auth() { + let overrides = vec![ + "--sandbox".into(), + "read-only".into(), + "-c".into(), + "approval_policy=\"never\"".into(), + ]; + let args = super::codex_launch_args(true, overrides.clone()); + assert_eq!(&args[4..], overrides); + for command in ["login", "logout"] { + assert_eq!( + super::codex_launch_args(true, vec![command.into()]), + vec![command] + ); + } } #[test] diff --git a/src/guest.rs b/src/guest.rs index 39f6d7c..824d039 100644 --- a/src/guest.rs +++ b/src/guest.rs @@ -233,6 +233,9 @@ pub const SCRIPT_CLAUDE_CODE: &str = include_str!("../scripts/guest/claude-code. pub const SCRIPT_CODEX: &str = include_str!("../scripts/guest/codex.sh"); pub const SCRIPT_CODEX_ACCOUNT: &str = include_str!("../scripts/guest/codex-account.sh"); +/// Low-priority VM defaults shared by terminal and desktop Codex sessions. +pub const SCRIPT_CODEX_PERMISSIONS: &str = include_str!("../scripts/guest/codex-permissions.sh"); + pub const SCRIPT_CODEX_KEYRING_MIGRATE: &str = include_str!("../scripts/guest/codex-keyring-migrate.sh"); diff --git a/src/lib.rs b/src/lib.rs index d3ef752..30f3352 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -385,7 +385,7 @@ enum Commands { add = ArgValueCandidates::new(completions::instance_candidates), )] name: Option, - /// Keep Codex's sandbox and approval prompts instead of bypassing them + /// Restore workspace sandboxing and on-request approvals (caller arguments override) #[arg(long)] ask: bool, /// Extra arguments passed to `codex` diff --git a/src/lima.rs b/src/lima.rs index e587733..4b5df2f 100644 --- a/src/lima.rs +++ b/src/lima.rs @@ -1495,6 +1495,7 @@ fn compose_provision_script( s.push('\n'); s.push_str(crate::guest::SCRIPT_CODEX_KEYRING); s.push_str(SCRIPT_CODEX_ACCOUNT); + s.push_str(crate::guest::SCRIPT_CODEX_PERMISSIONS); s.push('\n'); // Test hook: inject a provision failure to exercise error detection. diff --git a/src/setup.rs b/src/setup.rs index 122226a..6c1634d 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -890,6 +890,7 @@ fn compose_recipe( s.push_str(SCRIPT_CODEX); s.push_str(crate::guest::SCRIPT_CODEX_KEYRING); s.push_str(SCRIPT_CODEX_ACCOUNT); + s.push_str(crate::guest::SCRIPT_CODEX_PERMISSIONS); s } diff --git a/tests/integration.sh b/tests/integration.sh index 2df3a65..ff7abe6 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1195,6 +1195,18 @@ test_codex_bin_path() { check_native_codex + # Empty temporary Codex homes exercise the provisioned system defaults, + # explicit --ask settings and desktop thread overrides without OAuth. + local permissions_probe + permissions_probe=$(cat "$(dirname "$0")/test-codex-permissions.py") + if guest_exec env COOP_TEST_CODEX=/usr/local/bin/codex \ + python3 -c "$permissions_probe" ServerTests; then + pass "Codex system permissions and explicit overrides work through app-server" + else + fail "Codex system permissions and explicit overrides work through app-server" \ + "stderr: $(guest_stderr)" + fi + if coop_exec /usr/local/bin/codex --version >/dev/null; then pass "codex binary invocable via full path" else diff --git a/tests/run-integration.sh b/tests/run-integration.sh index 713bc94..bd7a5c8 100755 --- a/tests/run-integration.sh +++ b/tests/run-integration.sh @@ -93,7 +93,8 @@ source_archive="" trap '[[ -z "$source_archive" ]] || rm -f "$source_archive"; ssh "$REMOTE_HOST" rm -rf "$REMOTE_DIR"' EXIT echo "Copying binary and test script to $REMOTE_HOST:$REMOTE_DIR..." -scp -q "$LOCAL_BINARY" "$TEST_SCRIPT" "$REMOTE_HOST:$REMOTE_DIR/" +scp -q "$LOCAL_BINARY" "$TEST_SCRIPT" "$SCRIPT_DIR/test-codex-permissions.py" \ + "$REMOTE_HOST:$REMOTE_DIR/" # The full network gate builds on the remote, as does the proxy fallback. # Include tracked working-tree edits so the gate tests the same code as coop. diff --git a/tests/test-codex-permissions.py b/tests/test-codex-permissions.py new file mode 100644 index 0000000..c26c9ac --- /dev/null +++ b/tests/test-codex-permissions.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +"""System-default installation and opt-in real Codex permission precedence.""" +import json +import os +from pathlib import Path +import select +import subprocess +import tempfile +import time +import unittest + +class InstallTests(unittest.TestCase): + def test_defaults_are_complete_and_existing_config_survives(self): + root = Path(__file__).resolve().parent.parent + installer = (root / 'scripts/guest/codex-permissions.sh').read_text() + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) / 'codex' + script = installer.replace('/etc/codex', str(root)) + subprocess.run(['sh', '-c', script], check=True) + config = root / 'config.toml' + self.assertIn('approval_policy = "never"', config.read_text()) + self.assertIn('default_permissions = ":danger-full-access"', config.read_text()) + self.assertEqual(config.stat().st_mode & 0o777, 0o644) + custom = 'approval_policy = "on-request"\nsandbox_mode = "read-only"\n' + config.write_text(custom) + subprocess.run(['sh', '-c', script], check=True) + self.assertEqual(config.read_text(), custom) + self.assertEqual(sorted(p.name for p in root.iterdir()), ['config.toml']) + config.unlink() + config.symlink_to(root / 'absent-admin-config') + subprocess.run(['sh', '-c', script], check=True) + self.assertTrue(config.is_symlink()) + self.assertFalse(config.exists()) + + +@unittest.skipUnless(os.environ.get('COOP_TEST_CODEX'), 'set COOP_TEST_CODEX for real app-server tests') +class ServerTests(unittest.TestCase): + def test_system_defaults_and_explicit_thread_choices(self): + # The integration guest must have the production system defaults. The + # empty temporary CODEX_HOME proves these are not user-config defaults. + self.assertTrue(Path('/etc/codex/config.toml').is_file()) + for label, cli, config, expected in [ + ('default', [], '', ('never', 'dangerFullAccess')), + ('ask', ['-c', 'sandbox_mode="workspace-write"', '-c', + 'approval_policy="on-request"'], '', ('on-request', 'workspaceWrite')), + ('caller flags', ['-c', 'sandbox_mode="workspace-write"', '-c', + 'approval_policy="on-request"', '-c', + 'sandbox_mode="read-only"', '-c', + 'approval_policy="never"'], '', ('never', 'readOnly')), + ('user legacy config', [], 'sandbox_mode="read-only"\napproval_policy="on-request"\n', + ('on-request', 'readOnly')), + ('user profile config', [], 'default_permissions=":read-only"\napproval_policy="on-request"\n', + ('on-request', 'readOnly')), + ]: + with self.subTest(label=label), tempfile.TemporaryDirectory(prefix='coop-permissions-') as directory: + env = {k: v for k, v in os.environ.items() + if not k.startswith(('CODEX_', 'OPENAI_'))} + env['CODEX_HOME'] = directory + Path(directory, 'config.toml').write_text(config + '[features]\nplugins=false\n') + process = subprocess.Popen([os.environ['COOP_TEST_CODEX'], *cli, 'app-server', '--stdio'], + env=env, cwd=directory, stdin=subprocess.PIPE, + stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, + text=True) + self.sequence = 0 + self.pending = b'' + try: + self.rpc(process, 'initialize', {'clientInfo': {'name': 'coop_permissions_test', 'version': '1'}, + 'capabilities': {'experimentalApi': True}}) + result = self.rpc(process, 'thread/start', {'cwd': directory, 'ephemeral': True}) + self.assertEqual((result['approvalPolicy'], result['sandbox']['type']), expected) + if label == 'default': + result = self.rpc(process, 'command/exec', {'command': ['/usr/bin/true'], + 'cwd': directory, 'timeoutMs': 5000}) + self.assertEqual(result['exitCode'], 0, result) + for permissions, approval, sandbox in [ + (':workspace', 'on-request', 'workspaceWrite'), + (':danger-full-access', 'never', 'dangerFullAccess'), + ]: + result = self.rpc(process, 'thread/start', { + 'cwd': directory, 'ephemeral': True, + 'permissions': permissions, 'approvalPolicy': approval}) + self.assertEqual((result['approvalPolicy'], result['sandbox']['type']), + (approval, sandbox)) + self.assertFalse(Path(directory, 'auth.json').exists()) + finally: + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait(timeout=5) + process.stdin.close() + process.stdout.close() + + def rpc(self, process, method, params): + self.sequence += 1 + process.stdin.write(json.dumps({'id': self.sequence, 'method': method, 'params': params}) + '\n') + process.stdin.flush() + deadline = time.monotonic() + 30 + while time.monotonic() < deadline: + if b'\n' not in self.pending: + if not select.select([process.stdout], [], [], 1)[0]: + continue + chunk = os.read(process.stdout.fileno(), 65536) + self.assertTrue(chunk, 'server exited before responding') + self.pending += chunk + continue + line, self.pending = self.pending.split(b'\n', 1) + message = json.loads(line) + if message.get('id') == self.sequence: + self.assertNotIn('error', message, message) + return message['result'] + self.fail('app-server request timed out: ' + method) + + +if __name__ == '__main__': + unittest.main() From 2ae231eae75a5f5ad62d04160d03cfc86f7e3e6b Mon Sep 17 00:00:00 2001 From: hbrodin <90325907+hbrodin@users.noreply.github.com> Date: Wed, 16 Sep 2026 07:30:09 +0200 Subject: [PATCH 4/4] Restore Firecracker SSH sessions for shared Codex keyrings --- .github/workflows/ci.yml | 3 ++ docs/codex-integration.md | 5 +++ docs/platform-notes.md | 13 +++++++ docs/testing.md | 6 +++ scripts/guest/codex-keyring-setup.sh | 11 +++--- scripts/guest/codex-session.sh | 32 ++++++++++++++++ src/commands/codex.rs | 2 + src/guest.rs | 1 + tests/integration.sh | 44 ++++++++++++++++++++++ tests/test-codex-session.py | 55 ++++++++++++++++++++++++++++ 10 files changed, 167 insertions(+), 5 deletions(-) create mode 100644 scripts/guest/codex-session.sh create mode 100644 tests/test-codex-session.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c323fce..2e4fa12 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,9 @@ jobs: - name: Codex permission defaults regression tests run: python3 tests/test-codex-permissions.py + - name: Codex SSH session provisioning regression tests + run: python3 tests/test-codex-session.py + - name: Codex shared keyring readiness tests run: /usr/bin/python3 tests/test-codex-keyring.py diff --git a/docs/codex-integration.md b/docs/codex-integration.md index 5a650f7..4681854 100644 --- a/docs/codex-integration.md +++ b/docs/codex-integration.md @@ -53,6 +53,11 @@ and plaintext stores require explicit migration; coop never selects a history or replaces them automatically. Back up the files inside the guest before resolving conflicts or reauthenticating. +`codex-unlock` also upgrades guests missing the SSH session support required +by older Firecracker images, then requests the same stop/start cycle. See +[Firecracker SSH user sessions](platform-notes.md#firecracker-ssh-user-sessions) +for the PAM and persistent-linger details. + After a keyring crash or locked-to-unlocked transition, rerun `codex-unlock` and reconnect the desktop. Recovery retires the desktop server through Codex's native `daemon stop`; the desktop owns its next startup and updater. Closing or diff --git a/docs/platform-notes.md b/docs/platform-notes.md index fe87e7c..dfa07bf 100644 --- a/docs/platform-notes.md +++ b/docs/platform-notes.md @@ -69,3 +69,16 @@ network managed by systemd-networkd. Provisioning disables and masks the inherited service so it cannot install a second prefix and make another instance's IP a broadcast destination. Rebuild older images with `coop setup` to apply this guest-only fix; it does not change host networking or Lima. + +## Firecracker SSH user sessions + +The Firecracker base image mounts `/var/lib/systemd` as tmpfs and has a custom +PAM session stack that can prevent package installation from enabling +`pam_systemd`. Codex keyring provisioning installs a tmpfiles rule to recreate +the guest user's linger marker at each boot. It adds an SSH `pam_systemd` +session entry when neither SSH nor `common-session` already contains one, +preserving the existing common PAM stack. This gives fresh SSH sessions the +shared user bus and `XDG_RUNTIME_DIR` required by desktop authentication. +Rebuild older images with `coop setup`. For existing ChatGPT-auth guests, run +`coop codex-unlock` to install the missing session support, stop and start the +VM, then rerun `coop codex-unlock`. diff --git a/docs/testing.md b/docs/testing.md index 986489a..f10e39d 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -69,6 +69,12 @@ defaults, user overrides, terminal `--ask` settings, desktop thread selections, and unrestricted command execution. The real test uses temporary Codex homes without account credentials; it requires the installed system defaults. +Run `python3 tests/test-codex-session.py` for SSH PAM preservation, idempotent +session installation, and incomplete-upgrade detection. These tests use a +temporary filesystem and never modify host PAM. The VM suite verifies the +user bus and keyring after reboot, then independently removes each session +support marker to exercise `codex-unlock` upgrades and their reboot barrier. + The production helper's PAM, systemd, native server recovery and fresh SSH connections are exercised on Linux with a disposable user: diff --git a/scripts/guest/codex-keyring-setup.sh b/scripts/guest/codex-keyring-setup.sh index 7a51cd7..042fc27 100644 --- a/scripts/guest/codex-keyring-setup.sh +++ b/scripts/guest/codex-keyring-setup.sh @@ -19,13 +19,14 @@ chmod 644 /etc/pam.d/coop-codex-keyring # Ubuntu's package enables a common-password hook. Coop uses only the dedicated # service above; explicitly remove that global hook after package installation. DEBIAN_FRONTEND=noninteractive pam-auth-update --package --remove gnome-keyring -# Offline-safe equivalent of enabling linger; works while building in a chroot. -install -d -m 755 /var/lib/systemd/linger -touch "/var/lib/systemd/linger/$GUEST_USER" systemctl --global add-wants default.target gnome-keyring-daemon.service systemctl --global enable gnome-keyring-daemon.socket -# Never overwrite a migration barrier on repeated installs in the same boot. -if [ ! -f /var/lib/coop/codex-keyring-install-boot ]; then +# A session-support upgrade needs a fresh reboot barrier too. Ordinary repeated +# installs preserve the barrier, so they cannot hide an outstanding restart. +if [ ! -f /var/lib/coop/codex-keyring-install-boot ] || [ "$KEYRING_SESSION_UPGRADE" = 1 ]; then cat /proc/sys/kernel/random/boot_id >/var/lib/coop/codex-keyring-install-boot fi chmod 644 /var/lib/coop/codex-keyring-install-boot +# Publish upgrade completion only after every installer step succeeded. +touch /var/lib/coop/codex-session-v1 +chmod 644 /var/lib/coop/codex-session-v1 diff --git a/scripts/guest/codex-session.sh b/scripts/guest/codex-session.sh new file mode 100644 index 0000000..f2285af --- /dev/null +++ b/scripts/guest/codex-session.sh @@ -0,0 +1,32 @@ +# SSH must register a systemd session even when the base image's locally +# customized common-session prevents pam-auth-update from enabling its profile. +# Preserve that stack; add only the missing session module to SSH's stack. +: "${GUEST_USER:?GUEST_USER must be set by the orchestrator}" +KEYRING_SESSION_UPGRADE=0 +if [ ! -f /var/lib/coop/codex-session-v1 ] || [ ! -f /etc/tmpfiles.d/coop-codex-keyring.conf ]; then + KEYRING_SESSION_UPGRADE=1 + # A later failure must not let the next unlock mistake a partial repair + # for completed session support. + rm -f /var/lib/coop/codex-session-v1 +fi +if ! awk ' + $1 == "session" || $1 == "-session" { + for (i = 2; i <= NF; i++) { + if (substr($i, 1, 1) == "#") break + if ($i ~ /(^|\/)pam_systemd[.]so$/) found = 1 + } + } + END { exit !found } +' /etc/pam.d/sshd /etc/pam.d/common-session; then + printf '\n# coop: register SSH sessions with the systemd user manager.\nsession optional pam_systemd.so\n' >> /etc/pam.d/sshd +fi + +# Firecracker's base image mounts /var/lib/systemd as tmpfs. Recreate linger +# after local filesystems mount and before logind starts on every boot. +install -d -m 755 /etc/tmpfiles.d /var/lib/systemd/linger +cat >/etc/tmpfiles.d/coop-codex-keyring.conf <\"$KEYRING_BUILD/keyring.py\" <<'COOPKEYRINGEOF'\n", include_str!("../scripts/guest/codex-keyring.py"), "COOPKEYRINGEOF\n", + include_str!("../scripts/guest/codex-session.sh"), include_str!("../scripts/guest/codex-keyring-setup.sh"), ")\n", ); diff --git a/tests/integration.sh b/tests/integration.sh index ff7abe6..202e771 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1242,6 +1242,21 @@ test_codex_account_auth_support() { echo "" echo "=== Phase: codex account-auth (Secret Service) support ===" + # Earlier phases restarted the VM. This must work through fresh SSH using + # provisioned PAM/linger, without a manual loginctl or service-start repair. + if guest_exec sh -ec ' + test "$XDG_RUNTIME_DIR" = "/run/user/$(id -u)" + test -f "/var/lib/systemd/linger/$(id -un)" + test -S "$XDG_RUNTIME_DIR/bus" + systemctl --user is-active --quiet gnome-keyring-daemon.service + busctl --user status org.freedesktop.secrets >/dev/null + '; then + pass "fresh SSH has a persistent user manager and shared Secret Service after reboot" + else + fail "fresh SSH has a persistent user manager and shared Secret Service after reboot" \ + "stderr: $(guest_stderr)" + fi + if guest_exec test -x /usr/local/bin/codex-account; then pass "codex-account wrapper exists" else @@ -1402,6 +1417,35 @@ CFGEOF fi chatgpt_exec rm -rf "$alternate_codex_home" + # Model each incomplete session-support state independently. The helper + # and old boot marker remain, so each prerequisite must trigger migration. + local session_file + for session_file in /etc/tmpfiles.d/coop-codex-keyring.conf /var/lib/coop/codex-session-v1; do + if chatgpt_exec sudo rm "$session_file"; then + if chatgpt codex-unlock "$INSTANCE"