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
This commit is contained in:
Lance Bragstad 2018-12-06 21:14:44 +00:00
parent e7a64ed179
commit d2cc4c83c0
6 changed files with 77 additions and 135 deletions

View File

@ -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",

View File

@ -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.

View File

@ -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()

View File

@ -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:

View File

@ -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.

View File

@ -0,0 +1,15 @@
---
upgrade:
- |
[`bug 1806762 <https://bugs.launchpad.net/keystone/+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 <https://bugs.launchpad.net/keystone/+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.