feat: add ai_gateway_coderd_keys table and related RBAC#25563
Conversation
Docs preview📖 View docs preview for |
3248cc3 to
056ea4d
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Well-scoped boilerplate: table schema, RBAC, audit wiring, and nothing extra. The RBAC test is comprehensive (owner-only, all three actions, explicit denials for 12 other role subjects). Audit field tracking is deliberate (ActionSecret for hashed_secret, ActionIgnore for timestamps). Migration follows established patterns.
Severity count: 1 P2, 2 P3, 3 Nit, 1 Note.
The P2 is a schema gap: key_prefix has no UNIQUE constraint. Every other prefix-based lookup column in the codebase (oauth2_provider_app_secrets.secret_prefix, oauth2_provider_app_codes.secret_prefix) enforces uniqueness because the auth flow does WHERE prefix = $1 and expects exactly one row. The future handler will either assume uniqueness (bug if duplicates exist) or iterate candidates (timing side-channel). Free to fix now on an empty table.
The PR title says "aibridge_coderd_keys" but the table is ai_gateway_coderd_keys. Every code artifact uses the latter. git log --grep=ai_gateway_coderd_keys will miss this commit.
"Boring code gets silence, and most of this earned it." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
ea066e6 to
3c2624b
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
6 of 8 findings addressed (3c2624b), 1 acknowledged. DEREM-1 (P4) is unaddressed: the table, model, RBAC object, and audit config still have zero runtime callers. No query file, no handlers, no dbauthz integration.
This is a scaffolding PR and the finding was posted as "expected for scaffolding, noting for completeness." A brief acknowledgment (e.g., "follow-up PRs add handlers") unblocks the next review round.
🤖 This review was automatically generated with Coder Agents.
follow-up PRs add handlers, see WIP PRs #25564 and #25565 /coder-agents-review |
There was a problem hiding this comment.
All six R1 fixes verified clean by 9 reviewers. DEREM-1 deferred with linked follow-up PRs. Schema, RBAC, and audit wiring are consistent across all layers.
One new P3: the name column lacks a CHECK constraint, the same class of gap that DEREM-4 flagged on secret_prefix (and was fixed). An empty name produces an empty audit target string.
"This is the same class as DEREM-4 (empty-string acceptance on identifying column in this table). The fix for DEREM-4 hardened secret_prefix but left name unprotected." (Mafuuu)
🤖 This review was automatically generated with Coder Agents.
3c2624b to
ca08383
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 9 findings resolved across 4 rounds. 7 fixed by the author (DEREM-2 through -7, DEREM-9), 1 accepted (DEREM-8, no expires_at, follows provisioner_keys pattern), 1 deferred with linked follow-up PRs (DEREM-1, #25564 and #25565 add handlers).
The schema is tight: three unique indexes (name, secret_prefix, hashed_secret), two CHECK constraints (secret_prefix exact length, name slug regex), COMMENT ON TABLE/COLUMN, and a down migration that documents its intentional enum retention. RBAC is owner-only for create/read/delete. Audit boilerplate marks hashed_secret as ActionSecret. All fixes addressed root causes, not symptoms.
"I tried to build a case against this and could not." (Pariston)
🤖 This review was automatically generated with Coder Agents.
79e7499 to
35bc8d3
Compare
ssncferreira
left a comment
There was a problem hiding this comment.
Overall LGTM 👍 just a few questions/nits.
| return typed.Name | ||
| case database.AIProviderKey: | ||
| return typed.ID.String() | ||
| case database.AiGatewayCoderdKey: |
There was a problem hiding this comment.
nit: to match other types like AIProviderKey
| case database.AiGatewayCoderdKey: | |
| case database.AIGatewayCoderdKey: |
| name text NOT NULL, | ||
| secret_prefix varchar(11) NOT NULL, | ||
| hashed_secret bytea NOT NULL, | ||
| last_used_at timestamptz NULL, |
There was a problem hiding this comment.
Would it make sense to add a created_by column with the user that created the key? Since this is deployment scoped, it might be useful.
There was a problem hiding this comment.
I can see it bringing some value but also I see this potentially causing some PII issues.
Key creation / deletion is audit logged so this information should be available (just not for everyone).
| id uuid PRIMARY KEY, | ||
| created_at timestamptz NOT NULL, | ||
| name text NOT NULL, | ||
| secret_prefix varchar(11) NOT NULL, |
There was a problem hiding this comment.
Do we have any safeguards at the application level in case the secret length is less than 11? Because in that case, we could be storing the full secret at the database.
Actually, do we need a secret_prefix? 🤔 Isn't the (id, name) enough for identifying a key?
There was a problem hiding this comment.
Yes, name could be enough but secret prefix provides way to make sure that given secret is used in standalone deployment anyway, or in general check which key is used by which deployment.
In hindsight maybe name is redundant here. It provides human readable description but maybe it is overcomplicating key management process more then it improves it.
| ResourceTypeAISeat ResourceType = "ai_seat" | ||
| ResourceTypeAIProvider ResourceType = "ai_provider" | ||
| ResourceTypeAIProviderKey ResourceType = "ai_provider_key" | ||
| ResourceTypeAIGatewayCoderdKey ResourceType = "ai_gateway_coderd_key" |
There was a problem hiding this comment.
nit: remove coderd to be easier to read and consistent with provider key?
| ResourceTypeAIGatewayCoderdKey ResourceType = "ai_gateway_coderd_key" | |
| ResourceTypeAIGatewayKey ResourceType = "ai_gateway_key" |
There was a problem hiding this comment.
Naming is not the best but I don't have good idea.
Problem is that Keyis very overloaded word in AI Gateway context. When I read KeyI first think about provider keys or BYOK.
Maybe this is far fetched since provider keys have descriptive name ResourceTypeAIProviderKey.
Specifying Coderdat least notes that this is related to Coderd. Still it doesn't explain well what it is used for. JustAuthKeywould also be confusing. Auth to what?
AIGatewayCoderdAuthKey seems too long? Or maybe not and would be better?
Credentials have similar issue as Key and doens't match naming used by provisioners.
Emyrk
left a comment
There was a problem hiding this comment.
Gateway keys are deployment wide, correct?
Yes. There may be organization scoped keys in the future but no plans right now. |
a73fcbc to
8239275
Compare
13dd6ae to
1bfe3cc
Compare
37daa9f to
318ebc9
Compare
318ebc9 to
06ad9f0
Compare

Adds table to store keys that AI Gateway standalone replicas will use for authenticating into Coderd.
Also adds RBAC and audit boilerplate.