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.

Sign up for GitHub

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

Authenticated user need privileges on global images and themselves #1341

Merged
merged 18 commits into from
Jul 12, 2022
Merged

Authenticated user need privileges on global images and themselves #1341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2e36125
Authenticated user can list and read global images
jmpesp Jul 1, 2022
bf8a5d3
Undo changes, add unprivileged_access instead
jmpesp Jul 7, 2022
3c0ea0d
Merge remote-tracking branch 'upstream/main' into global_images_for_e…
jmpesp Jul 7, 2022
79de404
correct comments
jmpesp Jul 7, 2022
d525f41
actor.silo is not a list, use =
jmpesp Jul 7, 2022
cc0b58d
make ssh keys work
jmpesp Jul 7, 2022
d0ced98
remove roles that do not exist in the db
jmpesp Jul 7, 2022
48ded19
merge
jmpesp Jul 7, 2022
88d8c73
blanket permission statement
jmpesp Jul 7, 2022
10c7e99
Roll back changes to unauthorized.rs
jmpesp Jul 7, 2022
8537597
test accessing your own silo
jmpesp Jul 7, 2022
106d301
add a bunch of authz tests
jmpesp Jul 8, 2022
54dfa15
fmt and clippy
jmpesp Jul 8, 2022
b02b652
Merge remote-tracking branch 'upstream/main' into global_images_for_e…
jmpesp Jul 8, 2022
80c9843
Merge branch 'main' into global_images_for_everyone
davepacheco Jul 12, 2022
21dd23c
fix mismerge
davepacheco Jul 12, 2022
4338cbe
another mismerge
davepacheco Jul 12, 2022
31b8895
Merge remote-tracking branch 'origin/main' into global_images_for_eve…
davepacheco Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions nexus/src/authz/actor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use super::roles::RoleSet;
use crate::authn;
use crate::authz::SiloUser;
use omicron_common::api::external::LookupType;
use omicron_common::api::external::ResourceType;
use uuid::Uuid;
Expand Down Expand Up @@ -90,5 +91,9 @@ impl oso::PolarClass for AuthenticatedActor {
)
})
})
.add_method(
"equals_silo_user",
|a: &AuthenticatedActor, u: SiloUser| a.actor_id == u.id(),
)
}
}
18 changes: 14 additions & 4 deletions nexus/src/authz/omicron.polar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,17 @@ has_relation(fleet: Fleet, "parent_fleet", silo: Silo)
#
# It's unclear what else would break if users couldn't see their own Silo.
has_permission(actor: AuthenticatedActor, "read", silo: Silo)
# TODO-security TODO-coverage We should have a test that exercises this
# syntax.
# TODO actor.silo is *not* a list, so `in` is incorrect here, but if you
# replace that with `=` it fails! test_silo_read_for_unpriv covers this
# statement
if silo in actor.silo;

# Any authenticated user should be allowed to list the identity providers of
# their silo.
has_permission(actor: AuthenticatedActor, "list_identity_providers", silo: Silo)
# TODO-security TODO-coverage We should have a test that exercises this
# syntax.
# TODO actor.silo is *not* a list, so `in` is incorrect here, but if you
# replace that with `=` it fails! test_list_silo_idps_for_unpriv covers
# this statement
if silo in actor.silo;

resource Organization {
Expand Down Expand Up @@ -249,6 +251,10 @@ resource SiloUser {
has_relation(silo: Silo, "parent_silo", user: SiloUser)
if user.silo = silo;

# authenticated actors have all permissions on themselves
has_permission(actor: AuthenticatedActor, _perm: String, silo_user: SiloUser)
if actor.equals_silo_user(silo_user);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason not to just do something like actor.id = silo_user.id?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can't make UUIDs into Polar classes, since we don't own the crate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think they're already supported:
https://docs.osohq.com/rust/reference/polar/classes.html#uuids-via-the-uuid-crate

I'll take a swing at this in a follow-up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other problem here is that there's no id attribute on silo_user. We could add one, but this type is generated by the authz_resource! macro so either we'd have to make that configurable or everything would get a new id attribute. I decided this isn't worth spending more time on.


resource SshKey {
permissions = [ "read", "modify" ];
relations = { silo_user: SiloUser };
Expand Down Expand Up @@ -346,6 +352,10 @@ resource GlobalImageList {
has_relation(fleet: Fleet, "parent_fleet", global_image_list: GlobalImageList)
if global_image_list.fleet = fleet;

# Any authenticated user can list and read global images
has_permission(_actor: AuthenticatedActor, "list_children", _global_image_list: GlobalImageList);
has_permission(_actor: AuthenticatedActor, "read", _global_image: GlobalImage);

# Describes the policy for creating and managing web console sessions.
resource ConsoleSessionList {
permissions = [ "create_child" ];
Expand Down
Loading