fix: persist MCP OAuth tokens in sandboxes via file-backed keyring fallback (#3037)

[dgageot]

What

Fixes #3037 — remote MCP OAuth tokens cannot be stored when running inside a sandbox (sbx), because no OS keyring is available there.

Why it failed

keyring.Open() was called with no backend restriction. The generic file backend's opener always succeeds (it just constructs a struct), so in a sandbox openKeyring() returned a half-broken file keyring with an empty FileDir instead of an error. The in-memory fallback was therefore never reached, and the first token write failed with the opaque "No directory provided for file keyring" error — so the agent failed to initialize after completing the OAuth flow.

The fix

Backend resolution now follows a clear, layered order: OS-native keyring → encrypted file-backed keyring → in-memory.

1. Restrict openKeyring() to secure OS-native backends (wincred, keychain, secret-service, kwallet, keyctl) via AllowedBackends. The generic file/pass backends are excluded, so a missing OS keyring is now a detectable Open error we can act on instead of a deferred failure.
2. Add openFileKeyring(dir) — a persistent file-backed fallback rooted in a 0700 directory under the config dir. This lets OAuth tokens survive sandbox restarts rather than being lost (or forcing a fresh login) every run, which is the "fallback storage mechanism" the issue asks for.
3. Wire the fallback into the default store, with a final in-memory fallback if even the file keyring can't be opened. A (nil, nil) return from any opener is treated as a failure and falls through to the next tier, so the store can never be built around a nil keyring.

Security model of the file-backed fallback

The file keyring is sealed with a per-install random passphrase, not a hardcoded constant: a random 32-byte secret is generated on first use and persisted to a 0600 passphrase file inside the keyring directory, then reused. This means reading the source code alone is not enough to decrypt the keyring — an attacker also needs read access to the install's passphrase file.

The owner-only 0700 directory remains the real security boundary (the same boundary already protecting the encrypted token file beside it). This tier is intentionally weaker than an OS keyring and is only used when no native store exists.

Tests

• secureBackends excludes the generic file/pass backends.
• The file keyring persists and round-trips across reopen.
• Full token round-trip through the fallback path (using https://mcp.notion.com/sse).
• The file-keyring passphrase is random, persisted at 0600, reused across calls, and unique per install dir.
• The buildDefaultStore ordering is exercised directly via injectable openers: native preferred, file fallback (the Remote MCP server OAuth flow fails inside sbx — no keyring directory available #3037 regression path), last-resort in-memory, and (nil, nil) rings falling through.

All tests, golangci-lint, and the custom linter pass cleanly.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

2 finding(s): 1 medium, 1 low — both in newly introduced code.

The three-tier fallback chain (OS-native → file-backed → in-memory) is well-structured and the injectable openers make the ordering verifiable in tests. The main concerns are around the security model of the file-backed keyring tier.

[docker-agent]

[MEDIUM] Hardcoded file-keyring password provides no additional protection over plaintext key storage

fallbackKeyringPassword = "docker-agent-oauth-fallback" is a well-known constant in public source code. The file-backed keyring uses it to "encrypt" the AES-256 key that seals mcp-oauth-tokens.enc. Since the password is public, any process that can read the mcp-oauth-keyring/ directory can trivially recover the AES key — meaning the token file's AES-256 encryption provides no defence beyond filesystem permissions.

The comment acknowledges this ("the fallback's only real boundary is the 0700 directory"), which is correct, but it also means the layered appearance of "keyring → encrypted token file" is somewhat misleading: if the 0700 boundary is ever relaxed (chmod race, misconfigured umask, root access, path traversal bug, or volume mount in a container context), the tokens are immediately fully exposed with no additional cost to an attacker.

Suggested improvement: Rather than a fixed password, derive the key from a per-machine or per-install secret (e.g. generate a random 32-byte value on first run and store it in a separate file, or use the machine-id as a salt). This wouldn't break the sandbox use-case and would ensure that knowing the source code alone is not sufficient to decrypt the keyring.

This is an intentional documented tradeoff, so whether to address it is a reviewer decision — just flagging it so the threat model is explicitly confirmed.

[docker-agent]

[LOW] No nil-ring guard after successful keyring.Open — potential nil pointer dereference

In buildDefaultStore, after openNative() or openFallback() returns (ring, nil), ring is passed directly to newKeyringTokenStore(ring, ...) without a nil check. The 99designs/keyring library normally returns an error if it cannot open a backend, but the interface contract doesn't strictly prohibit a (nil, nil) return. If that ever occurs (e.g., a future library version or a mock in a test that accidentally returns nil, nil), the store will be constructed with a nil keyring and panic on the first Get/Set call.

Suggested fix: add a guard after each Open call:

if ring == nil {
    // treat same as an error — fall through to next tier
}

This costs one line and makes the fallback chain robust against unexpected library behaviour.

[dgageot]

Thanks for the review! Addressed both findings in 525ecab:

[MEDIUM] Hardcoded file-keyring password — Replaced the public fallbackKeyringPassword constant with a per-install random passphrase. fileKeyringPassphrase(dir) generates a random 32-byte hex secret on first use, persists it to a 0600 passphrase file inside the keyring dir, and reuses it thereafter. Knowing the source code is no longer sufficient to decrypt the keyring; each install is unique. The 0700 directory remains the real boundary, as before. Covered by TestFileKeyringPassphrase_RandomAndPersistent.

[LOW] Nil-ring guard — buildDefaultStore now treats a (nil, nil) return from either opener as a failure and falls through to the next tier, so the store is never constructed around a nil keyring. Covered by TestBuildDefaultStore_NilRingFallsThrough.

All tests, golangci-lint, and the custom linter pass.