From 373ddbd57f5d9814b0c1f23aa4d02e8c732af46e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 20 Sep 2023 08:23:01 -0700 Subject: [PATCH] Add hostname validation to certs uploaded to nexus (#4100) This PR changes the `CertificateValidator` from taking "0 or 1" hostname to validate to taking a list of hostnames, and the cert passes validation if _any_ hostname from the list matches. This is to support a future world where we support adding and removing external DNS zone names, where an operator might perform an operation like: 1. The rack has an external name of `myrack.oxide.computer`, and every silo has a `$SILO.sys.myrack.oxide.computer` cert 2. Add a new external name of `something-else.oxide.computer` 3. Upload new certs to all silos with SAN `$SILO.sys.something-else.oxide.computer` 4. Remove the external name `myrack.oxide.computer` To be clear this is a hypothetical world, because today we don't support modifying the external DNS name at all. But step 3 from this sequence is the argument for why the validation logic is "any of the hostnames" instead of "all of the hostnames": we want to add certs for the new hostname (that don't need to also work for the old hostname we're about to remove). The check is performed three places: 1. When the recovery silo is created (which is maybe wrong? I could see an argument for omitting this check, although since wicket is also performing it it should certainly pass) 2. When any other silo is created 3. When a cert is uploaded to an existing silo In all cases we build the list of hostnames for the validator by combining the list of external DNS zone names (which is currently always length 1) with the silo's `.sys` prefix. For the recovery silo specifically we build the list of length 1 by hand; for other silos and when adding to an existing silo, we query CRDB for all external DNS zone names. I haven't yet deployed this to a full system and want to do that before merging, but this passes tests and we're about to have a busy week, so I wanted to go ahead and open it as an easy way to keep it on the radar. Fixes #4045. --- certificates/src/lib.rs | 98 ++++++++++++++----- nexus/db-model/src/certificate.rs | 5 +- nexus/db-queries/src/db/datastore/dns.rs | 19 +++- nexus/db-queries/src/db/datastore/rack.rs | 38 +++++-- nexus/db-queries/src/db/datastore/silo.rs | 4 + nexus/src/app/certificate.rs | 5 + nexus/src/app/external_endpoints.rs | 5 + nexus/src/app/rack.rs | 6 +- nexus/src/app/silo.rs | 41 +++++++- nexus/test-utils/src/lib.rs | 4 + nexus/tests/integration_tests/certificates.rs | 69 +++++++++++-- nexus/tests/integration_tests/endpoints.rs | 4 +- package/src/bin/omicron-package.rs | 22 ++++- test-utils/src/certificates.rs | 5 +- wicketd/src/rss_config.rs | 14 ++- 15 files changed, 271 insertions(+), 68 deletions(-) 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 8bd9d857ac..ddf2718930 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 b1ee6bc452..3ac4b9063d 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.as_str()), 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 fd0414aad3..a6ffd8ef5e 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.as_str()), 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) } }