diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 1d6aaf7eba..a61a0f8885 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -516,3 +516,4 @@ pub type Instance = ProjectChild; pub type Vpc = ProjectChild; pub type VpcRouter = ProjectChild; pub type VpcSubnet = ProjectChild; +pub type NetworkInterface = ProjectChild; diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index 3f2774caa9..cc407ab9aa 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 f07090a5e8..39c5fe82f2 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1674,8 +1674,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::CreateChild, 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; @@ -1686,7 +1707,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)) } @@ -1718,82 +1743,109 @@ 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, + ) + }) } - pub async fn subnet_lookup_network_interface( + /// 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, - subnet_id: &Uuid, + 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, - ) -> LookupResult { - use db::schema::network_interface::dsl; + ) -> DeleteResult { + let (authz_interface, _) = self + .network_interface_fetch(opctx, &authz_instance, interface_name) + .await?; + opctx.authorize(authz::Action::Delete, &authz_interface).await?; - dsl::network_interface - .filter(dsl::subnet_id.eq(*subnet_id)) + 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)) } @@ -1801,26 +1853,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 e6ecf5ec02..c248db361e 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 f5346cd807..e3de714b78 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1516,9 +1516,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 } @@ -1542,9 +1541,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 { @@ -1561,7 +1566,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?; @@ -1578,7 +1583,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) @@ -1611,20 +1621,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 } @@ -1645,10 +1647,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 338bffb334..53197ecb60 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, },