Skip to content

Commit 679179f

Browse files
EmyrkEmyrk
authored
feat: scope allow_list to include resource_type (#19748)
This feature allows the `allow_list` in the scopes to specify the `type`
1 parent 3df9d8e commit 679179f

6 files changed

Lines changed: 203 additions & 24 deletions

File tree

‎coderd/rbac/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ This command answers the question: “Is the user allowed?”
169169
### Partial Evaluation
170170

171171
```bash
172-
opa eval --partial --format=pretty 'data.authz.allow' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner --unknowns input.object.acl_user_list --unknowns input.object.acl_group_list -i input.json
172+
opa eval --partial --format=pretty 'data.authz.allow' -d policy.rego --unknowns input.object.id --unknowns input.object.owner --unknowns input.object.org_owner --unknowns input.object.acl_user_list --unknowns input.object.acl_group_list -i input.json
173173
```
174174

175175
This command performs a partial evaluation of the policy, specifying a set of unknown input parameters.

‎coderd/rbac/astvalue.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,25 @@ func (s Scope) regoValue() ast.Value {
182182
if !ok {
183183
panic("developer error: role is not an object")
184184
}
185+
186+
terms := make([]*ast.Term, len(s.AllowIDList))
187+
for i, v := range s.AllowIDList {
188+
terms[i] = ast.NewTerm(ast.NewObject(
189+
[2]*ast.Term{
190+
ast.StringTerm("type"),
191+
ast.StringTerm(v.Type),
192+
},
193+
[2]*ast.Term{
194+
ast.StringTerm("id"),
195+
ast.StringTerm(v.ID),
196+
},
197+
),
198+
)
199+
}
200+
185201
r.Insert(
186202
ast.StringTerm("allow_list"),
187-
ast.NewTerm(regoSliceString(s.AllowIDList...)),
203+
ast.NewTerm(ast.NewArray(terms...)),
188204
)
189205
return r
190206
}

‎coderd/rbac/authz_internal_test.go‎

Lines changed: 130 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -929,7 +929,7 @@ func TestAuthorizeScope(t *testing.T) {
929929
Org: map[string][]Permission{},
930930
User: []Permission{},
931931
},
932-
AllowIDList: []string{workspaceID.String()},
932+
AllowIDList: []AllowListElement{{Type: ResourceWorkspace.Type, ID: workspaceID.String()}},
933933
},
934934
}
935935

@@ -1019,7 +1019,9 @@ func TestAuthorizeScope(t *testing.T) {
10191019
User: []Permission{},
10201020
},
10211021
// Empty string allow_list is allowed for actions like 'create'
1022-
AllowIDList: []string{""},
1022+
AllowIDList: []AllowListElement{{
1023+
Type: ResourceWorkspace.Type, ID: "",
1024+
}},
10231025
},
10241026
}
10251027

@@ -1145,7 +1147,7 @@ func TestAuthorizeScope(t *testing.T) {
11451147
ResourceUser.Type: {policy.ActionRead},
11461148
}),
11471149
},
1148-
AllowIDList: []string{policy.WildcardSymbol},
1150+
AllowIDList: []AllowListElement{AllowListAll()},
11491151
},
11501152
}
11511153

@@ -1163,6 +1165,131 @@ func TestAuthorizeScope(t *testing.T) {
11631165
)
11641166
}
11651167

