Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nexus] Allow silo admins to upload new certs #4669

Merged
merged 3 commits into from
Dec 12, 2023
Merged

[nexus] Allow silo admins to upload new certs #4669

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33179be
[nexus] Allow silo admins to upload new certs
jgallagher Dec 11, 2023
fa74a7a
review feedback
jgallagher Dec 12, 2023
726776b
typo fixes
jgallagher Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions nexus/src/app/certificate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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()),
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we've been a little inconsistent on how we create OpContexts that Nexus uses to do privileged things on behalf of unprivileged users.

  • For the "instance allocator" and "external authn" OpContexts, we create them once at startup and store them into Nexus. Callers can access the former directly as a field. They can access the latter that way or using self.opctx_external_authn().
  • For the service balancer and internal API OpContexts, we have similar methods self.opctx_for_service_balancer() and self.opctx_for_internal_api(), but these construct the OpContext anew each time rather than just doing that once on startup.

I generally prefer the first approach but clearly it's common to do the latter too. I was going to suggest not doing this logic inline here, creating a helper instead, although maybe you did this on purpose to preserve the existing request log (in opctx.log)?

There's also the question of whether this is the right OpContext. This definitely isn't the service balancer...but neither is the other user of this OpContext that I found. In a sense, it really doesn't matter. These internal identities only exist to limit the privileges of code paths. You could create a new identity here that can only read DNS and use that. But it might be overkill to do that everywhere we need to use this pattern.

I see that we have an "internal-read" identity that only gets "fleet viewer", not "fleet admin". I also see that we already have Nexus.opctx_alloc using that identity. You could use that here and it'd be a little safer (fewer privileges) and also would avoid constructing a new OpContext inline with the request. The only downside is that the label attached to the log for that one is "InstanceAllocator", which would be a little misleading. You could also just go ahead and construct a new OpContext here using authn::Context::internal_read() instead, or add a helper to do it.

I don't have a strong feeling about all these options. I would at least use the internal_read() identity because there's not much work and I don't think there's any downside. As for whether to create a new identity, whether to create the OpContext inline or use a helper or use the existing one, these all have tradeoffs and one could go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing an offline chat:

  • We can't use "internal-read", because the DNS config policy only grants read to "fleet admin" (which "internal-read" isn't).
  • Per the comments on DnsConfig, Nexus uses the "external-authenticator" role when creating silos, so it seems reasonable to use it here as well.

Changed in fa74a7a.

self.silo_fq_dns_names(&dns_opctx, authz_silo.id()).await?
};

let kind = params.service;
let new_certificate = db::model::Certificate::new(
Expand Down
85 changes: 85 additions & 0 deletions nexus/tests/integration_tests/silos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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,
};
Expand All @@ -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};
Expand Down Expand Up @@ -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.
jgallagher marked this conversation as resolved.
Show resolved Hide resolved
#[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,
&params::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::<ResultsPage<Certificate>>()
.expect("failed to parse body as ResultsPage<Certificate>")
.items;

assert_eq!(silo_certs.len(), 1);
assert_eq!(silo_certs[0].identity.id, cert.identity.id);
}
Loading