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.

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

Fix ip pool policy for instance allocation #2113

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jan 6, 2023

Fixes a regression introduced by #2056

Updates polar policy to allow instance creation from regular Silo users, and adds a regression test

@smklein smklein requested review from luqmana and plotnick January 6, 2023 16:16
Comment on lines +2980 to +2986
NexusRequest::objects_post(client, &url_instances, &instance_params)
.authn_as(AuthnMode::SiloUser(user_id))
.execute()
.await
.expect("Failed to create instance")
.parsed_body::<Instance>()
.expect("Failed to parse instance");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before updating the polar policy, this failed.

The instance creation pathway checks for authz::Action::CreateChild on the target IP pool, which works fine for AuthnMode::PrivilegedUser, but fails for a silo user.

For example:

.ip_pools_fetch_default_for(&opctx, authz::Action::CreateChild)

populate_ip_pool(&client, "default", None).await;

// Create test organization and projects.
NexusRequest::objects_post(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really part of this PR, but it would be neat if the resource_helpers used a builder pattern to make these calls a little easier to customize. They currently hard-code "privileged user", which means I need to do the request myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open a bug for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Thanks, with this I can create an instance while logged in as a silo user (alice)

populate_ip_pool(&client, "default", None).await;

// Create test organization and projects.
NexusRequest::objects_post(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open a bug for that?

Copy link
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

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

Nice catch, and thanks for the regression test!

nexus/src/authz/omicron.polar Outdated Show resolved Hide resolved
@@ -373,6 +373,10 @@ resource IpPoolList {
has_relation(fleet: Fleet, "parent_fleet", ip_pool_list: IpPoolList)
if ip_pool_list.fleet = fleet;

# Any authenticated user can create a child of a provided IP Pools.
# This is necessary to use the pools when provisioning instances.
has_permission(_actor: AuthenticatedActor, "create_child", _ip_pool: IpPool);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters much right now since we don't really support fleets yet, but for consistency with other policy rules I think there should probably be an additional restriction here:

has_permission(actor: AuthenticatedActor, "create_child", ip_pool: IpPool)
        if actor.silo.fleet = ip_pool.fleet;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hrm, this appears to actually break the parsing of the oso file:

    error_message_internal: failed to compute authorization: trace (most recent evaluation last):
      009: allow(oso::host::value::PolarValue TYPE `oso::host::value::PolarValue`, oso::host::value::PolarValue TYPE `oso::host::value::PolarValue`, oso::host::value::PolarValue TYPE `oso::host::value::PolarValue`)
      008: actor.authenticated and
        has_permission(actor.authn_actor.unwrap(), action.to_perm(), resource)
        in rule allow at line 21, column 5
      007: actor.authn_actor
        in rule allow at line 22, column 20
      006: actor.authn_actor.unwrap()
        in rule allow at line 22, column 20
      005: action.to_perm()
        in rule allow at line 22, column 48
      004: has_permission(actor.authn_actor.unwrap(), action.to_perm(), resource)
        in rule allow at line 22, column 5
      003: actor.silo.fleet = ip_pool.fleet
        in rule has_permission at line 379, column 6
      002: actor.silo.fleet = ip_pool.fleet
        in rule has_permission at line 379, column 6
      001: actor.silo.fleet
        in rule has_permission at line 379, column 6
     
    Application error: Attribute fleet not found on type Option. at line 379, column 6:
        379:   if actor.silo.fleet = ip_pool.fleet;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm unwrapping in a11fc26 , though I'm open to a safer way to do this in polar.

@smklein smklein enabled auto-merge (squash) January 6, 2023 17:52
@smklein smklein disabled auto-merge January 6, 2023 19:28
@smklein smklein enabled auto-merge (squash) January 6, 2023 19:45
@smklein smklein merged commit d9b464a into main Jan 6, 2023
@smklein smklein deleted the external-ip-access branch January 6, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants