Replace revoke tree with linear search

The RevokeTree was built out of an attempt to optimize the search for
a match between a candidate token and the list of revocation events.
The performance proved to be poor, mostly due to the cost of creating
and checking hash values.

The RevokeTree code is also so complex that most of the team could not
understand it or troubleshoot it.  There are some subtle bugs due to
race conditions with revocation events, and it is impossible to track
them down due to the code complexity.

This change replaces the tree based search with a linear search through
the list of revocation events.  A failure-to-match will pass through
the entire list.  A revoked token should match on O(n/2) comparisons.

With the past year of Fernet tokens in deployment, the feedback is that
the number of revocation events is small, and they only are kept for
the lifetime of the tokens (usually 1-8 hours) so the linear search is
not expected to slow down token validations in live deployments.

Future work will also reduce the number of revocation events.

Change-Id: Ib6a686494e897840b09d134ecf1ca50ce712f281

keystone/common/cache/_context_cache.py

@@ -16,34 +16,33 @@ from dogpile.cache import proxy
 from oslo_context import context as oslo_context
 from oslo_serialization import msgpackutils
 
+
 from keystone.models import revoke_model
 
 
-class _RevokeModelHandler(object):
+class _RevokeEventHandler(object):
     # NOTE(morganfainberg): There needs to be reserved "registry" entries set
     # in oslo_serialization for application-specific handlers. We picked 127
     # here since it's waaaaaay far out before oslo_serialization will use it.
     identity = 127
-    handles = (revoke_model.RevokeTree,)
+    handles = (revoke_model.RevokeEvent,)
 
     def __init__(self, registry):
         self._registry = registry
 
     def serialize(self, obj):
-        return msgpackutils.dumps(obj.revoke_map,
+        return msgpackutils.dumps(obj.__dict__, registry=self._registry)
-                                  registry=self._registry)
 
     def deserialize(self, data):
-        revoke_map = msgpackutils.loads(data, registry=self._registry)
+        revoke_event_data = msgpackutils.loads(data, registry=self._registry)
-        revoke_tree = revoke_model.RevokeTree()
+        revoke_event = revoke_model.RevokeEvent(**revoke_event_data)
-        revoke_tree.revoke_map = revoke_map
+        return revoke_event
-        return revoke_tree
 
 
 # Register our new handler.
 _registry = msgpackutils.default_registry
 _registry.frozen = False
-_registry.register(_RevokeModelHandler(registry=_registry))
+_registry.register(_RevokeEventHandler(registry=_registry))
 _registry.frozen = True
 
 

keystone/models/revoke_model.py

@@ -126,134 +126,86 @@ def attr_keys(event):
     return list(map(event.key_for_name, _EVENT_NAMES))
 
 
-class RevokeTree(object):
+def is_revoked(events, token_data):
-    """Fast Revocation Checking Tree Structure.
+    """Check if a token matches a revocation event.
 
-    The Tree is an index to quickly match tokens against events.
+    Compare a token against every revocation event. If the token matches an
-    Each node is a hashtable of key=value combinations from revocation events.
+    event in the `events` list, the token is revoked. If the token is compared
-    The
+    against every item in the list without a match, it is not considered
+    revoked from the `revoke_api`.
 
+    :param events: a list of RevokeEvent instances
+    :param token_data: map based on a flattened view of the token. The required
+                       fields are `expires_at`,`user_id`, `project_id`,
+                       `identity_domain_id`, `assignment_domain_id`,
+                       `trust_id`, `trustor_id`, `trustee_id` `consumer_id` and
+                       `access_token_id`
+    :returns: True if the token matches an existing revocation event, meaning
+              the token is revoked. False is returned if the token does not
+              match any revocation events, meaning the token is considered
+              valid by the revocation API.
     """
+    return any([matches(e, token_data) for e in events])
 
-    def __init__(self, revoke_events=None):
-        self.revoke_map = dict()
-        self.add_events(revoke_events)
 
-    def add_event(self, event):
+def matches(event, token_values):
-        """Update the tree based on a revocation event.
+    """See if the token matches the revocation event.
 
-        Creates any necessary internal nodes in the tree corresponding to the
+    A brute force approach to checking.
-        fields of the revocation event.  The leaf node will always be set to
+    Compare each attribute from the event with the corresponding
-        the latest 'issued_before' for events that are otherwise identical.
+    value from the token.  If the event does not have a value for
-
+    the attribute, a match is still possible.  If the event has a
-        :param:  Event to add to the tree
+    value for the attribute, and it does not match the token, no match
-
+    is possible, so skip the remaining checks.
-        :returns:  the event that was passed in.
 
+    :param event: a RevokeEvent instance
+    :param token_values: dictionary with set of values taken from the
+                         token
+    :returns: True if the token matches the revocation event, indicating the
+              token has been revoked
     """
