support having silo-level roles confer fleet-level roles#3349
Conversation
…'s closer to what it needs to be
luqmana
left a comment
There was a problem hiding this comment.
Thanks for tackling this @davepacheco! Looks mostly good. I did have one question about doing a check in the polar vs rust.
Also, there's no way currently modify a Silo right? Which means a conferred role can't be removed without deleting + recreating the Silo?
That's correct. The role also can't be added without deleting and recreating the Silo. I don't think there's anything too tricky about implementing a Silo update that supports changing this. The big thing to remember when we do that is someone will probably need to have Fleet-level permissions to change this particular property of the Silo. |
Ok, that matches what I thought. I definitely do think we should have a way to modify this for an existing Silo. Let's open a bug for that? |
|
Filed #3400. |
| }, | ||
| "uniqueItems": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
Just noticed while integrating this into the console that this is probably wrong. I don't think Dropshot is handling BTreeMap correctly. I will look into it further.
omicron/nexus/types/src/external_api/views.rs
Lines 42 to 47 in 51d2c58
There was a problem hiding this comment.
Hmmm, maybe it is right. I guess the keys can't be type-constrained, but we are correctly specifying the type of the values as an array of FleetRole where the items are unique. Ok!
https://swagger.io/docs/specification/data-models/dictionaries/
I will look at improving the TS client generator to handle this.
There was a problem hiding this comment.
If we wanted to be slightly more elaborate and more correct, instead of unconstrained string keys, maybe we could do the OpenAPI analogue of this TS type:
mapped_fleet_role: {
viewer?: Set<FleetRole>,
collaborator?: Set<FleetRole>,
admin?: Set<FleetRole>,
}i.e., a hard-coded set of keys, all of which are optional. It's probably fine for now. In the console, the user is going to be choosing these roles from a menu, so it will be hard to pass in something that doesn't exist.
Fixes #3275 using Option 3.
Notes about the change:
params::SiloCreate. But their impls ofDatabaseStringcouldn't move there, which meant it had to move tonexus/db-modelinstead (where that trait is defined). So I also moved test_database_string_impl() there so that the tests would live with those impls. This also necessitated moving some expected test output files around.authn::Contextas aSiloAuthnPolicy.SiloAuthnPolicyis also plumbed into theAuthenticatedActorthat we hand over to Oso so that it can be used during authorization.This whole mechanism is less general than I thought it would be because care must be taken to load the Silo (to get its policy) before we get to the authorization phase, and that needs to happen using Nexus's own credentials. If we try to load the Silo during authorization using the user's credentials, we'd enter infinite recursion because loading the Silo requires Fleet-level privileges. The net result is that if we ever want to do this kind of thing with some resource other than the Silo, we'll need to do something more sophisticated.