- Notifications
You must be signed in to change notification settings - Fork905
feat: add API key scope to restrict access to user data#17692
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
feat: add API key scope to restrict access to user data#17692
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ThomasK33 commentedMay 6, 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.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
6ee1c64 tofa4810bCompare5ffda26 tof43c610Comparefa4810b to51a0361CompareThere 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.
Pull Request Overview
This PR adds the new API key scope feature for workspace agents to restrict access to user-specific data. Key changes include:
- Adding an api_key_scope field in the Agent proto, database model, and terraform resource.
- Integrating the new field into middleware (AgentAPIKeyScopeCheckMW) for access enforcement.
- Updating audit logs and documentation to reflect the new API key scope attribute.
Reviewed Changes
Copilot reviewed 15 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site/e2e/provisionerGenerated.ts | Adds the apiKeyScope field in the Agent interface and updates its encoding. |
| provisionersdk/proto/provisioner.proto | Introduces the new api_key_scope field with field number 26. |
| provisioner/terraform/resources.go | Updates agentAttributes and ConvertState to include APIKeyScope and improves slice allocation. |
| enterprise/audit/table.go & docs/admin/security/audit-logs.md | Updates audit log configurations to include the api_key_scope field. |
| coderd/... | Integrates api_key_scope handling in database queries, models, middleware, and tests, plus a minor improvement in tz_darwin.go using strings.ReplaceAll. |
Files not reviewed (4)
- coderd/database/dump.sql: Language not supported
- coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.down.sql: Language not supported
- coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.up.sql: Language not supported
- coderd/database/queries/workspaceagents.sql: Language not supported
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
51a0361 tof685750Comparef43c610 to4fe32d3Compare39814d8 to07de6c8Compare5734989 to937e55aCompare67f2973 tod9b3dbcCompare937e55a to4deeaf3Compared9b3dbc to9220437Compare4deeaf3 to5582625Compare171c263 toafa0215Compare5582625 to6cc32e3Compareafa0215 to795b6b5Compare6cc32e3 tobc33d16Compare795b6b5 tod0766e9Compared0766e9 to2d58a59Compare7440aa8 to2bdc470CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.up.sql OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
57f1735 to50943c1CompareThere 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.
Needs to bump the provisionerd API version, since it adds a new (backward compatible) field.
Uh oh!
There was an error while loading.Please reload this page.
50943c1 to31e4751Compare31e4751 toc453560Comparefixed; but I haven't reviewed the whole thing.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c453560 tobc9cfb8Comparegraphite-appbot commentedMay 8, 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.
Merge activity
|
bc9cfb8 toefbe50aCompareThere 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 would like to see some additional tests around the RBAC scope, but I don't need to review again after that.
provisionerd/proto/version.go Outdated
| // API v1.5: | ||
| // - Add new field named `api_key_scope` in the Agent. |
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.
heads-up: there are a number of in-flight PRs that will be bumping this field.
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.
Since the 1.5 version has yet to be released, I don't think I need to bump it again (?).
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.
No, just giving a heads-up of a potential need to rebase.
ThomasK33May 15, 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.
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 had to rebase anyway since six new migrations happened, and new fields were added to the agent, too.
Uh oh!
There was an error while loading.Please reload this page.
| Role: Role{ | ||
| Identifier: RoleIdentifier{Name: fmt.Sprintf("Scope_%s", ScopeNoUserData)}, | ||
| DisplayName: "Scope without access to user data", | ||
| Site: allPermsExcept(ResourceUser), |
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.
Thisseems like the right way to do this if I recall correctly but it does make me nervous that we're essentially saying "this scope can do anything at site level except read user data". I'd like to see a test case added that confirms that this scope can't do things that the actual auth subject can't do.
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.
It is an intersection with the roles.
The default scope we use is*:
Lines 59 to 70 inc041459
| ScopeAll: { | |
| Role:Role{ | |
| Identifier:RoleIdentifier{Name:fmt.Sprintf("Scope_%s",ScopeAll)}, | |
| DisplayName:"All operations", | |
| Site:Permissions(map[string][]policy.Action{ | |
| ResourceWildcard.Type: {policy.WildcardSymbol}, | |
| }), | |
| Org:map[string][]Permission{}, | |
| User: []Permission{}, | |
| }, | |
| AllowIDList: []string{policy.WildcardSymbol}, | |
| }, |
coderd/database/migrations/000320_add_api_key_scope_to_workspace_agents.up.sql OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…ut user data accessChange-Id: Ia5a7085afea6ad6ab7fdba2ab738357f4c519966Signed-off-by: Thomas Kosiewski <tk@coder.com>
efbe50a tode14cfaCompareThere 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.
LG, really like seeing the scopes have some utility!
| workspaceAgentInfo := httpmw.ExtractWorkspaceAgentAndLatestBuild(httpmw.ExtractWorkspaceAgentAndLatestBuildConfig{ | ||
| DB: options.Database, | ||
| Optional: false, | ||
| }) |
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.
If this is only used once, we can probably go back to doing it inline, rather than declaring it up here.
Just a nit
| meID := uuid.New() | ||
| user = Subject{ | ||
| ID: meID.String(), | ||
| Roles: Roles{ | ||
| must(RoleByName(RoleMember())), | ||
| must(RoleByName(ScopedRoleOrgMember(defOrg))), | ||
| }, | ||
| Scope: must(ScopeNoUserData.Expand()), | ||
| } | ||
| // Test 1: Verify that no_user_data scope prevents accessing user data | ||
| testAuthorize(t, "ReadPersonalUser", user, | ||
| cases(func(c authTestCase) authTestCase { | ||
| c.actions = ResourceUser.AvailableActions() | ||
| c.allow = false | ||
| c.resource.ID = meID.String() | ||
| return c | ||
| }, []authTestCase{ | ||
| {resource: ResourceUser.WithOwner(meID.String()).InOrg(defOrg).WithID(meID)}, | ||
| }), | ||
| ) | ||
| // Test 2: Verify token can still perform regular member actions that don't involve user data | ||
| testAuthorize(t, "NoUserData_CanStillUseRegularPermissions", user, | ||
| // Test workspace access - should still work | ||
| cases(func(c authTestCase) authTestCase { | ||
| c.actions = []policy.Action{policy.ActionRead} | ||
| c.allow = true | ||
| return c | ||
| }, []authTestCase{ | ||
| // Can still read owned workspaces | ||
| {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID)}, | ||
| }), | ||
| // Test workspace create - should still work | ||
| cases(func(c authTestCase) authTestCase { | ||
| c.actions = []policy.Action{policy.ActionCreate} | ||
| c.allow = true | ||
| return c | ||
| }, []authTestCase{ | ||
| // Can still create workspaces | ||
| {resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID)}, | ||
| }), | ||
| ) | ||
| // Test 3: Verify token cannot perform actions outside of member role | ||
| testAuthorize(t, "NoUserData_CannotExceedMemberRole", user, | ||
| cases(func(c authTestCase) authTestCase { | ||
| c.actions = []policy.Action{policy.ActionRead, policy.ActionUpdate, policy.ActionDelete} | ||
| c.allow = false | ||
| return c | ||
| }, []authTestCase{ | ||
| // Cannot access other users' workspaces | ||
| {resource: ResourceWorkspace.InOrg(defOrg).WithOwner("other-user")}, | ||
| // Cannot access admin resources | ||
| {resource: ResourceOrganization.WithID(defOrg)}, | ||
| }), | ||
| ) |
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.
This is great! Thank you! 👍
1bacd82 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Part of#17649
Related to:coder/terraform-provider-coder#391
Add API Key Scope Restriction for Workspace Agents
This PR adds a new security feature that allows workspace agents to have restricted API key scopes. It introduces a new
api_key_scopefield to workspace agents with two possible values:all: Full access to all endpoints (existing behavior)no_user_data: Restricts access to user-specific data endpointsThe implementation includes:
api_key_scopecolumn to theworkspace_agentstableapi_key_scopeattributeThe scope restrictions have been applied to several endpoints that expose user data:
/external-authendpointsThis change allows administrators to create workspaces with agents that have limited access to sensitive user data, improving security in multi-agent environments where some agents may need to be more restricted than others.