[RFE] Only enforce policy when selected option does not match default

[Nate Johnston]

Certain API behaviors are regulated by oslo.policy policy at a granular level, but also have default values. If a user supplies API options that match the defaults, bypass the policy check since the result will be the same regardless.

A good example of this is creating a port with the the boolean "enable_port_security" value, which in a typical deployment defaults to 'True'. The "create_port:port_security_enabled" policy governs this behavior, and is typically set to "rule:context_is_advsvc or rule:admin_or_network_owner" which means a non-admin user that is not the network owner would fail. Such a user should be able to specify port_security=True when creating a port and not have that operation fail the policy check.

Implementation
--------------
The policy check occurs almost immediately upon request reciept. Check for calls to enforce() in neutron/api/v2/base.py [1]. A data structure would need to be created from the policy-processing code that matches policy names with their respective default values. Then the enforce() call would be made contingent on divergence from the default.

[1] example: https://opendev.org/openstack/neutron/src/branch/master/neutron/api/v2/base.py#L468

[Miguel Lavalle (minsel)]

1) Why implement this in Neutron? Why not in oslo.policy?

2) What is the use case that requires this change? In other words, what's the potential gain for the effort put in implementing this proposal?

[Nate Johnston (nate-johnston)]

1.) The reason not to implement this in oslo.policy is that oslo.policy has no concept of the rvalues for the operations it is filtering by policy. This proposal discusses filtering by the tuple of "API method:argument value" while oslo.policy only screens by "API method:identity of caller". I asked in #openstack-oslo about this kind of filtering, and they directed me towards the Keystone folks, who said for this end this was the proper means, "so you can move bits up and pre-process the request as needed" [1].

2.) There are two reasons, a specific reason and a general reason.

- General reason: The current behavior is highly non-intuitive for users. As engineers we think in terms of the code, and so having oslo.policy act like a filter that screens out calls users aren't allowed to do is something that seems intuitive to us. But to a user saying "You can create a port that ends up having port security enabled set to 'True', but if you explicitly ask for port security enabled then it fails" is a bad UI interaction because from their perspective it is nonsensical.

- Specific reason: The specific port security example I have cited comes up in the context of Horizon. Sometimes when creating a port, Horizon displays an "enable port security" checkbox. When that display happens, Horizon will send the appropriate port security enabled True or False to Neutron, which essentially means that for a non-admin user port creation is impossible. There's no available setting that won't send a port security option.

[1] http://eavesdrop.openstack.org/irclogs/%23openstack-keystone/%23openstack-keystone.2019-03-21.log.html#t2019-03-21T16:24:19

[Miguel Lavalle (minsel)]

Fair enough. Let's discuss it during the next driver meeting

[Akihiro Motoki (amotoki)]

I have several comments on answers to 2) in #2.

> General reason

I think there are two viewpoints in this topic.
One viewpoint is what Nate mentions, i.e., when the default value of an attribute and a post value are equal, no error should be returned and it should be accepted.
The other viewpoint is a role-based approach, i.e., hey, you can specify this field and you cannot specify that field (as a regular user).

If we move the first viewpoint forward, doesn't it mean that non-admin user try to specify provider network attributes and if it matches current values it will succeed. The non-admin user can estimate what provider network attributes are actually used. I think it is not a good behavior from a security reason. (From the same reason, we return 404 for resources which a requester does not own.)

> Specific reason

If it is only limited to horizon, it should be a bug in horizon. horizon controls which fields should be displayed based on policy.json, but the implementation is not throughout and there are many missing points. I can check it soon later.

[Nate Johnston (nate-johnston)]

> If we move the first viewpoint forward, doesn't it mean that non-admin user
> try to specify provider network attributes and if it matches current values
> it will succeed. The non-admin user can estimate what provider network
> attributes are actually used. I think it is not a good behavior from a
> security reason.

If a user creates an instance without specifying provider network attributes they will succeed. Can't they divine most if not all of the provider network attributes from inspecting the VM? If so then we aren't keeping the provider network attributes secret. I'm not sure why we would want to anyway.

But the main point is this: you are changing behaviors that do not affect the result. Let's say port security defaults to true, and permission to indicate the port security flag is restricted by policy to admin or network_owner. If this change was implemented, if a generic tenant user that owns no networks sends a request to create an instance with port security enabled, it would succeed because the end result is exactly the same as if the tenant user sent a request to create an instance and did not specify port security. The resulting VM is exactly the same if nothing is specified or if the offending port security is specified. For a results-oriented user that is a bad interaction.

And it doesn't have to be that way; oslo.policy is implemented in such a way that the policy check need not the the first thing that is done, we could prevalidate certain conditions before the oslo.policy check.

If there is a security concern then obviously that would take precedence, but I could not enumerate a specific security concern with this. So I am very interested to see what specific security principle this would go against, or better yet a scenario where this could be used to escalate permissions or otherwise violate security, assuming the prevalidation I mentioned before is done correctly. It would be very helpful for me to learn that.

[YAMAMOTO Takashi (yamamoto)]

is this only for POST? or, also for PUT matching the current value?

[Akihiro Motoki (amotoki)]

Sorry for late.

The original request talks about boolean fields, but I am trying to generalize this behavior.

The proposed behavior looks okay if it applies to visible fields.
There is a case where some field is visible to regular users but it is not updatable by regular users.
In such case, there is no side effect to POST or PUT the current value to such fields.

I don't see any direct security concerns on this. I am just worried about leaking information which is not exposed to regular users (for example network segmentation ID). They do not lead to security risks directly but some operators might not want to expose them.

[Slawek Kaplonski (slaweq)]

@Akihiro: how about proposing config option, like "policy_allows_default_values_always" (or something like that, I'm not good in naming :)) and default it to False.
That way we may keep original behaviour and give some operators possibility to change it to make some custom scripts working if they are not affraid about leaking informations that way.

[Slawek Kaplonski (slaweq)]

On last drivers team meeting we agreed to abandon this rfe as this is going to address only corner case which can be easily workarounded.