feat: hierarchical agent credentials and per-intent CLI selection - #405
feat: hierarchical agent credentials and per-intent CLI selection#405JWThewes wants to merge 10 commits into
Conversation
|
Please also update documentation (at least |
|
@jeromevdl addressed your comments |
Not sure everything was addressed... Please mark resolved the comments you have fixed. |
|
Documentation was updated in 0e37f0d, including docs/using-the-platform/platform-settings.md. The docs now cover personal > space > platform precedence, where each scope is managed, SSM paths, per-invocation binding resolution, start-time pinning/rotation behavior, and why there is no intent-owned secret store. Related updates are also in prerequisites.md, projects.md, first-intent.md, and concepts/architecture.md. I have now replied to and resolved every inline review thread individually; the final broker/IAM fix is in ef6a6ab. |
eipasteur
left a comment
eipasteur
left a comment
There was a problem hiding this comment.
Reviewed at ef6a6ab. Seven inline comments below: three on the credential trust boundary (MCP env inheritance, decryption scope, start race), two frontend state-reset issues, one cleanup gap on space deletion, one missing validation on DRAFT assists.
| missingCredentialBindings.push(credentialBinding); | ||
| continue; | ||
| } | ||
| invocationEnv[credentialEnvName(binding.provider)] = value; |
There was a problem hiding this comment.
I think this is where the new personal-credential boundary leaks. The value ends up in run-stage's childEnv (run-stage.js:1947-1952), and for Claude, Kiro and OpenCode the stdio MCP servers inherit that flat env; the comment block right above childEnv calls this an accepted trade-off. That was fine while the only thing in there was the platform token. With user bindings, a project owner/admin can configure a custom MCP server that reads a member's personal KIRO_API_KEY / AWS_BEARER_TOKEN_BEDROCK whenever that member runs an intent. RESERVED_MCP_ENV_KEYS only rejects ${VAR} refs using those names; it doesn't remove them from what children inherit. Codex already does the right thing here (sanitized env, env_vars whitelist reserved to aidlc), which seems like the model to follow. Could we scrub the model-auth variables from custom MCP children for the other three CLIs (or go through a brokered per-process mechanism), and add a regression test proving a custom MCP server can't observe the selected user's credential? I know #406 covers output redaction. This feels like a separate issue since it's about inheritance, not logging.
| const result = await ssm.send( | ||
| new GetParametersCommand({ | ||
| Names: names, | ||
| WithDecryption: true, |
There was a problem hiding this comment.
I don't think the broker is the sole decryption boundary yet. fetchValues decrypts, and it's reached from resolveEffectiveCredentialBindings() in the intents and discussions Lambdas, and from both that and readCredentialScopeStatus() in the agents Lambda. The intents and discussions roles have ssm:GetParameter(s) over users/*/agent-credentials/* and projects/*/agent-credentials/* (terraform/modules/api/lambda/main.tf:1750-1758, :2178-2188), so a code-execution or logging bug in any of the three can read raw keys without a grant, even though they only need "configured + source" to mint a binding. Could we separate presence metadata from value redemption and reserve WithDecryption: true for the broker? One caveat: platform-scope clears write the literal placeholder SecureString, so platform presence currently requires decrypting. User/space scopes delete the parameter, so existence would be enough there. Might be worth moving the platform sentinel out of the secret value as part of this.
| starterEmail: starter.email, | ||
| ...(priorStatus === 'DRAFT' | ||
| ? { | ||
| agentCli: selectedAgentCli, |
There was a problem hiding this comment.
I think there's a read-after-write race here. The pin is written, then invokeOrchestrator fires with only { action, intentId, executionId }, and the orchestrator's load-meta step (v2-orchestrator/index.js:316) is a plain GetItem without ConsistentRead (v2-process-store.js:111-115). The CLI and binding it reads there are closed over for every grant in the run. If that read returns the previous DRAFT image: a new draft has no CLI/binding, so no grant is minted, availableClis is empty and the stage fails no_cli; a legacy draft still has its project CLI and a null binding, so the orchestrator falls back to a platform grant (:319-322). And since AgentCore rereads META on its own (auth-resolver.js:118-120), if its read is fresh while the orchestrator's was stale we'd get credential_grant_required or credential_grant_mismatch instead. So the run can intermittently fail, or bill a different scope than the one the starter selected. It seems like a strongly consistent initial META read, or carrying the pin in the handoff and verifying it against META, would close this, ideally with a stale-read regression test.
| setPlatformFallback(result.platformFallback); | ||
| }, [projectId, scope]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
For the space-scoped instance, changing projectId reruns this effect but only resets status/error. bearerToken and kiroApiKey survive, and the in-flight load has no identity guard. Neither the card (AgentTab.tsx:267) nor the /space/:projectId/settings route is keyed, so React reuses the instance when navigating from space A to space B: an unsaved A credential can get saved into B, and a late A response can overwrite B's displayed status. I'd suggest clearing the write-only inputs and the pending save/error state whenever { scope, projectId } changes and ignoring stale responses (or simply keying the card on projectId), with a delayed A to B route-switch test.
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [projectId]); |
There was a problem hiding this comment.
selectedCli is per-intent but this reset is keyed only on projectId. The compose route is unkeyed, so browser back/forward (or any same-route navigation) between draft A and draft B in the same space reuses the instance and B keeps A's CLI, with Start enabled without a fresh explicit selection. The normal in-app path seems to unmount (Project list to /intent/:id, then redirect to /compose), so this is mostly a history-navigation case, but it's real. The second effect is that DiscussionProvider clears assistAgentCli on intentId while the mirror effect above is keyed on selectedCli only, so after that navigation Start shows CLI X and Quorum assist sends none (see the discussions comment). Including intentId in this reset (or keying the page on it) plus a same-space draft-switch test would cover both.
| case 'platform': | ||
| return `${prefix}/${config.parameterName}`; | ||
| case 'space': | ||
| return `${prefix}/projects/${assertIdentifier(projectId, 'projectId')}/agent-credentials/${ |
There was a problem hiding this comment.
Space credentials live under this path, but the project teardown (lambda/projects/index.js:1839, not part of this diff) drops the Project vertex without deleting either SecureString, so a deleted space keeps its Bedrock/Kiro keys indefinitely, and they stay inside the broker's projects/*/agent-credentials/* read scope. Could we delete both provider parameters before dropping the vertex (idempotent, ParameterNotFound tolerated) and cover it in the deletion tests? Note the projects Lambda runs as neptune_artifacts, which only has SSM rights on projects/*/mcp-secrets*, so this needs a scoped ssm:DeleteParameter in Terraform as well. Project-tier MCP secrets seem to have the same gap today; fine to keep that out of this PR, but it might be worth handling in the same pass.
| }; | ||
| } | ||
| } | ||
| if (!credentialProvider) { |
There was a problem hiding this comment.
For a DRAFT assist, omitting agentCli falls through here to { requestedCli: null, credentialBinding: null } instead of a 400. The handler then persists and broadcasts a running Quorum message, invokes AgentCore with no requestedCli/binding/grant, and returns 202; AgentCore's one-shot later hits no_cli and updates that message to a generic failed state. Since intents already enforce agent_cli_required for DRAFT start and compose, it feels like the assist endpoint should do the same before creating the pending message. It's also reachable from the UI: useDiscussion omits agentCli when assistAgentCli is null, which happens after the compose-page desync noted on IntentComposePage.tsx:138. The started-intent path keeps working as is (pinned CLI/binding win over the body, already covered by the existing test).
3 participants
Summary
user > space > platformprecedenceTesting
Closes #404