From 33179bead3247f0e942bbcd148119710f9729f1a Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 11 Dec 2023 13:56:31 -0500 Subject: [PATCH 1/3] [nexus] Allow silo admins to upload new certs The additional cert validation added in #4100 broke the ability for silo admins to upload new certs, because it introduced a call to fetch the rack DNS configuration (in order to assemble the FQDNs for the silo to check that the cert is valid for them). This PR fixes that by using an elevated internal privilege for that DNS config lookup. Fixes #4532. --- nexus/src/app/certificate.rs | 27 +++++++- nexus/tests/integration_tests/silos.rs | 85 ++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/certificate.rs b/nexus/src/app/certificate.rs index 71be93f5b7..870ad81985 100644 --- a/nexus/src/app/certificate.rs +++ b/nexus/src/app/certificate.rs @@ -6,6 +6,7 @@ use crate::external_api::params; use crate::external_api::shared; +use nexus_db_queries::authn; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; @@ -20,6 +21,7 @@ use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::NameOrId; use ref_cast::RefCast; +use std::sync::Arc; use uuid::Uuid; impl super::Nexus { @@ -47,8 +49,29 @@ impl super::Nexus { .silo_required() .internal_context("creating a Certificate")?; - let silo_fq_dns_names = - self.silo_fq_dns_names(opctx, authz_silo.id()).await?; + // The `opctx` we received is going to be checked for permission to + // create a cert below in `db_datastore.certificate_create`, but first + // we need to look up this silo's fully-qualified domain names in order + // to check that the cert we've been given is valid for this silo. + // Looking up DNS names requires reading the DNS configuration of the + // _rack_, which this user may not be able to do (even if they have + // permission to upload new certs, which almost certainly implies a + // silo-level admin. We'll construct a new opctx here to allow this DNS + // config lookup, because we believe it does not leak any information + // that a silo admin doesn't already know (the external DNS name(s) of + // the rack, which leads to their silo's DNS name(s)). + // + // See https://github.com/oxidecomputer/omicron/issues/4532 for + // additional background. + let silo_fq_dns_names = { + let dns_opctx = OpContext::for_background( + opctx.log.clone(), + Arc::clone(&self.authz), + authn::Context::internal_service_balancer(), + Arc::clone(self.datastore()), + ); + self.silo_fq_dns_names(&dns_opctx, authz_silo.id()).await? + }; let kind = params.service; let new_certificate = db::model::Certificate::new( diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index e5f41d294d..1f8a714c70 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::integration_tests::saml::SAML_IDP_DESCRIPTOR; +use dropshot::ResultsPage; use nexus_db_queries::authn::silos::{ AuthenticatedSubject, IdentityProviderType, }; @@ -19,6 +20,7 @@ use nexus_test_utils::resource_helpers::{ objects_list_page_authz, projects_list, }; use nexus_test_utils_macros::nexus_test; +use nexus_types::external_api::views::Certificate; use nexus_types::external_api::views::{ self, IdentityProvider, Project, SamlIdentityProvider, Silo, }; @@ -27,6 +29,7 @@ use omicron_common::api::external::ObjectIdentity; use omicron_common::api::external::{ IdentityMetadataCreateParams, LookupType, Name, }; +use omicron_test_utils::certificates::CertificateChain; use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError}; use std::collections::{BTreeMap, BTreeSet, HashSet}; @@ -2437,3 +2440,85 @@ async fn check_fleet_privileges( .unwrap(); } } + +// Test that a silo admin can create new certificates for their silo +// +// Internally, the certificate validation check requires the `authz::DNS_CONFIG` +// resource (to check that the certificate is valid for +// `{silo_name}.{external_dns_zone_name}`), which silo admins may not have. We +// have to use an alternate, elevated context to perform that check, and this +// test confirms we do so. +// +// Ensures no regression on +// https://github.com/oxidecomputer/omicron/issues/4532. +#[nexus_test] +async fn test_silo_admin_can_create_certs(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let certs_url = "/v1/certificates"; + + // Create a silo with an admin user + let silo = create_silo( + client, + "silo-name", + true, + shared::SiloIdentityMode::LocalOnly, + ) + .await; + + let new_silo_user_id = create_local_user( + client, + &silo, + &"admin".parse().unwrap(), + params::UserPassword::LoginDisallowed, + ) + .await + .id; + + grant_iam( + client, + "/v1/system/silos/silo-name", + SiloRole::Admin, + new_silo_user_id, + AuthnMode::PrivilegedUser, + ) + .await; + + // The user should be able to create certs for this silo + let chain = CertificateChain::new(cptestctx.wildcard_silo_dns_name()); + let (cert, key) = + (chain.cert_chain_as_pem(), chain.end_cert_private_key_as_pem()); + + let cert: Certificate = NexusRequest::objects_post( + client, + certs_url, + ¶ms::CertificateCreate { + identity: IdentityMetadataCreateParams { + name: "test-cert".parse().unwrap(), + description: "the test cert".to_string(), + }, + cert, + key, + service: shared::ServiceUsingCertificate::ExternalApi, + }, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .expect("failed to create certificate") + .parsed_body() + .unwrap(); + + // The cert should exist when listing the silo's certs as the silo admin + let silo_certs = + NexusRequest::object_get(client, &format!("{certs_url}?limit=10")) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .expect("failed to list certificates") + .parsed_body::>() + .expect("failed to parse body as ResultsPage") + .items; + + assert_eq!(silo_certs.len(), 1); + assert_eq!(silo_certs[0].identity.id, cert.identity.id); +} From fa74a7aa263a2ed092867b3e952c22300a7e05d5 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 12 Dec 2023 14:40:10 -0500 Subject: [PATCH 2/3] review feedback --- nexus/src/app/certificate.rs | 27 +++++++++----------------- nexus/tests/integration_tests/silos.rs | 3 --- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/nexus/src/app/certificate.rs b/nexus/src/app/certificate.rs index 870ad81985..1caa4b3ad0 100644 --- a/nexus/src/app/certificate.rs +++ b/nexus/src/app/certificate.rs @@ -6,7 +6,6 @@ use crate::external_api::params; use crate::external_api::shared; -use nexus_db_queries::authn; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; @@ -21,7 +20,6 @@ use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::NameOrId; use ref_cast::RefCast; -use std::sync::Arc; use uuid::Uuid; impl super::Nexus { @@ -56,22 +54,15 @@ impl super::Nexus { // Looking up DNS names requires reading the DNS configuration of the // _rack_, which this user may not be able to do (even if they have // permission to upload new certs, which almost certainly implies a - // silo-level admin. We'll construct a new opctx here to allow this DNS - // config lookup, because we believe it does not leak any information - // that a silo admin doesn't already know (the external DNS name(s) of - // the rack, which leads to their silo's DNS name(s)). - // - // See https://github.com/oxidecomputer/omicron/issues/4532 for - // additional background. - let silo_fq_dns_names = { - let dns_opctx = OpContext::for_background( - opctx.log.clone(), - Arc::clone(&self.authz), - authn::Context::internal_service_balancer(), - Arc::clone(self.datastore()), - ); - self.silo_fq_dns_names(&dns_opctx, authz_silo.id()).await? - }; + // silo-level admin. We'll use our `opctx_external_authn()` context, + // which is the same context used to create a silo. This is a higher + // privelege than the current user may have, but we believe it does not + // leak any information that a silo admin doesn't already know (the + // external DNS name(s) of the rack, which leads to their silo's DNS + // name(s)). + let silo_fq_dns_names = self + .silo_fq_dns_names(self.opctx_external_authn(), authz_silo.id()) + .await?; let kind = params.service; let new_certificate = db::model::Certificate::new( diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 1f8a714c70..3c69c8b7cd 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -2448,9 +2448,6 @@ async fn check_fleet_privileges( // `{silo_name}.{external_dns_zone_name}`), which silo admins may not have. We // have to use an alternate, elevated context to perform that check, and this // test confirms we do so. -// -// Ensures no regression on -// https://github.com/oxidecomputer/omicron/issues/4532. #[nexus_test] async fn test_silo_admin_can_create_certs(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; From 726776b507e48d84acb022edf2c4febee626a64f Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 12 Dec 2023 15:36:44 -0500 Subject: [PATCH 3/3] typo fixes --- nexus/src/app/certificate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/certificate.rs b/nexus/src/app/certificate.rs index 1caa4b3ad0..2f130d1ad3 100644 --- a/nexus/src/app/certificate.rs +++ b/nexus/src/app/certificate.rs @@ -54,9 +54,9 @@ impl super::Nexus { // Looking up DNS names requires reading the DNS configuration of the // _rack_, which this user may not be able to do (even if they have // permission to upload new certs, which almost certainly implies a - // silo-level admin. We'll use our `opctx_external_authn()` context, + // silo-level admin). We'll use our `opctx_external_authn()` context, // which is the same context used to create a silo. This is a higher - // privelege than the current user may have, but we believe it does not + // privilege than the current user may have, but we believe it does not // leak any information that a silo admin doesn't already know (the // external DNS name(s) of the rack, which leads to their silo's DNS // name(s)).