From e963a55eedd3a8419a7af61d608143686e77fc0e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 10:50:22 +0100 Subject: [PATCH 1/8] Fix recursion error when fetching auth chain When fetching the state of a room over federation we receive the event IDs of the state and auth chain. We then fetch those events that we don't already have. However, we used a function that recursively fetched any missing auth events for the fetched events, which can lead to a lot of recursion if the server is missing most of the auth chain. This work is entirely pointless because would have queued up the missing events in the auth chain to be fetched already. Let's just diable the recursion, since it only gets called from one place anyway. --- synapse/handlers/federation.py | 41 ++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ca7da42a3fd6..ef92d35e7b2b 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -618,6 +618,10 @@ async def _get_events_from_store_or_dest( will be omitted from the result. Likewise, any events which turn out not to be in the given room. + This function *does not* recursively get missing auth events of the + newly fetched events. Callers must include any missing events from the + auth chain. + Returns: map from event_id to event """ @@ -1131,12 +1135,16 @@ async def _get_events_and_persist( ): """Fetch the given events from a server, and persist them as outliers. + This function *does not* recursively get missing auth events of the + newly fetched events. Callers must include any missing events from the + auth chain. + Logs a warning if we can't find the given event. """ room_version = await self.store.get_room_version(room_id) - event_infos = [] + event_map = {} # type: Dict[str, EventBase] async def get_event(event_id: str): with nested_logging_context(event_id): @@ -1150,17 +1158,7 @@ async def get_event(event_id: str): ) return - # recursively fetch the auth events for this event - auth_events = await self._get_events_from_store_or_dest( - destination, room_id, event.auth_event_ids() - ) - auth = {} - for auth_event_id in event.auth_event_ids(): - ae = auth_events.get(auth_event_id) - if ae: - auth[(ae.type, ae.state_key)] = ae - - event_infos.append(_NewEventInfo(event, None, auth)) + event_map[event.event_id] except Exception as e: logger.warning( @@ -1172,6 +1170,25 @@ async def get_event(event_id: str): await concurrently_execute(get_event, events, 5) + # Make a map of auth events for each event. We do this after fetching + # all the events as some of the events auth events will be in the list + # of requested events. + + persisted_events = await self.store.get_events( + (event.auth_event_ids() for event in event_map.values()), + allow_rejected=True, + ) + + event_infos = [] + for event in event_map.values(): + auth = {} + for auth_event_id in event.auth_event_ids(): + ae = persisted_events.get(auth_event_id) or event_map.get(auth_event_id) + if ae: + auth[(ae.type, ae.state_key)] = ae + + event_infos.append(_NewEventInfo(event, None, auth)) + await self._handle_new_events( destination, event_infos, ) From 2b3916dde8c4ec37d3de03ff6a4cb1bb3edd3d7f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 10:56:45 +0100 Subject: [PATCH 2/8] Newsfile --- changelog.d/7817.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7817.bugfix diff --git a/changelog.d/7817.bugfix b/changelog.d/7817.bugfix new file mode 100644 index 000000000000..1c001070d503 --- /dev/null +++ b/changelog.d/7817.bugfix @@ -0,0 +1 @@ +Fix bug where Synapse fails to process an incoming event over federation if the server is missing too much of the event's auth chain. From cad85d5fbf986295dcfa77822bf0d55912ceca3a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 11:31:53 +0100 Subject: [PATCH 3/8] Review comments --- synapse/handlers/federation.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ef92d35e7b2b..7cf7292e4b4b 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -619,8 +619,8 @@ async def _get_events_from_store_or_dest( be in the given room. This function *does not* recursively get missing auth events of the - newly fetched events. Callers must include any missing events from the - auth chain. + newly fetched events. Callers must include in the `event_ids` argument + any missing events from the auth chain. Returns: map from event_id to event @@ -1136,8 +1136,8 @@ async def _get_events_and_persist( """Fetch the given events from a server, and persist them as outliers. This function *does not* recursively get missing auth events of the - newly fetched events. Callers must include any missing events from the - auth chain. + newly fetched events. Callers must include in the `event_ids` argument + any missing events from the auth chain. Logs a warning if we can't find the given event. """ @@ -1158,7 +1158,7 @@ async def get_event(event_id: str): ) return - event_map[event.event_id] + event_map[event.event_id] = event except Exception as e: logger.warning( @@ -1171,7 +1171,7 @@ async def get_event(event_id: str): await concurrently_execute(get_event, events, 5) # Make a map of auth events for each event. We do this after fetching - # all the events as some of the events auth events will be in the list + # all the events as some of the events' auth events will be in the list # of requested events. persisted_events = await self.store.get_events( From 7825a59506a2cddc58b34e8c4e66200233def364 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 11:32:17 +0100 Subject: [PATCH 4/8] Fix tests --- synapse/handlers/federation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 7cf7292e4b4b..ab54ffd67c52 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1175,7 +1175,7 @@ async def get_event(event_id: str): # of requested events. persisted_events = await self.store.get_events( - (event.auth_event_ids() for event in event_map.values()), + (aid for event in event_map.values() for aid in event.auth_event_ids()), allow_rejected=True, ) From c6df9ac28aa7de31f73922284023959435c9c8e2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 12:48:01 +0100 Subject: [PATCH 5/8] get_events requires a list... --- synapse/handlers/federation.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ab54ffd67c52..c76c77b32709 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1174,9 +1174,14 @@ async def get_event(event_id: str): # all the events as some of the events' auth events will be in the list # of requested events. + auth_events = [ + aid + for event in event_map.values() + for aid in event.auth_event_ids() + if aid not in event_map + ] persisted_events = await self.store.get_events( - (aid for event in event_map.values() for aid in event.auth_event_ids()), - allow_rejected=True, + auth_events, allow_rejected=True, ) event_infos = [] @@ -1186,6 +1191,8 @@ async def get_event(event_id: str): ae = persisted_events.get(auth_event_id) or event_map.get(auth_event_id) if ae: auth[(ae.type, ae.state_key)] = ae + else: + logger.info("Missing auth event %s", auth_event_id) event_infos.append(_NewEventInfo(event, None, auth)) From f6856d8e05a9bfb22732538ca7315276a4942ece Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 16:26:47 +0100 Subject: [PATCH 6/8] Make exception autherror in check auth --- synapse/event_auth.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index c58235514650..129b72182723 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -69,10 +69,11 @@ def check( # sanity-check it for auth_event in auth_events.values(): if auth_event.room_id != room_id: - raise Exception( + raise AuthError( + 500, "During auth for event %s in room %s, found event %s in the state " "which is in room %s" - % (event.event_id, room_id, auth_event.event_id, auth_event.room_id) + % (event.event_id, room_id, auth_event.event_id, auth_event.room_id), ) if do_sig_check: From 212f9e77a33393bc5d6423f3b1a4e5519329123a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 16:52:35 +0100 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/federation.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index c76c77b32709..930ae088c65c 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -618,9 +618,10 @@ async def _get_events_from_store_or_dest( will be omitted from the result. Likewise, any events which turn out not to be in the given room. - This function *does not* recursively get missing auth events of the - newly fetched events. Callers must include in the `event_ids` argument - any missing events from the auth chain. + This function *does not* automatically get missing auth events of the + newly fetched events. Callers must include the full auth chain of + of the missing events in the `event_ids` argument, to ensure that any + missing auth events are correctly fetched. Returns: map from event_id to event @@ -1136,7 +1137,7 @@ async def _get_events_and_persist( """Fetch the given events from a server, and persist them as outliers. This function *does not* recursively get missing auth events of the - newly fetched events. Callers must include in the `event_ids` argument + newly fetched events. Callers must include in the `events` argument any missing events from the auth chain. Logs a warning if we can't find the given event. From e0c9f59d3b46ec837c4542966d566afb03e00553 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 10 Jul 2020 17:39:39 +0100 Subject: [PATCH 8/8] Use 403 and update comment --- synapse/event_auth.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 129b72182723..c0981eee6243 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -65,12 +65,13 @@ def check( room_id = event.room_id - # I'm not really expecting to get auth events in the wrong room, but let's - # sanity-check it + # We need to ensure that the auth events are actually for the same room, to + # stop people from using powers they've been granted in other rooms for + # example. for auth_event in auth_events.values(): if auth_event.room_id != room_id: raise AuthError( - 500, + 403, "During auth for event %s in room %s, found event %s in the state " "which is in room %s" % (event.event_id, room_id, auth_event.event_id, auth_event.room_id),