- Notifications
You must be signed in to change notification settings - Fork1k
feat: add allow list to API keys#19972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ThomasK33 commentedSep 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
05cdebd to5442fcdCompare1fea0d8 to8013625Compareda32dec tof2fbea4Compare8013625 to4382587Comparef2fbea4 to847f52eCompare4382587 tofb2440eCompare847f52e to1361b86Comparefb2440e toce693c0Compare1361b86 to581bc52Compare581bc52 to6d17ef1Comparece693c0 tof1a4e56Comparef1a4e56 toa591e0aCompare6d17ef1 to6fa5fe1Compare56cd860 tod38ed5bCompare5dd3400 to5a6e8ccCompared38ed5b to5cb53f7Compare5a6e8cc to73a65faCompare5cb53f7 to6248fffCompare7d422aa tod590c1dCompare6248fff to67ed32bCompared590c1d to49af2b4Compare4beef99 to86f7de9Compare49af2b4 tofa53285Compare86f7de9 to854efb0Comparefa53285 tod7df2aaCompare87fb51c to3bdb267Compare| funcAPIAllowListTarget(entry rbac.AllowListElement) codersdk.APIAllowListTarget { | ||
| target:=codersdk.AllowAllTarget() | ||
| ifentry.Type!="" { | ||
| target.Type=codersdk.RBACResource(entry.Type) | ||
| } | ||
| ifentry.ID!="" { | ||
| target.ID=entry.ID | ||
| } | ||
| returntarget | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know we have to figure out the semantics of an empty string for theid, but it should not be treated as a wildcard imo.
I think we should just pass the rbac entry to the SDK as is. Do not do:target := codersdk.AllowAllTarget() as the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated
coderd/users.go Outdated
| allowList:=db2sdk.List(k.AllowList,db2sdk.APIAllowListTarget) | ||
| iflen(allowList)==0 { | ||
| allowList=append(allowList,codersdk.AllowAllTarget()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I still think we should never have an empty allow list. The db migration handles this iirc, so defaulting toall if the list is empty feels like it's hiding some other issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, that's true. I've added an assert here since something else must have gone horribly wrong then.
d7df2aa toa1346f5Compare3bdb267 toc41112aComparec41112a toa3de31eComparea1346f5 toed90ecfComparea3de31e toad057a4CompareAdd allow_list field to API key data structures and ensure properJSON serialization across backend and frontend. Initialize with default wildcard entry (*:*) for backward compatibility withexisting API keys that don't have explicit resource restrictions.Fixes#19854
ad057a4 to79107e6Compare| iflen(allowList)==0 { | ||
| panic(fmt.Sprintf("developer error: API key %s has empty allow list",k.ID)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know it is annoying, but we should never panic. Should either return an error, or just leave the list empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The caller of this function is always an http route, which is able to handle and properly return an error.
This should be an internal server error. I know we have a recover MW, but that is a last resort to catch uncaught errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, but this isn't just about returning or bubbling up an error—it's about making our assumptions explicit. We assume that an empty allow list will never be stored in the database.
Therefore, this code path should only occur if something is seriously wrong with our database. In such a case, I don't think we should treat it as a typical application error. Instead, we should signal clearly that our foundational assumptions have been violated and something beyond our control is wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add API key allow list to the SDK
This PR adds an allow list to API keys in the SDK. The allow list is a list of targets that the API key is allowed to access. If the allow list is empty, a default allow list with a single entry that allows access to all resources is created.
The changes include:
Fixes#19854