Skip to content

Commit 5552f18

Browse files
david-crespodavid-crespo
authored
Limited collaborators should be able to use external subnets (#9920)
I had Claude vaguely looking for security issues, and it noticed that external subnet authz for limited collaborators is wrong: limited collaborators could create external subnets but could not update them or attach/detach to instances because the latter requires the `modify` permission. This is **not a security issue** — we are being more restrictive than we want to be, not less. This is a good example of what LLMs are good at vs. not: it noticed the authz logic was inconsistent, which you can tell from the code alone. But it was wrong about what the fix should be. I think Claude was biased by the use of `InProjectFull` into assuming what we want is to require collaborator. In reality, what we want is to *lower* the permission requirements here so that limited collaborators can work with external subnets. ### Limited collaborator The limited collaborator role (added in #9299) is meant to allow users to work with "regular" project resources (instances, disks, floating IPs) without being able to reconfigure network infrastructure (VPCs, subnets, routers). External subnets are more like floating IPs than VPCs — they're a thing you allocate and attach to an instance, working within a framework established by the other networking resources. I [talked](https://matrix.to/#/!YNYPOVxjAUeXksTcRj:oxide.computer/$Ja43ryODaGs5w69D23rPdVxi1b4-WQIACgAwvNV6i3k?via=oxide.computer&via=unix.house&via=matrix.org) to @rmustacc and confirmed we do want external subnets to be accessible to limited collaborators. ### Status quo External subnets use `InProjectFull` (requires `collaborator`), but the create and list paths check `CreateChild`/`ListChildren` on the Project, which allows `limited-collaborator`. So on main today: - **Create**: allowed (checks Project's `create_child` → `limited-collaborator`). Confirmed in test - **List**: allowed (checks Project's `list_children` → `viewer`). Confirmed in test - **Read**: allowed (`InProjectFull` grants read at `viewer` level). Confirmed in test - **Update**: 403 confirmed - **Attach**: 403 confirmed - **Detach**: 403 confirmed - **Delete**: 403 confirmed A limited collaborator can create external subnets but can't do anything useful with them. ### Fix Change `InProjectFull` to `InProjectLimited` on `ExternalSubnet` in the authz resource definition. No synthetic list resource needed like we have for VPCs and others — floating IPs don't have one. The new test passes with the one-line change.
1 parent adec935 commit 5552f18

3 files changed

Lines changed: 178 additions & 4 deletions

File tree

‎nexus/auth/src/authz/api_resources.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1438,7 +1438,7 @@ authz_resource! {
14381438
parent = "Project",
14391439
primary_key = { uuid_kind = ExternalSubnetKind },
14401440
roles_allowed = false,
1441-
polar_snippet = InProjectFull,
1441+
polar_snippet = InProjectLimited,
14421442
}
14431443

14441444
// MulticastGroup Authorization

‎nexus/db-queries/tests/output/authz-roles.out‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -788,11 +788,11 @@ resource: ExternalSubnet id "762e3d39-cd8a-4c59-ae6a-6efc9b2421df"
788788
fleet-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘
789789
silo1-admin ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
790790
silo1-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
791-
silo1-limited-collaborator ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
791+
silo1-limited-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
792792
silo1-viewer ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
793793
silo1-proj1-admin ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
794794
silo1-proj1-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
795-
silo1-proj1-limited-collaborator ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
795+
silo1-proj1-limited-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
796796
silo1-proj1-viewer ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
797797
unauthenticated ! ! ! ! ! ! ! !
798798
scim ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘
@@ -1077,7 +1077,7 @@ resource: ExternalSubnet id "762e3d39-cd8a-4c59-ae6a-6efc9b2421df"
10771077
fleet-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘
10781078
silo1-admin ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
10791079
silo1-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
1080-
silo1-limited-collaborator ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
1080+
silo1-limited-collaborator ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔
10811081
silo1-viewer ✘ ✔ ✔ ✔ ✘ ✘ ✘ ✘
10821082
silo1-proj1-admin ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘
10831083
silo1-proj1-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘

‎nexus/tests/integration_tests/external_subnets.rs‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use dropshot::ResultsPage;
1010
use dropshot::test_util::ClientTestContext;
1111
use http::Method;
1212
use http::StatusCode;
13+
use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO;
1314
use nexus_db_queries::db::queries::external_subnet::MAX_ATTACHED_SUBNETS_PER_INSTANCE;
1415
use nexus_test_utils::http_testing::AuthnMode;
1516
use nexus_test_utils::http_testing::NexusRequest;
@@ -19,15 +20,21 @@ use nexus_test_utils::resource_helpers::create_default_subnet_pool;
1920
use nexus_test_utils::resource_helpers::create_external_subnet_in_pool;
2021
use nexus_test_utils::resource_helpers::create_instance;
2122
use nexus_test_utils::resource_helpers::create_instance_with;
23+
use nexus_test_utils::resource_helpers::create_local_user;
2224
use nexus_test_utils::resource_helpers::create_project;
2325
use nexus_test_utils::resource_helpers::create_subnet_pool;
2426
use nexus_test_utils::resource_helpers::create_subnet_pool_member;
27+
use nexus_test_utils::resource_helpers::grant_iam;
2528
use nexus_test_utils::resource_helpers::object_create_error;
2629
use nexus_test_utils::resource_helpers::objects_list_page_authz;
30+
use nexus_test_utils::resource_helpers::test_params;
2731
use nexus_test_utils_macros::nexus_test;
2832
use nexus_types::external_api::external_subnet as external_subnet_types;
2933
use nexus_types::external_api::external_subnet::ExternalSubnet;
3034
use nexus_types::external_api::ip_pool;
35+
use nexus_types::external_api::policy::SiloRole;
36+
use nexus_types::external_api::silo;
37+
use nexus_types::identity::Resource as _;
3138
use omicron_common::address::IpVersion;
3239
use omicron_common::api::external::IdentityMetadataCreateParams;
3340
use omicron_common::api::external::IdentityMetadataUpdateParams;
@@ -901,6 +908,173 @@ async fn external_subnet_create_nonexistent_pool(
901908
assert_eq!(error.error_code.as_deref(), Some("ObjectNotFound"));
902909
}
903910

911+
/// Limited collaborators should be able to do the full external subnet
912+
/// lifecycle: create, list, read, update, attach, detach, delete.
913+
#[nexus_test]
914+
async fn test_limited_collaborator_external_subnet_lifecycle(
915+
cptestctx: &ControlPlaneTestContext,
916+
) {
917+
let client = &cptestctx.external_client;
918+
919+
// Setup infrastructure as privileged user.
920+
let _pool =
921+
create_default_subnet_pool(client, SUBNET_POOL_NAME, IpVersion::V4)
922+
.await;
923+
create_subnet_pool_member(
924+
client,
925+
SUBNET_POOL_NAME,
926+
"8.8.8.0/24".parse().unwrap(),
927+
)
928+
.await;
929+
let (_v4_pool, _v6_pool) = create_default_ip_pools(client).await;
930+
create_project(client, PROJECT_NAME).await;
931+
932+
// Create a limited collaborator user.
933+
let silo: silo::Silo = NexusRequest::object_get(
934+
client,
935+
&format!("/v1/system/silos/{}", DEFAULT_SILO.name()),
936+
)
937+
.authn_as(AuthnMode::PrivilegedUser)
938+
.execute_and_parse_unwrap()
939+
.await;
940+
let limited_user = create_local_user(
941+
client,
942+
&silo,
943+
&"limited-user".parse().unwrap(),
944+
test_params::UserPassword::LoginDisallowed,
945+
)
946+
.await;
947+
let silo_url = format!("/v1/system/silos/{}", DEFAULT_SILO.name());
948+
grant_iam(
949+
client,
950+
&silo_url,
951+
SiloRole::LimitedCollaborator,
952+
limited_user.id,
953+
AuthnMode::PrivilegedUser,
954+
)
955+
.await;
956+
let authn = AuthnMode::SiloUser(limited_user.id);
957+
958+
// Create an external subnet.
959+
let create_params = external_subnet_types::ExternalSubnetCreate {
960+
identity: IdentityMetadataCreateParams {
961+
name: EXTERNAL_SUBNET_NAME.parse().unwrap(),
962+
description: String::from("A test external subnet"),
963+
},
964+
allocator: external_subnet_types::ExternalSubnetAllocator::Auto {
965+
prefix_len: 28,
966+
pool_selector: ip_pool::PoolSelector::Explicit {
967+
pool: NameOrId::Name(SUBNET_POOL_NAME.parse().unwrap()),
968+
},
969+
},
970+
};
971+
let subnet = NexusRequest::objects_post(
972+
client,
973+
&external_subnets_url(PROJECT_NAME),
974+
&create_params,
975+
)
976+
.authn_as(authn.clone())
977+
.execute_and_parse_unwrap::<ExternalSubnet>()
978+
.await;
979+
assert_eq!(subnet.identity.name.as_str(), EXTERNAL_SUBNET_NAME);
980+
981+
// List external subnets.
982+
let list: ResultsPage<ExternalSubnet> =
983+
NexusRequest::object_get(client, &external_subnets_url(PROJECT_NAME))
984+
.authn_as(authn.clone())
985+
.execute_and_parse_unwrap()
986+
.await;
987+
assert_eq!(list.items.len(), 1);
988+
assert_eq!(list.items[0].identity.id, subnet.identity.id);
989+
990+
// Read the external subnet.
991+
let read = NexusRequest::object_get(
992+
client,
993+
&external_subnet_url(EXTERNAL_SUBNET_NAME, PROJECT_NAME),
994+
)
995+
.authn_as(authn.clone())
996+
.execute_and_parse_unwrap::<ExternalSubnet>()
997+
.await;
998+
assert_eq!(read.identity.id, subnet.identity.id);
999+
1000+
// Update the external subnet.
1001+
let updates = external_subnet_types::ExternalSubnetUpdate {
1002+
identity: IdentityMetadataUpdateParams {
1003+
name: None,
1004+
description: Some(String::from("updated description")),
1005+
},
1006+
};
1007+
let updated = NexusRequest::object_put(
1008+
client,
1009+
&external_subnet_url(EXTERNAL_SUBNET_NAME, PROJECT_NAME),
1010+
Some(&updates),
1011+
)
1012+
.authn_as(authn.clone())
1013+
.execute_and_parse_unwrap::<ExternalSubnet>()
1014+
.await;
1015+
assert_eq!(updated.identity.description.as_str(), "updated description");
1016+
1017+
// Create a stopped instance to attach the subnet to.
1018+
create_instance_with(
1019+
client,
1020+
PROJECT_NAME,
1021+
INSTANCE_NAME,
1022+
&Default::default(),
1023+
vec![],
1024+
vec![],
1025+
/* start = */ false,
1026+
None,
1027+
None,
1028+
vec![],
1029+
)
1030+
.await;
1031+
1032+
// Attach the subnet.
1033+
let attach_url =
1034+
external_subnet_attach_url(EXTERNAL_SUBNET_NAME, PROJECT_NAME);
1035+
let attached = NexusRequest::new(
1036+
RequestBuilder::new(client, Method::POST, &attach_url)
1037+
.body(Some(&external_subnet_types::ExternalSubnetAttach {
1038+
instance: INSTANCE_NAME.parse::<Name>().unwrap().into(),
1039+
}))
1040+
.expect_status(Some(StatusCode::ACCEPTED)),
1041+
)
1042+
.authn_as(authn.clone())
1043+
.execute_and_parse_unwrap::<ExternalSubnet>()
1044+
.await;
1045+
assert!(attached.instance_id.is_some());
1046+
1047+
// Detach the subnet.
1048+
let detach_url =
1049+
external_subnet_detach_url(EXTERNAL_SUBNET_NAME, PROJECT_NAME);
1050+
let detached = NexusRequest::new(
1051+
RequestBuilder::new(client, Method::POST, &detach_url)
1052+
.expect_status(Some(StatusCode::ACCEPTED)),
1053+
)
1054+
.authn_as(authn.clone())
1055+
.execute_and_parse_unwrap::<ExternalSubnet>()
1056+
.await;
1057+
assert!(detached.instance_id.is_none());
1058+
1059+
// Delete the subnet.
1060+
NexusRequest::object_delete(
1061+
client,
1062+
&external_subnet_url(EXTERNAL_SUBNET_NAME, PROJECT_NAME),
1063+
)
1064+
.authn_as(authn.clone())
1065+
.execute()
1066+
.await
1067+
.expect("limited collaborator should be able to delete external subnet");
1068+
1069+
// Verify it's gone.
1070+
let list: ResultsPage<ExternalSubnet> =
1071+
NexusRequest::object_get(client, &external_subnets_url(PROJECT_NAME))
1072+
.authn_as(authn.clone())
1073+
.execute_and_parse_unwrap()
1074+
.await;
1075+
assert!(list.items.is_empty());
1076+
}
1077+
9041078
// Verify that creating a subnet from a pool that exists but is not linked to
9051079
// the current silo fails. This covers the guard CTE
9061080
// (`ensure_silo_is_linked_to_pool`) that checks linking — if the CTE were

0 commit comments

Comments
 (0)