Skip to content

Commit

Permalink
Fix "project" (silo) IP pool view perms for non-admins (#5887)
Browse files Browse the repository at this point in the history 
Closes #5883

Authz for IP pools is undercooked. Every user has `CreateChild` on them
so they can allocate IPs, but they generally don't have `Read` on them
unless they're a fleet viewer. Ideally, we'd be able to say "you have
`Read` on an IP pool if it is linked to your silo", but I don't know how
to express that relationship with polar (plus it requires a join to the
silo-pool links table to tell).

Update: after
[discussing](https://matrix.to/#/!YNYPOVxjAUeXksTcRj:oxide.computer/$XTbZiYkEFRKWHwe6XSVivBrqiWWl_z25u2_TcjwRohE?via=oxide.computer&via=unix.house&via=matrix.org)
this with @davepacheco in chat, we agreed on a direction but also agreed
it might be fairly complicated. Based on that, I think it would be most
expedient to

1. Merge this fix as-is (with a link to
#3995, which I forgot I
had made 10 months ago 😢)
2. Update #3995 with more details from the chat
  • Loading branch information
@david-crespo
david-crespo authored Jun 17, 2024
1 parent 50e353f commit 064d9ea
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 35 deletions.
16 changes: 14 additions & 2 deletions nexus/src/app/ip_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,20 @@ impl super::Nexus {
opctx: &'a OpContext,
pool: &'a NameOrId,
) -> LookupResult<(db::model::IpPool, db::model::IpPoolResource)> {
let (authz_pool, pool) =
self.ip_pool_lookup(opctx, pool)?.fetch().await?;
let (authz_pool, pool) = self
.ip_pool_lookup(opctx, pool)?
// TODO-robustness: https://github.com/oxidecomputer/omicron/issues/3995
// Checking CreateChild works because it is the permission for
// allocating IPs from a pool, which any authenticated user has.
// But what we really want to say is that any authenticated user
// has actual Read permission on any IP pool linked to their silo.
// Instead we are backing into this with the next line: never fail
// this auth check as long as you're authed, then 404 if unlinked.
// This is not a correctness issue per se because the logic as-is is
// correct. The main problem is that it is fiddly to get right and
// has to be done manually each time.
.fetch_for(authz::Action::CreateChild)
.await?;

// 404 if no link is found in the current silo
let link = self.db_datastore.ip_pool_fetch_link(opctx, pool.id()).await;
Expand Down
113 changes: 80 additions & 33 deletions nexus/tests/integration_tests/ip_pools.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

//! Integration tests for operating on IP Pools
use std::net::Ipv4Addr;

use dropshot::test_util::ClientTestContext;
use dropshot::HttpErrorResponseBody;
use dropshot::ResultsPage;
Expand All @@ -20,8 +22,10 @@ use nexus_test_utils::http_testing::RequestBuilder;
use nexus_test_utils::resource_helpers::assert_ip_pool_utilization;
use nexus_test_utils::resource_helpers::create_instance;
use nexus_test_utils::resource_helpers::create_ip_pool;
use nexus_test_utils::resource_helpers::create_local_user;
use nexus_test_utils::resource_helpers::create_project;
use nexus_test_utils::resource_helpers::create_silo;
use nexus_test_utils::resource_helpers::grant_iam;
use nexus_test_utils::resource_helpers::link_ip_pool;
use nexus_test_utils::resource_helpers::object_create;
use nexus_test_utils::resource_helpers::object_create_error;
Expand All @@ -41,6 +45,7 @@ use nexus_types::external_api::params::IpPoolUpdate;
use nexus_types::external_api::shared::IpRange;
use nexus_types::external_api::shared::Ipv4Range;
use nexus_types::external_api::shared::SiloIdentityMode;
use nexus_types::external_api::shared::SiloRole;
use nexus_types::external_api::views::IpPool;
use nexus_types::external_api::views::IpPoolRange;
use nexus_types::external_api::views::IpPoolSiloLink;
Expand Down Expand Up @@ -1144,52 +1149,94 @@ async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) {
#[nexus_test]
async fn test_ip_pool_list_in_silo(cptestctx: &ControlPlaneTestContext) {
let client = &cptestctx.external_client;
let mypool_name = "mypool";

const PROJECT_NAME: &str = "myproj";
create_project(client, PROJECT_NAME).await;
let silo_url = format!("/v1/system/silos/{}", cptestctx.silo_name);
let silo: Silo = object_get(client, &silo_url).await;

// create pool with range and link (as default pool) to default silo, which
// is the privileged user's silo
let mypool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 51),
std::net::Ipv4Addr::new(10, 0, 0, 52),
)
.unwrap(),
// manually create default pool and link to test silo, as opposed to default
// silo, which is what the helper would do
let _ = create_ip_pool(&client, "default", None).await;
let default_name = "default";
link_ip_pool(&client, default_name, &silo.identity.id, true).await;

// create other pool and link to silo
let other_pool_range = IpRange::V4(
Ipv4Range::new(Ipv4Addr::new(10, 1, 0, 1), Ipv4Addr::new(10, 1, 0, 5))
.unwrap(),
);
create_ip_pool(client, mypool_name, Some(mypool_range)).await;
link_ip_pool(client, mypool_name, &DEFAULT_SILO.id(), true).await;
let other_name = "other-pool";
create_ip_pool(&client, other_name, Some(other_pool_range)).await;
link_ip_pool(&client, other_name, &silo.identity.id, false).await;

// create another pool and don't link it to anything
let otherpool_name = "other-pool";
let otherpool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 53),
std::net::Ipv4Addr::new(10, 0, 0, 54),
)
.unwrap(),
// create third pool and don't link to silo
let unlinked_pool_range = IpRange::V4(
Ipv4Range::new(Ipv4Addr::new(10, 2, 0, 1), Ipv4Addr::new(10, 2, 0, 5))
.unwrap(),
);
create_ip_pool(client, otherpool_name, Some(otherpool_range)).await;
let unlinked_name = "unlinked-pool";
create_ip_pool(&client, unlinked_name, Some(unlinked_pool_range)).await;

