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

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

Open
david-crespo opened this issue Jun 21, 2022 · 4 comments
Open
Labels
bug Something that isn't working.

Comments

@david-crespo
Copy link
Contributor

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.

let mut new_assignments = new_assignments
.iter()
.map(|r| {
db::model::RoleAssignment::new(
db::model::IdentityType::from(r.identity_type),
r.identity_id,
resource_type,
resource_id,
&r.role_name.to_database_string(),
)
})
.collect::<Vec<_>>();
new_assignments.sort_by(|r1, r2| {
(&r1.role_name, r1.identity_id)
.cmp(&(&r2.role_name, r2.identity_id))
});
use db::schema::role_assignment::dsl;
let delete_old_query = diesel::delete(dsl::role_assignment)
.filter(dsl::resource_id.eq(resource_id))
.filter(dsl::resource_type.eq(resource_type.to_string()))
.filter(dsl::identity_type.ne(IdentityType::UserBuiltin));
let insert_new_query = diesel::insert_into(dsl::role_assignment)
.values(new_assignments)
.returning(RoleAssignment::as_returning());

@david-crespo david-crespo added the bug Something that isn't working. label Jun 21, 2022
@davepacheco
Copy link
Collaborator

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

@david-crespo
Copy link
Contributor Author

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

leftwo pushed a commit that referenced this issue Apr 10, 2024
Propolis changes:
Rework storage for Accessors

Crucible Changes
Update oximeter dependency (#1244)
Dummy downstairs cleanup (#1247)
Some DTrace updates. (#1246)
HEY! LISTEN! HEY! LISTEN! (#1215)
Fix clippy lints (#1245)
leftwo added a commit that referenced this issue Apr 10, 2024
Propolis changes:
Rework storage for Accessors

Crucible Changes
Update oximeter dependency (#1244)
Dummy downstairs cleanup (#1247)
Some DTrace updates. (#1246)
HEY! LISTEN! HEY! LISTEN! (#1215)
Fix clippy lints (#1245)

Co-authored-by: Alan Hanson <[email protected]>
@morlandi7 morlandi7 added this to the 9 milestone Jul 26, 2024
@david-crespo
Copy link
Contributor Author

david-crespo commented Sep 6, 2024

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 david-crespo changed the title PUT resource policy allows assigning roles to non-silo_user users PUT resource policy does not validate type or existence of identity IDs Sep 6, 2024
@david-crespo
Copy link
Contributor Author

david-crespo commented Sep 6, 2024

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

@david-crespo david-crespo removed this from the 9 milestone Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working.
Projects
None yet
Development

No branches or pull requests

3 participants