From 91415a4571c916c88c9432fc5ad7620722191afe Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 16 Dec 2022 11:20:30 -0500 Subject: [PATCH] improve authz --- nexus/src/app/sagas/instance_create.rs | 2 +- nexus/src/db/datastore/external_ip.rs | 25 +++++-------- nexus/src/db/datastore/ip_pool.rs | 49 +++++++++++++++----------- nexus/src/db/queries/external_ip.rs | 5 ++- 4 files changed, 42 insertions(+), 39 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 7ef063c515..d1879e2141 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -628,7 +628,7 @@ async fn sic_allocate_instance_snat_ip( let ip_id = sagactx.lookup::("snat_ip_id")?; let (.., pool) = datastore - .ip_pools_lookup_by_name_no_auth(&opctx, "default") + .ip_pools_fetch_default_for(&opctx, authz::Action::CreateChild) .await .map_err(ActionError::action_failed)?; let pool_id = pool.identity.id; diff --git a/nexus/src/db/datastore/external_ip.rs b/nexus/src/db/datastore/external_ip.rs index b46ee33ac7..c7afdc8c06 100644 --- a/nexus/src/db/datastore/external_ip.rs +++ b/nexus/src/db/datastore/external_ip.rs @@ -5,6 +5,7 @@ //! [`DataStore`] methods on [`ExternalIp`]s. use super::DataStore; +use crate::authz; use crate::context::OpContext; use crate::db; use crate::db::error::public_error_from_diesel_pool; @@ -23,6 +24,8 @@ use nexus_types::identity::Resource; use omicron_common::api::external::CreateResult; use omicron_common::api::external::Error; use omicron_common::api::external::LookupResult; +use omicron_common::api::external::Name as ExternalName; +use std::str::FromStr; use uuid::Uuid; impl DataStore { @@ -50,22 +53,12 @@ impl DataStore { instance_id: Uuid, pool_name: Option, ) -> CreateResult { - let name = if let Some(name) = pool_name { - name.as_str().to_string() - } else { - "default".to_string() - }; - - // We'd like to add authz checks here, and use the `LookupPath` - // methods on the project-scoped view of this resource. It's not - // entirely clear how that'll work in the API, so see RFD 288 and - // https://github.com/oxidecomputer/omicron/issues/1470 for more - // details. - // - // For now, we just ensure that the pool is either unreserved, or - // reserved for the instance's project. - let (.., pool) = - self.ip_pools_lookup_by_name_no_auth(opctx, &name).await?; + let name = pool_name.unwrap_or_else(|| { + Name(ExternalName::from_str("default").unwrap()) + }); + let (.., pool) = self + .ip_pools_fetch_for(opctx, authz::Action::CreateChild, &name) + .await?; let pool_id = pool.identity.id; let data = diff --git a/nexus/src/db/datastore/ip_pool.rs b/nexus/src/db/datastore/ip_pool.rs index 4fb28ee9f9..c4504ed330 100644 --- a/nexus/src/db/datastore/ip_pool.rs +++ b/nexus/src/db/datastore/ip_pool.rs @@ -6,6 +6,7 @@ use super::DataStore; use crate::authz; +use crate::authz::ApiResource; use crate::context::OpContext; use crate::db; use crate::db::collection_insert::AsyncInsertError; @@ -14,6 +15,7 @@ use crate::db::error::diesel_pool_result_optional; use crate::db::error::public_error_from_diesel_pool; use crate::db::error::ErrorHandler; use crate::db::identity::Resource; +use crate::db::lookup::LookupPath; use crate::db::model::IpPool; use crate::db::model::IpPoolRange; use crate::db::model::IpPoolUpdate; @@ -33,8 +35,10 @@ use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; +use omicron_common::api::external::Name as ExternalName; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; +use std::str::FromStr; use uuid::Uuid; impl DataStore { @@ -76,32 +80,35 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } + /// Looks up the default IP pool by name. + pub(crate) async fn ip_pools_fetch_default_for( + &self, + opctx: &OpContext, + action: authz::Action, + ) -> LookupResult<(authz::IpPool, IpPool)> { + self.ip_pools_fetch_for( + opctx, + action, + &Name(ExternalName::from_str("default").unwrap()), + ) + .await + } + /// Looks up an IP pool by name. - pub(crate) async fn ip_pools_lookup_by_name_no_auth( + pub(crate) async fn ip_pools_fetch_for( &self, opctx: &OpContext, - name: &str, + action: authz::Action, + name: &Name, ) -> LookupResult<(authz::IpPool, IpPool)> { - use db::schema::ip_pool::dsl; + let (.., authz_pool, pool) = LookupPath::new(opctx, &self) + .ip_pool_name(&name) + .fetch_for(action) + .await?; + if pool.internal { + return Err(authz_pool.not_found()); + } - let (authz_pool, pool) = dsl::ip_pool - .filter(dsl::internal.eq(false)) - .filter(dsl::time_deleted.is_null()) - .filter(dsl::name.eq(name.to_string())) - .select(IpPool::as_select()) - .get_result_async(self.pool_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) - .map(|ip_pool| { - ( - authz::IpPool::new( - authz::FLEET, - ip_pool.id(), - LookupType::ByName(name.to_string()), - ), - ip_pool, - ) - })?; Ok((authz_pool, pool)) } diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index 3c20f7772d..9a284967ce 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -673,7 +673,10 @@ mod tests { async fn default_pool_id(&self) -> Uuid { let (.., pool) = self .db_datastore - .ip_pools_lookup_by_name_no_auth(&self.opctx, "default") + .ip_pools_fetch_default_for( + &self.opctx, + crate::authz::Action::ListChildren, + ) .await .expect("Failed to lookup default ip pool"); pool.identity.id