Open Stack

On 04/17/2015 08:45 AM, Ihar Hrachyshka wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> tl;dr neutron has special semantics for policy targets that relies on
> private symbols from oslo.policy, and it's impossible to introduce
> this semantics into oslo.policy itself due to backwards compatibility
> concerns, meaning we need to expose some more symbols as part of
> public API for the library to facilitate neutron switch to it.
>
> ===
>
> oslo.policy was graduated during Kilo [1]. Neutron considered the
> switch to it [2], but failed to achieve it because some library
> symbols that were originally public (or at least looked like public)
> in policy.py from oslo-incubator, became private in oslo.policy.
> Specifically, Neutron policy code [3] relies on the following symbols
> that are now hidden inside oslo_policy._checks (note the underscore in
> the name of the module that suggests we cannot use the module directly):
>
> - - RoleCheck
> - - RuleCheck
> - - AndCheck
>
> Those symbols are used for the following matters:
> (all the relevant neutron code is in neutron/policy.py)
>
> 1. debug logging in case policy does not authorize an action
> (RuleCheck, AndCheck) [log_rule_list]
>
> 2. filling in admin context with admin roles (RoleCheck, RuleCheck,
> AndCheck/OrCheck internals) [get_admin_roles]
>
> 3. aggregating core, attribute and subattribute policies (RuleCheck,
> AndCheck) [_prepare_check]
If we make the checks public, is that sufficient?  I have no problem 
with most of these being public.
The only one that is Keystone specific is the Role check, which I would 
like to rework.  Instead of RoleCheck, can you build on top of  the 
newly submitted ability to do List checks?

https://review.openstack.org/#/c/169045/3/oslo_policy/_checks.py,cm


Rule, And, Or, Not and Generic checks are building blocks for other 
logic, and should be public.


