Deterministic UUIDs path auth tests

oxidecomputer · Dec 27, 2023 · 10127d7 · 10127d7
1 parent d479633
commit 10127d7
Show file tree

Hide file tree

Showing 15 changed files with 108 additions and 35 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -388,7 +388,7 @@ tufaceous-lib = { path = "tufaceous-lib" }
 unicode-width = "0.1.11"
 update-engine = { path = "update-engine" }
 usdt = "0.3"
-uuid = { version = "1.6.1", features = ["serde", "v4"] }
+uuid = { version = "1.6.1", features = ["serde", "v4", "v5"] }
 walkdir = "2.4"
 wicket = { path = "wicket" }
 wicket-common = { path = "wicket-common" }

diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs
@@ -9,6 +9,8 @@ extern crate diesel;
 #[macro_use]
 extern crate newtype_derive;
 
+use uuid::Uuid;
+
 mod address_lot;
 mod bgp;
 mod block_size;
@@ -307,6 +309,15 @@ macro_rules! impl_enum_type {
 
 pub(crate) use impl_enum_type;
 
+/// This is an arbitrary UUID, but it's stable because it's embedded as a
+/// constant. This defines a namespace, according to
+/// <https://www.rfc-editor.org/rfc/rfc4122>, which allows generation of v5
+/// UUIDs which are deterministic.
+///
+/// This UUID is used to identify hardware.
+pub(crate) const HARDWARE_UUID_NAMESPACE: Uuid =
+    Uuid::from_u128(206230429496795504636731999500138461979);
+
 /// Describes a type that's represented in the database using a String
 ///
 /// If you're reaching for this type, consider whether it'd be better to use an

diff --git a/nexus/db-model/src/physical_disk.rs b/nexus/db-model/src/physical_disk.rs
@@ -36,8 +36,24 @@ impl PhysicalDisk {
         variant: PhysicalDiskKind,
         sled_id: Uuid,
     ) -> Self {
+        // NOTE: We may want to be more restrictive when parsing the vendor,
+        // serial, and model values, so that we can supply a separator
+        // distinguishing them.
+        //
+        // Theoretically, we could have the following problem:
+        //
+        // - A Disk vendor "Foo" makes a disk with serial "Bar", and model "Rev1".
+        //   - This becomes: "FooBarRev1".
+        // - A Disk vendor "FooBar" makes a disk with serial "Rev", and model "1".
+        //   - This becomes: "FooBarRev1", and conflicts.
+        let interpolated_name = format!("{vendor}{serial}{model}");
+        let disk_id = Uuid::new_v5(
+            &crate::HARDWARE_UUID_NAMESPACE,
+            interpolated_name.as_bytes(),
+        );
+        println!("Physical Disk ID: {disk_id}, from {interpolated_name}");
         Self {
-            identity: PhysicalDiskIdentity::new(Uuid::new_v4()),
+            identity: PhysicalDiskIdentity::new(disk_id),
             time_deleted: None,
             rcgen: Generation::new(),
             vendor,

diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs
@@ -1348,7 +1348,7 @@ mod test {
             .unwrap();
 
         let (.., physical_disk_authz) = LookupPath::new(&opctx, &datastore)
-            .physical_disk(&disk.vendor, &disk.serial, &disk.model)
+            .physical_disk(disk.vendor, disk.serial, disk.model)
             .lookup_for(authz::Action::Modify)
             .await
             .unwrap();

diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs
@@ -34,7 +34,7 @@ use uuid::Uuid;
 impl DataStore {
     /// - Sled Agents like to look up physical disks by "Vendor, Serial, Model"
     /// - The external API likes to look up physical disks by UUID
-    /// - LookuPath objects are opinionated about how they perform lookups. They
+    /// - LookupPath objects are opinionated about how they perform lookups. They
     /// support "primary keys" or "names", but they're opinionated about the
     /// name objects being a single string.
     ///

diff --git a/nexus/db-queries/src/db/lookup.rs b/nexus/db-queries/src/db/lookup.rs
@@ -370,15 +370,15 @@ impl<'a> LookupPath<'a> {
     /// Select a resource of type PhysicalDisk, identified by its id
     pub fn physical_disk(
         self,
-        vendor: &str,
-        serial: &str,
-        model: &str,
+        vendor: String,
+        serial: String,
+        model: String,
     ) -> PhysicalDisk<'a> {
         PhysicalDisk::PrimaryKey(
             Root { lookup_root: self },
-            vendor.to_string(),
-            serial.to_string(),
-            model.to_string(),
+            vendor,
+            serial,
+            model,
         )
     }
 

diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml
@@ -0,0 +1,41 @@
+<!-- This file was generated automatically.
+     Do not edit it: it is likely to be discarded and generated again before it's read next time.
+     Files used to generate this file:
+       config.xml      -->
+
+<!-- Config that is used when server is run without config file. --><clickhouse>
+    <logger>
+        <level>trace</level>
+        <console>true</console>
+    </logger>
+
+    <http_port>8123</http_port>
+    <tcp_port>9000</tcp_port>
+    <mysql_port>9004</mysql_port>
+
+    <path>./</path>
+
+    <mlock_executable>true</mlock_executable>
+
+    <users>
+        <default>
+            <password/>
+
+            <networks>
+                <ip>::/0</ip>
+            </networks>
+
+            <profile>default</profile>
+            <quota>default</quota>
+            <access_management>1</access_management>
+        </default>
+    </users>
+
+    <profiles>
+        <default/>
+    </profiles>
+
+    <quotas>
+        <default/>
+    </quotas>
+</clickhouse>
diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs
@@ -168,11 +168,10 @@ impl super::Nexus {
     ) -> Result<lookup::PhysicalDisk<'a>, Error> {
         let (v, s, m) = self
             .db_datastore
-            .physical_disk_id_to_name_no_auth(disk_selector.id)
+            .physical_disk_id_to_name_no_auth(disk_selector.disk_id)
             .await?;
 
-        Ok(LookupPath::new(&opctx, &self.db_datastore)
-            .physical_disk(&v, &s, &m))
+        Ok(LookupPath::new(&opctx, &self.db_datastore).physical_disk(v, s, m))
     }
 
     pub(crate) async fn sled_list_physical_disks(
@@ -278,9 +277,9 @@ impl super::Nexus {
         let (_authz_disk, db_disk) =
             LookupPath::new(&opctx, &self.db_datastore)
                 .physical_disk(
-                    &info.disk_vendor,
-                    &info.disk_serial,
-                    &info.disk_model,
+                    info.disk_vendor,
+                    info.disk_serial,
+                    info.disk_model,
                 )
                 .fetch()
                 .await?;

diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs
@@ -4869,7 +4869,7 @@ async fn physical_disk_list(
 /// Get a physical disk
 #[endpoint {
     method = GET,
-    path = "/v1/system/hardware/disks/{id}",
+    path = "/v1/system/hardware/disks/{disk_id}",
     tags = ["system/hardware"],
 }]
 async fn physical_disk_view(
@@ -4892,7 +4892,7 @@ async fn physical_disk_view(
 /// Update a physical disk's state
 #[endpoint {
     method = PUT,
-    path = "/v1/system/hardware/disks/{id}",
+    path = "/v1/system/hardware/disks/{disk_id}",
     tags = ["system/hardware"],
 }]
 async fn physical_disk_update(

diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs
@@ -60,7 +60,13 @@ pub const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc";
 pub const SWITCH_UUID: &str = "dae4e1f1-410e-4314-bff1-fec0504be07e";
 pub const OXIMETER_UUID: &str = "39e6175b-4df2-4730-b11d-cbc1e60a2e78";
 pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de";
-pub const PHYSICAL_DISK_UUID: &str = "2092c014-cb33-4654-b43e-d0958f855642";
+/// This is not random: It's a v5 UUID derived from:
+///
+/// Uuid::new_v5(
+///     HARDWARE_UUID_NAMESPACE,
+///     ("test-vendor", "test-serial", "test-model"),
+/// )
+pub const PHYSICAL_DISK_UUID: &str = "25849923-2232-5d20-b939-ffee5bc3dd89";
 pub const RACK_SUBNET: &str = "fd00:1122:3344:01::/56";
 
 /// The reported amount of hardware threads for an emulated sled agent.

diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs
@@ -1844,7 +1844,7 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
 
         VerifyEndpoint {
             url: &HARDWARE_DISK_URL,
-            visibility: Visibility::Public,
+            visibility: Visibility::Protected,
             unprivileged_access: UnprivilegedAccess::None,
             allowed_methods: vec![
                 AllowedMethod::Get,

diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt
@@ -122,8 +122,8 @@ networking_switch_port_apply_settings    POST     /v1/system/hardware/switch-por
 networking_switch_port_clear_settings    DELETE   /v1/system/hardware/switch-port/{port}/settings
 networking_switch_port_list              GET      /v1/system/hardware/switch-port
 physical_disk_list                       GET      /v1/system/hardware/disks
-physical_disk_update                     PUT      /v1/system/hardware/disks/{id}
-physical_disk_view                       GET      /v1/system/hardware/disks/{id}
+physical_disk_update                     PUT      /v1/system/hardware/disks/{disk_id}
+physical_disk_view                       GET      /v1/system/hardware/disks/{disk_id}
 rack_list                                GET      /v1/system/hardware/racks
 rack_view                                GET      /v1/system/hardware/racks/{rack_id}
 sled_instance_list                       GET      /v1/system/hardware/sleds/{sled_id}/instances

diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs
@@ -70,16 +70,7 @@ id_path_param!(GroupPath, group_id, "group");
 // ID that can be used to deterministically generate the UUID.
 id_path_param!(SledPath, sled_id, "sled");
 id_path_param!(SwitchPath, switch_id, "switch");
-
-pub struct SledSelector {
-    /// ID of the sled
-    pub sled: Uuid,
-}
-
-#[derive(Serialize, Deserialize, JsonSchema)]
-pub struct PhysicalDiskPath {
-    pub id: Uuid,
-}
+id_path_param!(PhysicalDiskPath, disk_id, "physical_disk");
 
 /// Updateable properties of a `PhysicalDisk`.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]

diff --git a/openapi/nexus.json b/openapi/nexus.json
@@ -3604,7 +3604,7 @@
         }
       }
     },
-    "/v1/system/hardware/disks/{id}": {
+    "/v1/system/hardware/disks/{disk_id}": {
       "get": {
         "tags": [
           "system/hardware"
@@ -3614,7 +3614,8 @@
         "parameters": [
           {
             "in": "path",
-            "name": "id",
+            "name": "disk_id",
+            "description": "ID of the physical_disk",
             "required": true,
             "schema": {
               "type": "string",
@@ -3650,7 +3651,8 @@
         "parameters": [
           {
             "in": "path",
-            "name": "id",
+            "name": "disk_id",
+            "description": "ID of the physical_disk",
             "required": true,
             "schema": {
               "type": "string",