diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index a2240306d2..6bd7fa32de 100644 --- a/certificates/src/lib.rs +++ b/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>( &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:?}" diff --git a/nexus/db-model/src/certificate.rs b/nexus/db-model/src/certificate.rs index 18faa0d37d..2cd1bcf08a 100644 --- a/nexus/db-model/src/certificate.rs +++ b/nexus/db-model/src/certificate.rs @@ -45,15 +45,14 @@ impl Certificate { id: Uuid, service: ServiceKind, params: params::CertificateCreate, + possible_dns_names: &[String], ) -> Result { 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)) diff --git a/nexus/db-queries/src/db/datastore/dns.rs b/nexus/db-queries/src/db/datastore/dns.rs index 5cdc031a1b..3b39961a40 100644 --- a/nexus/db-queries/src/db/datastore/dns.rs +++ b/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( + pub async fn dns_zones_list_all( + &self, + opctx: &OpContext, + dns_group: DnsGroup, + ) -> ListResultVec { + 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( &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( &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 { diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index e507550fa9..54346b31c0 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/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 + 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, { 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, Error> { + ) -> Result<(Vec, Vec), 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; 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,\ diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index fd86af8a6b..ed2b97257e 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/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 { 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 where @@ -253,6 +256,7 @@ impl DataStore { Uuid::new_v4(), ServiceKind::Nexus, c, + new_silo_dns_names, ) }) .collect::, _>>() diff --git a/nexus/src/app/certificate.rs b/nexus/src/app/certificate.rs index 200039b6f2..71be93f5b7 100644 --- a/nexus/src/app/certificate.rs +++ b/nexus/src/app/certificate.rs @@ -46,12 +46,17 @@ impl super::Nexus { .authn .silo_required() .internal_context("creating a Certificate")?; + + let silo_fq_dns_names = + self.silo_fq_dns_names(opctx, authz_silo.id()).await?; + let kind = params.service; let new_certificate = db::model::Certificate::new( authz_silo.id(), Uuid::new_v4(), kind.into(), params, + &silo_fq_dns_names, )?; let cert = self .db_datastore diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index c96258de0d..f95c64d3eb 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -933,6 +933,7 @@ mod test { Uuid::new_v4(), ServiceKind::Nexus, cert_create, + &["dummy.sys.oxide1.test".to_string()], ) .unwrap(); let ee2 = ExternalEndpoints::new(vec![], vec![cert], vec![]); @@ -962,6 +963,7 @@ mod test { Uuid::new_v4(), ServiceKind::Nexus, cert_create, + &["dummy.sys.oxide1.test".to_string()], ) .unwrap(); @@ -1095,6 +1097,7 @@ mod test { Uuid::new_v4(), ServiceKind::Nexus, silo1_cert1_params, + &["silo1.sys.oxide1.test".to_string()], ) .unwrap(); let silo1_cert2_params = @@ -1120,6 +1123,7 @@ mod test { Uuid::new_v4(), ServiceKind::Nexus, silo2_cert2_params, + &["silo2.sys.oxide1.test".to_string()], ) .unwrap(); let silo3_cert_params = @@ -1129,6 +1133,7 @@ mod test { Uuid::new_v4(), ServiceKind::Nexus, silo3_cert_params, + &["silo3.sys.oxide1.test".to_string()], ) .unwrap(); // Corrupt a byte of this last certificate. (This has to be done after diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 2165f25a5e..849f689903 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -164,7 +164,10 @@ impl super::Nexus { format!("create silo: {:?}", silo_name), self.id.to_string(), ); - dns_update.add_name(silo_dns_name(silo_name), dns_records)?; + let silo_dns_name = silo_dns_name(silo_name); + let recovery_silo_fq_dns_name = + format!("{silo_dns_name}.{}", request.external_dns_zone_name); + dns_update.add_name(silo_dns_name, dns_records)?; // Administrators of the Recovery Silo are automatically made // administrators of the Fleet. @@ -196,6 +199,7 @@ impl super::Nexus { internal_dns, external_dns, recovery_silo, + recovery_silo_fq_dns_name, recovery_user_id: request.recovery_silo.user_name, recovery_user_password_hash: request .recovery_silo diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index b07764b299..a53f20ec79 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -65,6 +65,25 @@ impl super::Nexus { } } + pub(crate) async fn silo_fq_dns_names( + &self, + opctx: &OpContext, + silo_id: Uuid, + ) -> ListResultVec { + let (_, silo) = + self.silo_lookup(opctx, silo_id.into())?.fetch().await?; + let silo_dns_name = silo_dns_name(&silo.name()); + let external_dns_zones = self + .db_datastore + .dns_zones_list_all(opctx, nexus_db_model::DnsGroup::External) + .await?; + + Ok(external_dns_zones + .into_iter() + .map(|zone| format!("{silo_dns_name}.{}", zone.zone_name)) + .collect()) + } + pub(crate) async fn silo_create( &self, opctx: &OpContext, @@ -79,9 +98,9 @@ impl super::Nexus { // Set up an external DNS name for this Silo's API and console // endpoints (which are the same endpoint). - let dns_records: Vec = datastore - .nexus_external_addresses(nexus_opctx) - .await? + let (nexus_external_ips, nexus_external_dns_zones) = + datastore.nexus_external_addresses(nexus_opctx).await?; + let dns_records: Vec = nexus_external_ips .into_iter() .map(|addr| match addr { IpAddr::V4(addr) => DnsRecord::A(addr), @@ -95,10 +114,22 @@ impl super::Nexus { format!("create silo: {:?}", silo_name), self.id.to_string(), ); - dns_update.add_name(silo_dns_name(silo_name), dns_records)?; + let silo_dns_name = silo_dns_name(silo_name); + let new_silo_dns_names = nexus_external_dns_zones + .into_iter() + .map(|zone| format!("{silo_dns_name}.{}", zone.zone_name)) + .collect::>(); + + dns_update.add_name(silo_dns_name, dns_records)?; let silo = datastore - .silo_create(&opctx, &nexus_opctx, new_silo_params, dns_update) + .silo_create( + &opctx, + &nexus_opctx, + new_silo_params, + &new_silo_dns_names, + dns_update, + ) .await?; self.background_tasks .activate(&self.background_tasks.task_external_dns_config); diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index fb88ac83a5..3c3247790a 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -93,6 +93,10 @@ pub struct ControlPlaneTestContext { } impl ControlPlaneTestContext { + pub fn wildcard_silo_dns_name(&self) -> String { + format!("*.sys.{}", self.external_dns_zone_name) + } + pub async fn teardown(mut self) { self.server.close().await; self.database.cleanup().await.unwrap(); diff --git a/nexus/tests/integration_tests/certificates.rs b/nexus/tests/integration_tests/certificates.rs index a35b4dec5c..9532dacf92 100644 --- a/nexus/tests/integration_tests/certificates.rs +++ b/nexus/tests/integration_tests/certificates.rs @@ -150,7 +150,7 @@ async fn test_crud(cptestctx: &ControlPlaneTestContext) { let certs = certs_list(&client).await; assert!(certs.is_empty()); - let chain = CertificateChain::new(); + let chain = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); let (cert, key) = (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem()); @@ -190,14 +190,19 @@ async fn test_cannot_create_certificate_with_bad_key( ) { let client = &cptestctx.external_client; - let chain = CertificateChain::new(); + let chain = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); let (cert, der_key) = (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_der()); let key = String::from_utf8_lossy(&der_key).to_string(); // Cannot create a certificate with a bad key (e.g. not PEM encoded) - cert_create_expect_error(&client, CERT_NAME, cert, key).await; + let error = cert_create_expect_error(&client, CERT_NAME, cert, key).await; + let expected = "Failed to parse private key"; + assert!( + error.contains(expected), + "{error:?} does not contain {expected:?}" + ); } #[nexus_test] @@ -206,14 +211,19 @@ async fn test_cannot_create_certificate_with_mismatched_key( ) { let client = &cptestctx.external_client; - let chain1 = CertificateChain::new(); + let chain1 = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); let cert1 = chain1.cert_chain_as_pem(); - let chain2 = CertificateChain::new(); + let chain2 = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); let key2 = chain2.end_cert_private_key_as_pem(); // Cannot create a certificate with a key that doesn't match the cert - cert_create_expect_error(&client, CERT_NAME, cert1, key2).await; + let error = cert_create_expect_error(&client, CERT_NAME, cert1, key2).await; + let expected = "Certificate and private key do not match"; + assert!( + error.contains(expected), + "{error:?} does not contain {expected:?}" + ); } #[nexus_test] @@ -222,7 +232,7 @@ async fn test_cannot_create_certificate_with_bad_cert( ) { let client = &cptestctx.external_client; - let chain = CertificateChain::new(); + let chain = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); let (cert, key) = (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem()); @@ -231,7 +241,15 @@ async fn test_cannot_create_certificate_with_bad_cert( let cert = String::from_utf8(tmp).unwrap(); - cert_create_expect_error(&client, CERT_NAME, cert, key).await; + let error = cert_create_expect_error(&client, CERT_NAME, cert, key).await; + + // TODO-correctness It's suprising this is the error we get back instead of + // "Failed to parse certificate". Why? + let expected = "Certificate exists, but is empty"; + assert!( + error.contains(expected), + "{error:?} does not contain {expected:?}" + ); } #[nexus_test] @@ -240,14 +258,45 @@ async fn test_cannot_create_certificate_with_expired_cert( ) { let client = &cptestctx.external_client; - let mut params = rcgen::CertificateParams::new(vec!["localhost".into()]); + let mut params = + rcgen::CertificateParams::new(vec![cptestctx.wildcard_silo_dns_name()]); params.not_after = std::time::SystemTime::UNIX_EPOCH.into(); let chain = CertificateChain::with_params(params); let (cert, key) = (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem()); - cert_create_expect_error(&client, CERT_NAME, cert, key).await; + let error = cert_create_expect_error(&client, CERT_NAME, cert, key).await; + let expected = "Certificate exists, but is expired"; + assert!( + error.contains(expected), + "{error:?} does not contain {expected:?}" + ); +} + +#[nexus_test] +async fn test_cannot_create_certificate_with_incorrect_subject_alt_name( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + let bad_subject_alt_name = + format!("some-other-silo.sys.{}", cptestctx.external_dns_zone_name); + + let chain = CertificateChain::new(&bad_subject_alt_name); + let (cert, key) = + (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem()); + + let error = cert_create_expect_error(&client, CERT_NAME, cert, key).await; + for expected in [ + "Certificate not valid for".to_string(), + format!("SANs: {bad_subject_alt_name}"), + ] { + assert!( + error.contains(&expected), + "{error:?} does not contain {expected:?}" + ); + } } #[tokio::test] diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 2b802ef5e6..e04d26cc45 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -10,6 +10,7 @@ use crate::integration_tests::unauthorized::HTTP_SERVER; use chrono::Utc; use http::method::Method; +use internal_dns::names::DNS_ZONE_EXTERNAL_TESTING; use lazy_static::lazy_static; use nexus_db_queries::authn; use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; @@ -334,7 +335,8 @@ lazy_static! { pub static ref DEMO_CERTIFICATES_URL: String = format!("/v1/certificates"); pub static ref DEMO_CERTIFICATE_URL: String = format!("/v1/certificates/demo-certificate"); - pub static ref DEMO_CERTIFICATE: CertificateChain = CertificateChain::new(); + pub static ref DEMO_CERTIFICATE: CertificateChain = + CertificateChain::new(format!("*.sys.{DNS_ZONE_EXTERNAL_TESTING}")); pub static ref DEMO_CERTIFICATE_CREATE: params::CertificateCreate = params::CertificateCreate { identity: IdentityMetadataCreateParams { diff --git a/package/src/bin/omicron-package.rs b/package/src/bin/omicron-package.rs index 959d8e204a..a0146eee50 100644 --- a/package/src/bin/omicron-package.rs +++ b/package/src/bin/omicron-package.rs @@ -185,7 +185,9 @@ async fn do_target( subcommand: &TargetCommand, ) -> Result<()> { let target_dir = artifact_dir.join("target"); - tokio::fs::create_dir_all(&target_dir).await?; + tokio::fs::create_dir_all(&target_dir).await.with_context(|| { + format!("failed to create directory {}", target_dir.display()) + })?; match subcommand { TargetCommand::Create { image, machine, switch } => { let target = KnownTarget::new( @@ -195,7 +197,11 @@ async fn do_target( )?; let path = get_single_target(&target_dir, name).await?; - tokio::fs::write(&path, Target::from(target).to_string()).await?; + tokio::fs::write(&path, Target::from(target).to_string()) + .await + .with_context(|| { + format!("failed to write target to {}", path.display()) + })?; replace_active_link(&name, &target_dir).await?; @@ -263,7 +269,13 @@ async fn replace_active_link( bail!("Target file {} does not exist", src.display()); } let _ = tokio::fs::remove_file(&dst).await; - tokio::fs::symlink(src, dst).await?; + tokio::fs::symlink(src, &dst).await.with_context(|| { + format!( + "failed creating symlink to {} at {}", + src.display(), + dst.display() + ) + })?; Ok(()) } @@ -881,7 +893,9 @@ async fn main() -> Result<()> { if let Ok(manifest) = env::var("CARGO_MANIFEST_DIR") { let manifest_dir = PathBuf::from(manifest); let root = manifest_dir.parent().unwrap(); - env::set_current_dir(&root)?; + env::set_current_dir(root).with_context(|| { + format!("failed to set current directory to {}", root.display()) + })?; } match &args.subcommand { diff --git a/test-utils/src/certificates.rs b/test-utils/src/certificates.rs index 08698aabd5..ab84f30b15 100644 --- a/test-utils/src/certificates.rs +++ b/test-utils/src/certificates.rs @@ -13,8 +13,9 @@ pub struct CertificateChain { } impl CertificateChain { - pub fn new() -> Self { - let params = rcgen::CertificateParams::new(vec!["localhost".into()]); + pub fn new>(subject_alt_name: S) -> Self { + let params = + rcgen::CertificateParams::new(vec![subject_alt_name.into()]); Self::with_params(params) } diff --git a/wicketd/src/rss_config.rs b/wicketd/src/rss_config.rs index 8e6709a30f..1dc9f84985 100644 --- a/wicketd/src/rss_config.rs +++ b/wicketd/src/rss_config.rs @@ -556,10 +556,14 @@ impl CertificateValidator { } fn validate(&self, cert: &str, key: &str) -> Result<(), CertificateError> { - self.inner.validate( - cert.as_bytes(), - key.as_bytes(), - self.silo_dns_name.as_deref(), - ) + // Cert validation accepts multiple possible silo DNS names, but at rack + // setup time we only have one. Stuff it into a Vec. + let silo_dns_names = + if let Some(silo_dns_name) = self.silo_dns_name.as_deref() { + vec![silo_dns_name] + } else { + vec![] + }; + self.inner.validate(cert.as_bytes(), key.as_bytes(), &silo_dns_names) } }