Authenticated user need privileges on global images and themselves #1341

jmpesp · 2022-07-01T14:02:10Z

Add polar statements so that any authenticated user can list and read
global images.

In order to test this, the set of integration tests that check endpoints
for their authz visibility had to change: GET /images/ was now
accessible by any authenticated user, even if they had no permissions at
all, meaning new type of visibility was required. But POST /images was
protected by authz, meaning separate visibility settings were required
for different HTTP methods. This commit also adds per-method authz
visibility and updates each VerifyEndpoint struct accordingly.

A few of the endpoints in uncovered-authz-endpoints.txt were able to be
added in because of the new visibility variant, but not all. In order
to test those for authz visibility, more work is required to support
endpoints that redirect (for example) or endpoints with query
parameters.

Add polar statements so that any authenticated user can list and read global images. In order to test this, the set of integration tests that check endpoints for their authz visibility had to change: `GET /images/` was now accessible by any authenticated user, even if they had no permissions at all, meaning new type of visibility was required. But `POST /images` was protected by authz, meaning separate visibility settings were required for different HTTP methods. This commit also adds per-method authz visibility and updates each VerifyEndpoint struct accordingly. A few of the endpoints in uncovered-authz-endpoints.txt were able to be added in because of the new visibility variant, but not all. In order to test those for authz visibility, more work is required to support endpoints that redirect (for example) or endpoints with query parameters.

jmpesp · 2022-07-01T14:02:27Z

To anyone reviewing: there's a few XXX is this right? that should be cleared up before merging.

leftwo · 2022-07-01T15:51:58Z

Are their separate permissions for viewing global images and uploading (or replacing) global images?
I think that is what you are adding here, but I don't know enough authentication buzzwords to be sure :)

jmpesp · 2022-07-01T16:04:45Z

Are their separate permissions for viewing global images and uploading (or replacing) global images?
I think that is what you are adding here, but I don't know enough authentication buzzwords to be sure :)

There are :) This PR grants "list_children" and "read" only, not "modify".

plotnick

This looks great to me, but I'd feel better if @davepacheco gave his 👍 too.

plotnick · 2022-07-01T16:14:34Z

nexus/tests/integration_tests/endpoints.rs

+            allowed_methods: vec![
+                AllowedMethod {
+                    method: MethodAndBody::Get,
+                    visibility: Visibility::Protected, // XXX is this right?


Yes, I believe this (and the ones below) is correct. There's an argument to be made that SSH public keys should be truly public (as they are on GitHub, for example), but we decided not to go that route for now. In particular, there's no was currently for one user to even address another user's keys, since we go through /session/me.

What I mean is it was a little surprising that GET /session/me/sshkeys is expected to return a 403 - there is no endpoint to address other user's keys, this is for the current user, and I would have expected 200 OK with a body of [] or something like that instead.

Oh, I misunderstood what the test was saying. You are correct: for the user that added the demo key, they should get a 200 OK with the key that was added; for a user that has no keys, they should get 200 OK with an empty list; and for an unauthenticated user, they should get 403 Forbidden.

Shouldn't an unauthenticated user get a 401?

plotnick · 2022-07-01T16:15:35Z

nexus/tests/output/uncovered-authz-endpoints.txt

-session_me                               (get    "/session/me")
-sshkeys_get                              (get    "/session/me/sshkeys")
-sshkeys_get_key                          (get    "/session/me/sshkeys/{ssh_key_name}")


Hooray 🎉

davepacheco

Thanks for doing this. The policy change unblocks the demo flow and it's great to get a bunch of endpoints out of the ignore list. For what it's worth I sometimes make separate PRs for Nexus vs. this test so that it's easier to be sure we didn't break behavior (and not notice because we also broke the test). I'm not sure if it's worth doing that here.

davepacheco · 2022-07-05T20:23:17Z

nexus/tests/integration_tests/endpoints.rs

-    pub visibility: Visibility,
-
-    /// Specifies what HTTP methods are supported for this HTTP resource
+    /// Specifies what HTTP methods are supported for this HTTP resource, along


This feels a little off to me and I'm trying to figure out why.

Before this change, there was an idea of "visibility" that applied at the HTTP-resource-level. e.g., "/organizations" is visible to everybody, but "/organizations/some-org" is not). That's not just "can you GET it", but also whether you get a 403 or 404 when you can't get it. This also enforced some level of API coherence: you could not specify that an unprivileged user is allowed to delete a resource but not see it. I think this was useful because in practice I expect us to conform to a small number of patterns: "everyone can see and modify this" (like your own ssh keys), "everyone can see but not modify this" (like /images), and "only privileged users can do anything with this" (most other things). This also enforced some useful invariants like "if you can delete this thing, then you can also see it".

With this change, the idea of "visibility" of an HTTP resource goes away and we're really talking about access for a specific method+URL combination. This allows allows you to specify some nonsensical configurations now (e.g., you can say that an unauthenticated user can delete a resource but you have to be authenticated to see it). There's nothing to enforce that the behavior matches one of these patterns.

An alternative might be to have another field here at the endpoint level called "unprivileged_access" or something that specifies what authenticated, unprivileged users are expected to be able to do here. We could have variants for the patterns that we expect to have: maybe UnprivilegedAccess::Full, UnprivilegedAccess::ReadOnly, or UnprivilegedAccess::None corresponding to the cases I mentioned above. This would enforce only a handful of possible configurations here, which I think is a good thing because you couldn't (accidentally or otherwise) create the nonsensical configurations I mentioned above. It'd also be a lot less boilerplate for each endpoint and this change would be much smaller. What do you think?

I agree wholeheartedly with this suggestion, and rolled back all the original changes. There's nothing to enforce that the behavior matches one of these patterns. was a very clarifying statement about the intent of the visibility part of the test and see now how the original changes allow for nonsense combinations like you said. Sorry for all the churn :)

nexus/tests/integration_tests/endpoints.rs

davepacheco · 2022-07-05T20:33:19Z

nexus/tests/integration_tests/endpoints.rs

-                        }
-                    ).unwrap()
-                ),
+                AllowedMethod {


In terms of reviewing these changes, since there are a lot of them and I want to make sure I got it right, I think the rule is: for any endpoint that was already in this file, whatever visibility it had before should now be applied identically and individually to each of its methods, modulo that Public becomes PublicPermissionRequired. Is that right?

With the latest changes, it should be more straight forward to review. Almost everything gets UnprivilegedAccess::None, except for a few endpoints.

nexus/tests/integration_tests/unauthorized.rs

davepacheco · 2022-07-05T20:49:18Z

nexus/tests/integration_tests/endpoints.rs

+                    visibility: Visibility::Protected, // XXX is this right?
+                },
+                AllowedMethod {
+                    method: MethodAndBody::Delete,


How does this not cause us to actually delete the ssh key when we run this test?

It would! Any combination of UnprivilegedAccess::Full and a DELETE call would. But I'm deferring the SSH keys changes because I couldn't get the authz policy to work and didn't want to hold the global image changes up.

I think this is ok from the context of this test - we should be testing that DELETE works to test what permissions UnprivilegedAccess::Full should give

davepacheco · 2022-07-05T20:49:44Z

nexus/tests/integration_tests/endpoints.rs

+            allowed_methods: vec![
+                AllowedMethod {
+                    method: MethodAndBody::Get,
+                    visibility: Visibility::Protected, // XXX is this right?


Shouldn't an unauthenticated user get a 401?

Can't get SSH keys tests to work, deferring for another commit.

…veryone

davepacheco

Nice! I like this a lot.

plotnick · 2022-07-07T16:40:56Z

cc0b58d fixes #1362.

davepacheco · 2022-07-07T16:44:11Z

nexus/src/authz/omicron.polar

@@ -240,15 +244,28 @@ resource SiloUser {
 	    "create_child",
 	];

+	roles = ["admin", "viewer"];


This isn't ironclad or anything but we don't bother specifying Oso roles for things where we don't support actually assigning those roles. That is, Oso roles are 1:1 with roles that might be stored in the database.

You could remove these, plus the rules at L255-L260, and just define some has_permission rules.

It's funny because that was the original way we implemented it :)

done in d0ced98

fmt, clippy, and uncovered-authz-endpoints.txt update

davepacheco · 2022-07-07T17:57:12Z

I have some thoughts on some cleanup here but I think we should land this now and deal with that later.

davepacheco · 2022-07-07T18:43:54Z

Sorry for the terseness in my previous comment. I was trying to avoid holding up today's demo.

One cleanup we could make is that we don't need four has_permission rules -- we can just leave the specific permission unspecified (by just naming the argument, like _perm, instead of a specific string like "read"). I had another issue in mind related to the fact that the Polar file has other code to do something similar, but I think I was wrong about that.

After talking with @jmpesp offline I realized we (myself included) might be confusing the purpose of this test. Today, the purpose of this test ("unauthorized.rs") is to verify uniform behavior of the API when users are not authorized to do things. If we stick with that, then I think the expected behavior here is:

for the unauthenticated user, the behavior of the test is unchanged
for the authenticated, unprivileged user:
- for UnprivilegedAccess::None, we do the same thing we used to do (same battery of tests)
- for UnprivilegedAccess::ReadOnly, we don't do the GET test at all. We still test PUT and DELETE and expect the same results as before.
- for UnprivilegedAccess::Full, we don't do any tests as this user at all

Of course, something should probably test that unprivileged users can read things that have UnprivilegedAccess::ReadOnly and can read/write/delete things that have UnprivilegedAccess::Full. I think this is a more complicated problem than it looks, which is why I don't think we should try to bake it into this test.

There's a related problem here, which is the output format of the test assumes that unprivileged users will never get 200-level responses. If we punt this testing to a separate test as I suggested, we don't have to deal with this.

It seems like this PR now has three changes that could be separated:

granting privileges for authenticated users to list and read global images (the synopsis of the PR).
granting privileges for authenticated users to read their ssh keys (basically unrelated, I think?)
changes to the "unauthorized.rs" test to better support endpoints that unprivileged users have access to.

As I understand it, the first two are straightforward and unrelated to each other. The third thing is where we're still working through details. Is that right?

davepacheco · 2022-07-07T19:07:34Z

I filed #1374 with some thoughts on how to better test that particular roles can access certain resources.

jmpesp · 2022-07-07T19:20:48Z

Changing the unauthorized.rs test to support unprivileged users retrieving results for GET /images was what lead down this rabbit hole - once that test was changed to support unprivileged users accessing things successfully, I tried to pull in changes for SSH keys and other endpoints.

Of course, something should probably test that unprivileged users can read things that have UnprivilegedAccess::ReadOnly and can read/write/delete things that have UnprivilegedAccess::Full. I think this is a more complicated problem than it looks, which is why I don't think we should try to bake it into this test.

I agree that given the original purpose of the unauthorized tests, there shouldn't be paths testing for when users do have access - that belongs somewhere else. Right now I'm backing out basically all changes to the unauthorized.rs test(s), and writing separate tests for an unprivileged user's access.

davepacheco · 2022-07-07T19:30:19Z

Yeah, that path makes sense. I still think the changes with the UnprivilegedAccess enum are useful because they allow us to remove things from the ignore-list and process what we can process here (particularly for the read-only things). Whatever you think. Sorry for my own confusion along the way here.

Roll back changes to unauthorized.rs (mostly) and add specific authz tests for policy that allows unprivileged access.

…veryone

jmpesp · 2022-07-08T18:47:38Z

This is ready for re-review now :)

davepacheco

I have some suggestions here but they're mostly minor. Given the urgency of this I think we should land it as-is and follow up separately on any of these other things.

davepacheco · 2022-07-12T17:31:14Z

nexus/tests/integration_tests/unauthorized.rs

+        // Some authz policy states that authenticated users get implicit
+        // privileges for some resources. Do not test for those here, they
+        // should be covered in other resource specific tests.
+        if endpoint.unprivileged_access != UnprivilegedAccess::None {


I think we still want to test POST/PUT/DELETE on "readonly" endpoints here.

Addressed in #1399.

davepacheco · 2022-07-12T17:35:25Z

nexus/src/authz/omicron.polar

@@ -249,6 +251,10 @@ resource SiloUser {
 has_relation(silo: Silo, "parent_silo", user: SiloUser)
 	if user.silo = silo;

+# authenticated actors have all permissions on themselves
+has_permission(actor: AuthenticatedActor, _perm: String, silo_user: SiloUser)
+    if actor.equals_silo_user(silo_user);


Is there any reason not to just do something like actor.id = silo_user.id?

Yeah, we can't make UUIDs into Polar classes, since we don't own the crate.

I think they're already supported:
https://docs.osohq.com/rust/reference/polar/classes.html#uuids-via-the-uuid-crate

I'll take a swing at this in a follow-up PR.

The other problem here is that there's no id attribute on silo_user. We could add one, but this type is generated by the authz_resource! macro so either we'd have to make that configurable or everything would get a new id attribute. I decided this isn't worth spending more time on.

davepacheco · 2022-07-12T17:37:39Z

nexus/tests/integration_tests/authz.rs

+            .parsed_body()
+            .unwrap();
+
+    // - cannot create a global image - should get 403


I think this should be covered by the unauthorized test (commented there that we should still test ReadOnly endpoints)

Addressed in #1399.

davepacheco · 2022-07-12T17:37:55Z

nexus/tests/integration_tests/authz.rs

+    .await
+    .expect("POST request should have failed");
+
+    // - cannot delete a global image - also should get a 404 because the


Addressed in #1399.

davepacheco · 2022-07-12T17:39:10Z

nexus/tests/integration_tests/authz.rs

+
+use uuid::Uuid;
+
+// Test that an authenticated, unprivileged user has full CRUD access to their SSH keys


Is this not tested already by the sshkey tests? (Or, could they be, if those tests used UnprivilegedUser?) (cc @plotnick)

I think so, yes. It looks like creation, listing, and deletion are all covered by the tests below, but as you say not by an unprivileged user.

Addressed in #1399.

davepacheco · 2022-07-12T17:52:56Z

I went ahead and sync'd this with "main" and will plan to land this PR.

…ryone

davepacheco · 2022-07-12T19:40:34Z

The last test run hit #962 (on buildomat only) and I've rerun it.

Added a new package, crucible-dtrace that pulls from buildomat a package that contains a set of DTrace scripts. These scripts are extracted into the global zone at /opt/oxide/crucible_dtrace/ Update Crucible to latest includes these updates: Clean up dependency checking, fixing space leak (#1372) Make a DTrace package (#1367) Use a single context in all messages (#1363) Remove `DownstairsWork`, because it's redundant (#1371) Remove `WorkState`, because it's implicit (#1370) Do work immediately upon receipt of a job, if possible (#1366) Move 'do work for one job' into a helper function (#1365) Remove `DownstairsWork` from map when handling it (#1361) Using `block_in_place` for IO operations (#1357) update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369) Parameterize more tests (#1364) Misc cleanup, remove sqlite references. (#1360) Fix `Extent::close` docstring (#1359) Make many `Region` functions synchronous (#1356) Remove `Workstate::Done` (unused) (#1355) Return a sorted `VecDeque` directly (#1354) Combine `proc_frame` and `do_work_for` (#1351) Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support arbitrary Volumes during replace compare (#1349) Remove the SQLite backend (#1352) Add a custom timeout for buildomat tests (#1344) Move `proc_frame` into `ActiveConnection` (#1348) Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into ConnectionState (#1340) Make `ConnectionState` an enum type (#1339) Parameterize `test_repair.sh` directories (#1345) Remove `Arc<Mutex<Downstairs>>` (#1338) Send message to Downstairs directly (#1336) Consolidate `on_disconnected` and `remove_connection` (#1333) Move disconnect logic to the Downstairs (#1332) Remove invalid DTrace probes. (#1335) Fix outdated comments (#1331) Use message passing when a new connection starts (#1330) Move cancellation into Downstairs, using a token to kill IO tasks (#1329) Make the Downstairs own per-connection state (#1328) Move remaining local state into a `struct ConnectionState` (#1327) Consolidate negotiation + IO operations into one loop (#1322) Allow replacement of a target in a read_only_parent (#1281) Do all IO through IO tasks (#1321) Make `reqwest_client` only present if it's used (#1326) Move negotiation into Downstairs as well (#1320) Update Rust crate clap to v4.5.4 (#1301) Reuse a reqwest client when creating Nexus clients (#1317) Reuse a reqwest client when creating repair client (#1324) Add % to keep buildomat happy (#1323) Downstairs task cleanup (#1313) Update crutest replace test, and mismatch printing. (#1314) Added more DTrace scripts. (#1309) Update Rust crate async-trait to 0.1.80 (#1298)

Update Crucible and Propolis to the latest Added a new package, crucible-dtrace that pulls from buildomat a package that contains a set of DTrace scripts. These scripts are extracted into the global zone at /opt/oxide/crucible_dtrace/ Crucible latest includes these updates: Clean up dependency checking, fixing space leak (#1372) Make a DTrace package (#1367) Use a single context in all messages (#1363) Remove `DownstairsWork`, because it's redundant (#1371) Remove `WorkState`, because it's implicit (#1370) Do work immediately upon receipt of a job, if possible (#1366) Move 'do work for one job' into a helper function (#1365) Remove `DownstairsWork` from map when handling it (#1361) Using `block_in_place` for IO operations (#1357) update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369) Parameterize more tests (#1364) Misc cleanup, remove sqlite references. (#1360) Fix `Extent::close` docstring (#1359) Make many `Region` functions synchronous (#1356) Remove `Workstate::Done` (unused) (#1355) Return a sorted `VecDeque` directly (#1354) Combine `proc_frame` and `do_work_for` (#1351) Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support arbitrary Volumes during replace compare (#1349) Remove the SQLite backend (#1352) Add a custom timeout for buildomat tests (#1344) Move `proc_frame` into `ActiveConnection` (#1348) Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into ConnectionState (#1340) Make `ConnectionState` an enum type (#1339) Parameterize `test_repair.sh` directories (#1345) Remove `Arc<Mutex<Downstairs>>` (#1338) Send message to Downstairs directly (#1336) Consolidate `on_disconnected` and `remove_connection` (#1333) Move disconnect logic to the Downstairs (#1332) Remove invalid DTrace probes. (#1335) Fix outdated comments (#1331) Use message passing when a new connection starts (#1330) Move cancellation into Downstairs, using a token to kill IO tasks (#1329) Make the Downstairs own per-connection state (#1328) Move remaining local state into a `struct ConnectionState` (#1327) Consolidate negotiation + IO operations into one loop (#1322) Allow replacement of a target in a read_only_parent (#1281) Do all IO through IO tasks (#1321) Make `reqwest_client` only present if it's used (#1326) Move negotiation into Downstairs as well (#1320) Update Rust crate clap to v4.5.4 (#1301) Reuse a reqwest client when creating Nexus clients (#1317) Reuse a reqwest client when creating repair client (#1324) Add % to keep buildomat happy (#1323) Downstairs task cleanup (#1313) Update crutest replace test, and mismatch printing. (#1314) Added more DTrace scripts. (#1309) Update Rust crate async-trait to 0.1.80 (#1298) Propolis just has this one update: Allow boot order config in propolis-standalone --------- Co-authored-by: Alan Hanson <[email protected]>

jmpesp requested a review from davepacheco July 1, 2022 14:02

jmpesp requested a review from plotnick July 1, 2022 14:02

jmpesp mentioned this pull request Jul 1, 2022

"unauthorized" test should support more-public endpoints #1277

Open

plotnick approved these changes Jul 1, 2022

View reviewed changes

davepacheco reviewed Jul 5, 2022

View reviewed changes

jmpesp added 4 commits July 7, 2022 10:12

Undo changes, add unprivileged_access instead

bf8a5d3

Can't get SSH keys tests to work, deferring for another commit.

Merge remote-tracking branch 'upstream/main' into global_images_for_e…

3c0ea0d

…veryone

correct comments

79de404

actor.silo is not a list, use =

d525f41

davepacheco approved these changes Jul 7, 2022

View reviewed changes

make ssh keys work

cc0b58d

davepacheco reviewed Jul 7, 2022

View reviewed changes

jmpesp added 2 commits July 7, 2022 12:59

remove roles that do not exist in the db

d0ced98

fmt, clippy, and uncovered-authz-endpoints.txt update

merge

48ded19

davepacheco mentioned this pull request Jul 7, 2022

need better testing around minimum privileges #1374

Open

jmpesp added 5 commits July 7, 2022 15:41

blanket permission statement

88d8c73

Roll back changes to unauthorized.rs

10c7e99

Roll back changes to unauthorized.rs (mostly) and add specific authz tests for policy that allows unprivileged access.

test accessing your own silo

8537597

add a bunch of authz tests

106d301

fmt and clippy

54dfa15

Merge remote-tracking branch 'upstream/main' into global_images_for_e…

b02b652

…veryone

davepacheco approved these changes Jul 12, 2022

View reviewed changes

Merge branch 'main' into global_images_for_everyone

80c9843

davepacheco changed the title ~~Authenticated user can list and read global images~~ Authenticated user need privileges on global images and themselves Jul 12, 2022

davepacheco enabled auto-merge (squash) July 12, 2022 17:54

davepacheco added 3 commits July 12, 2022 11:16

fix mismerge

21dd23c

another mismerge

4338cbe

Merge remote-tracking branch 'origin/main' into global_images_for_eve…

31b8895

…ryone

davepacheco disabled auto-merge July 12, 2022 19:39

davepacheco enabled auto-merge (squash) July 12, 2022 19:39

davepacheco merged commit 1699402 into oxidecomputer:main Jul 12, 2022

davepacheco added a commit that referenced this pull request Jul 12, 2022

follow-ups from #1341

cbf1b8d

davepacheco mentioned this pull request Jul 12, 2022

test write access by unprivileged users to read-only resources #1399

Merged

plotnick mentioned this pull request Jul 13, 2022

Alice can't set her own SSH keys #1362

Closed

jmpesp deleted the global_images_for_everyone branch July 18, 2022 15:07

leftwo mentioned this pull request Jun 26, 2024

Update crucible, add DTrace crucible to host image #5954

Merged


		use uuid::Uuid;

		// Test that an authenticated, unprivileged user has full CRUD access to their SSH keys

Authenticated user need privileges on global images and themselves #1341

Authenticated user need privileges on global images and themselves #1341

Conversation

jmpesp commented Jul 1, 2022

jmpesp commented Jul 1, 2022

leftwo commented Jul 1, 2022

jmpesp commented Jul 1, 2022

plotnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco left a comment

Choose a reason for hiding this comment

plotnick commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Jul 7, 2022

davepacheco commented Jul 7, 2022

davepacheco commented Jul 7, 2022

jmpesp commented Jul 7, 2022

davepacheco commented Jul 7, 2022

jmpesp commented Jul 8, 2022

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Jul 12, 2022

davepacheco commented Jul 12, 2022