openstack
/
keystone
Code Issues Proposed changes

Move delete_trust enforcement to default policies 

Without this change, policy enforcement for the DELETE
/OS-TRUST/trusts/{trust_id} API is hardcoded in the flask dispatcher
code. This is a problem because this enforcement can't be controlled by
the operator, as is the norm. Moreover, it makes the transition to
system-scope and default-roles-aware policies more difficult because
there's no sensible migration from "" to a logical role-based check
string.

This converts the hardcoded enforcement to enforcement via default
policies for DELETE /OS-TRUST/trusts/{trust_id}. Currently only the
trustor or the is_admin user can access this API (since the is_admin
user bypasses the policy loading). This behavior will be changed in a
future patch that will allow the system admin to access this API.

This change does not use the formal oslo.policy deprecation system
because "" OR'd with the new default is entirely useless as a policy.

Change-Id: I1aaba72b69b389ffbfcf7d5b8cc70453ffa59e73
Partial-bug: #1818850
Partial-bug: #1818846
This commit is contained in:
Colleen Murphy 2019-08-13 13:09:41 -07:00
parent 0df8d0e2e1
commit a09163a320
5 changed files with 106 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
keystone/api/trusts.py
View File

@ -46,6 +46,18 @@ TRUST_ID_PARAMETER_RELATION = _build_parameter_relation(
parameter_name='trust_id')
def _build_trust_target_enforcement():
target = {}
# NOTE(cmurphy) unlike other APIs, in the event the trust doesn't exist or
# has 0 remaining uses, we actually do expect it to return a 404 and not a
# 403, so don't catch NotFound here (lp#1840288)
target['trust'] = PROVIDERS.trust_api.get_trust(
flask.request.view_args.get('trust_id')
)
return target
def _trustor_trustee_only(trust):
user_id = flask.request.environ.get(context.REQUEST_CONTEXT_ENV).user_id
if user_id not in [trust.get('trustee_user_id'),
@ -263,18 +275,32 @@ class TrustResource(ks_flask.ResourceBase):
return self.wrap_member(return_trust), http_client.CREATED
def delete(self, trust_id):
ENFORCER.enforce_call(action='identity:delete_trust')
ENFORCER.enforce_call(action='identity:delete_trust',
build_target=_build_trust_target_enforcement)
self._check_unrestricted()
trust = PROVIDERS.trust_api.get_trust(trust_id)
# TODO(morgan): convert this check to an oslo_policy checkstr that
# can be referenced/enforced on.
if (self.oslo_context.user_id != trust.get('trustor_user_id') and
not self.oslo_context.is_admin):
action = _('Only admin or trustor can delete a trust')
raise exception.ForbiddenAction(action=action)
# NOTE(cmurphy) As of Train, the default policies enforce the
# identity:delete_trust rule. However, in case the
# identity:delete_trust rule has been locally overridden by the
# default that would have been produced by the sample config, we need
# to enforce it again and warn that the behavior is changing.
rules = policy._ENFORCER._enforcer.rules.get('identity:delete_trust')
# rule check_str is ""
if isinstance(rules, op_checks.TrueCheck):
LOG.warning(
"The policy check string for rule \"identity:delete_trust\" "
"has been overridden to \"always true\". In the next release, "
"this will cause the" "\"identity:delete_trust\" action to "
"be fully permissive as hardcoded enforcement will be "
"removed. To correct this issue, either stop overriding the "
"\"identity:delete_trust\" rule in config to accept the "
"defaults, or explicitly set a rule that is not empty."
)
trust = PROVIDERS.trust_api.get_trust(trust_id)
if (self.oslo_context.user_id != trust.get('trustor_user_id') and
not self.oslo_context.is_admin):
action = _('Only admin or trustor can delete a trust')
raise exception.ForbiddenAction(action=action)
PROVIDERS.trust_api.delete_trust(trust_id,
initiator=self.audit_initiator)
return '', http_client.NO_CONTENT

25
keystone/cmd/status.py
View File

@ -34,20 +34,25 @@ class Checks(upgradecheck.UpgradeCommands):
enforcer = policy.Enforcer(CONF)
ENFORCER.register_rules(enforcer)
enforcer.load_rules()
rule = enforcer.rules.get('identity:list_trusts')
if isinstance(rule, _checks.TrueCheck):
rules = ['identity:list_trusts', 'identity:delete_trust']
failed_rules = []
for rule in rules:
current_rule = enforcer.rules.get(rule)
if isinstance(current_rule, _checks.TrueCheck):
failed_rules.append(rule)
if any(failed_rules):
return upgradecheck.Result(
upgradecheck.Code.FAILURE,
"Policy check string for \"identity:list_trusts\" is "
"overridden to \"\", \"@\", or []. In the next release, "
"this will cause the \"identity:list_trusts\" action to be "
"fully permissive as hardcoded enforcement will be removed. "
"To correct this issue, either stop overriding this rule in "
"config to accept the defaults, or explicitly set a rule that "
"is not empty."
"Policy check string for rules \"%s\" are overridden to "
"\"\", \"@\", or []. In the next release, this will cause "
"these rules to be fully permissive as hardcoded enforcement "
"will be removed. To correct this issue, either stop "
"overriding these rules in config to accept the defaults, or "
"explicitly set check strings that are not empty." %
"\", \"".join(failed_rules)
)
return upgradecheck.Result(
upgradecheck.Code.SUCCESS, "\"identity:list_trusts\" policy is safe")
upgradecheck.Code.SUCCESS, 'Trust policies are safe.')
_upgrade_checks = (
("Check trust policies are not empty",

2
keystone/common/policies/trust.py
View File

@ -75,7 +75,7 @@ trust_policies = [
'method': 'HEAD'}]),
policy.DocumentedRuleDefault(
name=base.IDENTITY % 'delete_trust',
check_str='',
check_str=RULE_TRUSTOR,
scope_types=['project'],
description='Revoke trust.',
operations=[{'path': '/v3/OS-TRUST/trusts/{trust_id}',

51
keystone/tests/unit/protection/v3/test_trusts.py
View File

@ -109,6 +109,7 @@ class TrustTests(base_classes.TestCaseWithBootstrap,
with open(self.policy_file_name, 'w') as f:
overridden_policies = {
'identity:list_trusts': '',
'identity:delete_trust': '',
}
f.write(jsonutils.dumps(overridden_policies))
@ -258,6 +259,19 @@ class SystemAdminTests(TrustTests, _AdminTestsMixin):
)
self.assertEqual(1, len(r.json['trusts']))
def test_admin_cannot_delete_trust_for_user_overridden_defaults(self):
# only the is_admin admin can do this
self._override_policy_old_defaults()
ref = PROVIDERS.trust_api.create_trust(
self.trust_id, **self.trust_data)
with self.test_client() as c:
c.delete(
'/v3/OS-TRUST/trusts/%s' % ref['id'],
headers=self.headers,
expected_status_code=http_client.FORBIDDEN
)
class ProjectUserTests(TrustTests):
"""Tests for all project users."""
@ -573,7 +587,7 @@ class ProjectUserTests(TrustTests):
expected_status_code=http_client.FORBIDDEN
)
def test_user_cannot_list_trusts_for_other_trustee_overridden_default(self):
def test_user_cannot_list_trusts_for_trustee_overridden_default(self):
self._override_policy_old_defaults()
PROVIDERS.trust_api.create_trust(
self.trust_id, **self.trust_data)
@ -597,3 +611,38 @@ class ProjectUserTests(TrustTests):
headers=self.trustee_headers,
expected_status_code=http_client.FORBIDDEN
)
def test_trustor_can_delete_trust_overridden_default(self):
self._override_policy_old_defaults()
ref = PROVIDERS.trust_api.create_trust(
self.trust_id, **self.trust_data)
with self.test_client() as c:
c.delete(
'/v3/OS-TRUST/trusts/%s' % ref['id'],
headers=self.trustor_headers
)
def test_trustee_cannot_delete_trust_overridden_default(self):
self._override_policy_old_defaults()
ref = PROVIDERS.trust_api.create_trust(
self.trust_id, **self.trust_data)
with self.test_client() as c:
c.delete(
'/v3/OS-TRUST/trusts/%s' % ref['id'],
headers=self.trustee_headers,
expected_status_code=http_client.FORBIDDEN
)
def test_user_cannot_delete_trust_for_other_user_overridden_default(self):
self._override_policy_old_defaults()
ref = PROVIDERS.trust_api.create_trust(
self.trust_id, **self.trust_data)
with self.test_client() as c:
c.delete(
'/v3/OS-TRUST/trusts/%s' % ref['id'],
headers=self.other_headers,
expected_status_code=http_client.FORBIDDEN
)

6
keystone/tests/unit/test_cli.py
View File

@ -1865,14 +1865,16 @@ class CliStatusTestCase(unit.SQLDriverOverrides, unit.TestCase):
def test_check_safe_trust_policies(self):
with open(self.policy_file_name, 'w') as f:
overridden_policies = {
'identity:list_trusts': ''
'identity:list_trusts': '',
'identity:delete_trust': ''
}
f.write(jsonutils.dumps(overridden_policies))
result = self.checks.check_trust_policies_are_not_empty()
self.assertEqual(upgradecheck.Code.FAILURE, result.code)
with open(self.policy_file_name, 'w') as f:
overridden_policies = {
'identity:list_trusts': 'rule:admin_required'
'identity:list_trusts': 'rule:admin_required',
'identity:delete_trust': 'rule:admin_required'
}
f.write(jsonutils.dumps(overridden_policies))
result = self.checks.check_trust_policies_are_not_empty()