From 22a0179d9172b1d3d6b184d2389bdbfbb05b0300 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 14 Sep 2023 14:28:30 -0400 Subject: [PATCH] [wicket] Add hostname and purpose checks to uploaded SSL certs (#4086) This is a rework of #3436; I think it's enough different that it warrants a separate review. Many of the comments on the previous PR were on the bits where I was implementing the checks; this version leans much more heavily on OpenSSL to do those checks. This addresses the initial bit of #4045 (validating names on certs prior to RSS), but not the full bit: we should also validate names when creating silos. That didn't look completely trivial to plumb through, so I left a `TODO` and will try to tackle that in a followup PR. --- Cargo.lock | 14 +- Cargo.toml | 1 + certificates/Cargo.toml | 7 + certificates/src/lib.rs | 306 +++++++++++++++++- certificates/src/openssl_ext.rs | 125 +++++++ nexus/db-model/src/certificate.rs | 9 +- nexus/tests/integration_tests/certificates.rs | 85 +---- nexus/tests/integration_tests/endpoints.rs | 2 +- test-utils/Cargo.toml | 3 + test-utils/src/certificates.rs | 89 +++++ test-utils/src/lib.rs | 1 + wicketd/src/http_entrypoints.rs | 2 +- wicketd/src/rss_config.rs | 93 +++++- 13 files changed, 621 insertions(+), 116 deletions(-) create mode 100644 certificates/src/openssl_ext.rs create mode 100644 test-utils/src/certificates.rs diff --git a/Cargo.lock b/Cargo.lock index 2bcc144406..33b5567dba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -399,7 +399,7 @@ dependencies = [ "cfg-if 1.0.0", "libc", "miniz_oxide", - "object 0.32.0", + "object 0.32.1", "rustc-demangle", ] @@ -4678,9 +4678,9 @@ dependencies = [ [[package]] name = "object" -version = "0.32.0" +version = "0.32.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77ac5bbd07aea88c60a577a1ce218075ffd59208b2d7ca97adf9bfc5aeb21ebe" +checksum = "9cf5f9dd3933bd50a9e1f149ec995f39ae2c496d31fd772c1fd45ebc27e902b0" dependencies = [ "memchr", ] @@ -4700,8 +4700,13 @@ dependencies = [ name = "omicron-certificates" version = "0.1.0" dependencies = [ + "display-error-chain", + "foreign-types 0.3.2", "omicron-common 0.1.0", + "omicron-test-utils", "openssl", + "openssl-sys", + "rcgen", "thiserror", ] @@ -5178,8 +5183,11 @@ dependencies = [ "http", "libc", "omicron-common 0.1.0", + "pem", + "rcgen", "regex", "reqwest", + "rustls", "slog", "subprocess", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 1f00842e4b..7f4c4767d7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -181,6 +181,7 @@ expectorate = "1.0.7" fatfs = "0.3.6" flate2 = "1.0.27" flume = "0.11.0" +foreign-types = "0.3.2" fs-err = "2.9.0" futures = "0.3.28" gateway-client = { path = "gateway-client" } diff --git a/certificates/Cargo.toml b/certificates/Cargo.toml index 698a192930..29c4a8bc2e 100644 --- a/certificates/Cargo.toml +++ b/certificates/Cargo.toml @@ -5,7 +5,14 @@ edition = "2021" license = "MPL-2.0" [dependencies] +display-error-chain.workspace = true +foreign-types.workspace = true openssl.workspace = true +openssl-sys.workspace = true thiserror.workspace = true omicron-common.workspace = true + +[dev-dependencies] +omicron-test-utils.workspace = true +rcgen.workspace = true diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index fed97db12c..a2240306d2 100644 --- a/certificates/src/lib.rs +++ b/certificates/src/lib.rs @@ -4,15 +4,21 @@ //! Utilities for validating X509 certificates, used by both nexus and wicketd. +use display_error_chain::DisplayErrorChain; use omicron_common::api::external::Error; use openssl::asn1::Asn1Time; use openssl::pkey::PKey; use openssl::x509::X509; +use std::ffi::CString; + +mod openssl_ext; + +use openssl_ext::X509Ext; #[derive(Debug, thiserror::Error)] pub enum CertificateError { - #[error("Failed to parse certificate: {0}")] - BadCertificate(openssl::error::ErrorStack), + #[error("Failed to parse certificate")] + BadCertificate(#[source] openssl::error::ErrorStack), #[error("Certificate exists, but is empty")] CertificateEmpty, @@ -21,31 +27,49 @@ pub enum CertificateError { CertificateExpired, #[error("Failed to parse private key")] - BadPrivateKey(openssl::error::ErrorStack), + BadPrivateKey(#[source] openssl::error::ErrorStack), #[error("Certificate and private key do not match")] Mismatch, - #[error("Unexpected error: {0}")] - Unexpected(openssl::error::ErrorStack), + #[error("Hostname provided for validation is invalid: {0:?}")] + InvalidValidationHostname(String), + + #[error("Error validating certificate hostname")] + ErrorValidatingHostname(#[source] openssl::error::ErrorStack), + + #[error("Certificate not valid for {hostname:?}: {cert_description:?}")] + NoDnsNameMatchingHostname { hostname: String, cert_description: String }, + + #[error("Unsupported certificate purpose (not usable for server auth)")] + UnsupportedPurpose, + + #[error("Unexpected error")] + Unexpected(#[source] openssl::error::ErrorStack), } impl From for Error { fn from(error: CertificateError) -> Self { use CertificateError::*; match error { - BadCertificate(_) | CertificateEmpty | CertificateExpired - | Mismatch => Error::InvalidValue { + BadCertificate(_) + | CertificateEmpty + | CertificateExpired + | Mismatch + | InvalidValidationHostname(_) + | ErrorValidatingHostname(_) + | NoDnsNameMatchingHostname { .. } + | UnsupportedPurpose => Error::InvalidValue { label: String::from("certificate"), - message: error.to_string(), + message: DisplayErrorChain::new(&error).to_string(), }, BadPrivateKey(_) => Error::InvalidValue { label: String::from("private-key"), - message: error.to_string(), + message: DisplayErrorChain::new(&error).to_string(), + }, + Unexpected(_) => Error::InternalError { + internal_message: DisplayErrorChain::new(&error).to_string(), }, - Unexpected(_) => { - Error::InternalError { internal_message: error.to_string() } - } } } } @@ -78,14 +102,16 @@ impl CertificateValidator { /// /// `key` is expected to be the private key for the leaf certificate of /// `certs` in PEM format. - // - // TODO-completeness: Can we take an optional hostname and validate that - // this cert will work for it? (This might be a parameter to this method, or - // could be an additional property on `Self`.) + /// + /// 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( &self, certs: &[u8], key: &[u8], + hostname: Option<&str>, ) -> Result<(), CertificateError> { // Checks on the certs themselves. let mut certs = X509::stack_from_pem(certs) @@ -108,6 +134,39 @@ 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)? + { + let cert_description = + cert.hostname_description().unwrap_or_else(|err| { + format!( + "Error reading cert hostname: {}", + DisplayErrorChain::new(&err) + ) + }); + return Err(CertificateError::NoDnsNameMatchingHostname { + hostname: hostname.to_string(), + cert_description, + }); + } + } + + // If the cert has extended key usage bits set at all, require the bit + // for servers (`XKU_SSL_SERVER` corresponds to `id-kp-serverAuth`, + // which is "TLS WWW server authentication" from RFC 5280). + if let Some(extended_key_usage) = cert.extended_key_usage() { + if extended_key_usage & openssl_sys::XKU_SSL_SERVER == 0 { + return Err(CertificateError::UnsupportedPurpose); + } + } + // Checks on the private key. let key = PKey::private_key_from_pem(key) .map_err(CertificateError::BadPrivateKey)?; @@ -125,3 +184,218 @@ impl CertificateValidator { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use omicron_test_utils::certificates::CertificateChain; + use rcgen::CertificateParams; + use rcgen::DistinguishedName; + use rcgen::DnType; + use rcgen::ExtendedKeyUsagePurpose; + use rcgen::SanType; + + fn validate_cert_with_params( + params: CertificateParams, + hostname: Option<&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, + ) + } + + #[test] + fn test_subject_alternate_names_are_validated() { + // Expected-successful matches + for (dns_name, hostname) in &[ + ("oxide.computer", "oxide.computer"), + ("*.oxide.computer", "*.oxide.computer"), + ("*.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, Some(hostname)) { + Ok(()) => (), + Err(err) => panic!( + "certificate with SAN {dns_name} \ + failed to validate for hostname {hostname}: {err}" + ), + } + } + + // Expected-unsuccessful matches + for &(dns_name, server_hostname) in &[ + ("oxide.computer", "foo.oxide.computer"), + ("oxide.computer", "*.oxide.computer"), + ("*.oxide.computer", "foo.bar.oxide.computer"), + ] { + 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)) { + Ok(()) => panic!( + "certificate with SAN {dns_name} unexpectedly \ + passed validation for hostname {server_hostname}" + ), + Err(CertificateError::NoDnsNameMatchingHostname { + hostname, + cert_description, + }) => { + assert_eq!(hostname, server_hostname); + assert_eq!(cert_description, format!("SANs: {dns_name}")); + } + Err(err) => panic!( + "certificate with SAN {dns_name} \ + validation failed with unexpected error {err}" + ), + } + } + } + + #[test] + fn test_common_name_is_validated() { + // Expected-successful matches + for &(dns_name, hostname) in &[ + ("oxide.computer", "oxide.computer"), + ("*.oxide.computer", "*.oxide.computer"), + ("*.oxide.computer", "foo.oxide.computer"), + ] { + let mut dn = DistinguishedName::new(); + dn.push(DnType::CommonName, dns_name); + let mut params = CertificateParams::new([]); + params.distinguished_name = dn; + + match validate_cert_with_params(params, Some(hostname)) { + Ok(()) => (), + Err(err) => panic!( + "certificate with SAN {dns_name} \ + failed to validate for hostname {hostname}: {err}" + ), + } + } + + // Expected-unsuccessful matches + for &(dns_name, server_hostname) in &[ + ("oxide.computer", "foo.oxide.computer"), + ("oxide.computer", "*.oxide.computer"), + ("*.oxide.computer", "foo.bar.oxide.computer"), + ] { + let mut dn = DistinguishedName::new(); + dn.push(DnType::CommonName, dns_name); + let mut params = CertificateParams::new([]); + params.distinguished_name = dn; + + match validate_cert_with_params(params, Some(server_hostname)) { + Ok(()) => panic!( + "certificate with SAN {dns_name} unexpectedly \ + passed validation for hostname {server_hostname}" + ), + Err(CertificateError::NoDnsNameMatchingHostname { + hostname, + cert_description, + }) => { + assert_eq!(hostname, server_hostname); + assert_eq!(cert_description, format!("CN: {dns_name}")); + } + Err(err) => panic!( + "certificate with SAN {dns_name} \ + validation failed with unexpected error {err}" + ), + } + } + } + + #[test] + fn common_name_is_ignored_if_subject_alternate_names_exist() { + // Set a common name that will pass validation, but a SAN that will not. + // If a SAN exists, the CN should not be used in validation. + const COMMON_NAME: &str = "*.oxide.computer"; + const SUBJECT_ALT_NAME: &str = "bar.oxide.computer"; + const HOSTNAME: &str = "foo.oxide.computer"; + + let mut dn = DistinguishedName::new(); + dn.push(DnType::CommonName, COMMON_NAME); + + let mut params = CertificateParams::new([]); + params.distinguished_name = dn; + + params.subject_alt_names = + vec![SanType::DnsName(SUBJECT_ALT_NAME.to_string())]; + + match validate_cert_with_params(params, Some(HOSTNAME)) { + Ok(()) => panic!( + "certificate unexpectedly passed validation for hostname" + ), + Err(CertificateError::NoDnsNameMatchingHostname { + hostname, + cert_description, + }) => { + assert_eq!(hostname, HOSTNAME); + assert_eq!( + cert_description, + format!("SANs: {SUBJECT_ALT_NAME}") + ); + } + Err(err) => panic!( + "certificate validation failed with unexpected error {err}" + ), + } + } + + #[test] + fn test_cert_extended_key_usage() { + const HOST: &str = "foo.oxide.computer"; + + let valid_ext_key_usage = vec![ + vec![], + // Restore once https://github.com/est31/rcgen/issues/130 is fixed + // vec![ExtendedKeyUsagePurpose::Any], + vec![ExtendedKeyUsagePurpose::ServerAuth], + vec![ + ExtendedKeyUsagePurpose::Any, + ExtendedKeyUsagePurpose::ServerAuth, + ], + vec![ + ExtendedKeyUsagePurpose::ServerAuth, + ExtendedKeyUsagePurpose::ClientAuth, + ], + ]; + + // Valid certs: either no key usage values, or valid ones. + for ext_key_usage in &valid_ext_key_usage { + let mut params = CertificateParams::new(vec![HOST.to_string()]); + params.extended_key_usages = ext_key_usage.clone(); + + assert!( + validate_cert_with_params(params, Some(HOST)).is_ok(), + "unexpected failure with {ext_key_usage:?}" + ); + } + + let invalid_ext_key_usage = vec![ + vec![ExtendedKeyUsagePurpose::ClientAuth], + vec![ExtendedKeyUsagePurpose::EmailProtection], + vec![ + ExtendedKeyUsagePurpose::EmailProtection, + ExtendedKeyUsagePurpose::ClientAuth, + ], + ]; + + for ext_key_usage in &invalid_ext_key_usage { + let mut params = CertificateParams::new(vec![HOST.to_string()]); + params.extended_key_usages = ext_key_usage.clone(); + + assert!( + matches!( + validate_cert_with_params(params, Some(HOST)), + Err(CertificateError::UnsupportedPurpose) + ), + "unexpected success with {ext_key_usage:?}" + ); + } + } +} diff --git a/certificates/src/openssl_ext.rs b/certificates/src/openssl_ext.rs new file mode 100644 index 0000000000..e5fcd2dcbc --- /dev/null +++ b/certificates/src/openssl_ext.rs @@ -0,0 +1,125 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Extensions to the `openssl` and `openssl-sys` crates that have not yet been +//! published upstream. + +use foreign_types::ForeignTypeRef; +use openssl::error::ErrorStack; +use openssl::nid::Nid; +use openssl::x509::X509Ref; +use openssl_sys::X509 as RawX509; +use std::ffi::c_char; +use std::ffi::c_int; +use std::ffi::c_uint; +use std::ffi::CStr; +use std::ptr; + +extern "C" { + // `X509_check_host()` is only exported by `openssl-sys` if the `bindgen` + // feature is enabled + // (https://github.com/sfackler/rust-openssl/issues/2041). For now, we'll + // define the function ourselves; it was added in OpenSSL 1.0.2 and did not + // change in OpenSSL 3.0, and we do not need to support anything older. + fn X509_check_host( + cert: *mut RawX509, + name: *const c_char, + namelen: usize, + flags: c_uint, + peername: *mut *mut c_char, + ) -> c_int; +} + +pub(crate) trait X509Ext { + /// Check whether this cert is valid for the provided `hostname`. + fn valid_for_hostname(&self, hostname: &CStr) -> Result; + + /// Return a description of the hostname(s) of this cert: either the Subject + /// Alternate Names, if present, or the Common Name if not. + fn hostname_description(&self) -> Result; + + /// Returns the extended key usage bitmask from + /// `X509_get_extended_key_usage()` if this cert has an EKU extension. + fn extended_key_usage(&self) -> Option; +} + +impl X509Ext for X509Ref { + fn valid_for_hostname(&self, hostname: &CStr) -> Result { + // Safety: We know `self` is a valid X509Ref and `hostname` is a valid C + // string. We pass a hostname length of 0 which instructs OpenSSL to use + // `strlen()` to check its length. We do not pass any flags and do not + // want the cert name returned to us. + let rc = unsafe { + X509_check_host( + self.as_ptr(), + hostname.as_ptr(), + 0, + 0, + ptr::null_mut(), + ) + }; + + match rc { + 1 => Ok(true), + 0 => Ok(false), + _ => Err(ErrorStack::get()), + } + } + + fn hostname_description(&self) -> Result { + // Most expected case: we have SANs. + if let Some(stack) = self.subject_alt_names() { + let mut dns_names = Vec::new(); + for item in &stack { + if let Some(name) = item.dnsname() { + dns_names.push(name); + } + } + + return if dns_names.is_empty() { + Ok("SANs present but no DNS names found".to_string()) + } else { + Ok(format!("SANs: {}", dns_names.join(", "))) + }; + } + + // Less expected: no SANs, so we check the CN. + let subject_name = self.subject_name(); + let mut common_names = subject_name + .entries_by_nid(Nid::COMMONNAME) + .map(|entry| entry.data().as_utf8()); + + // Check for very unexpected contents: zero or multiple CNs. + let Some(common_name) = common_names.next() else { + return Ok("Neither SANs nor CN present".to_string()); + }; + let common_name = common_name?; + if let Some(next_common_name) = common_names.next() { + let next_common_name = next_common_name?; + let mut all_cns = format!("{}, {}", common_name, next_common_name); + for cn in common_names { + let cn = cn?; + all_cns.push_str(", "); + all_cns.push_str(&cn); + } + return Ok(format!("Multiple CNs: {all_cns}")); + } + + Ok(format!("CN: {common_name}")) + } + + fn extended_key_usage(&self) -> Option { + let extension_flags = + unsafe { openssl_sys::X509_get_extension_flags(self.as_ptr()) }; + + if extension_flags & openssl_sys::EXFLAG_XKUSAGE == 0 { + return None; + } + + let eku = + unsafe { openssl_sys::X509_get_extended_key_usage(self.as_ptr()) }; + + Some(eku) + } +} diff --git a/nexus/db-model/src/certificate.rs b/nexus/db-model/src/certificate.rs index f29cb7862f..18faa0d37d 100644 --- a/nexus/db-model/src/certificate.rs +++ b/nexus/db-model/src/certificate.rs @@ -47,7 +47,14 @@ impl Certificate { params: params::CertificateCreate, ) -> Result { let validator = CertificateValidator::default(); - validator.validate(params.cert.as_bytes(), params.key.as_bytes())?; + + 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, + )?; Ok(Self::new_unvalidated(silo_id, id, service, params)) } diff --git a/nexus/tests/integration_tests/certificates.rs b/nexus/tests/integration_tests/certificates.rs index 649e5f56a7..a35b4dec5c 100644 --- a/nexus/tests/integration_tests/certificates.rs +++ b/nexus/tests/integration_tests/certificates.rs @@ -21,6 +21,7 @@ use nexus_types::external_api::shared; use nexus_types::external_api::views::Certificate; use nexus_types::internal_api::params as internal_params; use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_test_utils::certificates::CertificateChain; use omicron_test_utils::dev::poll::wait_for_condition; use omicron_test_utils::dev::poll::CondCheckError; use oxide_client::ClientSessionExt; @@ -33,90 +34,6 @@ use std::time::Duration; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; -// Utility structure for making a test certificate -pub struct CertificateChain { - root_cert: rustls::Certificate, - intermediate_cert: rustls::Certificate, - end_cert: rustls::Certificate, - end_keypair: rcgen::Certificate, -} - -impl CertificateChain { - pub fn new() -> Self { - let params = rcgen::CertificateParams::new(vec!["localhost".into()]); - Self::with_params(params) - } - - pub fn with_params(params: rcgen::CertificateParams) -> Self { - let mut root_params = rcgen::CertificateParams::new(vec![]); - root_params.is_ca = - rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - let root_keypair = rcgen::Certificate::from_params(root_params) - .expect("failed to generate root keys"); - - let mut intermediate_params = rcgen::CertificateParams::new(vec![]); - intermediate_params.is_ca = - rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - let intermediate_keypair = - rcgen::Certificate::from_params(intermediate_params) - .expect("failed to generate intermediate keys"); - - let end_keypair = rcgen::Certificate::from_params(params) - .expect("failed to generate end-entity keys"); - - let root_cert = rustls::Certificate( - root_keypair - .serialize_der() - .expect("failed to serialize root cert"), - ); - let intermediate_cert = rustls::Certificate( - intermediate_keypair - .serialize_der_with_signer(&root_keypair) - .expect("failed to serialize intermediate cert"), - ); - let end_cert = rustls::Certificate( - end_keypair - .serialize_der_with_signer(&intermediate_keypair) - .expect("failed to serialize end-entity cert"), - ); - - Self { root_cert, intermediate_cert, end_cert, end_keypair } - } - - pub fn end_cert_private_key_as_der(&self) -> Vec { - self.end_keypair.serialize_private_key_der() - } - - pub fn end_cert_private_key_as_pem(&self) -> String { - self.end_keypair.serialize_private_key_pem() - } - - fn cert_chain(&self) -> Vec { - vec![ - self.end_cert.clone(), - self.intermediate_cert.clone(), - self.root_cert.clone(), - ] - } - - pub fn cert_chain_as_pem(&self) -> String { - tls_cert_to_pem(&self.cert_chain()) - } -} - -fn tls_cert_to_pem(certs: &Vec) -> String { - let mut serialized_certs = String::new(); - for cert in certs { - let encoded_cert = pem::encode(&pem::Pem { - tag: "CERTIFICATE".to_string(), - contents: cert.0.clone(), - }); - - serialized_certs.push_str(&encoded_cert); - } - serialized_certs -} - const CERTS_URL: &str = "/v1/certificates"; const CERT_NAME: &str = "my-certificate"; const CERT_NAME2: &str = "my-other-certificate"; diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 4c4366af0a..2b802ef5e6 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -7,7 +7,6 @@ //! This is used for various authz-related tests. //! THERE ARE NO TESTS IN THIS FILE. -use crate::integration_tests::certificates::CertificateChain; use crate::integration_tests::unauthorized::HTTP_SERVER; use chrono::Utc; use http::method::Method; @@ -35,6 +34,7 @@ use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; use omicron_common::api::external::SemverVersion; use omicron_common::api::external::VpcFirewallRuleUpdateParams; +use omicron_test_utils::certificates::CertificateChain; use std::net::IpAddr; use std::net::Ipv4Addr; use std::str::FromStr; diff --git a/test-utils/Cargo.toml b/test-utils/Cargo.toml index e44e25d3ce..b566f6c373 100644 --- a/test-utils/Cargo.toml +++ b/test-utils/Cargo.toml @@ -12,6 +12,8 @@ headers.workspace = true http.workspace = true libc.workspace = true omicron-common.workspace = true +pem.workspace = true +rustls.workspace = true slog.workspace = true subprocess.workspace = true tempfile.workspace = true @@ -19,6 +21,7 @@ thiserror.workspace = true tokio = { workspace = true, features = [ "full" ] } tokio-postgres.workspace = true usdt.workspace = true +rcgen.workspace = true regex.workspace = true reqwest.workspace = true diff --git a/test-utils/src/certificates.rs b/test-utils/src/certificates.rs new file mode 100644 index 0000000000..08698aabd5 --- /dev/null +++ b/test-utils/src/certificates.rs @@ -0,0 +1,89 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Utilities for tests that need certificates. + +// Utility structure for making a test certificate +pub struct CertificateChain { + root_cert: rustls::Certificate, + intermediate_cert: rustls::Certificate, + end_cert: rustls::Certificate, + end_keypair: rcgen::Certificate, +} + +impl CertificateChain { + pub fn new() -> Self { + let params = rcgen::CertificateParams::new(vec!["localhost".into()]); + Self::with_params(params) + } + + pub fn with_params(params: rcgen::CertificateParams) -> Self { + let mut root_params = rcgen::CertificateParams::new(vec![]); + root_params.is_ca = + rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); + let root_keypair = rcgen::Certificate::from_params(root_params) + .expect("failed to generate root keys"); + + let mut intermediate_params = rcgen::CertificateParams::new(vec![]); + intermediate_params.is_ca = + rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); + let intermediate_keypair = + rcgen::Certificate::from_params(intermediate_params) + .expect("failed to generate intermediate keys"); + + let end_keypair = rcgen::Certificate::from_params(params) + .expect("failed to generate end-entity keys"); + + let root_cert = rustls::Certificate( + root_keypair + .serialize_der() + .expect("failed to serialize root cert"), + ); + let intermediate_cert = rustls::Certificate( + intermediate_keypair + .serialize_der_with_signer(&root_keypair) + .expect("failed to serialize intermediate cert"), + ); + let end_cert = rustls::Certificate( + end_keypair + .serialize_der_with_signer(&intermediate_keypair) + .expect("failed to serialize end-entity cert"), + ); + + Self { root_cert, intermediate_cert, end_cert, end_keypair } + } + + pub fn end_cert_private_key_as_der(&self) -> Vec { + self.end_keypair.serialize_private_key_der() + } + + pub fn end_cert_private_key_as_pem(&self) -> String { + self.end_keypair.serialize_private_key_pem() + } + + fn cert_chain(&self) -> Vec { + vec![ + self.end_cert.clone(), + self.intermediate_cert.clone(), + self.root_cert.clone(), + ] + } + + pub fn cert_chain_as_pem(&self) -> String { + tls_cert_to_pem(&self.cert_chain()) + } +} + +fn tls_cert_to_pem(certs: &Vec) -> String { + let mut serialized_certs = String::new(); + for cert in certs { + let encoded_cert = pem::encode(&pem::Pem { + tag: "CERTIFICATE".to_string(), + contents: cert.0.clone(), + }); + + serialized_certs.push_str(&encoded_cert); + } + serialized_certs +} diff --git a/test-utils/src/lib.rs b/test-utils/src/lib.rs index 41c5b487c7..c46fad4130 100644 --- a/test-utils/src/lib.rs +++ b/test-utils/src/lib.rs @@ -12,6 +12,7 @@ use anyhow::anyhow; use anyhow::Context; use headers::authorization::Credentials; +pub mod certificates; pub mod dev; #[macro_use] diff --git a/wicketd/src/http_entrypoints.rs b/wicketd/src/http_entrypoints.rs index 22f0558c43..98cac8dc5d 100644 --- a/wicketd/src/http_entrypoints.rs +++ b/wicketd/src/http_entrypoints.rs @@ -418,7 +418,7 @@ async fn post_run_rack_setup( .map_err(|err| HttpError::for_bad_request(None, format!("{err:#}")))?; let request = { - let config = ctx.rss_config.lock().unwrap(); + let mut config = ctx.rss_config.lock().unwrap(); config.start_rss_request(&ctx.bootstrap_peers, log).map_err(|err| { HttpError::for_bad_request(None, format!("{err:#}")) })? diff --git a/wicketd/src/rss_config.rs b/wicketd/src/rss_config.rs index 55a9b456f0..8e6709a30f 100644 --- a/wicketd/src/rss_config.rs +++ b/wicketd/src/rss_config.rs @@ -13,6 +13,7 @@ use crate::http_entrypoints::CurrentRssUserConfigSensitive; use crate::RackV1Inventory; use anyhow::anyhow; use anyhow::bail; +use anyhow::Context; use anyhow::Result; use bootstrap_agent_client::types::BootstrapAddressDiscovery; use bootstrap_agent_client::types::Certificate; @@ -20,8 +21,9 @@ use bootstrap_agent_client::types::Name; use bootstrap_agent_client::types::RackInitializeRequest; use bootstrap_agent_client::types::RecoverySiloConfig; use bootstrap_agent_client::types::UserId; +use display_error_chain::DisplayErrorChain; use gateway_client::types::SpType; -use omicron_certificates::CertificateValidator; +use omicron_certificates::CertificateError; use omicron_common::address; use omicron_common::address::Ipv4Range; use omicron_common::api::internal::shared::RackNetworkConfig; @@ -127,7 +129,7 @@ impl CurrentRssConfig { } pub(crate) fn start_rss_request( - &self, + &mut self, bootstrap_peers: &BootstrapPeers, log: &slog::Logger, ) -> Result { @@ -153,6 +155,28 @@ impl CurrentRssConfig { if self.external_certificates.is_empty() { bail!("at least one certificate/key pair is required"); } + + // We validated all the external certs as they were uploaded, but if we + // didn't yet have our `external_dns_zone_name` that validation would've + // skipped checking the hostname. Repeat validation on all certs now + // that we definitely have it. + let cert_validator = + CertificateValidator::new(Some(&self.external_dns_zone_name)); + for (i, pair) in self.external_certificates.iter().enumerate() { + if let Err(err) = cert_validator + .validate(&pair.cert, &pair.key) + .with_context(|| { + let i = i + 1; + let tot = self.external_certificates.len(); + format!("certificate {i} of {tot} is invalid") + }) + { + // Remove the invalid cert prior to returning. + self.external_certificates.remove(i); + return Err(err); + } + } + let Some(recovery_silo_password_hash) = self.recovery_silo_password_hash.as_ref() else { @@ -294,15 +318,25 @@ impl CurrentRssConfig { (None, None) => unreachable!(), }; - let mut validator = CertificateValidator::default(); - - // We are running pre-NTP, so we can't check cert expirations; nexus - // will have to do that. - validator.danger_disable_expiration_validation(); + // We store `external_dns_zone_name` as a `String` for simpler TOML + // parsing, but we want to convert an empty string to an option here so + // we don't reject certs if the external DNS zone name hasn't been set + // yet. + let external_dns_zone_name = if self.external_dns_zone_name.is_empty() { + None + } else { + Some(self.external_dns_zone_name.as_str()) + }; - validator - .validate(cert.as_bytes(), key.as_bytes()) - .map_err(|err| err.to_string())?; + // If the certificate is invalid, clear out the cert and key before + // returning an error. + if let Err(err) = CertificateValidator::new(external_dns_zone_name) + .validate(cert, key) + { + self.partial_external_certificate.cert = None; + self.partial_external_certificate.key = None; + return Err(DisplayErrorChain::new(&err).to_string()); + } // Cert and key appear to be valid; steal them out of // `partial_external_certificate` and promote them to @@ -490,3 +524,42 @@ fn validate_rack_network_config( .collect(), }) } + +// Thin wrapper around an `omicron_certificates::CertificateValidator` that we +// use both when certs are uploaded and when we start RSS. +struct CertificateValidator { + inner: omicron_certificates::CertificateValidator, + silo_dns_name: Option, +} + +impl CertificateValidator { + fn new(external_dns_zone_name: Option<&str>) -> Self { + let mut inner = omicron_certificates::CertificateValidator::default(); + + // We are running in 1986! We're in the code path where the operator is + // giving us NTP servers so we can find out the actual time, but any + // validation we attempt now must ignore certificate expiration (and in + // particular, we don't want to fail a "not before" check because we + // think the cert is from the next century). + inner.danger_disable_expiration_validation(); + + // We validate certificates both when they are uploaded and just before + // beginning RSS. In the former case we may not yet know our external + // DNS name (e.g., if certs are uploaded before the config TOML that + // provides the DNS name), but in the latter case we do. + let silo_dns_name = + external_dns_zone_name.map(|external_dns_zone_name| { + format!("{RECOVERY_SILO_NAME}.sys.{external_dns_zone_name}",) + }); + + Self { inner, silo_dns_name } + } + + fn validate(&self, cert: &str, key: &str) -> Result<(), CertificateError> { + self.inner.validate( + cert.as_bytes(), + key.as_bytes(), + self.silo_dns_name.as_deref(), + ) + } +}