From 9db0fcf14b086e5af11c961d23cfd2fbc8aacb77 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 15 Apr 2022 14:24:43 -0700 Subject: [PATCH 1/5] first cut --- common/src/api/external/error.rs | 2 +- nexus/src/authz/api_resources.rs | 2 +- nexus/src/authz/omicron.polar | 33 +++++++++-- nexus/src/db/datastore.rs | 14 ++--- nexus/src/db/db-macros/src/lookup.rs | 13 ++++- nexus/src/db/lookup.rs | 84 ++++++++++++++++------------ nexus/src/db/model.rs | 6 +- nexus/src/nexus.rs | 16 +++--- 8 files changed, 106 insertions(+), 64 deletions(-) diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index 36a1f33eee..04e7f9dae3 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -24,7 +24,7 @@ use uuid::Uuid; /// General best practices for error design apply here. Where possible, we want /// to reuse existing variants rather than inventing new ones to distinguish /// cases that no programmatic consumer needs to distinguish. -#[derive(Debug, Deserialize, thiserror::Error, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, thiserror::Error, PartialEq, Serialize)] pub enum Error { /// An object needed as part of this operation was not found. #[error("Object (of type {lookup_type:?}) not found: {type_name}")] diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index d419d1c49c..d97e9854f4 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -254,7 +254,7 @@ impl AuthorizedResource for ConsoleSessionList { authz_resource! { name = "Organization", - parent = "Fleet", + parent = "Silo", primary_key = Uuid, roles_allowed = true, polar_snippet = Custom, diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 370debb2f4..cb1d334ca0 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -50,7 +50,7 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource) # # Permissions and predefined roles for resources in the -# Fleet/Organization/Project hierarchy +# Fleet/Silo/Organization/Project hierarchy # # For now, we define the following permissions for most resources in the system: # @@ -79,8 +79,11 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource) # The complete set of predefined roles: # # - fleet.admin (superuser for the whole system) -# - fleet.collaborator (can create and own orgs) +# - fleet.collaborator (can create and own silos) # - fleet.viewer (can read fleet-wide data) +# - silo.admin (superuser for the silo) +# - silo.collaborator (can create and own orgs) +# - silo.viewer (can read silo-wide data) # - organization.admin (complete control over an organization) # - organization.collaborator (can create, modify, and delete projects) # - project.admin (complete control over a project) @@ -125,6 +128,24 @@ resource Fleet { "modify" if "admin"; } +resource Silo { + permissions = [ + "list_children", + "modify", + "read", + "create_child", + ]; + roles = [ "admin", "collaborator", "viewer" ]; + + "list_children" if "viewer"; + "read" if "viewer"; + + "viewer" if "collaborator"; + "create_child" if "collaborator"; + "collaborator" if "admin"; + "modify" if "admin"; +} + resource Organization { permissions = [ "list_children", @@ -148,11 +169,11 @@ resource Organization { "collaborator" if "admin"; "modify" if "admin"; - relations = { parent_fleet: Fleet }; - "admin" if "admin" on "parent_fleet"; + relations = { parent_silo: Silo }; + "admin" if "admin" on "parent_silo"; } -has_relation(fleet: Fleet, "parent_fleet", organization: Organization) - if organization.fleet = fleet; +has_relation(silo: Silo, "parent_silo", organization: Organization) + if organization.silo = silo; resource Project { permissions = [ diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index e9a47a8bbc..6d5441995d 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -523,7 +523,7 @@ impl DataStore { opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?; let name = organization.name().as_str().to_string(); - let silo_id = organization.silo_id(); + let silo_id = organization.silo_id; Silo::insert_resource( silo_id, @@ -2768,12 +2768,12 @@ mod test { }, }, ); - let org = authz::Organization::new( - authz::FLEET, - organization.id(), - LookupType::ById(organization.id()), - ); - datastore.project_create(&opctx, &org, project).await.unwrap(); + let (.., authz_org) = LookupPath::new(&opctx, &datastore) + .organization_id(organization.id()) + .lookup_for(authz::Action::CreateChild) + .await + .unwrap(); + datastore.project_create(&opctx, &authz_org, project).await.unwrap(); let (.., organization_after_project_create) = LookupPath::new(&opctx, &datastore) diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index 973c48f36e..dafa32dee5 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -223,6 +223,11 @@ fn generate_struct(config: &Config) -> TokenStream { /// Describes how we're looking up this resource enum #lookup_enum<'a>{ + /// An error occurred while selecting the resource + /// + /// This error will be returned by any lookup/fetch attempts. + Error(Root<'a>, Error), + #name_variant /// We're looking for a resource with the given primary key @@ -317,6 +322,8 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { /// used for this lookup. fn lookup_root(&self) -> &LookupPath<'a> { match &self.key { + #lookup_enum::Error(root, ..) => root.lookup_root(), + #name_variant #lookup_enum::PrimaryKey(root, ..) => root.lookup_root(), @@ -429,6 +436,8 @@ fn generate_lookup_methods(config: &Config) -> TokenStream { let datastore = &lookup.datastore; match &self.key { + #lookup_enum::Error(_, error) => Err(error.clone()), + #fetch_for_name_variant #lookup_enum::PrimaryKey(_, #(#pkey_names,)*) => { @@ -478,6 +487,8 @@ fn generate_lookup_methods(config: &Config) -> TokenStream { let datastore = &lookup.datastore; match &self.key { + #lookup_enum::Error(_, error) => Err(error.clone()), + #lookup_name_variant #lookup_enum::PrimaryKey(_, #(#pkey_names,)*) => { @@ -742,7 +753,7 @@ mod test { let output = lookup_resource( quote! { name = "Organization", - ancestors = [], + ancestors = ["Silo"], children = [ "Project" ], lookup_by_name = true, soft_deletes = true, diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 2c7144a3fc..2476da5414 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -185,9 +185,20 @@ impl<'a> LookupPath<'a> { 'a: 'c, 'b: 'c, { - Organization { - key: OrganizationKey::Name(Root { lookup_root: self }, name), - } + // XXX-dap XXX-dap The macro needs to be updated so that the by-id + // functions all check whether the resource exists in the right Silo + let key = match self.opctx.authn.silo_required() { + Ok(silo_id) => { + let root = Root { lookup_root: self }; + let silo_key = SiloKey::PrimaryKey(root, silo_id); + OrganizationKey::Name(Silo { key: silo_key }, name) + } + Err(error) => { + let root = Root { lookup_root: self }; + OrganizationKey::Error(root, error) + } + }; + Organization { key } } /// Select a resource of type Organization, identified by its id @@ -260,23 +271,26 @@ impl<'a> LookupPath<'a> { } /// Select a resource of type RoleBuiltin, identified by its `name` - pub fn role_builtin_name( - self, - name: &str, - ) -> Result, Error> { - let (resource_type, role_name) = - name.split_once(".").ok_or_else(|| Error::ObjectNotFound { - type_name: ResourceType::RoleBuiltin, - lookup_type: LookupType::ByName(String::from(name)), - })?; - - Ok(RoleBuiltin { - key: RoleBuiltinKey::PrimaryKey( + pub fn role_builtin_name(self, name: &str) -> RoleBuiltin<'a> { + let parts = name.split_once("."); + let key = if let Some((resource_type, role_name)) = parts { + RoleBuiltinKey::PrimaryKey( Root { lookup_root: self }, resource_type.to_string(), role_name.to_string(), - ), - }) + ) + } else { + let root = Root { lookup_root: self }; + RoleBuiltinKey::Error( + root, + Error::ObjectNotFound { + type_name: ResourceType::RoleBuiltin, + lookup_type: LookupType::ByName(String::from(name)), + }, + ) + }; + + RoleBuiltin { key } } /// Select a resource of type Silo, identified by its id @@ -354,8 +368,17 @@ impl<'a> Root<'a> { // Main resource hierarchy: Organizations, Projects, and their resources lookup_resource! { - name = "Organization", + name = "Silo", ancestors = [], + children = [ "Organization" ], + lookup_by_name = true, + soft_deletes = true, + primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] +} + +lookup_resource! { + name = "Organization", + ancestors = [ "Silo" ], children = [ "Project" ], lookup_by_name = true, soft_deletes = true, @@ -364,7 +387,7 @@ lookup_resource! { lookup_resource! { name = "Project", - ancestors = [ "Organization" ], + ancestors = [ "Silo", "Organization" ], children = [ "Disk", "Instance", "Vpc" ], lookup_by_name = true, soft_deletes = true, @@ -373,7 +396,7 @@ lookup_resource! { lookup_resource! { name = "Disk", - ancestors = [ "Organization", "Project" ], + ancestors = [ "Silo", "Organization", "Project" ], children = [], lookup_by_name = true, soft_deletes = true, @@ -382,7 +405,7 @@ lookup_resource! { lookup_resource! { name = "Instance", - ancestors = [ "Organization", "Project" ], + ancestors = [ "Silo", "Organization", "Project" ], children = [ "NetworkInterface" ], lookup_by_name = true, soft_deletes = true, @@ -391,7 +414,7 @@ lookup_resource! { lookup_resource! { name = "NetworkInterface", - ancestors = [ "Organization", "Project", "Instance" ], + ancestors = [ "Silo", "Organization", "Project", "Instance" ], children = [], lookup_by_name = true, soft_deletes = true, @@ -400,7 +423,7 @@ lookup_resource! { lookup_resource! { name = "Vpc", - ancestors = [ "Organization", "Project" ], + ancestors = [ "Silo", "Organization", "Project" ], children = [ "VpcRouter", "VpcSubnet" ], lookup_by_name = true, soft_deletes = true, @@ -409,7 +432,7 @@ lookup_resource! { lookup_resource! { name = "VpcRouter", - ancestors = [ "Organization", "Project", "Vpc" ], + ancestors = [ "Silo", "Organization", "Project", "Vpc" ], children = [ "RouterRoute" ], lookup_by_name = true, soft_deletes = true, @@ -418,7 +441,7 @@ lookup_resource! { lookup_resource! { name = "RouterRoute", - ancestors = [ "Organization", "Project", "Vpc", "VpcRouter" ], + ancestors = [ "Silo", "Organization", "Project", "Vpc", "VpcRouter" ], children = [], lookup_by_name = true, soft_deletes = true, @@ -427,7 +450,7 @@ lookup_resource! { lookup_resource! { name = "VpcSubnet", - ancestors = [ "Organization", "Project", "Vpc" ], + ancestors = [ "Silo", "Organization", "Project", "Vpc" ], children = [ ], lookup_by_name = true, soft_deletes = true, @@ -459,15 +482,6 @@ lookup_resource! { ] } -lookup_resource! { - name = "Silo", - ancestors = [], - children = [], - lookup_by_name = true, - soft_deletes = true, - primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] -} - lookup_resource! { name = "SiloUser", ancestors = [], diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index dfd268a41a..1152584f7e 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1015,7 +1015,7 @@ pub struct Organization { #[diesel(embed)] identity: OrganizationIdentity, - silo_id: Uuid, + pub silo_id: Uuid, /// child resource generation number, per RFD 192 pub rcgen: Generation, @@ -1031,10 +1031,6 @@ impl Organization { rcgen: Generation::new(), } } - - pub fn silo_id(&self) -> Uuid { - self.silo_id - } } impl DatastoreCollection for Organization { diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index bd05aee1c5..8b448ef266 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -887,7 +887,7 @@ impl Nexus { organization_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let (authz_org,) = LookupPath::new(opctx, &self.db_datastore) + let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .lookup_for(authz::Action::CreateChild) .await?; @@ -902,7 +902,7 @@ impl Nexus { organization_name: &Name, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - let (authz_org,) = LookupPath::new(opctx, &self.db_datastore) + let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .lookup_for(authz::Action::CreateChild) .await?; @@ -1765,14 +1765,14 @@ impl Nexus { instance_name: &Name, disk_name: &Name, ) -> UpdateResult { - let (_, authz_project, authz_disk, db_disk) = + let (.., authz_project, authz_disk, db_disk) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .project_name(project_name) .disk_name(disk_name) .fetch() .await?; - let (_, _, authz_instance, db_instance) = + let (.., authz_instance, db_instance) = LookupPath::new(opctx, &self.db_datastore) .project_id(authz_project.id()) .instance_name(instance_name) @@ -1914,14 +1914,14 @@ impl Nexus { instance_name: &Name, disk_name: &Name, ) -> UpdateResult { - let (_, authz_project, authz_disk, db_disk) = + let (.., authz_project, authz_disk, db_disk) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .project_name(project_name) .disk_name(disk_name) .fetch() .await?; - let (_, _, authz_instance, db_instance) = + let (.., authz_instance, db_instance) = LookupPath::new(opctx, &self.db_datastore) .project_id(authz_project.id()) .instance_name(instance_name) @@ -2063,7 +2063,7 @@ impl Nexus { instance_name: &Name, params: ¶ms::NetworkInterfaceCreate, ) -> CreateResult { - let (_, authz_project, authz_instance, db_instance) = + let (.., authz_project, authz_instance, db_instance) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .project_name(project_name) @@ -3112,7 +3112,7 @@ impl Nexus { name: &str, ) -> LookupResult { let (.., db_role_builtin) = LookupPath::new(opctx, &self.db_datastore) - .role_builtin_name(name)? + .role_builtin_name(name) .fetch() .await?; Ok(db_role_builtin) From ccf976f680ac6eca1ebbd0dda216b73d2e632057 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 15 Apr 2022 15:13:00 -0700 Subject: [PATCH 2/5] fixes --- nexus/src/authz/omicron.polar | 6 ++++++ nexus/src/db/lookup.rs | 20 ++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index cb1d334ca0..2dd84a460f 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -144,7 +144,13 @@ resource Silo { "create_child" if "collaborator"; "collaborator" if "admin"; "modify" if "admin"; + relations = { parent_fleet: Fleet }; + "admin" if "admin" on "parent_fleet"; + "collaborator" if "collaborator" on "parent_fleet"; + "viewer" if "viewer" on "parent_fleet"; } +has_relation(fleet: Fleet, "parent_fleet", silo: Silo) + if silo.fleet = fleet; resource Organization { permissions = [ diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 2476da5414..6a0c98ccd8 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -52,7 +52,8 @@ use uuid::Uuid; /// /// // Fetch an organization by name /// let organization_name = db::model::Name("engineering".parse().unwrap()); -/// let (authz_org, db_org): (authz::Organization, db::model::Organization) = +/// let (authz_silo, authz_org, db_org): +/// (authz::Silo, authz::Organization, db::model::Organization) = /// LookupPath::new(opctx, datastore) /// .organization_name(&organization_name) /// .fetch() @@ -60,7 +61,8 @@ use uuid::Uuid; /// /// // Fetch an organization by id /// let id: Uuid = todo!(); -/// let (authz_org, db_org): (authz::Organization, db::model::Organization) = +/// let (authz_silo, authz_org, db_org): +/// (authz::Silo, authz::Organization, db::model::Organization) = /// LookupPath::new(opctx, datastore) /// .organization_id(id) /// .fetch() @@ -70,7 +72,7 @@ use uuid::Uuid; /// // this purpose, we don't need the database row for the Project, so we use /// // `lookup_for()`. /// let project_name = db::model::Name("omicron".parse().unwrap()); -/// let (authz_org, authz_project) = +/// let (authz_silo, authz_org, authz_project) = /// LookupPath::new(opctx, datastore) /// .organization_name(&organization_name) /// .project_name(&project_name) @@ -80,7 +82,7 @@ use uuid::Uuid; /// // Fetch an Instance by a path of names (Organization name, Project name, /// // Instance name) /// let instance_name = db::model::Name("test-server".parse().unwrap()); -/// let (authz_org, authz_project, authz_instance, db_instance) = +/// let (authz_silo, authz_org, authz_project, authz_instance, db_instance) = /// LookupPath::new(opctx, datastore) /// .organization_name(&organization_name) /// .project_name(&project_name) @@ -91,7 +93,7 @@ use uuid::Uuid; /// // Having looked up the Instance, you have the `authz::Project`. Use this /// // to look up a Disk that you expect is in the same Project. /// let disk_name = db::model::Name("my-disk".parse().unwrap()); -/// let (_, _, authz_disk, db_disk) = +/// let (.., authz_disk, db_disk) = /// LookupPath::new(opctx, datastore) /// .project_id(authz_project.id()) /// .disk_name(&disk_name) @@ -134,6 +136,12 @@ use uuid::Uuid; // +----------+ // | // v +// Silo +// key: Key::PrimaryKey(r, id) +// | +// +--------------------+ +// | +// v // Root // lookup_root: LookupPath (references OpContext and // DataStore) @@ -142,7 +150,7 @@ use uuid::Uuid; // (rather than references) the previous node. This is important: the caller's // going to do something like this: // -// let (authz_org, authz_project, authz_instance, db_instance) = +// let (authz_silo, authz_org, authz_project, authz_instance, db_instance) = // LookupPath::new(opctx, datastore) // returns LookupPath // .organization_name("org1") // consumes LookupPath, // // returns Organization From 93ae1d1f1e141f6979333b2f22e3bfa07a4b968a Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 18 Apr 2022 10:11:31 -0700 Subject: [PATCH 3/5] check the resource's silo --- nexus/src/db/db-macros/src/lookup.rs | 105 +++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index dafa32dee5..91a3be6611 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -67,6 +67,9 @@ pub struct Config { /// This resources supports soft-deletes soft_deletes: bool, + /// This resource is nested under the Silo hierarchy + siloed: bool, + /// Name of the enum describing how we're looking up this resource lookup_enum: syn::Ident, @@ -116,9 +119,11 @@ impl Config { let child_resources = input.children; let parent = input.ancestors.last().map(|s| Resource::for_name(&s)); + let siloed = input.ancestors.iter().any(|s| s == "Silo"); Config { resource, + siloed, lookup_enum, path_types, path_authz_names, @@ -291,6 +296,7 @@ fn generate_child_selectors(config: &Config) -> TokenStream { fn generate_misc_helpers(config: &Config) -> TokenStream { let fleet_name = format_ident!("Fleet"); let resource_name = &config.resource.name; + let resource_name_str = resource_name.to_string(); let parent_resource_name = config.parent.as_ref().map(|p| &p.name).unwrap_or(&fleet_name); let lookup_enum = &config.lookup_enum; @@ -301,6 +307,68 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { quote! {} }; + let resource_authz_name = &config.resource.authz_name; + let silo_check_fn = if config.siloed { + quote! { + /// For a "siloed" resource (i.e., one that's nested under "Silo" in + /// the resource hierarchy), check whether a given resource's Silo + /// (given by `authz_silo`) matches the Silo of the actor doing the + /// fetch/lookup (given by `opctx`). + /// + /// This check should not be strictly necessary. We should never + /// wind up hitting the error conditions here. That's because in + /// order to reach this check, we must have done a successful authz + /// check. That check should have failed because there's no way to + /// grant users access to resources in other Silos. So why do this + /// check at all? As a belt-and-suspenders way to make sure we + /// never return objects to a user that are from a different Silo + /// than the one they're attached to. But what do we do if the + /// check fails? We definitely want to know about it so that we can + /// determine if there's an authz bug here, and if so, fix it. + /// That's why we log this at "error" level. We also override the + /// lookup return value with a suitable error indicating the + /// resource does not exist or the caller did not supply + /// credentials, just as if they didn't have access to the object. + // TODO-support These log messages should trigger support cases. + fn silo_check( + opctx: &OpContext, + authz_silo: &authz::Silo, + #resource_authz_name: &authz::#resource_name, + ) -> Result<(), Error> { + let log = &opctx.log; + let maybe_silo_id = opctx.authn.silo_required(); + if let Err(_) = &maybe_silo_id { + error!( + log, + "unexpected successful lookup of siloed resource\ + {:?} with no silo in OpContext", + #resource_name_str, + ); + }; + let actor_silo_id = maybe_silo_id?; + + let resource_silo_id = authz_silo.id(); + if resource_silo_id != actor_silo_id { + use crate::authz::ApiResourceError; + error!( + log, + "unexpected successful lookup of siloed resource\ + {:?} in a different Silo from current actor (resource\ + Silo {}, actor Silo {})", + #resource_name_str, + resource_silo_id, + actor_silo_id, + ); + Err(#resource_authz_name.not_found()) + } else { + Ok(()) + } + } + } + } else { + quote! {} + }; + quote! { /// Build the `authz` object for this resource fn make_authz( @@ -329,6 +397,8 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { #lookup_enum::PrimaryKey(root, ..) => root.lookup_root(), } } + + #silo_check_fn } } @@ -408,6 +478,39 @@ fn generate_lookup_methods(config: &Config) -> TokenStream { quote! {} }; + // If this resource is "Siloed", then tack on an extra check that the + // resource's Silo matches the Silo of the actor doing the fetch/lookup. + // See the generation of `silo_check()` for details. + let (silo_check_lookup, silo_check_fetch) = if config.siloed { + ( + quote! { + .and_then(|input| { + let ( + ref authz_silo, + .., + ref #resource_authz_name, + ) = &input; + Self::silo_check(opctx, authz_silo, #resource_authz_name)?; + Ok(input) + }) + }, + quote! { + .and_then(|input| { + let ( + ref authz_silo, + .., + ref #resource_authz_name, + ref _db_row, + ) = &input; + Self::silo_check(opctx, authz_silo, #resource_authz_name)?; + Ok(input) + }) + }, + ) + } else { + (quote! {}, quote! {}) + }; + quote! { /// Fetch the record corresponding to the selected resource /// @@ -449,6 +552,7 @@ fn generate_lookup_methods(config: &Config) -> TokenStream { ).await } } + #silo_check_fetch } /// Fetch an `authz` object for the selected resource and check @@ -469,6 +573,7 @@ fn generate_lookup_methods(config: &Config) -> TokenStream { let (#(#path_authz_names,)*) = self.lookup().await?; opctx.authorize(action, &#resource_authz_name).await?; Ok((#(#path_authz_names,)*)) + #silo_check_lookup } /// Fetch the "authz" objects for the selected resource and all its From ddd3cc5c701ff8246526d7eabbcd727f155605cf Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 18 Apr 2022 10:19:45 -0700 Subject: [PATCH 4/5] organization_create should accept and check silo cred --- nexus/src/authn/mod.rs | 12 ++++++-- nexus/src/authz/actor.rs | 38 +++++++++++++++--------- nexus/src/authz/api_resources.rs | 2 +- nexus/src/authz/omicron.polar | 2 ++ nexus/src/db/datastore.rs | 44 ++++++++++++++++------------ nexus/src/db/db-macros/src/lookup.rs | 6 ++-- nexus/src/db/lookup.rs | 6 ++-- nexus/src/nexus.rs | 5 +--- 8 files changed, 69 insertions(+), 46 deletions(-) diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index d3984a2174..3520a0b00c 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -36,6 +36,8 @@ pub use crate::db::fixed_data::user_builtin::USER_TEST_PRIVILEGED; pub use crate::db::fixed_data::user_builtin::USER_TEST_UNPRIVILEGED; use crate::db::model::ConsoleSession; +use crate::authz; +use omicron_common::api::external::LookupType; use serde::Deserialize; use serde::Serialize; use uuid::Uuid; @@ -81,8 +83,14 @@ impl Context { pub fn silo_required( &self, - ) -> Result { - self.actor_required().map(|actor| actor.silo_id) + ) -> Result { + self.actor_required().map(|actor| { + authz::Silo::new( + authz::FLEET, + actor.silo_id, + LookupType::ById(actor.silo_id), + ) + }) } /// Returns the list of schemes tried, in order diff --git a/nexus/src/authz/actor.rs b/nexus/src/authz/actor.rs index 572262c835..43fac5dd2a 100644 --- a/nexus/src/authz/actor.rs +++ b/nexus/src/authz/actor.rs @@ -8,24 +8,22 @@ use super::roles::RoleSet; use crate::authn; use omicron_common::api::external::ResourceType; use uuid::Uuid; +use omicron_common::api::external::LookupType; /// Represents [`authn::Context`] (which is either an authenticated or /// unauthenticated actor) for Polar #[derive(Clone, Debug)] pub struct AnyActor { authenticated: bool, - actor_id: Option, + actor_info: Option<(Uuid, Uuid)>, roles: RoleSet, } impl AnyActor { pub fn new(authn: &authn::Context, roles: RoleSet) -> Self { let actor = authn.actor(); - AnyActor { - authenticated: actor.is_some(), - actor_id: actor.map(|a| a.id), - roles, - } + let actor_info = actor.map(|a| (a.id, a.silo_id)); + AnyActor { authenticated: actor.is_some(), actor_info, roles } } } @@ -36,8 +34,9 @@ impl oso::PolarClass for AnyActor { a.authenticated }) .add_attribute_getter("authn_actor", |a: &AnyActor| { - a.actor_id.map(|actor_id| AuthenticatedActor { + a.actor_info.map(|(actor_id, silo_id)| AuthenticatedActor { actor_id, + silo_id, roles: a.roles.clone(), }) }) @@ -48,6 +47,7 @@ impl oso::PolarClass for AnyActor { #[derive(Clone, Debug)] pub struct AuthenticatedActor { actor_id: Uuid, + silo_id: Uuid, roles: RoleSet, } @@ -73,12 +73,22 @@ impl Eq for AuthenticatedActor {} impl oso::PolarClass for AuthenticatedActor { fn get_polar_class_builder() -> oso::ClassBuilder { - oso::Class::builder().with_equality_check().add_constant( - AuthenticatedActor { - actor_id: authn::USER_DB_INIT.id, - roles: RoleSet::new(), - }, - "USER_DB_INIT", - ) + oso::Class::builder() + .with_equality_check() + .add_constant( + AuthenticatedActor { + actor_id: authn::USER_DB_INIT.id, + silo_id: authn::USER_DB_INIT.silo_id, + roles: RoleSet::new(), + }, + "USER_DB_INIT", + ) + .add_attribute_getter("silo", |a: &AuthenticatedActor| { + super::Silo::new( + super::FLEET, + a.silo_id, + LookupType::ById(a.silo_id), + ) + }) } } diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index d97e9854f4..a1c6f1e114 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -371,7 +371,7 @@ authz_resource! { parent = "Fleet", primary_key = Uuid, roles_allowed = true, - polar_snippet = FleetChild, + polar_snippet = Custom, } authz_resource! { diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 2dd84a460f..633cada063 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -151,6 +151,8 @@ resource Silo { } has_relation(fleet: Fleet, "parent_fleet", silo: Silo) if silo.fleet = fleet; +has_role(actor: AuthenticatedActor, "viewer", silo: Silo) + if actor.silo = silo; resource Organization { permissions = [ diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 6d5441995d..9f2b20e34a 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -516,14 +516,15 @@ impl DataStore { pub async fn organization_create( &self, opctx: &OpContext, - organization: Organization, + organization: ¶ms::OrganizationCreate, ) -> CreateResult { - use db::schema::organization::dsl; - - opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?; + let authz_silo = opctx.authn.silo_required()?; + opctx.authorize(authz::Action::CreateChild, &authz_silo).await?; + use db::schema::organization::dsl; + let silo_id = authz_silo.id(); + let organization = Organization::new(organization.clone(), silo_id); let name = organization.name().as_str().to_string(); - let silo_id = organization.silo_id; Silo::insert_resource( silo_id, @@ -533,7 +534,11 @@ impl DataStore { .await .map_err(|e| match e { AsyncInsertError::CollectionNotFound => Error::InternalError { - internal_message: format!("attempting to create an organization under non-existent silo {}", silo_id), + internal_message: format!( + "attempting to create an \ + organization under non-existent silo {}", + silo_id + ), }, AsyncInsertError::DatabaseError(e) => { public_error_from_diesel_pool( @@ -603,10 +608,13 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { + let authz_silo = opctx.authn.silo_required()?; + opctx.authorize(authz::Action::ListChildren, &authz_silo).await?; + use db::schema::organization::dsl; - opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; paginated(dsl::organization, dsl::id, pagparams) .filter(dsl::time_deleted.is_null()) + .filter(dsl::silo_id.eq(authz_silo.id())) .select(Organization::as_select()) .load_async::(self.pool_authorized(opctx).await?) .await @@ -618,10 +626,13 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - use db::schema::organization::dsl; + let authz_silo = opctx.authn.silo_required()?; opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + + use db::schema::organization::dsl; paginated(dsl::organization, dsl::name, pagparams) .filter(dsl::time_deleted.is_null()) + .filter(dsl::silo_id.eq(authz_silo.id())) .select(Organization::as_select()) .load_async::(self.pool_authorized(opctx).await?) .await @@ -2727,7 +2738,7 @@ mod test { use crate::db::identity::Resource; use crate::db::lookup::LookupPath; use crate::db::model::{ - ConsoleSession, DatasetKind, Organization, Project, + ConsoleSession, DatasetKind, Project, }; use crate::external_api::params; use chrono::{Duration, Utc}; @@ -2746,18 +2757,15 @@ mod test { let logctx = dev::test_setup_log("test_project_creation"); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - let organization = Organization::new( - params::OrganizationCreate { - identity: IdentityMetadataCreateParams { - name: "org".parse().unwrap(), - description: "desc".to_string(), - }, + let organization = params::OrganizationCreate { + identity: IdentityMetadataCreateParams { + name: "org".parse().unwrap(), + description: "desc".to_string(), }, - *SILO_ID, - ); + }; let organization = - datastore.organization_create(&opctx, organization).await.unwrap(); + datastore.organization_create(&opctx, &organization).await.unwrap(); let project = Project::new( organization.id(), diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index 91a3be6611..af1b05743c 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -336,8 +336,8 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { #resource_authz_name: &authz::#resource_name, ) -> Result<(), Error> { let log = &opctx.log; - let maybe_silo_id = opctx.authn.silo_required(); - if let Err(_) = &maybe_silo_id { + let maybe_silo = opctx.authn.silo_required(); + if let Err(_) = &maybe_silo { error!( log, "unexpected successful lookup of siloed resource\ @@ -345,7 +345,7 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { #resource_name_str, ); }; - let actor_silo_id = maybe_silo_id?; + let actor_silo_id = maybe_silo?.id(); let resource_silo_id = authz_silo.id(); if resource_silo_id != actor_silo_id { diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 6a0c98ccd8..5860c1a64e 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -193,12 +193,10 @@ impl<'a> LookupPath<'a> { 'a: 'c, 'b: 'c, { - // XXX-dap XXX-dap The macro needs to be updated so that the by-id - // functions all check whether the resource exists in the right Silo let key = match self.opctx.authn.silo_required() { - Ok(silo_id) => { + Ok(authz_silo) => { let root = Root { lookup_root: self }; - let silo_key = SiloKey::PrimaryKey(root, silo_id); + let silo_key = SiloKey::PrimaryKey(root, authz_silo.id()); OrganizationKey::Name(Silo { key: silo_key }, name) } Err(error) => { diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 8b448ef266..c6edb4e775 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -748,10 +748,7 @@ impl Nexus { opctx: &OpContext, new_organization: ¶ms::OrganizationCreate, ) -> CreateResult { - let silo_id = opctx.authn.silo_required()?; - let db_org = - db::model::Organization::new(new_organization.clone(), silo_id); - self.db_datastore.organization_create(opctx, db_org).await + self.db_datastore.organization_create(opctx, new_organization).await } pub async fn organization_fetch( From 2e751f0df857624952d553bc66de50473bf4d9f4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 18 Apr 2022 11:14:49 -0700 Subject: [PATCH 5/5] rustfmt --- nexus/src/authz/actor.rs | 2 +- nexus/src/db/datastore.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/nexus/src/authz/actor.rs b/nexus/src/authz/actor.rs index 43fac5dd2a..d89152b5f3 100644 --- a/nexus/src/authz/actor.rs +++ b/nexus/src/authz/actor.rs @@ -6,9 +6,9 @@ use super::roles::RoleSet; use crate::authn; +use omicron_common::api::external::LookupType; use omicron_common::api::external::ResourceType; use uuid::Uuid; -use omicron_common::api::external::LookupType; /// Represents [`authn::Context`] (which is either an authenticated or /// unauthenticated actor) for Polar diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 9f2b20e34a..c3b32c125f 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2737,9 +2737,7 @@ mod test { use crate::db::explain::ExplainableAsync; use crate::db::identity::Resource; use crate::db::lookup::LookupPath; - use crate::db::model::{ - ConsoleSession, DatasetKind, Project, - }; + use crate::db::model::{ConsoleSession, DatasetKind, Project}; use crate::external_api::params; use chrono::{Duration, Utc}; use nexus_test_utils::db::test_setup_database;