From ee9c85799bc040aa9bf5a1490d1253166416f464 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 10:45:52 -0700 Subject: [PATCH 1/9] authz: fix several bugs --- common/src/sql/dbinit.sql | 1 + nexus/src/app/project.rs | 4 ++-- nexus/src/authz/omicron.polar | 39 ++++++++++++++++++++++++++++------- nexus/src/db/datastore.rs | 2 +- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 3e1d553f999..23c98529fab 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -273,6 +273,7 @@ CREATE TABLE omicron.public.organization ( ); CREATE UNIQUE INDEX ON omicron.public.organization ( + silo_id, name ) WHERE time_deleted IS NULL; diff --git a/nexus/src/app/project.rs b/nexus/src/app/project.rs index e63a14ecf10..e8b50de9ee4 100644 --- a/nexus/src/app/project.rs +++ b/nexus/src/app/project.rs @@ -96,7 +96,7 @@ impl super::Nexus { ) -> ListResultVec { let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) - .lookup_for(authz::Action::CreateChild) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .projects_list_by_name(opctx, &authz_org, pagparams) @@ -111,7 +111,7 @@ impl super::Nexus { ) -> ListResultVec { let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) - .lookup_for(authz::Action::CreateChild) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .projects_list_by_id(opctx, &authz_org, pagparams) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 9de39b4522d..fe8112b86cf 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -137,23 +137,42 @@ resource Silo { ]; roles = [ "admin", "collaborator", "viewer" ]; + # permissions granted by this resource's roles "list_children" if "viewer"; "read" if "viewer"; + "create_child" if "collaborator"; + "modify" if "admin"; + # roles implied by other roles "viewer" if "collaborator"; - "create_child" if "collaborator"; "collaborator" if "admin"; - "modify" if "admin"; + + # roles implied by relationships with the parent fleet relations = { parent_fleet: Fleet }; - "admin" if "admin" on "parent_fleet"; - "collaborator" if "collaborator" on "parent_fleet"; + "admin" if "collaborator" on "parent_fleet"; "viewer" if "viewer" on "parent_fleet"; + "list_children" if "viewer" on "parent_fleet"; } has_relation(fleet: Fleet, "parent_fleet", silo: Silo) if silo.fleet = fleet; -has_role(actor: AuthenticatedActor, "viewer", silo: Silo) + +# All authenticated users can read their own Silo. That's not quite the same as +# having the "viewer" role. For example, they cannot list Organizations in the +# Silo. +# +# One reason this is necessary is because if an unprivileged user tries to +# create an Organization using "POST /organizations", they should get back a 403 +# (which implies they're able to see /organizations, which is essentially seeing +# the Silo itself) rather than a 404. This behavior isn't a hard constraint +# (i.e., you could reasonably get a 404 for an API you're not allowed to call). +# Nor is the implementation (i.e., we could special-case this endpoint somehow). +# But granting this permission is the simplest way to keep this endpoint's +# behavior consistent with the rest of the API. +# +# 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 - # case. + # syntax. if silo in actor.silo; resource Organization { @@ -180,7 +199,9 @@ resource Organization { "modify" if "admin"; relations = { parent_silo: Silo }; - "admin" if "admin" on "parent_silo"; + "admin" if "collaborator" on "parent_silo"; + "read" if "viewer" on "parent_silo"; + "list_children" if "viewer" on "parent_silo"; } has_relation(silo: Silo, "parent_silo", organization: Organization) if organization.silo = silo; @@ -212,7 +233,9 @@ resource Project { "modify" if "admin"; relations = { parent_organization: Organization }; - "admin" if "admin" on "parent_organization"; + "admin" if "collaborator" on "parent_organization"; + + "viewer" if "list_children" on "parent_organization"; } has_relation(organization: Organization, "parent_organization", project: Project) if project.organization = organization; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 02f53ad2f00..c80d3c81e89 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -676,7 +676,7 @@ impl DataStore { pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { let authz_silo = opctx.authn.silo_required()?; - opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + opctx.authorize(authz::Action::ListChildren, &authz_silo).await?; use db::schema::organization::dsl; paginated(dsl::organization, dsl::name, pagparams) From 0ed8bc2fde503e8d77e7c809e377b4415562d3aa Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 11:43:58 -0700 Subject: [PATCH 2/9] add test for Org namespacing --- nexus/test-utils/src/resource_helpers.rs | 47 +++++++++++++++++++ nexus/tests/integration_tests/silos.rs | 57 ++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 952e2a454af..ff562631d44 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -18,6 +18,8 @@ use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; use omicron_nexus::crucible_agent_client::types::State as RegionState; use omicron_nexus::external_api::params; +use omicron_nexus::external_api::shared; +use omicron_nexus::external_api::shared::IdentityType; use omicron_nexus::external_api::views::{ Organization, Project, Silo, Vpc, VpcRouter, }; @@ -280,6 +282,51 @@ pub async fn create_router( .unwrap() } +/// Grant a role on a resource to a user +/// +/// * `grant_resource_url`: URL of the resource we're granting the role on +/// * `grant_role`: the role we're granting +/// * `grant_user`: the uuid of the user we're granting the role to +/// * `run_as`: the user _doing_ the granting +pub async fn grant_iam( + client: &ClientTestContext, + grant_resource_url: &str, + grant_role: T, + grant_user: Uuid, + run_as: AuthnMode, +) where + T: serde::Serialize + serde::de::DeserializeOwned, +{ + let policy_url = format!("{}/policy", grant_resource_url); + let existing_policy: shared::Policy = + NexusRequest::object_get(client, &policy_url) + .authn_as(run_as.clone()) + .execute() + .await + .expect("failed to fetch policy") + .parsed_body() + .expect("failed to parse policy"); + let new_role_assignment = shared::RoleAssignment { + identity_type: IdentityType::SiloUser, + identity_id: grant_user, + role_name: grant_role, + }; + let new_role_assignments = existing_policy + .role_assignments + .into_iter() + .chain(std::iter::once(new_role_assignment)) + .collect(); + + let new_policy = shared::Policy { role_assignments: new_role_assignments }; + + // TODO-correctness use etag when we have it + NexusRequest::object_put(client, &policy_url, Some(&new_policy)) + .authn_as(run_as) + .execute() + .await + .expect("failed to update policy"); +} + pub async fn project_get( client: &ClientTestContext, project_url: &str, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index bad2be1294a..99a6cee8d5f 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -3,17 +3,20 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; -use omicron_nexus::external_api::views::{Organization, Silo}; +use omicron_common::api::external::{IdentityMetadataCreateParams, Name}; +use omicron_nexus::external_api::views::{self, Organization, Silo}; use omicron_nexus::TestInterfaces as _; use http::method::Method; use http::StatusCode; use nexus_test_utils::resource_helpers::{ - create_organization, create_silo, objects_list_page_authz, + create_organization, create_silo, grant_iam, objects_list_page_authz, }; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use omicron_nexus::authz::SiloRoles; +use omicron_nexus::external_api::params; #[nexus_test] async fn test_silos(cptestctx: &ControlPlaneTestContext) { @@ -74,6 +77,16 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); + // Grant the user "admin" privileges on that Silo. + grant_iam( + client, + "/silos/discoverable", + SiloRoles::Admin, + new_silo_user_id, + AuthnMode::PrivilegedUser, + ) + .await; + // TODO-coverage, TODO-security: Add test for Silo-local session // when we can use users in another Silo. @@ -81,7 +94,45 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { // Create organization with built-in user auth // Note: this currently goes to the built-in silo! - create_organization(&client, "someorg").await; + let org_name: Name = "someorg".parse().unwrap(); + let new_org_in_default_silo = + create_organization(&client, org_name.as_str()).await; + + // Create an Organization of the same name in a different Silo to verify + // that's possible. + let new_org_in_our_silo = NexusRequest::objects_post( + client, + "/organizations", + ¶ms::OrganizationCreate { + identity: IdentityMetadataCreateParams { + name: org_name.clone(), + description: String::new(), + }, + }, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .expect("failed to create same-named Organization in a different Silo") + .parsed_body::() + .expect("failed to parse new Organization"); + assert_eq!( + new_org_in_default_silo.identity.name, + new_org_in_our_silo.identity.name + ); + assert_ne!( + new_org_in_default_silo.identity.id, + new_org_in_our_silo.identity.id + ); + // Delete it so that we can delete the Silo later. + NexusRequest::object_delete( + client, + &format!("/organizations/{}", org_name), + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .expect("failed to delete test Organization"); // Verify GET /organizations works with built-in user auth let organizations = From da703ed78a3f498ec2ec541f994bdbfd90ae3534 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 12:07:35 -0700 Subject: [PATCH 3/9] remove redundant rule --- nexus/src/authz/omicron.polar | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index fe8112b86cf..39388c179cc 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -151,7 +151,6 @@ resource Silo { relations = { parent_fleet: Fleet }; "admin" if "collaborator" on "parent_fleet"; "viewer" if "viewer" on "parent_fleet"; - "list_children" if "viewer" on "parent_fleet"; } has_relation(fleet: Fleet, "parent_fleet", silo: Silo) if silo.fleet = fleet; From 23a673c28e673cf719f920303bb9e41dda31161d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 15:22:49 -0700 Subject: [PATCH 4/9] clean up Polar file (non-functional changes) --- nexus/src/authz/omicron.polar | 267 +++++++++++++++++++--------------- 1 file changed, 153 insertions(+), 114 deletions(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 39388c179cc..c306de348ba 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -3,9 +3,8 @@ # This file is augmented by generated snippets. # - # -# General types and rules +# ACTOR TYPES AND BASIC RULES # # `AnyActor` includes both authenticated and unauthenticated users. @@ -22,79 +21,66 @@ allow(actor: AnyActor, action: Action, resource) if actor.authenticated and has_permission(actor.authn_actor.unwrap(), action.to_perm(), resource); -# -# Resources -# - -# The "database" resource allows us to limit what users are allowed to perform -# operations that query the database (whether those read or write queries). -resource Database { - permissions = [ "query", "modify" ]; - roles = [ "user", "init" ]; - - "query" if "user"; - - "modify" if "init"; - "user" if "init"; -} - -# All authenticated users have the "user" role on the database. -has_role(_actor: AuthenticatedActor, "user", _resource: Database); -# The "db-init" user is the only one with the "init" role. -has_role(actor: AuthenticatedActor, "init", _resource: Database) - if actor = USER_DB_INIT; - # Define role relationships has_role(actor: AuthenticatedActor, role: String, resource: Resource) if resource.has_role(actor, role); # -# Permissions and predefined roles for resources in the -# Fleet/Silo/Organization/Project hierarchy +# ROLES AND PERMISSIONS IN THE FLEET/SILO/ORGANIZATION/PROJECT HIERARCHY # -# For now, we define the following permissions for most resources in the system: +# We define the following permissions for most resources in the system: # -# - "create_child": required to create child resources. +# - "create_child": required to create child resources (of any type) # -# - "list_children": required to list children (of all types) of a resources +# - "list_children": required to list child resources (of all types) of a +# resource # -# - "modify": required to modify or delete a resource or any of its children +# - "modify": required to modify or delete a resource # # - "read": required to read a resource # # We define the following predefined roles for only a few high-level resources: +# the Fleet (see below), Silo, Organization, and Project. The specific roles +# are oriented around intended use-cases: # # - "admin": has all permissions on the resource # -# - "collaborator": has "list_children" and "create_$child" for all children. -# They'll inherit the "admin" role for any resources that they create. +# - "collaborator": has "read", "list_children", and "create_child", plus +# the "admin" role for child resources. The idea is that if you're an +# Organization Collaborator, you have full control over the Projects within +# the Organization, but you cannot modify or delete the Organization itself. # # - "viewer": has "read" and "list_children" on a resource # -# Below the project level, permissions are granted at the Project level. For -# example, for someone to be able to create, modify, or delete any Instances, -# they must be granted project.collaborator, which means they can create, -# modify, or delete _all_ resources in the Project. +# Below the Project level, permissions are granted via roles at the Project +# level. For example, for someone to be able to create, modify, or delete any +# Instances, they must be granted project.collaborator, which means they can +# create, modify, or delete _all_ resources in the Project. # # The complete set of predefined roles: # # - fleet.admin (superuser for the whole system) -# - fleet.collaborator (can create and own silos) -# - fleet.viewer (can read fleet-wide data) +# - fleet.collaborator (can manage Silos) +# - fleet.viewer (can read most resources in the system) # - silo.admin (superuser for the silo) -# - silo.collaborator (can create and own orgs) -# - silo.viewer (can read silo-wide data) +# - silo.collaborator (can create and own Organizations) +# - silo.viewer (can read most resources within the Silo) # - organization.admin (complete control over an organization) -# - organization.collaborator (can create, modify, and delete projects) -# - project.admin (complete control over a project) -# - project.collaborator (can create, modify, and delete all resources within -# the project, but cannot modify or delete the project -# itself) -# - project.viewer (can see everything in the project, but cannot modify -# anything) +# - organization.collaborator (can manage Projects) +# - project.admin (complete control over a Project) +# - project.collaborator (can manage all resources within the Project) +# - project.viewer (can read most resources within the Project) +# +# Outside the Silo/Organization/Project hierarchy, we (currently) treat most +# resources as nested under Fleet or else a synthetic resource (see below). We +# do not yet support role assignments on anything other than Fleet, Silo, +# Organization, or Project. # -# At the top level is the "Fleet" resource. +# "Fleet" is a global singleton representing the whole system. The name comes +# from the idea described in RFD 24, but it's not quite right. This probably +# should be more like "Region" or "AvailabilityZone". The precise boundaries +# have not yet been figured out. resource Fleet { permissions = [ "list_children", @@ -104,27 +90,23 @@ resource Fleet { ]; roles = [ + # Roles that can be attached by users "admin", "collaborator", "viewer", - # internal roles + # Internal-only roles "external-authenticator" ]; - # Fleet viewers can view Fleet-wide data + # Roles implied by other roles on this resource + "viewer" if "collaborator"; + "collaborator" if "admin"; + + # Permissions granted directly by roles on this resource "list_children" if "viewer"; "read" if "viewer"; - - # Fleet collaborators can create Organizations and see fleet-wide - # information, including Organizations that they don't have permissions - # on. (They cannot list projects within those organizations, however.) - # They cannot modify fleet-wide information. - "viewer" if "collaborator"; "create_child" if "collaborator"; - - # Fleet administrators are whole-system superusers. - "collaborator" if "admin"; "modify" if "admin"; } @@ -137,27 +119,28 @@ resource Silo { ]; roles = [ "admin", "collaborator", "viewer" ]; - # permissions granted by this resource's roles + # Roles implied by other roles on this resource + "viewer" if "collaborator"; + "collaborator" if "admin"; + + # Permissions granted directly by roles on this resource "list_children" if "viewer"; "read" if "viewer"; "create_child" if "collaborator"; "modify" if "admin"; - # roles implied by other roles - "viewer" if "collaborator"; - "collaborator" if "admin"; - - # roles implied by relationships with the parent fleet + # Roles implied by roles on this resource's parent (Fleet) relations = { parent_fleet: Fleet }; "admin" if "collaborator" on "parent_fleet"; "viewer" if "viewer" on "parent_fleet"; } + has_relation(fleet: Fleet, "parent_fleet", silo: Silo) if silo.fleet = fleet; -# All authenticated users can read their own Silo. That's not quite the same as -# having the "viewer" role. For example, they cannot list Organizations in the -# Silo. +# As a special case, all authenticated users can read their own Silo. That's +# not quite the same as having the "viewer" role. For example, they cannot list +# Organizations in the Silo. # # One reason this is necessary is because if an unprivileged user tries to # create an Organization using "POST /organizations", they should get back a 403 @@ -183,20 +166,16 @@ resource Organization { ]; roles = [ "admin", "collaborator" ]; - # Organization collaborators can create Projects and see - # organization-wide information, including Projects that they don't have - # permissions on. (They cannot see anything inside those Projects, - # though.) They cannot modify or delete the organization itself. + # Roles implied by other roles on this resource + "collaborator" if "admin"; + + # Permissions granted directly by roles on this resource "list_children" if "collaborator"; "read" if "collaborator"; "create_child" if "collaborator"; - - # Organization administrators can modify and delete the Organization - # itself. They can also see and administer everything in the - # Organization (recursively). - "collaborator" if "admin"; "modify" if "admin"; + # Roles implied by roles on this resource's parent (Silo) relations = { parent_silo: Silo }; "admin" if "collaborator" on "parent_silo"; "read" if "viewer" on "parent_silo"; @@ -214,31 +193,70 @@ resource Project { ]; roles = [ "admin", "collaborator", "viewer" ]; - # Project viewers can see everything in the Project. + # Roles implied by other roles on this resource + "viewer" if "collaborator"; + "collaborator" if "admin"; + + # Permissions granted directly by roles on this resource "list_children" if "viewer"; "read" if "viewer"; - - # Project collaborators can see, modify, and delete everything inside - # the Project recursively. (This is different from Fleet and - # Organization-level collaborators, who can only modify and delete child - # resources that they have specific permissions on. That's because - # we're not implementing fine-grained permissions within Projects yet.) - # They cannot modify or delete the Project itself. - "viewer" if "collaborator"; "create_child" if "collaborator"; - - # Project administrators can modify and delete the Project" itself. - "collaborator" if "admin"; "modify" if "admin"; + # Roles implied by roles on this resource's parent (Organization) relations = { parent_organization: Organization }; "admin" if "collaborator" on "parent_organization"; - "viewer" if "list_children" on "parent_organization"; } has_relation(organization: Organization, "parent_organization", project: Project) if project.organization = organization; +# +# GENERAL RESOURCES OUTSIDE THE SILO/ORGANIZATION/PROJECT HIERARCHY +# +# Many resources use snippets of Polar generated by the `authz_resource!` Rust +# macro. Some resources require custom Polar code. Those appear here. +# + +resource SiloUser { + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + + relations = { parent_silo: Silo }; + "list_children" if "viewer" on "parent_silo"; + "read" if "viewer" on "parent_silo"; + "modify" if "admin" on "parent_silo"; + "create_child" if "admin" on "parent_silo"; +} +has_relation(silo: Silo, "parent_silo", user: SiloUser) + if user.silo = silo; + +resource SshKey { + permissions = [ "read", "modify" ]; + relations = { silo_user: SiloUser }; + + "read" if "read" on "silo_user"; + "modify" if "modify" on "silo_user"; +} +has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) + if ssh_key.silo_user = user; + +# +# SYNTHETIC RESOURCES OUTSIDE THE SILO HIERARCHY +# +# The resources here do not correspond to anything that appears explicitly in +# the API or is stored in the database. These are used either at the top level +# of the API path (e.g., "/images") or as an implementation detail of the system +# (in the case of console sessions and "Database"). The policies are +# either statically-defined in this file or driven by role assignments on the +# Fleet. +# + +# Describes the policy for accessing "/images" (in the API) resource GlobalImageList { permissions = [ "list_children", @@ -246,19 +264,18 @@ resource GlobalImageList { "create_child", ]; - # Only admins can create or modify the global images list + # Fleet Administrators can create or modify the global images list. relations = { parent_fleet: Fleet }; "modify" if "admin" on "parent_fleet"; "create_child" if "admin" on "parent_fleet"; - # Anyone with viewer can list global images + # Fleet Viewers can list global images. "list_children" if "viewer" on "parent_fleet"; } has_relation(fleet: Fleet, "parent_fleet", global_image_list: GlobalImageList) if global_image_list.fleet = fleet; -# ConsoleSessionList is a synthetic resource used for modeling who has access -# to create sessions. +# Describes the policy for creating and managing web console sessions. resource ConsoleSessionList { permissions = [ "create_child" ]; relations = { parent_fleet: Fleet }; @@ -277,29 +294,51 @@ has_permission(actor: AuthenticatedActor, "read", session: ConsoleSession) has_permission(actor: AuthenticatedActor, "modify", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); -resource SiloUser { + +# Describes the policy for who can access the internal database. +resource Database { permissions = [ - "list_children", - "modify", - "read", - "create_child", + # "query" is required to perform any query against the database, + # whether a read or write query. This is checked when an operation + # checks out a database connection from the connection pool. + # + # Any authenticated user gets this permission. There's generally + # some other authz check involved in the database query. For + # example, if you'er querying the database to "read" a "Project", we + # should also be checking that. So why do we do this at all? It's + # a belt-and-suspenders measure so that if we somehow introduced an + # unauthenticated code path that hits the database, it cannot be + # used to DoS the database because we won't allow the operation to + # make the query. (As long as the code path _is_ authenticated, we + # can use throttling mechanisms to prevent DoS.) + "query", + + # "modify" is required to populate database data that's delivered + # with the system. It should also be required for schema changes, + # when we support those. This is separate from "query" so that we + # cannot accidentally invoke these code paths from API calls and + # other general functions. + "modify" ]; - relations = { parent_silo: Silo }; + roles = [ + # All authenticated users get the "user" role, which grants the + # "query" permission. See above. + "user", - "list_children" if "viewer" on "parent_silo"; - "read" if "viewer" on "parent_silo"; - "modify" if "admin" on "parent_silo"; - "create_child" if "admin" on "parent_silo"; -} -has_relation(silo: Silo, "parent_silo", user: SiloUser) - if user.silo = silo; + # The special "db-init" user gets the "init" role, which grants the + # additional "modify" permission. + "init" + ]; -resource SshKey { - permissions = [ "read", "modify" ]; - relations = { silo_user: SiloUser }; + # See above. + "query" if "user"; - "read" if "read" on "silo_user"; - "modify" if "modify" on "silo_user"; + "user" if "init"; + "modify" if "init"; } -has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) - if ssh_key.silo_user = user; + +# All authenticated users have the "user" role on the database. +has_role(_actor: AuthenticatedActor, "user", _resource: Database); +# The "db-init" user is the only one with the "init" role. +has_role(actor: AuthenticatedActor, "init", _resource: Database) + if actor = USER_DB_INIT; From cdfdca6d1a5ae1578b862650ef6df6d2bf10cc62 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 15:35:56 -0700 Subject: [PATCH 5/9] clean up Polar file: includes minor functional changes --- nexus/src/authz/api_resources.rs | 3 +++ nexus/src/authz/omicron.polar | 37 +++++++++++--------------------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 915b98b19b5..2f86a7425a1 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -391,6 +391,7 @@ impl ApiResourceWithRolesType for Organization { pub enum OrganizationRoles { Admin, Collaborator, + Viewer, } impl db::model::DatabaseString for OrganizationRoles { @@ -400,6 +401,7 @@ impl db::model::DatabaseString for OrganizationRoles { match self { OrganizationRoles::Admin => "admin", OrganizationRoles::Collaborator => "collaborator", + OrganizationRoles::Viewer => "viewer", } } @@ -407,6 +409,7 @@ impl db::model::DatabaseString for OrganizationRoles { match s { "admin" => Ok(OrganizationRoles::Admin), "collaborator" => Ok(OrganizationRoles::Collaborator), + "viewer" => Ok(OrganizationRoles::Viewer), _ => Err(anyhow!( "unsupported Organization role from database: {:?}", s diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index c306de348ba..634cdf6d5aa 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -67,6 +67,7 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource) # - silo.viewer (can read most resources within the Silo) # - organization.admin (complete control over an organization) # - organization.collaborator (can manage Projects) +# - organization.viewer (can read most resources within the Organization) # - project.admin (complete control over a Project) # - project.collaborator (can manage all resources within the Project) # - project.viewer (can read most resources within the Project) @@ -164,22 +165,22 @@ resource Organization { "read", "create_child", ]; - roles = [ "admin", "collaborator" ]; + roles = [ "admin", "collaborator", "viewer" ]; # Roles implied by other roles on this resource + "viewer" if "collaborator"; "collaborator" if "admin"; # Permissions granted directly by roles on this resource - "list_children" if "collaborator"; - "read" if "collaborator"; + "list_children" if "viewer"; + "read" if "viewer"; "create_child" if "collaborator"; "modify" if "admin"; # Roles implied by roles on this resource's parent (Silo) relations = { parent_silo: Silo }; "admin" if "collaborator" on "parent_silo"; - "read" if "viewer" on "parent_silo"; - "list_children" if "viewer" on "parent_silo"; + "viewer" if "viewer" on "parent_silo"; } has_relation(silo: Silo, "parent_silo", organization: Organization) if organization.silo = silo; @@ -206,7 +207,7 @@ resource Project { # Roles implied by roles on this resource's parent (Organization) relations = { parent_organization: Organization }; "admin" if "collaborator" on "parent_organization"; - "viewer" if "list_children" on "parent_organization"; + "viewer" if "viewer" on "parent_organization"; } has_relation(organization: Organization, "parent_organization", project: Project) if project.organization = organization; @@ -253,7 +254,7 @@ has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) # of the API path (e.g., "/images") or as an implementation detail of the system # (in the case of console sessions and "Database"). The policies are # either statically-defined in this file or driven by role assignments on the -# Fleet. +# Fleet. None of these resources define their own roles. # # Describes the policy for accessing "/images" (in the API) @@ -320,25 +321,11 @@ resource Database { # other general functions. "modify" ]; - roles = [ - # All authenticated users get the "user" role, which grants the - # "query" permission. See above. - "user", - - # The special "db-init" user gets the "init" role, which grants the - # additional "modify" permission. - "init" - ]; - - # See above. - "query" if "user"; - - "user" if "init"; - "modify" if "init"; } -# All authenticated users have the "user" role on the database. -has_role(_actor: AuthenticatedActor, "user", _resource: Database); +# All authenticated users have the "query" permission on the database. +has_permission(_actor: AuthenticatedActor, "query", _resource: Database); + # The "db-init" user is the only one with the "init" role. -has_role(actor: AuthenticatedActor, "init", _resource: Database) +has_permission(actor: AuthenticatedActor, "modify", _resource: Database) if actor = USER_DB_INIT; From 2c16b2d77d1678b5fc8651a0d582e4142ab29e14 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 16:44:57 -0700 Subject: [PATCH 6/9] add a test that the enums and fixed_data are in sync --- nexus/src/db/fixed_data/role_builtin.rs | 56 ++++++++++++++++++- .../tests/integration_tests/roles_builtin.rs | 1 + .../tests/output/authz-roles-organization.txt | 1 + openapi/nexus.json | 3 +- 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/nexus/src/db/fixed_data/role_builtin.rs b/nexus/src/db/fixed_data/role_builtin.rs index 8522bafd658..33c64ea7a1d 100644 --- a/nexus/src/db/fixed_data/role_builtin.rs +++ b/nexus/src/db/fixed_data/role_builtin.rs @@ -6,7 +6,7 @@ use lazy_static::lazy_static; use omicron_common::api; -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct RoleBuiltinConfig { pub resource_type: api::external::ResourceType, pub role_name: &'static str, @@ -67,6 +67,11 @@ lazy_static! { }, ORGANIZATION_ADMINISTRATOR.clone(), ORGANIZATION_COLLABORATOR.clone(), + RoleBuiltinConfig { + resource_type: api::external::ResourceType::Organization, + role_name: "viewer", + description: "Organization Viewer", + }, RoleBuiltinConfig { resource_type: api::external::ResourceType::Project, role_name: "admin", @@ -84,3 +89,52 @@ lazy_static! { }, ]; } + +#[cfg(test)] +mod test { + use super::BUILTIN_ROLES; + use crate::authz; + use crate::db::model::DatabaseString; + use omicron_common::api::external::ResourceType; + use strum::IntoEnumIterator; + + #[test] + fn test_fixed_role_data() { + // Every role that's defined in the public API as assignable on a + // resource must have a corresponding entry in BUILTIN_ROLES above. + // The reverse is not necessarily true because we have some internal + // roles that are not exposed to end users. + check_public_roles::(ResourceType::Fleet); + check_public_roles::(ResourceType::Silo); + check_public_roles::( + ResourceType::Organization, + ); + check_public_roles::(ResourceType::Project); + } + + fn check_public_roles(resource_type: ResourceType) + where + T: std::fmt::Debug + DatabaseString + IntoEnumIterator, + { + for variant in T::iter() { + let role_name = variant.to_database_string(); + + let found = BUILTIN_ROLES.iter().find(|role_config| { + role_config.resource_type == resource_type + && role_config.role_name == role_name + }); + if let Some(found_config) = found { + println!( + "variant: {:?} found fixed data {:?}", + variant, found_config + ); + } else { + panic!( + "found public role {:?} on {:?} with no corresponding \ + built-in role", + role_name, resource_type + ); + } + } + } +} diff --git a/nexus/tests/integration_tests/roles_builtin.rs b/nexus/tests/integration_tests/roles_builtin.rs index 3e29f3211c5..2f6ce82827c 100644 --- a/nexus/tests/integration_tests/roles_builtin.rs +++ b/nexus/tests/integration_tests/roles_builtin.rs @@ -34,6 +34,7 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { ("fleet.viewer", "Fleet Viewer"), ("organization.admin", "Organization Administrator"), ("organization.collaborator", "Organization Collaborator"), + ("organization.viewer", "Organization Viewer"), ("project.admin", "Project Administrator"), ("project.collaborator", "Project Collaborator"), ("project.viewer", "Project Viewer"), diff --git a/nexus/tests/output/authz-roles-organization.txt b/nexus/tests/output/authz-roles-organization.txt index 3ce6f774599..2d9076f78be 100644 --- a/nexus/tests/output/authz-roles-organization.txt +++ b/nexus/tests/output/authz-roles-organization.txt @@ -1,2 +1,3 @@ variant Admin: serialized form = admin variant Collaborator: serialized form = collaborator +variant Viewer: serialized form = viewer diff --git a/openapi/nexus.json b/openapi/nexus.json index 066c0aa399e..2f0cbb242c2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6643,7 +6643,8 @@ "type": "string", "enum": [ "admin", - "collaborator" + "collaborator", + "viewer" ] }, "OrganizationRolesPolicy": { From 92c760c8909918a0c02e337c82f4128a9e01d0d8 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 15:35:56 -0700 Subject: [PATCH 7/9] clean up Polar file: includes minor functional changes --- nexus/src/authz/api_resources.rs | 3 +++ nexus/src/authz/omicron.polar | 37 +++++++++++--------------------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 915b98b19b5..2f86a7425a1 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -391,6 +391,7 @@ impl ApiResourceWithRolesType for Organization { pub enum OrganizationRoles { Admin, Collaborator, + Viewer, } impl db::model::DatabaseString for OrganizationRoles { @@ -400,6 +401,7 @@ impl db::model::DatabaseString for OrganizationRoles { match self { OrganizationRoles::Admin => "admin", OrganizationRoles::Collaborator => "collaborator", + OrganizationRoles::Viewer => "viewer", } } @@ -407,6 +409,7 @@ impl db::model::DatabaseString for OrganizationRoles { match s { "admin" => Ok(OrganizationRoles::Admin), "collaborator" => Ok(OrganizationRoles::Collaborator), + "viewer" => Ok(OrganizationRoles::Viewer), _ => Err(anyhow!( "unsupported Organization role from database: {:?}", s diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 35829817295..39a66bb4352 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -67,6 +67,7 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource) # - silo.viewer (can read most resources within the Silo) # - organization.admin (complete control over an organization) # - organization.collaborator (can manage Projects) +# - organization.viewer (can read most resources within the Organization) # - project.admin (complete control over a Project) # - project.collaborator (can manage all resources within the Project) # - project.viewer (can read most resources within the Project) @@ -164,22 +165,22 @@ resource Organization { "read", "create_child", ]; - roles = [ "admin", "collaborator" ]; + roles = [ "admin", "collaborator", "viewer" ]; # Roles implied by other roles on this resource + "viewer" if "collaborator"; "collaborator" if "admin"; # Permissions granted directly by roles on this resource - "list_children" if "collaborator"; - "read" if "collaborator"; + "list_children" if "viewer"; + "read" if "viewer"; "create_child" if "collaborator"; "modify" if "admin"; # Roles implied by roles on this resource's parent (Silo) relations = { parent_silo: Silo }; "admin" if "collaborator" on "parent_silo"; - "read" if "viewer" on "parent_silo"; - "list_children" if "viewer" on "parent_silo"; + "viewer" if "viewer" on "parent_silo"; } has_relation(silo: Silo, "parent_silo", organization: Organization) if organization.silo = silo; @@ -206,7 +207,7 @@ resource Project { # Roles implied by roles on this resource's parent (Organization) relations = { parent_organization: Organization }; "admin" if "collaborator" on "parent_organization"; - "viewer" if "list_children" on "parent_organization"; + "viewer" if "viewer" on "parent_organization"; } has_relation(organization: Organization, "parent_organization", project: Project) if project.organization = organization; @@ -253,7 +254,7 @@ has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) # of the API path (e.g., "/images") or as an implementation detail of the system # (in the case of console sessions and "Database"). The policies are # either statically-defined in this file or driven by role assignments on the -# Fleet. +# Fleet. None of these resources define their own roles. # # Describes the policy for accessing "/images" (in the API) @@ -320,25 +321,11 @@ resource Database { # other general functions. "modify" ]; - roles = [ - # All authenticated users get the "user" role, which grants the - # "query" permission. See above. - "user", - - # The special "db-init" user gets the "init" role, which grants the - # additional "modify" permission. - "init" - ]; - - # See above. - "query" if "user"; - - "user" if "init"; - "modify" if "init"; } -# All authenticated users have the "user" role on the database. -has_role(_actor: AuthenticatedActor, "user", _resource: Database); +# All authenticated users have the "query" permission on the database. +has_permission(_actor: AuthenticatedActor, "query", _resource: Database); + # The "db-init" user is the only one with the "init" role. -has_role(actor: AuthenticatedActor, "init", _resource: Database) +has_permission(actor: AuthenticatedActor, "modify", _resource: Database) if actor = USER_DB_INIT; From f8eed9da6d647f6a7a195d86139c6d2c8b94c053 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 16:44:57 -0700 Subject: [PATCH 8/9] add a test that the enums and fixed_data are in sync --- nexus/src/db/fixed_data/role_builtin.rs | 56 ++++++++++++++++++- .../tests/integration_tests/roles_builtin.rs | 1 + .../tests/output/authz-roles-organization.txt | 1 + openapi/nexus.json | 3 +- 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/nexus/src/db/fixed_data/role_builtin.rs b/nexus/src/db/fixed_data/role_builtin.rs index 8522bafd658..33c64ea7a1d 100644 --- a/nexus/src/db/fixed_data/role_builtin.rs +++ b/nexus/src/db/fixed_data/role_builtin.rs @@ -6,7 +6,7 @@ use lazy_static::lazy_static; use omicron_common::api; -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct RoleBuiltinConfig { pub resource_type: api::external::ResourceType, pub role_name: &'static str, @@ -67,6 +67,11 @@ lazy_static! { }, ORGANIZATION_ADMINISTRATOR.clone(), ORGANIZATION_COLLABORATOR.clone(), + RoleBuiltinConfig { + resource_type: api::external::ResourceType::Organization, + role_name: "viewer", + description: "Organization Viewer", + }, RoleBuiltinConfig { resource_type: api::external::ResourceType::Project, role_name: "admin", @@ -84,3 +89,52 @@ lazy_static! { }, ]; } + +#[cfg(test)] +mod test { + use super::BUILTIN_ROLES; + use crate::authz; + use crate::db::model::DatabaseString; + use omicron_common::api::external::ResourceType; + use strum::IntoEnumIterator; + + #[test] + fn test_fixed_role_data() { + // Every role that's defined in the public API as assignable on a + // resource must have a corresponding entry in BUILTIN_ROLES above. + // The reverse is not necessarily true because we have some internal + // roles that are not exposed to end users. + check_public_roles::(ResourceType::Fleet); + check_public_roles::(ResourceType::Silo); + check_public_roles::( + ResourceType::Organization, + ); + check_public_roles::(ResourceType::Project); + } + + fn check_public_roles(resource_type: ResourceType) + where + T: std::fmt::Debug + DatabaseString + IntoEnumIterator, + { + for variant in T::iter() { + let role_name = variant.to_database_string(); + + let found = BUILTIN_ROLES.iter().find(|role_config| { + role_config.resource_type == resource_type + && role_config.role_name == role_name + }); + if let Some(found_config) = found { + println!( + "variant: {:?} found fixed data {:?}", + variant, found_config + ); + } else { + panic!( + "found public role {:?} on {:?} with no corresponding \ + built-in role", + role_name, resource_type + ); + } + } + } +} diff --git a/nexus/tests/integration_tests/roles_builtin.rs b/nexus/tests/integration_tests/roles_builtin.rs index 3e29f3211c5..2f6ce82827c 100644 --- a/nexus/tests/integration_tests/roles_builtin.rs +++ b/nexus/tests/integration_tests/roles_builtin.rs @@ -34,6 +34,7 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { ("fleet.viewer", "Fleet Viewer"), ("organization.admin", "Organization Administrator"), ("organization.collaborator", "Organization Collaborator"), + ("organization.viewer", "Organization Viewer"), ("project.admin", "Project Administrator"), ("project.collaborator", "Project Collaborator"), ("project.viewer", "Project Viewer"), diff --git a/nexus/tests/output/authz-roles-organization.txt b/nexus/tests/output/authz-roles-organization.txt index 3ce6f774599..2d9076f78be 100644 --- a/nexus/tests/output/authz-roles-organization.txt +++ b/nexus/tests/output/authz-roles-organization.txt @@ -1,2 +1,3 @@ variant Admin: serialized form = admin variant Collaborator: serialized form = collaborator +variant Viewer: serialized form = viewer diff --git a/openapi/nexus.json b/openapi/nexus.json index 066c0aa399e..2f0cbb242c2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6643,7 +6643,8 @@ "type": "string", "enum": [ "admin", - "collaborator" + "collaborator", + "viewer" ] }, "OrganizationRolesPolicy": { From 40f60bd52f84a2cc6a2565bf78fc160ae2b8fd7b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 May 2022 18:31:45 -0700 Subject: [PATCH 9/9] grammar --- nexus/src/authz/omicron.polar | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 39a66bb4352..98afd891bdf 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -254,7 +254,7 @@ has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) # of the API path (e.g., "/images") or as an implementation detail of the system # (in the case of console sessions and "Database"). The policies are # either statically-defined in this file or driven by role assignments on the -# Fleet. None of these resources define their own roles. +# Fleet. None of these resources defines their own roles. # # Describes the policy for accessing "/images" (in the API)