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] Fix overzealous authz check on floating IP create #5660

Merged
merged 2 commits into from
Apr 30, 2024
Merged

[nexus] Fix overzealous authz check on floating IP create #5660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03b100f
draft of test that fails without fix
david-crespo Apr 29, 2024
5b60bcb
fix the problem
david-crespo Apr 29, 2024
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
6 changes: 5 additions & 1 deletion nexus/src/app/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,11 @@ impl super::Nexus {
let pool = match pool {
Some(pool) => Some(
self.ip_pool_lookup(opctx, &pool)?
.lookup_for(authz::Action::Read)
// every authenticated user has CreateChild on IP pools
// because they need to be able to allocate IPs from them.
// The check that the pool is linked to the current silo
// happens inside allocate_floating_ip
.lookup_for(authz::Action::CreateChild)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing a CreateChild instead of a Read makes sense but Datastore::allocate_floating_ip also then does the same lookup? I'd say just drop this lookup and have allocate_floating_ip take the Option<NameOrId> pool parameter from the floating IP create args directly and only do the lookup there. That's basically what allocate_probe_ephemeral_ip does. (allocate_instance_ephemeral_ip should do the same in that case as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try it. I noticed it was redundant but wanted to avoid changing much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would second this; from what I can see it's the only (non-test) use of allocate_floating_ip. My bad on the issue itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge this to make sure the fix is in the next dogfood deploy, but I will do that in a followup.

.await?
.0,
),
Expand Down
125 changes: 125 additions & 0 deletions nexus/tests/integration_tests/external_ips.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ use nexus_test_utils::resource_helpers::create_default_ip_pool;
use nexus_test_utils::resource_helpers::create_floating_ip;
use nexus_test_utils::resource_helpers::create_instance_with;
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 @@ -34,6 +36,7 @@ use nexus_test_utils::resource_helpers::object_put;
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::params;
use nexus_types::external_api::shared;
use nexus_types::external_api::shared::SiloRole;
use nexus_types::external_api::views;
use nexus_types::external_api::views::FloatingIp;
use nexus_types::identity::Resource;
Expand Down Expand Up @@ -249,6 +252,128 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) {
assert_ip_pool_utilization(client, "other-pool", 2, 5, 0, 0).await;
}

#[nexus_test]
async fn test_floating_ip_create_non_admin(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;

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

// 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;
link_ip_pool(&client, "default", &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, "other-pool", Some(other_pool_range)).await;
link_ip_pool(&client, "other-pool", &silo.identity.id, false).await;

// 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, "unlinked-pool", Some(unlinked_pool_range)).await;

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

// Make silo collaborator
grant_iam(
client,
&silo_url,
SiloRole::Collaborator,
user.id,
AuthnMode::PrivilegedUser,
)
.await;

// create project as user (i.e., in their silo)
NexusRequest::objects_post(
client,
"/v1/projects",
&params::ProjectCreate {
identity: IdentityMetadataCreateParams {
name: PROJECT_NAME.parse().unwrap(),
description: "floating ip project".to_string(),
},
},
)
.authn_as(AuthnMode::SiloUser(user.id))
.execute()
.await
.expect("Failed to create project");

let create_url = get_floating_ips_url(PROJECT_NAME);

// create a floating IP as this user, first with default pool
let body = params::FloatingIpCreate {
identity: IdentityMetadataCreateParams {
name: "root-beer".parse().unwrap(),
description: String::from("a floating ip"),
},
pool: None,
ip: None,
};
let fip: views::FloatingIp =
NexusRequest::objects_post(client, &create_url, &body)
.authn_as(AuthnMode::SiloUser(user.id))
.execute_and_parse_unwrap()
.await;
assert_eq!(fip.identity.name.to_string(), "root-beer");

// now with other pool linked to my silo
let body = params::FloatingIpCreate {
identity: IdentityMetadataCreateParams {
name: "another-soda".parse().unwrap(),
description: String::from("a floating ip"),
},
pool: Some(NameOrId::Name("other-pool".parse().unwrap())),
ip: None,
};
let fip: views::FloatingIp =
NexusRequest::objects_post(client, &create_url, &body)
.authn_as(AuthnMode::SiloUser(user.id))
.execute_and_parse_unwrap()
.await;
assert_eq!(fip.identity.name.to_string(), "another-soda");
Copy link
Contributor Author

@david-crespo david-crespo Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This request fails with 403 as expected without the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do really like this test for running through (default, linked, unlinked) pools on a more typical user. Thanks for putting it together.


// now with pool not linked to my silo (fails with 404)
let body = params::FloatingIpCreate {
identity: IdentityMetadataCreateParams {
name: "secret-third-soda".parse().unwrap(),
description: String::from("a floating ip"),
},
pool: Some(NameOrId::Name("unlinked-pool".parse().unwrap())),
ip: None,
};
let error = NexusRequest::new(
RequestBuilder::new(client, Method::POST, &create_url)
.body(Some(&body))
.expect_status(Some(StatusCode::NOT_FOUND)),
)
.authn_as(AuthnMode::SiloUser(user.id))
.execute()
.await
.unwrap()
.parsed_body::<HttpErrorResponseBody>()
.unwrap();

assert_eq!(error.message, "not found: ip-pool with name \"unlinked-pool\"");
}

#[nexus_test]
async fn test_floating_ip_create_fails_in_other_silo_pool(
cptestctx: &ControlPlaneTestContext,
Expand Down
Loading