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

Fleet privileges should not cascade into Silos #1580

Merged
merged 38 commits into from
Aug 12, 2022
Merged

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Aug 11, 2022

See #1340. This change makes it so that having privileges at the Fleet level does not mean you can see anything at all inside any Silo. You can read, modify, and delete Silos, but not list children or create children. One motivation for this change is that it means a security team wanting to audit a Silo can limit the search to users in the Silo's IdP, knowing that nobody else is able to do anything within the Silo.

Right now, Fleet administrators can still read and update the Silo's IAM policy. I'm not sure if that's right, since in principle they could update the policy to include themselves (although they wouldn't have a way to access any siloed resources if they were in a different Silo, per RFD 297). Tangentially, with this change in place, there's no reason any Silo's role assignments should ever refer to a silo user or group from another Silo. We could enforce this. That would satisfy the goal above (because it would still be true that only users in the Silo's IdP could actually do anything). However, it would still let a Fleet admin grant a role to someone inside a Silo, which seems not what we want. I think to prevent this, we'd have to actually separate the Read/ReadPolicy actions (which are currently identical) and the Modify/ModifyPolicy actions (same) so that we can grant "read" and "modify" here but not "ReadPolicy" or "ModifyPolicy".

If folks think this is a good idea, I can do this in a follow-on change.

CC @kc8apf

@davepacheco davepacheco requested a review from plotnick August 11, 2022 22:21
Base automatically changed from authz-policy-test to main August 12, 2022 22:00
Copy link
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty unambiguous improvement to the overall Silo functionality; previously, the fleet-admin was a kind of enormous loop-hole in the whole Silo concept. But I do think the follow-up to separate read/read-policy and modify/modify-policy is also important, primarily to patch the "fleet admin can modify the silo policy to include themselves" hole, which seems like a serious one.

That said, we are going to have to very carefully document the distinction between a fleet admin and a silo admin. I could see operators being confused about why, say, a fleet admin can't modify a VPC on racks that they are nominally administering. I don't think that's a reason not to make this change, I just think the user-facing documentation will have to spell it out explicitly.

@davepacheco davepacheco enabled auto-merge (squash) August 12, 2022 22:10
@davepacheco davepacheco mentioned this pull request Aug 12, 2022
69 tasks
@davepacheco
Copy link
Collaborator Author

Thanks! I've added some items to #849 around this:

  • Enforce that Silo, Organization, and Project policies cannot refer to identities outside the Silo.
  • Disallow fleet administrators from reading or modifying the policy on the Silo.

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on splitting read/write from read policy/write policy.

@davepacheco davepacheco merged commit 2737f24 into main Aug 12, 2022
@davepacheco davepacheco deleted the isolate-silos branch August 12, 2022 23:03
@smklein
Copy link
Collaborator

smklein commented Aug 15, 2022

As a heads up, this PR seems to have broken the process for adding an IdP, at least according to our demo flow: https://github.com/oxidecomputer/meta/blob/master/engineering/remote-access-preview-demo-setup.adoc#setting-up-a-new-silo-with-saml-authentication

With this PR:

$ oxide api /silos/mercury/saml-identity-providers <arguments>
403 Forbidden Forbidden

After reverting this PR:

$ oxide api /silos/mercury/saml-identity-providers <arguments>
<JSON indicating success>

Are there additional arguments I should be using to set up the demo flow?

@davepacheco
Copy link
Collaborator Author

Do you have any of the logs from that failure? I would want to go through this with them to confirm exactly what check was failing. At first glance, I see that Fleet Admins still have privileges to modify the Silo, but not create a child:
https://github.com/oxidecomputer/omicron/blob/main/nexus/tests/output/authz-roles.out#L103-L107
But I thought we had integration tests that create IdPs...

@davepacheco
Copy link
Collaborator Author

Ah, I think I updated the tests to grant "Silo Admin" to the "test-privileged" user on the newly-created Silos. I wonder if the better fix is to treat this as a "modify" operation on the Silo...but I'll file a separate issue to discuss this.

@davepacheco
Copy link
Collaborator Author

See also #1681.

leftwo pushed a commit that referenced this pull request Dec 4, 2024
Skip jobs when reinitializing to `Faulted` (#1583)
Clear `repair_check_deadline` if repair is successfully started (#1581)
Update rust-version reqs to reflect reality (#1580)

Propolis:
60886290 update nvme-trace.d to match current probe definitions (#821)
8e5693bf Fix clippy lints for Rust 1.83
leftwo added a commit that referenced this pull request Dec 4, 2024
Crucible:
Skip jobs when reinitializing to `Faulted` (#1583)
Clear `repair_check_deadline` if repair is successfully started (#1581)
Update rust-version reqs to reflect reality (#1580)

Propolis:
Update nvme-trace.d to match current probe definitions (#821) 
Fix clippy lints for Rust 1.83
PHD: use stty to widen the effective terminal for Linux guests (#818)

Co-authored-by: Alan Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants