From b17a9b120cbe2519b8489ae27a414a1ee2c14fe5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 15:33:36 +0100 Subject: [PATCH 1/9] drop old-room hack pretty sure we don't need this any more. --- synapse/handlers/federation.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 8197b60b7673..a444a0050647 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2611,16 +2611,6 @@ async def _check_event_auth( 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()} - # This is a hack to fix some old rooms where the initial join event - # didn't reference the create event in its auth events. - if event.type == EventTypes.Member and not event.auth_event_ids(): - if len(event.prev_event_ids()) == 1 and event.depth < 5: - c = await self.store.get_event( - event.prev_event_ids()[0], allow_none=True - ) - if c and c.type == EventTypes.Create: - auth_events[(c.type, c.state_key)] = c - try: context = await self._update_auth_events_and_context_for_auth( origin, event, context, auth_events From c12940c5109164d8b15bcef78356ce3a3c965bf6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 17:01:12 +0100 Subject: [PATCH 2/9] Remove incorrect comment about modifying `context` It doesn't look like the supplied context is ever modified. --- synapse/handlers/federation.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index a444a0050647..c776b58c2596 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2220,7 +2220,6 @@ async def _auth_and_persist_event( context: The event context. - NB that this function potentially modifies it. state: The state events used to check the event for soft-fail. If this is not provided the current state events will be used. @@ -2579,8 +2578,6 @@ async def _check_event_auth( event: The event itself. context: The event context. - - NB that this function potentially modifies it. state: The state events used to check the event for soft-fail. If this is not provided the current state events will be used. From da5976ecfec515f6c9e23c239fc667c304c7ec1e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 11:49:13 +0100 Subject: [PATCH 3/9] Stop `_auth_and_persist_event` modifying its parameters This is only called in three places. Two of them don't pass `auth_events`, and the third doesn't use the dict after passing it in, so this should be non-functional. --- synapse/handlers/federation.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index c776b58c2596..85eac3c39cde 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2208,7 +2208,7 @@ async def _auth_and_persist_event( event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - auth_events: Optional[MutableStateMap[EventBase]] = None, + auth_events: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> None: """ @@ -2232,12 +2232,17 @@ async def _auth_and_persist_event( server. backfilled: True if the event was backfilled. """ + # take a copy of auth_events before _check_event_auth modifies it + mut_auth_events: Optional[MutableStateMap[EventBase]] = None + if auth_events: + mut_auth_events = dict(auth_events) + context = await self._check_event_auth( origin, event, context, state=state, - auth_events=auth_events, + auth_events=mut_auth_events, backfilled=backfilled, ) From 9f3ff3329e1e635b36d836afcc425af7e02f448b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 12:04:55 +0100 Subject: [PATCH 4/9] Stop `_check_event_auth` modifying its parameters `_check_event_auth` is only called in three places. `on_send_membership_event` doesn't pass an `auth_events`, and `prep` and `_auth_and_persist_event` do not use the map after passing it in. --- synapse/handlers/federation.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 85eac3c39cde..97494a181a84 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -122,7 +122,7 @@ class _NewEventInfo: event = attr.ib(type=EventBase) state = attr.ib(type=Optional[Sequence[EventBase]], default=None) - auth_events = attr.ib(type=Optional[MutableStateMap[EventBase]], default=None) + auth_events = attr.ib(type=Optional[StateMap[EventBase]], default=None) class FederationHandler(BaseHandler): @@ -2232,17 +2232,12 @@ async def _auth_and_persist_event( server. backfilled: True if the event was backfilled. """ - # take a copy of auth_events before _check_event_auth modifies it - mut_auth_events: Optional[MutableStateMap[EventBase]] = None - if auth_events: - mut_auth_events = dict(auth_events) - context = await self._check_event_auth( origin, event, context, state=state, - auth_events=mut_auth_events, + auth_events=auth_events, backfilled=backfilled, ) @@ -2572,7 +2567,7 @@ async def _check_event_auth( event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - auth_events: Optional[MutableStateMap[EventBase]] = None, + auth_events: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> EventContext: """ @@ -2594,8 +2589,6 @@ async def _check_event_auth( event is an outlier), may be the auth events claimed by the remote server. - Also NB that this function adds entries to it. - If this is not provided, it is calculated from the previous state IDs. backfilled: True if the event was backfilled. @@ -2613,9 +2606,13 @@ async def _check_event_auth( 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()} + # take a copy of auth_events before _update_auth_events_and_context_for_auth + # modifies it + mut_auth_events: MutableStateMap[EventBase] = dict(auth_events) + try: context = await self._update_auth_events_and_context_for_auth( - origin, event, context, auth_events + origin, event, context, mut_auth_events ) except Exception: # We don't really mind if the above fails, so lets not fail @@ -2629,7 +2626,7 @@ async def _check_event_auth( ) try: - event_auth.check(room_version_obj, event, auth_events=auth_events) + event_auth.check(room_version_obj, event, auth_events=mut_auth_events) except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR From 9c01fe192ef4face8dd4a5c6256c90c06b47ade9 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 15:51:38 +0100 Subject: [PATCH 5/9] Stop `_update_auth_events_and_context_for_auth` modifying its parameters Return the updated auth event dict, rather than modifying the parameter. This is only called from `_check_event_auth`. --- synapse/handlers/federation.py | 37 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 97494a181a84..7857a19e0fb9 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2606,13 +2606,12 @@ async def _check_event_auth( 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()} - # take a copy of auth_events before _update_auth_events_and_context_for_auth - # modifies it - mut_auth_events: MutableStateMap[EventBase] = dict(auth_events) - try: - context = await self._update_auth_events_and_context_for_auth( - origin, event, context, mut_auth_events + ( + context, + auth_events_for_auth, + ) = await self._update_auth_events_and_context_for_auth( + origin, event, context, auth_events ) except Exception: # We don't really mind if the above fails, so lets not fail @@ -2624,9 +2623,10 @@ async def _check_event_auth( "Ignoring failure and continuing processing of event.", event.event_id, ) + auth_events_for_auth = auth_events try: - event_auth.check(room_version_obj, event, auth_events=mut_auth_events) + event_auth.check(room_version_obj, event, auth_events=auth_events_for_auth) except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -2651,8 +2651,8 @@ async def _update_auth_events_and_context_for_auth( origin: str, event: EventBase, context: EventContext, - auth_events: MutableStateMap[EventBase], - ) -> EventContext: + input_auth_events: StateMap[EventBase], + ) -> Tuple[EventContext, StateMap[EventBase]]: """Helper for _check_event_auth. See there for docs. Checks whether a given event has the expected auth events. If it @@ -2669,7 +2669,7 @@ async def _update_auth_events_and_context_for_auth( event: context: - auth_events: + input_auth_events: Map from (event_type, state_key) to event Normally, our calculated auth_events based on the state of the room @@ -2677,11 +2677,12 @@ async def _update_auth_events_and_context_for_auth( event is an outlier), may be the auth events claimed by the remote server. - Also NB that this function adds entries to it. - Returns: - updated context + 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) + event_auth_events = set(event.auth_event_ids()) # missing_auth is the set of the event's auth_events which we don't yet have @@ -2710,7 +2711,7 @@ async def _update_auth_events_and_context_for_auth( # The other side isn't around or doesn't implement the # endpoint, so lets just bail out. logger.info("Failed to get event auth from remote: %s", e1) - return context + return context, auth_events seen_remotes = await self.store.have_seen_events( event.room_id, [e.event_id for e in remote_auth_chain] @@ -2759,14 +2760,14 @@ async def _update_auth_events_and_context_for_auth( # obviously be empty # (b) alternatively, why don't we do it earlier? logger.info("Skipping auth_event fetch for outlier") - return context + return context, auth_events different_auth = event_auth_events.difference( e.event_id for e in auth_events.values() ) if not different_auth: - return context + return context, auth_events logger.info( "auth_events refers to events which are not in our calculated auth " @@ -2792,7 +2793,7 @@ async def _update_auth_events_and_context_for_auth( # XXX: should we reject the event in this case? It feels like we should, # but then shouldn't we also do so if we've failed to fetch any of the # auth events? - return context + return context, auth_events # now we state-resolve between our own idea of the auth events, and the remote's # idea of them. @@ -2822,7 +2823,7 @@ async def _update_auth_events_and_context_for_auth( event, context, auth_events ) - return context + return context, auth_events async def _update_context_for_auth_events( self, event: EventBase, context: EventContext, auth_events: StateMap[EventBase] From eddd41ba93a0d3b8e16da98b9418cf7a67c86735 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 15:22:57 +0100 Subject: [PATCH 6/9] Improve documentation on `_auth_and_persist_event` Rename `auth_events` parameter to better reflect what it contains. --- synapse/handlers/federation.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 7857a19e0fb9..cdd1c60b04cd 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2208,7 +2208,7 @@ async def _auth_and_persist_event( event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - auth_events: Optional[StateMap[EventBase]] = None, + claimed_auth_event_map: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> None: """ @@ -2223,13 +2223,15 @@ async def _auth_and_persist_event( state: The state events used to check the event for soft-fail. If this is not provided the current state events will be used. - 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. + claimed_auth_event_map: + A map of (type, state_key) => event for the event's claimed auth_events. + 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. + backfilled: True if the event was backfilled. """ context = await self._check_event_auth( @@ -2237,7 +2239,7 @@ async def _auth_and_persist_event( event, context, state=state, - auth_events=auth_events, + auth_events=claimed_auth_event_map, backfilled=backfilled, ) @@ -2742,7 +2744,10 @@ async def _update_auth_events_and_context_for_auth( await self.state_handler.compute_event_context(e) ) await self._auth_and_persist_event( - origin, e, missing_auth_event_context, auth_events=auth + origin, + e, + missing_auth_event_context, + claimed_auth_event_map=auth, ) if e.event_id in event_auth_events: From 09173aaf47b165bcd2219e22879d12f636bcc283 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 13:57:11 +0100 Subject: [PATCH 7/9] Improve documentation on `_NewEventInfo` --- synapse/handlers/federation.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index cdd1c60b04cd..63c651cb0e4d 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -108,21 +108,33 @@ ) -@attr.s(slots=True) +@attr.s(slots=True, frozen=True, auto_attribs=True) class _NewEventInfo: """Holds information about a received event, ready for passing to _auth_and_persist_events Attributes: event: the received event - state: the state at that event + state: the state at that event, according to /state_ids from a remote + homeserver. Only populated for backfilled events which are going to be a + new backwards extremity. + + claimed_auth_event_map: a map of (type, state_key) => event for the event's + claimed auth_events. + + This can include events which have not yet been persisted, in the case that + we are backfilling a batch of events. + + Note: May be incomplete: if we were unable to find all of the claimed auth + events. Also, treat the contents with caution: the events might also have + been rejected, might not yet have been authorized themselves, or they might + be in the wrong room. - auth_events: the auth_event map for that event """ - event = attr.ib(type=EventBase) - state = attr.ib(type=Optional[Sequence[EventBase]], default=None) - auth_events = attr.ib(type=Optional[StateMap[EventBase]], default=None) + event: EventBase + state: Optional[Sequence[EventBase]] + claimed_auth_event_map: StateMap[EventBase] class FederationHandler(BaseHandler): @@ -1000,7 +1012,7 @@ async def backfill( _NewEventInfo( event=ev, state=events_to_state[e_id], - auth_events={ + claimed_auth_event_map={ ( auth_events[a_id].type, auth_events[a_id].state_key, @@ -2303,7 +2315,7 @@ async def prep(ev_info: _NewEventInfo): event, res, state=ev_info.state, - auth_events=ev_info.auth_events, + auth_events=ev_info.claimed_auth_event_map, backfilled=backfilled, ) return res From 7c2488d7eb6cc93e2401b64c30298d2c26cb2f06 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 4 Aug 2021 22:17:16 +0100 Subject: [PATCH 8/9] Improve documentation on `_check_event_auth` rename `auth_events` parameter to better describe what it contains --- synapse/handlers/federation.py | 29 ++++++++++++++++++----------- tests/test_federation.py | 6 ++---- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 63c651cb0e4d..56235cd8eca0 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2251,7 +2251,7 @@ async def _auth_and_persist_event( event, context, state=state, - auth_events=claimed_auth_event_map, + claimed_auth_event_map=claimed_auth_event_map, backfilled=backfilled, ) @@ -2315,7 +2315,7 @@ async def prep(ev_info: _NewEventInfo): event, res, state=ev_info.state, - auth_events=ev_info.claimed_auth_event_map, + claimed_auth_event_map=ev_info.claimed_auth_event_map, backfilled=backfilled, ) return res @@ -2581,7 +2581,7 @@ async def _check_event_auth( event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - auth_events: Optional[StateMap[EventBase]] = None, + claimed_auth_event_map: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> EventContext: """ @@ -2592,18 +2592,20 @@ async def _check_event_auth( event: The event itself. context: The event context. + state: The state events used to check the event for soft-fail. If this is not provided the current state events will be used. - 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. + claimed_auth_event_map: + A map of (type, state_key) => event for the event's claimed auth_events. + 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. - If this is not provided, it is calculated from the previous state IDs. backfilled: True if the event was backfilled. Returns: @@ -2612,7 +2614,12 @@ async def _check_event_auth( room_version = await self.store.get_room_version_id(event.room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] - if not auth_events: + 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 diff --git a/tests/test_federation.py b/tests/test_federation.py index 0ed8326f55b8..3785799f46d2 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -75,10 +75,8 @@ def setUp(self): ) self.handler = self.homeserver.get_federation_handler() - self.handler._check_event_auth = ( - lambda origin, event, context, state, auth_events, backfilled: succeed( - context - ) + self.handler._check_event_auth = lambda origin, event, context, state, claimed_auth_event_map, backfilled: succeed( + context ) self.client = self.homeserver.get_federation_client() self.client._check_sigs_and_hash_and_fetch = lambda dest, pdus, **k: succeed( From b45fcfcd87e41dead1abb8a6bca2b31720e228a6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 5 Aug 2021 09:50:44 +0100 Subject: [PATCH 9/9] changelog --- changelog.d/10539.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10539.misc diff --git a/changelog.d/10539.misc b/changelog.d/10539.misc new file mode 100644 index 000000000000..9a765435dbe4 --- /dev/null +++ b/changelog.d/10539.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity.