search

oxidecomputer / omicron

Omicron: Oxide control plane
Mozilla Public License 2.0
244 stars 36 forks source link

PUT resource policy does not validate type or existence of identity IDs #1246

Open david-crespo opened 2 years ago

david-crespo commented 2 years ago

I've been using the system users from /users to test role assignment on orgs and projects because I can't list silo users yet (#1235). It just occurred to me that I should not be able to assign resource roles to system users! Here is the PUT body:

image

It seems that as long as I say identity_type: "silo_user", the API does not validate that that is true.

https://github.com/oxidecomputer/omicron/blob/0588fac574963a9380313447bb90e1f2eda88cc5/nexus/src/db/datastore.rs#L3761-L3785

davepacheco commented 2 years ago

Yeah, this looks like a bug. It also seems like a separate bug if the resulting role assignment actually works.

david-crespo commented 2 years ago

I have not yet tested whether it actually works, but I am working on that.

david-crespo commented 1 week ago

Since @morlandi7 asked, I checked the code and can't see any evidence this has changed since being reported. Testing on colo rack:

// trying to use the same ID twice. this 500s
(await oxide.policyUpdate({ body: { role_assignments: [
  { identity_type: "silo_group", identity_id: "7da1c977-ba19-4a26-bd68-3fb1463ab2c7", role_name: "admin" },
  { identity_type: "silo_group", identity_id: "7da1c977-ba19-4a26-bd68-3fb1463ab2c7", role_name: "admin" },
] } }))

// using a non-existent ID. this works just fine
(await oxide.policyUpdate({ body: { role_assignments: [
  { identity_type: "silo_group", identity_id: "7da1c977-ba19-4a26-bd68-3fb1463ab2c7", role_name: "admin" },
  { identity_type: "silo_group", identity_id: "00000000-0000-0000-0000-000000000000", role_name: "admin" },
] } })).data

So I'm thinking a better framing of this issue is that there is simply no validation on the IDs passed in, whether on their existence or their "type". The second request above also results in this amusing situation in the UI because we cannot find the ID in the list of users and groups.

image
david-crespo commented 1 week ago

Also, probably a separate issue, but a silo admin can nuke their own permissions by setting the policy to empty.

await oxide.policyUpdate({ body: { role_assignments: [] } })

You can also do this the easy way: simply deleting the role assignment in the UI.

image

At that point, I can load the empty access page, but pretty much any other page will blow up because I can't list projects or anything. The way we blow up is a matter of how the UI handles various kinds of API errors, but currently we blow up with our most generic Something went wrong page.

image