From 5007ba6f54196f4c71f3379189c0fae2290f4de4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 08:44:26 -0700 Subject: [PATCH 1/9] fm: add certificate diagnosis engine for expiring silo TLS certificates Nexus serves each silo's external API with the silo's certificate whose leaf not_after is latest (ExternalEndpoint::best_certificate). When that certificate is about to expire or has expired, no later-expiring replacement is installed, and until now nothing told the operator. Add a fault management diagnosis engine that predicts the same choice and opens a case per silo when the best certificate is inside a configurable warning window or already expired. The case carries one fact (BestCertificateExpiring or BestCertificateExpired) and requests a silo.certificate.expiring or silo.certificate.expired alert whenever that fact changes. The case closes once a later-expiring certificate is installed or the silo is removed. Silos with no certificates open no case. Like best_certificate, the engine ignores not_before. The warning window is a new FmConfig setting, certificate_expiry_warning_days (default 30, at most 3650), settable via omdb nexus fm-config set --certificate-expiry-warning-days. Supporting changes: - omicron-certificates: leaf_validity parses a PEM chain's leaf not_before/not_after into chrono timestamps. - nexus-types: DiagnosisEngineKind::Certificate, CertificateFact payloads, the ObservedSiloCertificates analysis input, the two alert classes with V0 payloads, and the FmConfig setting. - Schema version 300 (fm-certificate-de): diagnosis_engine and alert_class enum values, the fm_fact_certificate table, and the fm_config column with its CHECK constraint. - nexus-fm: Input carries the FmConfig and observed silo certificates; the fm_analysis task loads them from the silo and certificate tables. --- Cargo.lock | 2 + certificates/Cargo.toml | 1 + certificates/src/lib.rs | 99 +- .../omdb/src/bin/omdb/nexus/fm_config.rs | 17 + dev-tools/omdb/tests/successes.out | 98 +- dev-tools/omdb/tests/usage_errors.out | 19 +- nexus/Cargo.toml | 1 + nexus/db-model/src/alert_class.rs | 8 + nexus/db-model/src/fm.rs | 2 + nexus/db-model/src/fm/config.rs | 9 + nexus/db-model/src/fm/diagnosis_engine.rs | 7 + nexus/db-model/src/fm/fact_certificate.rs | 140 +++ nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-queries/src/db/datastore/fm.rs | 108 +- .../db-queries/src/db/datastore/fm_config.rs | 12 + .../fm_config_insert_latest_version.sql | 5 +- nexus/db-schema/src/enums.rs | 1 + nexus/db-schema/src/schema.rs | 17 + nexus/fm/Cargo.toml | 2 +- nexus/fm/src/analysis_input.rs | 96 +- nexus/fm/src/diagnosis/certificate.rs | 1114 +++++++++++++++++ nexus/fm/src/diagnosis/mod.rs | 2 + nexus/fm/src/diagnosis/power_shelf.rs | 2 + nexus/fm/src/test_util.rs | 9 +- nexus/src/app/background/tasks/fm_analysis.rs | 115 +- .../app/background/tasks/fm_config_load.rs | 1 + .../tasks/fm_sitrep_history_pruner.rs | 1 + nexus/src/app/external_endpoints.rs | 7 + .../silo.certificate.expired/v0.json | 74 ++ .../silo.certificate.expiring/v0.json | 74 ++ .../output/analysis_input_report_empty.out | 2 + .../output/analysis_input_report_same_inv.out | 2 + .../analysis_input_report_with_cases.out | 4 + nexus/types/src/alert.rs | 15 + nexus/types/src/alert/certificate.rs | 85 ++ nexus/types/src/fm.rs | 7 +- nexus/types/src/fm/analysis_reports.rs | 85 ++ nexus/types/src/fm/case.rs | 8 +- nexus/types/src/fm/config.rs | 72 ++ nexus/types/src/fm/fact.rs | 75 ++ nexus/types/src/lib.rs | 1 + nexus/types/src/observed_certificate.rs | 74 ++ openapi/nexus-lockstep.json | 11 +- schema/crdb/dbinit.sql | 80 +- schema/crdb/fm-certificate-de/up1.sql | 1 + schema/crdb/fm-certificate-de/up2.sql | 4 + schema/crdb/fm-certificate-de/up3.sql | 50 + schema/crdb/fm-certificate-de/up4.sql | 1 + schema/crdb/fm-certificate-de/up5.sql | 1 + schema/crdb/fm-certificate-de/up6.sql | 1 + schema/crdb/fm-certificate-de/up7.sql | 6 + schema/crdb/fm-certificate-de/up7.verify.sql | 2 + 52 files changed, 2563 insertions(+), 70 deletions(-) create mode 100644 nexus/db-model/src/fm/fact_certificate.rs create mode 100644 nexus/fm/src/diagnosis/certificate.rs create mode 100644 nexus/types/output/alert_schemas/silo.certificate.expired/v0.json create mode 100644 nexus/types/output/alert_schemas/silo.certificate.expiring/v0.json create mode 100644 nexus/types/src/alert/certificate.rs create mode 100644 nexus/types/src/observed_certificate.rs create mode 100644 schema/crdb/fm-certificate-de/up1.sql create mode 100644 schema/crdb/fm-certificate-de/up2.sql create mode 100644 schema/crdb/fm-certificate-de/up3.sql create mode 100644 schema/crdb/fm-certificate-de/up4.sql create mode 100644 schema/crdb/fm-certificate-de/up5.sql create mode 100644 schema/crdb/fm-certificate-de/up6.sql create mode 100644 schema/crdb/fm-certificate-de/up7.sql create mode 100644 schema/crdb/fm-certificate-de/up7.verify.sql diff --git a/Cargo.lock b/Cargo.lock index 8e905375f73..92c2ca1f592 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8705,6 +8705,7 @@ dependencies = [ name = "omicron-certificates" version = "0.1.0" dependencies = [ + "chrono", "display-error-chain", "foreign-types 0.3.2", "omicron-common", @@ -9248,6 +9249,7 @@ dependencies = [ "nexus-types-versions", "ntp-admin-client", "num-integer", + "omicron-certificates", "omicron-cockroach-metrics", "omicron-common", "omicron-debug-dropbox", diff --git a/certificates/Cargo.toml b/certificates/Cargo.toml index 850993426e9..10a8e9d89b9 100644 --- a/certificates/Cargo.toml +++ b/certificates/Cargo.toml @@ -8,6 +8,7 @@ license = "MPL-2.0" workspace = true [dependencies] +chrono.workspace = true display-error-chain.workspace = true foreign-types.workspace = true openssl.workspace = true diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index cc5e9924733..e7d45d5aaaa 100644 --- a/certificates/src/lib.rs +++ b/certificates/src/lib.rs @@ -4,9 +4,12 @@ //! Utilities for validating X509 certificates, used by both nexus and wicketd. +use chrono::DateTime; +use chrono::Utc; use display_error_chain::DisplayErrorChain; use omicron_common::api::external::Error; use openssl::asn1::Asn1Time; +use openssl::asn1::Asn1TimeRef; use openssl::pkey::PKey; use openssl::x509::X509; use std::borrow::Borrow; @@ -47,6 +50,9 @@ pub enum CertificateError { #[error("Unsupported certificate purpose (not usable for server auth)")] UnsupportedPurpose, + #[error("Certificate validity time is out of the representable range")] + TimeOutOfRange, + #[error("Unexpected error")] Unexpected(#[source] openssl::error::ErrorStack), } @@ -62,7 +68,8 @@ impl From for Error { | InvalidValidationHostname(_) | ErrorValidatingHostname(_) | NoDnsNameMatchingHostname { .. } - | UnsupportedPurpose => Error::invalid_value( + | UnsupportedPurpose + | TimeOutOfRange => Error::invalid_value( "certificate", DisplayErrorChain::new(&error).to_string(), ), @@ -77,6 +84,51 @@ impl From for Error { } } +/// The validity window of an X509 certificate. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct CertificateValidity { + /// The certificate is not valid before this time. + pub not_before: DateTime, + /// The certificate is not valid after this time. + pub not_after: DateTime, +} + +/// Returns the validity window of the leaf certificate in a PEM-encoded +/// certificate chain. +/// +/// The leaf certificate is the first certificate in the chain. This is the +/// same convention used when the chain is served to TLS clients, so the +/// returned window is the one clients will check. +pub fn leaf_validity( + certs_pem: &[u8], +) -> Result { + let certs = X509::stack_from_pem(certs_pem) + .map_err(CertificateError::BadCertificate)?; + let leaf = certs.first().ok_or(CertificateError::CertificateEmpty)?; + Ok(CertificateValidity { + not_before: asn1_time_to_chrono(leaf.not_before())?, + not_after: asn1_time_to_chrono(leaf.not_after())?, + }) +} + +/// Converts an ASN.1 time to a `chrono` timestamp by measuring its offset +/// from the Unix epoch. +/// +/// `Asn1TimeRef` offers no direct conversion to a Unix timestamp, but +/// `ASN1_TIME_diff` can compute the (days, seconds) difference between two +/// ASN.1 times. +fn asn1_time_to_chrono( + time: &Asn1TimeRef, +) -> Result, CertificateError> { + const SECS_PER_DAY: i64 = 24 * 60 * 60; + let epoch = Asn1Time::from_unix(0).map_err(CertificateError::Unexpected)?; + // `diff` computes `time - epoch`, split into whole days and the + // remaining seconds. + let diff = epoch.diff(time).map_err(CertificateError::Unexpected)?; + let secs = i64::from(diff.days) * SECS_PER_DAY + i64::from(diff.secs); + DateTime::from_timestamp(secs, 0).ok_or(CertificateError::TimeOutOfRange) +} + pub struct CertificateValidator { validate_expiration: bool, } @@ -444,4 +496,49 @@ mod tests { ); } } + + #[test] + fn test_leaf_validity_reads_leaf_certificate() { + // Pin the leaf's validity window to exact second offsets from the + // Unix epoch. The root and intermediate certificates in the chain + // keep rcgen's default (much wider) window, so a correct result + // proves we read the leaf and not some other link in the chain. + const NOT_BEFORE_SECS: u64 = 1_000_000_000; + const NOT_AFTER_SECS: u64 = 2_000_000_000; + let mut params = CertificateParams::new(vec![ + "fake.test.oxide.computer".to_string(), + ]); + params.not_before = (std::time::SystemTime::UNIX_EPOCH + + std::time::Duration::from_secs(NOT_BEFORE_SECS)) + .into(); + params.not_after = (std::time::SystemTime::UNIX_EPOCH + + std::time::Duration::from_secs(NOT_AFTER_SECS)) + .into(); + let chain = CertificateChain::with_params(params); + + let validity = leaf_validity(chain.cert_chain_as_pem().as_bytes()) + .expect("chain should parse"); + assert_eq!( + validity, + CertificateValidity { + not_before: DateTime::from_timestamp(NOT_BEFORE_SECS as i64, 0) + .unwrap(), + not_after: DateTime::from_timestamp(NOT_AFTER_SECS as i64, 0) + .unwrap(), + } + ); + } + + #[test] + fn test_leaf_validity_rejects_garbage_and_empty_input() { + assert!(matches!( + leaf_validity(b"not a certificate"), + Err(CertificateError::BadCertificate(_)) + | Err(CertificateError::CertificateEmpty) + )); + assert!(matches!( + leaf_validity(b""), + Err(CertificateError::CertificateEmpty) + )); + } } diff --git a/dev-tools/omdb/src/bin/omdb/nexus/fm_config.rs b/dev-tools/omdb/src/bin/omdb/nexus/fm_config.rs index eb384f4ee3e..73c46ce9086 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus/fm_config.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus/fm_config.rs @@ -137,6 +137,20 @@ struct ConfigOpts { /// revert this setting to the default value. #[clap(long, action = ArgAction::Set)] analysis_enabled: Option>, + + /// Sets how many days before a silo's external TLS certificate expires + /// the certificate diagnosis engine opens a case and requests an alert. + /// + /// The window applies to the certificate Nexus serves for the silo (the + /// one with the latest expiration time), so a case opens only when no + /// later-expiring replacement is installed. + /// + /// This must be a non-zero integer no greater than 3650, or 'default'. If + /// it is set to 'default', any previous override will be removed, and the + /// system will revert this setting to the default value. + #[clap(long, action = ArgAction::Set)] + certificate_expiry_warning_days: + Option>, } impl ConfigOpts { @@ -151,6 +165,9 @@ impl ConfigOpts { analysis_enabled: self .analysis_enabled .unwrap_or(current.analysis_enabled), + certificate_expiry_warning_days: self + .certificate_expiry_warning_days + .unwrap_or(current.certificate_expiry_warning_days), } } diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index 1bc7ea250ab..18b5918a96c 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -731,10 +731,11 @@ task: "fm_config_loader" config last loaded at: loaded by this activation: false current config: - source: default - analysis enabled: true (default) - sitrep limit: 2500 (default) - history pruning threshold: 2000 (default) + source: default + analysis enabled: true (default) + sitrep limit: 2500 (default) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) task: "fm_rendezvous" configured period: every m @@ -788,6 +789,8 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_ereport_in_case rows deleted: 0 batches: 1 + orphaned fm_fact_certificate rows deleted: 0 + batches: 1 orphaned fm_fact_physical_disk rows deleted: 0 batches: 1 orphaned fm_fact_saga rows deleted: 0 @@ -808,9 +811,10 @@ task: "fm_sitrep_history_pruner" last completed activation: , triggered by started at (s ago) and ran for ms configuration: - analysis enabled: true (default) - sitrep limit: 2500 (default) - history pruning threshold: 2000 (default) + analysis enabled: true (default) + sitrep limit: 2500 (default) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) deletion batch size: 1000 status: within limit (nothing was deleted) current count: 1 @@ -1459,10 +1463,11 @@ task: "fm_config_loader" config last loaded at: loaded by this activation: false current config: - source: default - analysis enabled: true (default) - sitrep limit: 2500 (default) - history pruning threshold: 2000 (default) + source: default + analysis enabled: true (default) + sitrep limit: 2500 (default) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) task: "fm_rendezvous" configured period: every m @@ -1516,6 +1521,8 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_ereport_in_case rows deleted: 0 batches: 1 + orphaned fm_fact_certificate rows deleted: 0 + batches: 1 orphaned fm_fact_physical_disk rows deleted: 0 batches: 1 orphaned fm_fact_saga rows deleted: 0 @@ -1536,9 +1543,10 @@ task: "fm_sitrep_history_pruner" last completed activation: , triggered by started at (s ago) and ran for ms configuration: - analysis enabled: true (default) - sitrep limit: 2500 (default) - history pruning threshold: 2000 (default) + analysis enabled: true (default) + sitrep limit: 2500 (default) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) deletion batch size: 1000 status: within limit (nothing was deleted) current count: 1 @@ -2425,10 +2433,11 @@ termination: Exited(0) --------------------------------------------- stdout: current fault management configuration: - source: default - analysis enabled: true (default) - sitrep limit: 2500 (default) - history pruning threshold: 2000 (default) + source: default + analysis enabled: true (default) + sitrep limit: 2500 (default) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: @@ -2439,9 +2448,10 @@ termination: Exited(0) --------------------------------------------- stdout: fault management config updated to version 1: - analysis enabled: true (default) - sitrep limit: 3000 (overriden) - history pruning threshold: 2000 (default) + analysis enabled: true (default) + sitrep limit: 3000 (overriden) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: @@ -2452,13 +2462,14 @@ termination: Exited(0) --------------------------------------------- stdout: current fault management configuration: - source: version 1 - modified at: - comment: + source: version 1 + modified at: + comment: I am altering the config. Pray I do not alter it further. - analysis enabled: true (default) - sitrep limit: 3000 (overriden) - history pruning threshold: 2000 (default) + analysis enabled: true (default) + sitrep limit: 3000 (overriden) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: @@ -2469,9 +2480,10 @@ termination: Exited(0) --------------------------------------------- stdout: fault management config updated to version 2: - analysis enabled: false (overriden) - sitrep limit: 3000 (overriden) - history pruning threshold: 2000 (default) + analysis enabled: false (overriden) + sitrep limit: 3000 (overriden) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: @@ -2482,13 +2494,14 @@ termination: Exited(0) --------------------------------------------- stdout: current fault management configuration: - source: version 2 - modified at: - comment: + source: version 2 + modified at: + comment: oops i altered it further - analysis enabled: false (overriden) - sitrep limit: 3000 (overriden) - history pruning threshold: 2000 (default) + analysis enabled: false (overriden) + sitrep limit: 3000 (overriden) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: @@ -2499,13 +2512,14 @@ termination: Exited(0) --------------------------------------------- stdout: fault management configuration v1: - source: version 1 - modified at: - comment: + source: version 1 + modified at: + comment: I am altering the config. Pray I do not alter it further. - analysis enabled: true (default) - sitrep limit: 3000 (overriden) - history pruning threshold: 2000 (default) + analysis enabled: true (default) + sitrep limit: 3000 (overriden) + history pruning threshold: 2000 (default) + certificate expiry warning days: 30 (default) --------------------------------------------- stderr: diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index e24959b248a..7e0cca5fc86 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/dev-tools/omdb/tests/usage_errors.out @@ -352,6 +352,10 @@ Options: power shelf - hardware.power_shelf.psu.remove: A power supply unit (PSU) has been removed from a power shelf + - silo.certificate.expiring: The TLS certificate that a silo's external API serves + is about to expire, and no later-expiring certificate is installed for that silo + - silo.certificate.expired: The TLS certificate that a silo's external API serves + has expired, and no later-expiring certificate is installed for that silo -d, --dispatched If `true`, include only alerts that have been fully dispatched. If `false`, include only @@ -396,7 +400,7 @@ stdout: --------------------------------------------- stderr: error: invalid value 'test.foo.box' for '--classes ...' - [possible values: probe, test.foo, test.foo.bar, test.foo.baz, test.quux.bar, test.quux.bar.baz, hardware.power_shelf.psu.insert, hardware.power_shelf.psu.remove] + [possible values: probe, test.foo, test.foo.bar, test.foo.baz, test.quux.bar, test.quux.bar.baz, hardware.power_shelf.psu.insert, hardware.power_shelf.psu.remove, silo.certificate.expiring, silo.certificate.expired] tip: a similar value exists: 'test.foo.baz' @@ -1950,7 +1954,7 @@ version. Use the `omdb nexus fm-config show-defaults` command to view the default values for all config options. -Usage: omdb nexus fm-config set [OPTIONS] --comment <--sitrep-limit |--history-pruning-threshold |--analysis-enabled > +Usage: omdb nexus fm-config set [OPTIONS] --comment <--sitrep-limit |--history-pruning-threshold |--analysis-enabled |--certificate-expiry-warning-days > Options: --comment @@ -1999,6 +2003,17 @@ Config Options: previous override will be removed, and the system will revert this setting to the default value. + --certificate-expiry-warning-days + Sets how many days before a silo's external TLS certificate expires the certificate + diagnosis engine opens a case and requests an alert. + + The window applies to the certificate Nexus serves for the silo (the one with the latest + expiration time), so a case opens only when no later-expiring replacement is installed. + + This must be a non-zero integer no greater than 3650, or 'default'. If it is set to + 'default', any previous override will be removed, and the system will revert this setting + to the default value. + Connection Options: --nexus-internal-url URL of the Nexus internal lockstep API diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index fd925632d9f..cae9085132e 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -152,6 +152,7 @@ nexus-reconfigurator-preparation.workspace = true nexus-reconfigurator-rendezvous.workspace = true nexus-types.workspace = true nexus-types-versions.workspace = true +omicron-certificates.workspace = true omicron-common.workspace = true omicron-passwords.workspace = true oxide-tokio-rt.workspace = true diff --git a/nexus/db-model/src/alert_class.rs b/nexus/db-model/src/alert_class.rs index 5a832c4441c..8186dca5e60 100644 --- a/nexus/db-model/src/alert_class.rs +++ b/nexus/db-model/src/alert_class.rs @@ -31,6 +31,8 @@ impl_enum_type!( TestQuuxBarBaz => b"test.quux.bar.baz" PsuInserted => b"hardware.power_shelf.psu.insert" PsuRemoved => b"hardware.power_shelf.psu.remove" + SiloCertificateExpiring => b"silo.certificate.expiring" + SiloCertificateExpired => b"silo.certificate.expired" ); impl AlertClass { @@ -66,6 +68,8 @@ impl From for AlertClass { In::TestQuuxBarBaz => Self::TestQuuxBarBaz, In::PsuInserted => Self::PsuInserted, In::PsuRemoved => Self::PsuRemoved, + In::SiloCertificateExpiring => Self::SiloCertificateExpiring, + In::SiloCertificateExpired => Self::SiloCertificateExpired, } } } @@ -81,6 +85,10 @@ impl From for nexus_types::alert::AlertClass { AlertClass::TestQuuxBarBaz => Self::TestQuuxBarBaz, AlertClass::PsuInserted => Self::PsuInserted, AlertClass::PsuRemoved => Self::PsuRemoved, + AlertClass::SiloCertificateExpiring => { + Self::SiloCertificateExpiring + } + AlertClass::SiloCertificateExpired => Self::SiloCertificateExpired, } } } diff --git a/nexus/db-model/src/fm.rs b/nexus/db-model/src/fm.rs index 2fdb505734c..5cc7632162b 100644 --- a/nexus/db-model/src/fm.rs +++ b/nexus/db-model/src/fm.rs @@ -32,6 +32,8 @@ mod config; pub use config::*; mod diagnosis_engine; pub use diagnosis_engine::*; +mod fact_certificate; +pub use fact_certificate::*; mod fact_physical_disk; pub use fact_physical_disk::*; mod fact_saga; diff --git a/nexus/db-model/src/fm/config.rs b/nexus/db-model/src/fm/config.rs index 0c9e540f212..23c825eeab4 100644 --- a/nexus/db-model/src/fm/config.rs +++ b/nexus/db-model/src/fm/config.rs @@ -27,6 +27,7 @@ pub struct FmConfig { pub analysis_enabled: Option, pub sitrep_limit: Option, pub history_pruning_threshold: Option, + pub certificate_expiry_warning_days: Option, } impl FmConfig { @@ -40,6 +41,7 @@ impl FmConfig { analysis_enabled, sitrep_limit, history_pruning_threshold, + certificate_expiry_warning_days, }, } = param; Ok(Self { @@ -50,6 +52,8 @@ impl FmConfig { sitrep_limit: sitrep_limit.map_override(|v| v.get().into()), history_pruning_threshold: history_pruning_threshold .map_override(|v| v.get().into()), + certificate_expiry_warning_days: certificate_expiry_warning_days + .map_override(|v| v.get().into()), }) } } @@ -69,6 +73,7 @@ impl TryFrom for fm::FmConfigView { analysis_enabled, sitrep_limit, history_pruning_threshold, + certificate_expiry_warning_days, time_modified, } = value; @@ -94,6 +99,10 @@ impl TryFrom for fm::FmConfigView { analysis_enabled: analysis_enabled.into(), sitrep_limit: nz!(sitrep_limit)?.into(), history_pruning_threshold: nz!(history_pruning_threshold)?.into(), + certificate_expiry_warning_days: nz!( + certificate_expiry_warning_days + )? + .into(), }; let version = NonZeroU32::new(version.into()).ok_or_else(|| { Error::invalid_value( diff --git a/nexus/db-model/src/fm/diagnosis_engine.rs b/nexus/db-model/src/fm/diagnosis_engine.rs index 951c5749396..fb1d4393d83 100644 --- a/nexus/db-model/src/fm/diagnosis_engine.rs +++ b/nexus/db-model/src/fm/diagnosis_engine.rs @@ -26,6 +26,7 @@ impl_enum_type!( PowerShelf => b"power_shelf" PhysicalDisk => b"physical_disk" Saga => b"saga" + Certificate => b"certificate" ); @@ -37,6 +38,9 @@ impl From for fm::DiagnosisEngineKind { fm::DiagnosisEngineKind::PhysicalDisk } DiagnosisEngine::Saga => fm::DiagnosisEngineKind::Saga, + DiagnosisEngine::Certificate => { + fm::DiagnosisEngineKind::Certificate + } } } } @@ -49,6 +53,9 @@ impl From for DiagnosisEngine { DiagnosisEngine::PhysicalDisk } fm::DiagnosisEngineKind::Saga => DiagnosisEngine::Saga, + fm::DiagnosisEngineKind::Certificate => { + DiagnosisEngine::Certificate + } } } } diff --git a/nexus/db-model/src/fm/fact_certificate.rs b/nexus/db-model/src/fm/fact_certificate.rs new file mode 100644 index 00000000000..3d5c8d60c59 --- /dev/null +++ b/nexus/db-model/src/fm/fact_certificate.rs @@ -0,0 +1,140 @@ +// 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/. + +//! Database representation of the certificate diagnosis engine's facts. +//! +//! Each certificate fact is stored as typed columns in the +//! `fm_fact_certificate` table. The `kind` discriminant selects which payload +//! columns are populated; per-kind CHECK constraints enforce that the right +//! columns are non-NULL for each kind. See +//! [`nexus_types::fm::CertificateFact`] for semantics. + +use crate::DbTypedUuid; +use crate::impl_enum_type; +use chrono::{DateTime, Utc}; +use nexus_db_schema::schema::fm_fact_certificate; +use nexus_types::fm; +use nexus_types::fm::case::FactMetadata; +use nexus_types::fm::{ + CertificateExpiryFactPayload, CertificateFact, FactPayload, +}; +use omicron_common::api::external::Error; +use omicron_uuid_kinds::{CaseKind, FactKind, SitrepKind}; +use uuid::Uuid; + +impl_enum_type!( + FmFactCertificateKindEnum: + + #[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, PartialEq, Eq)] + pub enum FmFactCertificateKind; + + BestCertificateExpiring => b"best_certificate_expiring" + BestCertificateExpired => b"best_certificate_expired" +); + +/// Diesel row for the `fm_fact_certificate` table. +/// +/// The payload columns are populated according to `kind`: a column is `Some` +/// if it belongs to that `kind`'s payload, and `None` otherwise. +#[derive(Queryable, Insertable, Clone, Debug, Selectable)] +#[diesel(table_name = fm_fact_certificate)] +pub struct FmFactCertificate { + pub id: DbTypedUuid, + /// The sitrep to which this fact belongs. + /// + /// This will change as the fact is carried forward from one sitrep to the + /// next. + pub sitrep_id: DbTypedUuid, + pub case_id: DbTypedUuid, + /// Sitrep in which this fact was first added. + /// + /// Preserved unchanged when the fact is carried forward; debug-only. + pub created_sitrep_id: DbTypedUuid, + pub comment: String, + + /// The silo this fact is about. + pub silo_id: Uuid, + pub kind: FmFactCertificateKind, + + // Columns shared by both kinds. + pub certificate_id: Option, + pub not_after: Option>, +} + +impl FmFactCertificate { + /// Build a row from a fact's shared metadata (`metadata`) and its + /// already-dispatched certificate payload (`cert_fact`). + pub fn from_sitrep( + sitrep_id: impl Into>, + case_id: impl Into>, + metadata: &FactMetadata, + cert_fact: &CertificateFact, + ) -> Self { + let FactMetadata { id, created_sitrep_id, comment } = metadata; + let kind = match cert_fact { + CertificateFact::BestCertificateExpiring(_) => { + FmFactCertificateKind::BestCertificateExpiring + } + CertificateFact::BestCertificateExpired(_) => { + FmFactCertificateKind::BestCertificateExpired + } + }; + let payload = cert_fact.payload(); + Self { + id: (*id).into(), + sitrep_id: sitrep_id.into(), + case_id: case_id.into(), + created_sitrep_id: (*created_sitrep_id).into(), + comment: comment.clone(), + silo_id: payload.silo_id, + kind, + certificate_id: Some(payload.certificate_id), + not_after: Some(payload.not_after), + } + } + + /// Reconstruct an in-memory fact from a row. + pub fn into_fact(self) -> Result { + let kind = self.kind; + let payload = CertificateExpiryFactPayload { + silo_id: self.silo_id, + certificate_id: self + .certificate_id + .ok_or_else(|| missing_column(kind, "certificate_id"))?, + not_after: self + .not_after + .ok_or_else(|| missing_column(kind, "not_after"))?, + }; + let payload = match kind { + FmFactCertificateKind::BestCertificateExpiring => { + FactPayload::Certificate( + CertificateFact::BestCertificateExpiring(payload), + ) + } + FmFactCertificateKind::BestCertificateExpired => { + FactPayload::Certificate( + CertificateFact::BestCertificateExpired(payload), + ) + } + }; + Ok(fm::case::Fact { + metadata: fm::case::FactMetadata { + id: self.id.into(), + created_sitrep_id: self.created_sitrep_id.into(), + comment: self.comment, + }, + payload, + }) + } +} + +fn missing_column(kind: FmFactCertificateKind, column: &str) -> Error { + Error::InternalError { + internal_message: format!( + "fm_fact_certificate row of kind {kind:?} has a NULL {column}, \ + violating the CHECK constraint requiring it to be non-NULL for \ + this kind" + ), + } +} diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index aebf8c9a43e..fc8cbce42f4 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(299, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(300, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(300, "fm-certificate-de"), KnownVersion::new(299, "blueprint-pruner-config"), KnownVersion::new(298, "blueprint-zone-multiple-external-ips"), KnownVersion::new(297, "inventory-zone-multiple-external-ips"), diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 5409c87e0f4..59e9dce20bb 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -40,6 +40,7 @@ use nexus_db_schema::schema::ereport::dsl as ereport_dsl; use nexus_db_schema::schema::fm_alert_request::dsl as alert_req_dsl; use nexus_db_schema::schema::fm_case::dsl as case_dsl; use nexus_db_schema::schema::fm_ereport_in_case::dsl as case_ereport_dsl; +use nexus_db_schema::schema::fm_fact_certificate::dsl as fact_cert_dsl; use nexus_db_schema::schema::fm_fact_physical_disk::dsl as fact_pd_dsl; use nexus_db_schema::schema::fm_fact_saga::dsl as fact_saga_dsl; use nexus_db_schema::schema::fm_sitrep::dsl as sitrep_dsl; @@ -136,6 +137,7 @@ sitrep_child_tables! { Case => { table: "fm_case" }, FmFactPhysicalDisk => { table: "fm_fact_physical_disk" }, FmFactSaga => { table: "fm_fact_saga" }, + FmFactCertificate => { table: "fm_fact_certificate" }, AnalysisReport => { table: "fm_sitrep_analysis_report" }, } @@ -640,6 +642,35 @@ impl DataStore { } } + // --- certificate diagnosis engine facts --- + let mut paginator: Paginator> = + Paginator::new(SQL_BATCH_SIZE, PaginationOrder::Descending); + while let Some(p) = paginator.next() { + let batch = paginated( + fact_cert_dsl::fm_fact_certificate, + fact_cert_dsl::id, + &p.current_pagparams(), + ) + .filter(fact_cert_dsl::sitrep_id.eq(id.into_untyped_uuid())) + .select(model::fm::FmFactCertificate::as_select()) + .load_async(conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + .internal_context("failed to load certificate case facts") + })?; + + paginator = p.found_batch(&batch, &|f| f.id); + for row in batch { + let case_id: CaseUuid = row.case_id.into(); + let fact_id = row.id; + let fact = row.into_fact().with_internal_context(|| { + format!("failed to read fact {fact_id} on case {case_id}") + })?; + insert_fact_for_case(&mut by_case, case_id, fact)?; + } + } + Ok(by_case) } @@ -894,6 +925,7 @@ impl DataStore { let mut case_ereports = Vec::new(); let mut physical_disk_facts = Vec::new(); let mut saga_facts = Vec::new(); + let mut certificate_facts = Vec::new(); for case in sitrep.cases { let case_id = case.id; cases.push(model::fm::CaseMetadata::from_sitrep(sitrep_id, &case)); @@ -938,6 +970,16 @@ impl DataStore { saga_fact, )); } + fm::FactPayload::Certificate(cert_fact) => { + certificate_facts.push( + model::fm::FmFactCertificate::from_sitrep( + sitrep_id, + case_id, + &fact.metadata, + cert_fact, + ), + ); + } } } } @@ -1022,6 +1064,19 @@ impl DataStore { })?; } + if !certificate_facts.is_empty() { + diesel::insert_into(fact_cert_dsl::fm_fact_certificate) + .values(certificate_facts) + .execute_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + .internal_context( + "failed to insert certificate case facts", + ) + })?; + } + if !cases.is_empty() { diesel::insert_into(case_dsl::fm_case) .values(cases) @@ -3004,11 +3059,60 @@ mod tests { } }; + // Certificate cases, exercising the fm_fact_certificate + // read/write/GC paths: one per fact kind, since a certificate case + // carries exactly one fact at a time. + let certificate_case = |kind: fn( + fm::CertificateExpiryFactPayload, + ) -> fm::CertificateFact, + comment: &str| { + let mut facts = iddqd::IdOrdMap::new(); + facts + .insert_unique(fm::case::Fact { + metadata: fm::case::FactMetadata { + id: FactUuid::new_v4(), + created_sitrep_id: sitrep_id, + comment: format!("a representative {comment} fact"), + }, + payload: fm::FactPayload::Certificate(kind( + fm::CertificateExpiryFactPayload { + silo_id: Uuid::new_v4(), + certificate_id: Uuid::new_v4(), + not_after: omicron_common::now_db_precision(), + }, + )), + }) + .unwrap(); + fm::Case { + id: omicron_uuid_kinds::CaseUuid::new_v4(), + metadata: fm::case::Metadata { + created_sitrep_id: sitrep_id, + closed_sitrep_id: None, + de: fm::DiagnosisEngineKind::Certificate, + comment: format!("a silo whose {comment}"), + }, + ereports: iddqd::IdOrdMap::new(), + alerts_requested: iddqd::IdOrdMap::new(), + support_bundles_requested: iddqd::IdOrdMap::new(), + facts, + } + }; + let case5 = certificate_case( + fm::CertificateFact::BestCertificateExpiring, + "best certificate is expiring", + ); + let case6 = certificate_case( + fm::CertificateFact::BestCertificateExpired, + "best certificate has expired", + ); + let mut cases = iddqd::IdOrdMap::new(); cases.insert_unique(case1.clone()).expect("failed to insert case 1"); cases.insert_unique(case2.clone()).expect("failed to insert case 2"); cases.insert_unique(case3).expect("failed to insert case 3"); cases.insert_unique(case4).expect("failed to insert case 4"); + cases.insert_unique(case5).expect("failed to insert case 5"); + cases.insert_unique(case6).expect("failed to insert case 6"); let mut ereports_by_id = iddqd::IdOrdMap::new(); for case in cases.iter() { ereports_by_id @@ -3057,6 +3161,7 @@ mod tests { closed_cases_copied_forward: Default::default(), in_service_disks: Default::default(), observed_sagas: Default::default(), + observed_silo_certificates: Default::default(), }; let analysis_report = AnalysisReport { log: Default::default(), @@ -3300,7 +3405,7 @@ mod tests { .get_result_async::(&*conn) .await .expect("failed to count cases before deletion"); - assert_eq!(cases_before, 4, "four cases should exist before deletion"); + assert_eq!(cases_before, 6, "six cases should exist before deletion"); let case_ereports_before: i64 = case_ereport_dsl::fm_ereport_in_case .filter( @@ -3682,6 +3787,7 @@ mod tests { num_ereporter_restarts: 0, in_service_disks: Default::default(), observed_sagas: Default::default(), + observed_silo_certificates: Default::default(), }; let analysis_report = AnalysisReport { log: Default::default(), diff --git a/nexus/db-queries/src/db/datastore/fm_config.rs b/nexus/db-queries/src/db/datastore/fm_config.rs index dd5d02c28c2..56e6306c8be 100644 --- a/nexus/db-queries/src/db/datastore/fm_config.rs +++ b/nexus/db-queries/src/db/datastore/fm_config.rs @@ -265,6 +265,7 @@ mod tests { analysis_enabled: Some(true), sitrep_limit: Some(SqlU32::new(2500)), history_pruning_threshold: Some(SqlU32::new(2000)), + certificate_expiry_warning_days: Some(SqlU32::new(30)), time_modified: chrono::DateTime::UNIX_EPOCH, } } @@ -331,6 +332,7 @@ mod tests { history_pruning_threshold: Setting::new( NonZeroU32::new(4).unwrap(), ), + certificate_expiry_warning_days: Setting::Default, }, }; assert!( @@ -370,6 +372,10 @@ mod tests { assert_eq!(comment, "first override"); assert_eq!(read.config.sitrep_limit.value().get(), 5); assert_eq!(read.config.history_pruning_threshold.value().get(), 4); + assert_eq!( + read.config.certificate_expiry_warning_days, + Setting::Default + ); // An invalid config is rejected with an invalid value error. // (Validation is tested exhaustively in `nexus-types`; this just @@ -416,6 +422,8 @@ mod tests { // Inserting version 2 with a valid config should work. config.comment = "second override".to_string(); config.config.analysis_enabled = Setting::new(false); + config.config.certificate_expiry_warning_days = + Setting::new(NonZeroU32::new(60).unwrap()); dbg!( datastore .fm_config_insert_latest_version(opctx, dbg!(config)) @@ -437,6 +445,10 @@ mod tests { assert!(!read.config.analysis_enabled.value()); assert_eq!(read.config.sitrep_limit.value().get(), 500); assert_eq!(read.config.history_pruning_threshold.value().get(), 400); + assert_eq!( + read.config.certificate_expiry_warning_days.value().get(), + 60 + ); db.terminate().await; logctx.cleanup_successful(); diff --git a/nexus/db-queries/tests/output/fm_config_insert_latest_version.sql b/nexus/db-queries/tests/output/fm_config_insert_latest_version.sql index 14507013e55..fc4bcc1a0b2 100644 --- a/nexus/db-queries/tests/output/fm_config_insert_latest_version.sql +++ b/nexus/db-queries/tests/output/fm_config_insert_latest_version.sql @@ -22,10 +22,11 @@ WITH time_modified, analysis_enabled, sitrep_limit, - history_pruning_threshold + history_pruning_threshold, + certificate_expiry_warning_days ) VALUES - ($4, $5, $6, $7, $8, $9) + ($4, $5, $6, $7, $8, $9, $10) RETURNING version ) diff --git a/nexus/db-schema/src/enums.rs b/nexus/db-schema/src/enums.rs index ad3816255c9..e8569dd6a63 100644 --- a/nexus/db-schema/src/enums.rs +++ b/nexus/db-schema/src/enums.rs @@ -59,6 +59,7 @@ define_enums! { EreporterTypeEnum => "ereporter_type", ExternalServiceKindEnum => "external_service_kind", FailureDomainEnum => "failure_domain", + FmFactCertificateKindEnum => "fm_fact_certificate_kind", FmFactPhysicalDiskKindEnum => "fm_fact_physical_disk_kind", FmFactSagaKindEnum => "fm_fact_saga_kind", FmFactSagaOrphanReasonEnum => "fm_fact_saga_orphan_reason", diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index df6783939fc..16b238d941b 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -3269,6 +3269,7 @@ table! { analysis_enabled -> Nullable, sitrep_limit -> Nullable, history_pruning_threshold -> Nullable, + certificate_expiry_warning_days -> Nullable, } } @@ -3416,6 +3417,20 @@ table! { } } +table! { + fm_fact_certificate (sitrep_id, id) { + id -> Uuid, + sitrep_id -> Uuid, + case_id -> Uuid, + created_sitrep_id -> Uuid, + comment -> Text, + silo_id -> Uuid, + kind -> crate::enums::FmFactCertificateKindEnum, + certificate_id -> Nullable, + not_after -> Nullable, + } +} + table! { fm_ereport_in_case (sitrep_id, id) { id -> Uuid, @@ -3435,6 +3450,8 @@ allow_tables_to_appear_in_same_query!(fm_sitrep, fm_fact_physical_disk); allow_tables_to_appear_in_same_query!(fm_case, fm_fact_physical_disk); allow_tables_to_appear_in_same_query!(fm_sitrep, fm_fact_saga); allow_tables_to_appear_in_same_query!(fm_case, fm_fact_saga); +allow_tables_to_appear_in_same_query!(fm_sitrep, fm_fact_certificate); +allow_tables_to_appear_in_same_query!(fm_case, fm_fact_certificate); table! { fm_alert_request (sitrep_id, id) { diff --git a/nexus/fm/Cargo.toml b/nexus/fm/Cargo.toml index 3d8d25525c5..fde7359cbcb 100644 --- a/nexus/fm/Cargo.toml +++ b/nexus/fm/Cargo.toml @@ -36,6 +36,7 @@ slog-error-chain.workspace = true steno.workspace = true thiserror.workspace = true typed-rng.workspace = true +uuid.workspace = true # deps for test utils omicron-test-utils = { workspace = true, optional = true } @@ -49,4 +50,3 @@ omicron-test-utils.workspace = true nexus-inventory.workspace = true nexus-reconfigurator-planning.workspace = true ereport-types.workspace = true -uuid.workspace = true diff --git a/nexus/fm/src/analysis_input.rs b/nexus/fm/src/analysis_input.rs index 106b41e3843..719f19b6393 100644 --- a/nexus/fm/src/analysis_input.rs +++ b/nexus/fm/src/analysis_input.rs @@ -8,9 +8,10 @@ use chrono::{DateTime, Utc}; use iddqd::IdOrdMap; use nexus_db_model::EreporterRestart; use nexus_types::fm::analysis_reports::ClosedCaseReport; -use nexus_types::fm::{self, Sitrep, SitrepVersion}; +use nexus_types::fm::{self, FmConfig, Sitrep, SitrepVersion}; use nexus_types::in_service_disk::InServiceDisk; use nexus_types::inventory; +use nexus_types::observed_certificate::ObservedSiloCertificates; use nexus_types::observed_saga::ObservedSaga; use omicron_uuid_kinds::AlertUuid; use omicron_uuid_kinds::CollectionUuid; @@ -64,6 +65,10 @@ pub struct Input { /// All non-terminal (running, unwinding, or abandoned) sagas, annotated /// with their latest node-event time and owning-Nexus state. observed_sagas: Arc>, + /// Every silo, with its installed external TLS certificates. + observed_silo_certificates: Arc>, + /// The fault management configuration in effect for this analysis. + config: FmConfig, } impl Input { @@ -125,12 +130,31 @@ impl Input { &self.observed_sagas } + /// Every silo observed in the database, with its installed external TLS + /// certificates, indexed by silo ID. See the certificate diagnosis engine + /// for how a silo's absence drives case closure. + pub fn observed_silo_certificates( + &self, + ) -> &IdOrdMap { + &self.observed_silo_certificates + } + + /// The fault management configuration in effect for this analysis. + /// + /// Diagnosis engines read their tunable thresholds from here rather than + /// from any global state, so a given input always produces the same + /// sitrep. + pub fn config(&self) -> &FmConfig { + &self.config + } + /// Returns a [`Builder`] for constructing a new `Input` from the provided /// `parent_sitrep` and inventory collection. /// - /// The queried input collections (in-service disks, observed sagas) are - /// provided via the builder's setters; [`Builder::build`] fails if any - /// was never provided. + /// The queried input collections (in-service disks, observed sagas, + /// observed silo certificates) and the FM configuration are provided via + /// the builder's setters; [`Builder::build`] fails if any was never + /// provided. pub fn builder( parent_sitrep: Option>, inv: Arc, @@ -160,6 +184,8 @@ impl Input { inv, in_service_disks: None, observed_sagas: None, + observed_silo_certificates: None, + config: None, new_ereports: IdOrdMap::default(), ereporter_restarts: IdOrdMap::default(), unmarked_seen_ereports: BTreeSet::default(), @@ -194,6 +220,11 @@ pub struct Builder { // e.g. an empty `observed_sagas` reads as "every saga case may close"). in_service_disks: Option>>, observed_sagas: Option>>, + observed_silo_certificates: Option>>, + /// The FM configuration. Required for the same reason as the queried + /// collections: analysis must not silently run against defaults when an + /// operator has overridden them. + config: Option, /// Ereports which are new and should be input to analysis in the next /// sitrep. new_ereports: IdOrdMap>, @@ -323,7 +354,25 @@ impl Builder { self } - /// Fills every required input not yet provided with an empty collection. + /// Provides the silos and their external TLS certificates queried from + /// the database. Required; [`Builder::build`] fails without it. + pub fn observed_silo_certificates( + mut self, + silos: Arc>, + ) -> Self { + self.observed_silo_certificates = Some(silos); + self + } + + /// Provides the fault management configuration in effect. + /// Required; [`Builder::build`] fails without it. + pub fn config(mut self, config: FmConfig) -> Self { + self.config = Some(config); + self + } + + /// Fills every required input not yet provided with an empty collection + /// (or, for the configuration, the default configuration). /// /// Test-only: lets a test populate just the inputs it exercises while /// still declaring, explicitly, that the rest are empty. Production @@ -332,6 +381,8 @@ impl Builder { pub fn with_empty_defaults(mut self) -> Self { self.in_service_disks.get_or_insert_with(Default::default); self.observed_sagas.get_or_insert_with(Default::default); + self.observed_silo_certificates.get_or_insert_with(Default::default); + self.config.get_or_insert_with(Default::default); self } @@ -342,6 +393,14 @@ impl Builder { let observed_sagas = self .observed_sagas .ok_or(InvalidInputs::MissingInput { name: "observed_sagas" })?; + let observed_silo_certificates = self + .observed_silo_certificates + .ok_or(InvalidInputs::MissingInput { + name: "observed_silo_certificates", + })?; + let config = self + .config + .ok_or(InvalidInputs::MissingInput { name: "config" })?; let parent_sitrep = self.parent_sitrep.as_ref().map(|s| &s.1); let (parent_sitrep_id, parent_inv_id) = match parent_sitrep { Some(sitrep) => { @@ -378,6 +437,31 @@ impl Builder { ) }) .collect(), + observed_silo_certificates: observed_silo_certificates + .iter() + .map(|silo| { + ( + silo.silo_id, + fm::analysis_reports::SiloCertificatesReport { + silo_name: silo.silo_name.to_string(), + certificates: silo + .certificates + .iter() + .map(|cert| { + ( + cert.id, + fm::analysis_reports::ObservedCertificateReport { + name: cert.name.to_string(), + not_before: cert.not_before, + not_after: cert.not_after, + }, + ) + }) + .collect(), + }, + ) + }) + .collect(), }; // Determine which cases must be copied forwards into the next sitrep. @@ -476,6 +560,8 @@ impl Builder { support_bundles_changed, in_service_disks, observed_sagas, + observed_silo_certificates, + config, }; Ok((input, report)) diff --git a/nexus/fm/src/diagnosis/certificate.rs b/nexus/fm/src/diagnosis/certificate.rs new file mode 100644 index 00000000000..3142bd60ea2 --- /dev/null +++ b/nexus/fm/src/diagnosis/certificate.rs @@ -0,0 +1,1114 @@ +// 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/. + +//! Certificate diagnosis engine. +//! +//! Nexus serves each silo's external API with the silo's TLS certificate +//! whose leaf `not_after` is latest (`ExternalEndpoint::best_certificate` in +//! Nexus). This engine predicts that choice from the analysis input and opens +//! a case (keyed by silo) when the certificate Nexus will serve: +//! - expires within the configured warning window +//! (`FmConfig::certificate_expiry_warning_days`), or +//! - has already expired. +//! +//! Either condition means no later-expiring replacement is installed; once an +//! operator uploads one, the case closes. A silo with no certificates at all +//! opens no case. +//! +//! An alert is requested whenever the case's fact changes: entering the +//! warning window requests a `silo.certificate.expiring` alert, passing +//! `not_after` requests a `silo.certificate.expired` alert, and a different +//! certificate becoming the (still expiring or expired) best certificate +//! requests a fresh alert for it. Unchanged input requests nothing. +//! +//! Like `best_certificate`, this engine ignores `not_before`. If that rule +//! ever changes, it must change in both places together, or the engine will +//! reason about a certificate Nexus does not actually serve. + +use crate::SitrepBuilder; +use chrono::{DateTime, TimeDelta, Utc}; +use nexus_types::alert::certificate as alert_types; +use nexus_types::fm; +use nexus_types::fm::DiagnosisEngineKind; +use nexus_types::fm::FmConfig; +use nexus_types::fm::{CertificateExpiryFactPayload, CertificateFact}; +use nexus_types::observed_certificate::{ + ObservedCertificate, ObservedSiloCertificates, +}; +use omicron_uuid_kinds::{CaseUuid, FactUuid}; +use std::collections::BTreeMap; +use std::collections::btree_map::Entry; +use uuid::Uuid; + +/// A parent-forwarded Certificate case, parsed into the form this engine acts +/// on. Every fact on a certificate case is about the same silo, and a case +/// carries at most one fact. +struct ParsedCertificateCase { + silo_id: Uuid, + /// The fact to consider when advancing the case. + fact: Option<(FactUuid, CertificateFact)>, + /// Facts that should not exist: any beyond the first. They carry no + /// information the kept fact doesn't. + duplicate_facts: Vec, +} + +/// Why a parent-forwarded Certificate case could not be interpreted. +/// +/// Uninterpretable cases are closed by [`analyze`]: an open case this engine +/// cannot process would otherwise be carried forward into every future sitrep +/// with no path to closure. +#[derive(Debug, Eq, PartialEq, thiserror::Error)] +enum UninterpretableCase { + #[error(transparent)] + ForeignFact(#[from] fm::case::ForeignFact), + #[error( + "facts reference different silos ({expected} and {found}, 1 expected)" + )] + DisagreeingSilos { expected: Uuid, found: Uuid }, + #[error("case has no facts, so the silo it concerns cannot be determined")] + NoFacts, +} + +/// Parse one parent-forwarded Certificate case into a +/// [`ParsedCertificateCase`], or explain why it cannot be interpreted. +fn parse_case( + case: &fm::Case, +) -> Result { + let mut silo_id: Option = None; + let mut kept: Option<(FactUuid, CertificateFact)> = None; + let mut duplicate_facts = Vec::new(); + // `case.facts` iterates in fact UUID order, so the kept fact is + // deterministically the one with the lowest UUID. + for fact in case.facts.iter() { + let cert_fact = fact.as_certificate()?; + let this_silo = cert_fact.silo_id(); + let expected = *silo_id.get_or_insert(this_silo); + if expected != this_silo { + return Err(UninterpretableCase::DisagreeingSilos { + expected, + found: this_silo, + }); + } + if kept.is_none() { + kept = Some((fact.metadata.id, cert_fact.clone())); + } else { + duplicate_facts.push(fact.metadata.id); + } + } + let Some(silo_id) = silo_id else { + return Err(UninterpretableCase::NoFacts); + }; + Ok(ParsedCertificateCase { silo_id, fact: kept, duplicate_facts }) +} + +pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { + let input = builder.input(); + // Expiry is judged against the input's deterministic "now"; see + // `Input::reference_time`. + let reference_time = input.reference_time(); + let window = warning_window(input.config()); + let silos = input.observed_silo_certificates(); + + // Parse the Certificate cases copied forward from the parent sitrep. Every + // case is about one silo, derived from its facts. Cases we cannot + // interpret are closed inline, so they don't ride along as + // open-but-unprocessable in every future sitrep. This is safe with + // respect to fault coverage: detection below is independent of case + // bookkeeping, so if a closed case concerned a silo that genuinely needs + // attention, a fresh, well-formed case is opened in this same pass. + let mut parent_cases: BTreeMap = + BTreeMap::new(); + for case in input + .open_cases() + .iter() + .filter(|c| c.metadata.de == DiagnosisEngineKind::Certificate) + { + match parse_case(case) { + Ok(parsed_case) => { + parent_cases.insert(case.id, parsed_case); + } + Err(reason) => { + builder + .log_warning("closing uninterpretable Certificate case") + .kv("case_id", case.id) + .kv("reason", reason.to_string()) + .finish(); + builder + .cases + .case_mut(&case.id) + .expect("case_id came from builder's open cases") + .close(format!("cannot interpret case: {reason}")); + } + } + } + + // Inverse index: which parent case is about which silo. Cases are + // per-silo, so a silo with two parent cases is already pathological. We + // keep one and close the rest as duplicates. `parent_cases` iterates + // ascending by CaseUuid, so we deterministically keep the lowest-ID case. + let mut case_for_silo: BTreeMap = BTreeMap::new(); + for (case_id, parsed_case) in &parent_cases { + match case_for_silo.entry(parsed_case.silo_id) { + Entry::Vacant(slot) => { + slot.insert(*case_id); + } + Entry::Occupied(kept) => { + let kept_case_id = *kept.get(); + builder + .log_warning("closing duplicate Certificate case") + .kv("case_id", case_id) + .kv("kept_case_id", kept_case_id) + .kv("silo_id", parsed_case.silo_id) + .finish(); + builder + .cases + .case_mut(case_id) + .expect("case_id came from builder's open cases") + .close(format!( + "duplicate of case {kept_case_id} for silo {}", + parsed_case.silo_id, + )); + } + } + } + + // Close the surviving parent case for any silo that no longer exists or + // whose best certificate is no longer expiring or expired. A silo whose + // condition still holds has its fact reconciled in the next loop, which + // owns all fact state for the silo. + for (silo_id, case_id) in &case_for_silo { + let mut case_mut = builder + .cases + .case_mut(case_id) + .expect("case_id came from builder's open cases"); + let Some(silo) = silos.get(silo_id) else { + case_mut.close(format!("silo {silo_id} no longer exists")); + continue; + }; + if desired_fact(silo, reference_time, window).is_some() { + continue; + } + match silo.best_certificate() { + None => case_mut.close(format!( + "silo {} ({silo_id}) has no certificates installed; that \ + is outside this engine's scope", + silo.silo_name, + )), + Some(cert) => case_mut.close(format!( + "silo {} ({silo_id}) now has a certificate, {} ({}), that \ + expires at {}, outside the {} warning window", + silo.silo_name, + cert.name, + cert.id, + cert.not_after, + omicron_common::format_time_delta(window), + )), + } + } + + // For each silo whose best certificate is expiring or expired, ensure its + // case carries exactly the fact matching the current observation: reuse + // the parent-forwarded case if any (dropping duplicate and stale facts), + // otherwise open a fresh case. This loop owns all fact state for a silo. + for silo in silos.iter() { + let Some((desired, best)) = desired_fact(silo, reference_time, window) + else { + continue; + }; + + let parent = case_for_silo + .get(&silo.silo_id) + .map(|case_id| (*case_id, &parent_cases[case_id])); + + let mut case_mut = match parent { + Some((case_id, _)) => builder + .cases + .case_mut(&case_id) + .expect("case_id came from builder's open cases"), + None => builder.cases.open_case( + DiagnosisEngineKind::Certificate, + format!( + "the external TLS certificate for silo {} ({}) needs \ + attention", + silo.silo_name, silo.silo_id, + ), + ), + }; + + // Duplicate facts carry no information the kept fact doesn't; remove + // them whether or not the kept fact still matches the observation. + if let Some((_, parsed_case)) = parent { + for fact_id in &parsed_case.duplicate_facts { + case_mut.remove_fact(*fact_id, "duplicate fact on the case"); + } + } + + let carried = parent.and_then(|(_, p)| p.fact.as_ref()); + if carried.map(|(_, fact)| fact) == Some(&desired) { + continue; + } + if let Some((fact_id, _)) = carried { + case_mut.remove_fact( + *fact_id, + "fact no longer matches the silo's best certificate", + ); + } + + let comment = match &desired { + CertificateFact::BestCertificateExpiring(p) => format!( + "best certificate {} ({}) expires at {}, in {}", + best.name, + best.id, + p.not_after, + omicron_common::format_time_delta( + p.not_after.signed_duration_since(reference_time) + ), + ), + CertificateFact::BestCertificateExpired(p) => format!( + "best certificate {} ({}) expired at {}, {} ago", + best.name, + best.id, + p.not_after, + omicron_common::format_time_delta( + reference_time.signed_duration_since(p.not_after) + ), + ), + }; + case_mut.add_fact(desired.clone(), comment.clone()); + + // The fact is new in this sitrep, so the condition it describes is + // new too (or concerns a different certificate than before): alert. + let alert_silo = alert_types::AlertSilo { + id: silo.silo_id, + name: silo.silo_name.clone(), + }; + let alert_cert = alert_types::AlertCertificate { + id: best.id, + name: best.name.clone(), + not_after: best.not_after, + }; + let alert_result = match &desired { + CertificateFact::BestCertificateExpiring(_) => case_mut + .request_alert( + &alert_types::SiloCertificateExpiringV0 { + silo: alert_silo, + certificate: alert_cert, + time: reference_time, + }, + &comment, + ), + CertificateFact::BestCertificateExpired(_) => case_mut + .request_alert( + &alert_types::SiloCertificateExpiredV0 { + silo: alert_silo, + certificate: alert_cert, + time: reference_time, + }, + &comment, + ), + }; + if let Err(err) = alert_result { + case_mut + .log_warning("failed to request alert for certificate fact") + .kv("silo_id", silo.silo_id) + .kv("certificate_id", best.id) + .kv("error", format_args!("{err}")); + } + } + + Ok(()) +} + +/// The warning window configured in `config`, as a duration. +fn warning_window(config: &FmConfig) -> TimeDelta { + TimeDelta::days(i64::from( + config.certificate_expiry_warning_days.value().get(), + )) +} + +/// The fact this silo's case should carry now, if any, paired with the +/// certificate it is about. +/// +/// The certificate considered is the one Nexus serves: the silo's certificate +/// with the latest leaf `not_after` (see the module docs). A silo with no +/// certificates carries no fact. +fn desired_fact( + silo: &ObservedSiloCertificates, + reference_time: DateTime, + window: TimeDelta, +) -> Option<(CertificateFact, &ObservedCertificate)> { + let best = silo.best_certificate()?; + let payload = CertificateExpiryFactPayload { + silo_id: silo.silo_id, + certificate_id: best.id, + not_after: best.not_after, + }; + if best.not_after <= reference_time { + Some((CertificateFact::BestCertificateExpired(payload), best)) + } else if best.not_after <= reference_time + window { + Some((CertificateFact::BestCertificateExpiring(payload), best)) + } else { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::analysis_input::Input; + use crate::builder::{SitrepBuilder, SitrepBuilderRng}; + use crate::test_util::FmTest; + use iddqd::IdOrdMap; + use nexus_types::alert::AlertClass; + use nexus_types::fm::config::Setting; + use nexus_types::fm::{Sitrep, SitrepVersion}; + use nexus_types::inventory; + use omicron_common::api::external::Name; + use omicron_generation_kinds::{AlertGeneration, SupportBundleGeneration}; + use omicron_test_utils::dev; + use omicron_uuid_kinds::{GenericUuid, OmicronZoneUuid, SitrepUuid}; + use std::num::NonZeroU32; + use std::sync::Arc; + + const SILO_A: Uuid = Uuid::from_u128(0xA); + const SILO_B: Uuid = Uuid::from_u128(0xB); + const CERT_1: Uuid = Uuid::from_u128(0x1); + const CERT_2: Uuid = Uuid::from_u128(0x2); + + /// Build a synthetic example collection (only used here for its + /// `time_done`, which is the expiry reference time). + fn setup( + test_name: &'static str, + ) -> (dev::LogContext, inventory::Collection) { + let (fm_test, logctx) = FmTest::new_with_logctx(test_name); + let (example, _bp) = fm_test.system_builder.build(); + (logctx, example.collection) + } + + fn name(s: &str) -> Name { + s.parse().expect("test names are valid") + } + + fn mk_cert( + id: Uuid, + cert_name: &str, + not_after: DateTime, + ) -> ObservedCertificate { + ObservedCertificate { + id, + name: name(cert_name), + not_before: not_after - TimeDelta::days(365), + not_after, + } + } + + fn mk_silo( + id: Uuid, + silo_name: &str, + certs: impl IntoIterator, + ) -> ObservedSiloCertificates { + ObservedSiloCertificates { + silo_id: id, + silo_name: name(silo_name), + certificates: certs.into_iter().collect(), + } + } + + fn silo_map( + silos: impl IntoIterator, + ) -> IdOrdMap { + silos.into_iter().collect() + } + + /// The default config with the warning window overridden to `days`. + fn config_with_window(days: u32) -> FmConfig { + FmConfig { + certificate_expiry_warning_days: Setting::new( + NonZeroU32::new(days).unwrap(), + ), + ..FmConfig::default() + } + } + + fn build_input( + collection: inventory::Collection, + parent_sitrep: Option, + silos: IdOrdMap, + config: FmConfig, + ) -> Input { + let parent = parent_sitrep.map(|s| { + Arc::new(( + SitrepVersion { + id: s.id(), + version: 0, + time_made_current: Utc::now(), + }, + s, + )) + }); + let builder = Input::builder(parent, Arc::new(collection)) + .expect("input builder should accept fresh inventory") + .observed_silo_certificates(Arc::new(silos)) + .config(config) + .with_empty_defaults(); + builder.build().expect("all inputs provided").0 + } + + /// Runs the engine over `input`. `seed` drives the sitrep's deterministic + /// UUID generation; multi-run tests pass a different seed per run so the + /// two sitreps (and any facts they create) get distinct IDs. + fn run_analyze(log: &slog::Logger, input: &Input, seed: &str) -> Sitrep { + let mut builder = SitrepBuilder::new_with_rng( + log, + input, + SitrepBuilderRng::from_seed(seed), + ); + analyze(&mut builder).expect("analyze ok"); + let (sitrep, report) = + builder.build(OmicronZoneUuid::new_v4(), Utc::now()); + eprintln!("\n--- analysis ---\n{}", report.display_multiline(0)); + for case in &sitrep.cases { + eprintln!("{}", case.display_indented(0, None)); + } + sitrep + } + + fn cert_cases(sitrep: &Sitrep) -> Vec<&fm::Case> { + sitrep + .cases + .iter() + .filter(|c| c.metadata.de == DiagnosisEngineKind::Certificate) + .collect() + } + + fn open_cert_cases(sitrep: &Sitrep) -> Vec<&fm::Case> { + cert_cases(sitrep).into_iter().filter(|c| c.is_open()).collect() + } + + #[track_caller] + fn sole_open_case(sitrep: &Sitrep) -> &fm::Case { + let cases = open_cert_cases(sitrep); + assert_eq!(cases.len(), 1, "expected exactly one open case"); + cases[0] + } + + /// The one certificate fact on `case`, with its decoded payload. + #[track_caller] + fn sole_fact(case: &fm::Case) -> (FactUuid, CertificateFact) { + assert_eq!( + case.facts.len(), + 1, + "expected exactly one fact on case {}", + case.id + ); + let fact = case.facts.iter().next().unwrap(); + let cert_fact = fact.as_certificate().expect("fact is a cert fact"); + (fact.metadata.id, cert_fact.clone()) + } + + fn alert_count(case: &fm::Case, class: AlertClass) -> usize { + case.alerts_requested.iter().filter(|a| a.class == class).count() + } + + #[track_caller] + fn assert_alert_counts(case: &fm::Case, expiring: usize, expired: usize) { + assert_eq!( + alert_count(case, AlertClass::SiloCertificateExpiring), + expiring, + "unexpected number of expiring alerts on case {}", + case.id + ); + assert_eq!( + alert_count(case, AlertClass::SiloCertificateExpired), + expired, + "unexpected number of expired alerts on case {}", + case.id + ); + } + + fn mk_fact( + parent_sitrep_id: SitrepUuid, + payload: impl Into, + ) -> fm::case::Fact { + fm::case::Fact { + metadata: fm::case::FactMetadata { + id: FactUuid::new_v4(), + created_sitrep_id: parent_sitrep_id, + comment: "parent certificate fact".to_string(), + }, + payload: payload.into(), + } + } + + fn make_certificate_case( + case_id: CaseUuid, + parent_sitrep_id: SitrepUuid, + facts: impl IntoIterator, + ) -> fm::Case { + let mut fact_map = IdOrdMap::new(); + for fact in facts { + fact_map.insert_unique(fact).unwrap(); + } + fm::Case { + id: case_id, + metadata: fm::case::Metadata { + created_sitrep_id: parent_sitrep_id, + closed_sitrep_id: None, + de: DiagnosisEngineKind::Certificate, + comment: "parent certificate case".to_string(), + }, + ereports: Default::default(), + alerts_requested: Default::default(), + support_bundles_requested: Default::default(), + facts: fact_map, + } + } + + fn make_parent_sitrep( + inv_collection_id: omicron_uuid_kinds::CollectionUuid, + cases: impl IntoIterator, + ) -> Sitrep { + let mut case_map = IdOrdMap::new(); + for case in cases { + case_map.insert_unique(case).unwrap(); + } + Sitrep { + metadata: fm::SitrepMetadata { + id: SitrepUuid::new_v4(), + inv_collection_id, + creator_id: OmicronZoneUuid::new_v4(), + parent_sitrep_id: None, + time_created: Utc::now(), + next_inv_min_time_started: Utc::now(), + comment: String::new(), + alert_generation: AlertGeneration::new(), + support_bundle_generation: SupportBundleGeneration::new(), + }, + cases: case_map, + ereports_by_id: Default::default(), + } + } + + fn expiring_payload(not_after: DateTime) -> CertificateFact { + CertificateFact::BestCertificateExpiring(CertificateExpiryFactPayload { + silo_id: SILO_A, + certificate_id: CERT_1, + not_after, + }) + } + + /// A collection identical to `collection` but observed `later`. + fn advance( + collection: &inventory::Collection, + later: TimeDelta, + ) -> inventory::Collection { + let mut c = collection.clone(); + c.time_done += later; + c + } + + #[test] + fn expiring_best_cert_opens_case_and_alerts() { + let (logctx, collection) = + setup("expiring_best_cert_opens_case_and_alerts"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + let input = build_input(collection, None, silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + let case = sole_open_case(&sitrep); + let (_, fact) = sole_fact(case); + assert_eq!(fact, expiring_payload(not_after)); + assert_alert_counts(case, 1, 0); + + let alert = case.alerts_requested.iter().next().unwrap(); + let payload = serde_json::from_value::< + alert_types::SiloCertificateExpiringV0, + >(alert.payload.clone()) + .expect("alert payload decodes"); + assert_eq!(payload.silo.id, SILO_A); + assert_eq!(payload.silo.name, name("fake-silo-a")); + assert_eq!(payload.certificate.id, CERT_1); + assert_eq!(payload.certificate.name, name("fake-cert-1")); + assert_eq!(payload.certificate.not_after, not_after); + assert_eq!(payload.time, now); + + logctx.cleanup_successful(); + } + + #[test] + fn expired_best_cert_opens_case_and_alerts() { + let (logctx, collection) = + setup("expired_best_cert_opens_case_and_alerts"); + let now = collection.time_done; + let not_after = now - TimeDelta::days(1); + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + let input = build_input(collection, None, silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + let case = sole_open_case(&sitrep); + let (_, fact) = sole_fact(case); + assert_eq!( + fact, + CertificateFact::BestCertificateExpired( + CertificateExpiryFactPayload { + silo_id: SILO_A, + certificate_id: CERT_1, + not_after, + } + ) + ); + assert_alert_counts(case, 0, 1); + + let alert = case.alerts_requested.iter().next().unwrap(); + let payload = serde_json::from_value::< + alert_types::SiloCertificateExpiredV0, + >(alert.payload.clone()) + .expect("alert payload decodes"); + assert_eq!(payload.certificate.id, CERT_1); + assert_eq!(payload.time, now); + + logctx.cleanup_successful(); + } + + #[test] + fn later_expiring_replacement_prevents_case() { + let (logctx, collection) = + setup("later_expiring_replacement_prevents_case"); + let now = collection.time_done; + // The expiring (even expired) certificate is not what Nexus serves, + // because a later-expiring one exists. + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [ + mk_cert(CERT_1, "fake-cert-1", now - TimeDelta::days(1)), + mk_cert(CERT_2, "fake-cert-2", now + TimeDelta::days(400)), + ], + )]); + let input = build_input(collection, None, silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + assert!(cert_cases(&sitrep).is_empty(), "no case should open"); + logctx.cleanup_successful(); + } + + #[test] + fn silo_without_certificates_opens_no_case() { + let (logctx, collection) = + setup("silo_without_certificates_opens_no_case"); + let silos = silo_map([mk_silo(SILO_A, "fake-silo-a", [])]); + let input = build_input(collection, None, silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + assert!(cert_cases(&sitrep).is_empty(), "no case should open"); + logctx.cleanup_successful(); + } + + #[test] + fn distinct_silos_get_distinct_cases() { + let (logctx, collection) = setup("distinct_silos_get_distinct_cases"); + let now = collection.time_done; + let silos = silo_map([ + mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(5))], + ), + mk_silo( + SILO_B, + "fake-silo-b", + [mk_cert(CERT_2, "fake-cert-2", now - TimeDelta::days(5))], + ), + ]); + let input = build_input(collection, None, silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + let cases = open_cert_cases(&sitrep); + assert_eq!(cases.len(), 2); + let mut silos_seen: Vec = + cases.iter().map(|c| sole_fact(c).1.silo_id()).collect(); + silos_seen.sort(); + assert_eq!(silos_seen, vec![SILO_A, SILO_B]); + logctx.cleanup_successful(); + } + + #[test] + fn carried_expiring_case_rotates_to_expired_with_one_alert() { + let (logctx, collection) = + setup("carried_expiring_case_rotates_to_expired_with_one_alert"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + + let input1 = build_input( + collection.clone(), + None, + silos.clone(), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + let case1_id = sole_open_case(&sitrep1).id; + let (fact1_id, _) = sole_fact(sole_open_case(&sitrep1)); + + // Twenty days later, the certificate has expired. + let input2 = build_input( + advance(&collection, TimeDelta::days(20)), + Some(sitrep1), + silos, + FmConfig::default(), + ); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + + let case = sole_open_case(&sitrep2); + assert_eq!(case.id, case1_id, "the same case should be reused"); + let (fact2_id, fact) = sole_fact(case); + assert_ne!(fact2_id, fact1_id, "the expired fact is a new fact"); + assert_eq!( + case.facts.iter().next().unwrap().metadata.created_sitrep_id, + sitrep2.id(), + "the expired fact was created in the second sitrep" + ); + assert_eq!( + fact, + CertificateFact::BestCertificateExpired( + CertificateExpiryFactPayload { + silo_id: SILO_A, + certificate_id: CERT_1, + not_after, + } + ) + ); + // The expiring alert is carried from the first sitrep; exactly one + // expired alert is new. + assert_alert_counts(case, 1, 1); + logctx.cleanup_successful(); + } + + #[test] + fn carried_case_closes_when_replacement_installed() { + let (logctx, collection) = + setup("carried_case_closes_when_replacement_installed"); + let now = collection.time_done; + let expiring = + mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(10)); + + let input1 = build_input( + collection.clone(), + None, + silo_map([mk_silo(SILO_A, "fake-silo-a", [expiring.clone()])]), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + let case1_id = sole_open_case(&sitrep1).id; + + let input2 = build_input( + collection, + Some(sitrep1), + silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [ + expiring, + mk_cert(CERT_2, "fake-cert-2", now + TimeDelta::days(400)), + ], + )]), + FmConfig::default(), + ); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + + assert!(open_cert_cases(&sitrep2).is_empty()); + let case = sitrep2.cases.get(&case1_id).expect("case carried"); + assert!(!case.is_open(), "case should be closed"); + assert_alert_counts(case, 1, 0); + logctx.cleanup_successful(); + } + + #[test] + fn carried_case_closes_when_silo_removed() { + let (logctx, collection) = + setup("carried_case_closes_when_silo_removed"); + let now = collection.time_done; + let input1 = build_input( + collection.clone(), + None, + silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(10))], + )]), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + let case1_id = sole_open_case(&sitrep1).id; + + let input2 = build_input( + collection, + Some(sitrep1), + silo_map([]), + FmConfig::default(), + ); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + let case = sitrep2.cases.get(&case1_id).expect("case carried"); + assert!(!case.is_open(), "case should be closed"); + logctx.cleanup_successful(); + } + + #[test] + fn new_best_cert_still_expiring_rotates_fact_and_realerts() { + let (logctx, collection) = + setup("new_best_cert_still_expiring_rotates_fact_and_realerts"); + let now = collection.time_done; + let cert1 = mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(10)); + + let input1 = build_input( + collection.clone(), + None, + silo_map([mk_silo(SILO_A, "fake-silo-a", [cert1.clone()])]), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + + // An operator uploads a replacement that is itself inside the window. + let cert2_not_after = now + TimeDelta::days(20); + let input2 = build_input( + collection, + Some(sitrep1), + silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [cert1, mk_cert(CERT_2, "fake-cert-2", cert2_not_after)], + )]), + FmConfig::default(), + ); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + + let case = sole_open_case(&sitrep2); + let (_, fact) = sole_fact(case); + assert_eq!( + fact, + CertificateFact::BestCertificateExpiring( + CertificateExpiryFactPayload { + silo_id: SILO_A, + certificate_id: CERT_2, + not_after: cert2_not_after, + } + ) + ); + assert_alert_counts(case, 2, 0); + logctx.cleanup_successful(); + } + + #[test] + fn rerun_with_unchanged_input_changes_nothing() { + let (logctx, collection) = + setup("rerun_with_unchanged_input_changes_nothing"); + let now = collection.time_done; + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(10))], + )]); + + let input1 = build_input( + collection.clone(), + None, + silos.clone(), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + let case1 = sole_open_case(&sitrep1).clone(); + let (fact1_id, fact1) = sole_fact(&case1); + + let input2 = + build_input(collection, Some(sitrep1), silos, FmConfig::default()); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + + let case2 = sole_open_case(&sitrep2); + assert_eq!(case2.id, case1.id); + let (fact2_id, fact2) = sole_fact(case2); + assert_eq!(fact2_id, fact1_id, "the fact should be carried unchanged"); + assert_eq!(fact2, fact1); + assert_eq!( + case2.facts.iter().next().unwrap().metadata.created_sitrep_id, + case1.metadata.created_sitrep_id, + "the carried fact keeps its original creation sitrep" + ); + assert_alert_counts(case2, 1, 0); + assert_eq!(case2.alerts_requested, case1.alerts_requested); + logctx.cleanup_successful(); + } + + #[test] + fn window_override_changes_outcome() { + let (logctx, collection) = setup("window_override_changes_outcome"); + let now = collection.time_done; + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", now + TimeDelta::days(45))], + )]); + + // 45 days out is outside the default 30-day window... + let input = build_input( + collection.clone(), + None, + silos.clone(), + FmConfig::default(), + ); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + assert!(cert_cases(&sitrep).is_empty()); + + // ...but inside a 60-day one. + let input = + build_input(collection, None, silos, config_with_window(60)); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + let case = sole_open_case(&sitrep); + assert_alert_counts(case, 1, 0); + logctx.cleanup_successful(); + } + + #[test] + fn uninterpretable_parent_cases_are_closed_and_replaced() { + let (logctx, collection) = + setup("uninterpretable_parent_cases_are_closed_and_replaced"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + + let parent_sitrep_id = SitrepUuid::new_v4(); + let foreign_case_id = CaseUuid::new_v4(); + let empty_case_id = CaseUuid::new_v4(); + let parent = { + let mut parent = make_parent_sitrep( + collection.id, + [ + // A Certificate case carrying another engine's fact. + make_certificate_case( + foreign_case_id, + parent_sitrep_id, + [mk_fact( + parent_sitrep_id, + fm::SagaFact::Abandoned( + fm::SagaAbandonedFactPayload { + saga_id: steno::SagaId(Uuid::new_v4()), + }, + ), + )], + ), + // A Certificate case with no facts at all. + make_certificate_case(empty_case_id, parent_sitrep_id, []), + ], + ); + parent.metadata.id = parent_sitrep_id; + parent + }; + + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + let input = + build_input(collection, Some(parent), silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + for id in [foreign_case_id, empty_case_id] { + let case = sitrep.cases.get(&id).expect("case carried"); + assert!(!case.is_open(), "case {id} should be closed"); + } + let case = sole_open_case(&sitrep); + assert!(case.id != foreign_case_id && case.id != empty_case_id); + let (_, fact) = sole_fact(case); + assert_eq!(fact, expiring_payload(not_after)); + assert_alert_counts(case, 1, 0); + logctx.cleanup_successful(); + } + + #[test] + fn duplicate_cases_for_silo_keep_lowest_id() { + let (logctx, collection) = + setup("duplicate_cases_for_silo_keep_lowest_id"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + + let parent_sitrep_id = SitrepUuid::new_v4(); + let low_id = CaseUuid::from_untyped_uuid(Uuid::from_u128(1)); + let high_id = CaseUuid::from_untyped_uuid(Uuid::from_u128(2)); + let mut parent = make_parent_sitrep( + collection.id, + [low_id, high_id].map(|id| { + make_certificate_case( + id, + parent_sitrep_id, + [mk_fact(parent_sitrep_id, expiring_payload(not_after))], + ) + }), + ); + parent.metadata.id = parent_sitrep_id; + + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + let input = + build_input(collection, Some(parent), silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + assert!(!sitrep.cases.get(&high_id).unwrap().is_open()); + let case = sole_open_case(&sitrep); + assert_eq!(case.id, low_id); + // The kept case's fact already matched, so nothing new was alerted. + assert_alert_counts(case, 0, 0); + logctx.cleanup_successful(); + } + + #[test] + fn duplicate_facts_on_case_are_removed() { + let (logctx, collection) = setup("duplicate_facts_on_case_are_removed"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + + let parent_sitrep_id = SitrepUuid::new_v4(); + let case_id = CaseUuid::new_v4(); + let facts = [ + mk_fact(parent_sitrep_id, expiring_payload(not_after)), + mk_fact(parent_sitrep_id, expiring_payload(not_after)), + ]; + let lowest_fact_id = facts.iter().map(|f| f.metadata.id).min().unwrap(); + let mut parent = make_parent_sitrep( + collection.id, + [make_certificate_case(case_id, parent_sitrep_id, facts)], + ); + parent.metadata.id = parent_sitrep_id; + + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [mk_cert(CERT_1, "fake-cert-1", not_after)], + )]); + let input = + build_input(collection, Some(parent), silos, FmConfig::default()); + let sitrep = run_analyze(&logctx.log, &input, "run-1"); + + let case = sole_open_case(&sitrep); + assert_eq!(case.id, case_id); + let (fact_id, fact) = sole_fact(case); + assert_eq!(fact_id, lowest_fact_id, "the lowest-UUID fact is kept"); + assert_eq!(fact, expiring_payload(not_after)); + assert_alert_counts(case, 0, 0); + logctx.cleanup_successful(); + } +} diff --git a/nexus/fm/src/diagnosis/mod.rs b/nexus/fm/src/diagnosis/mod.rs index a9b3ba058cc..fa7f39f518c 100644 --- a/nexus/fm/src/diagnosis/mod.rs +++ b/nexus/fm/src/diagnosis/mod.rs @@ -9,6 +9,7 @@ use crate::SitrepBuilder; +mod certificate; mod physical_disk; mod power_shelf; mod saga; @@ -17,6 +18,7 @@ pub fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { physical_disk::analyze(builder)?; power_shelf::analyze(builder)?; saga::analyze(builder)?; + certificate::analyze(builder)?; Ok(()) } diff --git a/nexus/fm/src/diagnosis/power_shelf.rs b/nexus/fm/src/diagnosis/power_shelf.rs index 748768ff97f..616f72631a5 100644 --- a/nexus/fm/src/diagnosis/power_shelf.rs +++ b/nexus/fm/src/diagnosis/power_shelf.rs @@ -1032,6 +1032,8 @@ mod tests { collection.into(), Arc::new(IdOrdMap::new()), Arc::new(IdOrdMap::new()), + Arc::new(IdOrdMap::new()), + Default::default(), ) .expect("input builder should accept fresh inventory"); builder.add_unmarked_ereports(new_ereports); diff --git a/nexus/fm/src/test_util.rs b/nexus/fm/src/test_util.rs index 94116cc33f9..f239e003552 100644 --- a/nexus/fm/src/test_util.rs +++ b/nexus/fm/src/test_util.rs @@ -12,9 +12,10 @@ use nexus_reconfigurator_planning::example; use nexus_types::fm::ereport::{ Ena, Ereport, EreportData, EreportId, Reporter, }; -use nexus_types::fm::{Sitrep, SitrepVersion}; +use nexus_types::fm::{FmConfig, Sitrep, SitrepVersion}; use nexus_types::in_service_disk::InServiceDisk; use nexus_types::inventory; +use nexus_types::observed_certificate::ObservedSiloCertificates; use nexus_types::observed_saga::ObservedSaga; use omicron_test_utils::dev; use omicron_uuid_kinds::EreporterRestartKind; @@ -71,10 +72,14 @@ impl FmTest { inv: Arc, in_service_disks: Arc>, observed_sagas: Arc>, + observed_silo_certificates: Arc>, + config: FmConfig, ) -> Result { let mut builder = Input::builder(parent_sitrep, inv)? .in_service_disks(in_service_disks) - .observed_sagas(observed_sagas); + .observed_sagas(observed_sagas) + .observed_silo_certificates(observed_silo_certificates) + .config(config); builder.add_ereporter_restarts( self.reporters.ereporter_restarts().iter().cloned(), ); diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index da78c462654..88197bc3afb 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -16,11 +16,14 @@ use nexus_db_model::DbMetadataNexusState; use nexus_db_model::PhysicalDiskPolicy; use nexus_db_model::SagaExecState; use nexus_db_model::SagaReasonAbandoned; +use nexus_db_model::ServiceKind; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore; +use nexus_db_queries::db::datastore::Discoverability; use nexus_db_queries::db::identity::Asset; +use nexus_db_queries::db::identity::Resource; use nexus_db_queries::db::pagination::Paginator; use nexus_fm as fm; use nexus_types::fm::FmConfig; @@ -30,16 +33,21 @@ use nexus_types::in_service_disk::InServiceDisk; use nexus_types::internal_api::background::FmAnalysisStatus; use nexus_types::internal_api::background::fm_analysis as status; use nexus_types::inventory; +use nexus_types::observed_certificate::ObservedCertificate; +use nexus_types::observed_certificate::ObservedSiloCertificates; use nexus_types::observed_saga::{ ObservedSaga, ObservedSagaState, SagaAbandonInfo, SagaAbandonReason, SagaOwnerState, }; +use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_uuid_kinds::AlertUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::SupportBundleUuid; use serde_json::json; use slog_error_chain::InlineErrorChain; +use std::collections::BTreeMap; +use std::num::NonZeroU32; use std::sync::Arc; use tokio::sync::watch; @@ -227,7 +235,7 @@ impl FmAnalysis { // Prepare analysis inputs. let (inputs, prep_status, input_report) = match self - .prepare_inputs(&opctx, parent_sitrep, inv) + .prepare_inputs(&opctx, parent_sitrep, inv, &cfg) .await { Ok(inputs) => inputs, @@ -321,6 +329,7 @@ impl FmAnalysis { opctx: &OpContext, parent_sitrep: Option, inv: Arc, + cfg: &FmConfig, ) -> Result<(Input, status::PreparationStatus, InputReport), PreparationError> { let mut warnings = Vec::new(); @@ -331,10 +340,15 @@ impl FmAnalysis { let observed_sagas = Arc::new(self.prepare_observed_sagas(opctx).await?); + let observed_silo_certificates = + Arc::new(self.load_silo_certificates(opctx, &mut warnings).await?); + let mut builder = fm::analysis_input::Input::builder(parent_sitrep.clone(), inv)? .in_service_disks(in_service_disks) - .observed_sagas(observed_sagas); + .observed_sagas(observed_sagas) + .observed_silo_certificates(observed_silo_certificates) + .config(*cfg); self.load_ereporter_restarts(opctx, &mut builder) .await .context("failed to load ereporter restarts")?; @@ -421,6 +435,98 @@ impl FmAnalysis { Ok(in_service_disks) } + /// Build the certificate diagnosis engine's input: every silo, with the + /// leaf validity window of each of its external TLS certificates. + /// + /// This reads the same rows the `external_endpoints` background task uses + /// to decide which certificate to serve, so the engine reasons about the + /// certificates Nexus actually presents. Certificates that cannot be + /// parsed, or that belong to a silo that no longer exists, are skipped + /// with a warning, mirroring how `external_endpoints` skips them. + async fn load_silo_certificates( + &self, + opctx: &OpContext, + warnings: &mut Vec, + ) -> anyhow::Result> { + // The batch size is arbitrary; most systems have a handful of silos + // and certificates, and a few have a few hundred certificates. + let batch_size = NonZeroU32::new(200).unwrap(); + + let mut silos = IdOrdMap::new(); + let mut paginator = + Paginator::new(batch_size, dropshot::PaginationOrder::Ascending); + while let Some(p) = paginator.next() { + let batch = self + .datastore + .silos_list( + opctx, + &PaginatedBy::Id(p.current_pagparams()), + Discoverability::All, + ) + .await + .context("failed to list silos")?; + paginator = p.found_batch(&batch, &|s| s.id()); + for silo in batch { + silos + .insert_unique(ObservedSiloCertificates { + silo_id: silo.id(), + silo_name: silo.name().clone(), + certificates: IdOrdMap::new(), + }) + .expect("silo IDs are unique"); + } + } + + let mut paginator = + Paginator::new(batch_size, dropshot::PaginationOrder::Ascending); + while let Some(p) = paginator.next() { + let batch = self + .datastore + .certificate_list_for( + opctx, + Some(ServiceKind::Nexus), + &PaginatedBy::Id(p.current_pagparams()), + false, + ) + .await + .context("failed to list certificates")?; + paginator = p.found_batch(&batch, &|c| c.id()); + for cert in batch { + let cert_id = cert.id(); + let Some(mut silo) = silos.get_mut(&cert.silo_id) else { + warnings.push(format!( + "certificate {cert_id} belongs to silo {}, which was not found; ignoring it", + cert.silo_id, + )); + continue; + }; + let validity = match omicron_certificates::leaf_validity( + &cert.cert, + ) { + Ok(validity) => validity, + Err(e) => { + warnings.push(format!( + "certificate {cert_id} for silo {} could not be parsed; ignoring it: {}", + cert.silo_id, + InlineErrorChain::new(&e), + )); + continue; + } + }; + silo.certificates + .insert_unique(ObservedCertificate { + id: cert_id, + name: cert.name().clone(), + not_before: validity.not_before, + not_after: validity.not_after, + }) + .expect("certificate IDs are unique"); + } + } + + Ok(silos) + } + /// Build the saga diagnosis engine's input: every non-terminal saga, /// annotated with the timestamp of its latest node event (the progress /// signal) and the state of its owning Nexus. @@ -428,8 +534,6 @@ impl FmAnalysis { &self, opctx: &OpContext, ) -> anyhow::Result> { - use std::collections::BTreeMap; - // All unfinished (running, unwinding, or abandoned) sagas. Completed // sagas are excluded; a parent case whose saga is absent from this // set is closed by the engine. @@ -967,6 +1071,7 @@ mod tests { analysis_enabled: Setting::new(true), sitrep_limit: Setting::new(sitrep_limit), history_pruning_threshold: Setting::new(history_pruning_threshold), + certificate_expiry_warning_days: Setting::Default, }; FmConfigView { config, source: Default::default() } } @@ -1347,7 +1452,7 @@ mod tests { ); let (input, prep, report) = task - .prepare_inputs(opctx, Some(parent), inv) + .prepare_inputs(opctx, Some(parent), inv, &FmConfig::default()) .await .expect("input preparation should succeed"); assert!( diff --git a/nexus/src/app/background/tasks/fm_config_load.rs b/nexus/src/app/background/tasks/fm_config_load.rs index 34b353ff9a0..8d53e48c022 100644 --- a/nexus/src/app/background/tasks/fm_config_load.rs +++ b/nexus/src/app/background/tasks/fm_config_load.rs @@ -216,6 +216,7 @@ mod test { history_pruning_threshold: Setting::new( NonZeroU32::new(400).unwrap(), ), + certificate_expiry_warning_days: Setting::Default, }, }; datastore diff --git a/nexus/src/app/background/tasks/fm_sitrep_history_pruner.rs b/nexus/src/app/background/tasks/fm_sitrep_history_pruner.rs index e6452065e52..ff1fe0b6946 100644 --- a/nexus/src/app/background/tasks/fm_sitrep_history_pruner.rs +++ b/nexus/src/app/background/tasks/fm_sitrep_history_pruner.rs @@ -217,6 +217,7 @@ mod tests { sitrep_limit: Setting::new( NonZeroU32::new(history_pruning_threshold + 1).unwrap(), ), + certificate_expiry_warning_days: Setting::Default, }; let view = FmConfigView { config, source: Default::default() }; // The sender is dropped here; watch receivers continue to yield the diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index 6b6cf87e2bf..3e1cd58ad54 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -311,6 +311,13 @@ impl ExternalEndpoint { // Anyway, we don't yet do anything of these things. For now, pick the // certificate chain whose leaf certificate has the latest expiration // time. + // + // The fault management certificate diagnosis engine + // (`nexus_fm::diagnosis::certificate`) predicts this choice from the + // same rule, so that it alerts when the certificate we will actually + // serve is expiring or expired. If the rule here changes, the engine + // (and `ObservedSiloCertificates::best_certificate`, which it uses) + // must change with it. // This would be cleaner if Asn1Time impl'd Ord or even just a way to // convert it to a Unix timestamp or any other comparable timestamp. diff --git a/nexus/types/output/alert_schemas/silo.certificate.expired/v0.json b/nexus/types/output/alert_schemas/silo.certificate.expired/v0.json new file mode 100644 index 00000000000..e4b7b148c30 --- /dev/null +++ b/nexus/types/output/alert_schemas/silo.certificate.expired/v0.json @@ -0,0 +1,74 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SiloCertificateExpiredV0", + "description": "An alert indicating that the TLS certificate served for a silo's external API has expired, and no later-expiring certificate is installed for that silo.\n\nNexus continues to serve the expired certificate rather than none at all, so that an operator can still connect (after choosing to trust it) and upload a replacement. Once a certificate with a later expiration time is uploaded for the silo, this condition is resolved.", + "type": "object", + "required": [ + "certificate", + "silo", + "time" + ], + "properties": { + "certificate": { + "$ref": "#/definitions/AlertCertificate" + }, + "silo": { + "$ref": "#/definitions/AlertSilo" + }, + "time": { + "description": "The time at which the condition was evaluated.", + "type": "string", + "format": "date-time" + } + }, + "definitions": { + "AlertCertificate": { + "description": "Describes the certificate involved in a certificate alert.", + "type": "object", + "required": [ + "id", + "name", + "not_after" + ], + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "name": { + "$ref": "#/definitions/Name" + }, + "not_after": { + "description": "The time after which the certificate is no longer valid.", + "type": "string", + "format": "date-time" + } + } + }, + "AlertSilo": { + "description": "Describes the silo involved in a certificate alert.", + "type": "object", + "required": [ + "id", + "name" + ], + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "name": { + "$ref": "#/definitions/Name" + } + } + }, + "Name": { + "title": "A name unique within the parent collection", + "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID, but they may contain a UUID. They can be at most 63 characters long.", + "type": "string", + "maxLength": 63, + "minLength": 1, + "pattern": "^(?![0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)^[a-z]([a-zA-Z0-9-]*[a-zA-Z0-9]+)?$" + } + } +} \ No newline at end of file diff --git a/nexus/types/output/alert_schemas/silo.certificate.expiring/v0.json b/nexus/types/output/alert_schemas/silo.certificate.expiring/v0.json new file mode 100644 index 00000000000..f864e403fa7 --- /dev/null +++ b/nexus/types/output/alert_schemas/silo.certificate.expiring/v0.json @@ -0,0 +1,74 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SiloCertificateExpiringV0", + "description": "An alert indicating that the TLS certificate served for a silo's external API is about to expire, and no later-expiring certificate is installed for that silo.\n\nNexus serves the silo's certificate with the latest expiration time. Once a certificate with a later expiration time is uploaded for the silo, this condition is resolved.", + "type": "object", + "required": [ + "certificate", + "silo", + "time" + ], + "properties": { + "certificate": { + "$ref": "#/definitions/AlertCertificate" + }, + "silo": { + "$ref": "#/definitions/AlertSilo" + }, + "time": { + "description": "The time at which the condition was evaluated.", + "type": "string", + "format": "date-time" + } + }, + "definitions": { + "AlertCertificate": { + "description": "Describes the certificate involved in a certificate alert.", + "type": "object", + "required": [ + "id", + "name", + "not_after" + ], + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "name": { + "$ref": "#/definitions/Name" + }, + "not_after": { + "description": "The time after which the certificate is no longer valid.", + "type": "string", + "format": "date-time" + } + } + }, + "AlertSilo": { + "description": "Describes the silo involved in a certificate alert.", + "type": "object", + "required": [ + "id", + "name" + ], + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "name": { + "$ref": "#/definitions/Name" + } + } + }, + "Name": { + "title": "A name unique within the parent collection", + "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID, but they may contain a UUID. They can be at most 63 characters long.", + "type": "string", + "maxLength": 63, + "minLength": 1, + "pattern": "^(?![0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)^[a-z]([a-zA-Z0-9-]*[a-zA-Z0-9]+)?$" + } + } +} \ No newline at end of file diff --git a/nexus/types/output/analysis_input_report_empty.out b/nexus/types/output/analysis_input_report_empty.out index 14f6b3afdf7..661498e4f5e 100644 --- a/nexus/types/output/analysis_input_report_empty.out +++ b/nexus/types/output/analysis_input_report_empty.out @@ -7,3 +7,5 @@ no cases copied forward no in-service control plane disks no non-terminal sagas observed + +no silos observed diff --git a/nexus/types/output/analysis_input_report_same_inv.out b/nexus/types/output/analysis_input_report_same_inv.out index 784c6253a7c..e760fd28ad7 100644 --- a/nexus/types/output/analysis_input_report_same_inv.out +++ b/nexus/types/output/analysis_input_report_same_inv.out @@ -8,3 +8,5 @@ no cases copied forward no in-service control plane disks no non-terminal sagas observed + +no silos observed diff --git a/nexus/types/output/analysis_input_report_with_cases.out b/nexus/types/output/analysis_input_report_with_cases.out index f3df07abedc..5ffb600648d 100644 --- a/nexus/types/output/analysis_input_report_with_cases.out +++ b/nexus/types/output/analysis_input_report_with_cases.out @@ -46,3 +46,7 @@ non-terminal sagas observed (2 total): state: Abandoned at 1970-01-01 00:00:00 UTC (Unrecoverable: fake recovery error) last event: owner: + +silo external TLS certificates observed (1 silos): + * silo 5110aaaa-0000-0000-0000-000000000001 (fake-silo): 1 certificate(s) + * certificate cccccccc-0000-0000-0000-000000000001 (fake-cert-1): valid from 2020-01-01 00:00:00 UTC until 2021-01-01 00:00:00 UTC diff --git a/nexus/types/src/alert.rs b/nexus/types/src/alert.rs index 0a4cbe0e0dd..96ffd1c1ba5 100644 --- a/nexus/types/src/alert.rs +++ b/nexus/types/src/alert.rs @@ -8,6 +8,7 @@ use schemars::JsonSchema; use serde::Serialize; use std::fmt; +pub mod certificate; pub mod power_shelf; /// Trait implemented by alerts. @@ -78,6 +79,10 @@ pub enum AlertClass { PsuInserted, #[strum(serialize = "hardware.power_shelf.psu.remove")] PsuRemoved, + #[strum(serialize = "silo.certificate.expiring")] + SiloCertificateExpiring, + #[strum(serialize = "silo.certificate.expired")] + SiloCertificateExpired, } impl AlertClass { @@ -121,6 +126,16 @@ impl AlertClass { Self::PsuRemoved => { "A power supply unit (PSU) has been removed from a power shelf" } + Self::SiloCertificateExpiring => { + "The TLS certificate that a silo's external API serves is \ + about to expire, and no later-expiring certificate is \ + installed for that silo" + } + Self::SiloCertificateExpired => { + "The TLS certificate that a silo's external API serves has \ + expired, and no later-expiring certificate is installed for \ + that silo" + } } } diff --git a/nexus/types/src/alert/certificate.rs b/nexus/types/src/alert/certificate.rs new file mode 100644 index 00000000000..80722093a75 --- /dev/null +++ b/nexus/types/src/alert/certificate.rs @@ -0,0 +1,85 @@ +// 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/. + +//! Silo TLS certificate alert types. + +use super::*; +use chrono::DateTime; +use chrono::Utc; +use omicron_common::api::external::Name; +use serde::Deserialize; +use uuid::Uuid; + +/// An alert indicating that the TLS certificate served for a silo's external +/// API is about to expire, and no later-expiring certificate is installed for +/// that silo. +/// +/// Nexus serves the silo's certificate with the latest expiration time. Once +/// a certificate with a later expiration time is uploaded for the silo, this +/// condition is resolved. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SiloCertificateExpiringV0 { + pub silo: AlertSilo, + pub certificate: AlertCertificate, + /// The time at which the condition was evaluated. + pub time: DateTime, +} + +impl AlertPayload for SiloCertificateExpiringV0 { + const CLASS: AlertClass = AlertClass::SiloCertificateExpiring; + const VERSION: u32 = 0; +} + +/// An alert indicating that the TLS certificate served for a silo's external +/// API has expired, and no later-expiring certificate is installed for that +/// silo. +/// +/// Nexus continues to serve the expired certificate rather than none at all, +/// so that an operator can still connect (after choosing to trust it) and +/// upload a replacement. Once a certificate with a later expiration time is +/// uploaded for the silo, this condition is resolved. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SiloCertificateExpiredV0 { + pub silo: AlertSilo, + pub certificate: AlertCertificate, + /// The time at which the condition was evaluated. + pub time: DateTime, +} + +impl AlertPayload for SiloCertificateExpiredV0 { + const CLASS: AlertClass = AlertClass::SiloCertificateExpired; + const VERSION: u32 = 0; +} + +/// Describes the silo involved in a certificate alert. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct AlertSilo { + pub id: Uuid, + pub name: Name, +} + +/// Describes the certificate involved in a certificate alert. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct AlertCertificate { + pub id: Uuid, + pub name: Name, + /// The time after which the certificate is no longer valid. + pub not_after: DateTime, +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::alert::tests::expectorate_alert_schema; + + #[test] + fn silo_certificate_expiring_v0_schema() { + expectorate_alert_schema::(); + } + + #[test] + fn silo_certificate_expired_v0_schema() { + expectorate_alert_schema::(); + } +} diff --git a/nexus/types/src/fm.rs b/nexus/types/src/fm.rs index 9d5101ca4fb..b0985b78e43 100644 --- a/nexus/types/src/fm.rs +++ b/nexus/types/src/fm.rs @@ -19,9 +19,9 @@ pub use config::{ pub mod fact; pub use fact::{ - DiskFact, FactPayload, SagaAbandonedFactPayload, SagaFact, - SagaNotProgressingFactPayload, SagaOwnerNotCurrentFactPayload, - ZpoolUnhealthyFactPayload, + CertificateExpiryFactPayload, CertificateFact, DiskFact, FactPayload, + SagaAbandonedFactPayload, SagaFact, SagaNotProgressingFactPayload, + SagaOwnerNotCurrentFactPayload, ZpoolUnhealthyFactPayload, }; pub mod display; @@ -250,4 +250,5 @@ pub enum DiagnosisEngineKind { PowerShelf, PhysicalDisk, Saga, + Certificate, } diff --git a/nexus/types/src/fm/analysis_reports.rs b/nexus/types/src/fm/analysis_reports.rs index d2526a3ec6a..de0d1bf7915 100644 --- a/nexus/types/src/fm/analysis_reports.rs +++ b/nexus/types/src/fm/analysis_reports.rs @@ -19,6 +19,7 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::fmt; +use uuid::Uuid; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct AnalysisReport { @@ -524,6 +525,10 @@ pub struct InputReport { /// analysis pass. #[serde(default)] pub observed_sagas: BTreeMap, + /// Every silo, with its installed external TLS certificates, visible to + /// the diagnosis engines for this analysis pass. Keyed by silo ID. + #[serde(default)] + pub observed_silo_certificates: BTreeMap, // Reports are serialized to the database, so any new field here should // be `#[serde(default)]` (or `Option`al) to keep reports written before // the field existed parseable. @@ -542,6 +547,22 @@ pub struct ObservedSagaReport { pub owner_state: Option, } +/// Summary of one silo's external TLS certificates in an [`InputReport`]. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] +pub struct SiloCertificatesReport { + pub silo_name: String, + /// The silo's certificates, keyed by certificate ID. + pub certificates: BTreeMap, +} + +/// Summary of one external TLS certificate in an [`InputReport`]. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] +pub struct ObservedCertificateReport { + pub name: String, + pub not_before: DateTime, + pub not_after: DateTime, +} + #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct ClosedCaseReport { pub metadata: case::Metadata, @@ -590,6 +611,7 @@ impl fmt::Display for InputReportMultilineDisplay<'_> { num_ereporter_restarts, in_service_disks, observed_sagas, + observed_silo_certificates, }, indent, colored, @@ -897,6 +919,43 @@ impl fmt::Display for InputReportMultilineDisplay<'_> { } } + if observed_silo_certificates.is_empty() { + writeln!(f, "\n{:indent$}no silos observed", "")?; + } else { + writeln!( + f, + "\n{:indent$}silo external TLS certificates observed ({} \ + silos):", + "", + observed_silo_certificates.len() + )?; + let indent = indent + 2; + for (silo_id, silo) in observed_silo_certificates { + let SiloCertificatesReport { silo_name, certificates } = silo; + writeln!( + f, + "{:indent$}* silo {silo_id} ({silo_name}): {} \ + certificate(s)", + "", + certificates.len() + )?; + let indent = indent + 2; + for (cert_id, cert) in certificates { + let ObservedCertificateReport { + name, + not_before, + not_after, + } = cert; + writeln!( + f, + "{:indent$}* certificate {cert_id} ({name}): valid \ + from {not_before} until {not_after}", + "" + )?; + } + } + } + Ok(()) } } @@ -1019,6 +1078,7 @@ mod tests { ); let observed_sagas = example_observed_sagas(); + let observed_silo_certificates = example_observed_silo_certificates(); InputReport { parent_sitrep_id: Some(parent_sitrep_id), @@ -1030,9 +1090,32 @@ mod tests { closed_cases_copied_forward, in_service_disks, observed_sagas, + observed_silo_certificates, } } + fn example_observed_silo_certificates() + -> BTreeMap { + let mut certificates = BTreeMap::new(); + certificates.insert( + Uuid::from_str("cccccccc-0000-0000-0000-000000000001").unwrap(), + ObservedCertificateReport { + name: "fake-cert-1".to_string(), + not_before: "2020-01-01T00:00:00Z".parse().unwrap(), + not_after: "2021-01-01T00:00:00Z".parse().unwrap(), + }, + ); + let mut silos = BTreeMap::new(); + silos.insert( + Uuid::from_str("5110aaaa-0000-0000-0000-000000000001").unwrap(), + SiloCertificatesReport { + silo_name: "fake-silo".to_string(), + certificates, + }, + ); + silos + } + fn example_observed_sagas() -> BTreeMap { let mut observed_sagas = BTreeMap::new(); observed_sagas.insert( @@ -1081,6 +1164,7 @@ mod tests { closed_cases_copied_forward: BTreeMap::new(), in_service_disks: BTreeSet::new(), observed_sagas: BTreeMap::new(), + observed_silo_certificates: BTreeMap::new(), } } @@ -1102,6 +1186,7 @@ mod tests { closed_cases_copied_forward: BTreeMap::new(), in_service_disks: BTreeSet::new(), observed_sagas: BTreeMap::new(), + observed_silo_certificates: BTreeMap::new(), } } diff --git a/nexus/types/src/fm/case.rs b/nexus/types/src/fm/case.rs index f9174d6f8aa..f0fa8a7e460 100644 --- a/nexus/types/src/fm/case.rs +++ b/nexus/types/src/fm/case.rs @@ -9,7 +9,7 @@ use crate::fm::DiagnosisEngineKind; use crate::fm::Ereport; use crate::fm::EreportId; use crate::fm::FactPayload; -use crate::fm::{DiskFact, SagaFact}; +use crate::fm::{CertificateFact, DiskFact, SagaFact}; use crate::support_bundle::BundleDataSelection; use iddqd::{IdOrdItem, IdOrdMap}; use omicron_uuid_kinds::{ @@ -225,6 +225,12 @@ impl Fact { self.payload.as_physical_disk().ok_or_else(|| self.foreign_fact()) } + /// The certificate payload, or a [`ForeignFact`] error if this fact + /// belongs to a different diagnosis engine. + pub fn as_certificate(&self) -> Result<&CertificateFact, ForeignFact> { + self.payload.as_certificate().ok_or_else(|| self.foreign_fact()) + } + fn foreign_fact(&self) -> ForeignFact { ForeignFact { fact_id: self.metadata.id, actual: self.payload.engine() } } diff --git a/nexus/types/src/fm/config.rs b/nexus/types/src/fm/config.rs index 00168e853fc..1626f7e5494 100644 --- a/nexus/types/src/fm/config.rs +++ b/nexus/types/src/fm/config.rs @@ -392,6 +392,9 @@ impl fmt::Display for FmConfigSource { /// [`Self::MAX_LIMIT`], /// - [`Self::history_pruning_threshold`] must be strictly less than /// [`Self::sitrep_limit`], +/// - [`Self::certificate_expiry_warning_days`] must be no more than +/// [`Self::MAX_CERTIFICATE_EXPIRY_WARNING_DAYS`] (it is a `NonZeroU32`, so +/// it is always at least 1). /// /// These rules are checked by the [`Self::validate`] method, which is called /// prior to accepting a config update. @@ -425,6 +428,17 @@ pub struct FmConfig { /// `fm_sitrep_history_pruner` background task will remove the oldest /// entries from the history. pub history_pruning_threshold: Setting, + + /// How many days before a silo's external TLS certificate expires the + /// certificate diagnosis engine opens a case and requests a + /// `silo.certificate.expiring` alert. + /// + /// The window applies to the certificate Nexus actually serves for the + /// silo (the one with the latest expiration time), so a case opens only + /// when no later-expiring replacement is installed. Time is measured + /// against the completion time of the inventory collection each sitrep + /// is generated from, not the wall clock. + pub certificate_expiry_warning_days: Setting, } use self::settings::*; @@ -466,6 +480,10 @@ pub mod settings { HistoryPruningThreshold: NonZeroU32 = FmConfig::DEFAULT_HISTORY_PRUNING_THRESHOLD } + define_setting! { + CertificateExpiryWarningDays: NonZeroU32 = + FmConfig::DEFAULT_CERTIFICATE_EXPIRY_WARNING_DAYS + } } impl FmConfig { @@ -520,6 +538,21 @@ impl FmConfig { /// enforced by the CHECK constraint on the `fm_config` table. pub const MAX_LIMIT: NonZeroU32 = NonZeroU32::new(5000).unwrap(); + /// The default value of [`Self::certificate_expiry_warning_days`], used + /// when there is no config override set. + pub const DEFAULT_CERTIFICATE_EXPIRY_WARNING_DAYS: NonZeroU32 = + NonZeroU32::new(30).unwrap(); + + /// The maximum permitted value of + /// [`Self::certificate_expiry_warning_days`]: ten years. A window this + /// large would flag essentially every certificate ever issued, so larger + /// values are treated as typos. + /// + /// **Note:** This value should be kept in sync with the maximum value + /// enforced by the CHECK constraint on the `fm_config` table. + pub const MAX_CERTIFICATE_EXPIRY_WARNING_DAYS: NonZeroU32 = + NonZeroU32::new(3650).unwrap(); + /// Returns a multi-line displayer for this config, with each line /// indented by `indent` spaces. pub fn display_multiline(&self, indent: usize) -> impl fmt::Display + '_ { @@ -536,6 +569,7 @@ impl FmConfig { analysis_enabled, sitrep_limit, history_pruning_threshold, + certificate_expiry_warning_days, }, indent, } = self; @@ -555,6 +589,12 @@ impl FmConfig { "{:>indent$}{HISTORY_PRUNING_THRESHOLD:indent$}{CERTIFICATE_EXPIRY_WARNING_DAYS: for FactPayload { @@ -47,12 +50,19 @@ impl From for FactPayload { } } +impl From for FactPayload { + fn from(fact: CertificateFact) -> Self { + FactPayload::Certificate(fact) + } +} + impl FactPayload { /// The diagnosis engine that owns this payload's variant. pub fn engine(&self) -> DiagnosisEngineKind { match self { FactPayload::PhysicalDisk(_) => DiagnosisEngineKind::PhysicalDisk, FactPayload::Saga(_) => DiagnosisEngineKind::Saga, + FactPayload::Certificate(_) => DiagnosisEngineKind::Certificate, } } @@ -73,6 +83,15 @@ impl FactPayload { _ => None, } } + + /// The certificate payload, or `None` if this fact belongs to a + /// different diagnosis engine. + pub fn as_certificate(&self) -> Option<&CertificateFact> { + match self { + FactPayload::Certificate(fact) => Some(fact), + _ => None, + } + } } /// Per-fact state for the physical-disk diagnosis engine. @@ -198,3 +217,59 @@ pub struct SagaAbandonedFactPayload { /// The saga this fact (and its parent case) is about. pub saga_id: steno::SagaId, } + +/// Per-fact state for the certificate diagnosis engine. +/// +/// A certificate case is keyed by silo and carries exactly one fact at a +/// time: the silo's best certificate (the one Nexus serves, chosen by latest +/// leaf `not_after`) is either about to expire or has already expired. +/// `BestCertificateExpired` supersedes `BestCertificateExpiring`: once +/// `not_after` has passed, the expiring fact is replaced rather than kept +/// alongside. +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub enum CertificateFact { + /// The silo's best certificate expires within the configured warning + /// window, and no later-expiring certificate is installed. + BestCertificateExpiring(CertificateExpiryFactPayload), + /// The silo's best certificate has expired, and no later-expiring + /// certificate is installed. Nexus still serves it, since serving an + /// expired certificate beats serving none. + BestCertificateExpired(CertificateExpiryFactPayload), +} + +impl CertificateFact { + /// The silo this fact (and its parent case) is about. Common to every + /// kind of certificate fact. + pub fn silo_id(&self) -> Uuid { + match self { + CertificateFact::BestCertificateExpiring(p) => p.silo_id, + CertificateFact::BestCertificateExpired(p) => p.silo_id, + } + } + + /// The condition-defining payload, shared by every kind. + pub fn payload(&self) -> &CertificateExpiryFactPayload { + match self { + CertificateFact::BestCertificateExpiring(p) => p, + CertificateFact::BestCertificateExpired(p) => p, + } + } +} + +/// Payload of a [`CertificateFact::BestCertificateExpiring`] or +/// [`CertificateFact::BestCertificateExpired`] fact. +/// +/// This carries only the fields that define the condition. Descriptive data +/// (the silo and certificate names) is looked up from the analysis input +/// when the case is acted on. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] +pub struct CertificateExpiryFactPayload { + /// The silo this fact (and its parent case) is about. + pub silo_id: Uuid, + /// The silo's best certificate at the time the fact was recorded. If a + /// different certificate becomes the best one, the fact is replaced. + pub certificate_id: Uuid, + /// The leaf `not_after` of that certificate. + pub not_after: DateTime, +} diff --git a/nexus/types/src/lib.rs b/nexus/types/src/lib.rs index bf6aa089146..738b6dbb298 100644 --- a/nexus/types/src/lib.rs +++ b/nexus/types/src/lib.rs @@ -40,6 +40,7 @@ pub mod instance; pub mod internal_api; pub mod inventory; pub mod multicast; +pub mod observed_certificate; pub mod observed_saga; pub mod quiesce; pub mod saga; diff --git a/nexus/types/src/observed_certificate.rs b/nexus/types/src/observed_certificate.rs new file mode 100644 index 00000000000..7ce65eb0af9 --- /dev/null +++ b/nexus/types/src/observed_certificate.rs @@ -0,0 +1,74 @@ +// 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/. + +//! "Currently installed external TLS certificates": the executed view from +//! the `silo` and `certificate` DB tables, reduced to what fault management +//! needs to reason about certificate expiry. +//! +//! Nexus serves each silo's external API with the silo's certificate whose +//! leaf `not_after` is latest (see `ExternalEndpoint::best_certificate` in +//! Nexus). The certificate diagnosis engine predicts that choice from this +//! view, so the view carries only the leaf certificate's validity window and +//! enough identity to name the certificate in a case or alert. + +use chrono::{DateTime, Utc}; +use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; +use omicron_common::api::external::Name; +use uuid::Uuid; + +/// One silo and every non-deleted external TLS certificate installed for it. +/// +/// A silo with no certificates is still represented (with an empty +/// `certificates` map), so that consumers can tell "silo exists with no +/// certificates" apart from "silo does not exist". +#[derive(Clone, Debug, PartialEq)] +pub struct ObservedSiloCertificates { + pub silo_id: Uuid, + pub silo_name: Name, + pub certificates: IdOrdMap, +} + +impl ObservedSiloCertificates { + /// The certificate Nexus will serve for this silo: the one whose leaf + /// `not_after` is latest, or `None` if the silo has no certificates. + /// + /// This must stay in lockstep with `ExternalEndpoint::best_certificate` + /// in Nexus, which applies the same rule to choose the certificate + /// actually presented to TLS clients. Like that function, this ignores + /// `not_before`. When several certificates share the latest `not_after`, + /// which one is returned is unspecified; callers only depend on the + /// `not_after` value itself. + pub fn best_certificate(&self) -> Option<&ObservedCertificate> { + self.certificates.iter().max_by_key(|cert| cert.not_after) + } +} + +impl IdOrdItem for ObservedSiloCertificates { + type Key<'a> = Uuid; + fn key(&self) -> Self::Key<'_> { + self.silo_id + } + id_upcast!(); +} + +/// One non-deleted external TLS certificate, reduced to its identity and the +/// validity window of its leaf certificate. +#[derive(Clone, Debug, PartialEq)] +pub struct ObservedCertificate { + pub id: Uuid, + pub name: Name, + /// The leaf certificate's `not_before`. Recorded for reporting; the + /// certificate diagnosis engine does not act on it. + pub not_before: DateTime, + /// The leaf certificate's `not_after`. + pub not_after: DateTime, +} + +impl IdOrdItem for ObservedCertificate { + type Key<'a> = Uuid; + fn key(&self) -> Self::Key<'_> { + self.id + } + id_upcast!(); +} diff --git a/openapi/nexus-lockstep.json b/openapi/nexus-lockstep.json index ee50fb94f8a..6e170023eb5 100644 --- a/openapi/nexus-lockstep.json +++ b/openapi/nexus-lockstep.json @@ -5233,7 +5233,7 @@ ] }, "FmConfig": { - "description": "A fault management configuration.\n\n# Validation\n\nFor a config to be valid, the following requirements must be upheld:\n\n- [`Self::sitrep_limit`] must be at least [`Self::MIN_SITREP_LIMIT`], and no more than [`Self::MAX_LIMIT`], - [`Self::history_pruning_threshold`] must be at least [`Self::MIN_HISTORY_PRUNING_THRESHOLD`], and no more than [`Self::MAX_LIMIT`], - [`Self::history_pruning_threshold`] must be strictly less than [`Self::sitrep_limit`],\n\nThese rules are checked by the [`Self::validate`] method, which is called prior to accepting a config update.", + "description": "A fault management configuration.\n\n# Validation\n\nFor a config to be valid, the following requirements must be upheld:\n\n- [`Self::sitrep_limit`] must be at least [`Self::MIN_SITREP_LIMIT`], and no more than [`Self::MAX_LIMIT`], - [`Self::history_pruning_threshold`] must be at least [`Self::MIN_HISTORY_PRUNING_THRESHOLD`], and no more than [`Self::MAX_LIMIT`], - [`Self::history_pruning_threshold`] must be strictly less than [`Self::sitrep_limit`], - [`Self::certificate_expiry_warning_days`] must be no more than [`Self::MAX_CERTIFICATE_EXPIRY_WARNING_DAYS`] (it is a `NonZeroU32`, so it is always at least 1).\n\nThese rules are checked by the [`Self::validate`] method, which is called prior to accepting a config update.", "type": "object", "properties": { "analysis_enabled": { @@ -5244,6 +5244,14 @@ } ] }, + "certificate_expiry_warning_days": { + "description": "How many days before a silo's external TLS certificate expires the certificate diagnosis engine opens a case and requests a `silo.certificate.expiring` alert.\n\nThe window applies to the certificate Nexus actually serves for the silo (the one with the latest expiration time), so a case opens only when no later-expiring replacement is installed. Time is measured against the completion time of the inventory collection each sitrep is generated from, not the wall clock.", + "allOf": [ + { + "$ref": "#/components/schemas/FmConfigNonZeroU32Setting" + } + ] + }, "history_pruning_threshold": { "description": "The maximum number of sitreps committed to the `fm_sitrep_history` table. If the number of sitreps exceeds this threshold, the `fm_sitrep_history_pruner` background task will remove the oldest entries from the history.", "allOf": [ @@ -5263,6 +5271,7 @@ }, "required": [ "analysis_enabled", + "certificate_expiry_warning_days", "history_pruning_threshold", "sitrep_limit" ] diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index e5b6662d4fd..297e3617f77 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -7474,7 +7474,9 @@ CREATE TYPE IF NOT EXISTS omicron.public.alert_class AS ENUM ( 'test.quux.bar', 'test.quux.bar.baz', 'hardware.power_shelf.psu.insert', - 'hardware.power_shelf.psu.remove' + 'hardware.power_shelf.psu.remove', + 'silo.certificate.expiring', + 'silo.certificate.expired' -- Add new alert classes here! ); @@ -8268,7 +8270,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_sitrep_analysis_report ( CREATE TYPE IF NOT EXISTS omicron.public.diagnosis_engine AS ENUM ( 'power_shelf', 'physical_disk', - 'saga' + 'saga', + 'certificate' ); CREATE TABLE IF NOT EXISTS omicron.public.fm_case ( @@ -8413,6 +8416,65 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_saga ( ) ); +-- The certificate diagnosis engine's facts. See the comment on the +-- physical-disk engine above: one table per engine, fact content as typed +-- columns. +CREATE TYPE IF NOT EXISTS omicron.public.fm_fact_certificate_kind AS ENUM ( + 'best_certificate_expiring', + 'best_certificate_expired' +); + +CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_certificate ( + -- Stable UUID for this fact across sitreps. + id UUID NOT NULL, + -- Sitrep this row belongs to. + sitrep_id UUID NOT NULL, + -- UUID of the case this fact attaches to. + case_id UUID NOT NULL, + -- UUID of the sitrep in which this fact was first added. Preserved + -- unchanged when the fact is carried forward into a child sitrep. + -- Debug-only. + created_sitrep_id UUID NOT NULL, + -- Free-form, debug-only comment. + comment TEXT NOT NULL, + + -- The silo this fact is about. Common to every kind of certificate fact + -- (the case is keyed by it), so it is always present regardless of + -- `kind`. + -- + -- Fact payloads carry only the fields that define the condition; data + -- that merely describes the silo or certificate (e.g., their names) is + -- looked up from the silo and certificate tables when a case is acted on. + silo_id UUID NOT NULL, + + -- Which certificate fact this row represents. The columns below are + -- populated according to this discriminant (see the CHECK constraints). + kind omicron.public.fm_fact_certificate_kind NOT NULL, + + -- Columns shared by the 'best_certificate_expiring' and + -- 'best_certificate_expired' kinds: the silo's best certificate (latest + -- leaf `not_after`) when the fact was recorded, and that `not_after`. + certificate_id UUID, + not_after TIMESTAMPTZ, + + PRIMARY KEY (sitrep_id, id), + + -- Each kind's constraint checks only that its own columns are present, + -- not that others are NULL, so future kinds may share columns. + CONSTRAINT best_certificate_expiring_columns_present CHECK ( + kind != 'best_certificate_expiring' OR ( + certificate_id IS NOT NULL + AND not_after IS NOT NULL + ) + ), + CONSTRAINT best_certificate_expired_columns_present CHECK ( + kind != 'best_certificate_expired' OR ( + certificate_id IS NOT NULL + AND not_after IS NOT NULL + ) + ) +); + CREATE TABLE IF NOT EXISTS omicron.public.fm_ereport_in_case ( -- ID of this association. When an ereport is assigned to a case, that -- association is assigned a UUID. These are used primarily to aid in @@ -9469,6 +9531,12 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_config ( -- -- This must be less than `sitrep_limit`, and must be at least 2. history_pruning_threshold INT8, + -- The number of days before a silo's external TLS certificate expires at + -- which the certificate diagnosis engine opens a case and requests an + -- alert, if no later-expiring certificate is installed for the silo. + -- + -- This must be at least 1 and at most 3650. + certificate_expiry_warning_days INT8, CONSTRAINT versions_are_positive CHECK (version > 0), @@ -9493,6 +9561,12 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_config ( CONSTRAINT history_limit_is_less_than_sirep_limit CHECK ( (history_pruning_threshold IS NULL OR sitrep_limit IS NULL) OR history_pruning_threshold < sitrep_limit + ), + CONSTRAINT certificate_expiry_warning_days_validity CHECK ( + certificate_expiry_warning_days IS NULL OR ( + certificate_expiry_warning_days >= 1 AND + certificate_expiry_warning_days <= 3650 + ) ) ); @@ -9506,7 +9580,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '299.0.0', NULL) + (TRUE, NOW(), NOW(), '300.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/fm-certificate-de/up1.sql b/schema/crdb/fm-certificate-de/up1.sql new file mode 100644 index 00000000000..943a9ba262d --- /dev/null +++ b/schema/crdb/fm-certificate-de/up1.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.diagnosis_engine ADD VALUE IF NOT EXISTS 'certificate' AFTER 'saga'; diff --git a/schema/crdb/fm-certificate-de/up2.sql b/schema/crdb/fm-certificate-de/up2.sql new file mode 100644 index 00000000000..d60af869798 --- /dev/null +++ b/schema/crdb/fm-certificate-de/up2.sql @@ -0,0 +1,4 @@ +CREATE TYPE IF NOT EXISTS omicron.public.fm_fact_certificate_kind AS ENUM ( + 'best_certificate_expiring', + 'best_certificate_expired' +); diff --git a/schema/crdb/fm-certificate-de/up3.sql b/schema/crdb/fm-certificate-de/up3.sql new file mode 100644 index 00000000000..40c6a919d96 --- /dev/null +++ b/schema/crdb/fm-certificate-de/up3.sql @@ -0,0 +1,50 @@ +CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_certificate ( + -- Stable UUID for this fact across sitreps. + id UUID NOT NULL, + -- Sitrep this row belongs to. + sitrep_id UUID NOT NULL, + -- UUID of the case this fact attaches to. + case_id UUID NOT NULL, + -- UUID of the sitrep in which this fact was first added. Preserved + -- unchanged when the fact is carried forward into a child sitrep. + -- Debug-only. + created_sitrep_id UUID NOT NULL, + -- Free-form, debug-only comment. + comment TEXT NOT NULL, + + -- The silo this fact is about. Common to every kind of certificate fact + -- (the case is keyed by it), so it is always present regardless of + -- `kind`. + -- + -- Fact payloads carry only the fields that define the condition; data + -- that merely describes the silo or certificate (e.g., their names) is + -- looked up from the silo and certificate tables when a case is acted on. + silo_id UUID NOT NULL, + + -- Which certificate fact this row represents. The columns below are + -- populated according to this discriminant (see the CHECK constraints). + kind omicron.public.fm_fact_certificate_kind NOT NULL, + + -- Columns shared by the 'best_certificate_expiring' and + -- 'best_certificate_expired' kinds: the silo's best certificate (latest + -- leaf `not_after`) when the fact was recorded, and that `not_after`. + certificate_id UUID, + not_after TIMESTAMPTZ, + + PRIMARY KEY (sitrep_id, id), + + -- Each kind's constraint checks only that its own columns are present, + -- not that others are NULL, so future kinds may share columns. + CONSTRAINT best_certificate_expiring_columns_present CHECK ( + kind != 'best_certificate_expiring' OR ( + certificate_id IS NOT NULL + AND not_after IS NOT NULL + ) + ), + CONSTRAINT best_certificate_expired_columns_present CHECK ( + kind != 'best_certificate_expired' OR ( + certificate_id IS NOT NULL + AND not_after IS NOT NULL + ) + ) +); diff --git a/schema/crdb/fm-certificate-de/up4.sql b/schema/crdb/fm-certificate-de/up4.sql new file mode 100644 index 00000000000..25d99b5273f --- /dev/null +++ b/schema/crdb/fm-certificate-de/up4.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.alert_class ADD VALUE IF NOT EXISTS 'silo.certificate.expiring' AFTER 'hardware.power_shelf.psu.remove'; diff --git a/schema/crdb/fm-certificate-de/up5.sql b/schema/crdb/fm-certificate-de/up5.sql new file mode 100644 index 00000000000..7f96c920b8f --- /dev/null +++ b/schema/crdb/fm-certificate-de/up5.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.alert_class ADD VALUE IF NOT EXISTS 'silo.certificate.expired' AFTER 'silo.certificate.expiring'; diff --git a/schema/crdb/fm-certificate-de/up6.sql b/schema/crdb/fm-certificate-de/up6.sql new file mode 100644 index 00000000000..4a849ade9de --- /dev/null +++ b/schema/crdb/fm-certificate-de/up6.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.fm_config ADD COLUMN IF NOT EXISTS certificate_expiry_warning_days INT8; diff --git a/schema/crdb/fm-certificate-de/up7.sql b/schema/crdb/fm-certificate-de/up7.sql new file mode 100644 index 00000000000..ddb6e5b97d6 --- /dev/null +++ b/schema/crdb/fm-certificate-de/up7.sql @@ -0,0 +1,6 @@ +ALTER TABLE omicron.public.fm_config ADD CONSTRAINT IF NOT EXISTS certificate_expiry_warning_days_validity CHECK ( + certificate_expiry_warning_days IS NULL OR ( + certificate_expiry_warning_days >= 1 AND + certificate_expiry_warning_days <= 3650 + ) +); diff --git a/schema/crdb/fm-certificate-de/up7.verify.sql b/schema/crdb/fm-certificate-de/up7.verify.sql new file mode 100644 index 00000000000..45b6bb53b76 --- /dev/null +++ b/schema/crdb/fm-certificate-de/up7.verify.sql @@ -0,0 +1,2 @@ +-- DO NOT EDIT. Generated by test_migration_verification_files. +SELECT CAST(IF((SELECT true WHERE EXISTS (SELECT 1 FROM [SHOW CONSTRAINTS FROM fm_config] WHERE constraint_name = 'certificate_expiry_warning_days_validity' AND validated = true)),'true','Schema change verification failed: constraint certificate_expiry_warning_days_validity not found on table fm_config') AS BOOL); From e8c36746a2d8b9768ce50039627b3685153784d0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 11:11:25 -0700 Subject: [PATCH 2/9] fm: name only the absolute expiration time in certificate fact comments A fact's comment is recorded when the fact is added and carried forward unchanged, so a relative phrase like "in 10days" or "1day ago" goes stale as sitreps advance. Record the absolute not_after only. --- nexus/fm/src/diagnosis/certificate.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/nexus/fm/src/diagnosis/certificate.rs b/nexus/fm/src/diagnosis/certificate.rs index 3142bd60ea2..02ed650b751 100644 --- a/nexus/fm/src/diagnosis/certificate.rs +++ b/nexus/fm/src/diagnosis/certificate.rs @@ -255,24 +255,17 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { ); } + // The comment is recorded once, when the fact is added, and carried + // forward unchanged, so it names only the absolute expiration time + // rather than a distance from a "now" that will go stale. let comment = match &desired { CertificateFact::BestCertificateExpiring(p) => format!( - "best certificate {} ({}) expires at {}, in {}", - best.name, - best.id, - p.not_after, - omicron_common::format_time_delta( - p.not_after.signed_duration_since(reference_time) - ), + "best certificate {} ({}) expires at {}", + best.name, best.id, p.not_after, ), CertificateFact::BestCertificateExpired(p) => format!( - "best certificate {} ({}) expired at {}, {} ago", - best.name, - best.id, - p.not_after, - omicron_common::format_time_delta( - reference_time.signed_duration_since(p.not_after) - ), + "best certificate {} ({}) expired at {}", + best.name, best.id, p.not_after, ), }; case_mut.add_fact(desired.clone(), comment.clone()); From 590fba9742140fd19aec4eb01b185bad5c64db25 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:16:19 -0700 Subject: [PATCH 3/9] fm: only observe certificates Nexus can actually serve The certificate diagnosis engine's loader accepted any certificate whose PEM parsed, but ExternalEndpoints::new also drops certificates whose key rustls cannot load. Such a certificate could be chosen as the silo's best and hide an expiring one that is served. Run the loader's rows through TlsCertificate::try_from so both sides agree. --- certificates/src/lib.rs | 52 ++--- nexus/src/app/background/tasks/fm_analysis.rs | 201 ++++++++++++++++-- nexus/src/app/external_endpoints.rs | 25 ++- 3 files changed, 233 insertions(+), 45 deletions(-) diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index e7d45d5aaaa..a778820bc24 100644 --- a/certificates/src/lib.rs +++ b/certificates/src/lib.rs @@ -12,6 +12,7 @@ use openssl::asn1::Asn1Time; use openssl::asn1::Asn1TimeRef; use openssl::pkey::PKey; use openssl::x509::X509; +use openssl::x509::X509Ref; use std::borrow::Borrow; use std::ffi::CString; @@ -93,21 +94,17 @@ pub struct CertificateValidity { pub not_after: DateTime, } -/// Returns the validity window of the leaf certificate in a PEM-encoded -/// certificate chain. +/// Returns the validity window of an X509 certificate. /// -/// The leaf certificate is the first certificate in the chain. This is the -/// same convention used when the chain is served to TLS clients, so the -/// returned window is the one clients will check. -pub fn leaf_validity( - certs_pem: &[u8], +/// When called on the leaf certificate of a chain (the first certificate in +/// the chain, which is the one presented to TLS clients), the returned window +/// is the one clients will check. +pub fn validity( + cert: &X509Ref, ) -> Result { - let certs = X509::stack_from_pem(certs_pem) - .map_err(CertificateError::BadCertificate)?; - let leaf = certs.first().ok_or(CertificateError::CertificateEmpty)?; Ok(CertificateValidity { - not_before: asn1_time_to_chrono(leaf.not_before())?, - not_after: asn1_time_to_chrono(leaf.not_after())?, + not_before: asn1_time_to_chrono(cert.not_before())?, + not_after: asn1_time_to_chrono(cert.not_after())?, }) } @@ -498,11 +495,12 @@ mod tests { } #[test] - fn test_leaf_validity_reads_leaf_certificate() { + fn test_validity_converts_asn1_times() { // Pin the leaf's validity window to exact second offsets from the // Unix epoch. The root and intermediate certificates in the chain // keep rcgen's default (much wider) window, so a correct result - // proves we read the leaf and not some other link in the chain. + // proves we read the certificate we were given and not some other + // link in the chain. const NOT_BEFORE_SECS: u64 = 1_000_000_000; const NOT_AFTER_SECS: u64 = 2_000_000_000; let mut params = CertificateParams::new(vec![ @@ -515,11 +513,13 @@ mod tests { + std::time::Duration::from_secs(NOT_AFTER_SECS)) .into(); let chain = CertificateChain::with_params(params); - - let validity = leaf_validity(chain.cert_chain_as_pem().as_bytes()) + let certs = X509::stack_from_pem(chain.cert_chain_as_pem().as_bytes()) .expect("chain should parse"); + + let leaf_validity = + validity(&certs[0]).expect("leaf validity should convert"); assert_eq!( - validity, + leaf_validity, CertificateValidity { not_before: DateTime::from_timestamp(NOT_BEFORE_SECS as i64, 0) .unwrap(), @@ -527,18 +527,10 @@ mod tests { .unwrap(), } ); - } - - #[test] - fn test_leaf_validity_rejects_garbage_and_empty_input() { - assert!(matches!( - leaf_validity(b"not a certificate"), - Err(CertificateError::BadCertificate(_)) - | Err(CertificateError::CertificateEmpty) - )); - assert!(matches!( - leaf_validity(b""), - Err(CertificateError::CertificateEmpty) - )); + // The intermediate certificate keeps rcgen's default window, which + // differs from the leaf's. + let intermediate_validity = + validity(&certs[1]).expect("intermediate validity should convert"); + assert_ne!(leaf_validity, intermediate_validity); } } diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 88197bc3afb..c72d4cee1d8 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -5,6 +5,7 @@ use crate::app::background::Activator; use crate::app::background::BackgroundTask; use crate::app::background::tasks::fm_sitrep_load::CurrentSitrep; +use crate::app::external_endpoints::TlsCertificate; use anyhow::Context; use chrono::Utc; use fm::analysis_input::Input; @@ -439,10 +440,14 @@ impl FmAnalysis { /// leaf validity window of each of its external TLS certificates. /// /// This reads the same rows the `external_endpoints` background task uses - /// to decide which certificate to serve, so the engine reasons about the - /// certificates Nexus actually presents. Certificates that cannot be - /// parsed, or that belong to a silo that no longer exists, are skipped - /// with a warning, mirroring how `external_endpoints` skips them. + /// to decide which certificate to serve, and applies the same acceptance + /// check (`TlsCertificate::try_from`), so the engine reasons about exactly + /// the certificates Nexus can present. A certificate that fails that check + /// is never served, however far off its expiration is, so it must not be + /// allowed to mask an expiring certificate that is served. Certificates + /// that fail the check, or that belong to a silo that no longer exists, + /// are skipped with a warning, mirroring how `external_endpoints` skips + /// them. async fn load_silo_certificates( &self, opctx: &OpContext, @@ -493,22 +498,21 @@ impl FmAnalysis { paginator = p.found_batch(&batch, &|c| c.id()); for cert in batch { let cert_id = cert.id(); - let Some(mut silo) = silos.get_mut(&cert.silo_id) else { + let cert_name = cert.name().clone(); + let silo_id = cert.silo_id; + let Some(mut silo) = silos.get_mut(&silo_id) else { warnings.push(format!( "certificate {cert_id} belongs to silo {}, which was not found; ignoring it", cert.silo_id, )); continue; }; - let validity = match omicron_certificates::leaf_validity( - &cert.cert, - ) { - Ok(validity) => validity, + let validity = match TlsCertificate::try_from(cert) { + Ok(tls_cert) => tls_cert.validity(), Err(e) => { warnings.push(format!( - "certificate {cert_id} for silo {} could not be parsed; ignoring it: {}", - cert.silo_id, - InlineErrorChain::new(&e), + "certificate {cert_id} for silo {silo_id} cannot \ + be served; ignoring it: {e:#}", )); continue; } @@ -516,7 +520,7 @@ impl FmAnalysis { silo.certificates .insert_unique(ObservedCertificate { id: cert_id, - name: cert.name().clone(), + name: cert_name, not_before: validity.not_before, not_after: validity.not_after, }) @@ -1495,6 +1499,177 @@ mod tests { logctx.cleanup_successful(); } + /// Builds a self-signed certificate whose key is 1024-bit RSA, returning + /// the certificate and key as PEM. + /// + /// Upload validation (`CertificateValidator`) accepts such a certificate: + /// openssl parses the key, and it matches the leaf's public key. But + /// rustls (via aws-lc-rs) refuses RSA keys shorter than 2048 bits, so + /// `TlsCertificate::try_from` rejects it and Nexus never serves it. + fn unservable_certificate( + hostname: &str, + not_after: chrono::DateTime, + ) -> (String, String) { + use openssl::asn1::Asn1Time; + use openssl::hash::MessageDigest; + use openssl::nid::Nid; + use openssl::pkey::PKey; + use openssl::rsa::Rsa; + use openssl::x509::X509Builder; + use openssl::x509::X509NameBuilder; + use openssl::x509::extension::SubjectAlternativeName; + + let key = PKey::from_rsa(Rsa::generate(1024).unwrap()).unwrap(); + let mut name = X509NameBuilder::new().unwrap(); + name.append_entry_by_nid(Nid::COMMONNAME, hostname).unwrap(); + let name = name.build(); + + let mut builder = X509Builder::new().unwrap(); + builder.set_version(2).unwrap(); + builder.set_subject_name(&name).unwrap(); + builder.set_issuer_name(&name).unwrap(); + builder.set_pubkey(&key).unwrap(); + builder.set_not_before(&Asn1Time::days_from_now(0).unwrap()).unwrap(); + builder + .set_not_after(&Asn1Time::from_unix(not_after.timestamp()).unwrap()) + .unwrap(); + let san = SubjectAlternativeName::new() + .dns(hostname) + .build(&builder.x509v3_context(None, None)) + .unwrap(); + builder.append_extension(san).unwrap(); + builder.sign(&key, MessageDigest::sha256()).unwrap(); + let cert = builder.build(); + + let cert_pem = String::from_utf8(cert.to_pem().unwrap()).unwrap(); + let key_pem = + String::from_utf8(key.private_key_to_pem_pkcs8().unwrap()).unwrap(); + (cert_pem, key_pem) + } + + /// The certificate loader must apply the same acceptance check as the + /// external endpoint code, so that a stored certificate Nexus cannot + /// serve does not mask the expiring certificate it does serve. + #[tokio::test] + async fn test_load_silo_certificates_skips_unservable_certificates() { + use async_bb8_diesel::AsyncRunQueryDsl; + use nexus_db_model::Certificate; + use nexus_db_schema::schema::certificate::dsl; + use nexus_types::external_api::certificate::CertificateCreate; + use nexus_types::external_api::certificate::ServiceUsingCertificate; + use nexus_types::silo::DEFAULT_SILO_ID; + use omicron_certificates::CertificateValidator; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_test_utils::certificates::CertificateChain; + use uuid::Uuid; + + let logctx = dev::test_setup_log( + "test_load_silo_certificates_skips_unservable_certificates", + ); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + const HOSTNAME: &str = "fake.test.oxide.computer"; + let make_cert = |name: &str, cert: String, key: String| { + Certificate::new_unvalidated( + DEFAULT_SILO_ID, + Uuid::new_v4(), + ServiceKind::Nexus, + CertificateCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: String::new(), + }, + cert, + key, + service: ServiceUsingCertificate::ExternalApi, + }, + ) + }; + + // A certificate Nexus can serve, expiring soon. + let mut params = + rcgen::CertificateParams::new(vec![HOSTNAME.to_string()]); + params.not_after = (std::time::SystemTime::now() + + std::time::Duration::from_secs(5 * 24 * 60 * 60)) + .into(); + let chain = CertificateChain::with_params(params); + let servable = make_cert( + "servable", + chain.cert_chain_as_pem(), + chain.end_cert_private_key_as_pem(), + ); + let servable_id = servable.id(); + + // A certificate Nexus cannot serve, expiring much later. If the + // loader admitted it, it would be the silo's "best" certificate and + // the expiring one above would go unreported. + let (cert, key) = unservable_certificate( + HOSTNAME, + Utc::now() + chrono::Duration::days(730), + ); + // Confirm the premise: upload validation accepts this certificate... + CertificateValidator::default() + .validate(cert.as_bytes(), key.as_bytes(), &[HOSTNAME]) + .expect("upload validation accepts a 1024-bit RSA certificate"); + let unservable = make_cert("unservable", cert, key); + let unservable_id = unservable.id(); + // ...but the external endpoint code refuses to serve it. + TlsCertificate::try_from(unservable.clone()) + .err() + .expect("external endpoints reject a 1024-bit RSA certificate"); + + diesel::insert_into(dsl::certificate) + .values(vec![servable, unservable]) + .execute_async( + &*datastore.pool_connection_for_tests().await.unwrap(), + ) + .await + .expect("inserted certificates"); + + let (_sitrep_tx, sitrep_rx) = watch::channel(None); + let (_inv_tx, inv_rx) = watch::channel(None); + let task = FmAnalysis::new( + datastore.clone(), + sitrep_rx, + inv_rx, + config_rx(Default::default()), + activators(), + OmicronZoneUuid::new_v4(), + ); + + let mut warnings = Vec::new(); + let silos = task + .load_silo_certificates(opctx, &mut warnings) + .await + .expect("loaded silo certificates"); + + let silo = silos.get(&DEFAULT_SILO_ID).expect("default silo observed"); + let observed_ids: Vec = + silo.certificates.iter().map(|c| c.id).collect(); + assert_eq!(observed_ids, vec![servable_id]); + assert_eq!( + silo.best_certificate().map(|c| c.id), + Some(servable_id), + "the expiring servable certificate must be the best one" + ); + let observed = silo.certificates.get(&servable_id).unwrap(); + assert!(observed.not_before <= Utc::now()); + assert!(observed.not_after > Utc::now()); + assert!(observed.not_after < Utc::now() + chrono::Duration::days(6)); + + assert_eq!(warnings.len(), 1, "unexpected warnings: {warnings:?}"); + assert!( + warnings[0].contains(&unservable_id.to_string()) + && warnings[0].contains("cannot be served"), + "unexpected warning: {}", + warnings[0] + ); + + db.terminate().await; + logctx.cleanup_successful(); + } + /// Exercises `check_sitrep_limit` at each capacity tier: /// /// - below 80% of the limit, the capacity is reported and nothing else diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index 3e1cd58ad54..6bb83a0163b 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -43,6 +43,7 @@ use nexus_db_queries::db::pagination::Paginator; use nexus_types::identity::Resource; use nexus_types::silo::DEFAULT_SILO_ID; use nexus_types::silo::silo_dns_name; +use omicron_certificates::CertificateValidity; use omicron_common::api::external::Error; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::bail_unless; @@ -388,9 +389,15 @@ impl PartialEq for ExternalEndpointError { } /// A parsed, validated TLS certificate ready to use with an external TLS server +/// +/// Constructing one of these (via `TryFrom`) is the acceptance +/// check for whether Nexus can serve a stored certificate at all. Anything +/// else that needs to reason about the certificates Nexus actually presents, +/// like the fault management certificate diagnosis engine, must go through the +/// same conversion so that it sees the same set of certificates. #[derive(Serialize)] #[serde(transparent)] -struct TlsCertificate { +pub(crate) struct TlsCertificate { /// This is what we need to provide to the TLS stack when we decide to use /// this certificate for an incoming TLS connection // NOTE: It's important that we do not serialize the private key! @@ -404,6 +411,10 @@ struct TlsCertificate { #[serde(skip)] parsed: X509, + /// Validity window of the leaf certificate + #[serde(skip)] + validity: CertificateValidity, + /// certificate digest (historically sometimes called a "fingerprint") // This is the only field that appears in the serialized output or debug // output. @@ -478,7 +489,17 @@ impl TryFrom for TlsCertificate { hex::encode(&digest_bytes) }; - Ok(TlsCertificate { certified_key, digest, parsed: end_cert }) + let validity = omicron_certificates::validity(&end_cert) + .context("reading leaf certificate validity")?; + + Ok(TlsCertificate { certified_key, digest, parsed: end_cert, validity }) + } +} + +impl TlsCertificate { + /// Returns the validity window of the leaf certificate + pub(crate) fn validity(&self) -> CertificateValidity { + self.validity } } From 4d7489d4570574263d75d73d130e2338b3529ef3 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:23:29 -0700 Subject: [PATCH 4/9] fm: fix wrapped warning string in the certificate loader The "silo not found" warning was wrapped without a trailing backslash, leaving a run of interior whitespace in the text omdb prints. --- nexus/src/app/background/tasks/fm_analysis.rs | 76 +++++++++++++------ 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index c72d4cee1d8..20cef4bc647 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -502,8 +502,8 @@ impl FmAnalysis { let silo_id = cert.silo_id; let Some(mut silo) = silos.get_mut(&silo_id) else { warnings.push(format!( - "certificate {cert_id} belongs to silo {}, which was not found; ignoring it", - cert.silo_id, + "certificate {cert_id} belongs to silo {silo_id}, \ + which was not found; ignoring it", )); continue; }; @@ -1570,22 +1570,23 @@ mod tests { let (opctx, datastore) = (db.opctx(), db.datastore()); const HOSTNAME: &str = "fake.test.oxide.computer"; - let make_cert = |name: &str, cert: String, key: String| { - Certificate::new_unvalidated( - DEFAULT_SILO_ID, - Uuid::new_v4(), - ServiceKind::Nexus, - CertificateCreate { - identity: IdentityMetadataCreateParams { - name: name.parse().unwrap(), - description: String::new(), + let make_cert = + |silo_id: Uuid, name: &str, cert: String, key: String| { + Certificate::new_unvalidated( + silo_id, + Uuid::new_v4(), + ServiceKind::Nexus, + CertificateCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: String::new(), + }, + cert, + key, + service: ServiceUsingCertificate::ExternalApi, }, - cert, - key, - service: ServiceUsingCertificate::ExternalApi, - }, - ) - }; + ) + }; // A certificate Nexus can serve, expiring soon. let mut params = @@ -1595,6 +1596,7 @@ mod tests { .into(); let chain = CertificateChain::with_params(params); let servable = make_cert( + DEFAULT_SILO_ID, "servable", chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem(), @@ -1612,15 +1614,27 @@ mod tests { CertificateValidator::default() .validate(cert.as_bytes(), key.as_bytes(), &[HOSTNAME]) .expect("upload validation accepts a 1024-bit RSA certificate"); - let unservable = make_cert("unservable", cert, key); + let unservable = make_cert(DEFAULT_SILO_ID, "unservable", cert, key); let unservable_id = unservable.id(); // ...but the external endpoint code refuses to serve it. TlsCertificate::try_from(unservable.clone()) .err() .expect("external endpoints reject a 1024-bit RSA certificate"); + // A servable certificate whose silo does not exist. The loader has + // no silo to attach it to, so it is skipped with a warning. + let orphan_silo_id = Uuid::new_v4(); + let orphan_chain = CertificateChain::new(HOSTNAME); + let orphan = make_cert( + orphan_silo_id, + "orphan", + orphan_chain.cert_chain_as_pem(), + orphan_chain.end_cert_private_key_as_pem(), + ); + let orphan_id = orphan.id(); + diesel::insert_into(dsl::certificate) - .values(vec![servable, unservable]) + .values(vec![servable, unservable, orphan]) .execute_async( &*datastore.pool_connection_for_tests().await.unwrap(), ) @@ -1658,12 +1672,26 @@ mod tests { assert!(observed.not_after > Utc::now()); assert!(observed.not_after < Utc::now() + chrono::Duration::days(6)); - assert_eq!(warnings.len(), 1, "unexpected warnings: {warnings:?}"); + // Both skipped certificates are reported, and the warnings are + // readable: no run of interior whitespace from a wrapped literal. + assert_eq!(warnings.len(), 2, "unexpected warnings: {warnings:?}"); + for warning in &warnings { + assert!(!warning.contains(" "), "unexpected warning: {warning}"); + } + assert!( + warnings.iter().any(|w| { + w.contains(&unservable_id.to_string()) + && w.contains("cannot be served") + }), + "unexpected warnings: {warnings:?}" + ); assert!( - warnings[0].contains(&unservable_id.to_string()) - && warnings[0].contains("cannot be served"), - "unexpected warning: {}", - warnings[0] + warnings.iter().any(|w| { + w.contains(&orphan_id.to_string()) + && w.contains(&orphan_silo_id.to_string()) + && w.contains("which was not found") + }), + "unexpected warnings: {warnings:?}" ); db.terminate().await; From 9e449d01e96885ff249f8df1777b7c5ff7020141 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:30:41 -0700 Subject: [PATCH 5/9] fm: break best-certificate ties deterministically ObservedSiloCertificates::best_certificate claimed callers depended only on not_after, but the engine stores the chosen certificate's id in its fact and re-alerts when it changes. Break ties toward the greatest id, document why, and test that equal-expiry certificates carry the fact unchanged across sitreps. --- nexus/fm/src/diagnosis/certificate.rs | 53 ++++++++++++++++++++++ nexus/src/app/external_endpoints.rs | 6 ++- nexus/types/src/observed_certificate.rs | 58 +++++++++++++++++++++++-- 3 files changed, 112 insertions(+), 5 deletions(-) diff --git a/nexus/fm/src/diagnosis/certificate.rs b/nexus/fm/src/diagnosis/certificate.rs index 02ed650b751..1f96b683797 100644 --- a/nexus/fm/src/diagnosis/certificate.rs +++ b/nexus/fm/src/diagnosis/certificate.rs @@ -944,6 +944,59 @@ mod tests { logctx.cleanup_successful(); } + /// Two certificates with the same `not_after` (say, the same PEM uploaded + /// under two names) must not make the engine flip between them across + /// sitreps: the fact and its alert are carried unchanged. + #[test] + fn equal_expiry_certificates_carry_fact_unchanged() { + let (logctx, collection) = + setup("equal_expiry_certificates_carry_fact_unchanged"); + let now = collection.time_done; + let not_after = now + TimeDelta::days(10); + let silos = silo_map([mk_silo( + SILO_A, + "fake-silo-a", + [ + mk_cert(CERT_1, "fake-cert-1", not_after), + mk_cert(CERT_2, "fake-cert-2", not_after), + ], + )]); + + let input1 = build_input( + collection.clone(), + None, + silos.clone(), + FmConfig::default(), + ); + let sitrep1 = run_analyze(&logctx.log, &input1, "run-1"); + let case1 = sole_open_case(&sitrep1).clone(); + let (fact1_id, fact1) = sole_fact(&case1); + assert_eq!( + fact1, + CertificateFact::BestCertificateExpiring( + CertificateExpiryFactPayload { + silo_id: SILO_A, + certificate_id: CERT_2, + not_after, + } + ), + "ties break toward the greatest certificate id" + ); + + let input2 = + build_input(collection, Some(sitrep1), silos, FmConfig::default()); + let sitrep2 = run_analyze(&logctx.log, &input2, "run-2"); + + let case2 = sole_open_case(&sitrep2); + assert_eq!(case2.id, case1.id); + let (fact2_id, fact2) = sole_fact(case2); + assert_eq!(fact2_id, fact1_id, "the fact should be carried unchanged"); + assert_eq!(fact2, fact1); + assert_alert_counts(case2, 1, 0); + assert_eq!(case2.alerts_requested, case1.alerts_requested); + logctx.cleanup_successful(); + } + #[test] fn window_override_changes_outcome() { let (logctx, collection) = setup("window_override_changes_outcome"); diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index 6bb83a0163b..7311fe2dbff 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -318,7 +318,11 @@ impl ExternalEndpoint { // same rule, so that it alerts when the certificate we will actually // serve is expiring or expired. If the rule here changes, the engine // (and `ObservedSiloCertificates::best_certificate`, which it uses) - // must change with it. + // must change with it. When several certificates share the latest + // expiration, the engine breaks the tie toward the greatest + // certificate id; this loop may settle on a different one of them, + // but they expire at the same time, so the engine's prediction of + // when the served certificate expires holds either way. // This would be cleaner if Asn1Time impl'd Ord or even just a way to // convert it to a Unix timestamp or any other comparable timestamp. diff --git a/nexus/types/src/observed_certificate.rs b/nexus/types/src/observed_certificate.rs index 7ce65eb0af9..3863afada92 100644 --- a/nexus/types/src/observed_certificate.rs +++ b/nexus/types/src/observed_certificate.rs @@ -36,11 +36,18 @@ impl ObservedSiloCertificates { /// This must stay in lockstep with `ExternalEndpoint::best_certificate` /// in Nexus, which applies the same rule to choose the certificate /// actually presented to TLS clients. Like that function, this ignores - /// `not_before`. When several certificates share the latest `not_after`, - /// which one is returned is unspecified; callers only depend on the - /// `not_after` value itself. + /// `not_before`. + /// + /// When several certificates share the latest `not_after`, the one with + /// the greatest id is returned. The tiebreak matters because the + /// certificate diagnosis engine records the chosen certificate's id in + /// its facts and treats a change of id as a new condition worth a fresh + /// alert, so the choice must not vary between analyses of the same set of + /// certificates. Nexus may serve a different one of the tied certificates, + /// but it expires at the same time, so what the engine says about the + /// expiration of the served certificate holds either way. pub fn best_certificate(&self) -> Option<&ObservedCertificate> { - self.certificates.iter().max_by_key(|cert| cert.not_after) + self.certificates.iter().max_by_key(|cert| (cert.not_after, cert.id)) } } @@ -72,3 +79,46 @@ impl IdOrdItem for ObservedCertificate { } id_upcast!(); } + +#[cfg(test)] +mod tests { + use super::*; + + fn cert(id: u128, not_after: DateTime) -> ObservedCertificate { + ObservedCertificate { + id: Uuid::from_u128(id), + name: format!("fake-cert-{id}").parse().unwrap(), + not_before: not_after - chrono::TimeDelta::days(365), + not_after, + } + } + + fn silo( + certs: impl IntoIterator, + ) -> ObservedSiloCertificates { + ObservedSiloCertificates { + silo_id: Uuid::from_u128(0xA), + silo_name: "fake-silo".parse().unwrap(), + certificates: certs.into_iter().collect(), + } + } + + #[test] + fn best_certificate_prefers_latest_not_after() { + let t = DateTime::from_timestamp(1_700_000_000, 0).unwrap(); + // The later expiration wins even when it has the smaller id. + let s = silo([cert(2, t), cert(1, t + chrono::TimeDelta::days(1))]); + assert_eq!(s.best_certificate().unwrap().id, Uuid::from_u128(1)); + assert!(silo([]).best_certificate().is_none()); + } + + #[test] + fn best_certificate_breaks_ties_toward_greatest_id() { + let t = DateTime::from_timestamp(1_700_000_000, 0).unwrap(); + // Insertion order must not matter. + for certs in [[cert(1, t), cert(2, t)], [cert(2, t), cert(1, t)]] { + let s = silo(certs); + assert_eq!(s.best_certificate().unwrap().id, Uuid::from_u128(2)); + } + } +} From 391d604ed0516c8869f797850ec0fc5d25ba2364 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:35:56 -0700 Subject: [PATCH 6/9] nexus: pick the best certificate by its converted not_after TlsCertificate now carries the leaf validity as a DateTime, so the served-certificate rule can be the same max_by_key expression the certificate diagnosis engine uses, instead of a hand-rolled loop over Asn1Time partial_cmp. Drop the parsed X509 field, which had no other readers left. --- nexus/src/app/external_endpoints.rs | 61 ++++++++--------------------- 1 file changed, 16 insertions(+), 45 deletions(-) diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index 7311fe2dbff..33c2e77674f 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -48,7 +48,6 @@ use omicron_common::api::external::Error; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::bail_unless; use openssl::pkey::PKey; -use openssl::x509::X509; use rustls::sign::CertifiedKey; use serde::Serialize; use serde_with::SerializeDisplay; @@ -314,41 +313,18 @@ impl ExternalEndpoint { // time. // // The fault management certificate diagnosis engine - // (`nexus_fm::diagnosis::certificate`) predicts this choice from the - // same rule, so that it alerts when the certificate we will actually - // serve is expiring or expired. If the rule here changes, the engine - // (and `ObservedSiloCertificates::best_certificate`, which it uses) - // must change with it. When several certificates share the latest - // expiration, the engine breaks the tie toward the greatest - // certificate id; this loop may settle on a different one of them, - // but they expire at the same time, so the engine's prediction of - // when the served certificate expires holds either way. - - // This would be cleaner if Asn1Time impl'd Ord or even just a way to - // convert it to a Unix timestamp or any other comparable timestamp. - let mut latest_expiration: Option<&TlsCertificate> = None; - for t in &self.tls_certs { - // We'll choose this certificate (so far) if we find that it's - // anything other than "earlier" than the best we've seen so far. - // That includes the case where we haven't seen any so far, where - // this one is greater than or equal to the best so far, as well as - // the case where they're incomparable for whatever reason. (This - // ensures that we always pick at least one.) - if latest_expiration.is_none() - || !matches!( - t.parsed.not_after().partial_cmp( - latest_expiration.unwrap().parsed.not_after() - ), - Some(std::cmp::Ordering::Less) - ) - { - latest_expiration = Some(t); - } - } - - latest_expiration.ok_or_else(|| { - anyhow!("silo {} has no usable certificates", self.silo_id) - }) + // (`nexus_fm::diagnosis::certificate`) predicts this choice with the + // same rule (`ObservedSiloCertificates::best_certificate`), so that it + // alerts when the certificate we will actually serve is expiring or + // expired. If the rule here changes, that one must change with it. + // When several certificates share the latest expiration, the engine + // breaks the tie toward the greatest certificate id; this may settle + // on a different one of them, but they expire at the same time, so + // the engine's prediction of when the served certificate expires + // holds either way. + self.tls_certs.iter().max_by_key(|t| t.validity().not_after).ok_or_else( + || anyhow!("silo {} has no usable certificates", self.silo_id), + ) } } @@ -408,13 +384,6 @@ pub(crate) struct TlsCertificate { #[serde(skip)] certified_key: Arc, - /// Parsed representation of the whole certificate chain - /// - /// This is used to extract metadata like the expiration time. - // NOTE: It's important that we do not serialize the private key! - #[serde(skip)] - parsed: X509, - /// Validity window of the leaf certificate #[serde(skip)] validity: CertificateValidity, @@ -496,7 +465,7 @@ impl TryFrom for TlsCertificate { let validity = omicron_certificates::validity(&end_cert) .context("reading leaf certificate validity")?; - Ok(TlsCertificate { certified_key, digest, parsed: end_cert, validity }) + Ok(TlsCertificate { certified_key, digest, validity }) } } @@ -911,7 +880,9 @@ mod test { fn cert_matches(tls_cert: &TlsCertificate, cert: &Certificate) -> bool { let parse_right = openssl::x509::X509::from_pem(&cert.cert).unwrap(); - tls_cert.parsed == parse_right + let digest_right = + parse_right.digest(openssl::hash::MessageDigest::sha256()).unwrap(); + tls_cert.digest == hex::encode(&digest_right) } #[test] From 2e1ca7301175f4974628efd87f5ff691040aadb1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:41:30 -0700 Subject: [PATCH 7/9] fm: make certificate fact payload columns NOT NULL Both certificate fact kinds carry the same payload, so the nullable columns plus two identical per-kind CHECK constraints were NOT NULL spelled twice. Declare them NOT NULL, drop the Option fields and the unreachable missing-column error path in the Diesel model. --- nexus/db-model/src/fm/fact_certificate.rs | 45 ++++++----------------- nexus/db-queries/src/db/datastore/fm.rs | 6 +-- nexus/db-schema/src/schema.rs | 4 +- schema/crdb/dbinit.sql | 31 ++++------------ schema/crdb/fm-certificate-de/up3.sql | 31 ++++------------ 5 files changed, 31 insertions(+), 86 deletions(-) diff --git a/nexus/db-model/src/fm/fact_certificate.rs b/nexus/db-model/src/fm/fact_certificate.rs index 3d5c8d60c59..052215e4963 100644 --- a/nexus/db-model/src/fm/fact_certificate.rs +++ b/nexus/db-model/src/fm/fact_certificate.rs @@ -5,9 +5,8 @@ //! Database representation of the certificate diagnosis engine's facts. //! //! Each certificate fact is stored as typed columns in the -//! `fm_fact_certificate` table. The `kind` discriminant selects which payload -//! columns are populated; per-kind CHECK constraints enforce that the right -//! columns are non-NULL for each kind. See +//! `fm_fact_certificate` table. Every kind of certificate fact carries the +//! same payload, so the `kind` discriminant alone distinguishes them. See //! [`nexus_types::fm::CertificateFact`] for semantics. use crate::DbTypedUuid; @@ -19,7 +18,6 @@ use nexus_types::fm::case::FactMetadata; use nexus_types::fm::{ CertificateExpiryFactPayload, CertificateFact, FactPayload, }; -use omicron_common::api::external::Error; use omicron_uuid_kinds::{CaseKind, FactKind, SitrepKind}; use uuid::Uuid; @@ -34,9 +32,6 @@ impl_enum_type!( ); /// Diesel row for the `fm_fact_certificate` table. -/// -/// The payload columns are populated according to `kind`: a column is `Some` -/// if it belongs to that `kind`'s payload, and `None` otherwise. #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = fm_fact_certificate)] pub struct FmFactCertificate { @@ -57,9 +52,8 @@ pub struct FmFactCertificate { pub silo_id: Uuid, pub kind: FmFactCertificateKind, - // Columns shared by both kinds. - pub certificate_id: Option, - pub not_after: Option>, + pub certificate_id: Uuid, + pub not_after: DateTime, } impl FmFactCertificate { @@ -89,24 +83,19 @@ impl FmFactCertificate { comment: comment.clone(), silo_id: payload.silo_id, kind, - certificate_id: Some(payload.certificate_id), - not_after: Some(payload.not_after), + certificate_id: payload.certificate_id, + not_after: payload.not_after, } } /// Reconstruct an in-memory fact from a row. - pub fn into_fact(self) -> Result { - let kind = self.kind; + pub fn into_fact(self) -> fm::case::Fact { let payload = CertificateExpiryFactPayload { silo_id: self.silo_id, - certificate_id: self - .certificate_id - .ok_or_else(|| missing_column(kind, "certificate_id"))?, - not_after: self - .not_after - .ok_or_else(|| missing_column(kind, "not_after"))?, + certificate_id: self.certificate_id, + not_after: self.not_after, }; - let payload = match kind { + let payload = match self.kind { FmFactCertificateKind::BestCertificateExpiring => { FactPayload::Certificate( CertificateFact::BestCertificateExpiring(payload), @@ -118,23 +107,13 @@ impl FmFactCertificate { ) } }; - Ok(fm::case::Fact { + fm::case::Fact { metadata: fm::case::FactMetadata { id: self.id.into(), created_sitrep_id: self.created_sitrep_id.into(), comment: self.comment, }, payload, - }) - } -} - -fn missing_column(kind: FmFactCertificateKind, column: &str) -> Error { - Error::InternalError { - internal_message: format!( - "fm_fact_certificate row of kind {kind:?} has a NULL {column}, \ - violating the CHECK constraint requiring it to be non-NULL for \ - this kind" - ), + } } } diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 59e9dce20bb..14e77449ecf 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -663,11 +663,7 @@ impl DataStore { paginator = p.found_batch(&batch, &|f| f.id); for row in batch { let case_id: CaseUuid = row.case_id.into(); - let fact_id = row.id; - let fact = row.into_fact().with_internal_context(|| { - format!("failed to read fact {fact_id} on case {case_id}") - })?; - insert_fact_for_case(&mut by_case, case_id, fact)?; + insert_fact_for_case(&mut by_case, case_id, row.into_fact())?; } } diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 16b238d941b..336e07d2d6f 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -3426,8 +3426,8 @@ table! { comment -> Text, silo_id -> Uuid, kind -> crate::enums::FmFactCertificateKindEnum, - certificate_id -> Nullable, - not_after -> Nullable, + certificate_id -> Uuid, + not_after -> Timestamptz, } } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 297e3617f77..078deb441ce 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -8447,32 +8447,17 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_certificate ( -- looked up from the silo and certificate tables when a case is acted on. silo_id UUID NOT NULL, - -- Which certificate fact this row represents. The columns below are - -- populated according to this discriminant (see the CHECK constraints). + -- Which certificate fact this row represents. kind omicron.public.fm_fact_certificate_kind NOT NULL, - -- Columns shared by the 'best_certificate_expiring' and - -- 'best_certificate_expired' kinds: the silo's best certificate (latest - -- leaf `not_after`) when the fact was recorded, and that `not_after`. - certificate_id UUID, - not_after TIMESTAMPTZ, + -- Both kinds carry the same payload: the silo's best certificate (latest + -- leaf `not_after`) when the fact was recorded, and that `not_after`. A + -- kind with a different payload would add nullable columns here with a + -- CHECK constraint keyed on `kind`, as `fm_fact_saga` does. + certificate_id UUID NOT NULL, + not_after TIMESTAMPTZ NOT NULL, - PRIMARY KEY (sitrep_id, id), - - -- Each kind's constraint checks only that its own columns are present, - -- not that others are NULL, so future kinds may share columns. - CONSTRAINT best_certificate_expiring_columns_present CHECK ( - kind != 'best_certificate_expiring' OR ( - certificate_id IS NOT NULL - AND not_after IS NOT NULL - ) - ), - CONSTRAINT best_certificate_expired_columns_present CHECK ( - kind != 'best_certificate_expired' OR ( - certificate_id IS NOT NULL - AND not_after IS NOT NULL - ) - ) + PRIMARY KEY (sitrep_id, id) ); CREATE TABLE IF NOT EXISTS omicron.public.fm_ereport_in_case ( diff --git a/schema/crdb/fm-certificate-de/up3.sql b/schema/crdb/fm-certificate-de/up3.sql index 40c6a919d96..b93e95ac489 100644 --- a/schema/crdb/fm-certificate-de/up3.sql +++ b/schema/crdb/fm-certificate-de/up3.sql @@ -21,30 +21,15 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_certificate ( -- looked up from the silo and certificate tables when a case is acted on. silo_id UUID NOT NULL, - -- Which certificate fact this row represents. The columns below are - -- populated according to this discriminant (see the CHECK constraints). + -- Which certificate fact this row represents. kind omicron.public.fm_fact_certificate_kind NOT NULL, - -- Columns shared by the 'best_certificate_expiring' and - -- 'best_certificate_expired' kinds: the silo's best certificate (latest - -- leaf `not_after`) when the fact was recorded, and that `not_after`. - certificate_id UUID, - not_after TIMESTAMPTZ, + -- Both kinds carry the same payload: the silo's best certificate (latest + -- leaf `not_after`) when the fact was recorded, and that `not_after`. A + -- kind with a different payload would add nullable columns here with a + -- CHECK constraint keyed on `kind`, as `fm_fact_saga` does. + certificate_id UUID NOT NULL, + not_after TIMESTAMPTZ NOT NULL, - PRIMARY KEY (sitrep_id, id), - - -- Each kind's constraint checks only that its own columns are present, - -- not that others are NULL, so future kinds may share columns. - CONSTRAINT best_certificate_expiring_columns_present CHECK ( - kind != 'best_certificate_expiring' OR ( - certificate_id IS NOT NULL - AND not_after IS NOT NULL - ) - ), - CONSTRAINT best_certificate_expired_columns_present CHECK ( - kind != 'best_certificate_expired' OR ( - certificate_id IS NOT NULL - AND not_after IS NOT NULL - ) - ) + PRIMARY KEY (sitrep_id, id) ); From de88eded9993414155fd622ec29397adcc40e3e8 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:53:55 -0700 Subject: [PATCH 8/9] fm: use silo_list_all_batched in the certificate loader The loader hand-rolled the same pagination loop the datastore helper already provides, with an ad-hoc batch size. Use the helper for silos and the standard SQL_BATCH_SIZE for certificates. --- nexus/src/app/background/tasks/fm_analysis.rs | 46 +++++++------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 20cef4bc647..7bb39cc5ec1 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -48,7 +48,6 @@ use omicron_uuid_kinds::SupportBundleUuid; use serde_json::json; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; -use std::num::NonZeroU32; use std::sync::Arc; use tokio::sync::watch; @@ -453,37 +452,26 @@ impl FmAnalysis { opctx: &OpContext, warnings: &mut Vec, ) -> anyhow::Result> { - // The batch size is arbitrary; most systems have a handful of silos - // and certificates, and a few have a few hundred certificates. - let batch_size = NonZeroU32::new(200).unwrap(); - let mut silos = IdOrdMap::new(); - let mut paginator = - Paginator::new(batch_size, dropshot::PaginationOrder::Ascending); - while let Some(p) = paginator.next() { - let batch = self - .datastore - .silos_list( - opctx, - &PaginatedBy::Id(p.current_pagparams()), - Discoverability::All, - ) - .await - .context("failed to list silos")?; - paginator = p.found_batch(&batch, &|s| s.id()); - for silo in batch { - silos - .insert_unique(ObservedSiloCertificates { - silo_id: silo.id(), - silo_name: silo.name().clone(), - certificates: IdOrdMap::new(), - }) - .expect("silo IDs are unique"); - } + for silo in self + .datastore + .silo_list_all_batched(opctx, Discoverability::All) + .await + .context("failed to list silos")? + { + silos + .insert_unique(ObservedSiloCertificates { + silo_id: silo.id(), + silo_name: silo.name().clone(), + certificates: IdOrdMap::new(), + }) + .expect("silo IDs are unique"); } - let mut paginator = - Paginator::new(batch_size, dropshot::PaginationOrder::Ascending); + let mut paginator = Paginator::new( + datastore::SQL_BATCH_SIZE, + dropshot::PaginationOrder::Ascending, + ); while let Some(p) = paginator.next() { let batch = self .datastore From 4a645936f92339ba85b20858000cba64532c5116 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 4 Sep 2026 16:56:12 -0700 Subject: [PATCH 9/9] fm: make the parsed certificate case's fact non-optional parse_case returns NoFacts when a case has no facts, so a successfully parsed case always has one. Store it directly and derive the silo id from it instead of threading an Option through the reconcile loop. --- nexus/fm/src/diagnosis/certificate.rs | 47 ++++++++++++++------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/nexus/fm/src/diagnosis/certificate.rs b/nexus/fm/src/diagnosis/certificate.rs index 1f96b683797..7ff4cb454c4 100644 --- a/nexus/fm/src/diagnosis/certificate.rs +++ b/nexus/fm/src/diagnosis/certificate.rs @@ -43,11 +43,12 @@ use uuid::Uuid; /// A parent-forwarded Certificate case, parsed into the form this engine acts /// on. Every fact on a certificate case is about the same silo, and a case -/// carries at most one fact. +/// carries exactly one fact. struct ParsedCertificateCase { + /// The silo `fact` is about. silo_id: Uuid, /// The fact to consider when advancing the case. - fact: Option<(FactUuid, CertificateFact)>, + fact: (FactUuid, CertificateFact), /// Facts that should not exist: any beyond the first. They carry no /// information the kept fact doesn't. duplicate_facts: Vec, @@ -75,31 +76,33 @@ enum UninterpretableCase { fn parse_case( case: &fm::Case, ) -> Result { - let mut silo_id: Option = None; let mut kept: Option<(FactUuid, CertificateFact)> = None; let mut duplicate_facts = Vec::new(); // `case.facts` iterates in fact UUID order, so the kept fact is // deterministically the one with the lowest UUID. for fact in case.facts.iter() { let cert_fact = fact.as_certificate()?; - let this_silo = cert_fact.silo_id(); - let expected = *silo_id.get_or_insert(this_silo); - if expected != this_silo { - return Err(UninterpretableCase::DisagreeingSilos { - expected, - found: this_silo, - }); - } - if kept.is_none() { - kept = Some((fact.metadata.id, cert_fact.clone())); - } else { - duplicate_facts.push(fact.metadata.id); + match &kept { + None => kept = Some((fact.metadata.id, cert_fact.clone())), + Some((_, first)) => { + if first.silo_id() != cert_fact.silo_id() { + return Err(UninterpretableCase::DisagreeingSilos { + expected: first.silo_id(), + found: cert_fact.silo_id(), + }); + } + duplicate_facts.push(fact.metadata.id); + } } } - let Some(silo_id) = silo_id else { + let Some(fact) = kept else { return Err(UninterpretableCase::NoFacts); }; - Ok(ParsedCertificateCase { silo_id, fact: kept, duplicate_facts }) + Ok(ParsedCertificateCase { + silo_id: fact.1.silo_id(), + fact, + duplicate_facts, + }) } pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { @@ -244,11 +247,11 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { } } - let carried = parent.and_then(|(_, p)| p.fact.as_ref()); - if carried.map(|(_, fact)| fact) == Some(&desired) { - continue; - } - if let Some((fact_id, _)) = carried { + if let Some((_, parsed_case)) = parent { + let (fact_id, carried) = &parsed_case.fact; + if *carried == desired { + continue; + } case_mut.remove_fact( *fact_id, "fact no longer matches the silo's best certificate",