Validate instance hostnames in the Nexus API (#4938)

- Add the `Hostname` type to the external API, which checks that a
string conforms to RFC 1035 (and others) describing valid Internet
hostnames. Add some tests for that.
- Use the new type everywhere in Nexus and its integration tests, though
we still serialize to a String in the database and when passing the
instance information to the sled-agent. The sled-agent does its own
validation internally through OPTE.
- Add a few regression tests for creating instances with known-bad
hostnames.

common/Cargo.toml

@@ -38,6 +38,7 @@ parse-display.workspace = true
 progenitor.workspace = true
 omicron-workspace-hack.workspace = true
 once_cell.workspace = true
+regress.workspace = true
 
 [dev-dependencies]
 camino-tempfile.workspace = true

common/src/api/external/mod.rs

@@ -696,6 +696,109 @@ impl TryFrom<i64> for Generation {
     }
 }
 
+/// An RFC-1035-compliant hostname.
+#[derive(
+    Clone, Debug, Deserialize, Display, Eq, PartialEq, SerializeDisplay,
+)]
+#[display("{0}")]
+#[serde(try_from = "String", into = "String")]
+pub struct Hostname(String);
+
+impl Hostname {
+    /// Return the hostname as a string slice.
+    pub fn as_str(&self) -> &str {
+        self.0.as_str()
+    }
+}
+
+// Regular expression for hostnames.
+//
+// Each name is a dot-separated sequence of labels. Each label is supposed to
+// be an "LDH": letter, dash, or hyphen. Hostnames can consist of one label, or
+// many, separated by a `.`. While _domain_ names are allowed to end in a `.`,
+// making them fully-qualified, hostnames are not.
+//
+// Note that labels are allowed to contain a hyphen, but may not start or end
+// with one. See RFC 952, "Lexical grammar" section.
+//
+// Note that we need to use a regex engine capable of lookbehind to support
+// this, since we need to check that labels don't end with a `-`.
+const HOSTNAME_REGEX: &str = r#"^([a-zA-Z0-9]+[a-zA-Z0-9\-]*(?<!-))(\.[a-zA-Z0-9]+[a-zA-Z0-9\-]*(?<!-))*$"#;
+
+// Labels need to be encoded on the wire, and prefixed with a signel length
+// octet. They also need to end with a length octet of 0 when encoded. So the
+// longest name is a single label of 253 characters, which will be encoded as
+// `\xfd<the label>\x00`.
+const HOSTNAME_MAX_LEN: u32 = 253;
+
+impl FromStr for Hostname {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        anyhow::ensure!(
+            s.len() <= HOSTNAME_MAX_LEN as usize,
+            "Max hostname length is {HOSTNAME_MAX_LEN}"
+        );
+        let re = regress::Regex::new(HOSTNAME_REGEX).unwrap();
+        if re.find(s).is_some() {
+            Ok(Hostname(s.to_string()))
+        } else {
+            anyhow::bail!("Hostnames must comply with RFC 1035")
+        }
+    }
+}
+
+impl TryFrom<&str> for Hostname {
+    type Error = <Hostname as FromStr>::Err;
+
+    fn try_from(s: &str) -> Result<Self, Self::Error> {
+        s.parse()
+    }
+}
+
+impl TryFrom<String> for Hostname {
+    type Error = <Hostname as FromStr>::Err;
+
+    fn try_from(s: String) -> Result<Self, Self::Error> {
+        s.as_str().parse()
+    }
+}
+
+// Custom implementation of JsonSchema for Hostname to ensure RFC-1035-style
+// validation
+impl JsonSchema for Hostname {
+    fn schema_name() -> String {
+        "Hostname".to_string()
+    }
+
+    fn json_schema(
+        _: &mut schemars::gen::SchemaGenerator,
+    ) -> schemars::schema::Schema {
+        schemars::schema::Schema::Object(schemars::schema::SchemaObject {
+            metadata: Some(Box::new(schemars::schema::Metadata {
+                title: Some("An RFC-1035-compliant hostname".to_string()),
+                description: Some(
+                    "A hostname identifies a host on a network, and \
+                    is usually a dot-delimited sequence of labels, \
+                    where each label contains only letters, digits, \
+                    or the hyphen. See RFCs 1035 and 952 for more details."
+                        .to_string(),
+                ),
+                ..Default::default()
+            })),
+            instance_type: Some(schemars::schema::SingleOrVec::Single(
+                Box::new(schemars::schema::InstanceType::String),
+            )),
+            string: Some(Box::new(schemars::schema::StringValidation {
+                max_length: Some(HOSTNAME_MAX_LEN),
+                min_length: Some(1),
+                pattern: Some(HOSTNAME_REGEX.to_string()),
+            })),
+            ..Default::default()
+        })
+    }
+}
+
 // General types used to implement API resources
 
 /// Identifies a type of API resource
@@ -939,7 +1042,7 @@ pub struct Instance {
     /// memory allocated for this Instance
     pub memory: ByteCount,
     /// RFC1035-compliant hostname for the Instance.
-    pub hostname: String, // TODO-cleanup different type?
+    pub hostname: String,
 
     #[serde(flatten)]
     pub runtime: InstanceRuntimeState,
@@ -2737,6 +2840,7 @@ mod test {
         VpcFirewallRuleUpdateParams,
     };
     use crate::api::external::Error;
+    use crate::api::external::Hostname;
     use crate::api::external::ResourceType;
     use std::convert::TryFrom;
     use std::str::FromStr;
@@ -3460,4 +3564,24 @@ mod test {
         let conv = mac.to_i64();
         assert_eq!(original, conv);
     }
+
+    #[test]
+    fn test_hostname_from_str() {
+        assert!(Hostname::from_str("name").is_ok());
+        assert!(Hostname::from_str("a.good.name").is_ok());
+        assert!(Hostname::from_str("another.very-good.name").is_ok());
+        assert!(Hostname::from_str("0name").is_ok());
+        assert!(Hostname::from_str("name0").is_ok());
+        assert!(Hostname::from_str("0name0").is_ok());
+
+        assert!(Hostname::from_str("").is_err());
+        assert!(Hostname::from_str("no_no").is_err());
+        assert!(Hostname::from_str("no.fqdns.").is_err());
+        assert!(Hostname::from_str("empty..label").is_err());
+        assert!(Hostname::from_str("-hypen.cannot.start").is_err());
+        assert!(Hostname::from_str("hypen.-cannot.start").is_err());
+        assert!(Hostname::from_str("hypen.cannot.end-").is_err());
+        assert!(Hostname::from_str("hyphen-cannot-end-").is_err());
+        assert!(Hostname::from_str(&"too-long".repeat(100)).is_err());
+    }
 }

common/src/api/internal/nexus.rs

@@ -5,8 +5,8 @@
 //! APIs exposed by Nexus.
 
 use crate::api::external::{
-    ByteCount, DiskState, Generation, InstanceCpuCount, InstanceState, IpNet,
-    SemverVersion, Vni,
+    ByteCount, DiskState, Generation, Hostname, InstanceCpuCount,
+    InstanceState, IpNet, SemverVersion, Vni,
 };
 use chrono::{DateTime, Utc};
 use parse_display::{Display, FromStr};
@@ -36,8 +36,7 @@ pub struct InstanceProperties {
     pub ncpus: InstanceCpuCount,
     pub memory: ByteCount,
     /// RFC1035-compliant hostname for the instance.
-    // TODO-cleanup different type?
-    pub hostname: String,
+    pub hostname: Hostname,
 }
 
 /// The dynamic runtime properties of an instance: its current VMM ID (if any),

end-to-end-tests/src/instance_launch.rs

@@ -64,7 +64,7 @@ async fn instance_launch() -> Result<()> {
         .body(InstanceCreate {
             name: generate_name("instance")?,
             description: String::new(),
-            hostname: "localshark".into(), // 🦈
+            hostname: "localshark".parse().unwrap(), // 🦈
             memory: ByteCount(1024 * 1024 * 1024),
             ncpus: InstanceCpuCount(2),
             disks: vec![InstanceDiskAttachment::Attach {

nexus/db-model/src/instance.rs

@@ -46,7 +46,10 @@ pub struct Instance {
     pub memory: ByteCount,
 
     /// The instance's hostname.
-    // TODO-cleanup: Different type?
+    // TODO-cleanup: We use a validated wrapper type in the API, but not in
+    // between the database. This is to handle existing names that do not pass
+    // the new validation. We should swap this for a SQL-serializable validated
+    // type.
     #[diesel(column_name = hostname)]
     pub hostname: String,
 
@@ -81,7 +84,7 @@ impl Instance {
             user_data: params.user_data.clone(),
             ncpus: params.ncpus.into(),
             memory: params.memory.into(),
-            hostname: params.hostname.clone(),
+            hostname: params.hostname.to_string(),
             boot_on_fault: false,
             runtime_state,
         }

nexus/db-queries/src/db/datastore/instance.rs

@@ -97,7 +97,11 @@ impl From<InstanceAndActiveVmm> for omicron_common::api::external::Instance {
             project_id: value.instance.project_id,
             ncpus: value.instance.ncpus.into(),
             memory: value.instance.memory.into(),
-            hostname: value.instance.hostname,
+            hostname: value
+                .instance
+                .hostname
+                .parse()
+                .expect("found invalid hostname in the database"),
             runtime: omicron_common::api::external::InstanceRuntimeState {
                 run_state: *run_state.state(),
                 time_run_state_updated,

nexus/db-queries/src/db/queries/external_ip.rs

@@ -998,7 +998,7 @@ mod tests {
                 identity: IdentityMetadataCreateParams { name: String::from(name).parse().unwrap(), description: format!("instance {}", name) },
                 ncpus: InstanceCpuCount(omicron_common::api::external::InstanceCpuCount(1)).into(),
                 memory: ByteCount(omicron_common::api::external::ByteCount::from_gibibytes_u32(1)).into(),
-                hostname: "test".into(),
+                hostname: "test".parse().unwrap(),
                 ssh_public_keys: None,
                 user_data: vec![],
                 network_interfaces: Default::default(),

nexus/db-queries/src/db/queries/network_interface.rs

@@ -1738,7 +1738,7 @@ mod tests {
             },
             ncpus: InstanceCpuCount(4),
             memory: ByteCount::from_gibibytes_u32(4),
-            hostname: "inst".to_string(),
+            hostname: "inst".parse().unwrap(),
             user_data: vec![],
             ssh_public_keys: Some(Vec::new()),
             network_interfaces: InstanceNetworkInterfaceAttachment::None,

nexus/src/app/instance.rs

@@ -1010,6 +1010,23 @@ impl super::Nexus {
     ) -> Result<(), Error> {
         opctx.authorize(authz::Action::Modify, authz_instance).await?;
 
+        // Check that the hostname is valid.
+        //
+        // TODO-cleanup: This can be removed when we are confident that no
+        // instances exist prior to the addition of strict hostname validation
+        // in the API.
+        let Ok(hostname) = db_instance.hostname.parse() else {
+            let msg = format!(
+                "The instance hostname '{}' is no longer valid. \
+                To access the data on its disks, this instance \
+                must be deleted, and a new one created with the \
+                relevant disks. The new hostname will be validated \
+                at that time.",
+                db_instance.hostname,
+            );
+            return Err(Error::invalid_request(&msg));
+        };
+
         // Gather disk information and turn that into DiskRequests
         let disks = self
             .db_datastore
@@ -1175,7 +1192,7 @@ impl super::Nexus {
             properties: InstanceProperties {
                 ncpus: db_instance.ncpus.into(),
                 memory: db_instance.memory.into(),
-                hostname: db_instance.hostname.clone(),
+                hostname,
             },
             nics,
             source_nat,

nexus/src/app/sagas/instance_create.rs

@@ -1102,7 +1102,7 @@ pub mod test {
                 },
                 ncpus: InstanceCpuCount::try_from(2).unwrap(),
                 memory: ByteCount::from_gibibytes_u32(4),
-                hostname: String::from("inst"),
+                hostname: "inst".parse().unwrap(),
                 user_data: vec![],
                 ssh_public_keys: None,
                 network_interfaces:

nexus/src/app/sagas/instance_delete.rs

@@ -235,7 +235,7 @@ mod test {
             },
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
-            hostname: String::from("inst"),
+            hostname: "inst".parse().unwrap(),
             user_data: vec![],
             ssh_public_keys: Some(Vec::new()),
             network_interfaces:

nexus/src/app/sagas/instance_migrate.rs

@@ -568,7 +568,7 @@ mod tests {
                 },
                 ncpus: InstanceCpuCount(2),
                 memory: ByteCount::from_gibibytes_u32(2),
-                hostname: String::from(INSTANCE_NAME),
+                hostname: INSTANCE_NAME.parse().unwrap(),
                 user_data: b"#cloud-config".to_vec(),
                 ssh_public_keys: Some(Vec::new()),
                 network_interfaces:

nexus/src/app/sagas/instance_start.rs

@@ -747,7 +747,7 @@ mod test {
                 },
                 ncpus: InstanceCpuCount(2),
                 memory: ByteCount::from_gibibytes_u32(2),
-                hostname: String::from(INSTANCE_NAME),
+                hostname: INSTANCE_NAME.parse().unwrap(),
                 user_data: b"#cloud-config".to_vec(),
                 ssh_public_keys: Some(Vec::new()),
                 network_interfaces:

nexus/src/app/sagas/snapshot_create.rs

@@ -1940,7 +1940,7 @@ mod test {
                 },
                 ncpus: InstanceCpuCount(2),
                 memory: ByteCount::from_gibibytes_u32(1),
-                hostname: String::from("base_instance"),
+                hostname: "base-instance".parse().unwrap(),
                 user_data:
                     b"#cloud-config\nsystem_info:\n  default_user:\n    name: oxide"
                         .to_vec(),

nexus/test-utils/src/resource_helpers.rs

@@ -518,7 +518,7 @@ pub async fn create_instance_with(
             },
             ncpus: InstanceCpuCount(4),
             memory: ByteCount::from_gibibytes_u32(1),
-            hostname: String::from("the_host"),
+            hostname: "the-host".parse().unwrap(),
             user_data:
                 b"#cloud-config\nsystem_info:\n  default_user:\n    name: oxide"
                     .to_vec(),
@@ -532,6 +532,23 @@ pub async fn create_instance_with(
     .await
 }
 
+/// Creates an instance, asserting a status code and returning the error.
+///
+/// Note that this accepts any serializable body, which allows users to create
+/// invalid inputs to test our parameter validation.
+pub async fn create_instance_with_error<T>(
+    client: &ClientTestContext,
+    project_name: &str,
+    body: &T,
+    status: StatusCode,
+) -> HttpErrorResponseBody
+where
+    T: serde::Serialize,
+{
+    let url = format!("/v1/instances?project={project_name}");
+    object_create_error(client, &url, body, status).await
+}
+
 pub async fn create_vpc(
     client: &ClientTestContext,
     project_name: &str,

nexus/tests/integration_tests/endpoints.rs

@@ -421,7 +421,7 @@ pub static DEMO_INSTANCE_CREATE: Lazy<params::InstanceCreate> =
         },
         ncpus: InstanceCpuCount(1),
         memory: ByteCount::from_gibibytes_u32(16),
-        hostname: String::from("demo-instance"),
+        hostname: "demo-instance".parse().unwrap(),
         user_data: vec![],
         ssh_public_keys: Some(Vec::new()),
         network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,