Skip to content

Commit

Permalink
Merge branch 'main' into omdb-sleds
Browse files Browse the repository at this point in the history
  • Loading branch information
@smklein
smklein committed Sep 21, 2023
2 parents f21b835 + 373ddbd commit 0891329
Show file tree
Hide file tree
Showing 15 changed files with 271 additions and 68 deletions.
98 changes: 71 additions & 27 deletions certificates/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)")]
Expand Down Expand Up @@ -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)
Expand All @@ -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!(
Expand All @@ -152,7 +162,7 @@ impl CertificateValidator {
)
});
return Err(CertificateError::NoDnsNameMatchingHostname {
hostname: hostname.to_string(),
hostname: possible_hostnames.join(", "),
cert_description,
});
}
Expand Down Expand Up @@ -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,
)
}

Expand All @@ -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} \
Expand All @@ -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}"
Expand Down Expand Up @@ -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} \
Expand All @@ -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}"
Expand Down Expand Up @@ -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"
),
Expand All @@ -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";
Expand All @@ -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:?}"
);
}
Expand All @@ -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:?}"
Expand Down
5 changes: 2 additions & 3 deletions nexus/db-model/src/certificate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
19 changes: 15 additions & 4 deletions nexus/db-queries/src/db/datastore/dns.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
38 changes: 32 additions & 6 deletions nexus/db-queries/src/db/datastore/rack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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)?;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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())),
Expand All @@ -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)
}
Expand Down Expand Up @@ -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,\
Expand Down
4 changes: 4 additions & 0 deletions nexus/db-queries/src/db/datastore/silo.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?;
Expand All @@ -131,6 +132,7 @@ impl DataStore {
opctx,
nexus_opctx,
new_silo_params,
new_silo_dns_names,
dns_update,
)
.await
Expand All @@ -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
Expand Down Expand Up @@ -253,6 +256,7 @@ impl DataStore {
Uuid::new_v4(),
ServiceKind::Nexus,
c,
new_silo_dns_names,
)
})
.collect::<Result<Vec<_>, _>>()
Expand Down
Loading

0 comments on commit 0891329

Please sign in to comment.