diff --git a/changelog.d/19921.feature b/changelog.d/19921.feature new file mode 100644 index 00000000000..61ddaeedd45 --- /dev/null +++ b/changelog.d/19921.feature @@ -0,0 +1 @@ +Add an `exclude_rooms_from_device_list_updates` configuration option to curb device-list update fan-out in large unencrypted rooms (encrypted rooms are ignored to preserve end-to-end encryption). diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 92eca4a7ffd..ae35acaf402 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4292,6 +4292,16 @@ exclude_rooms_from_sync: - '!foo:example.com' ``` --- +### `exclude_rooms_from_device_list_updates` + +*(array)* A list of rooms to exclude from device-list update fan-out. Device-list updates (login/logout, new device, key rotation) are normally sent to every user who shares a room with the changed user, and federated to every server in those rooms. For a very large room this fan-out can be expensive, waking many `/sync` streams and triggering `/keys/query` re-fetch storms. This option is intended for large, unencrypted, auto-joined "directory" rooms used only to make users mutually visible (e.g. in the user directory), where device-list tracking provides no benefit. **This option only takes effect for _unencrypted_ rooms.** Device-list tracking exists solely to serve end-to-end encryption, so excluding an encrypted room would break E2EE (peers would stop being notified of each other's key/device changes, leading to failed decryption and sending to stale devices). Any encrypted room listed here is therefore silently ignored and continues to receive device-list updates as normal. Defaults to `[]`. + +Example configuration: +```yaml +exclude_rooms_from_device_list_updates: +- '!bigdirectory:example.com' +``` +--- ## Opentracing Configuration options related to Opentracing support. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index 0e345b7b69d..b176d5735f9 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -5314,6 +5314,28 @@ properties: default: [] examples: - - "!foo:example.com" + exclude_rooms_from_device_list_updates: + type: array + description: >- + A list of rooms to exclude from device-list update fan-out. Device-list + updates (login/logout, new device, key rotation) are normally sent to + every user who shares a room with the changed user, and federated to every + server in those rooms. For a very large room this fan-out can be + expensive, waking many `/sync` streams and triggering `/keys/query` + re-fetch storms. This option is intended for large, unencrypted, + auto-joined "directory" rooms used only to make users mutually visible + (e.g. in the user directory), where device-list tracking provides no + benefit. **This option only takes effect for _unencrypted_ rooms.** + Device-list tracking exists solely to serve end-to-end encryption, so + excluding an encrypted room would break E2EE (peers would stop being + notified of each other's key/device changes, leading to failed decryption + and sending to stale devices). Any encrypted room listed here is therefore + silently ignored and continues to receive device-list updates as normal. + items: + type: string + default: [] + examples: + - - "!bigdirectory:example.com" opentracing: type: object description: >- diff --git a/synapse/config/server.py b/synapse/config/server.py index ca94c224ea5..37e060c5f1c 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -896,6 +896,10 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: config.get("exclude_rooms_from_sync") or [] ) + self.rooms_to_exclude_from_device_list_updates: list[str] = ( + config.get("exclude_rooms_from_device_list_updates") or [] + ) + delete_stale_devices_after: str | None = ( config.get("delete_stale_devices_after") or None ) diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index 22254666487..b483e5d0fbe 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -139,6 +139,14 @@ def __init__(self, hs: "HomeServer"): hs.config.registration.dont_notify_new_devices_for ) + # Rooms configured to be excluded from device-list update fan-out. Note + # that this is only honoured for *unencrypted* rooms: excluding an + # encrypted room would break E2EE, so encrypted rooms in this set are + # silently kept (see `_filter_device_list_excluded_rooms`). + self._rooms_to_exclude_from_device_list_updates = frozenset( + hs.config.server.rooms_to_exclude_from_device_list_updates + ) + self.device_list_updater = DeviceListWorkerUpdater(hs) self._task_scheduler.register_action( @@ -517,6 +525,47 @@ async def get_device(self, user_id: str, device_id: str) -> JsonDict: return device + async def _filter_device_list_excluded_rooms( + self, room_ids: StrCollection + ) -> set[str]: + """Drop rooms configured to be excluded from device-list updates, but + ONLY if they are not encrypted. + + Device-list tracking exists solely to serve end-to-end encryption, so it + is only safe to stop tracking a room if it is unencrypted. Encrypted + rooms in the exclusion set are therefore silently kept to preserve E2EE. + + Args: + room_ids: The rooms to filter. + + Returns: + The room IDs with excluded (and unencrypted) rooms removed. + """ + excluded = self._rooms_to_exclude_from_device_list_updates + result = set(room_ids) + if not excluded: + # Fast path: nothing configured, so nothing to filter. Avoids any + # per-room encryption lookups. + return result + + # Only rooms that are BOTH configured-excluded AND unencrypted may be + # dropped: encrypted rooms must keep flowing device-list updates or E2EE + # breaks. Look the encryption up in a single bulk (cached) call for just + # the candidate rooms rather than per-room, since `get_room_encryption` + # is a `@cachedList` stub that only resolves via `bulk_get_room_encryption`. + candidates = result & excluded + if not candidates: + return result + + encryption = await self.store.bulk_get_room_encryption(candidates) + for room_id in candidates: + # Drop only when we positively know the room is unencrypted (`None`). + # An encryption algorithm (encrypted) or the room-unknown sentinel are + # both kept, so we never silently stop tracking an encrypted room. + if encryption.get(room_id) is None: + result.discard(room_id) + return result + @cancellable async def get_device_changes_in_shared_rooms( self, @@ -550,13 +599,18 @@ async def get_device_changes_in_shared_rooms( return changed_users # If the DB returned None then the `from_token` is too old, so we fall - # back on looking for device updates for all users. - - users_who_share_room = await self.store.get_users_who_share_room_with_user( - user_id - ) - - tracked_users = set(users_who_share_room) + # back on looking for device updates for all users we share a room with. + # + # We derive the tracked users from the `room_ids` we were passed (which + # the caller has already filtered through + # `_filter_device_list_excluded_rooms`) rather than + # `get_users_who_share_room_with_user`, which would recompute the user's + # rooms from scratch and re-widen back to every shared-room peer, + # including those in excluded rooms. + tracked_users: set[str] = set() + for room_id in room_ids: + user_ids = await self.store.get_users_in_room(room_id) + tracked_users.update(user_ids) # Always tell the user about their own devices tracked_users.add(user_id) @@ -590,6 +644,13 @@ async def get_user_ids_changed( joined_room_ids = await self.store.get_rooms_for_user(user_id) + # Drop any rooms configured to be excluded from device-list updates + # (unencrypted rooms only) so we don't fan out re-fetches of device + # keys for peers we only share such a room with. + joined_room_ids = await self._filter_device_list_excluded_rooms( + joined_room_ids + ) + # Get the set of rooms that the user has joined/left membership_changes = ( await self.store.get_current_state_delta_membership_changes_for_user( @@ -744,10 +805,22 @@ async def generate_sync_entry_for_device_list( users_that_have_changed = set() # Step 1a, check for changes in devices of users we share a room - # with + # with. + # + # Drop any rooms configured to be excluded from device-list updates + # (unencrypted rooms only) from the recurring shared-rooms computation, + # so this matches `get_user_ids_changed` and the other device-list + # endpoints (notably on the too-old-token fallback in + # `get_device_changes_in_shared_rooms`). We filter narrowly here rather + # than the `joined_room_ids` parameter itself, as the latter is also + # used below to decide which newly-left users to keep tracking, which we + # intentionally leave unfiltered. + shared_room_ids = await self._filter_device_list_excluded_rooms( + joined_room_ids + ) users_that_have_changed = await self.get_device_changes_in_shared_rooms( user_id, - joined_room_ids, + shared_room_ids, from_token=since_token, now_token=now_token, ) @@ -978,6 +1051,14 @@ async def notify_device_update( room_ids = await self.store.get_rooms_for_user(user_id) + # Drop any rooms configured to be excluded from device-list updates + # (unencrypted rooms only). Doing this before recording the change means + # the change is neither recorded against these rooms nor federated to + # their hosts (federation destinations are derived from the recorded + # `device_lists_changes_in_room` rows), and local members who only share + # an excluded room won't be woken. + room_ids = await self._filter_device_list_excluded_rooms(room_ids) + position = await self.store.add_device_change_to_streams( user_id, device_ids, diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py index cb047d118a7..8bd8b16a7b9 100644 --- a/tests/handlers/test_device.py +++ b/tests/handlers/test_device.py @@ -45,7 +45,7 @@ from synapse.federation.units import Transaction from synapse.handlers.device import MAX_DEVICE_DISPLAY_NAME_LEN, DeviceWriterHandler from synapse.rest import admin -from synapse.rest.client import devices, login, register +from synapse.rest.client import devices, login, register, room from synapse.server import HomeServer from synapse.storage.databases.main.appservice import _make_exclusive_regex from synapse.types import ( @@ -499,6 +499,161 @@ def test_delete_device_removes_refresh_tokens(self) -> None: self.assertIsNone(remaining_refresh_token) +class DeviceListExcludedRoomsTestCase(unittest.HomeserverTestCase): + """Tests for `exclude_rooms_from_device_list_updates`. + + A configured room should be dropped from device-list update fan-out, but + ONLY if it is unencrypted: excluding an encrypted room would break E2EE, so + encrypted rooms in the exclusion set must keep being tracked. + """ + + servlets = [ + admin.register_servlets, + login.register_servlets, + register.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + handler = hs.get_device_handler() + assert isinstance(handler, DeviceWriterHandler) + self.handler = handler + self.store = hs.get_datastores().main + self.event_sources = hs.get_event_sources() + + self.user1 = self.register_user("user1", "pass") + self.user1_tok = self.login(self.user1, "pass", device_id="user1device") + self.user2 = self.register_user("user2", "pass") + self.user2_tok = self.login(self.user2, "pass", device_id="user2device") + + def _create_shared_room(self, encrypted: bool = False) -> str: + """Create a room owned by user1 that user2 also joins.""" + room_id = self.helper.create_room_as(self.user1, tok=self.user1_tok) + if encrypted: + self.helper.send_state( + room_id, + EventTypes.RoomEncryption, + {"algorithm": RoomEncryptionAlgorithms.MEGOLM_V1_AES_SHA2}, + tok=self.user1_tok, + ) + self.helper.join(room_id, self.user2, tok=self.user2_tok) + return room_id + + def _changed_users_for_user2(self) -> set: + """Have user1 change their device, then return the set of users that + user2's device-list stream reports as changed since just before.""" + from_token = self.event_sources.get_current_token() + + self.get_success( + self.handler.notify_device_update(self.user1, ["user1device"]) + ) + + result = self.get_success( + self.handler.get_user_ids_changed(self.user2, from_token) + ) + return set(result.changed) + + def test_change_in_shared_room_is_notified(self) -> None: + """Baseline: with no exclusion, a peer sharing a room IS notified.""" + self._create_shared_room() + + self.assertIn(self.user1, self._changed_users_for_user2()) + + def test_excluded_unencrypted_room_is_not_notified(self) -> None: + """A change in an excluded, unencrypted room does NOT notify a peer who + only shares that room.""" + room_id = self._create_shared_room(encrypted=False) + self.handler._rooms_to_exclude_from_device_list_updates = frozenset( + {room_id} + ) + + self.assertNotIn(self.user1, self._changed_users_for_user2()) + + def test_non_excluded_room_still_notified(self) -> None: + """A peer sharing a *non-excluded* room IS still notified, even when + another (unshared) room is excluded.""" + room_id = self._create_shared_room(encrypted=False) + self.handler._rooms_to_exclude_from_device_list_updates = frozenset( + {"!someotherroom:test"} + ) + # Sanity check the shared room isn't the excluded one. + self.assertNotEqual(room_id, "!someotherroom:test") + + self.assertIn(self.user1, self._changed_users_for_user2()) + + def test_excluded_but_encrypted_room_is_still_notified(self) -> None: + """If the excluded room is ENCRYPTED, the peer IS still notified: the + guard preserves E2EE.""" + room_id = self._create_shared_room(encrypted=True) + # Ensure the encryption state is visible via the bulk (cached) lookup + # the handler actually uses (`get_room_encryption` is a stub). + self.assertIsNotNone( + self.get_success(self.store.bulk_get_room_encryption({room_id})).get( + room_id + ) + ) + self.handler._rooms_to_exclude_from_device_list_updates = frozenset( + {room_id} + ) + + self.assertIn(self.user1, self._changed_users_for_user2()) + + def _changed_via_classic_sync_fallback(self, joined_room_ids: set) -> set: + """Run `generate_sync_entry_for_device_list` (the classic /sync path) + with the too-old-token fallback in `get_device_changes_in_shared_rooms` + forced, and return the reported changed users for user2.""" + from_token = self.event_sources.get_current_token() + # Record a device change for user1 so they appear in the per-user + # `device_lists_stream` that the fallback consults. + self.get_success( + self.handler.notify_device_update(self.user1, ["user1device"]) + ) + now_token = self.event_sources.get_current_token() + + # Force `get_device_list_changes_in_rooms` to report the token as too + # old, dropping into the fallback code path. + with patch.object( + self.store, + "get_device_list_changes_in_rooms", + AsyncMock(return_value=None), + ): + result = self.get_success( + self.handler.generate_sync_entry_for_device_list( + user_id=self.user2, + since_token=from_token, + now_token=now_token, + joined_room_ids=joined_room_ids, + newly_joined_rooms=set(), + newly_joined_or_invited_or_knocked_users=set(), + newly_left_rooms=set(), + newly_left_users=set(), + ) + ) + return set(result.changed) + + def test_classic_sync_fallback_excludes_unencrypted_room(self) -> None: + """The classic /sync path (`generate_sync_entry_for_device_list`) passes + an *unfiltered* joined-rooms set, so the filter must be applied inside + that method. On the too-old-token fallback a peer sharing only an + excluded, unencrypted room must NOT be reported as changed.""" + room_id = self._create_shared_room(encrypted=False) + + # Sanity: without exclusion, the fallback DOES surface user1 (otherwise + # the assertion below would pass vacuously). + self.handler._rooms_to_exclude_from_device_list_updates = frozenset() + self.assertIn( + self.user1, self._changed_via_classic_sync_fallback({room_id}) + ) + + # With the room excluded (and unencrypted), user1 is filtered out. + self.handler._rooms_to_exclude_from_device_list_updates = frozenset( + {room_id} + ) + self.assertNotIn( + self.user1, self._changed_via_classic_sync_fallback({room_id}) + ) + + class DehydrationTestCase(unittest.HomeserverTestCase): servlets = [ admin.register_servlets_for_client_rest_resource,