From 299da1f5c8812d49f5d391874cfc0cc7917fb359 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 20 Sep 2021 18:40:15 +0100 Subject: [PATCH 1/5] Inline `_check_event_auth` for outliers When we are persisting an outlier, most of `_check_event_auth` is redundant: * `_update_auth_events_and_context_for_auth` does nothing, because the `input_auth_events` are (now) exactly the event's auth_events, which means that `missing_auth` is empty. * we don't care about soft-fail, kicking guest users or `send_on_behalf_of` for outliers ... so the only thing that matters is the auth itself, so let's just do that. --- synapse/handlers/federation_event.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 01fd84112252..a6b0de286cee 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1189,6 +1189,9 @@ async def _auth_and_persist_fetched_events_inner( allow_rejected=True, ) + room_version = await self._store.get_room_version_id(room_id) + room_version_obj = KNOWN_ROOM_VERSIONS[room_version] + async def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: with nested_logging_context(suffix=event.event_id): auth = {} @@ -1207,12 +1210,12 @@ async def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: auth[(ae.type, ae.state_key)] = ae context = EventContext.for_outlier() - context = await self._check_event_auth( - origin, - event, - context, - claimed_auth_event_map=auth, - ) + try: + event_auth.check(room_version_obj, event, auth_events=auth) + except AuthError as e: + logger.warning("Rejecting %r because %s", event, e) + context.rejected = RejectedReason.AUTH_ERROR + return event, context events_to_persist = ( From 0f7750b672dcfbe765f2da3e388539e317507caa Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Sep 2021 18:37:14 +0100 Subject: [PATCH 2/5] `_auth_and_persist_fetched_events_inner`: de-async `prep` `prep` no longer calls any `async` methods, so let's make it synchronous. --- synapse/handlers/federation_event.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index a6b0de286cee..8e3e802f2572 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1192,7 +1192,7 @@ async def _auth_and_persist_fetched_events_inner( room_version = await self._store.get_room_version_id(room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] - async def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: + def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: with nested_logging_context(suffix=event.event_id): auth = {} for auth_event_id in event.auth_event_ids(): @@ -1218,9 +1218,7 @@ async def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: return event, context - events_to_persist = ( - x for x in await yieldable_gather_results(prep, fetched_events) if x - ) + events_to_persist = (x for x in (prep(event) for event in fetched_events) if x) await self.persist_events_and_notify(room_id, tuple(events_to_persist)) async def _check_event_auth( From a9e228e1399546cf5a1bdcedc7239aa0706a3eef Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 20 Sep 2021 18:44:32 +0100 Subject: [PATCH 3/5] Simplify `_check_event_auth` We no longer need to support outliers here, which makes things rather simpler. --- synapse/handlers/federation_event.py | 66 ++++++++++------------------ tests/test_federation.py | 1 - 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 8e3e802f2572..8ae607a33fe8 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1227,7 +1227,6 @@ async def _check_event_auth( event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - claimed_auth_event_map: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> EventContext: """ @@ -1243,42 +1242,36 @@ async def _check_event_auth( The state events used to check the event for soft-fail. If this is not provided the current state events will be used. - claimed_auth_event_map: - A map of (type, state_key) => event for the event's claimed auth_events. - Possibly including events that were rejected, or are in the wrong room. - - Only populated when populating outliers. - backfilled: True if the event was backfilled. Returns: The updated context object. """ - # claimed_auth_event_map should be given iff the event is an outlier - assert bool(claimed_auth_event_map) == event.internal_metadata.outlier + # This method should only be used for non-outliers + assert not event.internal_metadata.outlier room_version = await self._store.get_room_version_id(event.room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] - if claimed_auth_event_map: - # if we have a copy of the auth events from the event, use that as the - # basis for auth. - auth_events = claimed_auth_event_map - else: - # otherwise, we calculate what the auth events *should* be, and use that - prev_state_ids = await context.get_prev_state_ids() - auth_events_ids = self._event_auth_handler.compute_auth_events( - event, prev_state_ids, for_verification=True - ) - auth_events_x = await self._store.get_events(auth_events_ids) - auth_events = {(e.type, e.state_key): e for e in auth_events_x.values()} + # calculate what the auth events *should* be, to use as a basis for auth. + prev_state_ids = await context.get_prev_state_ids() + auth_events_ids = self._event_auth_handler.compute_auth_events( + event, prev_state_ids, for_verification=True + ) + auth_events_x = await self._store.get_events(auth_events_ids) + calculated_auth_event_map = { + (e.type, e.state_key): e for e in auth_events_x.values() + } try: ( context, auth_events_for_auth, ) = await self._update_auth_events_and_context_for_auth( - origin, event, context, auth_events + origin, + event, + context, + calculated_auth_event_map=calculated_auth_event_map, ) except Exception: # We don't really mind if the above fails, so lets not fail @@ -1290,7 +1283,7 @@ async def _check_event_auth( "Ignoring failure and continuing processing of event.", event.event_id, ) - auth_events_for_auth = auth_events + auth_events_for_auth = calculated_auth_event_map try: event_auth.check(room_version_obj, event, auth_events=auth_events_for_auth) @@ -1426,7 +1419,7 @@ async def _update_auth_events_and_context_for_auth( origin: str, event: EventBase, context: EventContext, - input_auth_events: StateMap[EventBase], + calculated_auth_event_map: StateMap[EventBase], ) -> Tuple[EventContext, StateMap[EventBase]]: """Helper for _check_event_auth. See there for docs. @@ -1444,19 +1437,17 @@ async def _update_auth_events_and_context_for_auth( event: context: - input_auth_events: - Map from (event_type, state_key) to event - - Normally, our calculated auth_events based on the state of the room - at the event's position in the DAG, though occasionally (eg if the - event is an outlier), may be the auth events claimed by the remote - server. + calculated_auth_event_map: + Our calculated auth_events based on the state of the room + at the event's position in the DAG. Returns: updated context, updated auth event map """ - # take a copy of input_auth_events before we modify it. - auth_events: MutableStateMap[EventBase] = dict(input_auth_events) + assert not event.internal_metadata.outlier + + # take a copy of calculated_auth_event_map before we modify it. + auth_events: MutableStateMap[EventBase] = dict(calculated_auth_event_map) event_auth_events = set(event.auth_event_ids()) @@ -1497,15 +1488,6 @@ async def _update_auth_events_and_context_for_auth( } ) - if event.internal_metadata.is_outlier(): - # XXX: given that, for an outlier, we'll be working with the - # event's *claimed* auth events rather than those we calculated: - # (a) is there any point in this test, since different_auth below will - # obviously be empty - # (b) alternatively, why don't we do it earlier? - logger.info("Skipping auth_event fetch for outlier") - return context, auth_events - different_auth = event_auth_events.difference( e.event_id for e in auth_events.values() ) diff --git a/tests/test_federation.py b/tests/test_federation.py index c51e018da1a1..24fc77d7a772 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -82,7 +82,6 @@ async def _check_event_auth( event, context, state=None, - claimed_auth_event_map=None, backfilled=False, ): return context From 2e7d8d7e7df2306a9ada14615d047d64e26e4f0f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 27 Sep 2021 14:58:09 +0100 Subject: [PATCH 4/5] changelog --- changelog.d/10896.misc | 2 +- changelog.d/10926.misc | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/10926.misc diff --git a/changelog.d/10896.misc b/changelog.d/10896.misc index 41de99584239..9a765435dbe4 100644 --- a/changelog.d/10896.misc +++ b/changelog.d/10896.misc @@ -1 +1 @@ - Clean up some of the federation event authentication code for clarity. +Clean up some of the federation event authentication code for clarity. diff --git a/changelog.d/10926.misc b/changelog.d/10926.misc new file mode 100644 index 000000000000..9a765435dbe4 --- /dev/null +++ b/changelog.d/10926.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity. From def567dc7ea8e9471a29316b9d3639a9cad87496 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 27 Sep 2021 16:21:12 +0100 Subject: [PATCH 5/5] lint --- synapse/handlers/federation_event.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 8ae607a33fe8..2c4644b4a32d 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -68,11 +68,7 @@ UserID, get_domain_from_id, ) -from synapse.util.async_helpers import ( - Linearizer, - concurrently_execute, - yieldable_gather_results, -) +from synapse.util.async_helpers import Linearizer, concurrently_execute from synapse.util.iterutils import batch_iter from synapse.util.retryutils import NotRetryingDestination from synapse.util.stringutils import shortstr