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
This commit is contained in:
“Richard
Ron De Rose
parent
477189d0c5
commit
9e84371461
@ -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
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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):
|
||||
|
||||
@ -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():
|
||||
|
||||
12
releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml
Normal file
12
releasenotes/notes/bug-1524030-ccff6b0ec9d1cbf2.yaml
Normal file
@ -0,0 +1,12 @@
|
||||
---
|
||||
fixes:
|
||||
- >
|
||||
[`bug 1524030 <https://bugs.launchpad.net/keystone/+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.
|
||||
Loading…
x
Reference in New Issue
Block a user