feat: generate STS external ID for Bedrock role assumption - #26869
Conversation
Docs preview📖 View docs preview for |
evgeniy-scherbina
changed the title
aed9153 to
2dffd27
Compare
11e33ab to
2573afa
Compare
Documentation CheckUpdates Needed
Automated review via Coder Agents |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 4 findings (3 P3, 1 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 18 reviewers. 0 P0-P1, 3 P3, 1 Nit, 3 P4 dropped. Reviewed against 89b0a66..2573afa. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped feature. The confused-deputy mitigation is implemented at the right level, with defense in depth across validation, merge, and generation. The 4:1 test-to-code ratio is solid, and the integration tests cover the full CRUD lifecycle. Three P3 findings and a Nit below; nothing blocking.
Severity count: 3 P3, 1 Nit.
"I tried to build a case against this and couldn't." (Pariston)
Two lower-severity observations worth noting in the body rather than inline:
-
Existing providers with a RoleARN created before this deploy will have an empty
external_iduntil a PATCH (any PATCH, even todisplay_name) triggers generation. The first touch backfills it, but operators who upgrade and immediately GET the provider will see nothing. If this matters, a startup backfill analogous toBackfillBedrockProviderTypewould close the gap. (Meruem) -
Concurrent PATCHes that both add a RoleARN can each generate a different ExternalID; last commit wins silently. This follows the pre-existing update pattern (no
FOR UPDATElock). The window is narrow but ExternalID is uniquely sensitive because it must be manually synced to an external trust policy. Not actionable in this PR. (Hisoka)
🤖 This review was automatically generated with Coder Agents.
| ensureBedrockExternalID(&s) | ||
| require.NotEmpty(t, s.Bedrock.ExternalID) | ||
| // crypto/rand.Text returns a 26-character base32 string. | ||
| require.Len(t, s.Bedrock.ExternalID, 26) |
There was a problem hiding this comment.
P3 [CRF-1] require.Len(t, s.Bedrock.ExternalID, 26) hardcodes the current output length of crypto/rand.Text(). The Go docs explicitly state: "A future version may return longer texts as needed to maintain those properties." A Go upgrade that increases the length breaks this test with no bug in the code. The require.NotEmpty on the preceding line already verifies generation works.
Replace with require.NotEmpty or, if a sanity floor is desired, require.GreaterOrEqual(t, len(s.Bedrock.ExternalID), 26). (Bisky P3, Hisoka P3, Komugi P3, Knov Nit, Gon P3, Razor Note)
🤖
| // ExternalID is the STS external ID sent on the AssumeRole call when | ||
| // RoleARN is set. The server generates and owns it: create and update | ||
| // reject any client-supplied value that differs from the stored one (an | ||
| // update may echo the stored value back). | ||
| ExternalID string `json:"external_id,omitempty"` |
There was a problem hiding this comment.
P3 [CRF-2] The generated TypeScript at site/src/api/typesGenerated.ts:339-346 includes operator-facing guidance absent from this Go source:
Operators add it to the target role's trust policy as an sts:ExternalId condition to constrain the assumption to this deployment. It is not a secret and is returned on GET.
Since the apitypings generator produces comments from Go doc comments, the next make gen will strip this text from the TypeScript file. The missing content matters: an operator reading this field in the SDK needs to know what to do with it (copy it into the trust policy) and whether to treat it as a secret (no).
Fix: add the extra sentences to this Go doc comment and re-run make gen. (Razor P2, Leorio P3, Zoro P3, Ryosuke Note)
🤖
| // The external ID is server-owned and stable: carry the stored value | ||
| // forward so a patch can't change it. A patch that sets a different | ||
| // value is rejected upstream. | ||
| merged.ExternalID = existing.Bedrock.ExternalID |
There was a problem hiding this comment.
P3 [CRF-3] The ExternalID survives role removal (carried forward unconditionally here) and is reused when a different role is later added. The PR description says "stable thereafter," so this is by design. But there is no test for the remove-then-readd sequence: create with role A → PATCH to clear RoleARN → PATCH to add role B → assert same ExternalID.
Without that test, a future refactor could accidentally clear the ExternalID on role removal, silently breaking trust policies. (Ryosuke)
🤖
| // validateBedrockExternalIDUnchanged rejects a patch that sets a Bedrock | ||
| // external ID different from the stored one. A patch may echo the stored | ||
| // value (read-modify-write resends it) but not change it; the value is | ||
| // server-owned. |
There was a problem hiding this comment.
Nit [CRF-4] The func doc restates the function name. The one trap worth preserving is the echo-vs-change asymmetry (read-modify-write sends the stored value back and must be accepted). Consider:
// validateBedrockExternalIDUnchanged allows echoing the stored value
// (read-modify-write) but rejects a changed one.Similarly, the merge comment at line 797-799 could compress to // Server-owned: carry the stored value forward. (Gon)
🤖
dannykopping
left a comment
dannykopping
left a comment
There was a problem hiding this comment.
Couple minor nits, feel free to ignore.
| require.Equal(t, original, updated.Settings.Bedrock.ExternalID, "external ID must be stable across PATCH") | ||
| }) | ||
|
|
||
| t.Run("StableAcrossRoleRemovalAndReadd", func(t *testing.T) { |
There was a problem hiding this comment.
| t.Run("StableAcrossRoleRemovalAndReadd", func(t *testing.T) { | |
| t.Run("StableAcrossRoleRemovalAndRead", func(t *testing.T) { |
There was a problem hiding this comment.
Actually it's supposed to mean ReAdd, but I renamed to Reassignment for better readability.
2 participants
Implements: https://linear.app/codercom/issue/AIGOV-495/add-externalid-to-prevent-confused-deputy-problem
When a Bedrock provider assumes an IAM role via STS, the gateway now generates a unique external ID for it and sends that value on every
AssumeRolecall. The external ID guards against the confused deputy problem on cross-account role assumption. Per AWS's recommendation, the gateway generates and owns the value rather than accepting one from the operator; that ownership is what makes it effective, since a party who knows another's external ID can't induce the gateway to send it.The external ID is server-owned and read-only over the API. It is generated once, when a provider first has a
role_arn, and is stable thereafter. Clients cannot set it: create rejects any suppliedexternal_id, and update rejects a value that differs from the stored one. An update may echo the stored value back unchanged, so the normal read-modify-write flow (GET the provider, change a field, PATCH the full settings object) keeps working. The value is not a secret and is returned on GET so operators can copy it into the target role's trust policy as ansts:ExternalIdcondition.It is persisted in the existing JSON settings blob, so there is no migration or audit-table change.