-        revoke_map = self.revoke_map
+    # If any one check does not match, the whole token does
-        for key in attr_keys(event):
+    # not match the event. The numerous return False indicate
-            revoke_map = revoke_map.setdefault(key, {})
+    # that the token is still valid and short-circuits the
-        revoke_map['issued_before'] = max(
+    # rest of the logic.
-            event.issued_before, revoke_map.get(
-                'issued_before', event.issued_before))
-        return event
 
-    def remove_event(self, event):
+    # The token has three attributes that can match the user_id
-        """Update the tree based on the removal of a Revocation Event.
+    if event.user_id is not None:
-
+        if all(event.user_id != token_values[attribute_name]
-        Removes empty nodes from the tree from the leaf back to the root.
+               for attribute_name in ['user_id', 'trustor_id', 'trustee_id']):
-
-        If multiple events trace the same path, but have different
-        'issued_before' values, only the last is ever stored in the tree.
-        So only an exact match on 'issued_before' ever triggers a removal
-
-        :param: Event to remove from the tree
-
-        """
-        stack = []
-        revoke_map = self.revoke_map
-        for name in _EVENT_NAMES:
-            key = event.key_for_name(name)
-            nxt = revoke_map.get(key)
-            if nxt is None:
-                break
-            stack.append((revoke_map, key, nxt))
-            revoke_map = nxt
-        else:
-            if event.issued_before == revoke_map['issued_before']:
-                revoke_map.pop('issued_before')
-        for parent, key, child in reversed(stack):
-            if not any(child):
-                del parent[key]
-
-    def add_events(self, revoke_events):
-        return list(map(self.add_event, revoke_events or []))
-
-    @staticmethod
-    def _next_level_keys(name, token_data):
-        """Generate keys based on current field name and token data.
-
-        Generate all keys to look for in the next iteration of revocation
-        event tree traversal.
-        """
-        yield '*'
-        if name == 'role_id':
-            # Roles are very special since a token has a list of them.
-            # If the revocation event matches any one of them,
-            # revoke the token.
-            for role_id in token_data.get('roles', []):
-                yield role_id
-        else:
-            # For other fields we try to get any branch that concur
-            # with any alternative field in the token.
-            for alt_name in ALTERNATIVES.get(name, [name]):
-                yield token_data[alt_name]
-
-    def _search(self, revoke_map, names, token_data):
-        """Search for revocation event by token_data.
-
-        Traverse the revocation events tree looking for event matching token
-        data issued after the token.
-        """
-        if not names:
-            # The last (leaf) level is checked in a special way because we
-            # verify issued_at field differently.
-            try:
-                return revoke_map['issued_before'] >= token_data['issued_at']
-            except KeyError:
             return False
 
-        name, remaining_names = names[0], names[1:]
+    # The token has two attributes that can match the domain_id
+    if event.domain_id is not None:
+        if all(event.domain_id != token_values[attribute_name]
+                for attribute_name in ['identity_domain_id',
+                                       'assignment_domain_id']):
+            return False
 
-        for key in self._next_level_keys(name, token_data):
+    if event.domain_scope_id is not None:
-            subtree = revoke_map.get('%s=%s' % (name, key))
+        if event.domain_scope_id != token_values['assignment_domain_id']:
-            if subtree and self._search(subtree, remaining_names, token_data):
+            return False
+
+    # If an event specifies an attribute name, but it does not  match,
+    # the token is not revoked.
+    attribute_names = ['project_id',
+                       'expires_at', 'trust_id', 'consumer_id',
+                       'access_token_id', 'audit_id', 'audit_chain_id']
+    for attribute_name in attribute_names:
+        if getattr(event, attribute_name) is not None:
+            if (getattr(event, attribute_name) !=
+                    token_values[attribute_name]):
+                        return False
+
+    if event.role_id is not None:
+        roles = token_values['roles']
+        if all(event.role_id != role for role in roles):
+            return False
+
+    if token_values['issued_at'] > event.issued_before:
+        return False
     return True
 
