fix(auth): dedup container credentials at injection time; add auth mode#18
Draft
vinnie357 wants to merge 2 commits into
Draft
Conversation
Part A: when the harness declares an env(VAR) overlay that covers a provider service (e.g. env(ANTHROPIC_API_KEY) → service "anthropic"), drop the matching keychain credential from agent_credentials at ResolvedContainerOptions::resolve time. This prevents containers from receiving two conflicting Anthropic credentials when the user has both ANTHROPIC_API_KEY declared as an overlay and a CLAUDE_CODE_OAUTH_TOKEN in the keychain. The dedup is keyed on declared env only — never on std::env::var — so cloud harnesses without a declared anthropic var continue to receive keychain OAuth unaffected. Part B: add a per-harness `auth` field to RepoConfig with three modes: `keychain` (default, inject keychain creds with Part-A dedup applied), `passthrough` (never inject keychain creds; rely on env() overlays), `none` (no credential injection). Gated in AuthEngine::resolve_agent_auth via session.effective_config().auth_mode(), which is resolved through the existing EffectiveConfig precedence layer. Shared infrastructure: service_for_credential is now the canonical table in engine::auth and is re-exported into the dsbx module rather than duplicated. CLAUDE_CODE_OAUTH_TOKEN is added to the table (→ "anthropic") so the dedup can match it against ANTHROPIC_API_KEY by service name; the sbx driver's existing key-name guard for the OAuth token is updated to be an unconditional continue-before-service-lookup so sbx behaviour is unchanged. Also apply the pre-existing clippy::unnecessary_min_or_max fix in src/frontend/tui/render.rs required to pass the lint gate.
… last-credential drop; doc precision
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The container/docker runtime diverged from the sbx (Docker Sandbox) path in a way that caused containers to receive two conflicting Anthropic credentials when users supply
env(ANTHROPIC_API_KEY)via an overlay:CLAUDE_CODE_OAUTH_TOKEN(src/engine/sandbox/dsbx/auth.rs) because sbx has no OAuth-token support yet.env_passthrough(the harness-declaredANTHROPIC_API_KEY) andagent_credentials(the keychainCLAUDE_CODE_OAUTH_TOKEN) as-eflags with no dedup.Our previous fix (PR #17) attempted to detect this at the keychain-resolution layer by reading
std::env::var("ANTHROPIC_API_KEY"). That approach was wrong: it reads the ambient process environment, not the harness-declared env, regressing users who haveANTHROPIC_API_KEYin their process env but not declared as anenv()overlay (they'd lose OAuth auth).Changes
Part A — injection-time credential dedup (all container runtimes)
ResolvedContainerOptions::resolve()now callsdedup_credentials_by_declared_env()after ingesting all options. When a var inenv_passthroughorenv_literalmaps to a provider service viaservice_for_credential, anyagent_credentialsentry mapping to the same service is dropped before the options bag is used by any backend.std::env::var.ResolvedContainerOptionspath).Part B — explicit per-harness
authmodeA new
authfield in.awman/config.json(per-repo) selects the credential injection policy:keychain(default)passthroughenv()overlays.noneResolved via
EffectiveConfig::auth_mode()and gated inAuthEngine::resolve_agent_auth.Shared infrastructure
service_for_credentialis now the canonical table inengine::auth(public). The dsbx module re-exports it rather than maintaining its own copy.CLAUDE_CODE_OAUTH_TOKENis added to the table (→ "anthropic") so the container-side dedup can match it againstANTHROPIC_API_KEYby service. The sbxinject_credentialsis updated to skipCLAUDE_CODE_OAUTH_TOKENvia an unconditional key-name guard (before the service lookup), preserving sbx behaviour.Supersedes
PR #17 (env-sniff approach). This PR replaces it — the new branch is a clean cherry off upstream/main with no Approach-1 guard.
Follow-up fixes (Forge review)
Part A gate on resolvable declared env:
dedup_credentials_by_declared_envnow only counts anenv_passthroughentry as "covering" its service when the host value is actually set (lookup_envreturnsSome(non-empty)), mirroringbuild_run_argv's own emission gate. A declared but unset passthrough var no longer suppresses a keychain credential — the passthrough would emit nothing and the container would be left with zero credentials for that service.env_literalentries still always count (they carry an explicit value). Atracing::warn!is emitted whenever a keychain credential is dropped so the situation is never silent.Injectable
lookup_envclosure: The coverage check takes a&dyn Fn(&str) -> Option<String>rather than callingstd::env::vardirectly, so tests stay hermetic without mutating global env state.Doc precision: "Per-harness" → "Per-repo" in
AgentAuthMode,resolve_agent_auth, andEffectiveConfig::auth_mode()doc comments.none/passthroughdocs now correctly say "No KEYCHAIN credential injection; declaredenv()overlays still apply" rather than "No credential injection of any kind."auth_mode()gains a note that single-layer (repo-only) is a deliberate conservative choice.Incidental:
src/frontend/tui/render.rscontains a pre-existingunnecessary_min_or_maxclippy lint (line 1190) kept in this PR for clippy-clean CI; it is unrelated to auth and was present before these changes.Test summary
All tests pass except one pre-existing infra-dependent test (
real_git_run_poll_ci_loop_*requires a GitHub remote — no git remotes in the test working directory).New tests added (all hermetic, injectable closure):
dedup_fn_drops_oauth_when_anthropic_passthrough_set_on_host— passthrough declared AND set → OAuth droppeddedup_fn_retains_oauth_when_anthropic_passthrough_declared_but_unset— passthrough declared but UNSET → OAuth retained (core fix)dedup_fn_retains_oauth_when_no_anthropic_declared— no anthropic var → OAuth retaineddedup_fn_handles_env_literal_source— env_literal always countsdedup_fn_retains_credential_when_no_declared_vars— no-op pathdedup_fn_unmapped_credential_is_never_dropped— custom token retaineddedup_fn_drops_when_credential_name_equals_declared_var— keychain var name == declared var → droppeddedup_fn_multi_service_declared_x_retains_credential_for_y— multi-service: X dropped, Y retaineddeclared_anthropic_env_literal_drops_oauth_token_from_agent_credentials— env_literal through resolve()engine::container::options— Part A dedup viaresolve()anddedup_credentials_by_declared_envunit tests (env_passthrough, env_literal, unmapped credentials, no-op paths)data::config::effective— Part Bauth_mode()precedence tests (default, passthrough, none, explicit keychain)engine::auth— Part Bresolve_agent_authgating tests (passthrough → empty, none → empty, keychain → Ok)