feat(provider): add Amazon Bedrock IAM Identity Center SSO - #3370
feat(provider): add Amazon Bedrock IAM Identity Center SSO#3370keyuchen21 wants to merge 4 commits into
Conversation
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Independent review at exact head c87e304c41c233e75cdca9086a4cee0064d0f7e5. Thanks for taking on Bedrock SSO — it's a genuinely wanted provider path, and the Host-side credential handling here is careful work. Three [P1]s stand between it and landing, and the first one is the important one.
[P1] The SSO sign-in flow cannot complete — apps/desktop/src/renderer/settings/bedrock-sso-setup.tsx:31-35
const mounted = useRef(true);
useEffect(() => () => {
mounted.current = false;
if (attemptId) void window.maka.amazonBedrockSso.cancel(attemptId, host).catch(() => undefined);
}, [attemptId, host]);A cleanup-only effect with dependencies runs its cleanup on every dependency change, not only on unmount. login() calls setAttemptId(started.attemptId) at :42, which changes a dependency, so the cleanup fires mid-sign-in and sets mounted.current = false. The if (attemptId) guard covers only the cancel call — the sentinel is cleared unconditionally. Nothing ever restores it, because the effect body is just () => cleanup.
The polling loop at :45 is while (mounted.current && ...), and :50 is if (!mounted.current) return. So the first sign-in — necessarily the render where attemptId goes from undefined to a value — always returns early and never reaches accounts or roles. This isn't a race; it's unconditional.
Suggested shape: separate the mount-lifetime effect (empty deps, owns the sentinel) from the attempt-cancellation effect (deps on attemptId/host, owns only cancel), so changing the attempt id can't retire the component's liveness flag.
Worth noting alongside it: there is no renderer behaviour test over BedrockSsoSetup, so nothing in 69 files and +21,862 lines could have caught this. A test that drives login() through a fake bridge and asserts roles appear would pin the whole flow.
[P1] Ambient AWS_BEARER_TOKEN_BEDROCK silently overrides the configured SSO identity
@ai-sdk/amazon-bedrock, pinned to 5.0.58 by this PR's lockfile:
const rawApiKey = loadOptionalSetting({
settingValue: options.apiKey,
environmentVariableName: "AWS_BEARER_TOKEN_BEDROCK"
});
const apiKey = rawApiKey && rawApiKey.trim().length > 0 ? rawApiKey.trim() : void 0;
const fetchFunction = apiKey
? createApiKeyFetchFunction(apiKey, options.fetch)
: createSigV4FetchFunction(async () => { /* ... options.credentialProvider ... */ });credentialProvider lives inside the SigV4 branch. model-factory.ts:99-103 passes it correctly, but with a non-empty ambient bearer token that branch is never taken and the provider is never constructed, let alone called. A fake-only probe confirmed Bearer auth on the wire with zero calls into the explicit SSO provider.
The harm is wrong identity, not failed auth: a user who configured SSO gets inference, connection tests and manual probes running as whoever the stale environment token belongs to, and Settings still reports the connection as authenticated because that display is derived from the vault entry alone. Nothing on the running path discloses which credential source is actually in use.
One trap when fixing: passing apiKey: undefined does not suppress the lookup — loadOptionalSetting falls through to the environment variable exactly when the setting value is absent. Suppressing it requires an explicit empty string, or an upstream option that genuinely disables the env bearer.
[P1] Cancel only aborts device authorization, not what follows it
The cancel path aborts the device-authorization polling loop, but the post-auth calls — listAccounts, listRoles, model discovery, the manual Converse probe — take no abort signal and continue after a cancel returns. The Cancel button also remains clickable while busy, so a user who cancels can still have credentials in use afterwards, including model probes that cost money. Host close doesn't wait on these either.
Credential lifecycle — no findings. Reporting this explicitly because it is the part most likely to be assumed rather than checked: the SSO session lands in the Host workspace's runtime-policy-onboarding.json crash journal and finally in credential-vault.json, written atomically at 0600; role credentials stay in Host memory with a 5-minute skew refresh and never touch disk or the protocol. No token or role-key exposure was found in logs or protocol frames within the reviewed surface. With no ambient bearer present, the explicit provider is invoked and refresh failures classify correctly as Auth — so there is no access-key or default-chain fallback defect.
Deletion axis — no closed candidate. +21,862 lines invites the question of how much duplicates the existing OAuth credential path. Merging the two generation/CAS authorities is a probe, not a recommendation: AWS's two-level session→role expiry and OAuth's uncertain-commit semantics are different enough that a shared owner might add branching rather than remove it. Not claiming a net deletion without proving that.
Validation limits. Core→Storage→MCP→Runtime→Runtime Host clean build; 17/17 targeted tests; two fake-only probes. astryx:surface-inventory reproduces a required test failure on this head (the new setup file is not in the inventory). Full repo suite, renderer/Electron E2E and Windows packaging were not run, and no real AWS credentials or live AWS calls were used at any point — every probe used fakes and placeholder values.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Review at c87e304c. One [P1] and one [P2], both inline on bedrock-sso-coordinator.ts. Holding.
Both are lifecycle rather than cryptography: the login flow's happy path reads fine, but the commit and start paths each lose an invariant the rest of the codebase already keeps.
This review was performed statically with fakes and placeholders only — no AWS calls were made and no real credentials were used, so anything that would require live IAM Identity Center access is explicitly unverified here rather than passed.
Gate status: audit passed; test fails on Astryx stale surface inventory (the new bedrock-sso-setup.tsx is not listed) and package fails on an unrelated Windows packaged-app CDP timeout. The PR is also currently dirty.
中文
在 c87e304c 上审，一条 [P1] 加一条 [P2]，均行内提在 bedrock-sso-coordinator.ts，暂缓。
两条都属生命周期问题而非加密问题：登录流程的正常路径没问题，但提交路径和启动路径各自丢掉了一个仓库其他地方已经维持的不变量。
本次审查仅为静态审查，全程使用 fake 与占位符——没有发起任何 AWS 调用，也没有使用真实凭证；因此凡是需要真实 IAM Identity Center 访问才能确认的部分，这里明确标为未验证，而不是视为通过。
门禁状态：audit 通过；test 因 Astryx surface inventory 过期而失败（新增的 bedrock-sso-setup.tsx 未登记），package 因与本 PR 无关的 Windows 打包应用 CDP 超时而失败。此外该 PR 当前处于冲突状态。
| ); | ||
| if (committed.kind !== 'committed') { | ||
| return failure('persistence_failed', 'Amazon Bedrock connection could not be committed'); | ||
| } |
There was a problem hiding this comment.
[P1] A durable commit can be reported to the client as a persistence failure, and activation is left unpoisoned.
commitConnectionOnboarding returns committed — the connection is durably persisted. The very next statement is await this.invalidateBackends(), and the enclosing catch maps any throw to failure('persistence_failed', …). If invalidation throws:
- the client is told persistence failed while the connection is in fact persisted;
this.#committed.set(attemptId, result)is never reached, so there is no replay record for the retry to find;#attemptsstill holds the attempt;- activation is not poisoned, so a later Turn can proceed against backends whose invalidation never completed.
This file's own sibling already encodes the correct rule. runtime-policy-coordinator.ts:282-299 wraps the same shape and comments it explicitly:
The durable outcome is authoritative, but no later Turn may activate against a backend whose invalidation did not complete.
…followed by this.activation.poison(). That is the invariant this path drops.
Suggest preserving the durable outcome (record the replay result before invalidation, or report success-with-degraded-activation) and poisoning or fataling activation when invalidation fails, matching the policy coordinator.
中文
commitConnectionOnboarding 返回 committed 时连接已经持久化，紧接着的 await this.invalidateBackends() 若抛出，外层 catch 会把它一律映射成 persistence_failed。后果有四点：客户端被告知持久化失败，而数据其实已经落盘；#committed.set(attemptId, result) 根本没执行到，重试找不到 replay 记录；#attempts 仍留着该 attempt；并且 activation 没有被 poison，后续 Turn 可以在"失效流程未完成"的 backend 上继续跑。
同一仓库里的 runtime-policy-coordinator.ts:282-299 已经把正确规则写下来了——注释明确说"持久结果是权威的，但任何后续 Turn 都不得在失效流程未完成的 backend 上激活"，随后调用 this.activation.poison()。这条路径丢掉的正是这个不变量。
建议：保住持久结果（在失效之前先记录 replay 结果，或返回"成功但激活降级"），并在失效失败时按 policy coordinator 的做法 poison 或 fatal 掉 activation。
| } | ||
| for (const previous of this.#attempts.values()) { | ||
| this.#cancelAttempt(previous); | ||
| await previous.settlement.catch(() => undefined); |
There was a problem hiding this comment.
[P2] #start has no admission gate, so a concurrent start can orphan an in-flight AWS device flow.
The only early return is this.#attempts.get(input.attemptId), which two starts carrying different attempt ids both pass. Each then cancels the other's attempts and awaits previous.settlement.
The problem is when settlement becomes real: it is initialised to Promise.resolve() at :137 and only replaced with the actual #poll promise at :168. The awaited AWS device-authorization start sits between those two points, so a second start arriving in that window awaits an already-resolved promise, clears #attempts, and proceeds. The first flow then completes, opens a browser URL and begins polling with no attempt record owning it.
Suggest serialising admissions through a lease and awaiting the genuinely in-flight start before superseding or clearing.
中文
#start 唯一的提前返回是 this.#attempts.get(input.attemptId)，两个携带不同 attemptId 的并发 start 都能通过，随后各自取消对方的 attempt 并 await previous.settlement。
关键在于 settlement 何时变成真的：它在 :137 被初始化为 Promise.resolve()，直到 :168 才被替换成真正的 #poll promise，而那段被 await 的 AWS device authorization 启动过程正好位于两者之间。因此落在这个窗口里的第二个 start 会 await 一个已经 resolve 的 promise，清空 #attempts 然后继续；第一个流程随后完成、打开浏览器 URL 并开始轮询，却已经没有任何 attempt 记录持有它，成为孤儿。
建议用 lease 把 admission 串行化，并在顶替或清空之前 await 真正在飞的那个 start。
d70794a to
3ab6af6
Compare
|
@Astro-Han Thanks for the detailed reviews, Astro-Han — especially for spelling out the lifecycle and identity invariants and the limits of fake-only validation. All reported blockers are addressed on the current rebased head Coordinator lifecycle findings
Independent-review P1s
The branch is rebased onto current Validation on the current head:
The cancellation and identity tests use fakes/placeholders only; no live AWS credentials or calls were used. Ready for re-review when convenient. |
Add Host-owned IAM Identity Center device authorization, account and role selection, atomic credential onboarding, Bedrock model discovery, and Converse execution through dynamic temporary role credentials. Expose the flow in Desktop settings, keep temporary AWS credentials inside Runtime Host memory, and make configured Bedrock connections available to every execution client. Include inference-profile metadata and pricing projection, error classification, protocol coverage, and dependency notices. Generated-by: Maka
62cbddf to
ab20534
Compare
2 participants
Summary
amazon-bedrockas a first-class provider with typed IAM Identity Center configuration and a dedicatedaws_ssocredential kindbedrock.sso.*device-authorization lifecycle with account/role selection, bounded model discovery, manual model validation, and atomic onboarding recovery@ai-sdk/amazon-bedrock, including auxiliary model calls, connection tests, inference-profile capability inheritance, and source-model pricingSecurity properties
Validation
npm run buildnpm run lintConverseStreamrequest with tool call, tool result replay, and final textnpm testcompleted every JavaScript/workspace suite, but the local@maka/evalPython continuation stopped because the machine has Python 3.9 and the existing test uses Python 3.10 union syntax (int | None). Its 75 Node tests passed before that environment-only failure.