1168+
func TestScopeAllowList(t *testing.T) {
1169+
t.Parallel()
1170+
1171+
defOrg := uuid.New()
1172+
1173+
// Some IDs to use
1174+
wid := uuid.New()
1175+
gid := uuid.New()
1176+
1177+
user := Subject{
1178+
ID: "me",
1179+
Roles: Roles{
1180+
must(RoleByName(RoleOwner())),
1181+
},
1182+
Scope: Scope{
1183+
Role: Role{
1184+
Identifier: RoleIdentifier{
1185+
Name: "AllowList",
1186+
OrganizationID: defOrg,
1187+
},
1188+
DisplayName: "AllowList",
1189+
// Allow almost everything
1190+
Site: allPermsExcept(ResourceUser),
1191+
},
1192+
AllowIDList: []AllowListElement{
1193+
{Type: ResourceWorkspace.Type, ID: wid.String()},
1194+
{Type: ResourceWorkspace.Type, ID: ""}, // Allow to create
1195+
{Type: ResourceTemplate.Type, ID: policy.WildcardSymbol},
1196+
{Type: ResourceGroup.Type, ID: gid.String()},
1197+
1198+
// This scope allows all users, but the permissions do not.
1199+
{Type: ResourceUser.Type, ID: policy.WildcardSymbol},
1200+
},
1201+
},
1202+
}
1203+
1204+
testAuthorize(t, "AllowList", user,
1205+
// Allowed:
1206+
cases(func(c authTestCase) authTestCase {
1207+
c.allow = true
1208+
return c
1209+
},
1210+
[]authTestCase{
1211+
{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID).WithID(wid), actions: []policy.Action{policy.ActionRead}},
1212+
// matching on empty id
1213+
{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: []policy.Action{policy.ActionCreate}},
1214+
1215+
// Template has wildcard ID, so any uuid is allowed, including the empty
1216+
{resource: ResourceTemplate.InOrg(defOrg).WithID(uuid.New()), actions: AllActions()},
1217+
{resource: ResourceTemplate.InOrg(defOrg).WithID(uuid.New()), actions: AllActions()},
1218+
{resource: ResourceTemplate.InOrg(defOrg), actions: AllActions()},
1219+
1220+
// Group
1221+
{resource: ResourceGroup.InOrg(defOrg).WithID(gid), actions: []policy.Action{policy.ActionRead}},
1222+
},
1223+
),
1224+
1225+
// Not allowed:
1226+
cases(func(c authTestCase) authTestCase {
1227+
c.allow = false
1228+
return c
1229+
},
1230+
[]authTestCase{
1231+
// Has the scope and allow list, but not the permission
1232+
{resource: ResourceUser.WithOwner(user.ID), actions: []policy.Action{policy.ActionRead}},
1233+
1234+
// `wid` matches on the uuid, but not the type
1235+
{resource: ResourceGroup.WithID(wid), actions: []policy.Action{policy.ActionRead}},
1236+
1237+
// no empty id for the create action
1238+
{resource: ResourceGroup.InOrg(defOrg), actions: []policy.Action{policy.ActionCreate}},
1239+
},
1240+
),
1241+
)
1242+
1243+
// Wildcard type
1244+
user = Subject{
1245+
ID: "me",
1246+
Roles: Roles{
1247+
must(RoleByName(RoleOwner())),
1248+
},
1249+
Scope: Scope{
1250+
Role: Role{
1251+
Identifier: RoleIdentifier{
1252+
Name: "WildcardType",
1253+
OrganizationID: defOrg,
1254+
},
1255+
DisplayName: "WildcardType",
1256+
// Allow almost everything
1257+
Site: allPermsExcept(ResourceUser),
1258+
},
1259+
AllowIDList: []AllowListElement{
1260+
{Type: policy.WildcardSymbol, ID: wid.String()},
1261+
},
1262+
},
1263+
}
1264+
1265+
testAuthorize(t, "WildcardType", user,
1266+
// Allowed:
1267+
cases(func(c authTestCase) authTestCase {
1268+
c.allow = true
1269+
return c
1270+
},
1271+
[]authTestCase{
1272+
// anything with the id is ok
1273+
{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID).WithID(wid), actions: []policy.Action{policy.ActionRead}},
1274+
{resource: ResourceGroup.InOrg(defOrg).WithID(wid), actions: []policy.Action{policy.ActionRead}},
1275+
{resource: ResourceTemplate.InOrg(defOrg).WithID(wid), actions: []policy.Action{policy.ActionRead}},
1276+
},
1277+
),
1278+
1279+
// Not allowed:
1280+
cases(func(c authTestCase) authTestCase {
1281+
c.allow = false
1282+
return c
1283+
},
1284+
[]authTestCase{
1285+
// Anything without the id is not allowed
1286+
{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID), actions: []policy.Action{policy.ActionCreate}},
1287+
{resource: ResourceWorkspace.InOrg(defOrg).WithOwner(user.ID).WithID(uuid.New()), actions: []policy.Action{policy.ActionRead}},
1288+
},
1289+
),
1290+
)
1291+
}
1292+
11661293
// cases applies a given function to all test cases. This makes generalities easier to create.
11671294
func cases(opt func(c authTestCase) authTestCase, cases []authTestCase) []authTestCase {
11681295
if opt == nil {

‎coderd/rbac/input.json‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"action": "never-match-action",
2+
"action": "read",
33
"object": {
44
"id": "9046b041-58ed-47a3-9c3a-de302577875a",
55
"owner": "00000000-0000-0000-0000-000000000000",
@@ -40,7 +40,11 @@
4040
],
4141
"org": {},
4242
"user": [],
43-
"allow_list": ["*"]
43+
"allow_list": [
44+
{
45+
"type": "workspace",
46+
"id": "*"
47+
}]
4448
}
4549
}
4650
}

‎coderd/rbac/policy.rego‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -246,18 +246,39 @@ user_allow(roles) := num if {
246246
num := number(allow)
247247
}
248248

249-
# Scope allow_list is a list of resource IDs explicitly allowed by the scope.
250-
# If the list is '*', then all resources are allowed.
249+
# Scope allow_list is a list of resource (Type, ID) tuples explicitly allowed by the scope.
250+
# If the list contains `(*,*)`, then all resources are allowed.
251251
scope_allow_list if {
252-
"*" in input.subject.scope.allow_list
252+
input.subject.scope.allow_list[_] == {"type": "*", "id": "*"}
253253
}
254254

