diff options
author | Zuul <zuul@review.opendev.org> | 2021-01-11 00:11:23 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2021-01-11 00:11:23 +0000 |
commit | 1c3131c6dcd10a4e4e82f86a55f26d2498799b6d (patch) | |
tree | 173b932adee37b14f961bd9320f8237e96758e63 | |
parent | 9ef800f0bc2593e6025aef00216648c21255ae40 (diff) | |
parent | 2d7bf10a5a145ed3b6e34c3cb95e05fb7e33e0d1 (diff) | |
download | keystone-1c3131c6dcd10a4e4e82f86a55f26d2498799b6d.tar.gz |
Merge "Use app cred user ID in policy enforcement"
-rw-r--r-- | keystone/api/users.py | 47 | ||||
-rw-r--r-- | keystone/tests/protection/v3/test_application_credential.py | 66 | ||||
-rw-r--r-- | releasenotes/notes/bug-1901207-13762f85b8a04481.yaml | 7 |
3 files changed, 118 insertions, 2 deletions
diff --git a/keystone/api/users.py b/keystone/api/users.py index f7f1ec0cf..10f26bd42 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -121,6 +121,41 @@ def _build_enforcer_target_data_owner_and_user_id_match(): return ref +def _update_request_user_id_attribute(): + # This method handles a special case in policy enforcement. The application + # credential API is underneath the user path (e.g., + # /v3/users/{user_id}/application_credentials/{application_credential_id}). + # The RBAC enforcer thinks the user to evaluate for application credential + # ownership comes from the path, but it should come from the actual + # application credential reference. By ensuring we pull the user ID from + # the application credential, we close a loop hole where users could + # effectively bypass authorization to view or delete any application + # credential in the system, assuming the attacker knows the application + # credential ID of another user. So long as the attacker matches the user + # ID in the request path to the user in the token of the request, they can + # pass the `rule:owner` policy check. This method protects against that by + # ensuring we use the application credential user ID and not something + # determined from the client. + try: + app_cred = ( + PROVIDERS.application_credential_api.get_application_credential( + flask.request.view_args.get('application_credential_id') + ) + ) + flask.request.view_args['user_id'] = app_cred['user_id'] + + # This target isn't really used in the default policy for application + # credentials, but we return it since we're using this method as a hook + # to update the flask request variables, which are used later in the + # keystone RBAC enforcer to populate the policy_dict, which ultimately + # turns into target attributes. + return {'user_id': app_cred['user_id']} + except ks_exception.NotFound: # nosec + # Defer existance in the event the application credential doesn't + # exist, we'll check this later anyway. + pass + + def _format_role_entity(role_id): role = PROVIDERS.role_api.get_role(role_id) formatted_entity = role.copy() @@ -652,7 +687,11 @@ class UserAppCredGetDeleteResource(ks_flask.ResourceBase): GET/HEAD /v3/users/{user_id}/application_credentials/ {application_credential_id} """ - ENFORCER.enforce_call(action='identity:get_application_credential') + target = _update_request_user_id_attribute() + ENFORCER.enforce_call( + action='identity:get_application_credential', + target_attr=target, + ) ref = PROVIDERS.application_credential_api.get_application_credential( application_credential_id) return self.wrap_member(ref) @@ -663,7 +702,11 @@ class UserAppCredGetDeleteResource(ks_flask.ResourceBase): DELETE /v3/users/{user_id}/application_credentials/ {application_credential_id} """ - ENFORCER.enforce_call(action='identity:delete_application_credential') + target = _update_request_user_id_attribute() + ENFORCER.enforce_call( + action='identity:delete_application_credential', + target_attr=target + ) token = self.auth_context['token'] _check_unrestricted_application_credential(token) PROVIDERS.application_credential_api.delete_application_credential( diff --git a/keystone/tests/protection/v3/test_application_credential.py b/keystone/tests/protection/v3/test_application_credential.py index 5807d7f90..5f7c2a202 100644 --- a/keystone/tests/protection/v3/test_application_credential.py +++ b/keystone/tests/protection/v3/test_application_credential.py @@ -418,6 +418,72 @@ class OwnerTests(_TestAppCredBase, def test_owner_can_delete_application_credential(self): self._test_delete_application_credential() + def test_user_cannot_lookup_application_credential_for_another_user(self): + # create another user + another_user = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id + ) + another_user_id = PROVIDERS.identity_api.create_user( + another_user + )['id'] + + auth = self.build_authentication_request( + user_id=another_user_id, + password=another_user['password'] + ) + + # authenticate for a token as a completely different user with + # completely different authorization + with self.test_client() as c: + r = c.post('/v3/auth/tokens', json=auth) + another_user_token = r.headers['X-Subject-Token'] + + # create an application credential as the self.user_id user on a + # project that the user above doesn't have any authorization on + app_cred = self._create_application_credential() + + # attempt to lookup the application credential as another user + with self.test_client() as c: + c.get( + '/v3/users/%s/application_credentials/%s' % ( + another_user_id, + app_cred['id']), + expected_status_code=http.client.FORBIDDEN, + headers={'X-Auth-Token': another_user_token}) + + def test_user_cannot_delete_application_credential_for_another_user(self): + # create another user + another_user = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id + ) + another_user_id = PROVIDERS.identity_api.create_user( + another_user + )['id'] + + auth = self.build_authentication_request( + user_id=another_user_id, + password=another_user['password'] + ) + + # authenticate for a token as a completely different user with + # completely different authorization + with self.test_client() as c: + r = c.post('/v3/auth/tokens', json=auth) + another_user_token = r.headers['X-Subject-Token'] + + # create an application credential as the self.user_id user on a + # project that the user above doesn't have any authorization on + app_cred = self._create_application_credential() + + # attempt to delete the application credential as another user + with self.test_client() as c: + c.delete( + '/v3/users/%s/application_credentials/%s' % ( + another_user_id, + app_cred['id']), + expected_status_code=http.client.FORBIDDEN, + headers={'X-Auth-Token': another_user_token}) + class DomainAdminTests(_TestAppCredBase, common_auth.AuthTestMixin, diff --git a/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml b/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml new file mode 100644 index 000000000..26e957a0d --- /dev/null +++ b/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml @@ -0,0 +1,7 @@ +--- +security: + - | + [`bug 1901207 <https://bugs.launchpad.net/keystone/+bug/1901207>`_] + Policy enforcement for application credentials has been updated to protect + against invalid ownership checks resulting in unauthorized users being able + to get and delete application credentials for other users. |