session and authn interfaces should do authz checks #912
Conversation
|
I haven't even gotten the tests to compile yet, so this isn't quite ready. (I'm hopeful it'll just pass of course but it's definitely possible this regressed something since we're now doing authz checks that we weren't before.) |
|
Neat. Let me know when to take a detailed look. |
| console_session, | ||
| silo_id: silo_user.silo_id, | ||
| }) | ||
| } |
There was a problem hiding this comment.
great, doing this in nexus.rs is much better
| /// ConsoleSessionList is a synthetic resource used for modeling who has access | ||
| /// to create sessions. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub struct ConsoleSessionList; |
There was a problem hiding this comment.
Why don't we need something like this to represent the ability to create the various other kinds of FleetChild?
There was a problem hiding this comment.
Good question. There's one set of permissions and roles on Fleet, and it's basically written to be pretty locked-down. You have to have Fleet "collaborator" to have the Fleet create_child permission. But we want the "external-authenticator" to be able to create sessions without them having full write access to the Fleet.
A similar problem comes up with global images (in another PR).
Another approach would be to create more different actions on Fleet and have more roles that grant those permissions to do those actions. The upside is we wouldn't need this synthetic resource. I'm not sure it's clearer to do that, though. It's a little funny to have these synthetic resources, but I think they neatly describe what's really going on (which is there's a collection -- albeit one not represented in the API -- that "internal-authenticator" has permission to create children in). There's a more practical downside to creating a bunch more actions: right now, there's only one authz::Action enum with the union of all actions on all resources, so it would wind up growing a bunch of variants that aren't applicable for most resources. I've been wondering if we could split this up. The challenge is it's an argument to OpContext::authorize(). Maybe it could be an associated type of the AuthorizedResource that you're doing a check on. I haven't dug into this.
There was a problem hiding this comment.
Ok good, that's all helpful and matches the picture in my head to the extent I had one — that the synthetic resource is an alternative to adding a very narrow action to the enum for this purpose. I imagine @plotnick has an opinion about this.
| let session = nexus.session_create(user_id).await?; | ||
| // For now, we use the external authn context to create the session. | ||
| // Once we have real SAML login, maybe we can cons up a real OpContext for | ||
| // this user and use their own privileges to create the session. |
There was a problem hiding this comment.
This is interesting — we will think of the user as having the ability to create a session for themselves? I suppose in theory that would also work for deletes in the logout case where they're logged in, but for delete in the expiration case they are no longer authenticated at delete time. Just thinking out loud.
Since we are stuck with having the external_authn opctx for a lot of the session stuff, I'm not sure it's worth the complication of having only session create (and renew I guess) handled by the user's opctx.
There was a problem hiding this comment.
Yeah, that's fair. I agree we wouldn't use this for logout. I'm also not positive this will work when we flesh out SAML login -- it relies on us having created an OpContext for the user that's authenticated not by the session cookie but by the SAML assertions. If that falls out of that work, I figured it'd be nice to use. If it doesn't, I think it's totally fine to continue using this context.
Crucible: update rust crate base64 to 0.21.3 (#913) update rust crate slog-async to 2.8 (#915) update rust crate async-recursion to 1.0.5 (#912) Move active jobs into a separate data structure and optimize `ackable_work` (#908) Check repair IDs correctly (#910) update actions/checkout action to v4 (#903) update rust crate tokio to 1.32 (#890) Remove single-item Vecs (#898) Move "extent under repair" into a helper function (#907) offset_mod shouldn't be randomized (#905) Only rehash if a write may have failed (#899) Make negotiation state an enum (#901) Test update for fast write ack and gather errors on test failure (#897) Propolis: Update cpuid-gen util for better coverage Make storage backend config more flexible and consistent Use correct register sizes for PIIX3 PM device Update bitflags dependency fix softnpu port order (#517) Use hex formatting for unhandled MMIO/PIO/MSRs Update deps for new crucible and oximeter Update standalone-with-crucible docs (#514)
Crucible: update rust crate base64 to 0.21.3 (#913) update rust crate slog-async to 2.8 (#915) update rust crate async-recursion to 1.0.5 (#912) Move active jobs into a separate data structure and optimize `ackable_work` (#908) Check repair IDs correctly (#910) update actions/checkout action to v4 (#903) update rust crate tokio to 1.32 (#890) Remove single-item Vecs (#898) Move "extent under repair" into a helper function (#907) offset_mod shouldn't be randomized (#905) Only rehash if a write may have failed (#899) Make negotiation state an enum (#901) Test update for fast write ack and gather errors on test failure (#897) Propolis: Update cpuid-gen util for better coverage Make storage backend config more flexible and consistent Use correct register sizes for PIIX3 PM device Update bitflags dependency fix softnpu port order (#517) Use hex formatting for unhandled MMIO/PIO/MSRs Update deps for new crucible and oximeter Update standalone-with-crucible docs (#514) Co-authored-by: Alan Hanson <[email protected]>
Fixes #846 and also wraps up #845.