255+
# This is a shortcut if the allow_list contains (type, *), then allow all IDs of that type.
256+
scope_allow_list if {
257+
input.subject.scope.allow_list[_] == {"type": input.object.type, "id": "*"}
258+
}
259+
260+
# A comprehension that iterates over the allow_list and checks if the
261+
# (object.type, object.id) is in the allowed ids.
255262
scope_allow_list if {
256263
# If the wildcard is listed in the allow_list, we do not care about the
257264
# object.id. This line is included to prevent partial compilations from
258265
# ever needing to include the object.id.
259-
not "*" in input.subject.scope.allow_list
260-
input.object.id in input.subject.scope.allow_list
266+
not {"type": "*", "id": "*"} in input.subject.scope.allow_list
267+
# This is equivalent to the above line, as `type` is known at partial query time.
268+
not {"type": input.object.type, "id": "*"} in input.subject.scope.allow_list
269+
270+
# allows_ids is the set of all ids allowed for the given object.type
271+
allowed_ids := {allowed_id |
272+
# Iterate over all allow list elements
273+
ele := input.subject.scope.allow_list[_]
274+
ele.type in [input.object.type, "*"]
275+
allowed_id := ele.id
276+
}
277+
278+
# Return if the object.id is in the allowed ids
279+
# This rule is evaluated at the end so the partial query can use the object.id
280+
# against this precomputed set of allowed ids.
281+
input.object.id in allowed_ids
261282
}
262283

263284
# -------------------

‎coderd/rbac/scopes.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,15 @@ func WorkspaceAgentScope(params WorkspaceAgentScopeParams) Scope {
4545
// incase we change the behavior of the allowlist. The allowlist is new
4646
// and evolving.
4747
Role: scope.Role,
48-
// This prevents the agent from being able to access any other resource.
49-
// Include the list of IDs of anything that is required for the
50-
// agent to function.
51-
AllowIDList: []string{
52-
params.WorkspaceID.String(),
53-
params.TemplateID.String(),
54-
params.VersionID.String(),
55-
params.OwnerID.String(),
48+
49+
// Limit the agent to only be able to access the singular workspace and
50+
// the template/version it was created from. Add additional resources here
51+
// as needed, but do not add more workspace or template resource ids.
52+
AllowIDList: []AllowListElement{
53+
{Type: ResourceWorkspace.Type, ID: params.WorkspaceID.String()},
54+
{Type: ResourceTemplate.Type, ID: params.TemplateID.String()},
55+
{Type: ResourceTemplate.Type, ID: params.VersionID.String()},
56+
{Type: ResourceUser.Type, ID: params.OwnerID.String()},
5657
},
5758
}
5859
}
@@ -77,7 +78,7 @@ var builtinScopes = map[ScopeName]Scope{
7778
Org: map[string][]Permission{},
7879
User: []Permission{},
7980
},
80-
AllowIDList: []string{policy.WildcardSymbol},
81+
AllowIDList: []AllowListElement{AllowListAll()},
8182
},
8283

8384
ScopeApplicationConnect: {
@@ -90,7 +91,7 @@ var builtinScopes = map[ScopeName]Scope{
9091
Org: map[string][]Permission{},
9192
User: []Permission{},
9293
},
93-
AllowIDList: []string{policy.WildcardSymbol},
94+
AllowIDList: []AllowListElement{AllowListAll()},
9495
},
9596

9697
ScopeNoUserData: {
@@ -101,7 +102,7 @@ var builtinScopes = map[ScopeName]Scope{
101102
Org: map[string][]Permission{},
102103
User: []Permission{},
103104
},
104-
AllowIDList: []string{policy.WildcardSymbol},
105+
AllowIDList: []AllowListElement{AllowListAll()},
105106
},
106107
}
107108

@@ -129,7 +130,17 @@ func (name ScopeName) Name() RoleIdentifier {
129130
// AllowIDList. Eg: 'AllowIDList: []string{WildcardSymbol}'
130131
type Scope struct {
131132
Role
132-
AllowIDList []string `json:"allow_list"`
133+
AllowIDList []AllowListElement `json:"allow_list"`
134+
}
135+
136+
type AllowListElement struct {
137+
// ID must be a string to allow for the wildcard symbol.
138+
ID string `json:"id"`
139+
Type string `json:"type"`
140+
}
141+
142+
func AllowListAll() AllowListElement {
143+
return AllowListElement{ID: policy.WildcardSymbol, Type: policy.WildcardSymbol}
133144
}
134145

135146
func (s Scope) Expand() (Scope, error) {

0 commit comments

Comments
 (0)