-        # If we made it out of the loop then no element in revocation tree
-        # corresponds to our token and it is good.
-        return False
-
-    def is_revoked(self, token_data):
-        """Check if a token matches the revocation event.
-
-        Compare the values for each level of the tree with the values from
-        the token, accounting for attributes that have alternative
-        keys, and for wildcard matches.
-        if there is a match, continue down the tree.
-        if there is no match, exit early.
-
-        token_data is a map based on a flattened view of token.
-        The required fields are:
-
-           'expires_at','user_id', 'project_id', 'identity_domain_id',
-           'assignment_domain_id', 'trust_id', 'trustor_id', 'trustee_id'
-           'consumer_id', 'access_token_id'
-
-        """
-        return self._search(self.revoke_map, _EVENT_NAMES, token_data)
-
 
 def build_token_values_v2(access, default_domain_id):
     token_data = access['token']

keystone/revoke/core.py

@@ -12,6 +12,7 @@
 
 """Main entry point into the Revoke service."""
 
+import oslo_cache
 from oslo_config import cfg
 from oslo_log import versionutils
 
@@ -47,7 +48,13 @@ EXTENSION_DATA = {
 extension.register_admin_extension(EXTENSION_DATA['alias'], EXTENSION_DATA)
 extension.register_public_extension(EXTENSION_DATA['alias'], EXTENSION_DATA)
 
-MEMOIZE = cache.get_memoization_decorator(group='revoke')
+# This builds a discrete cache region dedicated to revoke events. The API can
+# return a filtered list based upon last fetchtime. This is deprecated but
+# must be maintained.
+REVOKE_REGION = oslo_cache.create_region()
+MEMOIZE = cache.get_memoization_decorator(
+    group='revoke',
+    region=REVOKE_REGION)
 
 
 @dependency.provider('revoke_api')
@@ -68,6 +75,13 @@ class Manager(manager.Manager):
         self._register_listeners()
         self.model = revoke_model
 
+    @MEMOIZE
+    def _list_events(self, last_fetch):
+        return self.driver.list_events(last_fetch)
+
+    def list_events(self, last_fetch=None):
+        return self._list_events(last_fetch)
+
     def _user_callback(self, service, resource_type, operation,
                        payload):
         self.revoke_by_user(payload['resource_info'])
@@ -194,13 +208,6 @@ class Manager(manager.Manager):
         self.revoke(revoke_model.RevokeEvent(domain_id=domain_id,
                                              role_id=role_id))
 
-    @MEMOIZE
-    def _get_revoke_tree(self):
-        events = self.driver.list_events()
-        revoke_tree = revoke_model.RevokeTree(revoke_events=events)
-
-        return revoke_tree
-
     def check_token(self, token_values):
         """Check the values from a token against the revocation list.
 
@@ -211,12 +218,12 @@ class Manager(manager.Manager):
         :raises keystone.exception.TokenNotFound: If the token is invalid.
 
         """
-        if self._get_revoke_tree().is_revoked(token_values):
+        if revoke_model.is_revoked(self.list_events(), token_values):
             raise exception.TokenNotFound(_('Failed to validate token'))
 
     def revoke(self, event):
         self.driver.revoke(event)
-        self._get_revoke_tree.invalidate(self)
+        REVOKE_REGION.invalidate()
 
 
 @versionutils.deprecated(

keystone/server/backends.py

@@ -38,6 +38,9 @@ def load_backends():
     cache.apply_invalidation_patch(
         region=assignment.COMPUTED_ASSIGNMENTS_REGION,
         region_name=assignment.COMPUTED_ASSIGNMENTS_REGION.name)
+    cache.configure_cache(region=revoke.REVOKE_REGION)
+    cache.apply_invalidation_patch(region=revoke.REVOKE_REGION,
+                                   region_name=revoke.REVOKE_REGION.name)
 
     # Ensure that the identity driver is created before the assignment manager
     # and that the assignment driver is created before the resource manager.

keystone/tests/unit/ksfixtures/cache.py

@@ -15,9 +15,11 @@ import fixtures
 
 from keystone import catalog
 from keystone.common import cache
+from keystone import revoke
 
 
-CACHE_REGIONS = (cache.CACHE_REGION, catalog.COMPUTED_CATALOG_REGION)
+CACHE_REGIONS = (cache.CACHE_REGION, catalog.COMPUTED_CATALOG_REGION,
+                 revoke.REVOKE_REGION)
 
 
 class Cache(fixtures.Fixture):

keystone/tests/unit/test_revoke.py

@@ -190,11 +190,22 @@ class SqlRevokeTests(test_backend_sql.SqlTests, RevokeTests):
             revoke_by_id=False)
 
 
-class RevokeTreeTests(unit.TestCase):
+def add_event(events, event):
+    events.append(event)
+    return event
+
+
+def remove_event(events, event):
+    for target in events:
+        if target == event:
+            events.remove(target)
+
+
+class RevokeListTests(unit.TestCase):
     def setUp(self):
-        super(RevokeTreeTests, self).setUp()
+        super(RevokeListTests, self).setUp()
         self.events = []
-        self.tree = revoke_model.RevokeTree()
+        self.revoke_events = list()
         self._sample_data()
 
     def _sample_data(self):
@@ -238,27 +249,32 @@ class RevokeTreeTests(unit.TestCase):
 
     def _assertTokenRevoked(self, token_data):
         self.assertTrue(any([_matches(e, token_data) for e in self.events]))
-        return self.assertTrue(self.tree.is_revoked(token_data),
+        return self.assertTrue(
+            revoke_model.is_revoked(self.revoke_events, token_data),
             'Token should be revoked')
 
     def _assertTokenNotRevoked(self, token_data):
         self.assertFalse(any([_matches(e, token_data) for e in self.events]))
-        return self.assertFalse(self.tree.is_revoked(token_data),
+        return self.assertFalse(
+            revoke_model.is_revoked(self.revoke_events, token_data),
             'Token should not be revoked')
 
     def _revoke_by_user(self, user_id):
-        return self.tree.add_event(
+        return add_event(
+            self.revoke_events,
             revoke_model.RevokeEvent(user_id=user_id))
 
     def _revoke_by_audit_id(self, audit_id):
-        event = self.tree.add_event(
+        event = add_event(
+            self.revoke_events,
             revoke_model.RevokeEvent(audit_id=audit_id))
         self.events.append(event)
         return event
 
     def _revoke_by_audit_chain_id(self, audit_chain_id, project_id=None,
                                   domain_id=None):
-        event = self.tree.add_event(
+        event = add_event(
+            self.revoke_events,
             revoke_model.RevokeEvent(audit_chain_id=audit_chain_id,
                                      project_id=project_id,
                                      domain_id=domain_id)
@@ -268,7 +284,8 @@ class RevokeTreeTests(unit.TestCase):
 
     def _revoke_by_expiration(self, user_id, expires_at, project_id=None,
                               domain_id=None):
-        event = self.tree.add_event(
+        event = add_event(
+            self.revoke_events,
             revoke_model.RevokeEvent(user_id=user_id,
                                      expires_at=expires_at,
                                      project_id=project_id,
@@ -278,7 +295,8 @@ class RevokeTreeTests(unit.TestCase):
 
     def _revoke_by_grant(self, role_id, user_id=None,
                          domain_id=None, project_id=None):
-        event = self.tree.add_event(
+        event = add_event(
+            self.revoke_events,
             revoke_model.RevokeEvent(user_id=user_id,
                                      role_id=role_id,
                                      domain_id=domain_id,
@@ -287,28 +305,28 @@ class RevokeTreeTests(unit.TestCase):
         return event
 
     def _revoke_by_user_and_project(self, user_id, project_id):
-        event = self.tree.add_event(
+        event = add_event(self.revoke_events,
                           revoke_model.RevokeEvent(project_id=project_id,
                                                    user_id=user_id))
         self.events.append(event)
         return event
 
     def _revoke_by_project_role_assignment(self, project_id, role_id):
-        event = self.tree.add_event(
+        event = add_event(self.revoke_events,
                           revoke_model.RevokeEvent(project_id=project_id,
                                                    role_id=role_id))
         self.events.append(event)
         return event
 
     def _revoke_by_domain_role_assignment(self, domain_id, role_id):
-        event = self.tree.add_event(
+        event = add_event(self.revoke_events,
                           revoke_model.RevokeEvent(domain_id=domain_id,
                                                    role_id=role_id))
         self.events.append(event)
         return event
 
     def _revoke_by_domain(self, domain_id):
-        event = self.tree.add_event(
+        event = add_event(self.revoke_events,
                           revoke_model.RevokeEvent(domain_id=domain_id))
         self.events.append(event)
 
@@ -322,7 +340,7 @@ class RevokeTreeTests(unit.TestCase):
         token_data_u2 = _sample_blank_token()
         token_data_u2[field_name] = _new_id()
         self._assertTokenNotRevoked(token_data_u2)
-        self.tree.remove_event(event)
+        remove_event(self.revoke_events, event)
         self.events.remove(event)
         self._assertTokenNotRevoked(token_data_u1)
 
@@ -430,7 +448,7 @@ class RevokeTreeTests(unit.TestCase):
 
     def remove_event(self, event):
         self.events.remove(event)
-        self.tree.remove_event(event)
+        remove_event(self.revoke_events, event)
 
     def test_by_project_grant(self):
         token_to_revoke = self.token_to_revoke
@@ -549,64 +567,10 @@ class RevokeTreeTests(unit.TestCase):
     def _assertEmpty(self, collection):
         return self.assertEqual(0, len(collection), "collection not empty")
 
-    def _assertEventsMatchIteration(self, turn):
-        self.assertEqual(1, len(self.tree.revoke_map))
-        self.assertEqual(turn + 1, len(self.tree.revoke_map
-                                       ['trust_id=*']
-                                       ['consumer_id=*']
-                                       ['access_token_id=*']
-                                       ['audit_id=*']
-                                       ['audit_chain_id=*']))
-        # two different functions add  domain_ids, +1 for None
-        self.assertEqual(2 * turn + 1, len(self.tree.revoke_map
-                                           ['trust_id=*']
-                                           ['consumer_id=*']
-                                           ['access_token_id=*']
-                                           ['audit_id=*']
-                                           ['audit_chain_id=*']
-                                           ['expires_at=*']))
-        # two different functions add  project_ids, +1 for None
-        self.assertEqual(2 * turn + 1, len(self.tree.revoke_map
-                                           ['trust_id=*']
-                                           ['consumer_id=*']
-                                           ['access_token_id=*']
-                                           ['audit_id=*']
-                                           ['audit_chain_id=*']
-                                           ['expires_at=*']
-                                           ['domain_id=*']))
-        # 10 users added
-        self.assertEqual(turn, len(self.tree.revoke_map
-                                   ['trust_id=*']
-                                   ['consumer_id=*']
-                                   ['access_token_id=*']
-                                   ['audit_id=*']
-                                   ['audit_chain_id=*']
-                                   ['expires_at=*']
-                                   ['domain_id=*']
-                                   ['project_id=*']))
-
     def test_cleanup(self):
         events = self.events
-        self._assertEmpty(self.tree.revoke_map)
+        self._assertEmpty(self.revoke_events)
-        expiry_base_time = _future_time()
         for i in range(0, 10):
-            events.append(
-                self._revoke_by_user(_new_id()))
-
-            args = (_new_id(),
-                    expiry_base_time + datetime.timedelta(seconds=i))
-            events.append(
-                self._revoke_by_expiration(*args))
-
-            self.assertEqual(i + 2, len(self.tree.revoke_map
-                                        ['trust_id=*']
-                                        ['consumer_id=*']
-                                        ['access_token_id=*']
-                                        ['audit_id=*']
-                                        ['audit_chain_id=*']),
-                             'adding %s to %s' % (args,
-                                                  self.tree.revoke_map))
-
             events.append(
                 self._revoke_by_project_role_assignment(_new_id(), _new_id()))
             events.append(
@@ -615,8 +579,7 @@ class RevokeTreeTests(unit.TestCase):
                 self._revoke_by_domain_role_assignment(_new_id(), _new_id()))
             events.append(
                 self._revoke_by_user_and_project(_new_id(), _new_id()))
-            self._assertEventsMatchIteration(i + 1)
 
         for event in self.events:
-            self.tree.remove_event(event)
+            remove_event(self.revoke_events, event)
-        self._assertEmpty(self.tree.revoke_map)
+        self._assertEmpty(self.revoke_events)