From eb609230caca8e24db1f9adf7fc2f418e5e351bd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 7 Sep 2021 16:07:59 +0100 Subject: [PATCH 1/5] Assert that events in `on_receive_pdu` are not outliers We should never see any outliers here, so can simplify some code. --- synapse/handlers/federation_event.py | 125 +++++++++++++-------------- 1 file changed, 61 insertions(+), 64 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 69f8287b2bf0..c062497c5eed 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -173,6 +173,9 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None: pdu: received PDU """ + # We should never see any outliers here. + assert not pdu.internal_metadata.outlier + room_id = pdu.room_id event_id = pdu.event_id @@ -232,77 +235,71 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None: ) return None - # Check that the event passes auth based on the state at the event. This is - # done for events that are to be added to the timeline (non-outliers). - # - # Get missing pdus if necessary: - # - Fetching any missing prev events to fill in gaps in the graph - # - Fetching state if we have a hole in the graph - if not pdu.internal_metadata.is_outlier(): - prevs = set(pdu.prev_event_ids()) - seen = await self._store.have_events_in_timeline(prevs) - missing_prevs = prevs - seen + # Try to fetch any missing prev events to fill in gaps in the graph + prevs = set(pdu.prev_event_ids()) + seen = await self._store.have_events_in_timeline(prevs) + missing_prevs = prevs - seen - if missing_prevs: - # We only backfill backwards to the min depth. - min_depth = await self.get_min_depth_for_context(pdu.room_id) - logger.debug("min_depth: %d", min_depth) + if missing_prevs: + # We only backfill backwards to the min depth. + min_depth = await self.get_min_depth_for_context(pdu.room_id) + logger.debug("min_depth: %d", min_depth) - if min_depth is not None and pdu.depth > min_depth: - # If we're missing stuff, ensure we only fetch stuff one - # at a time. + if min_depth is not None and pdu.depth > min_depth: + # If we're missing stuff, ensure we only fetch stuff one + # at a time. + logger.info( + "Acquiring room lock to fetch %d missing prev_events: %s", + len(missing_prevs), + shortstr(missing_prevs), + ) + with (await self._room_pdu_linearizer.queue(pdu.room_id)): logger.info( - "Acquiring room lock to fetch %d missing prev_events: %s", + "Acquired room lock to fetch %d missing prev_events", len(missing_prevs), - shortstr(missing_prevs), ) - with (await self._room_pdu_linearizer.queue(pdu.room_id)): - logger.info( - "Acquired room lock to fetch %d missing prev_events", - len(missing_prevs), + + try: + await self._get_missing_events_for_pdu( + origin, pdu, prevs, min_depth ) + except Exception as e: + raise Exception( + "Error fetching missing prev_events for %s: %s" + % (event_id, e) + ) from e - try: - await self._get_missing_events_for_pdu( - origin, pdu, prevs, min_depth - ) - except Exception as e: - raise Exception( - "Error fetching missing prev_events for %s: %s" - % (event_id, e) - ) from e - - # Update the set of things we've seen after trying to - # fetch the missing stuff - seen = await self._store.have_events_in_timeline(prevs) - missing_prevs = prevs - seen - - if not missing_prevs: - logger.info("Found all missing prev_events") - - if missing_prevs: - # since this event was pushed to us, it is possible for it to - # become the only forward-extremity in the room, and we would then - # trust its state to be the state for the whole room. This is very - # bad. Further, if the event was pushed to us, there is no excuse - # for us not to have all the prev_events. (XXX: apart from - # min_depth?) - # - # We therefore reject any such events. - logger.warning( - "Rejecting: failed to fetch %d prev events: %s", - len(missing_prevs), - shortstr(missing_prevs), - ) - raise FederationError( - "ERROR", - 403, - ( - "Your server isn't divulging details about prev_events " - "referenced in this event." - ), - affected=pdu.event_id, - ) + # Update the set of things we've seen after trying to + # fetch the missing stuff + seen = await self._store.have_events_in_timeline(prevs) + missing_prevs = prevs - seen + + if not missing_prevs: + logger.info("Found all missing prev_events") + + if missing_prevs: + # since this event was pushed to us, it is possible for it to + # become the only forward-extremity in the room, and we would then + # trust its state to be the state for the whole room. This is very + # bad. Further, if the event was pushed to us, there is no excuse + # for us not to have all the prev_events. (XXX: apart from + # min_depth?) + # + # We therefore reject any such events. + logger.warning( + "Rejecting: failed to fetch %d prev events: %s", + len(missing_prevs), + shortstr(missing_prevs), + ) + raise FederationError( + "ERROR", + 403, + ( + "Your server isn't divulging details about prev_events " + "referenced in this event." + ), + affected=pdu.event_id, + ) await self._process_received_pdu(origin, pdu, state=None) From f3ba2421a99f8980cf27fecd4bc30ef43952654e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 7 Sep 2021 16:21:18 +0100 Subject: [PATCH 2/5] Assert that events in _process_received_pdu are not outliers It's only called from `on_receive_pdu` and `_process_pulled_event`, neither of which handle outliers, and in any case it doesn't make sense to do the magic encrypted event or marker event handling for outliers. --- synapse/handlers/federation_event.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index c062497c5eed..c04cc635b0f1 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -882,8 +882,13 @@ async def _process_received_pdu( state: Optional[Iterable[EventBase]], backfilled: bool = False, ) -> None: - """Called when we have a new pdu. We need to do auth checks and put it - through the StateHandler. + """Called when we have a new non-outlier event. + + This is called when we have a new event to add to the room DAG - either directly + via a /send request, retrieved via get_missing_events after a /send request, or + backfilled after a client request. + + We need to do auth checks and put it through the StateHandler. Args: origin: server sending the event @@ -898,6 +903,7 @@ async def _process_received_pdu( notification to clients, and validation of device keys.) """ logger.debug("Processing event: %s", event) + assert not event.internal_metadata.outlier try: context = await self._state_handler.compute_event_context( From e941b0f687e61705dc186e345d10d746c42d344f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 7 Sep 2021 16:25:59 +0100 Subject: [PATCH 3/5] add an assertion in _auth_and_persist_event `_auth_and_persist_event` is called in two places: * `_process_received_pdu`, which handles non-outliers and does *not* set `claimed_auth_event_map` * `_update_auth_events_and_context_for_auth`, which handles outliers and *does* set `claimed_auth_event_map` --- synapse/handlers/federation_event.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index c04cc635b0f1..82871c0c6c37 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1237,11 +1237,13 @@ async def _auth_and_persist_event( Possibly incomplete, and possibly including events that are not yet persisted, or authed, or in the right room. - Only populated where we may not already have persisted these events - - for example, when populating outliers. + Only populated when populating outliers. backfilled: True if the event was backfilled. """ + # claimed_auth_event_map should be given iff the event is an outlier + assert bool(claimed_auth_event_map) == event.internal_metadata.outlier + context = await self._check_event_auth( origin, event, From 21be8aed1892f3b13740ca5e649897034357fab5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 7 Sep 2021 16:29:36 +0100 Subject: [PATCH 4/5] add an assertion in `check_event_auth` `check_event_auth` is called in three places: * `_auth_and_persist_event`, which now has the same assertion * `on_send_membership_event`, which handles non-outliers (and asserts that), and does *not* set `claimed_auth_event_map`. * `_auth_and_persist_fetched_events`, which handles outliers, and *does* set `claimed_auth_event_map`. --- synapse/handlers/federation_event.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 82871c0c6c37..30c2c8738e28 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1282,15 +1282,16 @@ async def _check_event_auth( Possibly incomplete, and possibly including events that are not yet persisted, or authed, or in the right room. - Only populated where we may not already have persisted these events - - for example, when populating outliers, or the state for a backwards - extremity. + 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 + room_version = await self._store.get_room_version_id(event.room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] From 05ed6bc19712b6a80a98b13f7f2e42a35f7b2b0f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 7 Sep 2021 16:35:04 +0100 Subject: [PATCH 5/5] changelog --- changelog.d/10773.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10773.misc diff --git a/changelog.d/10773.misc b/changelog.d/10773.misc new file mode 100644 index 000000000000..9a765435dbe4 --- /dev/null +++ b/changelog.d/10773.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity.