Skip to content

Improve RBAC comprehensive testing #5204

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Emyrk opened this issue Nov 30, 2022 · 7 comments · Fixed by #5919
Closed

Improve RBAC comprehensive testing #5204

Emyrk opened this issue Nov 30, 2022 · 7 comments · Fixed by #5919
Assignees
@johnstcn @Emyrk
Labels
design needed Request for more beauty security Area: security

Comments

@Emyrk
Copy link
Member

Emyrk commented Nov 30, 2022 •
edited
Loading

Goal

Robust authorization testing to prevent security holes created by developer error. Reduce chance developer forgets to call Authorize, uses wrong rbac object/action, or a role is granted more privilege than it should have.

Current State

RBAC testing is broken up into 2 parts to achieve comprehensive tests across all the api routes.

  1. Assert various roles against given action+object combinations (link)
  • ✔️ owner, orgMemberMe, orgAdmin, templateAdmin
  • ❌ memberMe, otherOrgAdmin, otherOrgMember, userAdmin
  1. Assert a route checks a given action+object pair (link)

This makes testing a "route" with various user role combinations much simpler. Writing a unit test to check various roles against an endpoint is tedious and long. The above two unit tests give a high degree of confidence the correct rbac assertion will be made on the given route, and we know the user role combinations will return a true or false to that assertion.

Improve unit tests

Assert various roles

The current set of users and objects were created ad hoc. We should make sure all combinations of expected objects + actions that are hit in prod are covered.

Assert route checks

The current route check assertions are a bit weak.

Only asserts 1 rbac check per route

Firstly, only the last rbac assertion is checked. Some routes run 2 rbac checks (assuming the first one passed) (example). We need to have a way to assert all the checks that are run. Possibly requiring some design to allow the endpoint to run until the second Authorize check (as all the auth calls return false atm).

Idea (From @spikecurtis): If we have a route that calls authorize N times, we should run the assertion test on the route N times. Each call should have authorize fail on the Nth call.

Example: Route postWorkspacesByOrganization calls Authorize on create.workspace and read.template.

  1. Test will run 2 times on this route.
  2. 1st test runs and fails on create.workspace. Assert checks run assuming only 1 authorize call
  3. 2nd test runs and succeeds on create.workspace. Route continues and fails on read.template. Assert checks run assuming 2 authorize calls.

This is to ensure the route always fails as Unauthorized if any of the individual authorize calls returns false.

Asserts weakly

The object assertions do not check any fields not set. So for the workspace endpoint we do not check the Owner, OrgID, ACLUserList or ACLGroupList fields of the object. We only check the Type. We should assert exact matches.

Does not assert the caller's roles

We should also asser the roles of the user calling in this test. A non-admin user should be used. This would catch things where the wrong user is being asserted as the actor in cases where more than 1 user is involved. (eg creating a workspace for another user)
@spikecurtis
Copy link
Contributor

Assert a route checks a given action+object pair

Should assert the calling user (or their roles, can't remember what the API looks like) as well.

@spikecurtis
Copy link
Contributor

Firstly, only the last rbac assertion is checked. Some routes run 2 rbac checks (assuming the first one passed). We need to have a way to assert all the checks that are run. Possibly requiring some design to allow the endpoint to run until the second Authorize check (as all the auth calls return false atm).

For endpoints that do N authz checks, is it safe to assume (in tests) that we logically AND the results of the authz checks? If so, we should call them N times and fail a different single check.

If the logic can be branching or more complex, we're going to need a way to assert the different branches.

For ordinary business logic, I'm not usually a stickler for 100% branch coverage, but for something so security critical, I think we need to achieve it.

@spikecurtis
Copy link
Contributor

In the past, we’ve seen bugs not with the authz library, but checking side effects. Currently authz code is checked at the top of the http handler, then we call to the model package to do business logic. These model functions then expand over time, adding more side effects, and the authz checks are not updated. Large TOC-TOU

from v2 RBAC RFC

Ideally we have tests that call the APIs, then record the RBAC checks AND the objects touched. Then we assert both the checks are correct and the object modifications are correct. We also assert that the expected object modifications are all the modifications that happened so you can't add new side effects without updating the tests.

@Emyrk
Copy link
Member Author

Emyrk commented Dec 1, 2022

For endpoints that do N authz checks, is it safe to assume (in tests) that we logically AND the results of the authz checks?

99% of the time. I think this is the only one where we do not:

coder/coderd/authorize.go

Line 109 in 38bdae7

func (api *API) checkAuthorization(rw http.ResponseWriter, r *http.Request) {

The checkAuthorization returns each auth check in the response for the UI to toggle features based on the capability of the user.

If so, we should call them N times and fail a different single check.

Makes sense. So First time fail the first call. Second time fail the second. Nth call fail the Nth check 👍

If the logic can be branching or more complex, we're going to need a way to assert the different branches.

The complexity comes in that sometimes payloads are required to get to the next check to avoid BadRequest failures. So it might require some setup for particular endpoints.

For ordinary business logic, I'm not usually a stickler for 100% branch coverage, but for something so security critical, I think we need to achieve it.

👍

@Emyrk
Copy link
Member Author

Emyrk commented Dec 1, 2022

Ideally we have tests that call the APIs, then record the RBAC checks AND the objects touched. Then we assert both the checks are correct and the object modifications are correct. We also assert that the expected object modifications are all the modifications that happened so you can't add new side effects without updating the tests.

This gets tricky. We would have to write some assertion code that like "snapshots" the DB and then compares the changes in the DB right?

The current AGPLRoutes test does not pass valid payloads for the routes to make actual changes. Even if it did, different request payloads would have different side effects. So side effect changes would probably be a 3rd unit test part to compliment the 2 we currently have. Ideally linking it to the second somehow to keep those in sync.

@Emyrk Emyrk added the design needed Request for more beauty label Dec 1, 2022
@spikecurtis
Copy link
Contributor

This gets tricky. We would have to write some assertion code that like "snapshots" the DB and then compares the changes in the DB right?

I was thinking more like a fake DB that just records all calls (or all calls that modify data).

@spikecurtis
Copy link
Contributor

The current AGPLRoutes test does not pass valid payloads for the routes to make actual changes. Even if it did, different request payloads would have different side effects. So side effect changes would probably be a 3rd unit test part to compliment the 2 we currently have. Ideally linking it to the second somehow to keep those in sync.

Yeah, I don't think there is a way to be completely generic about what we send to each endpoint. We have to send real payloads to exercise the different ways the APIs can be used. I don't think we 100% adhere to having all security critical identifiers in the route, so some of them are going to be in the body.

@Emyrk Emyrk mentioned this issue Feb 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johnstcn johnstcn

@Emyrk Emyrk

Labels
design needed Request for more beauty security Area: security
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add initial AuthzQuerier implementation coder/coder
3 participants
@johnstcn @spikecurtis @Emyrk