// Create a silo user
let user = create_local_user(
client,
&silo,
&"user".parse().unwrap(),
params::UserPassword::LoginDisallowed,
)
.await;

let list = objects_list_page_authz::<SiloIpPool>(client, "/v1/ip-pools")
// Make silo collaborator
grant_iam(
client,
&silo_url,
SiloRole::Collaborator,
user.id,
AuthnMode::PrivilegedUser,
)
.await;

let list = NexusRequest::object_get(client, "/v1/ip-pools")
.authn_as(AuthnMode::SiloUser(user.id))
.execute_and_parse_unwrap::<ResultsPage<SiloIpPool>>()
.await
.items;

// only mypool shows up because it's linked to my silo
assert_eq!(list.len(), 1);
assert_eq!(list[0].identity.name.to_string(), mypool_name);
assert_eq!(list.len(), 2);
assert_eq!(list[0].identity.name.to_string(), default_name);
assert!(list[0].is_default);

// fetch the pool directly too
let url = format!("/v1/ip-pools/{}", mypool_name);
let pool = object_get::<SiloIpPool>(client, &url).await;
assert_eq!(pool.identity.name.as_str(), mypool_name);
assert_eq!(list[1].identity.name.to_string(), other_name);
assert!(!list[1].is_default);

// fetch the pools directly too
let url = format!("/v1/ip-pools/{}", default_name);
let pool = NexusRequest::object_get(client, &url)
.authn_as(AuthnMode::SiloUser(user.id))
.execute_and_parse_unwrap::<SiloIpPool>()
.await;
assert_eq!(pool.identity.name.as_str(), default_name);
assert!(pool.is_default);

let url = format!("/v1/ip-pools/{}", other_name);
let pool = NexusRequest::object_get(client, &url)
.authn_as(AuthnMode::SiloUser(user.id))
.execute_and_parse_unwrap::<SiloIpPool>()
.await;
assert_eq!(pool.identity.name.as_str(), other_name);
assert!(!pool.is_default);

// fetching the other pool directly 404s
let url = format!("/v1/ip-pools/{}", otherpool_name);
object_get_error(client, &url, StatusCode::NOT_FOUND).await;
let url = format!("/v1/ip-pools/{}", unlinked_name);
let _error = NexusRequest::new(
RequestBuilder::new(client, Method::GET, &url)
.expect_status(Some(StatusCode::NOT_FOUND)),
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body::<HttpErrorResponseBody>()
.unwrap();
dbg!(_error);
}

#[nexus_test]
Expand Down

0 comments on commit 064d9ea

Please sign in to comment.