>
> == 1. debug logging in case policy does not authorize an action ==
>
> Neutron logs rules that failed to match if policy module does not
> authorize an action. Not sure whether Neutron developers really want
> to have those debug logs, and whether we cannot just kill them to
> avoid this specific usage of private symbols; though it also seems
> that we could easily use __str__ that is present for all types of
> Checks instead. So it does not look like a blocker for the switch.
>
>
> == 2. filling in admin context with admin roles ==
>
> Admin context object is filled with .roles attribute that is a list of
> roles considered granting admin permissions [4]. The attribute would
> then be used by plugins that would like to do explicit policy checks.
> As per Salvatore, this attribute can probably be dropped now that all
> plugins and services don't rely on it (Salvatore mentioned lbaas
> mixins as the ones that previously relied on it, but are now not doing
> it since service split from neutron tree (?)).
>
> The problem with dropping the .roles attribute from context object in
> Liberty is that we, as a responsible upstream with lots of plugins
> maintained out-of-tree (see the ongoing vendor decomposition effort)
> would need to support the attribute while it's marked as deprecated
> for at least one cycle, meaning that if we don't get those oslo.policy
> internals we rely on in Liberty, we would need to postpone the switch
> till Mizzle, or rely on private symbols during the switch (while a new
> release of oslo.policy can easily break us).
>
> (BTW the code to extract admin roles is not really robust and has
> bugs, f.e. it does not handle AndChecks that could be used in
> context_is_admin. In theory, 'and' syntax would mean that both roles
> are needed to claim someone is an admin, while the code to extract
> admin roles handles 'and' the same way as 'or'. For the deprecation
> time being, we may need to document this limitation.)
>
>
> == 3. aggregating core, attribute and subattribute policies ==
>
> That's the most interesting issue.
>
> For oslo.policy, policies are described as "target: rule", where rule
> is interpreted as per registered checks, while target is opaque to the
> library.
>
> Neutron extended the syntax for target as:
> target[:attribute[:subattribute]].
>
> If attribute is present in a policy entry, it applies to target iff
> attribute is set, and 'enforce_policy' is set in attribute map for the
> attribute in question, and target is not read-only (=its name does not
> start from "get_").
>
> If subattribute is present, the rule applies to target if 'validate'
> is set in attribute map for the attribute, and its type is dict, plus
> all the requirements for :attribute described above.
>
> Note that those rules are *aggregated* into single matching rule with
> AndCheck, so f.e. if action is create_network, and provider is set in
> target, then the actual rule validated would be all the rules for
> create_network, create_network:provider, and e.g.
> create_network:provider:physical_network, joined into single rule with
> AndCheck (meaning, target should conform to all of those requirements).
>
> This stands for a significant extension of original oslo.policy intent.
>
> Originally, I thought that we would be able to introduce neutron
> policy semantics into oslo.policy, and just switch to it once it's
> there. But there is a problem with that approach. Other projects (like
> nova [5]) already use similar syntax for their policy targets, while
> not putting such semantics on top of what oslo.policy provides (which
> is basically nothing, for target is not interpreted in any special
> way). AFAIU the way those projects use this syntax does not introduce
> any new *meaning*, so it's used for mere convenience to split policy
> file into logical parts (or namespaces). It means that if we would
> introduce neutron semantics into oslo.policy, that would interfere
> with other consumers of the library that rely on the fact that target
> is opaque for policy mechanism, probably breaking their policies,
> requiring users to switch to new syntax, or even silently exposing
> actions to unauthorized users.
>
> AFAIU that leaves Neutron with its custom crafted semantics to
> maintain outside oslo.policy. On the other side, we cannot just drop
> this semantics from Neutron, otherwise we would break existing setups
> that may rely on it. Meaning, we still need access to RuleCheck and
> AndCheck to switch to the library.
>
> So the question to oslo.policy maintainers is: whether all that is
> said above makes sense, and if so, whether we may now consider
> exposing those private symbols (+ maybe OrCheck, NotCheck, and other
> primitives that are logically bound to AndCheck) as part of public
> API. If community agrees with my analysis and justification for the
> change, I am happy to propose a patch that would do just that.
>
> PS: one more question to oslo.policy maintainers is whether consuming
> projects may rely on Enforcer.rules attribute being present, and
> following dict semantics, as it currently does. Neutron and other
> projects (like Nova) relies on the attribute, while it's not really
> documented in official library documentation. We may need
> clarification on whether its usage is supported, and document it
> appropriately.
>
> [1]: https://review.openstack.org/#/c/140161/
> [2]:
> http://lists.openstack.org/pipermail/openstack-dev/2015-January/054753.h
> tml
> [3]:
> http://git.openstack.org/cgit/openstack/neutron/tree/neutron/policy.py
> [4]:
> http://git.openstack.org/cgit/openstack/neutron/tree/neutron/context.py#
> n71
> [5]:
> http://git.openstack.org/cgit/openstack/nova/tree/etc/nova/policy.json#n
> 33
>
> Thanks,
> /Ihar
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQEcBAEBAgAGBQJVMQCGAAoJEC5aWaUY1u57pGMH/j2dq6kqFlofrvz8uDpiv8Gr
> O3YhzbhfO/zkNkfS/N/bS48QmxsVOY6mAvSZ/lJwMteGU2EDipP8zVnSKIwnk9rX
> Wh2NWiPOT+dRkC/a8XCtT10n9gdz2yOcenB2ezEvRHNmKNjM76hSVT0RO8tk05+R
> K/n2ANQcRTY9q1WzC+w8xTSc1CpvbFY+1k0DqQqKZW1JInl0kFyxzTggtUxuIz4o
> j4epSiFL8Z4GkXJ51GVpDj5vQA+pgvYfieSnrF23VwF6gV1BD4uxccOtegrDZx+C
> b2L/9dQsAexBxp8x9Xm798lkhMjKE52GV9PrtMr0zLRMlJFDZSPbmramlthUh+I=
> =cr7A
> -----END PGP SIGNATURE-----
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev