Move network interface authz to the data store (#778)

* 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.

* Bring NIC create/delete permission in line with other containers

Author: bnaecker

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;

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;

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<NetworkInterface, NetworkInterfaceError> {
+        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<NetworkInterface, NetworkInterfaceError> {
         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,109 +1743,124 @@ 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::<db::model::NetworkInterface>(*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<db::model::NetworkInterface> {
-        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<NetworkInterface> {
+        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::<NetworkInterface>(self.pool())
+            .load_async::<NetworkInterface>(self.pool_authorized(opctx).await?)
             .await
             .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,
-        instance_id: &Uuid,
+        opctx: &OpContext,
+        authz_instance: &authz::Instance,
         interface_name: &Name,
     ) -> LookupResult<NetworkInterface> {
-        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

nexus/src/db/subnet_allocation.rs

@@ -1134,6 +1134,7 @@ impl QueryFragment<Pg> 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);