Add a cache around server ACL checking

synapse/events/validator.py

@@ -33,7 +33,7 @@
     CANONICALJSON_MIN_INT,
     validate_canonicaljson,
 )
-from synapse.federation.federation_server import server_matches_acl_event
+from synapse.federation.federation_server import server_acl_evaluator_from_event
 from synapse.http.servlet import validate_json_object
 from synapse.rest.models import RequestBodyModel
 from synapse.types import EventID, JsonDict, RoomID, StrCollection, UserID
@@ -100,7 +100,10 @@ def validate_new(self, event: EventBase, config: HomeServerConfig) -> None:
             self._validate_retention(event)
 
         elif event.type == EventTypes.ServerACL:
-            if not server_matches_acl_event(config.server.server_name, event):
+            server_acl_evaluator = server_acl_evaluator_from_event(event)
+            if not server_acl_evaluator.server_matches_acl_event(
+                config.server.server_name
+            ):
                 raise SynapseError(
                     400, "Can't create an ACL event that denies the local server"
                 )

synapse/federation/federation_server.py

@@ -25,10 +25,13 @@
     List,
     Mapping,
     Optional,
+    Pattern,
+    Sequence,
     Tuple,
     Union,
 )
 
+import attr
 from matrix_common.regex import glob_to_regex
 from prometheus_client import Counter, Gauge, Histogram
 
@@ -126,6 +129,50 @@
 _INBOUND_EVENT_HANDLING_LOCK_NAME = "federation_inbound_pdu"
 
 
+@attr.s(slots=True, frozen=True, auto_attribs=True)
+class ServerAclEvaluator:
+    allow_ip_literals: bool
+    allow: Sequence[Pattern[str]]
+    deny: Sequence[Pattern[str]]
+
+    def server_matches_acl_event(self, server_name: str) -> bool:
+        """Check if the given server is allowed by the ACL event
+
+        Args:
+            server_name: name of server, without any port part
+
+        Returns:
+            True if this server is allowed by the ACLs
+        """
+
+        # first of all, check if literal IPs are blocked, and if so, whether the
+        # server name is a literal IP
+        if not self.allow_ip_literals:
+            # check for ipv6 literals. These start with '['.
+            if server_name[0] == "[":
+                return False
+
+            # check for ipv4 literals. We can just lift the routine from twisted.
+            if isIPAddress(server_name):
+                return False
+
+        # next,  check the deny list
+        for e in self.deny:
+            if e.match(server_name):
+                # logger.info("%s matched deny rule %s", server_name, e)
+                return False
+
+        # then the allow list.
+        for e in self.allow:
+            if e.match(server_name):
+                # logger.info("%s matched allow rule %s", server_name, e)
+                return True
+
+        # everything else should be rejected.
+        # logger.info("%s fell through", server_name)
+        return False
+
+
 class FederationServer(FederationBase):
     def __init__(self, hs: "HomeServer"):
         super().__init__(hs)
@@ -1327,72 +1374,44 @@ async def check_server_matches_acl(self, server_name: str, room_id: str) -> None
         acl_event = await self._storage_controllers.state.get_current_state_event(
             room_id, EventTypes.ServerACL, ""
         )
-        if not acl_event or server_matches_acl_event(server_name, acl_event):
-            return
+        if acl_event:
+            server_acl_evaluator = server_acl_evaluator_from_event(acl_event)
+            if not server_acl_evaluator.server_matches_acl_event(server_name):
+                raise AuthError(code=403, msg="Server is banned from room")
 
-        raise AuthError(code=403, msg="Server is banned from room")
 
