Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

need better testing around minimum privileges #1374

Open
Tracked by #419
davepacheco opened this issue Jul 7, 2022 · 1 comment
Open
Tracked by #419

need better testing around minimum privileges #1374

davepacheco opened this issue Jul 7, 2022 · 1 comment

Comments

@davepacheco
Copy link
Collaborator

We've had a number of issues where certain operations checked the wrong privileges and this was not identified by the test suite.

  • In authz: protect VPC endpoints #743, I found a bug where we weren't checking a "delete" privilege but the "unauthorized" test succeeded by coincidence because the implementation happened to check the "read" privilege. So the unprivileged user still got a 403/404 (I don't remember which).
  • During the lead up to the July demo, we found cases where unprivileged users couldn't see global images or their own ssh keys (Authenticated user need privileges on global images and themselves #1341). We didn't notice because we'd been doing all our happy-path testing (both automated and otherwise) using "test-privileged", which has all privileges.

The "unauthorized.rs" test verifies that totally unauthenticated or unprivileged users are not able to do things. And the happy path tests verify that superusers are able to do everything. We don't have tests for anything in between. I'll record more thoughts on this below.

@davepacheco
Copy link
Collaborator Author

davepacheco commented Jul 7, 2022

I'm starting from the assumption that we want to leverage our existing happy-path tests to verify the minimum privileges required. That is: given that we're already writing happy-path tests that verify the behavior of endpoints when users are authorized, we may as well update those tests to verify what we want around authz rather than writing a whole separate battery of tests.

(Tangentially: it's a fair question whether we should automatically generate happy-path tests from the definitions in endpoints.rs -- see #784. I think that's a great idea to explore. I'm not sure whether it makes sense to couple that project to this one though.)

So how do we do this? One idea would be to provide a way for Nexus to return the list of privileges that were checked, either directly in the response or through a test-only side channel. Then the test suite could verify these privileges matched what we expected. This is very precise -- we'd be verifying that, say, deleting a project checked whether someone had permission to Delete this project. To make this complete, we'd also want a set of tests that verify that each role grants the privileges that we expect. I've prototyped this already in #1123. Together, this would give us high confidence that each role grants permissions to complete the expected set of API calls (no more, no less), though it does require a bit of interpretation.


[edit: the following is a bad idea, but keeping it here to preserve the thinking]: Another idea would be to use more fine-grained test users in the test suite. This seems compelling at first because it seems to directly test the behavior we want (i.e., that the "project viewer" role is sufficient to GET a project). But it's still imprecise, in that there might be some other role that has the required privileges, and that might be very wrong! For example, this would verify that "project admin" could DELETE a project, but it wouldn't tell us that "project viewer" can't delete the Project. To do that, we'd really need to try the same operation as various users with these other, less privileged roles and make sure they get 404s/403s. This devolves into the above test. There's an even worse problem with this second approach, which is that it tightly couples the tests to the specific roles that we support. If we were to add some new role in between "project viewer" and "project collaborator", we'd potentially have to go update a ton of tests. I think this highlights that roles are the wrong level to be testing functionality at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant