Add hostname validation to certs uploaded to nexus

certificates/src/lib.rs

@@ -9,6 +9,7 @@ use omicron_common::api::external::Error;
 use openssl::asn1::Asn1Time;
 use openssl::pkey::PKey;
 use openssl::x509::X509;
+use std::borrow::Borrow;
 use std::ffi::CString;
 
 mod openssl_ext;
@@ -38,7 +39,7 @@ pub enum CertificateError {
     #[error("Error validating certificate hostname")]
     ErrorValidatingHostname(#[source] openssl::error::ErrorStack),
 
-    #[error("Certificate not valid for {hostname:?}: {cert_description:?}")]
+    #[error("Certificate not valid for given hostnames {hostname:?}: {cert_description}")]
     NoDnsNameMatchingHostname { hostname: String, cert_description: String },
 
     #[error("Unsupported certificate purpose (not usable for server auth)")]
@@ -103,15 +104,15 @@ impl CertificateValidator {
     /// `key` is expected to be the private key for the leaf certificate of
     /// `certs` in PEM format.
     ///
-    /// If `hostname` is not `None`, the leaf certificate of `certs` must be
-    /// valid for `hostname`, as determined by a dNSName entry in its subject
-    /// alternate names or (if there are no dNSName SANs) the cert's common
-    /// name.
-    pub fn validate(
+    /// If `possible_hostnames` is empty, no hostname validation is performed.
+    /// If `possible_hostnames` is not empty, we require _at least one_ of its
+    /// hostnames to match the SANs (or CN, if no SANs are present) of the leaf
+    /// certificate.
+    pub fn validate<S: Borrow<str>>(
         &self,
         certs: &[u8],
         key: &[u8],
-        hostname: Option<&str>,
+        possible_hostnames: &[S],
     ) -> Result<(), CertificateError> {
         // Checks on the certs themselves.
         let mut certs = X509::stack_from_pem(certs)
@@ -134,16 +135,25 @@ impl CertificateValidator {
         // to use with verifying the private key.
         let cert = certs.swap_remove(0);
 
-        if let Some(hostname) = hostname {
-            let c_hostname = CString::new(hostname).map_err(|_| {
-                CertificateError::InvalidValidationHostname(
-                    hostname.to_string(),
-                )
-            })?;
-            if !cert
-                .valid_for_hostname(&c_hostname)
-                .map_err(CertificateError::ErrorValidatingHostname)?
-            {
+        if !possible_hostnames.is_empty() {
+            let mut found_valid_hostname = false;
+            for hostname in possible_hostnames {
+                let hostname = hostname.borrow();
+                let c_hostname = CString::new(hostname).map_err(|_| {
+                    CertificateError::InvalidValidationHostname(
+                        hostname.to_string(),
+                    )
+                })?;
+                if cert
+                    .valid_for_hostname(&c_hostname)
+                    .map_err(CertificateError::ErrorValidatingHostname)?
+                {
+                    found_valid_hostname = true;
+                    break;
+                }
+            }
+
+            if !found_valid_hostname {
                 let cert_description =
                     cert.hostname_description().unwrap_or_else(|err| {
                         format!(
@@ -152,7 +162,7 @@ impl CertificateValidator {
                         )
                     });
                 return Err(CertificateError::NoDnsNameMatchingHostname {
-                    hostname: hostname.to_string(),
+                    hostname: possible_hostnames.join(", "),
                     cert_description,
                 });
             }
@@ -197,13 +207,13 @@ mod tests {
 
     fn validate_cert_with_params(
         params: CertificateParams,
-        hostname: Option<&str>,
+        possible_hostnames: &[&str],
     ) -> Result<(), CertificateError> {
         let cert_chain = CertificateChain::with_params(params);
         CertificateValidator::default().validate(
             cert_chain.cert_chain_as_pem().as_bytes(),
             cert_chain.end_cert_private_key_as_pem().as_bytes(),
-            hostname,
+            possible_hostnames,
         )
     }
 
@@ -218,7 +228,7 @@ mod tests {
             let mut params = CertificateParams::new([]);
             params.subject_alt_names =
                 vec![SanType::DnsName(dns_name.to_string())];
-            match validate_cert_with_params(params, Some(hostname)) {
+            match validate_cert_with_params(params, &[hostname]) {
                 Ok(()) => (),
                 Err(err) => panic!(
                     "certificate with SAN {dns_name} \
@@ -236,7 +246,7 @@ mod tests {
             let mut params = CertificateParams::new([]);
             params.subject_alt_names =
                 vec![SanType::DnsName(dns_name.to_string())];
-            match validate_cert_with_params(params, Some(server_hostname)) {
+            match validate_cert_with_params(params, &[server_hostname]) {
                 Ok(()) => panic!(
                     "certificate with SAN {dns_name} unexpectedly \
                      passed validation for hostname {server_hostname}"
@@ -269,7 +279,7 @@ mod tests {
             let mut params = CertificateParams::new([]);
             params.distinguished_name = dn;
 
-            match validate_cert_with_params(params, Some(hostname)) {
+            match validate_cert_with_params(params, &[hostname]) {
                 Ok(()) => (),
                 Err(err) => panic!(
                     "certificate with SAN {dns_name} \
@@ -289,7 +299,7 @@ mod tests {
             let mut params = CertificateParams::new([]);
             params.distinguished_name = dn;
 
-            match validate_cert_with_params(params, Some(server_hostname)) {
+            match validate_cert_with_params(params, &[server_hostname]) {
                 Ok(()) => panic!(
                     "certificate with SAN {dns_name} unexpectedly \
                      passed validation for hostname {server_hostname}"
@@ -326,7 +336,7 @@ mod tests {
         params.subject_alt_names =
             vec![SanType::DnsName(SUBJECT_ALT_NAME.to_string())];
 
-        match validate_cert_with_params(params, Some(HOSTNAME)) {
+        match validate_cert_with_params(params, &[HOSTNAME]) {
             Ok(()) => panic!(
                 "certificate unexpectedly passed validation for hostname"
             ),
@@ -346,6 +356,40 @@ mod tests {
         }
     }
 
+    #[test]
+    fn cert_validated_if_any_possible_hostname_is_valid() {
+        // Expected-successful matches that contain a mix of valid and invalid
+        // possible hostnames
+        for (dns_name, hostnames) in &[
+            (
+                "oxide.computer",
+                // Since "any valid hostname" is allowed, an empty list of
+                // hostnames is also allowed
+                &[] as &[&str],
+            ),
+            ("oxide.computer", &["oxide.computer", "not-oxide.computer"]),
+            (
+                "*.oxide.computer",
+                &["*.oxide.computer", "foo.bar.oxide.computer"],
+            ),
+            (
+                "*.oxide.computer",
+                &["foo.bar.not-oxide.computer", "foo.oxide.computer"],
+            ),
+        ] {
+            let mut params = CertificateParams::new([]);
+            params.subject_alt_names =
+                vec![SanType::DnsName(dns_name.to_string())];
+            match validate_cert_with_params(params, hostnames) {
+                Ok(()) => (),
+                Err(err) => panic!(
+                    "certificate with SAN {dns_name} \
+                     failed to validate for hostname {hostnames:?}: {err}"
+                ),
+            }
+        }
+    }
+
     #[test]
     fn test_cert_extended_key_usage() {
         const HOST: &str = "foo.oxide.computer";
@@ -371,7 +415,7 @@ mod tests {
             params.extended_key_usages = ext_key_usage.clone();
 
             assert!(
-                validate_cert_with_params(params, Some(HOST)).is_ok(),
+                validate_cert_with_params(params, &[HOST]).is_ok(),
                 "unexpected failure with {ext_key_usage:?}"
             );
         }
@@ -391,7 +435,7 @@ mod tests {
 
             assert!(
                 matches!(
-                    validate_cert_with_params(params, Some(HOST)),
+                    validate_cert_with_params(params, &[HOST]),
                     Err(CertificateError::UnsupportedPurpose)
                 ),
                 "unexpected success with {ext_key_usage:?}"

nexus/db-model/src/certificate.rs

@@ -45,15 +45,14 @@ impl Certificate {
         id: Uuid,
         service: ServiceKind,
         params: params::CertificateCreate,
+        possible_dns_names: &[String],
     ) -> Result<Self, CertificateError> {
         let validator = CertificateValidator::default();
 
         validator.validate(
             params.cert.as_bytes(),
             params.key.as_bytes(),
-            // TODO-correctness: We should pass a hostname here for cert
-            // validation: https://github.com/oxidecomputer/omicron/issues/4045
-            None,
+            possible_dns_names,
         )?;
 
         Ok(Self::new_unvalidated(silo_id, id, service, params))

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

@@ -60,7 +60,18 @@ impl DataStore {
     ///
     /// We do not generally expect there to be more than 1-2 DNS zones in a
     /// group (and nothing today creates more than one).
-    async fn dns_zones_list_all<ConnErr>(
+    pub async fn dns_zones_list_all(
+        &self,
+        opctx: &OpContext,
+        dns_group: DnsGroup,
+    ) -> ListResultVec<DnsZone> {
+        let conn = self.pool_authorized(opctx).await?;
+        self.dns_zones_list_all_on_connection(opctx, conn, dns_group).await
+    }
+
+    /// Variant of [`Self::dns_zones_list_all`] which may be called from a
+    /// transaction context.
+    pub(crate) async fn dns_zones_list_all_on_connection<ConnErr>(
         &self,
         opctx: &OpContext,
         conn: &(impl async_bb8_diesel::AsyncConnection<
@@ -396,7 +407,6 @@ impl DataStore {
     /// **Callers almost certainly want to wake up the corresponding Nexus
     /// background task to cause these changes to be propagated to the
     /// corresponding DNS servers.**
-    #[must_use]
     pub async fn dns_update<ConnErr>(
         &self,
         opctx: &OpContext,
@@ -413,8 +423,9 @@ impl DataStore {
     {
         opctx.authorize(authz::Action::Modify, &authz::DNS_CONFIG).await?;
 
-        let zones =
-            self.dns_zones_list_all(opctx, conn, update.dns_group).await?;
+        let zones = self
+            .dns_zones_list_all_on_connection(opctx, conn, update.dns_group)
+            .await?;
 
         let result = conn
             .transaction_async(|c| async move {

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

@@ -32,6 +32,8 @@ use async_bb8_diesel::PoolError;
 use chrono::Utc;
 use diesel::prelude::*;
 use diesel::upsert::excluded;
+use nexus_db_model::DnsGroup;
+use nexus_db_model::DnsZone;
 use nexus_db_model::ExternalIp;
 use nexus_db_model::IncompleteNetworkInterface;
 use nexus_db_model::InitialDnsGroup;
@@ -65,6 +67,7 @@ pub struct RackInit {
     pub internal_dns: InitialDnsGroup,
     pub external_dns: InitialDnsGroup,
     pub recovery_silo: external_params::SiloCreate,
+    pub recovery_silo_fq_dns_name: String,
     pub recovery_user_id: external_params::UserId,
     pub recovery_user_password_hash: omicron_passwords::PasswordHashString,
     pub dns_update: DnsVersionUpdateBuilder,
@@ -197,6 +200,7 @@ impl DataStore {
         conn: &(impl AsyncConnection<DbConnection, ConnError> + Sync),
         log: &slog::Logger,
         recovery_silo: external_params::SiloCreate,
+        recovery_silo_fq_dns_name: String,
         recovery_user_id: external_params::UserId,
         recovery_user_password_hash: omicron_passwords::PasswordHashString,
         dns_update: DnsVersionUpdateBuilder,
@@ -210,7 +214,14 @@ impl DataStore {
             AsyncConnection<DbConnection, ConnError>,
     {
         let db_silo = self
-            .silo_create_conn(conn, opctx, opctx, recovery_silo, dns_update)
+            .silo_create_conn(
+                conn,
+                opctx,
+                opctx,
+                recovery_silo,
+                &[recovery_silo_fq_dns_name],
+                dns_update,
+            )
             .await
             .map_err(RackInitError::Silo)
             .map_err(TxnError::CustomError)?;
@@ -521,6 +532,7 @@ impl DataStore {
                     &conn,
                     &log,
                     rack_init.recovery_silo,
+                    rack_init.recovery_silo_fq_dns_name,
                     rack_init.recovery_user_id,
                     rack_init.recovery_user_password_hash,
                     rack_init.dns_update,
@@ -594,16 +606,16 @@ impl DataStore {
     pub async fn nexus_external_addresses(
         &self,
         opctx: &OpContext,
-    ) -> Result<Vec<IpAddr>, Error> {
+    ) -> Result<(Vec<IpAddr>, Vec<DnsZone>), Error> {
         opctx.authorize(authz::Action::Read, &authz::DNS_CONFIG).await?;
 
         use crate::db::schema::external_ip::dsl as extip_dsl;
         use crate::db::schema::service::dsl as service_dsl;
-        type TxnError = TransactionError<()>;
+        type TxnError = TransactionError<Error>;
         self.pool_authorized(opctx)
             .await?
             .transaction_async(|conn| async move {
-                Ok(extip_dsl::external_ip
+                let ips = extip_dsl::external_ip
                     .inner_join(
                         service_dsl::service.on(service_dsl::id
                             .eq(extip_dsl::parent_id.assume_not_null())),
@@ -617,11 +629,21 @@ impl DataStore {
                     .await?
                     .into_iter()
                     .map(|external_ip| external_ip.ip.ip())
-                    .collect())
+                    .collect();
+
+                let dns_zones = self
+                    .dns_zones_list_all_on_connection(
+                        opctx,
+                        &conn,
+                        DnsGroup::External,
+                    )
+                    .await?;
+
+                Ok((ips, dns_zones))
             })
             .await
             .map_err(|error: TxnError| match error {
-                TransactionError::CustomError(()) => unimplemented!(),
+                TransactionError::CustomError(err) => err,
                 TransactionError::Pool(e) => {
                     public_error_from_diesel_pool(e, ErrorHandler::Server)
                 }
@@ -699,6 +721,10 @@ mod test {
                     tls_certificates: vec![],
                     mapped_fleet_roles: Default::default(),
                 },
+                recovery_silo_fq_dns_name: format!(
+                    "test-silo.sys.{}",
+                    internal_dns::DNS_ZONE
+                ),
                 recovery_user_id: "test-user".parse().unwrap(),
                 // empty string password
                 recovery_user_password_hash: "$argon2id$v=19$m=98304,t=13,\

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

@@ -123,6 +123,7 @@ impl DataStore {
         opctx: &OpContext,
         nexus_opctx: &OpContext,
         new_silo_params: params::SiloCreate,
+        new_silo_dns_names: &[String],
         dns_update: DnsVersionUpdateBuilder,
     ) -> CreateResult<Silo> {
         let conn = self.pool_authorized(opctx).await?;
@@ -131,6 +132,7 @@ impl DataStore {
             opctx,
             nexus_opctx,
             new_silo_params,
+            new_silo_dns_names,
             dns_update,
         )
         .await
@@ -143,6 +145,7 @@ impl DataStore {
         opctx: &OpContext,
         nexus_opctx: &OpContext,
         new_silo_params: params::SiloCreate,
+        new_silo_dns_names: &[String],
         dns_update: DnsVersionUpdateBuilder,
     ) -> CreateResult<Silo>
     where
@@ -253,6 +256,7 @@ impl DataStore {
                         Uuid::new_v4(),
                         ServiceKind::Nexus,
                         c,
+                        new_silo_dns_names,
                     )
                 })
                 .collect::<Result<Vec<_>, _>>()