`/policy` for current silo, `/global/policy` for fleet #1573

david-crespo · 2022-08-10T16:37:29Z

Move existing GET/PUT /policy, which was previously about the fleet policy, to `/global/policy
GET /policy and PUT /policy are now about the user's current silo
Change fetch_silo_policy and update_silo_policy to take a db::lookup::Silo instead of a name or ID so they are agnostic about whether you're working with name or ID at the call site

david-crespo · 2022-08-10T16:39:13Z

nexus/src/external_api/http_entrypoints.rs

+            .silo_required()
+            .internal_context("loading current silo")?;
+        let policy =
+            nexus.silo_fetch_policy_by_id(&opctx, authz_silo.id()).await?;


Added this by_id fetcher because we have the ID on hand and it seems silly to fetch the silo to get the name just so we can use the existing silo_fetch_policy.

david-crespo · 2022-08-10T16:40:39Z

nexus/src/external_api/http_entrypoints.rs

@@ -316,10 +319,10 @@ pub fn external_api() -> NexusApiDescription {
 /// Fetch the top-level IAM policy
 #[endpoint {
    method = GET,
-    path = "/policy",
+    path = "/global/policy",


As @zephraph suggested, this breaks off a little chunk of the big /global/* change just to unblock this work. The /global/ thing is so big that I think this approach makes sense regardless, though the downside is we have a short time where some global things have the prefix and some don't.

david-crespo · 2022-08-10T16:42:30Z

nexus/src/app/silo.rs

+        silo_id: Uuid,
+    ) -> LookupResult<shared::Policy<authz::SiloRole>> {
+        let (.., authz_silo) = LookupPath::new(opctx, &self.db_datastore)
+            .silo_id(silo_id)


Feels a bit sad to copy silo_fetch_policy with only this line being different, may extract a private helper that takes the authz_silo instead.

zephraph · 2022-08-10T18:41:35Z

nexus/tests/output/nexus_tags.txt

+policy_update                            /policy
+policy_view                              /policy
 silo_create                              /silos
 silo_delete                              /silos/{silo_name}
 silo_identity_provider_create            /silos/{silo_name}/saml-identity-providers


I'm not certain this tag grouping is really correct. I'm not recommending a change in this PR, but broadly it's worth us talking about what we're going to call "global" in the context of the silo. If we're going to use the tag silo (and thus silo as a term to describe this global context) then we're going to need to educate users on it even before they really need to know what a silo is.

Eh, on second thought most of the stuff here will be grouped under the global tag eventually so it's probably moot. Still unsure if silo is the right tag for things at the root that aren't under /organizations but also aren't under /global, but I don't know what else we'd call it.

david-crespo · 2022-08-10T20:00:25Z

nexus/tests/integration_tests/endpoints.rs

+                    ).unwrap()
+                ),
+            ],
+        },


This fails with the following. It's making a request with the unprivileged user that it expects to 404 but it's 200ing instead. I don't understand why the unprivileged user would 200 — we are doing operations that require ReadPolicy on the silo.

G GET PUT POST DEL TRCE G URL 0thread 'integration_tests::unauthorized::test_unauthorized' panicked at 'called `Result::unwrap()` on an `Err` value: expected status code 404 Not Found, found 200 OK

omicron/nexus/tests/integration_tests/unauthorized.rs

Lines 444 to 457 in 000333b

if do_test_unprivileged {

let expected_status = match allowed {

Some(_) => unauthz_status,

None => StatusCode::METHOD_NOT_ALLOWED,

};

let response = NexusRequest::new(

RequestBuilder::new(client, method.clone(), &uri)

.body(body.as_ref())

.expect_status(Some(expected_status)),

)

.authn_as(AuthnMode::UnprivilegedUser)

.execute()

.await

.unwrap();

Given the /policy API is known and documented if you didn't have access to that it'd more likely be a 401. I think you want to change the visibility in your test to public instead of protected. That might not be the original issue, but it is one thing.

First pass at debugging using the lovely guide. Seems like the unprivileged user does have the permission in question, which is weird?

~/oxide/omicron $ cat /var/folders/lk/vsx86g0545g3g2tsmvs50py40000gn/T/test_all-af18dffa1640b050-test_unauthorized.49638.0.log | rg "authorize result" | rg "/policy" | rg "ReadPolicy" | rg "60001" | jq '{ msg, req_id, authenticated, method, uri, actor, action, result }' { "msg": "authorize result", "req_id": "49c6298b-e273-41f4-b3ac-a79de7259074", "authenticated": true, "method": "GET", "uri": "/global/policy", "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })", "action": "ReadPolicy", "result": "Err(Forbidden)" } { "msg": "authorize result", "req_id": "9b1a600f-70bc-4fd8-8103-5bde60c970bc", "authenticated": true, "method": "GET", "uri": "/silos/demo-silo/policy", "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })", "action": "ReadPolicy", "result": "Err(ObjectNotFound { type_name: Silo, lookup_type: ByName(\"demo-silo\") })" } { "msg": "authorize result", "req_id": "2ba41eb1-bf4f-42d8-9ee4-a1fbe8899082", "authenticated": true, "method": "GET", "uri": "/policy", "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })", "action": "ReadPolicy", "result": "Ok(())" } { "msg": "authorize result", "req_id": "2ba41eb1-bf4f-42d8-9ee4-a1fbe8899082", "authenticated": true, "method": "GET", "uri": "/policy", "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })", "action": "ReadPolicy", "result": "Ok(())" }

This was the explanation of what I was seeing. The existing silo policy test was not asking about the user's own silo, which is why the unprivileged user didn't have access. Even the unpriv user has access to their own silo. I am writing an issue to think about how we could do this differently if we wanted to.

omicron/nexus/src/authz/omicron.polar

Lines 148 to 163 in 4528641

# As a special case, all authenticated users can read their own Silo. That's

# not quite the same as having the "viewer" role. For example, they cannot list

# Organizations in the Silo.

#

# One reason this is necessary is because if an unprivileged user tries to

# create an Organization using "POST /organizations", they should get back a 403

# (which implies they're able to see /organizations, which is essentially seeing

# the Silo itself) rather than a 404. This behavior isn't a hard constraint

# (i.e., you could reasonably get a 404 for an API you're not allowed to call).

# Nor is the implementation (i.e., we could special-case this endpoint somehow).

# But granting this permission is the simplest way to keep this endpoint's

# 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)

if silo in actor.silo;

zephraph

Good stuff! The silo visibility stuff was tricky but educational.

david-crespo · 2022-08-11T04:30:43Z

nexus/src/app/silo.rs

-            .lookup_for(authz::Action::ModifyPolicy)
-            .await?;
+        let (.., authz_silo) =
+            silo_lookup.lookup_for(authz::Action::ModifyPolicy).await?;


Assuming the static lifetime isn't wrong, this approach of taking a lookup instead of a name or ID is rad. I like it.

david-crespo · 2022-08-11T04:34:51Z

nexus/src/app/mod.rs

+        opctx: &'a OpContext,
+    ) -> db::lookup::LookupPath {
+        db::lookup::LookupPath::new(opctx, &self.db_datastore)
+    }


As I say in the comment, we can do without this, but having to refer explicitly to nexus.datastore() inside an endpoint handler to build the lookup feels like it violates the abstraction boundary of the Nexus object. No caller will ever have to make a lookup with a different datastore than the one that hangs of nexus.

Crucible Changes: Send deactivate flush if `need_flush` is set (#1573) Improve DTrace scripts. (#1574) Move reconcile-specific data into `ReconcileData` struct (#1567) Remove unprinted header item from sled_upstairs_info (#1566) Downgrade warning about skipping IO on all 3x Downstairs (#1564) Propolis changes: Wire up viona params from illumos#16738

Crucible Changes: Send deactivate flush if `need_flush` is set (#1573) Improve DTrace scripts. (#1574) Move reconcile-specific data into `ReconcileData` struct (#1567) Remove unprinted header item from sled_upstairs_info (#1566) Downgrade warning about skipping IO on all 3x Downstairs (#1564) Propolis changes: Wire up viona params from illumos#16738 Co-authored-by: Alan Hanson <[email protected]>

/policy for current silo, /global/policy for fleet

f374892

david-crespo commented Aug 10, 2022

View reviewed changes

clippy!!!

4339550

zephraph reviewed Aug 10, 2022

View reviewed changes

david-crespo added 2 commits August 10, 2022 13:56

verify policy endpoint (well, it fails)

e531ea1

test silo role assignments with /policy endpoint

f0b7175

david-crespo commented Aug 10, 2022

View reviewed changes

david-crespo added 2 commits August 10, 2022 15:26

update openapi spec

03719ae

welp. it was supposed to be public readonly all along

1ce5a09

zephraph approved these changes Aug 10, 2022

View reviewed changes

david-crespo added 2 commits August 10, 2022 23:04

share common code. why not

aa1ee4b

clean it up. we are ready

69ae4c1

david-crespo commented Aug 11, 2022

View reviewed changes

david-crespo requested a review from davepacheco August 11, 2022 04:53

david-crespo mentioned this pull request Aug 11, 2022

Merge IAM policy hierarchy oxidecomputer/console#1113

Merged

6 tasks

davepacheco approved these changes Aug 12, 2022

View reviewed changes

david-crespo merged commit eef6567 into main Aug 12, 2022

david-crespo deleted the silo-policy branch August 12, 2022 21:35

david-crespo mentioned this pull request Aug 23, 2022

"Inherited" permissions not included in policy response #1326

Closed

zephraph mentioned this pull request Sep 6, 2022

Change /global/ to /system/ to reflect RFD-288 #1676

Merged

leftwo mentioned this pull request Nov 27, 2024

Update Crucible and Propolis #7177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`/policy` for current silo, `/global/policy` for fleet #1573

`/policy` for current silo, `/global/policy` for fleet #1573

david-crespo commented Aug 10, 2022 •

edited

Loading

david-crespo Aug 10, 2022

david-crespo Aug 10, 2022

david-crespo Aug 10, 2022

zephraph Aug 10, 2022 •

edited

Loading

david-crespo Aug 10, 2022 •

edited

Loading

zephraph Aug 10, 2022

david-crespo Aug 10, 2022

david-crespo Aug 10, 2022 •

edited

Loading

zephraph left a comment

david-crespo Aug 11, 2022 •

edited

Loading

david-crespo Aug 11, 2022

	if do_test_unprivileged {
	let expected_status = match allowed {
	Some(_) => unauthz_status,
	None => StatusCode::METHOD_NOT_ALLOWED,
	};
	let response = NexusRequest::new(
	RequestBuilder::new(client, method.clone(), &uri)
	.body(body.as_ref())
	.expect_status(Some(expected_status)),
	)
	.authn_as(AuthnMode::UnprivilegedUser)
	.execute()
	.await
	.unwrap();

	# As a special case, all authenticated users can read their own Silo. That's
	# not quite the same as having the "viewer" role. For example, they cannot list
	# Organizations in the Silo.
	#
	# One reason this is necessary is because if an unprivileged user tries to
	# create an Organization using "POST /organizations", they should get back a 403
	# (which implies they're able to see /organizations, which is essentially seeing
	# the Silo itself) rather than a 404. This behavior isn't a hard constraint
	# (i.e., you could reasonably get a 404 for an API you're not allowed to call).
	# Nor is the implementation (i.e., we could special-case this endpoint somehow).
	# But granting this permission is the simplest way to keep this endpoint's
	# 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)
	if silo in actor.silo;

/policy for current silo, /global/policy for fleet #1573

/policy for current silo, /global/policy for fleet #1573

Conversation

david-crespo commented Aug 10, 2022 • edited Loading

david-crespo Aug 10, 2022

Choose a reason for hiding this comment

david-crespo Aug 10, 2022

Choose a reason for hiding this comment

david-crespo Aug 10, 2022

Choose a reason for hiding this comment

zephraph Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

david-crespo Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

zephraph Aug 10, 2022

Choose a reason for hiding this comment

david-crespo Aug 10, 2022

Choose a reason for hiding this comment

david-crespo Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

zephraph left a comment

Choose a reason for hiding this comment

david-crespo Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

david-crespo Aug 11, 2022

Choose a reason for hiding this comment

`/policy` for current silo, `/global/policy` for fleet #1573

`/policy` for current silo, `/global/policy` for fleet #1573

david-crespo commented Aug 10, 2022 •

edited

Loading

zephraph Aug 10, 2022 •

edited

Loading

david-crespo Aug 10, 2022 •

edited

Loading

david-crespo Aug 10, 2022 •

edited

Loading

david-crespo Aug 11, 2022 •

edited

Loading