Neutron RBAC not working for multiple extensions

[Felipe Monteiro]

* Description *

Using QA automation as well as manual CLI validation, it appears to me as though many Neutron extensions aren't enforcing RBAC at all. This is because the extensions lack the "enforce_policy": True key/value pair in the extension resource definition code. Example: https://review.openstack.org/#/c/584217/2/neutron/extensions/subnet_service_types.py

This appears to be affecting many Neutron extensions.

* Pre-conditions *

1) Enable neutron-trunk plugin in local.conf by adding: enable_service neutron-trunk
2) Set create_trunk to "rule:admin_only" in /etc/neutron/policy.json, i.e.: "create_trunk": "rule:admin_only" or even "create_trunk": "!" which should deny absolutely everyone.
3) source ~/devstack/openrc demo demo or even source ~/devstack/openrc admin admin after setting "create_trunk" with the "!" rule.

* Reproduction Steps *

1) Run the following CLI commands:

- openstack network create test-network
- openstack port create --enable --network test-network test-port
- openstack network trunk create --parent-port test-port --enable --project demo test-trunk

* Expected Output *

Expected result: trunk creation fails with a 403 Unauthorized.

* Actual Output *

Observed result: trunk creation succeeds.

* Affected Plugins *
As far as I can tell:

- subnet service type
- subnet segment_id
- trunks
- trunk subports

Possibly many more.

* Outstanding Patches *

Outstanding patches that begin fixing these issues:

- https://review.openstack.org/#/c/584217/ (subnet service type RBAC fix)
- https://review.openstack.org/#/c/584601/ (subnet segment_id RBAC fix)

Validation patches that identify some of these issues:
- https://review.openstack.org/#/c/584424/
- https://review.openstack.org/#/c/582388/

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Jeremy Stanley (fungi)]

The public changes in review at least hint pretty heavily at this reported vulnerability, so there's probably very little point in trying to keep it under wraps with an embargo.

[Felipe Monteiro (fm577c)]

@Jeremy I apologize. I should've been more prudent about it. I just stumbled into this and am now trying to course correct.

[Jeremy Stanley (fungi)]

No need to apologize. I wasn't trying to point out any wrongdoing, merely highlighting that the additional effort needed for embargoed discussion and development probably isn't warranted given the public nature of the linked reviews (but I'd like to see someone from neutron-coresec agree with that assertion first, so I haven't switched the report to public security yet).

[Akihiro Motoki (amotoki)]

According to my test results, this topic is split into two:
(a) when a user does not have the admin role, I cannot reproduce the issue
(b) when a user has the admin role, the policy check (at least when creating a resource) seems skipped.

(a) is the expected result, but (b) needs further investigation.

The following is the detail.

Regarding (a), I cannot reproduce the issue reported here.

After running DevStack with 'enable_service neutron-trunk', I did the following steps:
1) Edit /etc/neutron/policy.json to have "create_trunk": "rule:admin_only"
2) Restart neutron-api service to ensure the updated policy.json is reloaded. (service devstack@neutron-api restart)
3) Set OS_CLOUD envvar to devstack (which is equivalent to "openrc demo demo")
4) Run "openstack network trunk create --parent-port p1 trunk1"
5) I got the message:
---
rule:create_trunk is disallowed by policy
Neutron server returns request_ids: ['req-df278e9a-f449-4a4e-a16f-e03b063c283c']
---

Regarding (b), when a user has the admin role, it seems a network trunk can be created regardless of policy configuration
1) Edit /etc/neutron/policy.json to have "create_trunk": "!" (or "create_trunk": "role:nothing)
2) Restart neutron-api service to ensure the updated policy.json is reloaded. (service devstack@neutron-api restart)
3) Set OS_CLOUD envvar to devstack-admin (which is equivalent to "openrc admin admin")
4) Run "openstack network trunk create --parent-port p1 trunk1"
5) The trunk was created successfully. <--- This is different from what we expect

[Felipe Monteiro (fm577c)]

@Akihiro, will comment on (b). Will look into (a) more closely when time permits.

I believe this is due to the following code: https://github.com/openstack/neutron/blob/1779a86712d2eaf42e1089369632df90942b1763/neutron/policy.py#L410

Meaning that, with role: admin, context.is_admin == True, causing the function to return True, prior to oslo.policy authorization check.

[Miguel Lavalle (minsel)]

I agree that patches are already public, so I don't the cost of the embargo is warranted any more

[Jeremy Stanley (fungi)]

Thanks, Miguel. In that case I've switched the report to Public Security and ended the embargo so discussion can continue in the open.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/598334

[OpenStack Infra (hudson-openstack)]

Change abandoned by Felipe Monteiro (<email address hidden>) on branch: master
Review: https://review.openstack.org/584217

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/598334
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=d14b379d1c16d451f35d026ab83702ca631f2e4f
Submitter: Zuul
Branch: master

commit d14b379d1c16d451f35d026ab83702ca631f2e4f
Author: Mykola Yakovliev <email address hidden>
Date: Thu Aug 30 14:51:57 2018 -0500

    Let Neutron enforce rule on create_subnet with segment_id [neutron-lib part]

    Neutron ignores rule in policy file [0], that allows non-admin users
    to create subnets with segment_id.

    [0] https://github.com/openstack/neutron/blob/master/etc/policy.json#L19

    Change-Id: I313aadc53f728663fd774957c1bd92247d1513ca
    Partial-Bug: 1784259

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/584601
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[Slawek Kaplonski (slaweq)]

Speaking about the "issue" when call is done by admin user, it's still the same code https://github.com/openstack/neutron/blob/4f10c3bd3f57836aeb1b6ba4305301d63c046bd0/neutron/policy.py#L470 so I assume that an admin user still can do everything. Should we change that?

[Jeremy Stanley (fungi)]

It looks like the neutron-lib half of the fix merged and was subsequently released, but the neutron half was eventually abandoned because nobody reviewed it after that. Is it still an appropriate solution, and if so would someone please restore and review it so we might be able to move forward on this report?

[Slawek Kaplonski (slaweq)]

Now neutron have support for new personas and system scopes. And with enforcing those new personas, there is no simple "is_admin==True" check which allows to do everything. So with that do we still have that issue? I guess not but wanted to be sure. Maybe I'm missing something.

[Jeremy Stanley (fungi)]

After discussing, the Vulnerability Management Team members have concluded that the in-progress but incomplete RBAC implementation in various projects does not rise to the level of requiring a published security advisory, particularly as this work is likely to take place primarily in development branches and not be backported to supported stable branches. Some clearer documentation on behalf of the implementing projects is likely warranted in order to warn users of the caveats and potential pitfalls of relying on RBAC in its current state, but that's separate from whether or not we publish advisories about any fixes which may merge to complete the implementation.