authz: fix several bugs #1110

davepacheco · 2022-05-24T19:01:33Z

I started writing an exhaustive test that creates users with all possible roles and verifies what actions they can do. I'd hoped to verify that each role grants exactly the privileges that we expect. While working on this, I found a bunch of bugs. I also decided that the test as written is probably not the best next step, so that's not here. I do plan to decompose it into a couple of other tests but I wanted to get these fixes in first.

I'll comment on the specific changes inline.

davepacheco · 2022-05-24T19:02:09Z

common/src/sql/dbinit.sql

@@ -273,6 +273,7 @@ CREATE TABLE omicron.public.organization (
 );

 CREATE UNIQUE INDEX ON omicron.public.organization (
+    silo_id,


Fixes #1096. There's a test for this now in tests/integration_tests/silos.rs.

davepacheco · 2022-05-24T19:02:44Z

nexus/src/app/project.rs

@@ -96,7 +96,7 @@ impl super::Nexus {
    ) -> ListResultVec<db::model::Project> {
        let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
            .organization_name(organization_name)
-            .lookup_for(authz::Action::CreateChild)
+            .lookup_for(authz::Action::ListChildren)


Here and at L114, a copy/paste mistake meant we were checking whether you can create a Project when all you were trying to do was list them.

Interesting. I wonder if there's a way to make the type system catch such errors in the future?

It's a good question. We've talked about it a little bit (well, we've talked about whether there's a way to encode in some type what checks you've done already so that we can have functions that accept arguments that must have already been authorized), but not gotten very far.

Ok, seems like it might be worth some investigation. I might play around with it a bit.

nexus/src/db/datastore.rs

nexus/src/authz/omicron.polar

davepacheco · 2022-05-24T19:09:38Z

nexus/src/authz/omicron.polar

 	relations = { parent_fleet: Fleet };
-	"admin" if "admin" on "parent_fleet";
-	"collaborator" if "collaborator" on "parent_fleet";
+	"admin" if "collaborator" on "parent_fleet";


Previously, you got the "admin" role on a Silo if you were an "admin" on the Fleet. That seems a bit too restrictive. If you're "collaborator" on the Fleet, you're supposed to be able to create Silos. Why wouldn't you be able to delete them? I concluded that you should get "admin" on Silos if you already have "collaborator" on the parent Fleet.

I think it'd be good to better articulate what's intended by the various roles (and the names that are reused, like "collaborator"). I didn't want to block this on that, but if folks are uncomfortable with this we can defer it. The concrete problem with not doing this is that a Fleet Collaborator can create a Silo that they then cannot delete. (There are other ways to address this too but this seemed cleanest to me.)

I agree that this makes sense, but I will note here for the record that I find "collaborator" to be a very unclear role name generally.

I'm not attached to it. Do you think the role makes sense and you'd just pick a different name? Do you have one in mind?

🤔... maybe writer? author? owner?

Looking at the role itself, I'm not sure that I understand the reason for grouping list_children with create_child. It seems to me like list_children should go with read, and create_child should go with some variant of write. My intuitions on this are probably from Unix directories, which might not match our model exactly. But looking at the Polar file, I see that (contrary to the comment at the top), we have list_children if "viewer" for (nearly) everything except Organization.

(sorry for the incredible delay on responding here)

Your intuition makes sense to me, and I think they match the comment as well? The comment says that "viewer" generally gets "read" and "list_children". "collaborator" gets these plus "create_child".

davepacheco · 2022-05-24T19:10:25Z

nexus/src/authz/omicron.polar

+# behavior consistent with the rest of the API.
+#
+# It's unclear what else would break if users couldn't see their own Silo.
+has_permission(actor: AuthenticatedActor, "read", silo: Silo)


Previously, every authenticated user got the "viewer" role on their Silo. This meant they could list all Organizations. They really only need the ability to "read" their Silo. So I've narrowed this rule to only provide that.

davepacheco · 2022-05-24T19:12:16Z

nexus/src/authz/omicron.polar

@@ -180,7 +199,9 @@ resource Organization {
 	"modify" if "admin";

 	relations = { parent_silo: Silo };
-	"admin" if "admin" on "parent_silo";
+	"admin" if "collaborator" on "parent_silo";


Same argument as I made above -- without this, if you're a Silo Collaborator, you can create Organizations but not delete them. That seemed wrong.

davepacheco · 2022-05-24T19:13:09Z

nexus/src/authz/omicron.polar

@@ -180,7 +199,9 @@ resource Organization {
 	"modify" if "admin";

 	relations = { parent_silo: Silo };
-	"admin" if "admin" on "parent_silo";
+	"admin" if "collaborator" on "parent_silo";
+	"read" if "viewer" on "parent_silo";


I found that Silo Viewers couldn't actually see inside an Organization.

We probably want to break down and add an "Organization Viewer" role. It wasn't obviously necessary when I was writing out the initial roles. But at this point, it's weirder not to have it. Still, that's a bigger change so for now I'm just granting these two permissions directly.

davepacheco · 2022-05-24T19:14:15Z

nexus/src/authz/omicron.polar

@@ -212,7 +233,9 @@ resource Project {
 	"modify" if "admin";

 	relations = { parent_organization: Organization };
-	"admin" if "admin" on "parent_organization";
+	"admin" if "collaborator" on "parent_organization";


Similar to the above two cases: without this, if you're an Organization Collaborator, you could create Projects that you couldn't delete.

davepacheco · 2022-05-24T19:14:45Z

nexus/src/authz/omicron.polar

-	"admin" if "admin" on "parent_organization";
+	"admin" if "collaborator" on "parent_organization";
+
+	"viewer" if "list_children" on "parent_organization";


Similar to the other case above, without this, a Fleet or Silo Viewer couldn't see into the Project.

plotnick

Authz is hard, let's go shopping!

And thank you for the explanatory comments in the PR. They were very helpful.

plotnick · 2022-05-24T22:56:36Z

nexus/src/app/project.rs

@@ -96,7 +96,7 @@ impl super::Nexus {
    ) -> ListResultVec<db::model::Project> {
        let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
            .organization_name(organization_name)
-            .lookup_for(authz::Action::CreateChild)
+            .lookup_for(authz::Action::ListChildren)


Interesting. I wonder if there's a way to make the type system catch such errors in the future?

plotnick · 2022-05-24T22:58:00Z

nexus/src/authz/omicron.polar

 	relations = { parent_fleet: Fleet };
-	"admin" if "admin" on "parent_fleet";
-	"collaborator" if "collaborator" on "parent_fleet";
+	"admin" if "collaborator" on "parent_fleet";


I agree that this makes sense, but I will note here for the record that I find "collaborator" to be a very unclear role name generally.

Crucible changes: Remove unused fields in IOop (#1149) New downstairs clone subcommand. (#1129) Simplify the do_work_task loop (#1150) Move `Guest` stuff into a module (#1125) Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite` work to a separate task (#1145) Use fewer borrows in ExtentInner API (#1147) Update Rust crate reedline to 0.28.0 (#1141) Update Rust crate tokio to 1.36 (#1143) Update Rust crate slog-bunyan to 2.5.0 (#1139) Update Rust crate rayon to 1.8.1 (#1138) Update Rust crate itertools to 0.12.1 (#1137) Update Rust crate byte-unit to 5.1.4 (#1136) Update Rust crate base64 to 0.21.7 (#1135) Update Rust crate async-trait to 0.1.77 (#1134) Discard deferred msgs (#1131) Minor Downstairs cleanup (#1127) Update test_fail_live_repair to support pstop (#1128) Ignore client messages after stopping the IO task (#1126) Move client IO task into a struct (#1124) Bump Rust to 1.75 and fix new Clippy lints (#1123) Propolis changes: PHD: convert to async (#633) PHD: assume specialized Windows images (#636) propolis-standalone-config needn't be a crate standalone: Use tar for snapshot/restore phd: use latest "lab-2.0-opte" target, not a specific version (#637) PHD: add tests for migration of running processes (#623) PHD: fix `cargo xtask phd` tidy not doing anything (#630) PHD: add documentation for `cargo xtask phd` (#629) standalone: improve virtual device creation errors (#632) phd: add Windows Server 2019 guest adapter (#627) PHD: add `cargo xtask phd` to make using PHD nicer (#619)

Crucible changes: Remove unused fields in IOop (#1149) New downstairs clone subcommand. (#1129) Simplify the do_work_task loop (#1150) Move `Guest` stuff into a module (#1125) Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite` work to a separate task (#1145) Use fewer borrows in ExtentInner API (#1147) Update Rust crate reedline to 0.28.0 (#1141) Update Rust crate tokio to 1.36 (#1143) Update Rust crate slog-bunyan to 2.5.0 (#1139) Update Rust crate rayon to 1.8.1 (#1138) Update Rust crate itertools to 0.12.1 (#1137) Update Rust crate byte-unit to 5.1.4 (#1136) Update Rust crate base64 to 0.21.7 (#1135) Update Rust crate async-trait to 0.1.77 (#1134) Discard deferred msgs (#1131) Minor Downstairs cleanup (#1127) Update test_fail_live_repair to support pstop (#1128) Ignore client messages after stopping the IO task (#1126) Move client IO task into a struct (#1124) Bump Rust to 1.75 and fix new Clippy lints (#1123) Propolis changes: PHD: convert to async (#633) PHD: assume specialized Windows images (#636) propolis-standalone-config needn't be a crate standalone: Use tar for snapshot/restore phd: use latest "lab-2.0-opte" target, not a specific version (#637) PHD: add tests for migration of running processes (#623) PHD: fix `cargo xtask phd` tidy not doing anything (#630) PHD: add documentation for `cargo xtask phd` (#629) standalone: improve virtual device creation errors (#632) phd: add Windows Server 2019 guest adapter (#627) PHD: add `cargo xtask phd` to make using PHD nicer (#619) Co-authored-by: Alan Hanson <alan@oxide.computer>

davepacheco added 3 commits May 24, 2022 10:45

authz: fix several bugs

ee9c857

add test for Org namespacing

0ed8bc2

remove redundant rule

da703ed

davepacheco commented May 24, 2022

View reviewed changes

davepacheco marked this pull request as ready for review May 24, 2022 19:15

davepacheco requested a review from plotnick May 24, 2022 19:15

davepacheco mentioned this pull request May 24, 2022

clean up Polar file: non-functional changes #1112

Merged

plotnick approved these changes May 24, 2022

View reviewed changes

davepacheco merged commit ca14079 into main May 24, 2022

davepacheco deleted the authz-role-test-fixes branch May 24, 2022 23:44

davepacheco mentioned this pull request May 25, 2022

tracking issue for MVP IAM work #849

Closed

69 tasks

plotnick mentioned this pull request May 25, 2022

clean up Polar file: minor functional changes #1113

Merged

davepacheco mentioned this pull request Aug 17, 2022

abandoned end-to-end role test #1609

Closed

leftwo mentioned this pull request Feb 9, 2024

Update Crucible and Propolis #5039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authz: fix several bugs #1110

authz: fix several bugs #1110

davepacheco commented May 24, 2022

davepacheco May 24, 2022

davepacheco May 24, 2022

plotnick May 24, 2022

davepacheco May 24, 2022

plotnick May 25, 2022

davepacheco May 24, 2022

plotnick May 24, 2022

davepacheco May 24, 2022

plotnick May 25, 2022

davepacheco Jul 14, 2022

davepacheco May 24, 2022

davepacheco May 24, 2022

davepacheco May 24, 2022

davepacheco May 24, 2022

davepacheco May 24, 2022

plotnick left a comment •

edited

Loading

plotnick May 24, 2022

plotnick May 24, 2022

authz: fix several bugs #1110

authz: fix several bugs #1110

Conversation

davepacheco commented May 24, 2022

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

Choose a reason for hiding this comment

plotnick left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plotnick left a comment •

edited

Loading