Policy check using new attribute value to validate access to the same attribute

[Cliff Parsons]

During policy testing using Patrole with the new multi-policy support, it became apparent that the Neutron policy enforcement code is using the new value of an attribute (on a PUT request) to validate access to that same attribute. Here are two scenarios that expose this problem, using devstack:

Scenario 1:
----------
 1. Set up Patrole as a Tempest plugin and set the [patrole] parameters in
    tempest.conf.
 2. Set the rbac_test_role to "Member".
 3. Change /etc/neutron/policy.json so that "update_network" and
    "update_network:shared" are defined as "rule:admin_only" (the "Member"
    role is not allowed to update the network)
 4. Change /etc/neutron/policy.json so that "get_network" is defined as
    "rule:shared", so that a "Member" is allowed to view only a shared
    network, but not a non-shared network.
 5. Run the patrole_tempest_plugin.tests.api.network.test_networks_rbac.
    NetworksRbacTest.test_update_network_shared test case. This test has
    recently been modified to test the multi-policy nature of Neutron's
    policy enforcement algorithm.
      - The test case expects a 404 NotFound exception to be raised if the
        role being tested ("Member" in this case) is not allowed to perform
        "get_network". A 403 Forbidden exception is expected if the role is
        not allowed to perform either of the "update_network" or
        "update_network:shared" actions.
 Result:
     With the current Neutron code in master, this test fails with the
     given policy because a 403 Forbidden is thrown instead of the expected
     404 NotFound exception.

     The reason it does not throw the expected 404 is because the "Member"
     role IS allowed to perform "get_network". That is because the Neutron
     policy enforcement code uses the NEW value of the network's "shared"
     attribute in order to check if the rule "get_network" is allowed
     instead of the original value. The original value is shared=False as
     set by the test case, and the test case is modifying it to True. So
     "get_network" should not have been allowed for the Member role here.

     See code snippet: https://github.com/openstack/neutron/blob/
     9cb68ce7777f3016f6affe0abbc7ebe3b859f7c3/neutron/pecan_wsgi/hooks/
     policy_enforcement.py#L123

Scenario 2: (setup is similar to scenario 1)
----------
 The same problem occurs by running the following test case against the
 "Member" role:
     patrole_tempest_plugin.tests.api.network.test_networks_rbac.
         NetworksRbacTest.test_update_network_router_external

 using this set of policy rules:
     - "update_network" and "update_network:router:external" defined as
       "rule:admin_only"
     - "get_network" is defined as "rule:external", so that a "Member" is
       allowed to view only external networks.
 Result:
     Although 404 is expected to be thrown by Neutron due to the
     "get_network" rule, Neutron actually throws a 403 exception, so the
     test case fails. That is because the Neutron policy enforcement code
     uses the NEW value of the network's "external" attribute in order to
     check if the rule "get_network" is allowed instead of the original
     value. The original value is external=False as set by the test case,
     and the test case is modifying it to True. So "get_network" should
     not have been allowed for the Member role here.

[OpenStack Infra (hudson-openstack)]

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

[Hongbin Lu (hongbin.lu)]

@Cliff,

Could you provide a reference for this statement:

  "The test case expects a 404 NotFound exception to be raised if the role being tested ("Member" in this case) is not allowed to perform "get_network""

I believe this is the convention of API design. Just want to confirm it with an official document.

[Cliff Parsons (cliffhparsons)]

Hello Hongbin,

Here is a reference that I found: https://docs.openstack.org/neutron/pike/contributor/internals/policy.html

Check out the "Request authorization" section, 3rd bullet.

[Hongbin Lu (hongbin.lu)]

I tried to reproduce scenario #1 but I couldn't: http://paste.openstack.org/show/725863/ . Could you provide the reproducing steps (without using Tempest plugin)?

[Cliff Parsons (cliffhparsons)]

Sorry for the delay, Hongbin, I was on a vacation for a while.

If you are using the default policy.json file you won't see the problem. In my company's policy file, we specify that a certain user role is not allowed to get or update a network. It is that role that illuminates the problem. Here is a sample policy file excerpt that will help you understand and reproduce the problem:

    ...
    "admin_or_owner": "rule:context_is_admin or (rule:owner and not role:anotherrole)",
    "context_is_advsvc": "role:advsvc",
    ...
    "get_network": "rule:admin_or_owner or rule:shared or rule:external or rule:context_is_advsvc",
    ...
    "update_network": "rule:admin_or_owner",
    "update_network:shared": "rule:admin_only",
    ...

Notice above that the role "anotherrole" is not allowed to execute "admin_or_owner" types of actions. This includes update network and also getting/showing a network, except the case where the network is shared or is an external network. For this test, ensure that the network is neither shared nor external to begin with.

Now grab a token as a user with the role "anotherrole", and then try to do the update network curl command:
  curl -g -i -X PUT "http://localhost:9696/v2.0/networks/$NET_ID" \
    -H "Accept: application/json" -H "Content-Type: application/json" -H "X-Auth-Token: $TOKEN" \
 -d '{"network": {"shared": true}}'

You will see that you will get a 403 Forbidden response because the user with "anotherrole" was allowed to perform "get_network" when in fact, it should not have been because at the time of the update, the network was not shared and not external. Expectation was 404.

You should be able to understand and reproduce the bug now. Please let me know if you have further questions.

[Cliff Parsons (cliffhparsons)]

As Slawek pointed out in the code review, the proposed fix does cause another problem with the evaluation of the "update_rbac_policy:target_tenant" rule. The proposed fix changes Neutron to look at current attribute values instead of attribute values from the API request, but for the "update_rbac_policy:target_tenant" rule, the desire is to look at the new value of "target_tenant" attribute to determine if its wildcarded or not.

So which is it? Should Neutron be interpreting a field/attribute value as
1) current value in the database or
2) as new value found in the API request?

If the answer is #2, then the rules "rule:shared" and "rule:external" are not being used correctly in this rule:

     "get_network": "rule:admin_or_owner or rule:shared or rule:external or rule:context_is_advsvc",

When I read this rule, I expect that network information can be retrieved only if:
   a) the user with that role is the owner of the network
   b) the user with that role is admin role
   c) the user with that role is advsvc role
   d) the network is *currently* shared
   e) the network is *currently* external

But let's say a non-owner, non-admin, non-advsvc user is trying to change the non-shared, non-external network to be shared. The user will not have permission to update the shared attribute, so this "get_network" rule is going to be invoked. If neutron looks at the new value of shared from the API request, then "get_network" will succeed, and the wrong error code will be returned (403 instead of 404).

[Slawek Kaplonski (slaweq)]

This bug has had a related patch abandoned and has been automatically un-assigned due to inactivity. Please re-assign yourself if you are continuing work or adjust the state as appropriate if it is no longer valid.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/582659
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.