[RFE] SG shared through RBAC mechanism can't be used to spawn instances

[Slawek Kaplonski]

Since some time Security groups can be shared with specific tenants using RBAC mechanism but it's not possible to share SG that way with TARGET-PROJECT and then, as a member or admin in that TARGET-PROJECT spawn vm which will use that SG:

$ openstack server create --image cirros-0.5.1-x86_64-disk --flavor m1.tiny --network TARGET-PROJECT-net1 --security-group sharedsg --wait testsg004
/usr/lib/python3/dist-packages/secretstorage/dhcrypto.py:15: CryptographyDeprecationWarning: int_from_bytes is deprecated, use int.from_bytes instead
  from cryptography.utils import int_from_bytes
/usr/lib/python3/dist-packages/secretstorage/util.py:19: CryptographyDeprecationWarning: int_from_bytes is deprecated, use int.from_bytes instead
  from cryptography.utils import int_from_bytes
Error creating server: testsg004
Error creating server

It is like that because nova in https://github.com/openstack/nova/blob/713b653fc0e09301a5674316a49a6f5ffd152b4c/nova/network/neutron.py#L814 is asking for security groups filtered by tenant_id. And Neutron returns only SGs which are owned to that tenant, without the ones shared with tenant using RBAC.

Looking at neutron api-ref https://docs.openstack.org/api-ref/network/v2/index.html?expanded=list-networks-detail,list-security-groups-detail#security-groups-security-groups it clearly says that it filters by tenant_id that OWNS the resource so it seems like correct (documented) behaviour.

Now the question is - should we relax that filter and return SG which project owns and which are shared with tenant? Or should we add additional flag to API, like "include_shared" which could be used by Nova? Or maybe do You have any other ideas about how to solve that issue?

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/807878

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/807878
Committed: https://opendev.org/openstack/neutron/commit/a383afa10f2068a28ef767296e32cec4cc96ed6b
Submitter: "Zuul (22348)"
Branch: master

commit a383afa10f2068a28ef767296e32cec4cc96ed6b
Author: Slawek Kaplonski <email address hidden>
Date: Wed Sep 8 15:41:29 2021 +0200

    [Docs] Add info about how to use shared SG with VMs

    This patch adds info about workaround how to spawn VM using Security
    Groups shared through RBAC mechanism in Neutron.
    Proper fix for that issue will require changes in the Neutron API and in
    Nova so will not be possible to backport.

    Related-bug: #1942615
    Change-Id: Iadb3fe0ca8fa9c14ec2912016bd3912e5dcee5ff

[Hang Yang (hangyang)]

Hi Slawek,

I'm interested to work on this. I also noticed Horizon faced a similar issue that when filtering by the tenant_id, the shared security groups are excluded. See: https://bugs.launchpad.net/horizon/+bug/1907843

I think Neutron's current API behavior makes sense because anyway we will need a way to know what resources actually owned by the requested tenant. As for the solution for both issues, I'd like to follow the example of how the shared networks are handled currently in Horizon and Nova:

https://opendev.org/openstack/horizon/src/branch/master/openstack_dashboard/api/neutron.py#L1055-L1101
https://github.com/openstack/nova/blob/62406b5728077afa9cd38d5c5d510bba64c43bd7/nova/network/neutron.py#L425-L455

We can implement the 'shared' filter for security groups as well and use that to make additional requests from Horizon and Nova to get the shared security groups and append them to the original lists filtered by the owner id.

I'd like to know your thoughts on this proposal, thank you!

[Slawek Kaplonski (slaweq)]

@Hang Yang - thx for working on this. I don't think that adding "shared" field will help here because we are here talking about SGs which are shared only with some specific tenant using RBAC mechanism. Such SGs aren't shared with all tenants, like is in case of the networks with "shared=True".
IMO we should add some flag in the API (it can be for both networks and security groups, and also maybe for other resources later) to tell neutron that it should include shared resources in the returned list too. So Nova or Horizon would be able to do just one call "get_security_groups(tenant_id=XXX, include_shared=True)" to get all SGs to which project has access in some way.
Does it makes sense?

[Hang Yang (hangyang)]

@Slawek, thanks for your reply. To ensure I understand your point, let's consider the following examples and assuming the shared and include_shared filters are implemented:

Tenant X is calling Neutron to list SGs that another tenant Y has access to with these search_opts:
1. search_opts = {'shared': True}
2. search_opts = {'tenant_id': Y, 'include_shared': True}
3. search_opts = {'tenant_id': Y, 'shared': True}

So #1 only returns SGs shared to X and #2 returns SGs owned and shared to Y. As for #3, Neutron will return SGs owned by Y plus SGs shared to X. Is the expected behavior of #1 and #3 the reason we need to go for #2 instead?

Other than adding the 'include_shared' flag, can we change Neutron to support #3 so that it can return SGs owned and shared to tenant Y?

[Slawek Kaplonski (slaweq)]

Hi,

