From 1d8f51ba9670dd28e919187b5dfa1ef7ce8925b4 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 16 Mar 2022 19:25:52 +0000 Subject: [PATCH 1/2] Move network interface authz to the data store - Adds a module-private function for actually inserting the database record, after performing authz checks. This is used in the publicly-available method and in tests. - Adds authz objects to the `DataStore::instance_create_network_interface` method and does authz checks inside them. - Reorders the instance-creation saga. This moves the instance DB record creation before the NIC creation, since the latter can only be attached to an existing instance record. This also allows uniform authz checks inside the `DataStore` method, which wouldn't be possible if the instance record were not yet in the database. Note that this also requires a small change to the data the instance-record-creation saga node serializes. It previously contained the NICs, but these are no longer available at that time. Instead, the NICs are deserialized from the saga node that creates them and used to instantiate the instance runtime object only inside the `sic_instance_ensure` saga node. - Moves authz check for listing NICs for an instance into `DataStore` method - Moves authz check for fetching a single NIC for an instance into the `DataStore` method - Adds the `network_interface_fetch` method, for returning an authz interface and the database record, after checking read access. This uses a `*_noauthz` method as well, both of which are analogous to the other similarly-named methods. Note there's no lookup by ID or path at this point, since they're not really needed yet. - Moves the check for deleting an interface into the `DataStore` method. - Changes how deletion of a previously-deleted NIC works. We used to return a success, but we now return a not-found error, in line with the rest of the API. --- nexus/src/authz/api_resources.rs | 1 + nexus/src/authz/mod.rs | 1 + nexus/src/db/datastore.rs | 160 +++++++++++++++++++----------- nexus/src/db/subnet_allocation.rs | 41 +++++--- nexus/src/nexus.rs | 42 ++++---- nexus/src/sagas.rs | 71 ++++++++----- 6 files changed, 197 insertions(+), 119 deletions(-) diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index d336ec48736..8617d4458ce 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -515,3 +515,4 @@ pub type Disk = ProjectChild; pub type Instance = ProjectChild; pub type Vpc = ProjectChild; pub type VpcSubnet = ProjectChild; +pub type NetworkInterface = ProjectChild; diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 27b50a1a32b..4f2ec6accc7 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -167,6 +167,7 @@ pub use api_resources::Disk; pub use api_resources::Fleet; pub use api_resources::FleetChild; pub use api_resources::Instance; +pub use api_resources::NetworkInterface; pub use api_resources::Organization; pub use api_resources::Project; pub use api_resources::Vpc; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 9ff2dd026fd..b30979b2127 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1676,8 +1676,29 @@ impl DataStore { } // Network interfaces + + /// Create a network interface attached to the provided instance. pub async fn instance_create_network_interface( &self, + opctx: &OpContext, + authz_subnet: &authz::VpcSubnet, + authz_instance: &authz::Instance, + interface: IncompleteNetworkInterface, + ) -> Result { + opctx + .authorize(authz::Action::Modify, authz_instance) + .await + .map_err(NetworkInterfaceError::External)?; + opctx + .authorize(authz::Action::CreateChild, authz_subnet) + .await + .map_err(NetworkInterfaceError::External)?; + self.instance_create_network_interface_raw(&opctx, interface).await + } + + pub(super) async fn instance_create_network_interface_raw( + &self, + opctx: &OpContext, interface: IncompleteNetworkInterface, ) -> Result { use db::schema::network_interface::dsl; @@ -1688,7 +1709,11 @@ impl DataStore { diesel::insert_into(dsl::network_interface) .values(query) .returning(NetworkInterface::as_returning()) - .get_result_async(self.pool()) + .get_result_async( + self.pool_authorized(opctx) + .await + .map_err(NetworkInterfaceError::External)?, + ) .await .map_err(|e| NetworkInterfaceError::from_pool(e, &interface)) } @@ -1720,82 +1745,111 @@ impl DataStore { Ok(()) } - pub async fn instance_delete_network_interface( + /// 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, - interface_id: &Uuid, - ) -> DeleteResult { + authz_instance: &authz::Instance, + interface_name: &Name, + ) -> LookupResult<(authz::NetworkInterface, NetworkInterface)> { use db::schema::network_interface::dsl; - let now = Utc::now(); - let result = diesel::update(dsl::network_interface) - .filter(dsl::id.eq(*interface_id)) + dsl::network_interface .filter(dsl::time_deleted.is_null()) - .set((dsl::time_deleted.eq(now),)) - .check_if_exists::(*interface_id) - .execute_and_check(self.pool()) + .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::ById(*interface_id), + LookupType::ByName(interface_name.as_str().to_owned()), ), ) - })?; - match result.status { - UpdateStatus::Updated => Ok(()), - UpdateStatus::NotUpdatedButExists => { - let interface = &result.found; - if interface.time_deleted().is_some() { - // Already deleted - Ok(()) - } else { - Err(Error::internal_error(&format!( - "failed to delete network interface: {}", - interface_id - ))) - } - } - } + }) + .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)) } - pub async fn subnet_lookup_network_interface( + /// Delete a `NetworkInterface` attached to a provided instance. + pub async fn instance_delete_network_interface( &self, - subnet_id: &Uuid, + opctx: &OpContext, + authz_instance: &authz::Instance, interface_name: &Name, - ) -> LookupResult { - use db::schema::network_interface::dsl; + ) -> DeleteResult { + opctx.authorize(authz::Action::Modify, authz_instance).await?; - dsl::network_interface - .filter(dsl::subnet_id.eq(*subnet_id)) + let (authz_interface, _) = self + .network_interface_fetch(opctx, &authz_instance, interface_name) + .await?; + opctx.authorize(authz::Action::Delete, &authz_interface).await?; + + use db::schema::network_interface::dsl; + let now = Utc::now(); + let interface_id = authz_interface.id(); + diesel::update(dsl::network_interface) + .filter(dsl::id.eq(interface_id)) .filter(dsl::time_deleted.is_null()) - .filter(dsl::name.eq(interface_name.clone())) - .select(db::model::NetworkInterface::as_select()) - .get_result_async(self.pool()) + .set((dsl::time_deleted.eq(now),)) + .execute_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, ErrorHandler::NotFoundByLookup( ResourceType::NetworkInterface, - LookupType::ByName(interface_name.to_string()), + LookupType::ById(interface_id), ), ) - }) + })?; + Ok(()) } /// List network interfaces associated with a given instance. pub async fn instance_list_network_interfaces( &self, - instance_id: &Uuid, + opctx: &OpContext, + authz_instance: &authz::Instance, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, authz_instance).await?; + use db::schema::network_interface::dsl; paginated(dsl::network_interface, dsl::name, &pagparams) .filter(dsl::time_deleted.is_null()) - .filter(dsl::instance_id.eq(*instance_id)) + .filter(dsl::instance_id.eq(authz_instance.id())) .select(NetworkInterface::as_select()) - .load_async::(self.pool()) + .load_async::(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } @@ -1803,26 +1857,14 @@ impl DataStore { /// Get a network interface by name attached to an instance pub async fn instance_lookup_network_interface( &self, - instance_id: &Uuid, + opctx: &OpContext, + authz_instance: &authz::Instance, interface_name: &Name, ) -> LookupResult { - use db::schema::network_interface::dsl; - dsl::network_interface - .filter(dsl::instance_id.eq(*instance_id)) - .filter(dsl::name.eq(interface_name.clone())) - .filter(dsl::time_deleted.is_null()) - .select(NetworkInterface::as_select()) - .get_result_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::NetworkInterface, - LookupType::ByName(interface_name.to_string()), - ), - ) - }) + Ok(self + .network_interface_fetch(opctx, &authz_instance, interface_name) + .await? + .1) } // Create a record for a new Oximeter instance diff --git a/nexus/src/db/subnet_allocation.rs b/nexus/src/db/subnet_allocation.rs index e6ecf5ec02a..c248db361ef 100644 --- a/nexus/src/db/subnet_allocation.rs +++ b/nexus/src/db/subnet_allocation.rs @@ -1134,6 +1134,7 @@ impl QueryFragment for InsertNetworkInterfaceQueryValues { mod test { use super::NetworkInterfaceError; use super::SubnetError; + use crate::context::OpContext; use crate::db::model::{ self, IncompleteNetworkInterface, NetworkInterface, VpcSubnet, }; @@ -1298,6 +1299,7 @@ mod test { let pool = Arc::new(crate::db::Pool::new(&cfg)); let db_datastore = Arc::new(crate::db::DataStore::new(Arc::clone(&pool))); + let opctx = OpContext::for_tests(log.new(o!()), db_datastore.clone()); // Two test VpcSubnets, in different VPCs. The IPv4 range has space for // 16 addresses, less the 6 that are reserved. @@ -1352,7 +1354,7 @@ mod test { ) .unwrap(); let inserted_interface = db_datastore - .instance_create_network_interface(interface.clone()) + .instance_create_network_interface_raw(&opctx, interface.clone()) .await .expect("Failed to insert interface with known-good IP address"); assert_interfaces_eq(&interface, &inserted_interface); @@ -1380,7 +1382,7 @@ mod test { ) .unwrap(); let inserted_interface = db_datastore - .instance_create_network_interface(interface.clone()) + .instance_create_network_interface_raw(&opctx, interface.clone()) .await .expect("Failed to insert interface with known-good IP address"); assert_interfaces_eq(&interface, &inserted_interface); @@ -1404,8 +1406,9 @@ mod test { Some(requested_ip), ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( matches!( result, @@ -1429,8 +1432,9 @@ mod test { None, ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( matches!( result, @@ -1455,8 +1459,9 @@ mod test { addr, ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( matches!(result, Err(NetworkInterfaceError::InstanceSpansMultipleVpcs(_))), "Attaching an interface to an instance which already has one in a different VPC should fail" @@ -1481,8 +1486,9 @@ mod test { None, ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( result.is_ok(), "We should be able to allocate 8 more interfaces successfully", @@ -1501,8 +1507,9 @@ mod test { None, ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( matches!( result, @@ -1528,8 +1535,9 @@ mod test { None, ) .unwrap(); - let result = - db_datastore.instance_create_network_interface(interface).await; + let result = db_datastore + .instance_create_network_interface_raw(&opctx, interface) + .await; assert!( result.is_ok(), concat!( @@ -1577,6 +1585,7 @@ mod test { let pool = Arc::new(crate::db::Pool::new(&cfg)); let db_datastore = Arc::new(crate::db::DataStore::new(Arc::clone(&pool))); + let opctx = OpContext::for_tests(log.new(o!()), db_datastore.clone()); let ipv4_block = Ipv4Net("172.30.0.0/28".parse().unwrap()); let ipv6_block = Ipv6Net("fd12:3456:7890::/64".parse().unwrap()); let subnet_name = "subnet-a".to_string().try_into().unwrap(); @@ -1610,13 +1619,13 @@ mod test { ) .unwrap(); let inserted_interface = db_datastore - .instance_create_network_interface(interface.clone()) + .instance_create_network_interface_raw(&opctx, interface.clone()) .await .expect("Failed to insert interface with known-good IP address"); // Attempt to insert the exact same record again. let result = db_datastore - .instance_create_network_interface(interface.clone()) + .instance_create_network_interface_raw(&opctx, interface.clone()) .await; if let Err(NetworkInterfaceError::DuplicatePrimaryKey(key)) = result { assert_eq!(key, inserted_interface.identity.id); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index f01050cabed..f682d86cc6d 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1514,9 +1514,8 @@ impl Nexus { instance_name, ) .await?; - opctx.authorize(authz::Action::ListChildren, &authz_instance).await?; self.db_datastore - .instance_list_network_interfaces(&authz_instance.id(), pagparams) + .instance_list_network_interfaces(opctx, &authz_instance, pagparams) .await } @@ -1540,9 +1539,15 @@ impl Nexus { .db_datastore .instance_fetch(opctx, &authz_project, instance_name) .await?; - opctx.authorize(authz::Action::Modify, &authz_instance).await?; - // TODO-completeness: We'd like to relax this once hot-plug is supported + // TODO-completeness: We'd like to relax this once hot-plug is + // supported. + // + // TODO-correctness: There's a TOCTOU race here. Someone might start the + // instance between this check and when we actually create the NIC + // record. One solution is to place the state verification in the query + // to create the NIC. Unfortunately, that query is already very + // complicated. let stopped = db::model::InstanceState::new(external::InstanceState::Stopped); if db_instance.runtime_state.state != stopped { @@ -1559,7 +1564,7 @@ impl Nexus { .db_datastore .vpc_fetch(opctx, &authz_project, &vpc_name) .await?; - let (_, db_subnet) = self + let (authz_subnet, db_subnet) = self .db_datastore .vpc_subnet_fetch(opctx, &authz_vpc, &subnet_name) .await?; @@ -1576,7 +1581,12 @@ impl Nexus { )?; let interface = self .db_datastore - .instance_create_network_interface(interface) + .instance_create_network_interface( + opctx, + &authz_subnet, + &authz_instance, + interface, + ) .await .map_err(NetworkInterfaceError::into_external)?; Ok(interface) @@ -1609,20 +1619,12 @@ impl Nexus { "Instance must be stopped to detach a network interface", )); } - // TODO-cleanup: It's annoying that we need to look up the interface by - // name, which gets a single record, and then soft-delete that one - // record. We should be able to do both at once, but the - // `update_and_check` tools only operate on the primary key. That means - // we need to fetch the whole record first. - let interface = self - .db_datastore - .instance_lookup_network_interface( - &authz_instance.id(), + self.db_datastore + .instance_delete_network_interface( + opctx, + &authz_instance, interface_name, ) - .await?; - self.db_datastore - .instance_delete_network_interface(&interface.id()) .await } @@ -1643,10 +1645,10 @@ impl Nexus { instance_name, ) .await?; - opctx.authorize(authz::Action::ListChildren, &authz_instance).await?; self.db_datastore .instance_lookup_network_interface( - &authz_instance.id(), + opctx, + &authz_instance, interface_name, ) .await diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index 338bffb334b..53197ecb607 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -148,6 +148,12 @@ pub fn saga_instance_create() -> SagaTemplate { new_action_noop_undo(sic_alloc_server), ); + template_builder.append( + "initial_runtime", + "CreateInstanceRecord", + new_action_noop_undo(sic_create_instance_record), + ); + // NOTE: The separation of the ID-allocation and NIC creation nodes is // intentional. // @@ -184,12 +190,6 @@ pub fn saga_instance_create() -> SagaTemplate { new_action_noop_undo(sic_create_network_interfaces), ); - template_builder.append( - "initial_runtime", - "CreateInstanceRecord", - new_action_noop_undo(sic_create_instance_record), - ); - template_builder.append( "instance_ensure", "InstanceEnsure", @@ -267,6 +267,12 @@ async fn sic_create_custom_network_interfaces( OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); let instance_id = sagactx.lookup::("instance_id")?; let ids = sagactx.lookup::>("network_interface_ids")?; + + // Lookup authz objects, used in the call to create the NIC itself. + let authz_instance = datastore + .instance_lookup_by_id(instance_id) + .await + .map_err(ActionError::action_failed)?; let authz_project = datastore .project_lookup_by_id(saga_params.project_id) .await @@ -303,7 +309,7 @@ async fn sic_create_custom_network_interfaces( // should probably either be in a transaction, or the // `subnet_create_network_interface` function/query needs some JOIN // on the `vpc_subnet` table. - let (_, db_subnet) = datastore + let (authz_subnet, db_subnet) = datastore .vpc_subnet_fetch( &opctx, &authz_vpc, @@ -323,8 +329,14 @@ async fn sic_create_custom_network_interfaces( params.ip, ) .map_err(ActionError::action_failed)?; - let result = - datastore.instance_create_network_interface(interface).await; + let result = datastore + .instance_create_network_interface( + &opctx, + &authz_subnet, + &authz_instance, + interface, + ) + .await; use crate::db::subnet_allocation::NetworkInterfaceError; let interface = match result { @@ -363,7 +375,8 @@ async fn sic_create_custom_network_interfaces( // saga node. datastore .instance_lookup_network_interface( - &instance_id, + &opctx, + &authz_instance, &db::model::Name(params.identity.name.clone()), ) .await @@ -400,6 +413,12 @@ async fn sic_create_default_network_interface( subnet_name: default_name.clone(), ip: None, // Request an IP address allocation }; + + // Lookup authz objects, used in the call to actually create the NIC. + let authz_instance = datastore + .instance_lookup_by_id(instance_id) + .await + .map_err(ActionError::action_failed)?; let authz_project = datastore .project_lookup_by_id(saga_params.project_id) .await @@ -408,7 +427,7 @@ async fn sic_create_default_network_interface( .vpc_fetch(&opctx, &authz_project, &internal_default_name.clone()) .await .map_err(ActionError::action_failed)?; - let (_, db_subnet) = datastore + let (authz_subnet, db_subnet) = datastore .vpc_subnet_fetch(&opctx, &authz_vpc, &internal_default_name) .await .map_err(ActionError::action_failed)?; @@ -426,7 +445,12 @@ async fn sic_create_default_network_interface( ) .map_err(ActionError::action_failed)?; let interface = datastore - .instance_create_network_interface(interface) + .instance_create_network_interface( + &opctx, + &authz_subnet, + &authz_instance, + interface, + ) .await .map_err(db::subnet_allocation::NetworkInterfaceError::into_external) .map_err(ActionError::action_failed)?; @@ -453,15 +477,12 @@ async fn sic_create_network_interfaces_undo( async fn sic_create_instance_record( sagactx: ActionContext, -) -> Result { +) -> Result { let osagactx = sagactx.user_data(); let params = sagactx.saga_params(); let sled_uuid = sagactx.lookup::("server_id"); let instance_id = sagactx.lookup::("instance_id"); let propolis_uuid = sagactx.lookup::("propolis_id"); - let nics = sagactx - .lookup::>>("network_interfaces")? - .unwrap_or_default(); let runtime = InstanceRuntimeState { run_state: InstanceState::Creating, @@ -490,11 +511,7 @@ async fn sic_create_instance_record( .await .map_err(ActionError::action_failed)?; - // See also: instance_set_runtime in nexus.rs for a similar construction. - Ok(InstanceHardware { - runtime: instance.runtime().clone().into(), - nics: nics.into_iter().map(|nic| nic.into()).collect(), - }) + Ok(instance.runtime().clone().into()) } async fn sic_instance_ensure( @@ -508,8 +525,14 @@ async fn sic_instance_ensure( }; let instance_id = sagactx.lookup::("instance_id")?; let sled_uuid = sagactx.lookup::("server_id")?; - let initial_runtime = - sagactx.lookup::("initial_runtime")?; + let nics = sagactx + .lookup::>>("network_interfaces")? + .unwrap_or_default(); + let runtime = sagactx.lookup::("initial_runtime")?; + let initial_hardware = InstanceHardware { + runtime: runtime.into(), + nics: nics.into_iter().map(|nic| nic.into()).collect(), + }; let sa = osagactx .sled_client(&sled_uuid) .await @@ -522,7 +545,7 @@ async fn sic_instance_ensure( .instance_put( &instance_id, &InstanceEnsureBody { - initial: initial_runtime, + initial: initial_hardware, target: runtime_params, migrate: None, }, From 4612766215faf7c212e08d07caad677a2da4ee91 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 16 Mar 2022 22:08:55 +0000 Subject: [PATCH 2/2] Bring NIC create/delete permission in line with other containers --- nexus/src/db/datastore.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index b30979b2127..ec5600afa13 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1686,7 +1686,7 @@ impl DataStore { interface: IncompleteNetworkInterface, ) -> Result { opctx - .authorize(authz::Action::Modify, authz_instance) + .authorize(authz::Action::CreateChild, authz_instance) .await .map_err(NetworkInterfaceError::External)?; opctx @@ -1807,8 +1807,6 @@ impl DataStore { authz_instance: &authz::Instance, interface_name: &Name, ) -> DeleteResult { - opctx.authorize(authz::Action::Modify, authz_instance).await?; - let (authz_interface, _) = self .network_interface_fetch(opctx, &authz_instance, interface_name) .await?;