Cyclical reference check seems broken

Bug #1843931 reported by Ben Nemec on 2019-09-13
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
oslo.policy
Medium
Ben Nemec

Bug Description

If I create a policy file with the following rules:

"identity:get_application_credential": "(role:reader and system_scope:all) or rule:owner"
"identity:get_application_credentials": "rule:identity:get_application_credential"

I am getting a warning from oslo.policy about them being part of a cyclical reference. I don't see how that's cyclical though. The latter rule refers to the former, but the former does not reciprocate. Unless somehow rule:owner has a nested reference to identity:get_application_credentials, but I'm pretty sure that's not the case (and if it is: why?).

This is particularly problematic as this pattern is how we handle deprecations in the generated sample policy files, so it's going to be a common thing.

Ben Nemec (bnemec) wrote :

Oh, actually I was mistaken. If you explicitly put both rules in the policy file then the warning is not triggered. It's only if you rely on the fact that the first rule is the default in code that you get the warning. So the problematic policy file is actually more like:

# This is the default from policy-in-code
#"identity:get_application_credential": "(role:reader and system_scope:all) or rule:owner"
"identity:get_application_credentials": "rule:identity:get_application_credential"

The rest of what I said is still valid.

Ben Nemec (bnemec) wrote :

Strange. If I dump out the list of rules in the cycle check, I see both

"identity:get_application_credentials": "rule:identity:get_application_credential",

and

"identity:get_application_credential": "rule:identity:get_application_credential",

If I explicitly set identity:get_application_credential then I get the expected rule. Something is implicitly adding a circular rule. :-/

Fix proposed to branch: master
Review: https://review.opendev.org/682150

Changed in oslo.policy:
assignee: nobody → Ben Nemec (bnemec)
status: Confirmed → In Progress

Reviewed: https://review.opendev.org/682150
Committed: https://git.openstack.org/cgit/openstack/oslo.policy/commit/?id=82a2c8d8b71896b1f8b7f33c560681367ae76755
Submitter: Zuul
Branch: master

commit 82a2c8d8b71896b1f8b7f33c560681367ae76755
Author: Ben Nemec <email address hidden>
Date: Fri Sep 13 20:04:56 2019 +0000

    Fix reference cycle caused by deprecated sample override

    In the sample policy generator, we create a rule that maps the
    deprecated name of a policy to the new rule name. For example:

    identity:old_rule: rule:identity:new_rule

    However, in the policy code, if we see an override of a deprecated
    name and no override for the new name, we apply the value of the
    deprecated name to the new name. In the above case, this results
    in us creating a rule that looks like:

    identity:new_rule: rule:identity:new_rule

    which is a circular reference and nonsense.

    To fix this, I added a check to the deprecated rule logic that looks
    for instances where the old override is just a reference to the new
    rule. If that's the case, then we don't need to do anything because
    it's already doing the right thing.

    Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
    Closes-Bug: 1843931

Changed in oslo.policy:
status: In Progress → Fix Released

Reviewed: https://review.opendev.org/684314
Committed: https://git.openstack.org/cgit/openstack/oslo.policy/commit/?id=0a4adba1a53c25ad75a10525130d832138f4fc68
Submitter: Zuul
Branch: stable/stein

commit 0a4adba1a53c25ad75a10525130d832138f4fc68
Author: Ben Nemec <email address hidden>
Date: Fri Sep 13 20:04:56 2019 +0000

    Fix reference cycle caused by deprecated sample override

    In the sample policy generator, we create a rule that maps the
    deprecated name of a policy to the new rule name. For example:

    identity:old_rule: rule:identity:new_rule

    However, in the policy code, if we see an override of a deprecated
    name and no override for the new name, we apply the value of the
    deprecated name to the new name. In the above case, this results
    in us creating a rule that looks like:

    identity:new_rule: rule:identity:new_rule

    which is a circular reference and nonsense.

    To fix this, I added a check to the deprecated rule logic that looks
    for instances where the old override is just a reference to the new
    rule. If that's the case, then we don't need to do anything because
    it's already doing the right thing.

    Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
    Closes-Bug: 1843931
    (cherry picked from commit 82a2c8d8b71896b1f8b7f33c560681367ae76755)

tags: added: in-stable-stein
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers