From b0efe7f4e5ced8d1e131b86744674ac55f54391c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 19 Sep 2023 11:40:20 -0400 Subject: [PATCH 01/12] Rework server ACL checking into an object. --- synapse/events/validator.py | 7 +- synapse/federation/federation_server.py | 103 ++++++++++++--------- tests/federation/test_federation_server.py | 35 ++++--- 3 files changed, 88 insertions(+), 57 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 5da50cb0d20b..1454266d8192 100644 --- a/synapse/events/validator.py +++ b/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" ) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index f9915e5a3f05..0ace1b45d818 100644 --- a/synapse/federation/federation_server.py +++ b/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,23 +1374,19 @@ 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 @@ -1351,48 +1394,24 @@ def server_matches_acl_event(server_name: str, acl_event: EventBase) -> bool: 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: diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 5c850d184380..e5b93d8c0581 100644 --- a/tests/federation/test_federation_server.py +++ b/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", ) From ef4e685ca42f32279f46fe631654c684820f6bf4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 19 Sep 2023 13:09:19 -0400 Subject: [PATCH 02/12] Add caching. --- synapse/federation/federation_server.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 0ace1b45d818..ed38617ed9b2 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -92,6 +92,7 @@ from synapse.types import JsonDict, StateMap, get_domain_from_id from synapse.util import unwrapFirstError from synapse.util.async_helpers import Linearizer, concurrently_execute, gather_results +from synapse.util.caches.descriptors import cached from synapse.util.caches.response_cache import ResponseCache from synapse.util.stringutils import parse_server_name @@ -1375,10 +1376,19 @@ async def check_server_matches_acl(self, server_name: str, room_id: str) -> None room_id, EventTypes.ServerACL, "" ) if acl_event: - server_acl_evaluator = server_acl_evaluator_from_event(acl_event) + server_acl_evaluator = await self._get_server_acl_evaluator( + acl_event.event_id, acl_event + ) if not server_acl_evaluator.server_matches_acl_event(server_name): raise AuthError(code=403, msg="Server is banned from room") + @cached(uncached_args=("acl_event",)) + def _get_server_acl_evaluator( + self, event_id: str, acl_event: EventBase + ) -> ServerAclEvaluator: + """Create a ServerAclEvaluator from an event, but cached on the event ID.""" + return server_acl_evaluator_from_event(acl_event) + def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator": """ From a45346bdbcfc5d53c8b1d15444b9846c90bfb4a9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 10:20:47 -0400 Subject: [PATCH 03/12] Convert checking server ACLs to rust. --- rust/src/acl/mod.rs | 100 ++++++++++++++++++++++++ rust/src/lib.rs | 2 + stubs/synapse/synapse_rust/acl.pyi | 21 +++++ synapse/federation/federation_server.py | 54 +------------ 4 files changed, 126 insertions(+), 51 deletions(-) create mode 100644 rust/src/acl/mod.rs create mode 100644 stubs/synapse/synapse_rust/acl.pyi diff --git a/rust/src/acl/mod.rs b/rust/src/acl/mod.rs new file mode 100644 index 000000000000..fae29aeca24a --- /dev/null +++ b/rust/src/acl/mod.rs @@ -0,0 +1,100 @@ +// Copyright 2023 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! An implementation of Matrix server ACL rules. + +use crate::push::utils::{glob_to_regex, GlobMatchType}; +use anyhow::Error; +use pyo3::prelude::*; +use regex::Regex; +use std::net::Ipv4Addr; +use std::str::FromStr; + +/// Called when registering modules with python. +pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { + let child_module = PyModule::new(py, "acl")?; + child_module.add_class::()?; + + m.add_submodule(child_module)?; + + // We need to manually add the module to sys.modules to make `from + // synapse.synapse_rust import acl` work. + py.import("sys")? + .getattr("modules")? + .set_item("synapse.synapse_rust.acl", child_module)?; + + Ok(()) +} + +#[derive(Debug, Clone)] +#[pyclass(frozen)] +pub struct ServerAclEvaluator { + allow_ip_literals: bool, + allow: Vec, + deny: Vec, +} + +#[pymethods] +impl ServerAclEvaluator { + #[new] + pub fn py_new( + allow_ip_literals: bool, + allow: Vec, + deny: Vec, + ) -> Result { + let allow = allow + .iter() + .map(|s| glob_to_regex(s, GlobMatchType::Whole).unwrap()) + .collect(); + let deny = deny + .iter() + .map(|s| glob_to_regex(s, GlobMatchType::Whole).unwrap()) + .collect(); + + Ok(ServerAclEvaluator { + allow_ip_literals, + allow, + deny, + }) + } + + pub fn server_matches_acl_event(&self, server_name: &str) -> bool { + // first of all, check if literal IPs are blocked, and if so, whether the + // server name is a literal IP + if !self.allow_ip_literals { + // check for ipv6 literals. These start with '['. + if server_name.starts_with("[") { + return false; + } + + // check for ipv4 literals. We can just lift the routine from std::net. + if let Ok(_) = Ipv4Addr::from_str(server_name) { + return false; + } + } + + // next, check the deny list + if self.deny.iter().any(|e| e.is_match(server_name)) { + return false; + } + + // then the allow list. + if self.allow.iter().any(|e| e.is_match(server_name)) { + return true; + } + + // everything else should be rejected. + false + } +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index ce67f5861183..c44c09bda719 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -2,6 +2,7 @@ use lazy_static::lazy_static; use pyo3::prelude::*; use pyo3_log::ResetHandle; +pub mod acl; pub mod push; lazy_static! { @@ -38,6 +39,7 @@ fn synapse_rust(py: Python<'_>, m: &PyModule) -> PyResult<()> { m.add_function(wrap_pyfunction!(get_rust_file_digest, m)?)?; m.add_function(wrap_pyfunction!(reset_logging_config, m)?)?; + acl::register_module(py, m)?; push::register_module(py, m)?; Ok(()) diff --git a/stubs/synapse/synapse_rust/acl.pyi b/stubs/synapse/synapse_rust/acl.pyi new file mode 100644 index 000000000000..e03989b627d8 --- /dev/null +++ b/stubs/synapse/synapse_rust/acl.pyi @@ -0,0 +1,21 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import List + +class ServerAclEvaluator: + def __init__( + self, allow_ip_literals: bool, allow: List[str], deny: List[str] + ) -> None: ... + def server_matches_acl_event(self, server_name: str) -> bool: ... diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index ed38617ed9b2..4dd35742d49f 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -25,17 +25,12 @@ List, Mapping, Optional, - Pattern, - Sequence, Tuple, Union, ) -import attr -from matrix_common.regex import glob_to_regex from prometheus_client import Counter, Gauge, Histogram -from twisted.internet.abstract import isIPAddress from twisted.python import failure from synapse.api.constants import ( @@ -89,6 +84,7 @@ from synapse.storage.databases.main.lock import Lock from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary from synapse.storage.roommember import MemberSummary +from synapse.synapse_rust.acl import ServerAclEvaluator from synapse.types import JsonDict, StateMap, get_domain_from_id from synapse.util import unwrapFirstError from synapse.util.async_helpers import Linearizer, concurrently_execute, gather_results @@ -130,50 +126,6 @@ _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) @@ -1411,7 +1363,7 @@ def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator logger.warning("Ignoring non-list deny ACL %s", deny) deny = [] else: - deny = [glob_to_regex(s) for s in deny if isinstance(s, str)] + deny = [s for s in deny if isinstance(s, str)] # then the allow list. allow = acl_event.content.get("allow", []) @@ -1419,7 +1371,7 @@ def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator logger.warning("Ignoring non-list allow ACL %s", allow) allow = [] else: - allow = [glob_to_regex(s) for s in allow if isinstance(s, str)] + allow = [s for s in allow if isinstance(s, str)] return ServerAclEvaluator(allow_ip_literals, allow, deny) From 431ae17486b90280441bf39314c580c7435f10d7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 12:02:34 -0400 Subject: [PATCH 04/12] Newsfragment --- changelog.d/16360.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16360.misc diff --git a/changelog.d/16360.misc b/changelog.d/16360.misc new file mode 100644 index 000000000000..b32d7b521ea7 --- /dev/null +++ b/changelog.d/16360.misc @@ -0,0 +1 @@ +Cache server ACL checking. From 31bbf762353dd23fc1a0b2cc7f8fd643b7f72783 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 12:07:18 -0400 Subject: [PATCH 05/12] Appease clippy --- rust/src/acl/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/src/acl/mod.rs b/rust/src/acl/mod.rs index fae29aeca24a..99d39319afd8 100644 --- a/rust/src/acl/mod.rs +++ b/rust/src/acl/mod.rs @@ -74,12 +74,12 @@ impl ServerAclEvaluator { // server name is a literal IP if !self.allow_ip_literals { // check for ipv6 literals. These start with '['. - if server_name.starts_with("[") { + if server_name.starts_with('[') { return false; } // check for ipv4 literals. We can just lift the routine from std::net. - if let Ok(_) = Ipv4Addr::from_str(server_name) { + if Ipv4Addr::from_str(server_name).is_ok() { return false; } } From 609cbe034915466a9752a679c2aa0b811ee2beca Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 12:13:51 -0400 Subject: [PATCH 06/12] rustfmt --- rust/src/acl/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rust/src/acl/mod.rs b/rust/src/acl/mod.rs index 99d39319afd8..f3e9b521ee1d 100644 --- a/rust/src/acl/mod.rs +++ b/rust/src/acl/mod.rs @@ -14,12 +14,14 @@ //! An implementation of Matrix server ACL rules. -use crate::push::utils::{glob_to_regex, GlobMatchType}; +use std::net::Ipv4Addr; +use std::str::FromStr; + use anyhow::Error; use pyo3::prelude::*; use regex::Regex; -use std::net::Ipv4Addr; -use std::str::FromStr; + +use crate::push::utils::{glob_to_regex, GlobMatchType}; /// Called when registering modules with python. pub fn register_module(py: Python<'_>, m: &PyModule) -> PyResult<()> { From 119056d98ab2c41ec5b610b9b08d80a8495b523a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 13:48:41 -0400 Subject: [PATCH 07/12] Cache server ACLs on a per-room basis. --- synapse/events/validator.py | 2 +- synapse/federation/federation_server.py | 52 ++----------------- synapse/storage/controllers/state.py | 58 ++++++++++++++++++++++ synapse/storage/databases/main/cache.py | 5 ++ tests/federation/test_federation_server.py | 2 +- 5 files changed, 68 insertions(+), 51 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 1454266d8192..5817f55b9abd 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -33,9 +33,9 @@ CANONICALJSON_MIN_INT, validate_canonicaljson, ) -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.storage.controllers.state import server_acl_evaluator_from_event from synapse.types import EventID, JsonDict, RoomID, StrCollection, UserID diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 4dd35742d49f..9a46cc6e4392 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1324,57 +1324,11 @@ async def check_server_matches_acl(self, server_name: str, room_id: str) -> None Raises: AuthError if the server does not match the ACL """ - acl_event = await self._storage_controllers.state.get_current_state_event( - room_id, EventTypes.ServerACL, "" - ) - if acl_event: - server_acl_evaluator = await self._get_server_acl_evaluator( - acl_event.event_id, acl_event - ) - if not server_acl_evaluator.server_matches_acl_event(server_name): + server_acl_evaluator = await self._storage_controllers.state.get_server_acl_for_room( + room_id) + if server_acl_evaluator and not server_acl_evaluator.server_matches_acl_event(server_name): raise AuthError(code=403, msg="Server is banned from room") - @cached(uncached_args=("acl_event",)) - def _get_server_acl_evaluator( - self, event_id: str, acl_event: EventBase - ) -> ServerAclEvaluator: - """Create a ServerAclEvaluator from an event, but cached on the event ID.""" - return server_acl_evaluator_from_event(acl_event) - - -def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator": - """ - Create a ServerAclEvaluator from a m.room.server_acl event's content. - - This does up-front parsing of the content to ignore bad data and pre-compile - regular expressions. - """ - - # 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 - - # 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 = [] - else: - deny = [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 = [] - else: - allow = [s for s in allow if isinstance(s, str)] - - return ServerAclEvaluator(allow_ip_literals, allow, deny) - class FederationHandlerRegistry: """Allows classes to register themselves as handlers for a given EDU or diff --git a/synapse/storage/controllers/state.py b/synapse/storage/controllers/state.py index 10d219c0452e..edc71678d483 100644 --- a/synapse/storage/controllers/state.py +++ b/synapse/storage/controllers/state.py @@ -37,6 +37,7 @@ PartialCurrentStateTracker, PartialStateEventsTracker, ) +from synapse.synapse_rust.acl import ServerAclEvaluator from synapse.types import MutableStateMap, StateMap, get_domain_from_id from synapse.types.state import StateFilter from synapse.util.async_helpers import Linearizer @@ -501,6 +502,29 @@ async def get_canonical_alias_for_room(self, room_id: str) -> Optional[str]: return event.content.get("alias") + @cached() + async def get_server_acl_for_room(self, room_id: str) -> Optional[ServerAclEvaluator]: + """Get the server ACL evaluator for room, if any + + This does up-front parsing of the content to ignore bad data and pre-compile + regular expressions. + + Args: + room_id: The room ID + + Returns: + The server ACL evaluator, if any + """ + + acl_event = await self.get_current_state_event( + room_id, EventTypes.ServerACL, "" + ) + + if not acl_event: + return None + + return server_acl_evaluator_from_event(acl_event) + @trace @tag_args async def get_current_state_deltas( @@ -760,3 +784,37 @@ async def _get_joined_hosts( cache.state_group = object() return frozenset(cache.hosts_to_joined_users) + + +def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator": + """ + Create a ServerAclEvaluator from a m.room.server_acl event's content. + + This does up-front parsing of the content to ignore bad data and pre-compile + regular expressions. + """ + + # 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 + + # 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 = [] + else: + deny = [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 = [] + else: + allow = [s for s in allow if isinstance(s, str)] + + return ServerAclEvaluator(allow_ip_literals, allow, deny) diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py index 2fbd389c7168..7609f8edb208 100644 --- a/synapse/storage/databases/main/cache.py +++ b/synapse/storage/databases/main/cache.py @@ -329,6 +329,9 @@ def _invalidate_caches_for_event( "get_forgotten_rooms_for_user", (state_key,) ) + if etype == EventTypes.ServerACL: + self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate((room_id,)) + if relates_to: self._attempt_to_invalidate_cache("get_relations_for_event", (relates_to,)) self._attempt_to_invalidate_cache("get_references_for_event", (relates_to,)) @@ -383,6 +386,8 @@ def _invalidate_caches_for_room_events(self, room_id: str) -> None: self._attempt_to_invalidate_cache("get_thread_participated", None) self._attempt_to_invalidate_cache("get_threads", (room_id,)) + self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate((room_id,)) + self._attempt_to_invalidate_cache("_get_state_group_for_event", None) self._attempt_to_invalidate_cache("get_event_ordering", None) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index e5b93d8c0581..1831a5b47ab7 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -22,10 +22,10 @@ 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_acl_evaluator_from_event from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer +from synapse.storage.controllers.state import server_acl_evaluator_from_event from synapse.types import JsonDict from synapse.util import Clock From 8a936b01637ab4ff5b86f45cb99d7ba547b5edbd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 21 Sep 2023 14:17:17 -0400 Subject: [PATCH 08/12] Lint --- synapse/federation/federation_server.py | 11 +++++++---- synapse/storage/controllers/state.py | 4 +++- synapse/storage/databases/main/cache.py | 8 ++++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 9a46cc6e4392..f151a52e7454 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1324,10 +1324,13 @@ async def check_server_matches_acl(self, server_name: str, room_id: str) -> None Raises: AuthError if the server does not match the ACL """ - server_acl_evaluator = await self._storage_controllers.state.get_server_acl_for_room( - room_id) - if server_acl_evaluator and not server_acl_evaluator.server_matches_acl_event(server_name): - raise AuthError(code=403, msg="Server is banned from room") + server_acl_evaluator = ( + await self._storage_controllers.state.get_server_acl_for_room(room_id) + ) + if server_acl_evaluator and not server_acl_evaluator.server_matches_acl_event( + server_name + ): + raise AuthError(code=403, msg="Server is banned from room") class FederationHandlerRegistry: diff --git a/synapse/storage/controllers/state.py b/synapse/storage/controllers/state.py index edc71678d483..f6a667f42ab4 100644 --- a/synapse/storage/controllers/state.py +++ b/synapse/storage/controllers/state.py @@ -503,7 +503,9 @@ async def get_canonical_alias_for_room(self, room_id: str) -> Optional[str]: return event.content.get("alias") @cached() - async def get_server_acl_for_room(self, room_id: str) -> Optional[ServerAclEvaluator]: + async def get_server_acl_for_room( + self, room_id: str + ) -> Optional[ServerAclEvaluator]: """Get the server ACL evaluator for room, if any This does up-front parsing of the content to ignore bad data and pre-compile diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py index 7609f8edb208..f9daa35456da 100644 --- a/synapse/storage/databases/main/cache.py +++ b/synapse/storage/databases/main/cache.py @@ -330,7 +330,9 @@ def _invalidate_caches_for_event( ) if etype == EventTypes.ServerACL: - self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate((room_id,)) + self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate( + (room_id,) + ) if relates_to: self._attempt_to_invalidate_cache("get_relations_for_event", (relates_to,)) @@ -386,7 +388,9 @@ def _invalidate_caches_for_room_events(self, room_id: str) -> None: self._attempt_to_invalidate_cache("get_thread_participated", None) self._attempt_to_invalidate_cache("get_threads", (room_id,)) - self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate((room_id,)) + self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate( + (room_id,) + ) self._attempt_to_invalidate_cache("_get_state_group_for_event", None) From a67b3f8a921af0107c902f5b03bf35f17157b52b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 25 Sep 2023 10:01:55 -0400 Subject: [PATCH 09/12] Lint --- synapse/federation/federation_server.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index f151a52e7454..ec8e770430fc 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -84,11 +84,9 @@ from synapse.storage.databases.main.lock import Lock from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary from synapse.storage.roommember import MemberSummary -from synapse.synapse_rust.acl import ServerAclEvaluator from synapse.types import JsonDict, StateMap, get_domain_from_id from synapse.util import unwrapFirstError from synapse.util.async_helpers import Linearizer, concurrently_execute, gather_results -from synapse.util.caches.descriptors import cached from synapse.util.caches.response_cache import ResponseCache from synapse.util.stringutils import parse_server_name From 5e3931e3a865d76f043e4389aef6617cfdcaf8bc Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 26 Sep 2023 10:19:00 -0400 Subject: [PATCH 10/12] Review comments for unwrapping errors. --- rust/src/acl/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rust/src/acl/mod.rs b/rust/src/acl/mod.rs index f3e9b521ee1d..5afd973e9e4c 100644 --- a/rust/src/acl/mod.rs +++ b/rust/src/acl/mod.rs @@ -52,17 +52,17 @@ impl ServerAclEvaluator { #[new] pub fn py_new( allow_ip_literals: bool, - allow: Vec, - deny: Vec, + allow: Vec<&str>, + deny: Vec<&str>, ) -> Result { let allow = allow .iter() - .map(|s| glob_to_regex(s, GlobMatchType::Whole).unwrap()) - .collect(); + .map(|s| glob_to_regex(s, GlobMatchType::Whole)) + .collect::>()?; let deny = deny .iter() - .map(|s| glob_to_regex(s, GlobMatchType::Whole).unwrap()) - .collect(); + .map(|s| glob_to_regex(s, GlobMatchType::Whole)) + .collect::>()?; Ok(ServerAclEvaluator { allow_ip_literals, From 5039fb0f2d004533039ca0f6150047a3149e7229 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 26 Sep 2023 10:42:19 -0400 Subject: [PATCH 11/12] Break datastore -> storage controller link. --- synapse/handlers/federation_event.py | 6 ++++++ synapse/handlers/message.py | 5 +++++ synapse/replication/tcp/client.py | 6 ++++++ synapse/storage/databases/main/cache.py | 9 --------- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 7c62cdfaef5f..0cc8e990d96a 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -2342,6 +2342,12 @@ async def _notify_persisted_event( # TODO retrieve the previous state, and exclude join -> join transitions self._notifier.notify_user_joined_room(event.event_id, event.room_id) + # If this is a server ACL event, clear the cache in the storage controller. + if event.type == EventTypes.ServerACL: + self._state_storage_controller.get_server_acl_for_room.invalidate( + (event.room_id,) + ) + def _sanity_check_event(self, ev: EventBase) -> None: """ Do some early sanity checks of a received event diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index c036578a3dce..44dbbf81dd48 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1730,6 +1730,11 @@ async def persist_and_notify_client_events( event.event_id, event.room_id ) + if event.type == EventTypes.ServerACL: + self._storage_controllers.state.get_server_acl_for_room.invalidate( + (event.room_id,) + ) + await self._maybe_kick_guest_users(event, context) if event.type == EventTypes.CanonicalAlias: diff --git a/synapse/replication/tcp/client.py b/synapse/replication/tcp/client.py index ca8a76f77c39..1c7946522a5e 100644 --- a/synapse/replication/tcp/client.py +++ b/synapse/replication/tcp/client.py @@ -205,6 +205,12 @@ async def on_rdata( self.notifier.notify_user_joined_room( row.data.event_id, row.data.room_id ) + + # If this is a server ACL event, clear the cache in the storage controller. + if row.data.type == EventTypes.ServerACL: + self._state_storage_controller.get_server_acl_for_room.invalidate( + (row.data.room_id,) + ) elif stream_name == UnPartialStatedRoomStream.NAME: for row in rows: assert isinstance(row, UnPartialStatedRoomStreamRow) diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py index f9daa35456da..2fbd389c7168 100644 --- a/synapse/storage/databases/main/cache.py +++ b/synapse/storage/databases/main/cache.py @@ -329,11 +329,6 @@ def _invalidate_caches_for_event( "get_forgotten_rooms_for_user", (state_key,) ) - if etype == EventTypes.ServerACL: - self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate( - (room_id,) - ) - if relates_to: self._attempt_to_invalidate_cache("get_relations_for_event", (relates_to,)) self._attempt_to_invalidate_cache("get_references_for_event", (relates_to,)) @@ -388,10 +383,6 @@ def _invalidate_caches_for_room_events(self, room_id: str) -> None: self._attempt_to_invalidate_cache("get_thread_participated", None) self._attempt_to_invalidate_cache("get_threads", (room_id,)) - self.hs.get_storage_controllers().state.get_server_acl_for_room.invalidate( - (room_id,) - ) - self._attempt_to_invalidate_cache("_get_state_group_for_event", None) self._attempt_to_invalidate_cache("get_event_ordering", None) From 00c2721b54d49d9d897d7d6438925b6826cb80d2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 26 Sep 2023 10:45:42 -0400 Subject: [PATCH 12/12] Tweak comments. --- rust/src/acl/mod.rs | 2 +- synapse/storage/controllers/state.py | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/rust/src/acl/mod.rs b/rust/src/acl/mod.rs index 5afd973e9e4c..071f2b77321c 100644 --- a/rust/src/acl/mod.rs +++ b/rust/src/acl/mod.rs @@ -86,7 +86,7 @@ impl ServerAclEvaluator { } } - // next, check the deny list + // next, check the deny list if self.deny.iter().any(|e| e.is_match(server_name)) { return false; } diff --git a/synapse/storage/controllers/state.py b/synapse/storage/controllers/state.py index f6a667f42ab4..46957723a14c 100644 --- a/synapse/storage/controllers/state.py +++ b/synapse/storage/controllers/state.py @@ -792,18 +792,17 @@ def server_acl_evaluator_from_event(acl_event: EventBase) -> "ServerAclEvaluator """ Create a ServerAclEvaluator from a m.room.server_acl event's content. - This does up-front parsing of the content to ignore bad data and pre-compile - regular expressions. + This does up-front parsing of the content to ignore bad data. It then creates + the ServerAclEvaluator which will pre-compile regular expressions from the globs. """ - # first of all, check if literal IPs are blocked, and if so, whether the - # server name is a literal IP + # first of all, parse if literal IPs are blocked. 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 - # next, check the deny list + # next, parse the deny list by ignoring any non-strings. deny = acl_event.content.get("deny", []) if not isinstance(deny, (list, tuple)): logger.warning("Ignoring non-list deny ACL %s", deny)