Skip to content

Commit 236a418

Browse files
Ambient Code Botclaude
andcommitted
fix(credentials): address Wave 5 code review findings (RHOAIENG-55817)
- RoleBinding name now includes session SA name to prevent subject collision when concurrent sessions in the same project share a provider (was credential-token-reader-{provider}, now {session-sa}-credential-{provider}) - clear_runtime_credentials() now removes GOOGLE_APPLICATION_CREDENTIALS path in addition to the hardcoded workspace credentials path, preventing SA key files from leaking across turns when GOOGLE_APPLICATION_CREDENTIALS is set to a non-default path - Simplified test_returns_empty_when_no_credential_id_for_provider to use monkeypatch instead of redundant nested patch.dict + pop pattern - Updated control-plane-development.md to clarify that credential scope filtering is server-side (RBAC), not implemented in resolveCredentialIDs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 79e1772 commit 236a418

4 files changed

Lines changed: 35 additions & 33 deletions

File tree

.claude/context/control-plane-development.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ The runner drains the agent's inbox before starting the Claude Code session. All
8282

8383
### Credential fetch (Wave 5)
8484

85-
The CP resolves credentials for the session before pod creation. For each provider (github, gitlab, jira, google), it walks agent → project → global scope and takes the most specific matching credential. It then:
85+
The CP resolves credentials for the session before pod creation. It calls `sdk.Credentials().ListAll()` — the API server applies RBAC-scoped filtering server-side, returning only credentials visible to the session's service account. The CP takes the first credential per provider (first-match wins; ordering is server-determined). It then:
8686

8787
1. Builds `CREDENTIAL_IDS` — a JSON map of `provider → credential_id` — and injects it into the runner pod env
8888
2. Grants `credential:token-reader` on each credential ID to the runner pod's service account

components/ambient-control-plane/internal/reconciler/kube_reconciler.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ func (r *SimpleKubeReconciler) resolveCredentialIDs(ctx context.Context, sdk *sd
647647

648648
func (r *SimpleKubeReconciler) ensureCredentialRoleBindings(ctx context.Context, namespace, saName string, credentialIDs map[string]string) error {
649649
for provider, credID := range credentialIDs {
650-
name := fmt.Sprintf("credential-token-reader-%s", provider)
650+
name := fmt.Sprintf("%s-credential-%s", saName, provider)
651651
rb := &unstructured.Unstructured{
652652
Object: map[string]interface{}{
653653
"apiVersion": "rbac.authorization.k8s.io/v1",

components/runners/ambient-runner/ambient_runner/platform/auth.py

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -446,18 +446,25 @@ def clear_runtime_credentials() -> None:
446446
except OSError as e:
447447
logger.warning(f"Failed to remove token file {token_file}: {e}")
448448

449-
# Remove Google Workspace credential file if present (uses same hardcoded path as populate_runtime_credentials)
450-
google_cred_file = _GOOGLE_WORKSPACE_CREDS_FILE
451-
if google_cred_file.exists():
452-
try:
453-
google_cred_file.unlink()
454-
cleared.append("google_workspace_credentials_file")
455-
# Clean up empty parent dirs
456-
cred_dir = google_cred_file.parent
457-
if cred_dir.exists() and not any(cred_dir.iterdir()):
458-
cred_dir.rmdir()
459-
except OSError as e:
460-
logger.warning(f"Failed to remove Google credential file: {e}")
449+
# Remove Google credential files — both the default workspace path and any
450+
# path set via GOOGLE_APPLICATION_CREDENTIALS (used for SA JSON in Wave 5).
451+
google_cred_files = {_GOOGLE_WORKSPACE_CREDS_FILE}
452+
gac_path = os.getenv("GOOGLE_APPLICATION_CREDENTIALS", "")
453+
if gac_path:
454+
google_cred_files.add(Path(gac_path))
455+
456+
for google_cred_file in google_cred_files:
457+
if google_cred_file.exists():
458+
try:
459+
google_cred_file.unlink()
460+
cleared.append(str(google_cred_file.name))
461+
cred_dir = google_cred_file.parent
462+
if cred_dir.exists() and not any(cred_dir.iterdir()):
463+
cred_dir.rmdir()
464+
except OSError as e:
465+
logger.warning(
466+
f"Failed to remove Google credential file {google_cred_file}: {e}"
467+
)
461468

462469
if cleared:
463470
logger.info(f"Cleared credentials: {', '.join(cleared)}")

components/runners/ambient-runner/tests/test_shared_session_credentials.py

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -361,20 +361,13 @@ async def test_omits_current_user_header_when_not_set(self):
361361
thread.join(timeout=2)
362362

363363
@pytest.mark.asyncio
364-
async def test_returns_empty_when_no_credential_id_for_provider(self):
364+
async def test_returns_empty_when_no_credential_id_for_provider(self, monkeypatch):
365365
"""Verify graceful skip when CREDENTIAL_IDS does not contain the requested provider."""
366-
with patch.dict(
367-
os.environ,
368-
{
369-
"BACKEND_API_URL": "http://127.0.0.1:1/api",
370-
"CREDENTIAL_IDS": json.dumps({"gitlab": "some-id"}),
371-
},
372-
clear=False,
373-
):
374-
os.environ.pop("CREDENTIAL_IDS", None)
375-
with patch.dict(os.environ, {"CREDENTIAL_IDS": json.dumps({"gitlab": "some-id"})}):
376-
ctx = _make_context(current_user_id="user-123")
377-
result = await _fetch_credential(ctx, "github")
366+
monkeypatch.setenv("BACKEND_API_URL", "http://127.0.0.1:1/api")
367+
monkeypatch.setenv("CREDENTIAL_IDS", json.dumps({"gitlab": "some-id"}))
368+
369+
ctx = _make_context(current_user_id="user-123")
370+
result = await _fetch_credential(ctx, "github")
378371

379372
assert result == {}
380373

@@ -442,12 +435,14 @@ def log_message(self, format, *args):
442435
)
443436
thread.start()
444437

445-
credential_ids = json.dumps({
446-
"github": "cred-gh",
447-
"google": "cred-google",
448-
"jira": "cred-jira",
449-
"gitlab": "cred-gl",
450-
})
438+
credential_ids = json.dumps(
439+
{
440+
"github": "cred-gh",
441+
"google": "cred-google",
442+
"jira": "cred-jira",
443+
"gitlab": "cred-gl",
444+
}
445+
)
451446

452447
try:
453448
with patch.dict(

0 commit comments

Comments
 (0)