Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix recursion error when fetching auth chain over federation #7817

Merged
merged 9 commits into from
Jul 10, 2020
1 change: 1 addition & 0 deletions changelog.d/7817.bugfix
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 3 additions & 2 deletions synapse/event_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be a 403?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oof there is AuthError(500) above. whatever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, probably actually. I was copying it from a previous line, but I don't think that makes sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but also: if we actually hit this code and it matters, the comment needs changing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/me awards past me a prize for including sanity checks for security-critical things

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, which comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it

"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:
Expand Down
48 changes: 36 additions & 12 deletions synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 in the `event_ids` argument
any missing events from the auth chain.
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved

Returns:
map from event_id to event
"""
Expand Down Expand Up @@ -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 in the `event_ids` argument
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
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):
Expand All @@ -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] = event

except Exception as e:
logger.warning(
Expand All @@ -1172,6 +1170,32 @@ 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.

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(
auth_events, 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
else:
logger.info("Missing auth event %s", auth_event_id)

event_infos.append(_NewEventInfo(event, None, auth))

await self._handle_new_events(
destination, event_infos,
)
Expand Down