From d1b6de6c11ba05a756b6380c814df968fb75091a Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 31 Jul 2024 15:22:41 +0200 Subject: [PATCH 01/11] Document RBAC usage Signed-off-by: Danny Kopping --- coderd/rbac/README.md | 6 +- coderd/rbac/USAGE.md | 318 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 321 insertions(+), 3 deletions(-) create mode 100644 coderd/rbac/USAGE.md diff --git a/coderd/rbac/README.md b/coderd/rbac/README.md index e867fa9cce50a..67e21368b2f32 100644 --- a/coderd/rbac/README.md +++ b/coderd/rbac/README.md @@ -1,15 +1,15 @@ # Authz -Package `authz` implements AuthoriZation for Coder. +Package `rbac` implements Role-Based Access Control for Coder. ## Overview Authorization defines what **permission** a **subject** has to perform **actions** to **objects**: - **Permission** is binary: _yes_ (allowed) or _no_ (denied). -- **Subject** in this case is anything that implements interface `authz.Subject`. +- **Subject** in this case is anything that implements interface `rbac.Subject`. - **Action** here is an enumerated list of actions, but we stick to `Create`, `Read`, `Update`, and `Delete` here. -- **Object** here is anything that implements `authz.Object`. +- **Object** here is anything that implements `rbac.Object`. ## Permission Structure diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md new file mode 100644 index 0000000000000..bd2d053463f5e --- /dev/null +++ b/coderd/rbac/USAGE.md @@ -0,0 +1,318 @@ +# Using RBAC + +# Overview + +> _NOTE: you should probably read [`README.md`](README.md) beforehand, but it's not essential._ + +## Basic structure + +RBAC is made up of nouns (the objects which are protected by RBAC rules) and verbs (actions which can be performed on +nouns).
+For example, a **workspace** (noun) can be **created** (verb), provided the requester has appropriate permissions. + +## Roles + +We have a number of roles (some of which have legacy connotations back to v1). + +These can be found in `coderd/rbac/roles.go`. + +| Role | Description | Example resources (non-exhaustive) | +|----------------------|--------------------------------------------------------------------|----------------------------------------------| +| **owner** | Super-user, first user in Coder installation, has all* permissions | all* | +| **member** | A regular user | workspaces, own details, provisioner daemons | +| **auditor** | Viewer of audit log events, read-only access to a few resources | audit logs, templates, users, groups | +| **templateAdmin** | Administrator of templates, read-only access to a few resources | templates, workspaces, users, groups | +| **userAdmin** | Administrator of users | users, groups, role assignments | +| **orgAdmin** | Like **owner**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgMember** | Like **member**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgAuditor** | Like **auditor**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgUserAdmin** | Like **userAdmin**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgTemplateAdmin** | Like **templateAdmin**, but scoped to a single organization | _(org-level equivalent)_ | + +_* except some, which are not important to this overview_ + +## Actions + +Roles are collections of permissions (we call them _actions_). + +These can be found in `coderd/rbac/policy/policy.go`. + +| Action | Description | +|-------------------------|-----------------------------------------| +| **create** | Create a resource | +| **read** | Read a resource | +| **update** | Update a resource | +| **delete** | Delete a resource | +| **use** | Use a resource | +| **read_personal** | Read owned resource | +| **update_personal** | Update owned resource | +| **ssh** | SSH into a workspace | +| **application_connect** | Connect to workspace apps via a browser | +| **view_insights** | View deployment insights | +| **start** | Start a workspace | +| **stop** | Stop a workspace | +| **assign** | Assign user to role / org | + +# Creating a new noun + +In the following example, we're going to create a new RBAC noun for a new entity called a "frobulator" _(just some nonsense word for demonstration purposes)_. + +_Refer to https://github.com/coder/coder/pull/14055 to see a full implementation._ + +## Creating a new entity + +If you're creating a new resource which has to be owned by users of differing roles, you need to create a new RBAC resource. + +Let's say we're adding a new table called `frobulators` (we'll use this table later): + +```sql +CREATE TABLE frobulators +( + id uuid NOT NULL, + user_id uuid NOT NULL, + model_number TEXT NOT NULL, + PRIMARY KEY (id), + UNIQUE (model_number), + FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE +); +``` + +Let's now add our frobulator noun to `coderd/rbac/policy/policy.go`: + +```go + ... + "frobulator": { + Actions: map[Action]ActionDefinition{ + ActionCreate: actDef("create a frobulator"), + ActionRead: actDef("read a frobulator"), + ActionUpdate: actDef("update a frobulator"), + ActionDelete: actDef("delete a frobulator"), + }, + }, + ... +``` + +Entries in the `frobulators` table be created/read/updated/deleted, so we define those actions. + +`policy.go` is used to generate code in `coderd/rbac/object_gen.go`, and we can execute this by running `make gen`. + +Now we have this change in `coderd/rbac/object_gen.go`: + +```go + ... + // ResourceFrobulator + // Valid Actions + // - "ActionCreate" :: + // - "ActionDelete" :: + // - "ActionRead" :: + // - "ActionUpdate" :: + ResourceFrobulator = Object{ + Type: "frobulator", + } + ... + + func AllResources() []Objecter { + ... + ResourceFrobulator, + ... + } +``` + +This creates a resource which represents this noun, and adds it to a list of all available resources. + +## Role Assignment + +In our case, we want **members** to be able to CRUD their own frobulators and we want **owners** to CRUD all members' frobulators. +This is how most resources work, and the RBAC system is setup for this by default. + +However, let's say we want **auditors** to have read-only access to all members' frobulators; we need to add it to `coderd/rbac/roles.go`: + +```go +func ReloadBuiltinRoles(opts *RoleOptions) { + ... + orgAuditor: func(organizationID uuid.UUID) Role { + ... + return Role{ + ... + Org: map[string][]Permission{ + organizationID.String(): Permissions(map[string][]policy.Action{ + ... + ResourceFrobulator.Type: {policy.ActionRead}, + }) + ... + ... +} +``` + +## Testing + +The RBAC system is configured to test all possible actions on all available resources. + +Let's run the RBAC test suite: + +`go test github.com/coder/coder/v2/coderd/rbac` + +We'll see a failure like this: + +```bash +--- FAIL: TestRolePermissions (0.61s) + --- FAIL: TestRolePermissions/frobulator-AllActions (0.00s) + roles_test.go:705: + Error Trace: /tmp/coder/coderd/rbac/roles_test.go:705 + Error: Not equal: + expected: map[policy.Action]bool{} + actual : map[policy.Action]bool{"create":true, "delete":true, "read":true, "update":true} + + Diff: + --- Expected + +++ Actual + @@ -1,2 +1,6 @@ + -(map[policy.Action]bool) { + +(map[policy.Action]bool) (len=4) { + + (policy.Action) (len=6) "create": (bool) true, + + (policy.Action) (len=6) "delete": (bool) true, + + (policy.Action) (len=4) "read": (bool) true, + + (policy.Action) (len=6) "update": (bool) true + } + Test: TestRolePermissions/frobulator-AllActions + Messages: remaining permissions should be empty for type "frobulator" +FAIL +FAIL github.com/coder/coder/v2/coderd/rbac 1.314s +FAIL +``` + +The message `remaining permissions should be empty for type "frobulator"` indicates that we're missing tests which validate +the desired actions on our new noun. + +> Take a look at `coderd/rbac/roles_test.go` in the [reference PR](https://github.com/coder/coder/pull/14055) for a complete example + +Let's add a test case: + +```go +func TestRolePermissions(t *testing.T) { + ... + { + // Users should be able to modify their own frobulators + // Admins from the current organization should be able to modify any other user's frobulators + // Owner should be able to modify any other user's frobulators + Name: "FrobulatorsModify", + Actions: []policy.Action{policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete}, + Resource: rbac.ResourceFrobulator.WithOwner(currentUser.String()).InOrg(orgID), + AuthorizeMap: map[bool][]hasAuthSubjects{ + true: {orgMemberMe, orgAdmin, owner}, + false: {setOtherOrg, memberMe, templateAdmin, userAdmin, orgTemplateAdmin, orgUserAdmin, orgAuditor}, + }, + }, + { + // Users should be able to read their own frobulators + // Admins from the current organization should be able to read any other user's frobulators + // Auditors should be able to read any other user's frobulators + // Owner should be able to read any other user's frobulators + Name: "FrobulatorsReadOnly", + Actions: []policy.Action{policy.ActionRead}, + Resource: rbac.ResourceFrobulator.WithOwner(currentUser.String()).InOrg(orgID), + AuthorizeMap: map[bool][]hasAuthSubjects{ + true: {orgMemberMe, orgAdmin, owner, orgAuditor}, + false: {setOtherOrg, memberMe, templateAdmin, userAdmin, orgTemplateAdmin, orgUserAdmin}, + }, + }, +``` + +Note how the `FrobulatorsModify` test case is just validating the `policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete` actions, +and only the **orgMember**, **orgAdmin**, and **owner** can access it. + +Similarly, the `FrobulatorsReadOnly` test case is only validating `policy.ActionRead`, which is allowed on all of the above +plus the **orgAuditor** role. + +Now the tests pass, because we have covered all the possible scenarios: + +```bash +$ go test github.com/coder/coder/v2/coderd/rbac -count=1 +ok github.com/coder/coder/v2/coderd/rbac 1.313s +``` + +When a case is not covered, you'll see an error like this (I moved the `orgAuditor` option from `true` to `false): + +```bash +--- FAIL: TestRolePermissions (0.79s) + --- FAIL: TestRolePermissions/FrobulatorsReadOnly (0.01s) + roles_test.go:737: + Error Trace: /tmp/coder/coderd/rbac/roles_test.go:737 + Error: An error is expected but got nil. + Test: TestRolePermissions/FrobulatorsReadOnly + Messages: Should fail: FrobulatorsReadOnly as "org_auditor" doing "read" on "frobulator" +FAIL +FAIL github.com/coder/coder/v2/coderd/rbac 1.390s +FAIL +``` + +This shows you that the `org_auditor` role has `read` permissions on the frobulator, but no test case covered it. + +**NOTE: don't just add cases which make the tests pass; consider all the way in which your resource must be used, and test +all of those scenarios!** + +# Database authorization + +Now that we have the RBAC system fully configured, we need to make use of it. + +Let's add a SQL query to `coderd/database/queries/frobulators.sql`: + +```sql +-- name: GetUserFrobulators :many +SELECT * +FROM frobulators +WHERE user_id = @user_id::uuid; +``` + +Once we run `make gen`, we'll find some stubbed code in `coderd/database/dbauthz/dbauthz.go`. + +```go +... +func (q *querier) GetUserFrobulators(ctx context.Context) ([]database.Frobulator, error) { + panic("not implemented") +} +... +``` + +Let's modify this function: + +```go +... +func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]database.Frobulator, error) { + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(userID.String())); err != nil { + return nil, err + } + return q.db.GetUserFrobulators(ctx, userID) +} +... +``` + +This states that the `policy.ActionRead` permission is required in this query on the `ResourceFrobulator` resources, +and `WithOwner(userID.String())` specifies that this user must own the resource. + +All queries are executed through `dbauthz`, and now our little frobulators are protected! + +# API authorization + +API authorization is not strictly required because we have database authorization in place, but it's a good practice to +reject requests as soon as possible when the requester is unprivileged. + +> Take a look at `coderd/frobulators.go` in the [reference PR](https://github.com/coder/coder/pull/14055) for a complete example + +```go +... +func (api *API) listUserFrobulators(rw http.ResponseWriter, r *http.Request) { + ctx := r.Context() + key := httpmw.APIKey(r) + if !api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String())) { + httpapi.Forbidden(rw) + return + } + ... +} +``` + +`api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String()))` is specifying that we only +want to permit a user to read their own frobulators. If the requester does not have this permission, we forbid the request. +We're checking the user associated to the API key here because this could also be an **owner** or **orgAdmin**, and we want to +permit those users. \ No newline at end of file From 23e01c55444b71df799bb293d5e6596374977202 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 31 Jul 2024 15:52:59 +0200 Subject: [PATCH 02/11] make fmt Signed-off-by: Danny Kopping --- coderd/rbac/USAGE.md | 148 +++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 61 deletions(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index bd2d053463f5e..f002739540443 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -2,13 +2,15 @@ # Overview -> _NOTE: you should probably read [`README.md`](README.md) beforehand, but it's not essential._ +> _NOTE: you should probably read [`README.md`](README.md) beforehand, but it's +> not essential._ ## Basic structure -RBAC is made up of nouns (the objects which are protected by RBAC rules) and verbs (actions which can be performed on -nouns).
-For example, a **workspace** (noun) can be **created** (verb), provided the requester has appropriate permissions. +RBAC is made up of nouns (the objects which are protected by RBAC rules) and +verbs (actions which can be performed on nouns).
For example, a +**workspace** (noun) can be **created** (verb), provided the requester has +appropriate permissions. ## Roles @@ -16,20 +18,20 @@ We have a number of roles (some of which have legacy connotations back to v1). These can be found in `coderd/rbac/roles.go`. -| Role | Description | Example resources (non-exhaustive) | -|----------------------|--------------------------------------------------------------------|----------------------------------------------| -| **owner** | Super-user, first user in Coder installation, has all* permissions | all* | -| **member** | A regular user | workspaces, own details, provisioner daemons | -| **auditor** | Viewer of audit log events, read-only access to a few resources | audit logs, templates, users, groups | -| **templateAdmin** | Administrator of templates, read-only access to a few resources | templates, workspaces, users, groups | -| **userAdmin** | Administrator of users | users, groups, role assignments | -| **orgAdmin** | Like **owner**, but scoped to a single organization | _(org-level equivalent)_ | -| **orgMember** | Like **member**, but scoped to a single organization | _(org-level equivalent)_ | -| **orgAuditor** | Like **auditor**, but scoped to a single organization | _(org-level equivalent)_ | -| **orgUserAdmin** | Like **userAdmin**, but scoped to a single organization | _(org-level equivalent)_ | -| **orgTemplateAdmin** | Like **templateAdmin**, but scoped to a single organization | _(org-level equivalent)_ | - -_* except some, which are not important to this overview_ +| Role | Description | Example resources (non-exhaustive) | +| -------------------- | ------------------------------------------------------------------- | -------------------------------------------- | +| **owner** | Super-user, first user in Coder installation, has all\* permissions | all\* | +| **member** | A regular user | workspaces, own details, provisioner daemons | +| **auditor** | Viewer of audit log events, read-only access to a few resources | audit logs, templates, users, groups | +| **templateAdmin** | Administrator of templates, read-only access to a few resources | templates, workspaces, users, groups | +| **userAdmin** | Administrator of users | users, groups, role assignments | +| **orgAdmin** | Like **owner**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgMember** | Like **member**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgAuditor** | Like **auditor**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgUserAdmin** | Like **userAdmin**, but scoped to a single organization | _(org-level equivalent)_ | +| **orgTemplateAdmin** | Like **templateAdmin**, but scoped to a single organization | _(org-level equivalent)_ | + +_\* except some, which are not important to this overview_ ## Actions @@ -38,7 +40,7 @@ Roles are collections of permissions (we call them _actions_). These can be found in `coderd/rbac/policy/policy.go`. | Action | Description | -|-------------------------|-----------------------------------------| +| ----------------------- | --------------------------------------- | | **create** | Create a resource | | **read** | Read a resource | | **update** | Update a resource | @@ -55,15 +57,19 @@ These can be found in `coderd/rbac/policy/policy.go`. # Creating a new noun -In the following example, we're going to create a new RBAC noun for a new entity called a "frobulator" _(just some nonsense word for demonstration purposes)_. +In the following example, we're going to create a new RBAC noun for a new entity +called a "frobulator" _(just some nonsense word for demonstration purposes)_. -_Refer to https://github.com/coder/coder/pull/14055 to see a full implementation._ +_Refer to https://github.com/coder/coder/pull/14055 to see a full +implementation._ ## Creating a new entity -If you're creating a new resource which has to be owned by users of differing roles, you need to create a new RBAC resource. +If you're creating a new resource which has to be owned by users of differing +roles, you need to create a new RBAC resource. -Let's say we're adding a new table called `frobulators` (we'll use this table later): +Let's say we're adding a new table called `frobulators` (we'll use this table +later): ```sql CREATE TABLE frobulators @@ -92,15 +98,17 @@ Let's now add our frobulator noun to `coderd/rbac/policy/policy.go`: ... ``` -Entries in the `frobulators` table be created/read/updated/deleted, so we define those actions. +Entries in the `frobulators` table be created/read/updated/deleted, so we define +those actions. -`policy.go` is used to generate code in `coderd/rbac/object_gen.go`, and we can execute this by running `make gen`. +`policy.go` is used to generate code in `coderd/rbac/object_gen.go`, and we can +execute this by running `make gen`. Now we have this change in `coderd/rbac/object_gen.go`: ```go ... - // ResourceFrobulator + // ResourceFrobulator // Valid Actions // - "ActionCreate" :: // - "ActionDelete" :: @@ -109,23 +117,26 @@ Now we have this change in `coderd/rbac/object_gen.go`: ResourceFrobulator = Object{ Type: "frobulator", } - ... + ... - func AllResources() []Objecter { - ... - ResourceFrobulator, - ... - } + func AllResources() []Objecter { + ... + ResourceFrobulator, + ... + } ``` -This creates a resource which represents this noun, and adds it to a list of all available resources. +This creates a resource which represents this noun, and adds it to a list of all +available resources. ## Role Assignment -In our case, we want **members** to be able to CRUD their own frobulators and we want **owners** to CRUD all members' frobulators. -This is how most resources work, and the RBAC system is setup for this by default. +In our case, we want **members** to be able to CRUD their own frobulators and we +want **owners** to CRUD all members' frobulators. This is how most resources +work, and the RBAC system is setup for this by default. -However, let's say we want **auditors** to have read-only access to all members' frobulators; we need to add it to `coderd/rbac/roles.go`: +However, let's say we want **auditors** to have read-only access to all members' +frobulators; we need to add it to `coderd/rbac/roles.go`: ```go func ReloadBuiltinRoles(opts *RoleOptions) { @@ -133,20 +144,21 @@ func ReloadBuiltinRoles(opts *RoleOptions) { orgAuditor: func(organizationID uuid.UUID) Role { ... return Role{ - ... + ... Org: map[string][]Permission{ organizationID.String(): Permissions(map[string][]policy.Action{ ... ResourceFrobulator.Type: {policy.ActionRead}, }) - ... + ... ... } ``` ## Testing -The RBAC system is configured to test all possible actions on all available resources. +The RBAC system is configured to test all possible actions on all available +resources. Let's run the RBAC test suite: @@ -181,10 +193,13 @@ FAIL github.com/coder/coder/v2/coderd/rbac 1.314s FAIL ``` -The message `remaining permissions should be empty for type "frobulator"` indicates that we're missing tests which validate -the desired actions on our new noun. +The message `remaining permissions should be empty for type "frobulator"` +indicates that we're missing tests which validate the desired actions on our new +noun. -> Take a look at `coderd/rbac/roles_test.go` in the [reference PR](https://github.com/coder/coder/pull/14055) for a complete example +> Take a look at `coderd/rbac/roles_test.go` in the +> [reference PR](https://github.com/coder/coder/pull/14055) for a complete +> example Let's add a test case: @@ -218,11 +233,13 @@ func TestRolePermissions(t *testing.T) { }, ``` -Note how the `FrobulatorsModify` test case is just validating the `policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete` actions, -and only the **orgMember**, **orgAdmin**, and **owner** can access it. +Note how the `FrobulatorsModify` test case is just validating the +`policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete` actions, and +only the **orgMember**, **orgAdmin**, and **owner** can access it. -Similarly, the `FrobulatorsReadOnly` test case is only validating `policy.ActionRead`, which is allowed on all of the above -plus the **orgAuditor** role. +Similarly, the `FrobulatorsReadOnly` test case is only validating +`policy.ActionRead`, which is allowed on all of the above plus the +**orgAuditor** role. Now the tests pass, because we have covered all the possible scenarios: @@ -231,7 +248,8 @@ $ go test github.com/coder/coder/v2/coderd/rbac -count=1 ok github.com/coder/coder/v2/coderd/rbac 1.313s ``` -When a case is not covered, you'll see an error like this (I moved the `orgAuditor` option from `true` to `false): +When a case is not covered, you'll see an error like this (I moved the +`orgAuditor` option from `true` to `false): ```bash --- FAIL: TestRolePermissions (0.79s) @@ -246,10 +264,11 @@ FAIL github.com/coder/coder/v2/coderd/rbac 1.390s FAIL ``` -This shows you that the `org_auditor` role has `read` permissions on the frobulator, but no test case covered it. +This shows you that the `org_auditor` role has `read` permissions on the +frobulator, but no test case covered it. -**NOTE: don't just add cases which make the tests pass; consider all the way in which your resource must be used, and test -all of those scenarios!** +**NOTE: don't just add cases which make the tests pass; consider all the way in +which your resource must be used, and test all of those scenarios!** # Database authorization @@ -264,7 +283,8 @@ FROM frobulators WHERE user_id = @user_id::uuid; ``` -Once we run `make gen`, we'll find some stubbed code in `coderd/database/dbauthz/dbauthz.go`. +Once we run `make gen`, we'll find some stubbed code in +`coderd/database/dbauthz/dbauthz.go`. ```go ... @@ -287,17 +307,22 @@ func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]d ... ``` -This states that the `policy.ActionRead` permission is required in this query on the `ResourceFrobulator` resources, -and `WithOwner(userID.String())` specifies that this user must own the resource. +This states that the `policy.ActionRead` permission is required in this query on +the `ResourceFrobulator` resources, and `WithOwner(userID.String())` specifies +that this user must own the resource. -All queries are executed through `dbauthz`, and now our little frobulators are protected! +All queries are executed through `dbauthz`, and now our little frobulators are +protected! # API authorization -API authorization is not strictly required because we have database authorization in place, but it's a good practice to -reject requests as soon as possible when the requester is unprivileged. +API authorization is not strictly required because we have database +authorization in place, but it's a good practice to reject requests as soon as +possible when the requester is unprivileged. -> Take a look at `coderd/frobulators.go` in the [reference PR](https://github.com/coder/coder/pull/14055) for a complete example +> Take a look at `coderd/frobulators.go` in the +> [reference PR](https://github.com/coder/coder/pull/14055) for a complete +> example ```go ... @@ -312,7 +337,8 @@ func (api *API) listUserFrobulators(rw http.ResponseWriter, r *http.Request) { } ``` -`api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String()))` is specifying that we only -want to permit a user to read their own frobulators. If the requester does not have this permission, we forbid the request. -We're checking the user associated to the API key here because this could also be an **owner** or **orgAdmin**, and we want to -permit those users. \ No newline at end of file +`api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String()))` +is specifying that we only want to permit a user to read their own frobulators. +If the requester does not have this permission, we forbid the request. We're +checking the user associated to the API key here because this could also be an +**owner** or **orgAdmin**, and we want to permit those users. From 706a3d8b442f7afb94222dc365611c2f3c5bfd3d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 Jul 2024 09:16:55 -0500 Subject: [PATCH 03/11] chore: fixup rbac/readme.md typos - Truth table had an incorrect result value in final row - Permission format examples was missing the object type - Fix actions list - Code block a bash command --- coderd/rbac/README.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/coderd/rbac/README.md b/coderd/rbac/README.md index 67e21368b2f32..fdd23a43b6aa6 100644 --- a/coderd/rbac/README.md +++ b/coderd/rbac/README.md @@ -8,7 +8,7 @@ Authorization defines what **permission** a **subject** has to perform **actions - **Permission** is binary: _yes_ (allowed) or _no_ (denied). - **Subject** in this case is anything that implements interface `rbac.Subject`. -- **Action** here is an enumerated list of actions, but we stick to `Create`, `Read`, `Update`, and `Delete` here. +- **Action** here is an enumerated list of actions. Actions can differ for each object type. They typically read like, `Create`, `Read`, `Update`, `Delete`, etc. - **Object** here is anything that implements `rbac.Object`. ## Permission Structure @@ -34,11 +34,11 @@ Both **negative** and **positive** permissions override **abstain** at the same This can be represented by the following truth table, where Y represents _positive_, N represents _negative_, and \_ represents _abstain_: | Action | Positive | Negative | Result | -| ------ | -------- | -------- | ------ | +| ------ | -------- | -------- |--------| | read | Y | \_ | Y | | read | Y | N | N | | read | \_ | \_ | \_ | -| read | \_ | N | Y | +| read | \_ | N | N | ## Permission Representation @@ -49,11 +49,11 @@ This can be represented by the following truth table, where Y represents _positi - `object` is any valid resource type. - `id` is any valid UUID v4. - `id` is included in the permission syntax, however only scopes may use `id` to specify a specific object. -- `action` is `create`, `read`, `modify`, or `delete`. +- `action` is `create`, `read`, `modify`, `delete`, or another verb. ## Example Permissions -- `+site.*.*.read`: allowed to perform the `read` action against all objects of type `app` in a given Coder deployment. +- `+site.app.*.read`: allowed to perform the `read` action against all objects of type `app` in a given Coder deployment. - `-user.workspace.*.create`: user is not allowed to create workspaces. ## Roles @@ -106,7 +106,9 @@ You can test outside of golang by using the `opa` cli. **Evaluation** +```bash opa eval --format=pretty "data.authz.allow" -d policy.rego -i input.json +``` **Partial Evaluation** From 4681b9d53cd961f71507332fabcc911ee1f6f45f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 31 Jul 2024 21:42:00 +0100 Subject: [PATCH 04/11] make fumpt --- coderd/rbac/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/README.md b/coderd/rbac/README.md index fdd23a43b6aa6..462ba0ee230b7 100644 --- a/coderd/rbac/README.md +++ b/coderd/rbac/README.md @@ -34,7 +34,7 @@ Both **negative** and **positive** permissions override **abstain** at the same This can be represented by the following truth table, where Y represents _positive_, N represents _negative_, and \_ represents _abstain_: | Action | Positive | Negative | Result | -| ------ | -------- | -------- |--------| +| ------ | -------- | -------- | ------ | | read | Y | \_ | Y | | read | Y | N | N | | read | \_ | \_ | \_ | From 6f240c90a7ae2c64240e0ef9f5fec3dad5445f7a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 1 Aug 2024 14:33:21 +0100 Subject: [PATCH 05/11] Apply suggestions from code review Co-authored-by: Steven Masley --- coderd/rbac/USAGE.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index f002739540443..ec993228efe2b 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -31,6 +31,8 @@ These can be found in `coderd/rbac/roles.go`. | **orgUserAdmin** | Like **userAdmin**, but scoped to a single organization | _(org-level equivalent)_ | | **orgTemplateAdmin** | Like **templateAdmin**, but scoped to a single organization | _(org-level equivalent)_ | +**Note an example resource indicates the role has at least 1 permission related to the resource. Not that the role has complete CRUD access to the resource.** + _\* except some, which are not important to this overview_ ## Actions @@ -135,7 +137,7 @@ In our case, we want **members** to be able to CRUD their own frobulators and we want **owners** to CRUD all members' frobulators. This is how most resources work, and the RBAC system is setup for this by default. -However, let's say we want **auditors** to have read-only access to all members' +However, let's say we want **organization auditors** to have read-only access to all organization's frobulators; we need to add it to `coderd/rbac/roles.go`: ```go @@ -208,7 +210,7 @@ func TestRolePermissions(t *testing.T) { ... { // Users should be able to modify their own frobulators - // Admins from the current organization should be able to modify any other user's frobulators + // Admins from the current organization should be able to modify any other members' frobulators // Owner should be able to modify any other user's frobulators Name: "FrobulatorsModify", Actions: []policy.Action{policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete}, @@ -220,8 +222,8 @@ func TestRolePermissions(t *testing.T) { }, { // Users should be able to read their own frobulators - // Admins from the current organization should be able to read any other user's frobulators - // Auditors should be able to read any other user's frobulators + // Admins from the current organization should be able to read any other members' frobulators + // Auditors should be able to read any other members' frobulators // Owner should be able to read any other user's frobulators Name: "FrobulatorsReadOnly", Actions: []policy.Action{policy.ActionRead}, @@ -299,13 +301,9 @@ Let's modify this function: ```go ... func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]database.Frobulator, error) { - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(userID.String())); err != nil { - return nil, err - } - return q.db.GetUserFrobulators(ctx, userID) + return fetch(q.log, q.auth, q.db.GetUserFrobulators)(ctx, id) } ... -``` This states that the `policy.ActionRead` permission is required in this query on the `ResourceFrobulator` resources, and `WithOwner(userID.String())` specifies From c2f29d0a3becf6772b630e6f6b9960913cdaf303 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 1 Aug 2024 14:36:03 +0100 Subject: [PATCH 06/11] make fmt --- coderd/rbac/USAGE.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index ec993228efe2b..9c513e3c55d8e 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -31,7 +31,8 @@ These can be found in `coderd/rbac/roles.go`. | **orgUserAdmin** | Like **userAdmin**, but scoped to a single organization | _(org-level equivalent)_ | | **orgTemplateAdmin** | Like **templateAdmin**, but scoped to a single organization | _(org-level equivalent)_ | -**Note an example resource indicates the role has at least 1 permission related to the resource. Not that the role has complete CRUD access to the resource.** +**Note an example resource indicates the role has at least 1 permission related +to the resource. Not that the role has complete CRUD access to the resource.** _\* except some, which are not important to this overview_ @@ -137,8 +138,8 @@ In our case, we want **members** to be able to CRUD their own frobulators and we want **owners** to CRUD all members' frobulators. This is how most resources work, and the RBAC system is setup for this by default. -However, let's say we want **organization auditors** to have read-only access to all organization's -frobulators; we need to add it to `coderd/rbac/roles.go`: +However, let's say we want **organization auditors** to have read-only access to +all organization's frobulators; we need to add it to `coderd/rbac/roles.go`: ```go func ReloadBuiltinRoles(opts *RoleOptions) { @@ -304,6 +305,7 @@ func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]d return fetch(q.log, q.auth, q.db.GetUserFrobulators)(ctx, id) } ... +``` This states that the `policy.ActionRead` permission is required in this query on the `ResourceFrobulator` resources, and `WithOwner(userID.String())` specifies From 26ca907e4b8108791f3ea0b672c5dc8d7bf79f6a Mon Sep 17 00:00:00 2001 From: Stephen Kirby <58410745+stirby@users.noreply.github.com> Date: Mon, 2 Sep 2024 14:47:38 -0500 Subject: [PATCH 07/11] Minor fixups, added troubleshooting (#14519) (#14530) (cherry picked from commit 66c806060543720b6063db9b6183d7e3dda53bbd) Co-authored-by: Danny Kopping From cf2574651f7f4f5680f3b27677d3036ba653f794 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Tue, 10 Sep 2024 11:02:37 +0200 Subject: [PATCH 08/11] Update usage docs Signed-off-by: Danny Kopping --- coderd/rbac/USAGE.md | 133 ++++++++++++++++++++++++++++++++----------- 1 file changed, 100 insertions(+), 33 deletions(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index 9c513e3c55d8e..62c7e58a5e04a 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -68,8 +68,8 @@ implementation._ ## Creating a new entity -If you're creating a new resource which has to be owned by users of differing -roles, you need to create a new RBAC resource. +If you're creating a new resource which has to be acted upon by users of +differing roles, you need to create a new RBAC resource. Let's say we're adding a new table called `frobulators` (we'll use this table later): @@ -79,10 +79,12 @@ CREATE TABLE frobulators ( id uuid NOT NULL, user_id uuid NOT NULL, + org_id uuid NOT NULL, model_number TEXT NOT NULL, PRIMARY KEY (id), UNIQUE (model_number), - FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE + FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE, + FOREIGN KEY (org_id) REFERENCES organizations (id) ON DELETE CASCADE ); ``` @@ -92,10 +94,10 @@ Let's now add our frobulator noun to `coderd/rbac/policy/policy.go`: ... "frobulator": { Actions: map[Action]ActionDefinition{ - ActionCreate: actDef("create a frobulator"), - ActionRead: actDef("read a frobulator"), - ActionUpdate: actDef("update a frobulator"), - ActionDelete: actDef("delete a frobulator"), + ActionCreate: {Description: "create a frobulator"}, + ActionRead: {Description: "read a frobulator"}, + ActionUpdate: {Description: "update a frobulator"}, + ActionDelete: {Description: "delete a frobulator"}, }, }, ... @@ -144,6 +146,16 @@ all organization's frobulators; we need to add it to `coderd/rbac/roles.go`: ```go func ReloadBuiltinRoles(opts *RoleOptions) { ... + auditorRole := Role{ + Identifier: RoleAuditor(), + DisplayName: "Auditor", + Site: Permissions(map[string][]policy.Action{ + ... + // The site-wide auditor is allowed to read *all* frobulators, regardless of who owns them. + ResourceFrobulator.Type: {policy.ActionRead}, + ... + + // orgAuditor: func(organizationID uuid.UUID) Role { ... return Role{ @@ -151,6 +163,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { Org: map[string][]Permission{ organizationID.String(): Permissions(map[string][]policy.Action{ ... + // The org-wide auditor is allowed to read *all* frobulators in their own org, regardless of who owns them. ResourceFrobulator.Type: {policy.ActionRead}, }) ... @@ -158,6 +171,9 @@ func ReloadBuiltinRoles(opts *RoleOptions) { } ``` +Note how we added the permission to both the **site-wide** auditor role and the +**org-level** auditor role. + ## Testing The RBAC system is configured to test all possible actions on all available @@ -222,16 +238,15 @@ func TestRolePermissions(t *testing.T) { }, }, { - // Users should be able to read their own frobulators // Admins from the current organization should be able to read any other members' frobulators // Auditors should be able to read any other members' frobulators // Owner should be able to read any other user's frobulators - Name: "FrobulatorsReadOnly", + Name: "FrobulatorsReadAnyUserInOrg", Actions: []policy.Action{policy.ActionRead}, - Resource: rbac.ResourceFrobulator.WithOwner(currentUser.String()).InOrg(orgID), + Resource: rbac.ResourceFrobulator.WithOwner(uuid.New().String()).InOrg(orgID), // read frobulators of any user AuthorizeMap: map[bool][]hasAuthSubjects{ - true: {orgMemberMe, orgAdmin, owner, orgAuditor}, - false: {setOtherOrg, memberMe, templateAdmin, userAdmin, orgTemplateAdmin, orgUserAdmin}, + true: {owner, orgAdmin, orgAuditor}, + false: {memberMe, orgMemberMe, setOtherOrg, templateAdmin, userAdmin, orgTemplateAdmin, orgUserAdmin}, }, }, ``` @@ -240,11 +255,14 @@ Note how the `FrobulatorsModify` test case is just validating the `policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete` actions, and only the **orgMember**, **orgAdmin**, and **owner** can access it. -Similarly, the `FrobulatorsReadOnly` test case is only validating -`policy.ActionRead`, which is allowed on all of the above plus the -**orgAuditor** role. +The `FrobulatorsReadAnyUserInOrg` test case is validating that owners, org +admins & auditors have the `policy.ActionRead` policy which enables them to read +frobulators belonging to any user in a given organization. + +The above tests are illustrative not exhaustive, see +[the reference PR](https://github.com/coder/coder/pull/14055) for the rest. -Now the tests pass, because we have covered all the possible scenarios: +Once we have covered all the possible scenarios, the tests will pass: ```bash $ go test github.com/coder/coder/v2/coderd/rbac -count=1 @@ -280,10 +298,10 @@ Now that we have the RBAC system fully configured, we need to make use of it. Let's add a SQL query to `coderd/database/queries/frobulators.sql`: ```sql --- name: GetUserFrobulators :many +-- name: GetFrobulators :many SELECT * FROM frobulators -WHERE user_id = @user_id::uuid; +WHERE user_id = $1 AND org_id = $2; ``` Once we run `make gen`, we'll find some stubbed code in @@ -291,7 +309,7 @@ Once we run `make gen`, we'll find some stubbed code in ```go ... -func (q *querier) GetUserFrobulators(ctx context.Context) ([]database.Frobulator, error) { +func (q *querier) GetFrobulators(ctx context.Context, arg database.GetFrobulatorsParams) ([]database.Frobulator, error) { panic("not implemented") } ... @@ -301,15 +319,33 @@ Let's modify this function: ```go ... -func (q *querier) GetUserFrobulators(ctx context.Context, userID uuid.UUID) ([]database.Frobulator, error) { - return fetch(q.log, q.auth, q.db.GetUserFrobulators)(ctx, id) +func (q *querier) GetFrobulators(ctx context.Context, arg database.GetFrobulatorsParams) ([]database.Frobulator, error) { + return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetFrobulators)(ctx, arg) } ... ``` -This states that the `policy.ActionRead` permission is required in this query on -the `ResourceFrobulator` resources, and `WithOwner(userID.String())` specifies -that this user must own the resource. +This states that the `policy.ActionRead` permission is enforced on all entries +returned from the database, ensuring that each requested frobulator is readable +by the given actor. + +In order for this to work, we need to implement the `rbac.Objector` interface. + +`coderd/database/modelmethods.go` is where we implement this interface for all +RBAC objects: + +```go +func (f Frobulator) RBACObject() rbac.Object { + return rbac.ResourceFrobulator. + WithID(f.ID). // Each frobulator has a unique identity. + WithOwner(f.UserID.String()). // It is owned by one and only one user. + InOrg(f.OrgID) // It belongs to an organization. +} +``` + +These values obviously have to be set on the `Frobulator` instance before this +function can work, hence why we have to fetch the object from the store first +before we validate (this explains the `fetchWithPostFilter` naming). All queries are executed through `dbauthz`, and now our little frobulators are protected! @@ -326,19 +362,50 @@ possible when the requester is unprivileged. ```go ... -func (api *API) listUserFrobulators(rw http.ResponseWriter, r *http.Request) { +func (api *API) createFrobulator(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - key := httpmw.APIKey(r) - if !api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String())) { - httpapi.Forbidden(rw) + member := httpmw.OrganizationMemberParam(r) + org := httpmw.OrganizationParam(r) + + var req codersdk.InsertFrobulatorRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + frob, err := api.Database.InsertFrobulator(ctx, database.InsertFrobulatorParams{ + ID: uuid.New(), + UserID: member.UserID, + OrgID: org.ID, + ModelNumber: req.ModelNumber, + }) + + // This will catch forbidden errors as well. + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) return } ... +``` + +If we look at the implementation of `httpapi.Is404Error`: + +```go +// Is404Error returns true if the given error should return a 404 status code. +// Both actual 404s and unauthorized errors should return 404s to not leak +// information about the existence of resources. +func Is404Error(err error) bool { + if err == nil { + return false + } + + // This tests for dbauthz.IsNotAuthorizedError and rbac.IsUnauthorizedError. + if IsUnauthorizedError(err) { + return true + } + return xerrors.Is(err, sql.ErrNoRows) } ``` -`api.Authorize(r, policy.ActionRead, rbac.ResourceFrobulator.WithOwner(key.UserID.String()))` -is specifying that we only want to permit a user to read their own frobulators. -If the requester does not have this permission, we forbid the request. We're -checking the user associated to the API key here because this could also be an -**owner** or **orgAdmin**, and we want to permit those users. +With this, we're able to handle unauthorized access to the resource but return a +`404 Not Found` to not leak the fact that the resources exist but are not +accessible by the given actor. From 5fa2b96bc12fe7ca5a41c772871eb585a3a0718d Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Tue, 10 Sep 2024 13:05:03 +0200 Subject: [PATCH 09/11] Apply suggestions from code review Co-authored-by: Cian Johnston --- coderd/rbac/README.md | 2 +- coderd/rbac/USAGE.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/README.md b/coderd/rbac/README.md index 462ba0ee230b7..e4d217d303b2f 100644 --- a/coderd/rbac/README.md +++ b/coderd/rbac/README.md @@ -49,7 +49,7 @@ This can be represented by the following truth table, where Y represents _positi - `object` is any valid resource type. - `id` is any valid UUID v4. - `id` is included in the permission syntax, however only scopes may use `id` to specify a specific object. -- `action` is `create`, `read`, `modify`, `delete`, or another verb. +- `action` is typically `create`, `read`, `modify`, `delete`, but you can define other verbs as needed. ## Example Permissions diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index 62c7e58a5e04a..722ef50419bb4 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -103,7 +103,7 @@ Let's now add our frobulator noun to `coderd/rbac/policy/policy.go`: ... ``` -Entries in the `frobulators` table be created/read/updated/deleted, so we define +We need to create/read/update/delete rows in the `frobulators` table, so we define those actions. `policy.go` is used to generate code in `coderd/rbac/object_gen.go`, and we can @@ -288,7 +288,7 @@ FAIL This shows you that the `org_auditor` role has `read` permissions on the frobulator, but no test case covered it. -**NOTE: don't just add cases which make the tests pass; consider all the way in +**NOTE: don't just add cases which make the tests pass; consider all the ways in which your resource must be used, and test all of those scenarios!** # Database authorization From 257962b985d757a58a8a5436adf8ae522648b401 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Tue, 10 Sep 2024 16:36:29 +0200 Subject: [PATCH 10/11] Update coderd/rbac/USAGE.md Co-authored-by: Steven Masley --- coderd/rbac/USAGE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index 722ef50419bb4..ab2e750d025fd 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -270,7 +270,7 @@ ok github.com/coder/coder/v2/coderd/rbac 1.313s ``` When a case is not covered, you'll see an error like this (I moved the -`orgAuditor` option from `true` to `false): +`orgAuditor` option from `true` to `false`): ```bash --- FAIL: TestRolePermissions (0.79s) From 02f74473452421e3395bad21e0e65b3f105ad97a Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Tue, 10 Sep 2024 17:03:53 +0200 Subject: [PATCH 11/11] Appease the linter gods Signed-off-by: Danny Kopping --- coderd/rbac/USAGE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/rbac/USAGE.md b/coderd/rbac/USAGE.md index ab2e750d025fd..76bff69a88c5a 100644 --- a/coderd/rbac/USAGE.md +++ b/coderd/rbac/USAGE.md @@ -103,8 +103,8 @@ Let's now add our frobulator noun to `coderd/rbac/policy/policy.go`: ... ``` -We need to create/read/update/delete rows in the `frobulators` table, so we define -those actions. +We need to create/read/update/delete rows in the `frobulators` table, so we +define those actions. `policy.go` is used to generate code in `coderd/rbac/object_gen.go`, and we can execute this by running `make gen`.