Make upgrades more robust with policy overrides
Previously, we'd notify operators of changing or deprecated policies by logging a warning while loading rules. However, this doesn't prevent unintended access if an operator is overriding a policy by its old policy name. In this case, let's make sure we check if the old policy is being overridden and use that override for the new policy's check value. This commit introduces this change along with a few tests. It also refactors the deprecated rule logic in load_rules() to separate methods so that it's a little easier to understand where that logic happens within the load_rules() method without cluttering it. Co-Authored-By: Juan Antonio Osorio Robles <jaosorior@redhat.com> Closes-Bug: 1800259 Change-Id: Ice27cdb44241da94693625776037ea6164bbb913
This commit is contained in:
Lance Bragstad
parent
a102757726
commit
5aeac664ae
|
|
@ -574,66 +574,11 @@ class Enforcer(object):
|
|||
force_reload, False)
|
||||
|
||||
for default in self.registered_rules.values():
|
||||
if default.deprecated_rule:
|
||||
deprecated_msg = (
|
||||
'Policy "%(old_name)s":"%(old_check_str)s" was '
|
||||
'deprecated in %(release)s in favor of "%(name)s":'
|
||||
'"%(check_str)s". Reason: %(reason)s. Either ensure '
|
||||
'your deployment is ready for the new default or '
|
||||
'copy/paste the deprecated policy into your policy '
|
||||
'file and maintain it manually.' % {
|
||||
'old_name': default.deprecated_rule.name,
|
||||
'old_check_str': default.deprecated_rule.check_str,
|
||||
'release': default.deprecated_since,
|
||||
'name': default.name,
|
||||
'check_str': default.check_str,
|
||||
'reason': default.deprecated_reason
|
||||
}
|
||||
)
|
||||
if default.deprecated_rule.name != default.name and (
|
||||
default.deprecated_rule.name in self.rules):
|
||||
# Print a warning because the actual policy name is
|
||||
# changing. If deployers are relying on an override for
|
||||
# foo:bar and it's getting renamed to foo:create_bar
|
||||
# then they need to be able to see that before they
|
||||
# roll out the next release.
|
||||
warnings.warn(deprecated_msg)
|
||||
if (default.deprecated_rule.check_str !=
|
||||
default.check_str and default.name not in
|
||||
self.rules):
|
||||
# In this case, the default check_str is changing. We
|
||||
# need to let operators know that this is going to
|
||||
# change. If they don't want to override it, they are
|
||||
# going to have to make sure the right infrastructure
|
||||
# exists before they upgrade. This overrides the new
|
||||
# check with an OrCheck that combines the new and old
|
||||
# check_str attributes from the new and deprecated
|
||||
# policies. This will make it so that deployments don't
|
||||
# break on upgrade, but they receive log messages
|
||||
# telling them stuff is going to change if they don't
|
||||
# maintain the policy manually or add infrastructure to
|
||||
# their deployment to support the new policy.
|
||||
default.check = _parser.parse_rule(
|
||||
default.check_str + ' or ' +
|
||||
default.deprecated_rule.check_str
|
||||
)
|
||||
warnings.warn(deprecated_msg)
|
||||
if default.deprecated_for_removal and (
|
||||
default.name in self.file_rules):
|
||||
# If a policy is going to be removed altogether, then we
|
||||
# need to make sure we let operators know so they can clean
|
||||
# up their policy files, if they are overriding it.
|
||||
warnings.warn(
|
||||
'Policy "%(policy)s":"%(check_str)s" was '
|
||||
'deprecated for removal in %(release)s. Reason: '
|
||||
'%(reason)s. Its value may be silently ignored in '
|
||||
'the future.' % {
|
||||
'policy': default.name,
|
||||
'check_str': default.check_str,
|
||||
'release': default.deprecated_since,
|
||||
'reason': default.deprecated_reason
|
||||
}
|
||||
)
|
||||
if default.deprecated_for_removal:
|
||||
self._emit_deprecated_for_removal_warning(default)
|
||||
elif default.deprecated_rule:
|
||||
self._handle_deprecated_rule(default)
|
||||
|
||||
if default.name not in self.rules:
|
||||
self.rules[default.name] = default.check
|
||||
|
||||
|
|
@ -667,6 +612,87 @@ class Enforcer(object):
|
|||
|
||||
return not violation
|
||||
|
||||
def _emit_deprecated_for_removal_warning(self, default):
|
||||
# If the policy is being removed completely, we need to let operators
|
||||
# know that the policy is going to be silently ignored in the future
|
||||
# and they can remove it from their overrides since it isn't being
|
||||
# replaced by another policy.
|
||||
if default.name in self.file_rules:
|
||||
warnings.warn(
|
||||
'Policy "%(policy)s":"%(check_str)s" was deprecated for '
|
||||
'removal in %(release)s. Reason: %(reason)s. Its value may be '
|
||||
'silently ignored in the future.' % {
|
||||
'policy': default.name,
|
||||
'check_str': default.check_str,
|
||||
'release': default.deprecated_since,
|
||||
'reason': default.deprecated_reason
|
||||
}
|
||||
)
|
||||
|
||||
def _handle_deprecated_rule(self, default):
|
||||
"""Handle cases where a policy rule has been deprecated.
|
||||
|
||||
:param default: an instance of RuleDefault that contains an instance of
|
||||
DeprecatedRule
|
||||
"""
|
||||
|
||||
deprecated_rule = default.deprecated_rule
|
||||
|
||||
deprecated_msg = (
|
||||
'Policy "%(old_name)s":"%(old_check_str)s" was deprecated in '
|
||||
'%(release)s in favor of "%(name)s":"%(check_str)s". Reason: '
|
||||
'%(reason)s. Either ensure your deployment is ready for the new '
|
||||
'default or copy/paste the deprecated policy into your policy '
|
||||
'file and maintain it manually.' % {
|
||||
'old_name': deprecated_rule.name,
|
||||
'old_check_str': deprecated_rule.check_str,
|
||||
'release': default.deprecated_since,
|
||||
'name': default.name,
|
||||
'check_str': default.check_str,
|
||||
'reason': default.deprecated_reason
|
||||
}
|
||||
)
|
||||
|
||||
# Print a warning because the actual policy name is changing. If
|
||||
# operators are relying on an override for foo:bar and it's getting
|
||||
# renamed to foo:create_bar then they need to be able to see that
|
||||
# before they roll out the next release. If the policy name is in
|
||||
# self.file_rules, we know that it's being overridden.
|
||||
if deprecated_rule.name != default.name and (
|
||||
deprecated_rule.name in self.file_rules):
|
||||
|
||||
warnings.warn(deprecated_msg)
|
||||
|
||||
# If the deprecated policy is being overridden and doesn't match
|
||||
# the default deprecated policy, override the new policy's default
|
||||
# with the old check string. This should prevents unwanted exposure
|
||||
# to APIs on upgrade.
|
||||
if (self.file_rules[deprecated_rule.name].check
|
||||
!= _parser.parse_rule(deprecated_rule.check_str)):
|
||||
if default.name not in self.file_rules.keys():
|
||||
self.rules[default.name] = self.file_rules[
|
||||
deprecated_rule.name
|
||||
].check
|
||||
|
||||
# In this case, the default check string is changing. We need to let
|
||||
# operators know that this is going to change. If they don't want to
|
||||
# override it, they are going to have to make sure the right
|
||||
# infrastructure exists before they upgrade. This overrides the new
|
||||
# check with an OrCheck that combines the new and old check_str
|
||||
# attributes from the new and deprecated policies. This will make it so
|
||||
# that deployments don't break on upgrade, but they receive log
|
||||
# messages telling them stuff is going to change if they don't maintain
|
||||
# the policy manually or add infrastructure to their deployment to
|
||||
# support the new policy.
|
||||
if (deprecated_rule.check_str != default.check_str
|
||||
and default.name not in self.file_rules):
|
||||
|
||||
default.check = _parser.parse_rule(
|
||||
default.check_str + ' or ' +
|
||||
deprecated_rule.check_str
|
||||
)
|
||||
warnings.warn(deprecated_msg)
|
||||
|
||||
def _undefined_check(self, check):
|
||||
'''Check if a RuleCheck references an undefined rule.'''
|
||||
if isinstance(check, RuleCheck):
|
||||
|
|
|
|||
|
|
@ -1203,6 +1203,124 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
|
|||
deprecated_since='N'
|
||||
)
|
||||
|
||||
def test_override_deprecated_policy_with_old_name(self):
|
||||
# Simulate an operator overriding a policy
|
||||
rules = jsonutils.dumps({'foo:bar': 'role:bazz'})
|
||||
self.create_config_file('policy.json', rules)
|
||||
|
||||
# Deprecate the policy name and check string in favor of something
|
||||
# better.
|
||||
deprecated_rule = policy.DeprecatedRule(
|
||||
name='foo:bar',
|
||||
check_str='role:fizz'
|
||||
)
|
||||
rule_list = [policy.DocumentedRuleDefault(
|
||||
name='foo:create_bar',
|
||||
check_str='role:bang',
|
||||
description='Create a bar.',
|
||||
operations=[{'path': '/v1/bars', 'method': 'POST'}],
|
||||
deprecated_rule=deprecated_rule,
|
||||
deprecated_reason='"role:bang" is a better default',
|
||||
deprecated_since='N'
|
||||
)]
|
||||
self.enforcer.register_defaults(rule_list)
|
||||
|
||||
# Make sure the override supplied by the operator using the old policy
|
||||
# name is used in favor of the old or new default.
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
|
||||
)
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
|
||||
)
|
||||
self.assertTrue(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
|
||||
)
|
||||
|
||||
def test_override_deprecated_policy_with_new_name(self):
|
||||
# Simulate an operator overriding a policy using the new policy name
|
||||
rules = jsonutils.dumps({'foo:create_bar': 'role:bazz'})
|
||||
self.create_config_file('policy.json', rules)
|
||||
|
||||
# Deprecate the policy name and check string in favor of something
|
||||
# better.
|
||||
deprecated_rule = policy.DeprecatedRule(
|
||||
name='foo:bar',
|
||||
check_str='role:fizz'
|
||||
)
|
||||
rule_list = [policy.DocumentedRuleDefault(
|
||||
name='foo:create_bar',
|
||||
check_str='role:bang',
|
||||
description='Create a bar.',
|
||||
operations=[{'path': '/v1/bars', 'method': 'POST'}],
|
||||
deprecated_rule=deprecated_rule,
|
||||
deprecated_reason='"role:bang" is a better default',
|
||||
deprecated_since='N'
|
||||
)]
|
||||
self.enforcer.register_defaults(rule_list)
|
||||
|
||||
# Make sure the override supplied by the operator is being used in
|
||||
# place of either default value.
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
|
||||
)
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
|
||||
)
|
||||
self.assertTrue(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
|
||||
)
|
||||
|
||||
def test_override_both_new_and_old_policy(self):
|
||||
# Simulate an operator overriding a policy using both the the new and
|
||||
# old policy names. The following doesn't make a whole lot of sense
|
||||
# because the overrides are conflicting, but we want to make sure that
|
||||
# oslo.policy uses foo:create_bar instead of foo:bar.
|
||||
rules_dict = {
|
||||
'foo:create_bar': 'role:bazz',
|
||||
'foo:bar': 'role:wee'
|
||||
}
|
||||
rules = jsonutils.dumps(rules_dict)
|
||||
self.create_config_file('policy.json', rules)
|
||||
|
||||
# Deprecate the policy name and check string in favor of something
|
||||
# better.
|
||||
deprecated_rule = policy.DeprecatedRule(
|
||||
name='foo:bar',
|
||||
check_str='role:fizz'
|
||||
)
|
||||
rule_list = [policy.DocumentedRuleDefault(
|
||||
name='foo:create_bar',
|
||||
check_str='role:bang',
|
||||
description='Create a bar.',
|
||||
operations=[{'path': '/v1/bars', 'method': 'POST'}],
|
||||
deprecated_rule=deprecated_rule,
|
||||
deprecated_reason='"role:bang" is a better default',
|
||||
deprecated_since='N'
|
||||
)]
|
||||
self.enforcer.register_defaults(rule_list)
|
||||
|
||||
# The default check string for the old policy name foo:bar should fail
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
|
||||
)
|
||||
|
||||
# The default check string for the new policy name foo:create_bar
|
||||
# should fail
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
|
||||
)
|
||||
|
||||
# The override for the old policy name foo:bar should fail
|
||||
self.assertFalse(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['wee']})
|
||||
)
|
||||
|
||||
# The override for foo:create_bar should pass
|
||||
self.assertTrue(
|
||||
self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
|
||||
)
|
||||
|
||||
|
||||
class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase):
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue