From d2cc4c83c00256440eed0ed5b8d7131cc02d72d4 Mon Sep 17 00:00:00 2001 From: Lance Bragstad Date: Thu, 6 Dec 2018 21:14:44 +0000 Subject: [PATCH] Consolidate user protection tests This commit removes user policies from policy.v3cloudsample.json. By incorporating system-scope, domain-scope, project-scope, and default roles, we've effectively made these policies obsolete. We can simplify what we maintain and provide a more consistent, unified view of default user behavior by removing them. This commit also adds an important filter to the GET /v3/users API by making sure the users in the response are filtered properly if the API was called with a domain-scoped token. This is needed in case domain configuration isn't setup and short-circuits normalization of the domain ID, which sometimes comes from the token if it is domain-scoped. Regardless of domain configuration being used, we should protect against cases where data leaks across domains in the name of security. Finally, this commit moves a couple of tests from test_v3_protection to test_users protection tests that ensures we do reasonable filtering while normalizing domain IDs. The remaining tests from test_v3_protection have been removed because they are no longer applicable. These tests were testing an HTTP 403 was returned when a domain users attempted to filter users for domains they didn't have authorization on. We don't use this approach consistently in keystone. Most other places where filtering is implemented, we ignore invalid filters and instead return an empty list. For domain users attempting to fish information out of another domain, they will receive an empty list to be consistent with other parts of the API. Change-Id: I60b2e2b8af172c369eab0eb2c29f056f5c98ad16 Parial-Bug: 1806762 --- etc/policy.v3cloudsample.json | 7 - keystone/api/users.py | 15 ++- .../tests/unit/protection/v3/test_users.py | 42 ++++++ keystone/tests/unit/test_policy.py | 7 +- keystone/tests/unit/test_v3_protection.py | 126 ------------------ .../notes/bug-1806762-09f414995924db23.yaml | 15 +++ 6 files changed, 77 insertions(+), 135 deletions(-) create mode 100644 releasenotes/notes/bug-1806762-09f414995924db23.yaml diff --git a/etc/policy.v3cloudsample.json b/etc/policy.v3cloudsample.json index 2366de76c6..fc912bcbb2 100644 --- a/etc/policy.v3cloudsample.json +++ b/etc/policy.v3cloudsample.json @@ -25,13 +25,6 @@ "identity:update_project_tags": "rule:admin_required", "admin_and_matching_target_user_domain_id": "rule:admin_required and domain_id:%(target.user.domain_id)s", - "admin_and_matching_user_domain_id": "rule:admin_required and domain_id:%(user.domain_id)s", - "identity:get_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id or rule:owner", - "identity:list_users": "rule:cloud_admin or rule:admin_and_matching_domain_id", - "identity:create_user": "rule:cloud_admin or rule:admin_and_matching_user_domain_id", - "identity:update_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id", - "identity:delete_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id", - "admin_and_matching_target_group_domain_id": "rule:admin_required and domain_id:%(target.group.domain_id)s", "admin_and_matching_group_domain_id": "rule:admin_required and domain_id:%(group.domain_id)s", "identity:get_group": "rule:cloud_admin or rule:admin_and_matching_target_group_domain_id", diff --git a/keystone/api/users.py b/keystone/api/users.py index 7009c87146..cf2bf7a405 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -174,7 +174,20 @@ class UserResource(ks_flask.ResourceBase): domain = self.oslo_context.domain_id refs = PROVIDERS.identity_api.list_users( domain_scope=domain, hints=hints) - return self.wrap_collection(refs, hints=hints) + + # If the user making the request used a domain-scoped token, let's make + # sure we filter out users that are not in that domain. Otherwise, we'd + # be exposing users in other domains. This if statement is needed in + # case _get_domain_id_for_list_request() short-circuits due to + # configuration and protects against information from other domains + # leaking to people who shouldn't see it. + if self.oslo_context.domain_id: + domain_id = self.oslo_context.domain_id + users = [user for user in refs if user['domain_id'] == domain_id] + else: + users = refs + + return self.wrap_collection(users, hints=hints) def post(self): """Create a user. diff --git a/keystone/tests/unit/protection/v3/test_users.py b/keystone/tests/unit/protection/v3/test_users.py index ae5317efce..f73b2d57a0 100644 --- a/keystone/tests/unit/protection/v3/test_users.py +++ b/keystone/tests/unit/protection/v3/test_users.py @@ -746,6 +746,28 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, with self.test_client() as c: c.post('/v3/users', json=create, headers=self.headers) + def test_user_cannot_create_users_within_domain_hyphened_domain_id(self): + # Finally, show that we can create a new user without any surprises. + # But if we specify a 'domain-id' instead of a 'domain_id', we get a + # Forbidden response because we fail a policy check before + # normalization occurs. + domain = PROVIDERS.resource_api.create_domain( + uuid.uuid4().hex, unit.new_domain_ref() + ) + + create = { + 'user': { + 'domain-id': domain['id'], + 'name': uuid.uuid4().hex + } + } + + with self.test_client() as c: + c.post( + '/v3/users', json=create, headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + def test_user_cannot_create_users_in_other_domain(self): domain = PROVIDERS.resource_api.create_domain( uuid.uuid4().hex, unit.new_domain_ref() @@ -775,6 +797,26 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, '/v3/users/%s' % user['id'], json=update, headers=self.headers ) + def test_user_can_update_users_within_domain_hyphened_domain_id(self): + # If we try updating the user's 'domain_id' by specifying a + # 'domain-id', then it'll be stored into extras rather than normalized, + # and the user's actual 'domain_id' is not affected. + domain = PROVIDERS.resource_api.create_domain( + uuid.uuid4().hex, unit.new_domain_ref() + ) + + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + + update = {'user': {'domain-id': domain['id']}} + with self.test_client() as c: + r = c.patch( + '/v3/users/%s' % user['id'], json=update, headers=self.headers + ) + self.assertEqual(domain['id'], r.json['user']['domain-id']) + self.assertEqual(self.domain_id, r.json['user']['domain_id']) + def test_user_cannot_update_users_in_other_domain(self): domain = PROVIDERS.resource_api.create_domain( uuid.uuid4().hex, unit.new_domain_ref() diff --git a/keystone/tests/unit/test_policy.py b/keystone/tests/unit/test_policy.py index a7d7259fa5..cc0a897492 100644 --- a/keystone/tests/unit/test_policy.py +++ b/keystone/tests/unit/test_policy.py @@ -196,6 +196,7 @@ class PolicyJsonTestCase(unit.TestCase): 'identity:list_service_providers', 'identity:update_service_provider', 'identity:delete_service_provider', + 'identity:list_role_assignments', 'identity:create_role', 'identity:get_role', 'identity:list_roles', @@ -250,7 +251,11 @@ class PolicyJsonTestCase(unit.TestCase): 'identity:list_services', 'identity:update_service', 'identity:delete_service', - 'identity:list_role_assignments' + 'identity:create_user', + 'identity:get_user', + 'identity:list_users', + 'identity:update_user', + 'identity:delete_user' ] policy_keys = self._get_default_policy_rules() for p in removed_policies: diff --git a/keystone/tests/unit/test_v3_protection.py b/keystone/tests/unit/test_v3_protection.py index 5e430683cd..0ee91c7144 100644 --- a/keystone/tests/unit/test_v3_protection.py +++ b/keystone/tests/unit/test_v3_protection.py @@ -793,25 +793,6 @@ class IdentityTestv3CloudPolicySample(test_v3.RestfulTestCase, else: return (expected_status, expected_status, expected_status) - def _test_user_management(self, user_id, domain_id, expected=None): - status_OK, status_created, status_no_data = self._stati(expected) - entity_url = '/users/%s' % user_id - list_url = '/users?domain_id=%s' % domain_id - - self.get(entity_url, auth=self.auth, - expected_status=status_OK) - self.get(list_url, auth=self.auth, - expected_status=status_OK) - user = {'description': 'Updated'} - self.patch(entity_url, auth=self.auth, body={'user': user}, - expected_status=status_OK) - self.delete(entity_url, auth=self.auth, - expected_status=status_no_data) - - user_ref = unit.new_user_ref(domain_id=domain_id) - self.post('/users', auth=self.auth, body={'user': user_ref}, - expected_status=status_created) - def _test_group_management(self, group, expected=None): status_OK, status_created, status_no_data = self._stati(expected) entity_url = '/groups/%s' % group['id'] @@ -938,113 +919,6 @@ class IdentityTestv3CloudPolicySample(test_v3.RestfulTestCase, self.post('/roles', auth=self.auth, body={'role': role_ref}, expected_status=status_created) - def test_user_management(self): - # First, authenticate with a user that does not have the domain - # admin role - shouldn't be able to do much. - self.auth = self.build_authentication_request( - user_id=self.just_a_user['id'], - password=self.just_a_user['password'], - domain_id=self.domainA['id']) - - self._test_user_management( - self.domain_admin_user['id'], self.domainA['id'], - expected=exception.ForbiddenAction.code) - - # Now, authenticate with a user that does have the domain admin role - self.auth = self.build_authentication_request( - user_id=self.domain_admin_user['id'], - password=self.domain_admin_user['password'], - domain_id=self.domainA['id']) - - self._test_user_management(self.just_a_user['id'], self.domainA['id']) - - def test_user_management_normalized_keys(self): - """Illustrate the inconsistent handling of hyphens in keys. - - To quote Morgan in bug 1526244: - - the reason this is converted from "domain-id" to "domain_id" is - because of how we process/normalize data. The way we have to handle - specific data types for known columns requires avoiding "-" in the - actual python code since "-" is not valid for attributes in python - w/o significant use of "getattr" etc. - - In short, historically we handle some things in conversions. The - use of "extras" has long been a poor design choice that leads to - odd/strange inconsistent behaviors because of other choices made in - handling data from within the body. (In many cases we convert from - "-" to "_" throughout openstack) - - Source: https://bugs.launchpad.net/keystone/+bug/1526244/comments/9 - - """ - # Authenticate with a user that has the domain admin role - self.auth = self.build_authentication_request( - user_id=self.domain_admin_user['id'], - password=self.domain_admin_user['password'], - domain_id=self.domainA['id']) - - # Show that we can read a normal user without any surprises. - r = self.get( - '/users/%s' % self.just_a_user['id'], - auth=self.auth, - expected_status=http_client.OK) - self.assertValidUserResponse(r) - - # We don't normalize query string keys, so both of these result in a - # 403, because we didn't specify a domain_id query string in either - # case, and we explicitly require one (it doesn't matter what - # 'domain-id' value you use). - self.get( - '/users?domain-id=%s' % self.domainA['id'], - auth=self.auth, - expected_status=exception.ForbiddenAction.code) - self.get( - '/users?domain-id=%s' % self.domainB['id'], - auth=self.auth, - expected_status=exception.ForbiddenAction.code) - - # If we try updating the user's 'domain_id' by specifying a - # 'domain-id', then it'll be stored into extras rather than normalized, - # and the user's actual 'domain_id' is not affected. - r = self.patch( - '/users/%s' % self.just_a_user['id'], - auth=self.auth, - body={'user': {'domain-id': self.domainB['id']}}, - expected_status=http_client.OK) - self.assertEqual(self.domainB['id'], r.json['user']['domain-id']) - self.assertEqual(self.domainA['id'], r.json['user']['domain_id']) - self.assertNotEqual(self.domainB['id'], self.just_a_user['domain_id']) - self.assertValidUserResponse(r, self.just_a_user) - - # Finally, show that we can create a new user without any surprises. - # But if we specify a 'domain-id' instead of a 'domain_id', we get a - # Forbidden response because we fail a policy check before - # normalization occurs. - user_ref = unit.new_user_ref(domain_id=self.domainA['id']) - r = self.post( - '/users', - auth=self.auth, - body={'user': user_ref}, - expected_status=http_client.CREATED) - self.assertValidUserResponse(r, ref=user_ref) - user_ref['domain-id'] = user_ref.pop('domain_id') - self.post( - '/users', - auth=self.auth, - body={'user': user_ref}, - expected_status=exception.ForbiddenAction.code) - - def test_user_management_by_cloud_admin(self): - # Test users management with a cloud admin. This user should - # be able to manage users in any domain. - self.auth = self.build_authentication_request( - user_id=self.cloud_admin_user['id'], - password=self.cloud_admin_user['password'], - project_id=self.admin_project['id']) - - self._test_user_management(self.just_a_user['id'], self.domainA['id']) - def test_group_management(self): # First, authenticate with a user that does not have the domain # admin role - shouldn't be able to do much. diff --git a/releasenotes/notes/bug-1806762-09f414995924db23.yaml b/releasenotes/notes/bug-1806762-09f414995924db23.yaml new file mode 100644 index 0000000000..891c9baec9 --- /dev/null +++ b/releasenotes/notes/bug-1806762-09f414995924db23.yaml @@ -0,0 +1,15 @@ +--- +upgrade: + - | + [`bug 1806762 `_] + The user policies defined in ``policy.v3cloudsample.json`` have + been removed. These policies are now obsolete after incorporating + system-scope, domain-scope, and project-scope into the user API + and implementing default roles. +fixes: + - | + [`bug 1806762 `_] + The user policies in ``policy.v3cloudsample.json`` policy file + have been removed in favor of better defaults in code. These + policies weren't tested exhaustively and were misleading to users + and operators.