From f463bdccf130ad5e6bd2adb5fba785455477de00 Mon Sep 17 00:00:00 2001 From: Lance Bragstad Date: Mon, 8 Jan 2018 22:03:50 +0000 Subject: [PATCH] Validate identity providers during token validation Previously, it was possible to validate a federated keystone token after the identity provider associated by that token was deleted, which is a security concern. This commit does two things. First it makes it so that the token cache is invalidated when identity providers are deleted. Second, it validates the identity provider in the token data and ensures it actually exists in the system before considering the token valid. Change-Id: I57491c5a7d657b25cc436452acd7fcc4cd285839 Closes-Bug: 1291157 --- keystone/federation/core.py | 13 +++++++++++++ keystone/notifications.py | 1 + keystone/tests/unit/test_v3_federation.py | 12 +++++------- keystone/tests/unit/token/test_fernet_provider.py | 12 +++++++++--- keystone/token/provider.py | 15 +++++++++++++++ keystone/token/providers/common.py | 7 +++++++ .../notes/bug-1291157-00b5c714a097e84c.yaml | 7 +++++++ 7 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/bug-1291157-00b5c714a097e84c.yaml diff --git a/keystone/federation/core.py b/keystone/federation/core.py index 1b7c2e1612..677e783438 100644 --- a/keystone/federation/core.py +++ b/keystone/federation/core.py @@ -22,6 +22,7 @@ import keystone.conf from keystone import exception from keystone.federation import utils from keystone.i18n import _ +from keystone import notifications # This is a general cache region for service providers. @@ -77,6 +78,18 @@ class Manager(manager.Manager): self._cleanup_idp_domain(idp['domain_id']) raise + def delete_idp(self, idp_id): + self.driver.delete_idp(idp_id) + # NOTE(lbragstad): If an identity provider is removed from the system, + # then we need to invalidate the token cache. Otherwise it will be + # possible for federated tokens to be considered valid after a service + # provider removes a federated identity provider resource. The `idp_id` + # isn't actually used when invalidating the token cache but we have to + # pass something. + notifications.Audit.internal( + notifications.INVALIDATE_TOKEN_CACHE_DELETED_IDP, idp_id + ) + def _cleanup_idp_domain(self, domain_id): domain = {'enabled': False} PROVIDERS.resource_api.update_domain(domain_id, domain) diff --git a/keystone/notifications.py b/keystone/notifications.py index 7fc41ae040..40e7c31acb 100644 --- a/keystone/notifications.py +++ b/keystone/notifications.py @@ -79,6 +79,7 @@ CONF = keystone.conf.CONF INVALIDATE_USER_TOKEN_PERSISTENCE = 'invalidate_user_tokens' INVALIDATE_USER_PROJECT_TOKEN_PERSISTENCE = 'invalidate_user_project_tokens' INVALIDATE_USER_OAUTH_CONSUMER_TOKENS = 'invalidate_user_consumer_tokens' +INVALIDATE_TOKEN_CACHE_DELETED_IDP = 'invalidate_token_cache_from_deleted_idp' class Audit(object): diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py index 2ab0d6681d..f21dfe8fed 100644 --- a/keystone/tests/unit/test_v3_federation.py +++ b/keystone/tests/unit/test_v3_federation.py @@ -43,7 +43,6 @@ from keystone.tests.unit import federation_fixtures from keystone.tests.unit import ksfixtures from keystone.tests.unit import mapping_fixtures from keystone.tests.unit import test_v3 -from keystone.tests.unit import utils from keystone.token.providers import common as token_common @@ -2061,9 +2060,7 @@ class FederatedTokenTests(test_v3.RestfulTestCase, FederatedSetupMixin): self.TOKEN_SCOPE_PROJECT_EMPLOYEE_FROM_CUSTOMER, expected_status=http_client.FORBIDDEN) - @utils.wip('This will fail because of bug #1291157. The token should be ' - 'invalid after deleting the identity provider.') - def test_validate_token_after_deleting_idp_fails(self): + def test_validate_token_after_deleting_idp_raises_not_found(self): token = self.v3_create_token( self.TOKEN_SCOPE_PROJECT_EMPLOYEE_FROM_ADMIN ) @@ -2074,13 +2071,14 @@ class FederatedTokenTests(test_v3.RestfulTestCase, FederatedSetupMixin): headers = { 'X-Subject-Token': token_id } - # FIXME(lbragstad): This should raise a 401 Unauthorized exception - # since the identity provider is gone. + # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity + # provider is no longer present. We raise 404 NOT FOUND when we + # validate a token and a project or domain no longer exists. self.get( '/auth/tokens/', token=token_id, headers=headers, - expected_status=http_client.UNAUTHORIZED + expected_status=http_client.NOT_FOUND ) def test_scope_to_bad_project(self): diff --git a/keystone/tests/unit/token/test_fernet_provider.py b/keystone/tests/unit/token/test_fernet_provider.py index 7d4fd4257b..9ee253ed8a 100644 --- a/keystone/tests/unit/token/test_fernet_provider.py +++ b/keystone/tests/unit/token/test_fernet_provider.py @@ -135,13 +135,19 @@ class TestValidate(unit.TestCase): method_names = ['mapped'] group_ids = [uuid.uuid4().hex, ] - identity_provider = uuid.uuid4().hex + idp_id = uuid.uuid4().hex + idp_ref = { + 'id': idp_id, + 'description': uuid.uuid4().hex, + 'enabled': True + } + self.federation_api.create_idp(idp_id, idp_ref) protocol = uuid.uuid4().hex auth_context_params = { 'user_id': user_ref['id'], 'user_name': user_ref['name'], 'group_ids': group_ids, - federation_constants.IDENTITY_PROVIDER: identity_provider, + federation_constants.IDENTITY_PROVIDER: idp_id, federation_constants.PROTOCOL: protocol, } auth_context = auth.core.AuthContext(**auth_context_params) @@ -157,7 +163,7 @@ class TestValidate(unit.TestCase): 'name': CONF.federation.federated_domain_name, }, federation_constants.FEDERATION: { 'groups': [{'id': group_id} for group_id in group_ids], - 'identity_provider': {'id': identity_provider, }, + 'identity_provider': {'id': idp_id, }, 'protocol': {'id': protocol, }, }, } diff --git a/keystone/token/provider.py b/keystone/token/provider.py index d9d81ee2e0..67cffe15f6 100644 --- a/keystone/token/provider.py +++ b/keystone/token/provider.py @@ -91,6 +91,8 @@ class Manager(manager.Manager): self._delete_user_project_tokens_callback], [notifications.INVALIDATE_USER_OAUTH_CONSUMER_TOKENS, self._delete_user_oauth_consumer_tokens_callback], + [notifications.INVALIDATE_TOKEN_CACHE_DELETED_IDP, + self._invalidate_token_cache_from_deleted_idp_callback], ] } @@ -329,3 +331,16 @@ class Manager(manager.Manager): if CONF.token.cache_on_issue: # NOTE(amakarov): preserving behavior TOKENS_REGION.invalidate() + + def _invalidate_token_cache_from_deleted_idp_callback(self, + service, + resource_type, + operation, payload): + """Callback to invalidate the token cache after deleting an idp. + + While this callback doesn't use the information about the deleted + identity provider to invalidate the cache, the method name and payload + are emitted when logging at the DEBUG level. + + """ + TOKENS_REGION.invalidate() diff --git a/keystone/token/providers/common.py b/keystone/token/providers/common.py index 740550401b..64f14dd583 100644 --- a/keystone/token/providers/common.py +++ b/keystone/token/providers/common.py @@ -462,6 +462,12 @@ class V3TokenDataHelper(provider_api.ProviderAPIMixin, object): if service_providers: token_data['service_providers'] = service_providers + def _validate_identity_provider(self, token_data): + federated_info = token_data['user'].get('OS-FEDERATION') + if federated_info: + idp_id = federated_info['identity_provider']['id'] + PROVIDERS.federation_api.get_idp(idp_id) + def _populate_token_dates(self, token_data, expires=None, issued_at=None): if not expires: expires = default_expire_time() @@ -518,6 +524,7 @@ class V3TokenDataHelper(provider_api.ProviderAPIMixin, object): token_data, user_id, system, domain_id, project_id, trust ) self._populate_service_providers(token_data) + self._validate_identity_provider(token_data) self._populate_token_dates(token_data, expires=expires, issued_at=issued_at) self._populate_oauth_section(token_data, access_token) diff --git a/releasenotes/notes/bug-1291157-00b5c714a097e84c.yaml b/releasenotes/notes/bug-1291157-00b5c714a097e84c.yaml new file mode 100644 index 0000000000..43414cfb1a --- /dev/null +++ b/releasenotes/notes/bug-1291157-00b5c714a097e84c.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + [`bug 1291157 `_] + Identity provider information is now validated in during token validation. + If an identity provider is removed from a keystone service provider, tokens + associated to that identity provider will be considered invalid.