From c1a3c4c170269a4e8b30717650e2a33710c29c0e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 31 Mar 2022 16:23:07 -0700 Subject: [PATCH 1/6] convert Instance-related lookups to new interface --- nexus/src/db/datastore.rs | 170 ++------------------- nexus/src/db/lookup.rs | 7 + nexus/src/external_api/http_entrypoints.rs | 2 +- nexus/src/nexus.rs | 156 ++++++++----------- nexus/src/sagas.rs | 37 +++-- 5 files changed, 108 insertions(+), 264 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index f28352a7f8e..af57457978d 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -980,81 +980,6 @@ impl DataStore { // Instances - /// Fetches an Instance from the database and returns both the database row - /// and an [`authz::Instance`] for doing authz checks - /// - /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases - /// and caveats. - // TODO-security See the note on organization_lookup_noauthz(). - async fn instance_lookup_noauthz( - &self, - authz_project: &authz::Project, - instance_name: &Name, - ) -> LookupResult<(authz::Instance, Instance)> { - use db::schema::instance::dsl; - dsl::instance - .filter(dsl::time_deleted.is_null()) - .filter(dsl::project_id.eq(authz_project.id())) - .filter(dsl::name.eq(instance_name.clone())) - .select(Instance::as_select()) - .first_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Instance, - LookupType::ByName(instance_name.as_str().to_owned()), - ), - ) - }) - .map(|d| { - ( - authz_project.child_generic( - ResourceType::Instance, - d.id(), - LookupType::from(&instance_name.0), - ), - d, - ) - }) - } - - /// Look up the id for an Instance based on its name - /// - /// Returns an [`authz::Instance`] (which makes the id available). - /// - /// Like the other "lookup_by_path()" functions, this function does no authz - /// checks. - // TODO-security See note on disk_lookup_by_path(). - pub async fn instance_lookup_by_path( - &self, - organization_name: &Name, - project_name: &Name, - instance_name: &Name, - ) -> LookupResult { - let authz_project = self - .project_lookup_by_path(organization_name, project_name) - .await?; - self.instance_lookup_noauthz(&authz_project, instance_name) - .await - .map(|(d, _)| d) - } - - /// Lookup an Instance by name and return the full database record, along - /// with an [`authz::Instance`] for subsequent authorization checks - pub async fn instance_fetch( - &self, - opctx: &OpContext, - authz_project: &authz::Project, - name: &Name, - ) -> LookupResult<(authz::Instance, Instance)> { - let (authz_instance, db_instance) = - self.instance_lookup_noauthz(authz_project, name).await?; - opctx.authorize(authz::Action::Read, &authz_instance).await?; - Ok((authz_instance, db_instance)) - } - /// Idempotently insert a database record for an Instance /// /// This is intended to be used by a saga action. When we say this is @@ -1626,94 +1551,35 @@ impl DataStore { // in other situations, such as moving an instance between VPC Subnets. pub async fn instance_delete_all_network_interfaces( &self, - instance_id: &Uuid, + opctx: &OpContext, + authz_instance: &authz::Instance, ) -> DeleteResult { + opctx.authorize(authz::Action::Modify, authz_instance).await?; + use db::schema::network_interface::dsl; let now = Utc::now(); diesel::update(dsl::network_interface) - .filter(dsl::instance_id.eq(*instance_id)) + .filter(dsl::instance_id.eq(authz_instance.id())) .filter(dsl::time_deleted.is_null()) .set(dsl::time_deleted.eq(now)) - .execute_async(self.pool()) + .execute_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Instance, - LookupType::ById(*instance_id), - ), + ErrorHandler::NotFoundByResource(authz_instance), ) })?; Ok(()) } - /// Fetches a `NetworkInterface` from the database and returns both the - /// database row and an [`authz::NetworkInterface`] for doing authz checks. - /// - /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases - /// and caveats. - // TODO-security See the note on organization_lookup_noauthz(). - async fn network_interface_lookup_noauthz( - &self, - authz_instance: &authz::Instance, - interface_name: &Name, - ) -> LookupResult<(authz::NetworkInterface, NetworkInterface)> { - use db::schema::network_interface::dsl; - dsl::network_interface - .filter(dsl::time_deleted.is_null()) - .filter(dsl::instance_id.eq(authz_instance.id())) - .filter(dsl::name.eq(interface_name.clone())) - .select(NetworkInterface::as_select()) - .first_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::NetworkInterface, - LookupType::ByName(interface_name.as_str().to_owned()), - ), - ) - }) - .map(|d| { - ( - authz_instance.child_generic( - ResourceType::NetworkInterface, - d.id(), - LookupType::from(&interface_name.0), - ), - d, - ) - }) - } - - /// Lookup a `NetworkInterface` by name and return the full database record, - /// along with an [`authz::NetworkInterface`] for subsequent authorization - /// checks. - pub async fn network_interface_fetch( - &self, - opctx: &OpContext, - authz_instance: &authz::Instance, - name: &Name, - ) -> LookupResult<(authz::NetworkInterface, NetworkInterface)> { - let (authz_interface, db_interface) = - self.network_interface_lookup_noauthz(authz_instance, name).await?; - opctx.authorize(authz::Action::Read, &authz_interface).await?; - Ok((authz_interface, db_interface)) - } - /// Delete a `NetworkInterface` attached to a provided instance. pub async fn instance_delete_network_interface( &self, opctx: &OpContext, - authz_instance: &authz::Instance, - interface_name: &Name, + authz_interface: &authz::NetworkInterface, ) -> DeleteResult { - let (authz_interface, _) = self - .network_interface_fetch(opctx, &authz_instance, interface_name) - .await?; - opctx.authorize(authz::Action::Delete, &authz_interface).await?; + opctx.authorize(authz::Action::Delete, authz_interface).await?; use db::schema::network_interface::dsl; let now = Utc::now(); @@ -1727,10 +1593,7 @@ impl DataStore { .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::NetworkInterface, - LookupType::ById(interface_id), - ), + ErrorHandler::NotFoundByResource(authz_interface), ) })?; Ok(()) @@ -1755,19 +1618,6 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Get a network interface by name attached to an instance - pub async fn instance_lookup_network_interface( - &self, - opctx: &OpContext, - authz_instance: &authz::Instance, - interface_name: &Name, - ) -> LookupResult { - Ok(self - .network_interface_fetch(opctx, &authz_instance, interface_name) - .await? - .1) - } - // Create a record for a new Oximeter instance pub async fn oximeter_create( &self, diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 9f312c0f0d8..64254b4f68e 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -271,6 +271,13 @@ lookup_resource! { lookup_resource! { name = "Instance", ancestors = [ "Organization", "Project" ], + children = [ "NetworkInterface" ], + authz_kind = Generic +} + +lookup_resource! { + name = "NetworkInterface", + ancestors = [ "Organization", "Project", "Instance" ], children = [], authz_kind = Generic } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 63381911e99..616ac3f2504 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1300,7 +1300,7 @@ async fn instance_network_interfaces_get_interface( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let interface = nexus - .instance_lookup_network_interface( + .network_interface_fetch( &opctx, organization_name, project_name, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index eb26921d14a..eae76f79137 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1027,13 +1027,11 @@ impl Nexus { // TODO-robustness We need to figure out what to do with Destroyed // instances? Presumably we need to clean them up at some point, but // not right away so that callers can see that they've been destroyed. - let authz_instance = self - .db_datastore - .instance_lookup_by_path( - organization_name, - project_name, - instance_name, - ) + let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .lookup_for(authz::Action::Delete) .await?; self.db_datastore.project_delete_instance(opctx, &authz_instance).await } @@ -1046,15 +1044,12 @@ impl Nexus { instance_name: &Name, params: params::InstanceMigrate, ) -> UpdateResult { - let authz_instance = self - .db_datastore - .instance_lookup_by_path( - organization_name, - project_name, - instance_name, - ) + let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .lookup_for(authz::Action::Modify) .await?; - opctx.authorize(authz::Action::Modify, &authz_instance).await?; // Kick off the migration saga let saga_params = Arc::new(sagas::ParamsInstanceMigrate { @@ -1082,15 +1077,13 @@ impl Nexus { project_name: &Name, instance_name: &Name, ) -> LookupResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., db_instance) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .fetch() .await?; - Ok(self - .db_datastore - .instance_fetch(opctx, &authz_project, instance_name) - .await? - .1) + Ok(db_instance) } fn check_runtime_change_allowed( @@ -1188,14 +1181,13 @@ impl Nexus { // even if the whole rack powered off while this was going on, we would // never lose track of the fact that this Instance was supposed to be // running. - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) - .await?; - let (authz_instance, db_instance) = self - .db_datastore - .instance_fetch(opctx, &authz_project, instance_name) - .await?; + let (.., authz_instance, db_instance) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .fetch() + .await?; let requested = InstanceRuntimeStateRequested { run_state: InstanceStateRequested::Reboot, migration_params: None, @@ -1218,14 +1210,13 @@ impl Nexus { project_name: &Name, instance_name: &Name, ) -> UpdateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) - .await?; - let (authz_instance, db_instance) = self - .db_datastore - .instance_fetch(opctx, &authz_project, instance_name) - .await?; + let (.., authz_instance, db_instance) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .fetch() + .await?; let requested = InstanceRuntimeStateRequested { run_state: InstanceStateRequested::Running, migration_params: None, @@ -1248,14 +1239,13 @@ impl Nexus { project_name: &Name, instance_name: &Name, ) -> UpdateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) - .await?; - let (authz_instance, db_instance) = self - .db_datastore - .instance_fetch(opctx, &authz_project, instance_name) - .await?; + let (.., authz_instance, db_instance) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .fetch() + .await?; let requested = InstanceRuntimeStateRequested { run_state: InstanceStateRequested::Stopped, migration_params: None, @@ -1410,13 +1400,11 @@ impl Nexus { instance_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_instance = self - .db_datastore - .instance_lookup_by_path( - organization_name, - project_name, - instance_name, - ) + let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .instance_list_disks(opctx, &authz_instance, pagparams) @@ -1707,13 +1695,11 @@ impl Nexus { instance_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_instance = self - .db_datastore - .instance_lookup_by_path( - organization_name, - project_name, - instance_name, - ) + let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .instance_list_network_interfaces(opctx, &authz_instance, pagparams) @@ -1800,15 +1786,18 @@ impl Nexus { instance_name: &Name, interface_name: &Name, ) -> DeleteResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) - .await?; - let (authz_instance, db_instance) = self - .db_datastore - .instance_fetch(opctx, &authz_project, instance_name) + let (.., authz_instance, db_instance) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .fetch_for(authz::Action::Modify) + .await?; + let (.., authz_interface) = LookupPath::new(opctx, &self.db_datastore) + .instance_id(authz_instance.id()) + .network_interface_name(interface_name) + .lookup_for(authz::Action::Delete) .await?; - opctx.authorize(authz::Action::Modify, &authz_instance).await?; // TODO-completeness: We'd like to relax this once hot-plug is supported let stopped = @@ -1819,16 +1808,12 @@ impl Nexus { )); } self.db_datastore - .instance_delete_network_interface( - opctx, - &authz_instance, - interface_name, - ) + .instance_delete_network_interface(opctx, &authz_interface) .await } /// Fetch a network interface attached to the given instance. - pub async fn instance_lookup_network_interface( + pub async fn network_interface_fetch( &self, opctx: &OpContext, organization_name: &Name, @@ -1836,21 +1821,14 @@ impl Nexus { instance_name: &Name, interface_name: &Name, ) -> LookupResult { - let authz_instance = self - .db_datastore - .instance_lookup_by_path( - organization_name, - project_name, - instance_name, - ) + let (.., db_interface) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .instance_name(instance_name) + .network_interface_name(interface_name) + .fetch() .await?; - self.db_datastore - .instance_lookup_network_interface( - opctx, - &authz_instance, - interface_name, - ) - .await + Ok(db_interface) } pub async fn project_list_vpcs( diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index 1daaa8384f1..cd1d3c3ffa0 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -432,13 +432,14 @@ async fn sic_create_custom_network_interfaces( // Refetch the interface itself, to serialize it for the next // saga node. - datastore - .instance_lookup_network_interface( - &opctx, - &authz_instance, - &db::model::Name(params.identity.name.clone()), - ) + LookupPath::new(&opctx, &datastore) + .instance_id(authz_instance.id()) + .network_interface_name(&db::model::Name( + params.identity.name.clone(), + )) + .fetch() .await + .map(|(.., db_interface)| db_interface) } Err(e) => Err(e.into_external()), } @@ -516,16 +517,24 @@ async fn sic_create_default_network_interface( async fn sic_create_network_interfaces_undo( sagactx: ActionContext, ) -> Result<(), anyhow::Error> { - // We issue a request to delete any interfaces associated with this instance. - // In the case we failed partway through allocating interfaces, we won't - // have cached the interface records in the saga log, but they're definitely - // still in the database. Just delete every interface that exists, even if - // there are zero such records. + // We issue a request to delete any interfaces associated with this + // instance. In the case we failed partway through allocating interfaces, + // we won't have cached the interface records in the saga log, but they're + // definitely still in the database. Just delete every interface that + // exists, even if there are zero such records. let osagactx = sagactx.user_data(); + let datastore = osagactx.datastore(); + let saga_params = sagactx.saga_params(); + let opctx = + OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); let instance_id = sagactx.lookup::("instance_id")?; - osagactx - .datastore() - .instance_delete_all_network_interfaces(&instance_id) + let (.., authz_instance) = LookupPath::new(&opctx, &datastore) + .instance_id(instance_id) + .lookup_for(authz::Action::Modify) + .await + .map_err(ActionError::action_failed)?; + datastore + .instance_delete_all_network_interfaces(&opctx, &authz_instance) .await .map_err(ActionError::action_failed)?; Ok(()) From 3ef853027b09254a54960d5d0c5204874a426bd7 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 31 Mar 2022 16:37:28 -0700 Subject: [PATCH 2/6] convert disk lookups to new API --- nexus/src/db/datastore.rs | 95 ++++----------------------------------- nexus/src/nexus.rs | 30 +++++-------- 2 files changed, 19 insertions(+), 106 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index af57457978d..c80d5327d28 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1183,85 +1183,6 @@ impl DataStore { // Disks - /// Fetches a Disk from the database and returns both the database row - /// and an [`authz::Disk`] for doing authz checks - /// - /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases - /// and caveats. - // TODO-security See the note on organization_lookup_noauthz(). - async fn disk_lookup_noauthz( - &self, - authz_project: &authz::Project, - disk_name: &Name, - ) -> LookupResult<(authz::Disk, Disk)> { - use db::schema::disk::dsl; - dsl::disk - .filter(dsl::time_deleted.is_null()) - .filter(dsl::project_id.eq(authz_project.id())) - .filter(dsl::name.eq(disk_name.clone())) - .select(Disk::as_select()) - .first_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Disk, - LookupType::ByName(disk_name.as_str().to_owned()), - ), - ) - }) - .map(|d| { - ( - authz_project.child_generic( - ResourceType::Disk, - d.id(), - LookupType::from(&disk_name.0), - ), - d, - ) - }) - } - - /// Look up the id for a Disk based on its name - /// - /// Returns an [`authz::Disk`] (which makes the id available). - /// - /// Like the other "lookup_by_path()" functions, this function does no authz - /// checks. - // TODO-security For Containers in the hierarchy (like Organizations and - // Projects), we don't do an authz check in the "lookup_by_path" functions - // because we don't know if the caller has access to do the lookup. For - // leaf resources (like Instances and Disks), though, we do. We could do - // the authz check here. Should we? - pub async fn disk_lookup_by_path( - &self, - organization_name: &Name, - project_name: &Name, - disk_name: &Name, - ) -> LookupResult { - let authz_project = self - .project_lookup_by_path(organization_name, project_name) - .await?; - self.disk_lookup_noauthz(&authz_project, disk_name) - .await - .map(|(d, _)| d) - } - - /// Lookup a Disk by name and return the full database record, along with - /// an [`authz::Disk`] for subsequent authorization checks - pub async fn disk_fetch( - &self, - opctx: &OpContext, - authz_project: &authz::Project, - name: &Name, - ) -> LookupResult<(authz::Disk, Disk)> { - let (authz_disk, db_disk) = - self.disk_lookup_noauthz(authz_project, name).await?; - opctx.authorize(authz::Action::Read, &authz_disk).await?; - Ok((authz_disk, db_disk)) - } - /// List disks associated with a given instance. pub async fn instance_list_disks( &self, @@ -1376,14 +1297,14 @@ impl DataStore { /// Fetches information about a Disk that the caller has previously fetched /// - /// The principal difference from `disk_fetch` is that this function takes - /// an `authz_disk` and does a lookup by id rather than the full path of - /// names (organization name, project name, and disk name). This could be - /// called `disk_lookup_by_id`, except that you might expect to get back an - /// `authz::Disk` as well. We cannot return you a new `authz::Disk` because - /// we don't know how you looked up the Disk in the first place. However, - /// you must have previously looked it up, which is why we call this - /// `refetch`. + /// The principal difference from fetching a disk is that this function + /// takes an `authz_disk` and does a lookup by id rather than the full path + /// of names (organization name, project name, and disk name). This could + /// be called `disk_lookup_by_id`, except that you might expect to get back + /// an `authz::Disk` as well. We cannot return you a new `authz::Disk` + /// because we don't know how you looked up the Disk in the first place. + /// However, you must have previously looked it up, which is why we call + /// this `refetch`. pub async fn disk_refetch( &self, opctx: &OpContext, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index eae76f79137..dce26ecc575 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -791,15 +791,13 @@ impl Nexus { project_name: &Name, disk_name: &Name, ) -> LookupResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., db_disk) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .disk_name(disk_name) + .fetch() .await?; - Ok(self - .db_datastore - .disk_fetch(opctx, &authz_project, disk_name) - .await? - .1) + Ok(db_disk) } pub async fn project_delete_disk( @@ -809,19 +807,13 @@ impl Nexus { project_name: &Name, disk_name: &Name, ) -> DeleteResult { - let authz_disk = self - .db_datastore - .disk_lookup_by_path(organization_name, project_name, disk_name) + let (.., authz_disk) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .disk_name(disk_name) + .lookup_for(authz::Action::Delete) .await?; - // TODO: We need to sort out the authorization checks. - // - // Normally, this would be coupled alongside access to the - // datastore, but now that disk deletion exists within a Saga, - // this would require OpContext to be serialized (which is - // not trivial). - opctx.authorize(authz::Action::Delete, &authz_disk).await?; - let saga_params = Arc::new(sagas::ParamsDiskDelete { disk_id: authz_disk.id() }); self.execute_saga( From de633eed09cae222609aa82478513f94606ad5d0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 31 Mar 2022 16:55:30 -0700 Subject: [PATCH 3/6] convert project and org lookups to new API --- nexus/src/db/datastore.rs | 217 ++++---------------------------------- nexus/src/nexus.rs | 107 +++++++++++-------- 2 files changed, 81 insertions(+), 243 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c80d5327d28..c6a15a0c72a 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -562,134 +562,26 @@ impl DataStore { }) } - /// Fetches an Organization from the database and returns both the database - /// row and an authz::Organization for doing authz checks - /// - /// There are a few different ways this can be used: - /// - /// * If a code path wants to use anything in the database row _aside_ from - /// the id, it should do an authz check for `authz::Action::Read` on the - /// returned [`authz::Organization`]. `organization_fetch()` does this, - /// for example. - /// * If a code path is only doing this lookup to get the id so that it can - /// look up something else inside the Organization, then the database - /// record is not required -- and neither is an authz check on the - /// Organization. Callers usually use `organization_lookup_id()` for - /// this. That function does not expose the database row to the caller. - /// - /// Callers in this bucket should still do _some_ authz check. Clients - /// must not be able to discover whether an Organization exists with a - /// particular name. It's just that we don't know at this point whether - /// they're allowed to see the Organization. It depends on whether, as we - /// look up things inside the Organization, we find something that they - /// _are_ able to see. - /// - /// **This is an internal-only function.** This function cannot know what - /// authz checks are required. As a result, it should not be made - /// accessible outside the DataStore. It should always be wrapped by - /// something that does the appropriate authz check. - // TODO-security We should refactor things so that it's harder to - // accidentally mark this "pub" or otherwise expose database data without - // doing an authz check. - async fn organization_lookup_noauthz( - &self, - name: &Name, - ) -> LookupResult<(authz::Organization, Organization)> { - use db::schema::organization::dsl; - - // TODO-security uncomment when all endpoints are protected by authn - //let silo_id = opctx.authn.silo_required()?; - let silo_id = *SILO_ID; - - dsl::organization - .filter(dsl::time_deleted.is_null()) - .filter(dsl::name.eq(name.clone())) - .filter(dsl::silo_id.eq(silo_id)) - .select(Organization::as_select()) - .get_result_async::(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Organization, - LookupType::ByName(name.as_str().to_owned()), - ), - ) - }) - .map(|o| { - ( - authz::FLEET - .organization(o.id(), LookupType::from(&name.0)), - o, - ) - }) - } - - /// Look up the id for an organization based on its name - /// - /// Returns an [`authz::Organization`] (which makes the id available). - /// - /// This function does no authz checks because it is not possible to know - /// just by looking up an Organization's id what privileges are required. - pub async fn organization_lookup_by_path( - &self, - name: &Name, - ) -> LookupResult { - self.organization_lookup_noauthz(name).await.map(|(o, _)| o) - } - - /// Lookup an organization by name. - pub async fn organization_fetch( - &self, - opctx: &OpContext, - name: &Name, - ) -> LookupResult<(authz::Organization, Organization)> { - let (authz_org, db_org) = - self.organization_lookup_noauthz(name).await?; - opctx.authorize(authz::Action::Read, &authz_org).await?; - Ok((authz_org, db_org)) - } - /// Delete a organization pub async fn organization_delete( &self, opctx: &OpContext, - name: &Name, + authz_org: &authz::Organization, + db_org: &db::model::Organization, ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_org).await?; + use db::schema::organization::dsl; use db::schema::project; - let (id, rcgen) = dsl::organization - .filter(dsl::time_deleted.is_null()) - .filter(dsl::name.eq(name.clone())) - .select((dsl::id, dsl::rcgen)) - .get_result_async::<(Uuid, Generation)>(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Organization, - LookupType::ByName(name.as_str().to_owned()), - ), - ) - })?; - - // TODO-cleanup TODO-security This should use a more common lookup - // function. - let authz_org = - authz::FLEET.organization(id, LookupType::from(&name.0)); - opctx.authorize(authz::Action::Delete, &authz_org).await?; - // Make sure there are no projects present within this organization. let project_found = diesel_pool_result_optional( project::dsl::project - .filter(project::dsl::organization_id.eq(id)) + .filter(project::dsl::organization_id.eq(authz_org.id())) .filter(project::dsl::time_deleted.is_null()) .select(project::dsl::id) .limit(1) - .first_async::(self.pool()) + .first_async::(self.pool_authorized(opctx).await?) .await, ) .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?; @@ -703,18 +595,15 @@ impl DataStore { let now = Utc::now(); let updated_rows = diesel::update(dsl::organization) .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(id)) - .filter(dsl::rcgen.eq(rcgen)) + .filter(dsl::id.eq(authz_org.id())) + .filter(dsl::rcgen.eq(db_org.rcgen)) .set(dsl::time_deleted.eq(now)) - .execute_async(self.pool()) + .execute_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Organization, - LookupType::ById(id), - ), + ErrorHandler::NotFoundByResource(authz_org), ) })?; @@ -761,14 +650,12 @@ impl DataStore { pub async fn organization_update( &self, opctx: &OpContext, - name: &Name, + authz_org: &authz::Organization, updates: OrganizationUpdate, ) -> UpdateResult { use db::schema::organization::dsl; - let authz_org = self.organization_lookup_by_path(name).await?; - opctx.authorize(authz::Action::Modify, &authz_org).await?; - + opctx.authorize(authz::Action::Modify, authz_org).await?; diesel::update(dsl::organization) .filter(dsl::time_deleted.is_null()) .filter(dsl::id.eq(authz_org.id())) @@ -779,7 +666,7 @@ impl DataStore { .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByResource(&authz_org), + ErrorHandler::NotFoundByResource(authz_org), ) }) } @@ -817,74 +704,6 @@ impl DataStore { }) } - /// Fetches a Project from the database and returns both the database row - /// and an authz::Project for doing authz checks - /// - /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases - /// and caveats. - // TODO-security See the note on organization_lookup_noauthz(). - async fn project_lookup_noauthz( - &self, - authz_org: &authz::Organization, - project_name: &Name, - ) -> LookupResult<(authz::Project, Project)> { - use db::schema::project::dsl; - dsl::project - .filter(dsl::time_deleted.is_null()) - .filter(dsl::organization_id.eq(authz_org.id())) - .filter(dsl::name.eq(project_name.clone())) - .select(Project::as_select()) - .first_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Project, - LookupType::ByName(project_name.as_str().to_owned()), - ), - ) - }) - .map(|p| { - ( - authz_org - .project(p.id(), LookupType::from(&project_name.0)), - p, - ) - }) - } - - /// Look up the id for a Project based on its name - /// - /// Returns an [`authz::Project`] (which makes the id available). - /// - /// This function does no authz checks because it is not possible to know - /// just by looking up an Project's id what privileges are required. - pub async fn project_lookup_by_path( - &self, - organization_name: &Name, - project_name: &Name, - ) -> LookupResult { - let authz_org = - self.organization_lookup_by_path(organization_name).await?; - self.project_lookup_noauthz(&authz_org, project_name) - .await - .map(|(p, _)| p) - } - - /// Lookup a project by name. - pub async fn project_fetch( - &self, - opctx: &OpContext, - authz_org: &authz::Organization, - name: &Name, - ) -> LookupResult<(authz::Project, Project)> { - let (authz_project, db_project) = - self.project_lookup_noauthz(authz_org, name).await?; - opctx.authorize(authz::Action::Read, &authz_project).await?; - Ok((authz_project, db_project)) - } - /// Delete a project // TODO-correctness This needs to check whether there are any resources that // depend on the Project (Disks, Instances). We can do this with a @@ -3132,10 +2951,12 @@ mod test { ); datastore.project_create(&opctx, &org, project).await.unwrap(); - let (_, organization_after_project_create) = datastore - .organization_fetch(&opctx, organization.name()) - .await - .unwrap(); + let (.., organization_after_project_create) = + LookupPath::new(&opctx, &datastore) + .organization_name(organization.name()) + .fetch() + .await + .unwrap(); assert!(organization_after_project_create.rcgen > organization.rcgen); db.cleanup().await.unwrap(); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index dce26ecc575..f334cf4712b 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -555,9 +555,13 @@ impl Nexus { pub async fn organization_fetch( &self, opctx: &OpContext, - name: &Name, + organization_name: &Name, ) -> LookupResult { - Ok(self.db_datastore.organization_fetch(opctx, name).await?.1) + let (.., db_organization) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .fetch() + .await?; + Ok(db_organization) } pub async fn organizations_list_by_name( @@ -579,19 +583,33 @@ impl Nexus { pub async fn organization_delete( &self, opctx: &OpContext, - name: &Name, + organization_name: &Name, ) -> DeleteResult { - self.db_datastore.organization_delete(opctx, name).await + let (.., authz_org, db_org) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .fetch() + .await?; + self.db_datastore.organization_delete(opctx, &authz_org, &db_org).await } pub async fn organization_update( &self, opctx: &OpContext, - name: &Name, + organization_name: &Name, new_params: ¶ms::OrganizationUpdate, ) -> UpdateResult { + let (.., authz_organization) = + LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .lookup_for(authz::Action::Modify) + .await?; self.db_datastore - .organization_update(opctx, name, new_params.clone().into()) + .organization_update( + opctx, + &authz_organization, + new_params.clone().into(), + ) .await } @@ -603,7 +621,7 @@ impl Nexus { organization_name: &Name, new_project: ¶ms::ProjectCreate, ) -> CreateResult { - 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?; @@ -653,12 +671,12 @@ impl Nexus { organization_name: &Name, project_name: &Name, ) -> LookupResult { - Ok(LookupPath::new(opctx, &self.db_datastore) + let (.., db_project) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .project_name(project_name) .fetch() - .await? - .2) + .await?; + Ok(db_project) } pub async fn projects_list_by_name( @@ -697,9 +715,10 @@ impl Nexus { organization_name: &Name, project_name: &Name, ) -> DeleteResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::Delete) .await?; self.db_datastore.project_delete(opctx, &authz_project).await } @@ -711,9 +730,10 @@ impl Nexus { project_name: &Name, new_params: ¶ms::ProjectUpdate, ) -> UpdateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::Modify) .await?; self.db_datastore .project_update(opctx, &authz_project, new_params.clone().into()) @@ -729,9 +749,10 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .project_list_disks(opctx, &authz_project, pagparams) @@ -745,16 +766,12 @@ impl Nexus { project_name: &Name, params: ¶ms::DiskCreate, ) -> CreateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::CreateChild) .await?; - // TODO-security This may need to be revisited once we implement authz - // checks for saga actions. Even then, though, it still will be correct - // (if possibly redundant) to check this here. - opctx.authorize(authz::Action::CreateChild, &authz_project).await?; - // Until we implement snapshots, do not allow disks to be created with a // snapshot id. if params.snapshot_id.is_some() { @@ -905,9 +922,10 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) .await?; self.db_datastore .project_list_instances(opctx, &authz_project, pagparams) @@ -921,13 +939,12 @@ impl Nexus { project_name: &Name, params: ¶ms::InstanceCreate, ) -> CreateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::CreateChild) .await?; - opctx.authorize(authz::Action::CreateChild, &authz_project).await?; - // Validate parameters if params.disks.len() > MAX_DISKS_PER_INSTANCE as usize { return Err(Error::invalid_request(&format!( @@ -1830,15 +1847,14 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) .await?; - let vpcs = self - .db_datastore + self.db_datastore .project_list_vpcs(&opctx, &authz_project, pagparams) - .await?; - Ok(vpcs) + .await } pub async fn project_create_vpc( @@ -1848,9 +1864,10 @@ impl Nexus { project_name: &Name, params: ¶ms::VpcCreate, ) -> CreateResult { - let authz_project = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) + let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::CreateChild) .await?; let vpc_id = Uuid::new_v4(); let system_router_id = Uuid::new_v4(); From 9a029e763fc72df3acfacb8e38fb0f8be023012c Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 31 Mar 2022 18:57:02 -0700 Subject: [PATCH 4/6] fix test --- nexus/src/db/datastore.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c6a15a0c72a..8b569d5322c 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2903,6 +2903,7 @@ mod test { use crate::authz; use crate::db::explain::ExplainableAsync; use crate::db::identity::Resource; + use crate::db::lookup::LookupPath; use crate::db::model::{ ConsoleSession, DatasetKind, Organization, Project, }; From 90247381339956bfad21eeaba342fd13990946e8 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 31 Mar 2022 21:09:38 -0700 Subject: [PATCH 5/6] misc small cleanups --- nexus/src/db/datastore.rs | 91 +++++++++++++-------------------------- 1 file changed, 31 insertions(+), 60 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 8b569d5322c..fcbc4560127 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -27,6 +27,7 @@ use super::pool::DbConnection; use super::Pool; use crate::authn; use crate::authz; +use crate::authz::ApiResourceError; use crate::context::OpContext; use crate::db::fixed_data::role_assignment_builtin::BUILTIN_ROLE_ASSIGNMENTS; use crate::db::fixed_data::role_builtin::BUILTIN_ROLES; @@ -58,6 +59,7 @@ use std::convert::{TryFrom, TryInto}; use std::sync::Arc; use uuid::Uuid; +use crate::db::lookup::LookupPath; use crate::db::{ self, error::{public_error_from_diesel_pool, ErrorHandler, TransactionError}, @@ -886,22 +888,18 @@ impl DataStore { opctx: &OpContext, authz_instance: &authz::Instance, ) -> LookupResult { - use db::schema::instance::dsl; - - opctx.authorize(authz::Action::Read, authz_instance).await?; - - dsl::instance - .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(authz_instance.id())) - .select(Instance::as_select()) - .get_result_async(self.pool_authorized(opctx).await?) + let (.., db_instance) = LookupPath::new(opctx, self) + .instance_id(authz_instance.id()) + .fetch() .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByResource(authz_instance), - ) - }) + .map_err(|e| match e { + // Use the "not found" message of the authz object we were + // given, which will reflect however the caller originally + // looked it up. + Error::ObjectNotFound { .. } => authz_instance.not_found(), + e => e, + })?; + Ok(db_instance) } // TODO-design It's tempting to return the updated state of the Instance @@ -1116,35 +1114,29 @@ impl DataStore { /// Fetches information about a Disk that the caller has previously fetched /// - /// The principal difference from fetching a disk is that this function - /// takes an `authz_disk` and does a lookup by id rather than the full path - /// of names (organization name, project name, and disk name). This could - /// be called `disk_lookup_by_id`, except that you might expect to get back - /// an `authz::Disk` as well. We cannot return you a new `authz::Disk` - /// because we don't know how you looked up the Disk in the first place. - /// However, you must have previously looked it up, which is why we call - /// this `refetch`. + /// The only difference between this function and a new fetch by id is that + /// this function preserves the `authz_disk` that you started with -- which + /// keeps track of how you looked it up. So if you looked it up by name, + /// the authz you get back will reflect that, whereas if you did a fresh + /// lookup by id, it wouldn't. + /// TODO-cleanup this could be provided by the Lookup API for any resource pub async fn disk_refetch( &self, opctx: &OpContext, authz_disk: &authz::Disk, ) -> LookupResult { - use db::schema::disk::dsl; - - opctx.authorize(authz::Action::Read, authz_disk).await?; - - dsl::disk - .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(authz_disk.id())) - .select(Disk::as_select()) - .get_result_async(self.pool_authorized(opctx).await?) + let (.., db_disk) = LookupPath::new(opctx, self) + .disk_id(authz_disk.id()) + .fetch() .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByResource(authz_disk), - ) - }) + .map_err(|e| match e { + // Use the "not found" message of the authz object we were + // given, which will reflect however the caller originally + // looked it up. + Error::ObjectNotFound { .. } => authz_disk.not_found(), + e => e, + })?; + Ok(db_disk) } /// Updates a disk record to indicate it has been deleted. @@ -1392,27 +1384,6 @@ impl DataStore { Ok(()) } - // Fetch a record for an Oximeter instance, by its ID. - pub async fn oximeter_fetch( - &self, - id: Uuid, - ) -> Result { - use db::schema::oximeter::dsl; - dsl::oximeter - .filter(dsl::id.eq(id)) - .first_async::(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Oximeter, - LookupType::ById(id), - ), - ) - }) - } - // List the oximeter collector instances pub async fn oximeter_list( &self, @@ -2742,7 +2713,7 @@ impl DataStore { self.silo_lookup_noauthz(name).await } - pub async fn silo_lookup_noauthz( + async fn silo_lookup_noauthz( &self, // XXX enable when all endpoints are protected by authn // opctx: &OpContext, From ed44365f5720836f255429d7b718b56ecec8c832 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 1 Apr 2022 08:43:16 -0700 Subject: [PATCH 6/6] try to trigger GitHub actions