<div dir="ltr">Thanks for this analysis Ihar.<div>Some comments inline.<br><div class="gmail_extra"><br><div class="gmail_quote">On 17 April 2015 at 14:45, Ihar Hrachyshka <span dir="ltr"><<a href="mailto:ihrachys@redhat.com" target="_blank">ihrachys@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA1<br>
<br>
Hi,<br>
<br>
tl;dr neutron has special semantics for policy targets that relies on<br>
private symbols from oslo.policy, and it's impossible to introduce<br>
this semantics into oslo.policy itself due to backwards compatibility<br>
concerns, meaning we need to expose some more symbols as part of<br>
public API for the library to facilitate neutron switch to it.<br>
<br>
===<br>
<br>
oslo.policy was graduated during Kilo [1]. Neutron considered the<br>
switch to it [2], but failed to achieve it because some library<br>
symbols that were originally public (or at least looked like public)<br>
in policy.py from oslo-incubator, became private in oslo.policy.<br>
Specifically, Neutron policy code [3] relies on the following symbols<br>
that are now hidden inside oslo_policy._checks (note the underscore in<br>
the name of the module that suggests we cannot use the module directly):<br>
<br>
- - RoleCheck<br>
- - RuleCheck<br>
- - AndCheck<br>
<br>
Those symbols are used for the following matters:<br>
(all the relevant neutron code is in neutron/policy.py)<br>
<br>
1. debug logging in case policy does not authorize an action<br>
(RuleCheck, AndCheck) [log_rule_list]<br>
<br>
2. filling in admin context with admin roles (RoleCheck, RuleCheck,<br>
AndCheck/OrCheck internals) [get_admin_roles]<br>
<br>
3. aggregating core, attribute and subattribute policies (RuleCheck,<br>
AndCheck) [_prepare_check]<br>
<br>
<br>
== 1. debug logging in case policy does not authorize an action ==<br>
<br>
Neutron logs rules that failed to match if policy module does not<br>
authorize an action. Not sure whether Neutron developers really want<br>
to have those debug logs, and whether we cannot just kill them to<br>
avoid this specific usage of private symbols; though it also seems<br>
that we could easily use __str__ that is present for all types of<br>
Checks instead. So it does not look like a blocker for the switch.<br></blockquote><div><br></div><div>Definitely not a blocker - we could as you suggest, or removing that logging altogether.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
== 2. filling in admin context with admin roles ==<br>
<br>
Admin context object is filled with .roles attribute that is a list of<br>
roles considered granting admin permissions [4]. The attribute would<br>
then be used by plugins that would like to do explicit policy checks.<br>
As per Salvatore, this attribute can probably be dropped now that all<br>
plugins and services don't rely on it (Salvatore mentioned lbaas<br>
mixins as the ones that previously relied on it, but are now not doing<br>
it since service split from neutron tree (?)).<br>
<br>
The problem with dropping the .roles attribute from context object in<br>
Liberty is that we, as a responsible upstream with lots of plugins<br>
maintained out-of-tree (see the ongoing vendor decomposition effort)<br>
would need to support the attribute while it's marked as deprecated<br>
for at least one cycle, meaning that if we don't get those oslo.policy<br>
internals we rely on in Liberty, we would need to postpone the switch<br>
till Mizzle, or rely on private symbols during the switch (while a new<br>
release of oslo.policy can easily break us).<br>
<br>
(BTW the code to extract admin roles is not really robust and has<br>
bugs, f.e. it does not handle AndChecks that could be used in<br>
context_is_admin. In theory, 'and' syntax would mean that both roles<br>
are needed to claim someone is an admin, while the code to extract<br>
admin roles handles 'and' the same way as 'or'. For the deprecation<br>
time being, we may need to document this limitation.)<br></blockquote><div><br></div><div>Roles are normally populated by the keystone middleware. I'm not sure whether we want to drop them altogether from the context, but I would expect the policy engine to use them even after switching to oslo.policy - Plugins should never leverage this kind of information. I am indeede tempted to drop roles and other AAA-related info from the context when it's dispatched to the plugin. This should be done carefully - beyond breaking some plugins it might also impact the notifiers we use to communicate with nova.</div><div><br></div><div>The function which artificially populates roles in the context was built for artificial contextes, which in some cases were created by plugins performing db operations at startup. I would check if we still have this requirement, and if not remove the function.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
== 3. aggregating core, attribute and subattribute policies ==<br>
<br>
That's the most interesting issue.<br>
<br>
For oslo.policy, policies are described as "target: rule", where rule<br>
is interpreted as per registered checks, while target is opaque to the<br>
library.<br>
<br>
Neutron extended the syntax for target as:<br>
target[:attribute[:subattribute]].<br>
<br>
If attribute is present in a policy entry, it applies to target iff<br>
attribute is set, and 'enforce_policy' is set in attribute map for the<br>
attribute in question, and target is not read-only (=its name does not<br>
start from "get_").<br>
<br>
If subattribute is present, the rule applies to target if 'validate'<br>
is set in attribute map for the attribute, and its type is dict, plus<br>
all the requirements for :attribute described above.<br>
<br>
Note that those rules are *aggregated* into single matching rule with<br>
AndCheck, so f.e. if action is create_network, and provider is set in<br>
target, then the actual rule validated would be all the rules for<br>
create_network, create_network:provider, and e.g.<br>
create_network:provider:physical_network, joined into single rule with<br>
AndCheck (meaning, target should conform to all of those requirements).<br>
<br>
This stands for a significant extension of original oslo.policy intent.<br>
<br>
Originally, I thought that we would be able to introduce neutron<br>
policy semantics into oslo.policy, and just switch to it once it's<br>
there. But there is a problem with that approach. Other projects (like<br>
nova [5]) already use similar syntax for their policy targets, while<br>
not putting such semantics on top of what oslo.policy provides (which<br>
is basically nothing, for target is not interpreted in any special<br>
way). AFAIU the way those projects use this syntax does not introduce<br>
any new *meaning*, so it's used for mere convenience to split policy<br>
file into logical parts (or namespaces). It means that if we would<br>
introduce neutron semantics into oslo.policy, that would interfere<br>
with other consumers of the library that rely on the fact that target<br>
is opaque for policy mechanism, probably breaking their policies,<br>
requiring users to switch to new syntax, or even silently exposing<br>
actions to unauthorized users.<br>
<br>
AFAIU that leaves Neutron with its custom crafted semantics to<br>
maintain outside oslo.policy. On the other side, we cannot just drop<br>
this semantics from Neutron, otherwise we would break existing setups<br>
that may rely on it. Meaning, we still need access to RuleCheck and<br>
AndCheck to switch to the library.<br></blockquote><div><br></div><div>Policies on subattributes are an abomination of nature and they should go. The problem however is that this needs first a rethink about some API extensions - namely the one for external gateway modes.</div><div>However, as you say we can't really do without policies on attributes at the moment.</div><div>Policies like the following:</div><div><br></div><div>"create_subnetpool:shared": "rule:admin_only"<br></div><div><br></div><div>Led us to implement [1], which uses the "symbols" which were now made private.</div><div>That logic is specific for Neutron, which adds semantic value to the policy target. As Ihar says, imposing this on all the other projects might not be welcome and in some cases break the project themselves.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
So the question to oslo.policy maintainers is: whether all that is<br>
said above makes sense, and if so, whether we may now consider<br>
exposing those private symbols (+ maybe OrCheck, NotCheck, and other<br>
primitives that are logically bound to AndCheck) as part of public<br>
API. If community agrees with my analysis and justification for the<br>
change, I am happy to propose a patch that would do just that.<br></blockquote><div><br></div><div>Making this possilbe would be the quickest path for Neutron.</div><div>However if the oslo_policy team took this decision it must have been for a solid design reasoning.</div><div>It is tough to ask to revise a design decision for a single user of a library.</div><div>Unfortunately a particular Neutron developer took the liberty of playing with authZ policies - I bet he was even proud of what he did: leaving the project with more technical debt. That particular developer should be dealt with appropriately, but this is another story.</div><div><br></div><div>I think that the only alternative to making those symbols public in oslo_policy is for Neutron to perform attribute and sub-attribute authZ checks in a different fashion (perhaps an additional engine). This will be a backward incompatible configuration change as deployers will have to replace their policy.json file. Probably scripts might be provided to ease the transition, but it's not going to be simple.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
PS: one more question to oslo.policy maintainers is whether consuming<br>
projects may rely on Enforcer.rules attribute being present, and<br>
following dict semantics, as it currently does. Neutron and other<br>
projects (like Nova) relies on the attribute, while it's not really<br>
documented in official library documentation. We may need<br>
clarification on whether its usage is supported, and document it<br>
appropriately.<br>
<br>
[1]: <a href="https://review.openstack.org/#/c/140161/" target="_blank">https://review.openstack.org/#/c/140161/</a><br>
[2]:<br>
<a href="http://lists.openstack.org/pipermail/openstack-dev/2015-January/054753.h
tml" target="_blank">http://lists.openstack.org/pipermail/openstack-dev/2015-January/054753.h<br>
tml</a><br>
[3]:<br>
<a href="http://git.openstack.org/cgit/openstack/neutron/tree/neutron/policy.py" target="_blank">http://git.openstack.org/cgit/openstack/neutron/tree/neutron/policy.py</a><br>
[4]:<br>
<a href="http://git.openstack.org/cgit/openstack/neutron/tree/neutron/context.py#
n71" target="_blank">http://git.openstack.org/cgit/openstack/neutron/tree/neutron/context.py#<br>
n71</a><br>
[5]:<br>
<a href="http://git.openstack.org/cgit/openstack/nova/tree/etc/nova/policy.json#n" target="_blank">http://git.openstack.org/cgit/openstack/nova/tree/etc/nova/policy.json#n</a><br>
33<br>
<br>
Thanks,<br>
/Ihar<br>
-----BEGIN PGP SIGNATURE-----<br>
Version: GnuPG v2<br>
<br>
iQEcBAEBAgAGBQJVMQCGAAoJEC5aWaUY1u57pGMH/j2dq6kqFlofrvz8uDpiv8Gr<br>
O3YhzbhfO/zkNkfS/N/bS48QmxsVOY6mAvSZ/lJwMteGU2EDipP8zVnSKIwnk9rX<br>
Wh2NWiPOT+dRkC/a8XCtT10n9gdz2yOcenB2ezEvRHNmKNjM76hSVT0RO8tk05+R<br>
K/n2ANQcRTY9q1WzC+w8xTSc1CpvbFY+1k0DqQqKZW1JInl0kFyxzTggtUxuIz4o<br>
j4epSiFL8Z4GkXJ51GVpDj5vQA+pgvYfieSnrF23VwF6gV1BD4uxccOtegrDZx+C<br>
b2L/9dQsAexBxp8x9Xm798lkhMjKE52GV9PrtMr0zLRMlJFDZSPbmramlthUh+I=<br>
=cr7A<br>
-----END PGP SIGNATURE-----<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div><br></div></div></div>