From ef48072d94f780ebaacee8c3ddf02a68193fa74d Mon Sep 17 00:00:00 2001 From: Steve Martinelli Date: Thu, 15 Dec 2016 17:48:16 -0800 Subject: [PATCH] Fix cloud_admin rule and ensure only project tokens can be cloud admin The current rule fails to load with oslo.policy, the correct value used to determine the admin project for the cloud_admin should simply be: `is_admin_project:True`, since that is what is stored in oslo.context. This problem was masking a more serious issue that domain admin tokens could be misinterpreted as cloud admin tokens. Change-Id: I3ea562c01e06e6c519fdaec3ab6e1dac204ced71 Closes-Bug: 1547684 Closes-Bug: 1651989 --- etc/policy.v3cloudsample.json | 2 +- keystone/models/token_model.py | 6 ++++ keystone/tests/unit/test_policy.py | 4 +-- keystone/tests/unit/token/test_token_model.py | 11 +++++-- .../notes/bug-1547684-911aed68a0d3df17.yaml | 29 +++++++++++++++++++ 5 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/bug-1547684-911aed68a0d3df17.yaml diff --git a/etc/policy.v3cloudsample.json b/etc/policy.v3cloudsample.json index 9c3eada9f3..d67a354556 100644 --- a/etc/policy.v3cloudsample.json +++ b/etc/policy.v3cloudsample.json @@ -1,6 +1,6 @@ { "admin_required": "role:admin", - "cloud_admin": "role:admin and (token.is_admin_project:True or domain_id:admin_domain_id)", + "cloud_admin": "role:admin and (is_admin_project:True or domain_id:admin_domain_id)", "service_role": "role:service", "service_or_admin": "rule:admin_required or rule:service_role", "owner" : "user_id:%(user_id)s or user_id:%(target.token.user_id)s", diff --git a/keystone/models/token_model.py b/keystone/models/token_model.py index 724ea4b0c0..2cda094227 100644 --- a/keystone/models/token_model.py +++ b/keystone/models/token_model.py @@ -194,7 +194,13 @@ class KeystoneToken(dict): @property def is_admin_project(self): + if self.domain_scoped: + # Currently, domain scoped tokens cannot act as is_admin_project + return False + # True gets returned by default for compatibility with older versions + # TODO(henry-nash): This seems inherently dangerous, and we should + # investigate how we can default this to False. return self.get('is_admin_project', True) @property diff --git a/keystone/tests/unit/test_policy.py b/keystone/tests/unit/test_policy.py index c3f6d6fc48..435c972516 100644 --- a/keystone/tests/unit/test_policy.py +++ b/keystone/tests/unit/test_policy.py @@ -209,8 +209,8 @@ class PolicyJsonTestCase(unit.TestCase): domain_policy = unit.dirs.etc('policy.v3cloudsample.json') enforcer = common_policy.Enforcer(CONF, policy_file=domain_policy) - self.assertRaises(TypeError, enforcer.enforce, - action, target, credentials) + result = enforcer.enforce(action, target, credentials) + self.assertTrue(result) def test_all_targets_documented(self): # All the targets in the sample policy file must be documented in diff --git a/keystone/tests/unit/token/test_token_model.py b/keystone/tests/unit/token/test_token_model.py index 7be2e86e6b..75b7a06201 100644 --- a/keystone/tests/unit/token/test_token_model.py +++ b/keystone/tests/unit/token/test_token_model.py @@ -72,6 +72,7 @@ class TestKeystoneTokenModel(core.TestCase): self.assertEqual( self.v3_sample_token['token']['OS-TRUST:trust']['trustee_user_id'], token_data.trustee_user_id) + # Project Scoped Token self.assertRaises(exception.UnexpectedError, getattr, token_data, 'domain_id') @@ -85,12 +86,18 @@ class TestKeystoneTokenModel(core.TestCase): self.assertTrue(token_data.project_scoped) self.assertTrue(token_data.scoped) self.assertTrue(token_data.trust_scoped) + + # by default admin project is True for project scoped tokens + self.assertTrue(token_data.is_admin_project) + self.assertEqual( [r['id'] for r in self.v3_sample_token['token']['roles']], token_data.role_ids) self.assertEqual( [r['name'] for r in self.v3_sample_token['token']['roles']], token_data.role_names) + + # Domain Scoped Token token_data.pop('project') self.assertFalse(token_data.project_scoped) self.assertFalse(token_data.scoped) @@ -119,8 +126,8 @@ class TestKeystoneTokenModel(core.TestCase): self.assertIsNone(token_data.audit_id) self.assertIsNone(token_data.audit_chain_id) - # by default admin project is True - self.assertTrue(token_data.is_admin_project) + # by default admin project is False for domain scoped tokens + self.assertFalse(token_data.is_admin_project) def test_token_model_v3_federated_user(self): token_data = token_model.KeystoneToken(token_id=uuid.uuid4().hex, diff --git a/releasenotes/notes/bug-1547684-911aed68a0d3df17.yaml b/releasenotes/notes/bug-1547684-911aed68a0d3df17.yaml new file mode 100644 index 0000000000..417c2cadbc --- /dev/null +++ b/releasenotes/notes/bug-1547684-911aed68a0d3df17.yaml @@ -0,0 +1,29 @@ +--- +fixes: + - | + [`bug 1651989 `_] + Due to ``bug 1547684``, when using the ``policy.v3cloudsample.json`` + sample file, a domain admin token was being treated as a cloud admin. + Since the ``is_admin_project`` functionality only supports project- + scoped tokens, we automatically set any domain scoped token to have + the property ``is_admin_project`` to ``False``. + + [`bug 1547684 `_] + A typo in the ``policy.v3cloudsample.json`` sample file was causing + `oslo.policy` to not load the file. See the ``upgrades`` section for + more details. +upgrade: + - | + [`bug 1547684 `_] + A minor change to the ``policy.v3cloudsample.json`` sample file was + performed so the sample file loads correctly. The ``cloud_admin`` + rule has changed from:: + + "role:admin and (token.is_admin_project:True or domain_id:admin_domain_id)" + + To the properly written:: + + "role:admin and (is_admin_project:True or domain_id:admin_domain_id)" + + Adjust configuration tools as necessary, see the ``fixes`` section for more + details on this change.