From de1fa54088e9f2fe399ed0d86d4b5c652498f2c9 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Fri, 1 Sep 2023 15:49:30 -0700 Subject: [PATCH] nexus: allow IpPoolList access via silo conferred roles as well (#3999) We were missing loading silo conferred roles for authz checks against `IpPoolList`. Also rename and make load_roles_for_resource private to try to limit same issue in the future. --- nexus/db-queries/src/authz/api_resources.rs | 50 ++++------------- nexus/db-queries/src/authz/policy_test/mod.rs | 53 +++++++++++++------ nexus/db-queries/src/authz/roles.rs | 6 +-- .../tests/output/authz-conferred-roles.out | 42 +++++++++++++++ 4 files changed, 91 insertions(+), 60 deletions(-) diff --git a/nexus/db-queries/src/authz/api_resources.rs b/nexus/db-queries/src/authz/api_resources.rs index 9e590c7452..e7c2589110 100644 --- a/nexus/db-queries/src/authz/api_resources.rs +++ b/nexus/db-queries/src/authz/api_resources.rs @@ -29,9 +29,7 @@ use super::actor::AnyActor; use super::context::AuthorizedResource; use super::oso_generic::Init; -use super::roles::{ - load_roles_for_resource, load_roles_for_resource_tree, RoleSet, -}; +use super::roles::{load_roles_for_resource_tree, RoleSet}; use super::Action; use super::{actor::AuthenticatedActor, Authz}; use crate::authn; @@ -290,15 +288,8 @@ impl AuthorizedResource for ConsoleSessionList { 'd: 'f, 'e: 'f, { - load_roles_for_resource( - opctx, - datastore, - authn, - ResourceType::Fleet, - *FLEET_ID, - roleset, - ) - .boxed() + load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) + .boxed() } fn on_unauthorized( @@ -353,15 +344,8 @@ impl AuthorizedResource for DnsConfig { 'd: 'f, 'e: 'f, { - load_roles_for_resource( - opctx, - datastore, - authn, - ResourceType::Fleet, - *FLEET_ID, - roleset, - ) - .boxed() + load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) + .boxed() } fn on_unauthorized( @@ -418,16 +402,9 @@ impl AuthorizedResource for IpPoolList { { // There are no roles on the IpPoolList, only permissions. But we still // need to load the Fleet-related roles to verify that the actor has the - // "admin" role on the Fleet. - load_roles_for_resource( - opctx, - datastore, - authn, - ResourceType::Fleet, - *FLEET_ID, - roleset, - ) - .boxed() + // "admin" role on the Fleet (possibly conferred from a Silo role). + load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) + .boxed() } fn on_unauthorized( @@ -477,15 +454,8 @@ impl AuthorizedResource for DeviceAuthRequestList { // There are no roles on the DeviceAuthRequestList, only permissions. But we // still need to load the Fleet-related roles to verify that the actor has the // "admin" role on the Fleet. - load_roles_for_resource( - opctx, - datastore, - authn, - ResourceType::Fleet, - *FLEET_ID, - roleset, - ) - .boxed() + load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) + .boxed() } fn on_unauthorized( diff --git a/nexus/db-queries/src/authz/policy_test/mod.rs b/nexus/db-queries/src/authz/policy_test/mod.rs index cd5e6ad5f2..00a9904499 100644 --- a/nexus/db-queries/src/authz/policy_test/mod.rs +++ b/nexus/db-queries/src/authz/policy_test/mod.rs @@ -160,6 +160,7 @@ async fn test_iam_roles_behavior() { &logctx.log, &user_contexts, &test_resources, + true, ) .await .unwrap(); @@ -185,6 +186,7 @@ async fn authorize_everything( log: &slog::Logger, user_contexts: &[Arc<(String, OpContext)>], test_resources: &ResourceSet, + print_actions: bool, ) -> std::io::Result<()> { // Run the per-resource tests in parallel. Since the caller will be // checking the overall output against some expected output, it's important @@ -204,11 +206,18 @@ async fn authorize_everything( write!(out, "{}", o)?; } - write!(out, "ACTIONS:\n\n")?; - for action in authz::Action::iter() { - write!(out, " {:>2} = {:?}\n", action_abbreviation(action), action)?; + if print_actions { + write!(out, "ACTIONS:\n\n")?; + for action in authz::Action::iter() { + write!( + out, + " {:>2} = {:?}\n", + action_abbreviation(action), + action + )?; + } + write!(out, "\n")?; } - write!(out, "\n")?; Ok(()) } @@ -343,19 +352,27 @@ async fn test_conferred_roles() { .await .unwrap(); - // Assemble the list of resources that we'll use for testing. This is much - // more limited than the main policy test because we only care about the - // behavior on the Fleet itself. We also create a Silo because the - // ResourceBuilder will create for us users that we can use to test the - // behavior of each role. let exemptions = resources::exempted_authz_classes(); let mut coverage = Coverage::new(&logctx.log, exemptions); + + // Assemble the list of resources that we'll use for testing. This is much + // more limited than the main policy test because we only care about the + // behavior on the Fleet itself, as well as some top-level resources that + // exist outside of a silo. let mut builder = ResourceBuilder::new(&opctx, &datastore, &mut coverage, main_silo_id); builder.new_resource(authz::FLEET); - builder.new_resource_with_users(main_silo).await; + builder.new_resource(authz::IP_POOL_LIST); let test_resources = builder.build(); + // We also create a Silo because the ResourceBuilder will create for us + // users that we can use to test the behavior of each role. + let mut silo_builder = + ResourceBuilder::new(&opctx, &datastore, &mut coverage, main_silo_id); + silo_builder.new_resource(authz::FLEET); + silo_builder.new_resource_with_users(main_silo).await; + let silo_resources = silo_builder.build(); + // Up to this point, this looks similar to the main policy test. Here's // where things get different. // @@ -404,7 +421,7 @@ async fn test_conferred_roles() { write!(out, "policy: {:?}\n", policy).unwrap(); let policy = SiloAuthnPolicy::new(policy); - let user_contexts: Vec> = test_resources + let user_contexts: Vec> = silo_resources .users() .map(|(username, user_id)| { let user_id = *user_id; @@ -426,13 +443,15 @@ async fn test_conferred_roles() { }) .collect(); - let o = authorize_one_resource( - logctx.log.clone(), - user_contexts, - Arc::new(authz::FLEET), + authorize_everything( + &mut out, + &logctx.log, + &user_contexts, + &test_resources, + false, ) - .await; - write!(out, "{}", o).unwrap(); + .await + .unwrap(); } } diff --git a/nexus/db-queries/src/authz/roles.rs b/nexus/db-queries/src/authz/roles.rs index 0cf995d25f..11b3d482d1 100644 --- a/nexus/db-queries/src/authz/roles.rs +++ b/nexus/db-queries/src/authz/roles.rs @@ -98,7 +98,7 @@ where if let Some(with_roles) = resource.as_resource_with_roles() { let resource_type = resource.resource_type(); let resource_id = with_roles.resource_id(); - load_roles_for_resource( + load_directly_attached_roles( opctx, datastore, authn, @@ -113,7 +113,7 @@ where if let Some((resource_type, resource_id)) = with_roles.conferred_roles_by(authn)? { - load_roles_for_resource( + load_directly_attached_roles( opctx, datastore, authn, @@ -141,7 +141,7 @@ where Ok(()) } -pub async fn load_roles_for_resource( +async fn load_directly_attached_roles( opctx: &OpContext, datastore: &DataStore, authn: &authn::Context, diff --git a/nexus/db-queries/tests/output/authz-conferred-roles.out b/nexus/db-queries/tests/output/authz-conferred-roles.out index 3ecfa0befa..04734acc18 100644 --- a/nexus/db-queries/tests/output/authz-conferred-roles.out +++ b/nexus/db-queries/tests/output/authz-conferred-roles.out @@ -6,6 +6,13 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + policy: {Admin: {Admin}} resource: Fleet id "001de000-1334-4000-8000-000000000000" @@ -14,6 +21,13 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✔ ✘ ✔ ✔ ✔ ✔ + silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + policy: {Viewer: {Viewer}} resource: Fleet id "001de000-1334-4000-8000-000000000000" @@ -22,6 +36,13 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘ silo-viewer ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + policy: {Admin: {Viewer}} resource: Fleet id "001de000-1334-4000-8000-000000000000" @@ -30,6 +51,13 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + policy: {Viewer: {Admin, Viewer}} resource: Fleet id "001de000-1334-4000-8000-000000000000" @@ -38,6 +66,13 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔ silo-viewer ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✔ ✘ ✔ ✔ ✔ ✔ + silo-collaborator ✘ ✘ ✔ ✘ ✔ ✔ ✔ ✔ + silo-viewer ✘ ✘ ✔ ✘ ✔ ✔ ✔ ✔ + policy: {Admin: {Admin}, Viewer: {Viewer}} resource: Fleet id "001de000-1334-4000-8000-000000000000" @@ -46,3 +81,10 @@ resource: Fleet id "001de000-1334-4000-8000-000000000000" silo-collaborator ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘ silo-viewer ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘ +resource: authz::IpPoolList + + USER Q R LC RP M MP CC D + silo-admin ✘ ✘ ✔ ✘ ✔ ✔ ✔ ✔ + silo-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ +