Skip to content

Commit

Permalink
nexus: allow IpPoolList access via silo conferred roles as well (#3999)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@luqmana
luqmana authored Sep 1, 2023
1 parent 4289ad2 commit de1fa54
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 60 deletions.
50 changes: 10 additions & 40 deletions nexus/db-queries/src/authz/api_resources.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
53 changes: 36 additions & 17 deletions nexus/db-queries/src/authz/policy_test/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ async fn test_iam_roles_behavior() {
&logctx.log,
&user_contexts,
&test_resources,
true,
)
.await
.unwrap();
Expand All @@ -185,6 +186,7 @@ async fn authorize_everything<W: Write>(
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
Expand All @@ -204,11 +206,18 @@ async fn authorize_everything<W: Write>(
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(())
}
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -404,7 +421,7 @@ async fn test_conferred_roles() {
write!(out, "policy: {:?}\n", policy).unwrap();
let policy = SiloAuthnPolicy::new(policy);

let user_contexts: Vec<Arc<(String, OpContext)>> = test_resources
let user_contexts: Vec<Arc<(String, OpContext)>> = silo_resources
.users()
.map(|(username, user_id)| {
let user_id = *user_id;
Expand All @@ -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();
}
}

Expand Down
6 changes: 3 additions & 3 deletions nexus/db-queries/src/authz/roles.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
42 changes: 42 additions & 0 deletions nexus/db-queries/tests/output/authz-conferred-roles.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"

Expand All @@ -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"

Expand All @@ -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"

Expand All @@ -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"

Expand All @@ -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 ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘

0 comments on commit de1fa54

Please sign in to comment.