From efb6b3fca0ba0ad768b3e803a324043095d326e2 Mon Sep 17 00:00:00 2001 From: Dolph Mathews Date: Fri, 7 Sep 2012 14:35:21 -0500 Subject: [PATCH] Delete user tokens after role grant/revoke Delete user tokens when a new role is granted or revoked, in order to prevent old tokens to continue to be valid for the original set of roles for the remainder of the token's lifespan. Addresses CVE-2012-4413. Fixes bug 1041396. Change-Id: Iecf891f274b67408f568b949a7028362c4c30312 --- keystone/identity/core.py | 7 ++++++- keystone/token/core.py | 11 +++++++++++ tests/test_keystoneclient.py | 18 +++++++++--------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/keystone/identity/core.py b/keystone/identity/core.py index c9c496c2c6..085a3dd1fe 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -590,6 +590,8 @@ def add_role_to_user(self, context, user_id, role_id, tenant_id=None): self.identity_api.add_user_to_tenant(context, tenant_id, user_id) self.identity_api.add_role_to_user_and_tenant( context, user_id, tenant_id, role_id) + self.token_api.revoke_tokens(context, user_id) + role_ref = self.identity_api.get_role(context, role_id) return {'role': role_ref} @@ -614,7 +616,7 @@ def remove_role_from_user(self, context, user_id, role_id, tenant_id=None): if not roles: self.identity_api.remove_user_from_tenant( context, tenant_id, user_id) - return + self.token_api.revoke_tokens(context, user_id) # COMPAT(diablo): CRUD extension def get_role_refs(self, context, user_id): @@ -657,6 +659,8 @@ def create_role_ref(self, context, user_id, role): self.identity_api.add_user_to_tenant(context, tenant_id, user_id) self.identity_api.add_role_to_user_and_tenant( context, user_id, tenant_id, role_id) + self.token_api.revoke_tokens(context, user_id) + role_ref = self.identity_api.get_role(context, role_id) return {'role': role_ref} @@ -684,3 +688,4 @@ def delete_role_ref(self, context, user_id, role_ref_id): if not roles: self.identity_api.remove_user_from_tenant( context, tenant_id, user_id) + self.token_api.revoke_tokens(context, user_id) diff --git a/keystone/token/core.py b/keystone/token/core.py index d6e9d38dd9..d3c1b67a0a 100644 --- a/keystone/token/core.py +++ b/keystone/token/core.py @@ -39,6 +39,10 @@ class Manager(manager.Manager): def __init__(self): super(Manager, self).__init__(CONF.token.driver) + def revoke_tokens(self, context, user_id): + for token_id in self.list_tokens(context, user_id): + self.delete_token(context, token_id) + class Driver(object): """Interface description for a Token driver.""" @@ -106,6 +110,13 @@ def list_revoked_tokens(self): """ raise exception.NotImplemented() + def revoke_tokens(self, user_id): + """Invalidates all tokens held by a user. + + :raises: keystone.exception.UserNotFound + """ + raise exception.NotImplemented() + def _get_default_expire_time(self): """Determine when a token should expire based on the config. diff --git a/tests/test_keystoneclient.py b/tests/test_keystoneclient.py index 944ca790fe..f42c3e70a1 100644 --- a/tests/test_keystoneclient.py +++ b/tests/test_keystoneclient.py @@ -794,15 +794,15 @@ def get_checkout(self): def test_tenant_add_and_remove_user(self): client = self.get_client(admin=True) client.roles.add_user_role(tenant=self.tenant_baz['id'], - user=self.user_foo['id'], + user=self.user_two['id'], role=self.role_useless['id']) user_refs = client.tenants.list_users(tenant=self.tenant_baz['id']) - self.assert_(self.user_foo['id'] in [x.id for x in user_refs]) + self.assert_(self.user_two['id'] in [x.id for x in user_refs]) client.roles.remove_user_role(tenant=self.tenant_baz['id'], - user=self.user_foo['id'], + user=self.user_two['id'], role=self.role_useless['id']) user_refs = client.tenants.list_users(tenant=self.tenant_baz['id']) - self.assert_(self.user_foo['id'] not in [x.id for x in user_refs]) + self.assert_(self.user_two['id'] not in [x.id for x in user_refs]) def test_user_role_add_404(self): from keystoneclient import exceptions as client_exceptions @@ -1001,16 +1001,16 @@ def get_checkout(self): def test_tenant_add_and_remove_user(self): client = self.get_client(admin=True) client.roles.add_user_to_tenant(tenant_id=self.tenant_baz['id'], - user_id=self.user_foo['id'], + user_id=self.user_two['id'], role_id=self.role_useless['id']) role_refs = client.roles.get_user_role_refs( - user_id=self.user_foo['id']) + user_id=self.user_two['id']) self.assert_(self.tenant_baz['id'] in [x.tenantId for x in role_refs]) # get the "role_refs" so we get the proper id, this is how the clients # do it roleref_refs = client.roles.get_user_role_refs( - user_id=self.user_foo['id']) + user_id=self.user_two['id']) for roleref_ref in roleref_refs: if (roleref_ref.roleId == self.role_useless['id'] and roleref_ref.tenantId == self.tenant_baz['id']): @@ -1018,11 +1018,11 @@ def test_tenant_add_and_remove_user(self): break client.roles.remove_user_from_tenant(tenant_id=self.tenant_baz['id'], - user_id=self.user_foo['id'], + user_id=self.user_two['id'], role_id=roleref_ref.id) role_refs = client.roles.get_user_role_refs( - user_id=self.user_foo['id']) + user_id=self.user_two['id']) self.assert_(self.tenant_baz['id'] not in [x.tenantId for x in role_refs])