+def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator":
+    """
+    Create a ServerAclEvaluator from a m.room.server_acl event's content.
 
-def server_matches_acl_event(server_name: str, acl_event: EventBase) -> bool:
-    """Check if the given server is allowed by the ACL event
-
-    Args:
-        server_name: name of server, without any port part
-        acl_event: m.room.server_acl event
-
-    Returns:
-        True if this server is allowed by the ACLs
+    This does up-front parsing of the content to ignore bad data and pre-compile
+    regular expressions.
     """
-    logger.debug("Checking %s against acl %s", server_name, acl_event.content)
 
     # first of all, check if literal IPs are blocked, and if so, whether the
     # server name is a literal IP
     allow_ip_literals = acl_event.content.get("allow_ip_literals", True)
     if not isinstance(allow_ip_literals, bool):
         logger.warning("Ignoring non-bool allow_ip_literals flag")
         allow_ip_literals = True
-    if not allow_ip_literals:
-        # check for ipv6 literals. These start with '['.
-        if server_name[0] == "[":
-            return False
-
-        # check for ipv4 literals. We can just lift the routine from twisted.
-        if isIPAddress(server_name):
-            return False
 
     # next,  check the deny list
     deny = acl_event.content.get("deny", [])
     if not isinstance(deny, (list, tuple)):
         logger.warning("Ignoring non-list deny ACL %s", deny)
         deny = []
-    for e in deny:
-        if _acl_entry_matches(server_name, e):
-            # logger.info("%s matched deny rule %s", server_name, e)
-            return False
+    else:
+        deny = [glob_to_regex(s) for s in deny if isinstance(s, str)]
 
     # then the allow list.
     allow = acl_event.content.get("allow", [])
     if not isinstance(allow, (list, tuple)):
         logger.warning("Ignoring non-list allow ACL %s", allow)
         allow = []
-    for e in allow:
-        if _acl_entry_matches(server_name, e):
-            # logger.info("%s matched allow rule %s", server_name, e)
-            return True
-
-    # everything else should be rejected.
-    # logger.info("%s fell through", server_name)
-    return False
-
+    else:
+        allow = [glob_to_regex(s) for s in allow if isinstance(s, str)]
 
-def _acl_entry_matches(server_name: str, acl_entry: Any) -> bool:
-    if not isinstance(acl_entry, str):
-        logger.warning(
-            "Ignoring non-str ACL entry '%s' (is %s)", acl_entry, type(acl_entry)
-        )
-        return False
-    regex = glob_to_regex(acl_entry)
-    return bool(regex.match(server_name))
+    return ServerAclEvaluator(allow_ip_literals, allow, deny)
 
 
 class FederationHandlerRegistry:

tests/federation/test_federation_server.py

@@ -22,7 +22,7 @@
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
 from synapse.config.server import DEFAULT_ROOM_VERSION
 from synapse.events import EventBase, make_event_from_dict
-from synapse.federation.federation_server import server_matches_acl_event
+from synapse.federation.federation_server import server_acl_evaluator_from_event
 from synapse.rest import admin
 from synapse.rest.client import login, room
 from synapse.server import HomeServer
@@ -67,37 +67,46 @@ def test_blocked_server(self) -> None:
         e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]})
         logging.info("ACL event: %s", e.content)
 
-        self.assertFalse(server_matches_acl_event("evil.com", e))
-        self.assertFalse(server_matches_acl_event("EVIL.COM", e))
+        server_acl_evalutor = server_acl_evaluator_from_event(e)
 
-        self.assertTrue(server_matches_acl_event("evil.com.au", e))
-        self.assertTrue(server_matches_acl_event("honestly.not.evil.com", e))
+        self.assertFalse(server_acl_evalutor.server_matches_acl_event("evil.com"))
+        self.assertFalse(server_acl_evalutor.server_matches_acl_event("EVIL.COM"))
+
+        self.assertTrue(server_acl_evalutor.server_matches_acl_event("evil.com.au"))
+        self.assertTrue(
+            server_acl_evalutor.server_matches_acl_event("honestly.not.evil.com")
+        )
 
     def test_block_ip_literals(self) -> None:
         e = _create_acl_event({"allow_ip_literals": False, "allow": ["*"]})
         logging.info("ACL event: %s", e.content)
 
-        self.assertFalse(server_matches_acl_event("1.2.3.4", e))
-        self.assertTrue(server_matches_acl_event("1a.2.3.4", e))
-        self.assertFalse(server_matches_acl_event("[1:2::]", e))
-        self.assertTrue(server_matches_acl_event("1:2:3:4", e))
+        server_acl_evalutor = server_acl_evaluator_from_event(e)
+
+        self.assertFalse(server_acl_evalutor.server_matches_acl_event("1.2.3.4"))
+        self.assertTrue(server_acl_evalutor.server_matches_acl_event("1a.2.3.4"))
+        self.assertFalse(server_acl_evalutor.server_matches_acl_event("[1:2::]"))
+        self.assertTrue(server_acl_evalutor.server_matches_acl_event("1:2:3:4"))
 
     def test_wildcard_matching(self) -> None:
         e = _create_acl_event({"allow": ["good*.com"]})
+
+        server_acl_evalutor = server_acl_evaluator_from_event(e)
+
         self.assertTrue(
-            server_matches_acl_event("good.com", e),
+            server_acl_evalutor.server_matches_acl_event("good.com"),
             "* matches 0 characters",
         )
         self.assertTrue(
-            server_matches_acl_event("GOOD.COM", e),
+            server_acl_evalutor.server_matches_acl_event("GOOD.COM"),
             "pattern is case-insensitive",
         )
         self.assertTrue(
-            server_matches_acl_event("good.aa.com", e),
+            server_acl_evalutor.server_matches_acl_event("good.aa.com"),
             "* matches several characters, including '.'",
         )
         self.assertFalse(
-            server_matches_acl_event("ishgood.com", e),
+            server_acl_evalutor.server_matches_acl_event("ishgood.com"),
             "pattern does not allow prefixes",
         )