From aacdce8fc0e0997c58a8415e1a4283375906212d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 8 Sep 2021 10:41:13 +0100 Subject: [PATCH] Add some assertions about outliers (#10773) I think I have finally teased apart the codepaths which handle outliers, and those that handle non-outliers. Let's add some assertions to demonstrate my newfound knowledge. --- changelog.d/10773.misc | 1 + synapse/handlers/federation_event.py | 158 ++++++++++++++------------- 2 files changed, 83 insertions(+), 76 deletions(-) create mode 100644 changelog.d/10773.misc diff --git a/changelog.d/10773.misc b/changelog.d/10773.misc new file mode 100644 index 000000000..9a765435d --- /dev/null +++ b/changelog.d/10773.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity. diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 9e188bb51..ccbfce021 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -173,6 +173,9 @@ class FederationEventHandler: 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 @@ class FederationEventHandler: ) 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 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( + "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 + + # 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: - # 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. - 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( - "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 - - # 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, - ) + # 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) @@ -885,8 +882,13 @@ class FederationEventHandler: 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 @@ -901,6 +903,7 @@ class FederationEventHandler: 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( @@ -1263,11 +1266,13 @@ class FederationEventHandler: 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, @@ -1306,15 +1311,16 @@ class FederationEventHandler: 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]