For the issue described in that bug, nova needs to have possibility to get security groups to which tenant X (the one who is spawning vm) has access. So it's in fact 4th case, something like:

search_opts = {'tenant_id': X, 'include_shared': True}

That way Neutron should returns all SG which are owned by tenant X or shared with tenant X (so tenant X has access to them).

Cases 2) and 3) from Your comment are something what only admin user should be really allowed to do probably.
As for returning SGs owned and shared with Y, I believe it should be doable currently using RBAC api to list objects (SG) like that.

[Hang Yang (hangyang)]

Hi Slawek,

So from the neutron_lib/db/model_query.py, I see currently the 'shared' value is determined by the context.tenant_id: https://github.com/openstack/neutron-lib/blob/f9b428667be5b30a0acacb284b607bc5d2e2dc4e/neutron_lib/db/model_query.py#L216-L251 If the context.tenant_id == (tenant_id in the filter), then making two API calls with: search_opts = {'tenant_id': X} and search_opts = {'shared': True}, then combining the returned lists should get the same result as using search_opts = {'tenant_id': X, 'include_shared': True}, right? Please see this as an example: https://bugs.launchpad.net/horizon/+bug/1907843/comments/7

I think in Horizon's case when a user tries to boot a VM, context.tenant_id == (tenant_id in the filter) is always true. As for Nova, the only exception may be an admin tries to boot for someone else? I'm not so sure what will be the tenant_id in the context. Generally speaking, I'd prefer using an additional call with the 'shared' field to combine and get the owned and shared results. This way it requires the minimum changes and does not change Neutron's API behavior. How do you think?

[Slawek Kaplonski (slaweq)]

@Hang Yang - yes, You are right. Doing to such queries should be enough. We can then modify nova to also do second query to look into shared SGs in case when it will not be found in the list of own SGs for the tenant.

[Slawek Kaplonski (slaweq)]

According to spawning vm for someone else - I'm not sure if nova allows that currently. For Neutron You can prepare resources and pass "project_id" in the request body to do that. But I think Nova don't allows that.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/811242

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron-lib/+/812617

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron-lib/+/812617
Committed: https://opendev.org/openstack/neutron-lib/commit/916326df389f82af86f2600b292f78950e25e6f6
Submitter: "Zuul (22348)"
Branch: master

commit 916326df389f82af86f2600b292f78950e25e6f6
Author: Hang Yang <email address hidden>
Date: Tue Oct 5 17:01:27 2021 -0500

    Add API extension "security-groups-shared-filtering"

    Add API extension "security-groups-shared-filtering". This extension
    adds the "shared" field to security groups and allows users to filter
    security groups based on the "shared" field.

    Related-Bug: #1942615
    Change-Id: Idba0af5a6ee1a2a8c02db51be197988d23412f0f

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/811242
Committed: https://opendev.org/openstack/neutron/commit/4bd1c82213e063f2e6a39e36d828bd800fd81ac6
Submitter: "Zuul (22348)"
Branch: master

commit 4bd1c82213e063f2e6a39e36d828bd800fd81ac6
Author: Hang Yang <email address hidden>
Date: Mon Sep 27 17:29:33 2021 -0500

    Add shared field to SG API response and filter

    Add the shared field to security group API responses and support
    using shared as a query filter.

    A follow-up patch will remove the temporary api def once it is merged
    and released in neutron-lib.

    Related-Bug: #1942615
    Depends-On: https://review.opendev.org/c/openstack/neutron-lib/+/812617
    Change-Id: Ic04be8f0b7097c8aed19365f06089aa7af333eb9

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/842581

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/842581
Committed: https://opendev.org/openstack/neutron/commit/6012ba074fd5e72fab52dbea6a212d6d9acd17f8
Submitter: "Zuul (22348)"
Branch: master

commit 6012ba074fd5e72fab52dbea6a212d6d9acd17f8
Author: Brian Haley <email address hidden>
Date: Thu May 19 12:44:22 2022 -0400

    Start using security-groups-shared-filtering from neutron-lib

    Remove security_groups_shared_filtering_lib extension and
    use security-groups-shared-filtering from neutron-lib as
    it is available since version 2.17.0 [0].

    [0] https://review.opendev.org/c/openstack/neutron-lib/+/812617

    Change-Id: Ife9b1ae47f5b447898bce0d8b44500f91f6dfbfb
    Related-Bug: #1942615

[Brian Haley (brian-haley)]

Looks like this bug is fixed, can it be moved to Fix Released?

[Vern Hart (vern)]

Can we get this backported to Yoga?

[Antony Messerli (antonym)]

Is this patch still in flight, we've seen this issue in 2023.1 and it seems like there are some related patches pending? I noticed https://review.opendev.org/c/openstack/nova/+/811521 was still in flight.

We see this issue currently where we have a build failure on a shared common Security Group. We have to add the group post VM build for us to use the SG. If we apply it during VM creation, the build fails.