From 9e84371461831880ce5736e9888c7d9648e3a77b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CRichard?= Date: Tue, 4 Oct 2016 23:00:31 +0000 Subject: [PATCH] Improve check_token validation performance This patch improves check_token validation performance by only pulling revocation events based on the token issued_at value, taking advantage of the table index. In this way, only a subset of relevant events will be returned for validation. Benchmarks can be seen at [1], but included here as well: Time per Request for Old Method ------------------------------- 10 revokes at 7.908 100 revokes at 18.224 1,000 revokes at 110.155 10,000 revokes at 1998.220 Time per Request New Method --------------------------- 10 revokes at 17.636ms, 100 revokes at 17.279ms, 1,000 revokes at 17.370, 10,000 revokes w/all revokes issued before token at 17.263 (best case) 10,000 revokes w/all revokes after token creation 44.934ms (worst case) [1] https://gist.github.com/csrichard1/4b7b8527ee5a6565a84956cff33cf29b Change-Id: I9c2f067d870d542ec5909eaf8b24ded07b75f433 Partial-Bug: 1524030 --- keystone/revoke/backends/base.py | 5 +- keystone/revoke/backends/sql.py | 34 ++- keystone/revoke/core.py | 7 +- keystone/tests/unit/test_revoke.py | 209 ++++++++++++++++++ .../notes/bug-1524030-ccff6b0ec9d1cbf2.yaml | 12 + 5 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml diff --git a/keystone/revoke/backends/base.py b/keystone/revoke/backends/base.py index 949e1cc5e9..b77a4a851d 100644 --- a/keystone/revoke/backends/base.py +++ b/keystone/revoke/backends/base.py @@ -37,10 +37,13 @@ class RevokeDriverBase(object): """Interface for recording and reporting revocation events.""" @abc.abstractmethod - def list_events(self, last_fetch=None): + def list_events(self, last_fetch=None, token=None): """return the revocation events, as a list of objects. :param last_fetch: Time of last fetch. Return all events newer. + :param token: dictionary of values from a token, normalized for + differences between v2 and v3. The checked values are a + subset of the attributes of model.TokenEvent :returns: A list of keystone.revoke.model.RevokeEvent newer than `last_fetch.` If no last_fetch is specified, returns all events diff --git a/keystone/revoke/backends/sql.py b/keystone/revoke/backends/sql.py index 7620e37a1e..88aa38680c 100644 --- a/keystone/revoke/backends/sql.py +++ b/keystone/revoke/backends/sql.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import sqlalchemy + from keystone.common import sql from keystone.models import revoke_model from keystone.revoke.backends import base @@ -88,7 +90,30 @@ class Revoke(base.RevokeDriverBase): session.flush() - def list_events(self, last_fetch=None): + def _list_token_events(self, token): + with sql.session_for_read() as session: + query = session.query(RevocationEvent).filter( + RevocationEvent.issued_before >= token['issued_at']) + user = [RevocationEvent.user_id.is_(None)] + proj = [RevocationEvent.project_id.is_(None)] + audit = [RevocationEvent.audit_id.is_(None)] + if token['user_id']: + user.append(RevocationEvent.user_id == token['user_id']) + if token['trustor_id']: + user.append(RevocationEvent.user_id == token['trustor_id']) + if token['trustee_id']: + user.append(RevocationEvent.user_id == token['trustee_id']) + if token['project_id']: + proj.append(RevocationEvent.project_id == token['project_id']) + if token['audit_id']: + audit.append(RevocationEvent.audit_id == token['audit_id']) + query = query.filter(sqlalchemy.and_(sqlalchemy.or_(*user), + sqlalchemy.or_(*proj), + sqlalchemy.or_(*audit))) + events = [revoke_model.RevokeEvent(**e.to_dict()) for e in query] + return events + + def _list_last_fetch_events(self, last_fetch=None): with sql.session_for_read() as session: query = session.query(RevocationEvent).order_by( RevocationEvent.revoked_at) @@ -97,9 +122,14 @@ class Revoke(base.RevokeDriverBase): query = query.filter(RevocationEvent.revoked_at > last_fetch) events = [revoke_model.RevokeEvent(**e.to_dict()) for e in query] - return events + def list_events(self, last_fetch=None, token=None): + if token: + return self._list_token_events(token) + else: + return self._list_last_fetch_events(last_fetch) + @oslo_db_api.wrap_db_retry(retry_on_deadlock=True) def revoke(self, event): kwargs = dict() diff --git a/keystone/revoke/core.py b/keystone/revoke/core.py index 1e0d3c7d29..23a90fd573 100644 --- a/keystone/revoke/core.py +++ b/keystone/revoke/core.py @@ -190,17 +190,18 @@ class Manager(manager.Manager): self.revoke(revoke_model.RevokeEvent(domain_id=domain_id, role_id=role_id)) - def check_token(self, token_values): + def check_token(self, token): """Check the values from a token against the revocation list. - :param token_values: dictionary of values from a token, normalized for + :param token: dictionary of values from a token, normalized for differences between v2 and v3. The checked values are a subset of the attributes of model.TokenEvent :raises keystone.exception.TokenNotFound: If the token is invalid. """ - if revoke_model.is_revoked(self.list_events(), token_values): + if revoke_model.is_revoked(self.driver.list_events(token=token), + token): raise exception.TokenNotFound(_('Failed to validate token')) def revoke(self, event): diff --git a/keystone/tests/unit/test_revoke.py b/keystone/tests/unit/test_revoke.py index 224b7e5a0d..355bcdedaf 100644 --- a/keystone/tests/unit/test_revoke.py +++ b/keystone/tests/unit/test_revoke.py @@ -21,6 +21,7 @@ from six.moves import range from keystone.common import utils from keystone import exception from keystone.models import revoke_model +from keystone.revoke.backends import sql from keystone.tests import unit from keystone.tests.unit import test_backend_sql from keystone.token import provider @@ -113,6 +114,16 @@ def _matches(event, token_values): class RevokeTests(object): + def _assertTokenRevoked(self, events, token_data): + return self.assertTrue( + revoke_model.is_revoked(events, token_data), + 'Token should be revoked') + + def _assertTokenNotRevoked(self, events, token_data): + return self.assertFalse( + revoke_model.is_revoked(events, token_data), + 'Token should not be revoked') + def test_list(self): self.revoke_api.revoke_by_user(user_id=1) self.assertEqual(1, len(self.revoke_api.list_events())) @@ -129,6 +140,204 @@ class RevokeTests(object): self.assertEqual(0, len(self.revoke_api.list_events(last_fetch=future))) + def test_list_revoked_user(self): + revocation_backend = sql.Revoke() + events = [] + + # This simulates creating a token for a specific user. When we revoke + # the token we should have a single revocation event in the list. We + # are going to assert that the token values match the only revocation + # event in the backend. + first_token = _sample_blank_token() + first_token['user_id'] = uuid.uuid4().hex + add_event( + events, revoke_model.RevokeEvent(user_id=first_token['user_id']) + ) + self.revoke_api.revoke_by_user(user_id=first_token['user_id']) + self._assertTokenRevoked(events, first_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=first_token)) + ) + + # This simulates creating a separate token for a separate user. We are + # going to revoke the token just like we did for the previous token. + # We should have two revocation events stored in the backend but only + # one should match the values of the second token. + second_token = _sample_blank_token() + second_token['user_id'] = uuid.uuid4().hex + add_event( + events, revoke_model.RevokeEvent(user_id=second_token['user_id']) + ) + self.revoke_api.revoke_by_user(user_id=second_token['user_id']) + self._assertTokenRevoked(events, second_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=second_token)) + ) + # This simulates creating another separate token for a separate user, + # but we're not going to issue a revocation event. Even though we have + # two revocation events persisted in the backend, neither of them + # should match the values of the third token. If they did - our + # revocation event matching would be too heavy handed, which would + # result in over-generalized revocation patterns. + third_token = _sample_blank_token() + third_token['user_id'] = uuid.uuid4().hex + self._assertTokenNotRevoked(events, third_token) + self.assertEqual( + 0, len(revocation_backend.list_events(token=third_token)) + ) + # This gets a token but overrides the user_id of the token to be None. + # Technically this should never happen because tokens must belong to + # a user. What we're testing here is that the two revocation events + # we've created won't match None values for the user_id. + fourth_token = _sample_blank_token() + fourth_token['user_id'] = None + self._assertTokenNotRevoked(events, fourth_token) + self.assertEqual( + 0, len(revocation_backend.list_events(token=fourth_token)) + ) + + def test_list_revoked_project(self): + revocation_backend = sql.Revoke() + events = [] + token = _sample_blank_token() + + # Create a token for a project, revoke token, check the token we + # created has been revoked, and check the list returned a match for + # the token when passed in. + first_token = _sample_blank_token() + first_token['project_id'] = uuid.uuid4().hex + add_event(events, revoke_model.RevokeEvent( + project_id=first_token['project_id'])) + revocation_backend.revoke(revoke_model.RevokeEvent( + project_id=first_token['project_id'])) + self._assertTokenRevoked(events, first_token) + self.assertEqual(1, len(revocation_backend.list_events( + token=first_token))) + + # Create a second token, revoke it, check the token has been revoked, + # and check the list to make sure that even though we now have 2 + # revoked events in the revocation list, it will only return 1 because + # only one match for our second_token should exist + second_token = _sample_blank_token() + second_token['project_id'] = uuid.uuid4().hex + add_event(events, revoke_model.RevokeEvent( + project_id=second_token['project_id'])) + revocation_backend.revoke(revoke_model.RevokeEvent( + project_id=second_token['project_id'])) + self._assertTokenRevoked(events, second_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=second_token))) + + # This gets a token but overrides project_id of the token to be None. + # We expect that since there are two events which both have populated + # project_ids, this should not match this third_token with any other + # event in the list so we should recieve 0 + third_token = _sample_blank_token() + third_token['project_id'] = None + self._assertTokenNotRevoked(events, token) + self.assertEqual(0, len(revocation_backend.list_events(token=token))) + + def test_list_revoked_audit(self): + revocation_backend = sql.Revoke() + events = [] + + # Create a token with audit_id set, revoke it, check it is revoked, + # check to make sure that list_events matches the token to the event we + # just revoked. + first_token = _sample_blank_token() + first_token['audit_id'] = provider.random_urlsafe_str() + add_event(events, revoke_model.RevokeEvent( + audit_id=first_token['audit_id'])) + self.revoke_api.revoke_by_audit_id( + audit_id=first_token['audit_id']) + self._assertTokenRevoked(events, first_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=first_token))) + + # Create a second token, revoke it, check it is revoked, check to make + # sure that list events only finds 1 match since there are 2 and they + # dont both have different populated audit_id fields + second_token = _sample_blank_token() + second_token['audit_id'] = provider.random_urlsafe_str() + add_event(events, revoke_model.RevokeEvent( + audit_id=second_token['audit_id'])) + self.revoke_api.revoke_by_audit_id( + audit_id=second_token['audit_id']) + self._assertTokenRevoked(events, second_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=second_token))) + + # Create a third token with audit_id set to None to make sure that + # since there are no events currently revoked with audit_id None this + # finds no matches + third_token = _sample_blank_token() + third_token['audit_id'] = None + self._assertTokenNotRevoked(events, third_token) + self.assertEqual( + 0, len(revocation_backend.list_events(token=third_token))) + + def test_list_revoked_since(self): + revocation_backend = sql.Revoke() + token = _sample_blank_token() + self.revoke_api.revoke_by_user(user_id=None) + self.revoke_api.revoke_by_user(user_id=None) + self.assertEqual(2, len(revocation_backend.list_events(token=token))) + future = timeutils.utcnow() + datetime.timedelta(seconds=1000) + token['issued_at'] = future + self.assertEqual(0, len(revocation_backend.list_events(token=token))) + + def test_list_revoked_multiple_filters(self): + revocation_backend = sql.Revoke() + events = [] + + # create token that sets key/value filters in list_revoked + first_token = _sample_blank_token() + first_token['user_id'] = uuid.uuid4().hex + first_token['project_id'] = uuid.uuid4().hex + first_token['audit_id'] = provider.random_urlsafe_str() + # revoke event and then verify that that there is only one revocation + # and verify the only revoked event is the token + add_event(events, revoke_model.RevokeEvent( + user_id=first_token['user_id'], + project_id=first_token['project_id'], + audit_id=first_token['audit_id'])) + self.revoke_api.revoke(revoke_model.RevokeEvent( + user_id=first_token['user_id'], + project_id=first_token['project_id'], + audit_id=first_token['audit_id'])) + self._assertTokenRevoked(events, first_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=first_token))) + # If a token has None values which the event contains it shouldn't + # match and not be revoked + second_token = _sample_blank_token() + self._assertTokenNotRevoked(events, second_token) + self.assertEqual( + 0, len(revocation_backend.list_events(token=second_token))) + # If an event column and corresponding dict value don't match, Then + # it should not add the event in the list. Demonstrate for project + third_token = _sample_blank_token() + third_token['project_id'] = uuid.uuid4().hex + self._assertTokenNotRevoked(events, third_token) + self.assertEqual( + 0, len(revocation_backend.list_events(token=third_token))) + # A revoked event with user_id as null and token user_id non null + # should still be return an event and be revoked if other non null + # event fields match non null token fields + fourth_token = _sample_blank_token() + fourth_token['user_id'] = uuid.uuid4().hex + fourth_token['project_id'] = uuid.uuid4().hex + fourth_token['audit_id'] = provider.random_urlsafe_str() + add_event(events, revoke_model.RevokeEvent( + project_id=fourth_token['project_id'], + audit_id=fourth_token['audit_id'])) + self.revoke_api.revoke(revoke_model.RevokeEvent( + project_id=fourth_token['project_id'], + audit_id=fourth_token['audit_id'])) + self._assertTokenRevoked(events, fourth_token) + self.assertEqual( + 1, len(revocation_backend.list_events(token=fourth_token))) + @mock.patch.object(timeutils, 'utcnow') def test_expired_events_are_removed(self, mock_utcnow): def _sample_token_values(): diff --git a/releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml b/releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml new file mode 100644 index 0000000000..b1db04ffd4 --- /dev/null +++ b/releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - > + [`bug 1524030 `_] + During token validation we've reduced the number of revocation events + returned, only returning a subset of events relevant to the token. Thus, + improving overall token validation performance. +other: + - > + The revoke backend driver interface has changed. We've added a token + parameter to the ``list_events`` method in order to improve performance by + reducing the number of revocation events returned during token validation.