Neutron is checking stricter policies than an operator would expect

[Gabriel Assis Bezerra]

I'm trying to set a custom policy.json for Neutron based on new roles I have defined.

In this task, I changed the "default" policy from "rule: admin_or_owner" to "rule:admin_only". After that, a bunch of operations stopped working, including, for instance, a regular user deleting a network or a router of his/her own project. Even with the policy for "delete_network" unchanged -- rule:admin_or_owner --, only the admin could delete a network.

I put a print statement in neutron.openstack.common.policy.check method to investigate what was happening. On the following lines you can compare the debug message in the logs with the actual content of the "rule" parameter passed to "check".

- - -

DEBUG neutron.policy [...] Failed policy check for 'delete_network'

(((rule:delete_network and rule:delete_network:provider:physical_network) and rule:delete_network:provider:network_type) and rule:delete_network:provider:segmentation_id)

- - -

DEBUG neutron.policy [...] Failed policy check for 'delete_port'

(((((((rule:delete_port and rule:delete_port:binding:host_id) and rule:delete_port:allowed_address_pairs) and rule:delete_port:binding:vif_details) and rule:delete_port:binding:vif_ty
pe) and rule:delete_port:mac_address) and rule:delete_port:binding:profile) and rule:delete_port:fixed_ips)

- - -

DEBUG neutron.policy [...] Failed policy check for 'delete_router'

(rule:delete_router and rule:delete_router:distributed)

- - -

DEBUG neutron.policy [...] Failed policy check for 'update_subnet'

(rule:update_subnet and rule:update_subnet:shared)

