Validate transit IPs on network interface update (#7559)

This PR adds in some checks when setting transit IPs on a NIC to ensure
that we have:
* Only unicast addresses.
* No loopback addresses.
* No duplicates.

As discussed in the ticket, violation of any of these isn't going to
leave OPTE or CRDB in a broken state -- just a confusing one for end
users. This should make things unambiguous in terms of  how the
transit IPs will be handled.

Closes #7530.

Author: FelixMcFelix

nexus/src/app/network_interface.rs

@@ -2,17 +2,17 @@ use nexus_db_queries::authz::ApiResource;
 use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db::queries::network_interface;
 use nexus_types::external_api::params;
+use omicron_common::api::external::http_pagination::PaginatedBy;
 use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DeleteResult;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::NameOrId;
-
-use omicron_common::api::external::http_pagination::PaginatedBy;
 use omicron_common::api::external::UpdateResult;
 use omicron_uuid_kinds::GenericUuid;
 use omicron_uuid_kinds::InstanceUuid;
+use oxnet::IpNet;
 use uuid::Uuid;
 
 use nexus_db_queries::authz;
@@ -148,6 +148,7 @@ impl super::Nexus {
     ) -> UpdateResult<db::model::InstanceNetworkInterface> {
         let (.., authz_instance, authz_interface) =
             network_interface_lookup.lookup_for(authz::Action::Modify).await?;
+        validate_transit_ips(updates.transit_ips.as_slice())?;
         self.db_datastore
             .instance_update_network_interface(
                 opctx,
@@ -213,3 +214,45 @@ impl super::Nexus {
         }
     }
 }
+
+fn validate_transit_ips(ips: &[IpNet]) -> Result<(), Error> {
+    for (i, ip) in ips.iter().enumerate() {
+        let (count, ty) = if ip.is_host_net() {
+            ("", "address")
+        } else {
+            (" block", "network")
+        };
+
+        if !ip.is_network_address() {
+            return Err(Error::invalid_request(format!(
+                "transit IP{count} {ip} has a non-zero host identifier"
+            )));
+        }
+
+        if ip.is_multicast() {
+            return Err(Error::invalid_request(format!(
+                "transit IP{count} {ip} is a multicast {ty}"
+            )));
+        }
+
+        if ip.is_loopback() {
+            return Err(Error::invalid_request(format!(
+                "transit IP{count} {ip} is a loopback {ty}"
+            )));
+        }
+
+        // Checking for overlapping CIDRs using all prior ips is O(n^2). This
+        // is an infrequent check, and we can bound n if desired.
+        // The fastest way to catch overlaps would be to make use of a trie for
+        // representing past `IpNet`s per address family.
+        let overlap = &ips[..i].iter().find(|el| ip.overlaps(el));
+
+        if let Some(past) = overlap {
+            return Err(Error::invalid_request(format!(
+                "transit IP{count} {ip} overlaps with {past}"
+            )));
+        }
+    }
+
+    Ok(())
+}

nexus/tests/integration_tests/instances.rs

@@ -32,7 +32,9 @@ use nexus_test_utils::resource_helpers::object_create;
 use nexus_test_utils::resource_helpers::object_create_error;
 use nexus_test_utils::resource_helpers::object_delete;
 use nexus_test_utils::resource_helpers::object_delete_error;
+use nexus_test_utils::resource_helpers::object_get;
 use nexus_test_utils::resource_helpers::object_put;
+use nexus_test_utils::resource_helpers::object_put_error;
 use nexus_test_utils::resource_helpers::objects_list_page_authz;
 use nexus_test_utils::resource_helpers::DiskTest;
 use nexus_test_utils::wait_for_producer;
@@ -3116,6 +3118,235 @@ async fn test_instance_update_network_interfaces(
     assert_eq!(iface.identity.name, if_params[0].identity.name);
 }
 
+#[nexus_test]
+async fn test_instance_update_network_interface_transit_ips(
+    cptestctx: &ControlPlaneTestContext,
+) {
+    let client = &cptestctx.external_client;
+    let instance_name = "transit-ips-test";
+    let nic_name = "net0";
+
+    create_project_and_pool(&client).await;
+
+    // Create a stopped instance with a single network interface.
+    let instance = create_instance_with(
+        &client,
+        PROJECT_NAME,
+        instance_name,
+        &params::InstanceNetworkInterfaceAttachment::Default,
+        vec![],
+        vec![],
+        false,
+        Default::default(),
+    )
+    .await;
+
+    let url_interface = format!(
+        "/v1/network-interfaces/{nic_name}?instance={}",
+        instance.identity.id
+    );
+
+    let base_update = params::InstanceNetworkInterfaceUpdate {
+        identity: IdentityMetadataUpdateParams {
+            name: None,
+            description: None,
+        },
+        primary: false,
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "1.1.1.1/32".parse().unwrap(),
+        ],
+    };
+
+    // Verify that a selection of transit IPs (mixture of private and global
+    // unicast, no overlaps) is accepted.
+    let updated_nic: InstanceNetworkInterface =
+        object_put(client, &url_interface, &base_update).await;
+
+    assert_eq!(base_update.transit_ips, updated_nic.transit_ips);
+
+    // Non-canonical form (e.g., host identifier is nonzero) subnets should
+    // be rejected.
+    let with_extra_bits = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            //  Invalid vvv
+            "172.30.255.255/24".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_extra_bits,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 172.30.255.255/24 has a non-zero host identifier",
+    );
+
+    // Multicast IP blocks should be rejected.
+    let with_mc1 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "224.0.0.0/4".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_mc1,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 224.0.0.0/4 is a multicast network",
+    );
+
+    let with_mc2 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "230.20.20.128/32".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_mc2,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP 230.20.20.128/32 is a multicast address",
+    );
+
+    // Loopback ranges.
+    let with_lo1 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "127.42.77.0/24".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_lo1,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 127.42.77.0/24 is a loopback network",
+    );
+
+    let with_lo2 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "127.0.0.1/32".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_lo2,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(err.message, "transit IP 127.0.0.1/32 is a loopback address");
+
+    // Overlapping IP ranges should be rejected, as should identical ranges.
+    let with_dup1 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "10.0.0.0/9".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_dup1,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 10.0.0.0/9 overlaps with 10.0.0.0/9",
+    );
+
+    let with_dup2 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.0.0.0/9".parse().unwrap(),
+            "10.128.0.0/9".parse().unwrap(),
+            "10.128.32.0/24".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_dup2,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 10.128.32.0/24 overlaps with 10.128.0.0/9",
+    );
+
+    // Verify that we also catch more specific CIDRs appearing sooner in the list.
+    let with_dup3 = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec![
+            "10.20.20.0/30".parse().unwrap(),
+            "10.0.0.0/8".parse().unwrap(),
+        ],
+        ..base_update.clone()
+    };
+    let err = object_put_error(
+        client,
+        &url_interface,
+        &with_dup3,
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(
+        err.message,
+        "transit IP block 10.0.0.0/8 overlaps with 10.20.20.0/30",
+    );
+
+    // ...and in the end, no changes have applied.
+    let final_nic: InstanceNetworkInterface =
+        object_get(client, &url_interface).await;
+    assert_eq!(updated_nic.transit_ips, final_nic.transit_ips);
+
+    // As a final sanity test, we can still effectively remove spoof checking
+    // using the unspecified network address.
+    let allow_all = params::InstanceNetworkInterfaceUpdate {
+        transit_ips: vec!["0.0.0.0/0".parse().unwrap()],
+        ..base_update.clone()
+    };
+
+    let updated_nic: InstanceNetworkInterface =
+        object_put(client, &url_interface, &allow_all).await;
+
+    assert_eq!(allow_all.transit_ips, updated_nic.transit_ips);
+}
+
 /// This test specifically creates two NICs, the second of which will fail the
 /// saga on purpose, since its IP address is the same. This is to verify that
 /// the initial NIC is also deleted.