- in this case, there is no "update_subnet:shared" rule, but there is a "subnets:shared:write" rule (which doesn't seem to be used).

- - -

These are the tests I've implemented that got broken after changing the default rule. The update tests simply try to rename the resource.

test_delete_network_of_own_project
test_delete_port_own_project
test_add_router_interface_to_router_of_own_project*
test_delete_router_of_own_project
test_remove_router_interface_from_router_of_own_project*
test_update_router_of_own_project
test_update_shared_subnet_of_own_project

* these tests got broken because of this bug: https://bugs.launchpad.net/neutron/+bug/1356678.

[Eugene Nikanorov (enikanorov)]

Can you post the changes you've done to policy.json?

[Gabriel Assis Bezerra (gabriel-bezerra)]

Sure.

$ diff -u /etc/neutron/policy.json{.bak,}
--- /etc/neutron/policy.json.bak 2014-08-08 19:10:16.761472999 +0000
+++ /etc/neutron/policy.json 2014-08-18 13:59:54.825472999 +0000
@@ -1,5 +1,9 @@
 {
- "context_is_admin": "role:admin",
+ "is_cloud_admin": "role:cloud_admin",
+ "is_project_admin": "role:project_admin and tenant_id:%(tenant_id)s",
+ "is_project_member": "role:project_member and tenant_id:%(tenant_id)s",
+
+ "context_is_admin": "rule:is_cloud_admin or role:admin",
     "admin_or_owner": "rule:context_is_admin or tenant_id:%(tenant_id)s",
     "admin_or_network_owner": "rule:context_is_admin or tenant_id:%(network:tenant_id)s",
     "admin_only": "rule:context_is_admin",
@@ -7,7 +11,7 @@
     "shared": "field:networks:shared=True",
     "shared_firewalls": "field:firewalls:shared=True",
     "external": "field:networks:router:external=True",
- "default": "rule:admin_or_owner",
+ "default": "rule:admin_only",

     "subnets:private:read": "rule:admin_or_owner",
     "subnets:private:write": "rule:admin_or_owner",

[Elena Ezhova (eezhova)]

The reason for such behavior is quite simple. When the policy engine starts to build rules to enforce it checks for each attribute of a to-be deleted resourse whether it is explicitly set. It's done by checking if an attribute is present and has a non-default value (https://github.com/openstack/neutron/blob/master/neutron/policy.py#L121). In your case with a network, such attributes are: provider:physical_network, provider:network_type and provider:segmentation_id.

Then, for each explicitly set attribute a rule is built. Rules that correspond to each attribute are listed in the policy.json file, but in case some of them are absent, they are replaced by a default rule. That is why, by setting a default rule to be "rule:admin_only", you made it impossible for a non-admin user to delete various resources. For example, if you add

delete_network:provider:network_type": "rule:admin_or_owner"
delete_network:provider:physical_network": "rule:admin_or_owner"
delete_network:provider:segmentation_id": "rule:admin_or_owner"

to your policy.json, then network delete will be successfull.

I will investigate, whether the current behaviour may cause problems, but for now I feel that forbidding a regular user to delete a resource which has admin-only attributes is quite logical.

[Gabriel Assis Bezerra (gabriel-bezerra)]

The problem is that, even though I have created the resources as admin, I have not set those attributes. I'm not even working with them. The only point close to an extension I have worked with in these tests was "network"."router:external".

I mean it for the other resources too, not only for networks.

[Elena Ezhova (eezhova)]

Some attributes, and I mean it not only for network too, may be set automatically, depending on the plugin you use and neutron settings.

Just an example: in the default devstack installation ml2+ovs plugin is used and in its conf file there is a setting tenant_network_types = vxlan, which is why creating a network leads to provider:network_type being set as vxlan. So you don't always have to set an attribute explicitly, sometimes it's done automatically.

[Gabriel Assis Bezerra (gabriel-bezerra)]

If the attribute is automatically set, it should not avoid a resource from being deleted then. There is no sense in forbidding a user from deleting a network of his/her own just because a plugin set an admin_only-settable attribute on it, without the intervention of the admin himself.

On the other hand, there is sense in making the default rule admin_only, trying to achieve higher security in policies.

There should be a better way to deal with this situation. Either working on fixing the way policies are applied on operations, or at least making it clear in the docs and policy.json which policies are checked.

The policies which are actually checked are not even listed on the sample policy.json. How would an operator have any clue of what is going on without having to touch the source code as I did? Not even the logs are helpful.

[Eugene Nikanorov (enikanorov)]

I think if you're making admin_only a default rule, you need to explicitly set lighter permissions for all operations that require it

[Gabriel Assis Bezerra (gabriel-bezerra)]

That is obvious. The problems in this case are:

! - The policies I would have to change weren't listed in the sample. How could I know they exist?
2 - The logs showed something different from what was actually being checked.
3 - It is checking attributes I didn't set for the resource.

Number 1 and 2 are not acceptable.

Number 3 is bad, but could be acceptable if 1 and 2 did not happen; so the operator could see what is being checked and do something about it.

[Salvatore Orlando (salvatore-orlando)]

The problem with #1 is that policy.json is not a sample. So there is no unacceptable problem #1.
The logic of that file is that the default rule applies for any policy which is not explicitly specified.

I think we should provide a sample with all possible policies - this should be doable. The admin guide should already contain a guide for editing policy.json, I'm going to double check that.

Regarding mismatches between rule checks and checks shown in logs, if you refer to situations like the following

DEBUG neutron.policy [...] Failed policy check for 'delete_network'

(((rule:delete_network and rule:delete_network:provider:physical_network) and rule:delete_network:provider:network_type) and rule:delete_network:provider:segmentation_id)

There is nothing unacceptable for me. This depends indeed on the fact that the policy engine has resource-level and attribute-level rules. For deleting a network one has to satisfy both resource-level (the first) and attribute level (the others) rules.

For point #3 this is by-product of the API controller behaviour. Elena is already working on improving that.

[Gabriel Assis Bezerra (gabriel-bezerra)]

#1 - I agree when you say:

"I think we should provide a sample with all possible policies - this should be doable. The admin guide should already contain a guide for editing policy.json"

#2 - There must be a way for an operator to know what is going on. The only way I could find something about it was editing the source code with a print statement and re-exercising the the API.

How could an operator know that delete_xxx:extension_yyy:attribute_zzz policy is also being checked and that it is the reason for Neutron not to be doing what s/he intends to do?

I still think that information should be in the logs or something equivalent, as it will never be outdated -- as would happen if it is in the documents or even in the sample policy.json.

[Elena Ezhova (eezhova)]

#1 +1 on that

I checked out the current OpenStack Cloud Administrator guide [1], it definitely has some clues on how to edit policy.json, but it does not provide a sample policy.json or even a list of all possible policies. I believe that it needs some improvement.

#2 Logging rules that are being checked also seems rational to me, because, as Gabriel pointed out, it is the easiest way for an administrator to find out what rules are really checked without looking into the code.

[1] http://docs.openstack.org/admin-guide-cloud/content/section_networking_auth.html

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/116925
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=0cc7444f7577e53f14068a2cb35431713d6d21a0
Submitter: Jenkins
Branch: master

commit 0cc7444f7577e53f14068a2cb35431713d6d21a0
Author: Elena Ezhova <email address hidden>
Date: Tue Aug 26 19:22:20 2014 +0400

    Add logging for enforced policy rules

    There are a lot of policy rules which should not necessarily
    be explicitly specified in policy.json to be checked while enforcement.
    There should be a way for an operator to know which policy rules are
    actually being enforced for each action.

    Added a unit test.

    Change-Id: I261d3e230eced9ea514b35cc3f5f8be04f84c751
    Closes-Bug: #1356679

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: feature/lbaasv2
Review: https://review.openstack.org/130864

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/130864
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=c089154a94e5872efc95eab33d3d0c9de8619fe4
Submitter: Jenkins
Branch: feature/lbaasv2

commit 62588957fbeccfb4f80eaa72bef2b86b6f08dcf8
Author: Kevin Benton <email address hidden>
Date: Wed Oct 22 13:04:03 2014 -0700

    Big Switch: Switch to TLSv1 in server manager

    Switch to TLSv1 for the connections to the backend
    controllers. The default SSLv3 is no longer considered
    secure.

    TLSv1 was chosen over .1 or .2 because the .1 and .2 weren't
    added until python 2.7.9 so TLSv1 is the only compatible option
    for py26.

    Closes-Bug: #1384487
    Change-Id: I68bd72fc4d90a102003d9ce48c47a4a6a3dd6e03

commit 17204e8f02fdad046dabdb8b31397289d72c877b
Author: OpenStack Proposal Bot <email address hidden>
Date: Wed Oct 22 06:20:15 2014 +0000

    Imported Translations from Transifex

    For more information about this automatic import see:
    https://wiki.openstack.org/wiki/Translations/Infrastructure

    Change-Id: I58db0476c810aa901463b07c42182eef0adb5114

commit d712663b99520e6d26269b0ca193527603178742
Author: Carl Baldwin <email address hidden>
Date: Mon Oct 20 21:48:42 2014 +0000

    Move disabling of metadata and ipv6_ra to _destroy_router_namespace

    I noticed that disable_ipv6_ra is called from the wrong place and that
    in some cases it was called with a bogus router_id because the code
    made an incorrect assumption about the context. In other case, it was
    never called because _destroy_router_namespace was being called
    directly. This patch moves the disabling of metadata and ipv6_ra in
    to _destroy_router_namespace to ensure they get called correctly and
    avoid duplication.

    Change-Id: Ia76a5ff4200df072b60481f2ee49286b78ece6c4
    Closes-Bug: #1383495

commit f82a5117f6f484a649eadff4b0e6be9a5a4d18bb
Author: OpenStack Proposal Bot <email address hidden>
Date: Tue Oct 21 12:11:19 2014 +0000

    Updated from global requirements

    Change-Id: Idcbd730f5c781d21ea75e7bfb15959c8f517980f

commit be6bd82d43fbcb8d1512d8eb5b7a106332364c31
Author: Angus Lees <email address hidden>
Date: Mon Aug 25 12:14:29 2014 +1000

    Remove duplicate import of constants module

    .. and enable corresponding pylint check now the only offending instance
    is fixed.

    Change-Id: I35a12ace46c872446b8c87d0aacce45e94d71bae

commit 9902400039018d77aa3034147cfb24ca4b2353f6
Author: rajeev <email address hidden>
Date: Mon Oct 13 16:25:36 2014 -0400

    Fix race condition on processing DVR floating IPs

    Fip namespace and agent gateway port can be shared by multiple dvr routers.
    This change uses a set as the control variable for these shared resources
    and ensures that Test and Set operation on the control variable are
    performed atomically so that race conditions do not occur among
    multiple threads processing floating IPs.
    Limitation: The scope of this change is limited to addressing the race
    condition described in the bug report. It may not address other issues
    such as pre-existing issue wit...