From 2622b28c5cbe38c60c556544aa7502a8684ee60b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 28 Sep 2021 15:25:07 +0100 Subject: Inline `_check_event_auth` for outliers (#10926) * Inline `_check_event_auth` for outliers When we are persisting an outlier, most of `_check_event_auth` is redundant: * `_update_auth_events_and_context_for_auth` does nothing, because the `input_auth_events` are (now) exactly the event's auth_events, which means that `missing_auth` is empty. * we don't care about soft-fail, kicking guest users or `send_on_behalf_of` for outliers ... so the only thing that matters is the auth itself, so let's just do that. * `_auth_and_persist_fetched_events_inner`: de-async `prep` `prep` no longer calls any `async` methods, so let's make it synchronous. * Simplify `_check_event_auth` We no longer need to support outliers here, which makes things rather simpler. * changelog * lint --- synapse/handlers/federation_event.py | 93 ++++++++++++++---------------------- 1 file changed, 36 insertions(+), 57 deletions(-) (limited to 'synapse/handlers/federation_event.py') diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 01fd841122..2c4644b4a3 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -68,11 +68,7 @@ from synapse.types import ( UserID, get_domain_from_id, ) -from synapse.util.async_helpers import ( - Linearizer, - concurrently_execute, - yieldable_gather_results, -) +from synapse.util.async_helpers import Linearizer, concurrently_execute from synapse.util.iterutils import batch_iter from synapse.util.retryutils import NotRetryingDestination from synapse.util.stringutils import shortstr @@ -1189,7 +1185,10 @@ class FederationEventHandler: allow_rejected=True, ) - async def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: + room_version = await self._store.get_room_version_id(room_id) + room_version_obj = KNOWN_ROOM_VERSIONS[room_version] + + def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: with nested_logging_context(suffix=event.event_id): auth = {} for auth_event_id in event.auth_event_ids(): @@ -1207,17 +1206,15 @@ class FederationEventHandler: auth[(ae.type, ae.state_key)] = ae context = EventContext.for_outlier() - context = await self._check_event_auth( - origin, - event, - context, - claimed_auth_event_map=auth, - ) + try: + event_auth.check(room_version_obj, event, auth_events=auth) + except AuthError as e: + logger.warning("Rejecting %r because %s", event, e) + context.rejected = RejectedReason.AUTH_ERROR + return event, context - events_to_persist = ( - x for x in await yieldable_gather_results(prep, fetched_events) if x - ) + events_to_persist = (x for x in (prep(event) for event in fetched_events) if x) await self.persist_events_and_notify(room_id, tuple(events_to_persist)) async def _check_event_auth( @@ -1226,7 +1223,6 @@ class FederationEventHandler: event: EventBase, context: EventContext, state: Optional[Iterable[EventBase]] = None, - claimed_auth_event_map: Optional[StateMap[EventBase]] = None, backfilled: bool = False, ) -> EventContext: """ @@ -1242,42 +1238,36 @@ class FederationEventHandler: The state events used to check the event for soft-fail. If this is not provided the current state events will be used. - claimed_auth_event_map: - A map of (type, state_key) => event for the event's claimed auth_events. - Possibly including events that were rejected, or are in the wrong room. - - 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 + # This method should only be used for non-outliers + assert not event.internal_metadata.outlier room_version = await self._store.get_room_version_id(event.room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] - 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 - ) - 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()} + # calculate what the auth events *should* be, to use as a basis for auth. + 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 + ) + auth_events_x = await self._store.get_events(auth_events_ids) + calculated_auth_event_map = { + (e.type, e.state_key): e for e in auth_events_x.values() + } try: ( context, auth_events_for_auth, ) = await self._update_auth_events_and_context_for_auth( - origin, event, context, auth_events + origin, + event, + context, + calculated_auth_event_map=calculated_auth_event_map, ) except Exception: # We don't really mind if the above fails, so lets not fail @@ -1289,7 +1279,7 @@ class FederationEventHandler: "Ignoring failure and continuing processing of event.", event.event_id, ) - auth_events_for_auth = auth_events + auth_events_for_auth = calculated_auth_event_map try: event_auth.check(room_version_obj, event, auth_events=auth_events_for_auth) @@ -1425,7 +1415,7 @@ class FederationEventHandler: origin: str, event: EventBase, context: EventContext, - input_auth_events: StateMap[EventBase], + calculated_auth_event_map: StateMap[EventBase], ) -> Tuple[EventContext, StateMap[EventBase]]: """Helper for _check_event_auth. See there for docs. @@ -1443,19 +1433,17 @@ class FederationEventHandler: event: context: - input_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. + calculated_auth_event_map: + Our calculated auth_events based on the state of the room + at the event's position in the DAG. Returns: 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) + assert not event.internal_metadata.outlier + + # take a copy of calculated_auth_event_map before we modify it. + auth_events: MutableStateMap[EventBase] = dict(calculated_auth_event_map) event_auth_events = set(event.auth_event_ids()) @@ -1496,15 +1484,6 @@ class FederationEventHandler: } ) - if event.internal_metadata.is_outlier(): - # XXX: given that, for an outlier, we'll be working with the - # event's *claimed* auth events rather than those we calculated: - # (a) is there any point in this test, since different_auth below will - # obviously be empty - # (b) alternatively, why don't we do it earlier? - logger.info("Skipping auth_event fetch for outlier") - return context, auth_events - different_auth = event_auth_events.difference( e.event_id for e in auth_events.values() ) -- cgit 1.5.1 From 428174f90249ec50f977b5ef5c5cf9f92599ee0a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 29 Sep 2021 18:59:15 +0100 Subject: Split `event_auth.check` into two parts (#10940) Broadly, the existing `event_auth.check` function has two parts: * a validation section: checks that the event isn't too big, that it has the rught signatures, etc. This bit is independent of the rest of the state in the room, and so need only be done once for each event. * an auth section: ensures that the event is allowed, given the rest of the state in the room. This gets done multiple times, against various sets of room state, because it forms part of the state res algorithm. Currently, this is implemented with `do_sig_check` and `do_size_check` parameters, but I think that makes everything hard to follow. Instead, we split the function in two and call each part separately where it is needed. --- changelog.d/10940.misc | 1 + synapse/event_auth.py | 153 +++++++++++++++++++++-------------- synapse/handlers/event_auth.py | 15 ++-- synapse/handlers/federation.py | 30 ++++--- synapse/handlers/federation_event.py | 18 +++-- synapse/handlers/message.py | 6 +- synapse/handlers/room.py | 6 +- synapse/state/v1.py | 8 +- synapse/state/v2.py | 4 +- tests/test_event_auth.py | 108 +++++++++---------------- 10 files changed, 177 insertions(+), 172 deletions(-) create mode 100644 changelog.d/10940.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/10940.misc b/changelog.d/10940.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/10940.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity. diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 5d7c6fa858..eef354de6e 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -41,42 +41,112 @@ from synapse.types import StateMap, UserID, get_domain_from_id logger = logging.getLogger(__name__) -def check( - room_version_obj: RoomVersion, - event: EventBase, - auth_events: StateMap[EventBase], - do_sig_check: bool = True, - do_size_check: bool = True, +def validate_event_for_room_version( + room_version_obj: RoomVersion, event: EventBase ) -> None: - """Checks if this event is correctly authed. + """Ensure that the event complies with the limits, and has the right signatures + + NB: does not *validate* the signatures - it assumes that any signatures present + have already been checked. + + NB: it does not check that the event satisfies the auth rules (that is done in + check_auth_rules_for_event) - these tests are independent of the rest of the state + in the room. + + NB: This is used to check events that have been received over federation. As such, + it can only enforce the checks specified in the relevant room version, to avoid + a split-brain situation where some servers accept such events, and others reject + them. + + TODO: consider moving this into EventValidator Args: - room_version_obj: the version of the room - event: the event being checked. - auth_events: the existing room state. - do_sig_check: True if it should be verified that the sending server - signed the event. - do_size_check: True if the size of the event fields should be verified. + room_version_obj: the version of the room which contains this event + event: the event to be checked Raises: - AuthError if the checks fail - - Returns: - if the auth checks pass. + SynapseError if there is a problem with the event """ - assert isinstance(auth_events, dict) - - if do_size_check: - _check_size_limits(event) + _check_size_limits(event) if not hasattr(event, "room_id"): raise AuthError(500, "Event has no room_id: %s" % event) - room_id = event.room_id + # check that the event has the correct signatures + sender_domain = get_domain_from_id(event.sender) + + is_invite_via_3pid = ( + event.type == EventTypes.Member + and event.membership == Membership.INVITE + and "third_party_invite" in event.content + ) + + # Check the sender's domain has signed the event + if not event.signatures.get(sender_domain): + # We allow invites via 3pid to have a sender from a different + # HS, as the sender must match the sender of the original + # 3pid invite. This is checked further down with the + # other dedicated membership checks. + if not is_invite_via_3pid: + raise AuthError(403, "Event not signed by sender's server") + + if event.format_version in (EventFormatVersions.V1,): + # Only older room versions have event IDs to check. + event_id_domain = get_domain_from_id(event.event_id) + + # Check the origin domain has signed the event + if not event.signatures.get(event_id_domain): + raise AuthError(403, "Event not signed by sending server") + + is_invite_via_allow_rule = ( + room_version_obj.msc3083_join_rules + and event.type == EventTypes.Member + and event.membership == Membership.JOIN + and "join_authorised_via_users_server" in event.content + ) + if is_invite_via_allow_rule: + authoriser_domain = get_domain_from_id( + event.content["join_authorised_via_users_server"] + ) + if not event.signatures.get(authoriser_domain): + raise AuthError(403, "Event not signed by authorising server") + + +def check_auth_rules_for_event( + room_version_obj: RoomVersion, event: EventBase, auth_events: StateMap[EventBase] +) -> None: + """Check that an event complies with the auth rules + + Checks whether an event passes the auth rules with a given set of state events + + Assumes that we have already checked that the event is the right shape (it has + enough signatures, has a room ID, etc). In other words: + + - it's fine for use in state resolution, when we have already decided whether to + accept the event or not, and are now trying to decide whether it should make it + into the room state + + - when we're doing the initial event auth, it is only suitable in combination with + a bunch of other tests. + + Args: + room_version_obj: the version of the room + event: the event being checked. + auth_events: the room state to check the events against. + + Raises: + AuthError if the checks fail + """ + assert isinstance(auth_events, dict) # 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. + # + # Arguably we don't need to do this when we're just doing state res, as presumably + # the state res algorithm isn't silly enough to give us events from different rooms. + # Still, it's easier to do it anyway. + room_id = event.room_id for auth_event in auth_events.values(): if auth_event.room_id != room_id: raise AuthError( @@ -86,45 +156,6 @@ def check( % (event.event_id, room_id, auth_event.event_id, auth_event.room_id), ) - if do_sig_check: - sender_domain = get_domain_from_id(event.sender) - - is_invite_via_3pid = ( - event.type == EventTypes.Member - and event.membership == Membership.INVITE - and "third_party_invite" in event.content - ) - - # Check the sender's domain has signed the event - if not event.signatures.get(sender_domain): - # We allow invites via 3pid to have a sender from a different - # HS, as the sender must match the sender of the original - # 3pid invite. This is checked further down with the - # other dedicated membership checks. - if not is_invite_via_3pid: - raise AuthError(403, "Event not signed by sender's server") - - if event.format_version in (EventFormatVersions.V1,): - # Only older room versions have event IDs to check. - event_id_domain = get_domain_from_id(event.event_id) - - # Check the origin domain has signed the event - if not event.signatures.get(event_id_domain): - raise AuthError(403, "Event not signed by sending server") - - is_invite_via_allow_rule = ( - room_version_obj.msc3083_join_rules - and event.type == EventTypes.Member - and event.membership == Membership.JOIN - and "join_authorised_via_users_server" in event.content - ) - if is_invite_via_allow_rule: - authoriser_domain = get_domain_from_id( - event.content["join_authorised_via_users_server"] - ) - if not event.signatures.get(authoriser_domain): - raise AuthError(403, "Event not signed by authorising server") - # Implementation of https://matrix.org/docs/spec/rooms/v1#authorization-rules # # 1. If type is m.room.create: diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index cb81fa0986..d089c56286 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -22,7 +22,8 @@ from synapse.api.constants import ( RestrictedJoinRuleTypes, ) from synapse.api.errors import AuthError, Codes, SynapseError -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion +from synapse.api.room_versions import RoomVersion +from synapse.event_auth import check_auth_rules_for_event from synapse.events import EventBase from synapse.events.builder import EventBuilder from synapse.events.snapshot import EventContext @@ -45,21 +46,17 @@ class EventAuthHandler: self._store = hs.get_datastore() self._server_name = hs.hostname - async def check_from_context( + async def check_auth_rules_from_context( self, - room_version: str, + room_version_obj: RoomVersion, event: EventBase, context: EventContext, - do_sig_check: bool = True, ) -> None: + """Check an event passes the auth rules at its own auth events""" auth_event_ids = event.auth_event_ids() auth_events_by_id = await self._store.get_events(auth_event_ids) auth_events = {(e.type, e.state_key): e for e in auth_events_by_id.values()} - - room_version_obj = KNOWN_ROOM_VERSIONS[room_version] - event_auth.check( - room_version_obj, event, auth_events=auth_events, do_sig_check=do_sig_check - ) + check_auth_rules_for_event(room_version_obj, event, auth_events) def compute_auth_events( self, diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 3b0b895b07..0a10a5c28a 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -40,6 +40,10 @@ from synapse.api.errors import ( ) from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion, RoomVersions from synapse.crypto.event_signing import compute_event_signature +from synapse.event_auth import ( + check_auth_rules_for_event, + validate_event_for_room_version, +) from synapse.events import EventBase from synapse.events.snapshot import EventContext from synapse.events.validator import EventValidator @@ -742,10 +746,9 @@ class FederationHandler(BaseHandler): # The remote hasn't signed it yet, obviously. We'll do the full checks # when we get the event back in `on_send_join_request` - await self._event_auth_handler.check_from_context( - room_version.identifier, event, context, do_sig_check=False + await self._event_auth_handler.check_auth_rules_from_context( + room_version, event, context ) - return event async def on_invite_request( @@ -916,8 +919,8 @@ class FederationHandler(BaseHandler): try: # The remote hasn't signed it yet, obviously. We'll do the full checks # when we get the event back in `on_send_leave_request` - await self._event_auth_handler.check_from_context( - room_version_obj.identifier, event, context, do_sig_check=False + await self._event_auth_handler.check_auth_rules_from_context( + room_version_obj, event, context ) except AuthError as e: logger.warning("Failed to create new leave %r because %s", event, e) @@ -978,8 +981,8 @@ class FederationHandler(BaseHandler): try: # The remote hasn't signed it yet, obviously. We'll do the full checks # when we get the event back in `on_send_knock_request` - await self._event_auth_handler.check_from_context( - room_version_obj.identifier, event, context, do_sig_check=False + await self._event_auth_handler.check_auth_rules_from_context( + room_version_obj, event, context ) except AuthError as e: logger.warning("Failed to create new knock %r because %s", event, e) @@ -1168,7 +1171,8 @@ class FederationHandler(BaseHandler): auth_for_e[(EventTypes.Create, "")] = create_event try: - event_auth.check(room_version, e, auth_events=auth_for_e) + validate_event_for_room_version(room_version, e) + check_auth_rules_for_event(room_version, e, auth_for_e) except SynapseError as err: # we may get SynapseErrors here as well as AuthErrors. For # instance, there are a couple of (ancient) events in some @@ -1266,8 +1270,9 @@ class FederationHandler(BaseHandler): event.internal_metadata.send_on_behalf_of = self.hs.hostname try: - await self._event_auth_handler.check_from_context( - room_version_obj.identifier, event, context + validate_event_for_room_version(room_version_obj, event) + await self._event_auth_handler.check_auth_rules_from_context( + room_version_obj, event, context ) except AuthError as e: logger.warning("Denying new third party invite %r because %s", event, e) @@ -1317,8 +1322,9 @@ class FederationHandler(BaseHandler): ) try: - await self._event_auth_handler.check_from_context( - room_version_obj.identifier, event, context + validate_event_for_room_version(room_version_obj, event) + await self._event_auth_handler.check_auth_rules_from_context( + room_version_obj, event, context ) except AuthError as e: logger.warning("Denying third party invite %r because %s", event, e) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 2c4644b4a3..e587b5b3b3 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -29,7 +29,6 @@ from typing import ( from prometheus_client import Counter -from synapse import event_auth from synapse.api.constants import ( EventContentFields, EventTypes, @@ -47,7 +46,11 @@ from synapse.api.errors import ( SynapseError, ) from synapse.api.room_versions import KNOWN_ROOM_VERSIONS -from synapse.event_auth import auth_types_for_event +from synapse.event_auth import ( + auth_types_for_event, + check_auth_rules_for_event, + validate_event_for_room_version, +) from synapse.events import EventBase from synapse.events.snapshot import EventContext from synapse.federation.federation_client import InvalidResponseError @@ -1207,7 +1210,8 @@ class FederationEventHandler: context = EventContext.for_outlier() try: - event_auth.check(room_version_obj, event, auth_events=auth) + validate_event_for_room_version(room_version_obj, event) + check_auth_rules_for_event(room_version_obj, event, auth) except AuthError as e: logger.warning("Rejecting %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -1282,7 +1286,8 @@ class FederationEventHandler: auth_events_for_auth = calculated_auth_event_map try: - event_auth.check(room_version_obj, event, auth_events=auth_events_for_auth) + validate_event_for_room_version(room_version_obj, event) + check_auth_rules_for_event(room_version_obj, event, auth_events_for_auth) except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -1394,7 +1399,10 @@ class FederationEventHandler: } try: - event_auth.check(room_version_obj, event, auth_events=current_auth_events) + # TODO: skip the call to validate_event_for_room_version? we should already + # have validated the event. + validate_event_for_room_version(room_version_obj, event) + check_auth_rules_for_event(room_version_obj, event, current_auth_events) except AuthError as e: logger.warning( "Soft-failing %r (from %s) because %s", diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 3b8cc50ec0..cdac53037c 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -44,6 +44,7 @@ from synapse.api.errors import ( ) from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.api.urls import ConsentURIBuilder +from synapse.event_auth import validate_event_for_room_version from synapse.events import EventBase from synapse.events.builder import EventBuilder from synapse.events.snapshot import EventContext @@ -1098,8 +1099,9 @@ class EventCreationHandler: assert event.content["membership"] == Membership.LEAVE else: try: - await self._event_auth_handler.check_from_context( - room_version_obj.identifier, event, context + validate_event_for_room_version(room_version_obj, event) + await self._event_auth_handler.check_auth_rules_from_context( + room_version_obj, event, context ) except AuthError as err: logger.warning("Denying new event %r because %s", event, err) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index bf8a85f563..873e08258e 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -52,6 +52,7 @@ from synapse.api.errors import ( ) from synapse.api.filtering import Filter from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion +from synapse.event_auth import validate_event_for_room_version from synapse.events import EventBase from synapse.events.utils import copy_power_levels_contents from synapse.rest.admin._base import assert_user_is_admin @@ -238,8 +239,9 @@ class RoomCreationHandler(BaseHandler): }, ) old_room_version = await self.store.get_room_version(old_room_id) - await self._event_auth_handler.check_from_context( - old_room_version.identifier, tombstone_event, tombstone_context + validate_event_for_room_version(old_room_version, tombstone_event) + await self._event_auth_handler.check_auth_rules_from_context( + old_room_version, tombstone_event, tombstone_context ) await self.clone_existing_room( diff --git a/synapse/state/v1.py b/synapse/state/v1.py index 92336d7cc8..017e6fd92d 100644 --- a/synapse/state/v1.py +++ b/synapse/state/v1.py @@ -329,12 +329,10 @@ def _resolve_auth_events( auth_events[(prev_event.type, prev_event.state_key)] = prev_event try: # The signatures have already been checked at this point - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, event, auth_events, - do_sig_check=False, - do_size_check=False, ) prev_event = event except AuthError: @@ -349,12 +347,10 @@ def _resolve_normal_events( for event in _ordered_events(events): try: # The signatures have already been checked at this point - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, event, auth_events, - do_sig_check=False, - do_size_check=False, ) return event except AuthError: diff --git a/synapse/state/v2.py b/synapse/state/v2.py index 7b1e8361de..586b0e12fe 100644 --- a/synapse/state/v2.py +++ b/synapse/state/v2.py @@ -546,12 +546,10 @@ async def _iterative_auth_checks( auth_events[key] = event_map[ev_id] try: - event_auth.check( + event_auth.check_auth_rules_for_event( room_version, event, auth_events, - do_sig_check=False, - do_size_check=False, ) resolved_state[(event.type, event.state_key)] = event_id diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index 6ebd01bcbe..e7a7d00883 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -37,21 +37,19 @@ class EventAuthTestCase(unittest.TestCase): } # creator should be able to send state - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _random_state_event(creator), auth_events, - do_sig_check=False, ) # joiner should not be able to send state self.assertRaises( AuthError, - event_auth.check, + event_auth.check_auth_rules_for_event, RoomVersions.V1, _random_state_event(joiner), auth_events, - do_sig_check=False, ) def test_state_default_level(self): @@ -76,19 +74,17 @@ class EventAuthTestCase(unittest.TestCase): # pleb should not be able to send state self.assertRaises( AuthError, - event_auth.check, + event_auth.check_auth_rules_for_event, RoomVersions.V1, _random_state_event(pleb), auth_events, - do_sig_check=False, ), # king should be able to send state - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _random_state_event(king), auth_events, - do_sig_check=False, ) def test_alias_event(self): @@ -101,37 +97,33 @@ class EventAuthTestCase(unittest.TestCase): } # creator should be able to send aliases - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _alias_event(creator), auth_events, - do_sig_check=False, ) # Reject an event with no state key. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _alias_event(creator, state_key=""), auth_events, - do_sig_check=False, ) # If the domain of the sender does not match the state key, reject. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _alias_event(creator, state_key="test.com"), auth_events, - do_sig_check=False, ) # Note that the member does *not* need to be in the room. - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _alias_event(other), auth_events, - do_sig_check=False, ) def test_msc2432_alias_event(self): @@ -144,34 +136,30 @@ class EventAuthTestCase(unittest.TestCase): } # creator should be able to send aliases - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _alias_event(creator), auth_events, - do_sig_check=False, ) # No particular checks are done on the state key. - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _alias_event(creator, state_key=""), auth_events, - do_sig_check=False, ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _alias_event(creator, state_key="test.com"), auth_events, - do_sig_check=False, ) # Per standard auth rules, the member must be in the room. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _alias_event(other), auth_events, - do_sig_check=False, ) def test_msc2209(self): @@ -191,20 +179,18 @@ class EventAuthTestCase(unittest.TestCase): } # pleb should be able to modify the notifications power level. - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V1, _power_levels_event(pleb, {"notifications": {"room": 100}}), auth_events, - do_sig_check=False, ) # But an MSC2209 room rejects this change. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _power_levels_event(pleb, {"notifications": {"room": 100}}), auth_events, - do_sig_check=False, ) def test_join_rules_public(self): @@ -221,59 +207,53 @@ class EventAuthTestCase(unittest.TestCase): } # Check join. - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user cannot be force-joined to a room. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _member_event(pleb, "join", sender=creator), auth_events, - do_sig_check=False, ) # Banned should be rejected. auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban") with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user who left can re-join. auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave") - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user can send a join if they're in the room. auth_events[("m.room.member", pleb)] = _member_event(pleb, "join") - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user can accept an invite. auth_events[("m.room.member", pleb)] = _member_event( pleb, "invite", sender=creator ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) def test_join_rules_invite(self): @@ -291,60 +271,54 @@ class EventAuthTestCase(unittest.TestCase): # A join without an invite is rejected. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user cannot be force-joined to a room. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _member_event(pleb, "join", sender=creator), auth_events, - do_sig_check=False, ) # Banned should be rejected. auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban") with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user who left cannot re-join. auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave") with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user can send a join if they're in the room. auth_events[("m.room.member", pleb)] = _member_event(pleb, "join") - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user can accept an invite. auth_events[("m.room.member", pleb)] = _member_event( pleb, "invite", sender=creator ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) def test_join_rules_msc3083_restricted(self): @@ -369,11 +343,10 @@ class EventAuthTestCase(unittest.TestCase): # Older room versions don't understand this join rule with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V6, _join_event(pleb), auth_events, - do_sig_check=False, ) # A properly formatted join event should work. @@ -383,11 +356,10 @@ class EventAuthTestCase(unittest.TestCase): "join_authorised_via_users_server": "@creator:example.com" }, ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, authorised_join_event, auth_events, - do_sig_check=False, ) # A join issued by a specific user works (i.e. the power level checks @@ -399,7 +371,7 @@ class EventAuthTestCase(unittest.TestCase): pl_auth_events[("m.room.member", "@inviter:foo.test")] = _join_event( "@inviter:foo.test" ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _join_event( pleb, @@ -408,16 +380,14 @@ class EventAuthTestCase(unittest.TestCase): }, ), pl_auth_events, - do_sig_check=False, ) # A join which is missing an authorised server is rejected. with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _join_event(pleb), auth_events, - do_sig_check=False, ) # An join authorised by a user who is not in the room is rejected. @@ -426,7 +396,7 @@ class EventAuthTestCase(unittest.TestCase): creator, {"invite": 100, "users": {"@other:example.com": 150}} ) with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _join_event( pleb, @@ -435,13 +405,12 @@ class EventAuthTestCase(unittest.TestCase): }, ), auth_events, - do_sig_check=False, ) # A user cannot be force-joined to a room. (This uses an event which # *would* be valid, but is sent be a different user.) with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _member_event( pleb, @@ -452,36 +421,32 @@ class EventAuthTestCase(unittest.TestCase): }, ), auth_events, - do_sig_check=False, ) # Banned should be rejected. auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban") with self.assertRaises(AuthError): - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, authorised_join_event, auth_events, - do_sig_check=False, ) # A user who left can re-join. auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave") - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, authorised_join_event, auth_events, - do_sig_check=False, ) # A user can send a join if they're in the room. (This doesn't need to # be authorised since the user is already joined.) auth_events[("m.room.member", pleb)] = _member_event(pleb, "join") - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _join_event(pleb), auth_events, - do_sig_check=False, ) # A user can accept an invite. (This doesn't need to be authorised since @@ -489,11 +454,10 @@ class EventAuthTestCase(unittest.TestCase): auth_events[("m.room.member", pleb)] = _member_event( pleb, "invite", sender=creator ) - event_auth.check( + event_auth.check_auth_rules_for_event( RoomVersions.V8, _join_event(pleb), auth_events, - do_sig_check=False, ) -- cgit 1.5.1 From cb88ed912b3e984e0a409e4e5fd3c22817a4840d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 5 Oct 2021 12:50:07 +0100 Subject: `_check_event_auth`: move event validation earlier (#10988) There's little point in doing a fancy state reconciliation dance if the event itself is invalid. Likewise, there's no point checking it again in `_check_for_soft_fail`. --- changelog.d/10988.misc | 1 + synapse/handlers/federation_event.py | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 changelog.d/10988.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/10988.misc b/changelog.d/10988.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/10988.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 e587b5b3b3..5938654338 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1250,9 +1250,18 @@ class FederationEventHandler: # This method should only be used for non-outliers assert not event.internal_metadata.outlier + # first of all, check that the event itself is valid. room_version = await self._store.get_room_version_id(event.room_id) room_version_obj = KNOWN_ROOM_VERSIONS[room_version] + try: + validate_event_for_room_version(room_version_obj, event) + except AuthError as e: + logger.warning("While validating received event %r: %s", event, e) + # TODO: use a different rejected reason here? + context.rejected = RejectedReason.AUTH_ERROR + return context + # calculate what the auth events *should* be, to use as a basis for auth. prev_state_ids = await context.get_prev_state_ids() auth_events_ids = self._event_auth_handler.compute_auth_events( @@ -1286,7 +1295,6 @@ class FederationEventHandler: auth_events_for_auth = calculated_auth_event_map try: - validate_event_for_room_version(room_version_obj, event) check_auth_rules_for_event(room_version_obj, event, auth_events_for_auth) except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) @@ -1399,9 +1407,6 @@ class FederationEventHandler: } try: - # TODO: skip the call to validate_event_for_room_version? we should already - # have validated the event. - validate_event_for_room_version(room_version_obj, event) check_auth_rules_for_event(room_version_obj, event, current_auth_events) except AuthError as e: logger.warning( -- cgit 1.5.1 From d099535deb5be31891719c61c3757c5150829053 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 5 Oct 2021 12:50:38 +0100 Subject: `_update_auth_events_and_context_for_auth`: add some comments (#10987) Add some more comments about wtf is going on here. --- changelog.d/10987.misc | 1 + synapse/handlers/federation_event.py | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 changelog.d/10987.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/10987.misc b/changelog.d/10987.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/10987.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 5938654338..aa20d75550 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1476,6 +1476,11 @@ class FederationEventHandler: logger.debug("Events %s are in the store", have_events) missing_auth.difference_update(have_events) + # missing_auth is now the set of event_ids which: + # a. are listed in event.auth_events, *and* + # b. are *not* part of our calculated auth events based on room state, *and* + # c. are *not* yet in our database. + if missing_auth: # If we don't have all the auth events, we need to get them. logger.info("auth_events contains unknown events: %s", missing_auth) @@ -1497,10 +1502,31 @@ class FederationEventHandler: } ) + # auth_events now contains + # 1. our *calculated* auth events based on the room state, plus: + # 2. any events which: + # a. are listed in `event.auth_events`, *and* + # b. are not part of our calculated auth events, *and* + # c. were not in our database before the call to /event_auth + # d. have since been added to our database (most likely by /event_auth). + different_auth = event_auth_events.difference( e.event_id for e in auth_events.values() ) + # different_auth is the set of events which *are* in `event.auth_events`, but + # which are *not* in `auth_events`. Comparing with (2.) above, this means + # exclusively the set of `event.auth_events` which we already had in our + # database before any call to /event_auth. + # + # I'm reasonably sure that the fact that events returned by /event_auth are + # blindly added to auth_events (and hence excluded from different_auth) is a bug + # - though it's a very long-standing one (see + # https://github.com/matrix-org/synapse/commit/78015948a7febb18e000651f72f8f58830a55b93#diff-0bc92da3d703202f5b9be2d3f845e375f5b1a6bc6ba61705a8af9be1121f5e42R786 + # from Jan 2015 which seems to add it, though it actually just moves it from + # elsewhere (before that, it gets lost in a mess of huge "various bug fixes" + # PRs). + if not different_auth: return context, auth_events -- cgit 1.5.1 From 787af4a1062fecc350fa14fe2abfc0e9d2f1555e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 5 Oct 2021 13:01:41 +0100 Subject: Host `cache_joined_hosts_for_event` to caller (#10986) `_check_event_auth` is only called in two places, and only one of those sets `send_on_behalf_of`. Warming the cache isn't really part of auth anyway, so moving it out makes a lot more sense. --- changelog.d/10986.misc | 1 + synapse/handlers/federation_event.py | 18 ++++++++---------- 2 files changed, 9 insertions(+), 10 deletions(-) create mode 100644 changelog.d/10986.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/10986.misc b/changelog.d/10986.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/10986.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 aa20d75550..9269cb444d 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -356,6 +356,11 @@ class FederationEventHandler: ) # all looks good, we can persist the event. + + # First, precalculate the joined hosts so that the federation sender doesn't + # need to. + await self._event_creation_handler.cache_joined_hosts_for_event(event, context) + await self._run_push_actions_and_persist_event(event, context) return event, context @@ -1299,17 +1304,10 @@ class FederationEventHandler: except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR + return context - if not context.rejected: - await self._check_for_soft_fail(event, state, backfilled, origin=origin) - await self._maybe_kick_guest_users(event) - - # If we are going to send this event over federation we precaclculate - # the joined hosts. - if event.internal_metadata.get_send_on_behalf_of(): - await self._event_creation_handler.cache_joined_hosts_for_event( - event, context - ) + await self._check_for_soft_fail(event, state, backfilled, origin=origin) + await self._maybe_kick_guest_users(event) return context -- cgit 1.5.1 From 392863fbf1ee31f8a1997446ab31919a7b6d9a14 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 5 Oct 2021 11:51:57 -0500 Subject: Fix logic flaw preventing tracking of MSC2716 events in existing room versions (#10962) We correctly allowed using the MSC2716 batch endpoint for the room creator in existing room versions but accidentally didn't track the events because of a logic flaw. This prevented you from connecting subsequent chunks together because it would throw the unknown batch ID error. We only want to process MSC2716 events when: - The room version supports MSC2716 - Any room where the homeserver has the `msc2716_enabled` experimental feature enabled and the event is from the room creator --- changelog.d/10962.bugfix | 1 + synapse/handlers/federation_event.py | 5 ++--- synapse/storage/databases/main/events.py | 10 ++++------ 3 files changed, 7 insertions(+), 9 deletions(-) create mode 100644 changelog.d/10962.bugfix (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/10962.bugfix b/changelog.d/10962.bugfix new file mode 100644 index 0000000000..9b0760d731 --- /dev/null +++ b/changelog.d/10962.bugfix @@ -0,0 +1 @@ +Fix [MSC2716](https://github.com/matrix-org/matrix-doc/pull/2716) `/batch_send` endpoint rejecting subsequent batches with unknown batch ID error in existing room versions from the room creator. diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 9269cb444d..243be46267 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1015,9 +1015,8 @@ class FederationEventHandler: room_version = await self._store.get_room_version(marker_event.room_id) create_event = await self._store.get_create_event_for_room(marker_event.room_id) room_creator = create_event.content.get(EventContentFields.ROOM_CREATOR) - if ( - not room_version.msc2716_historical - or not self._config.experimental.msc2716_enabled + if not room_version.msc2716_historical and ( + not self._config.experimental.msc2716_enabled or marker_event.sender != room_creator ): return diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index bc7d213fe2..19f55c19c5 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1763,9 +1763,8 @@ class PersistEventsStore: retcol="creator", allow_none=True, ) - if ( - not room_version.msc2716_historical - or not self.hs.config.experimental.msc2716_enabled + if not room_version.msc2716_historical and ( + not self.hs.config.experimental.msc2716_enabled or event.sender != room_creator ): return @@ -1825,9 +1824,8 @@ class PersistEventsStore: retcol="creator", allow_none=True, ) - if ( - not room_version.msc2716_historical - or not self.hs.config.experimental.msc2716_enabled + if not room_version.msc2716_historical and ( + not self.hs.config.experimental.msc2716_enabled or event.sender != room_creator ): return -- cgit 1.5.1 From 86af6b2f0ef92a317900fd4a4f6d3436ff8a011c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 7 Oct 2021 12:20:03 +0100 Subject: Add a comment in _process_received_pdu (#11011) --- changelog.d/11011.misc | 1 + synapse/handlers/federation_event.py | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 changelog.d/11011.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/11011.misc b/changelog.d/11011.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/11011.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 243be46267..0645ce9392 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -894,6 +894,9 @@ class FederationEventHandler: backfilled=backfilled, ) except AuthError as e: + # FIXME richvdh 2021/10/07 I don't think this is reachable. Let's log it + # for now + logger.exception("Unexpected AuthError from _check_event_auth") raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) await self._run_push_actions_and_persist_event(event, context, backfilled) -- cgit 1.5.1 From 96fe77c2546598449c1d423c125f84c92620b155 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 7 Oct 2021 12:43:25 +0100 Subject: Improve the logging in _auth_and_persist_outliers (#11010) Include the event ids being peristed --- changelog.d/11010.misc | 1 + synapse/handlers/federation_event.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11010.misc (limited to 'synapse/handlers/federation_event.py') diff --git a/changelog.d/11010.misc b/changelog.d/11010.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/11010.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 0645ce9392..f640b417b3 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1161,7 +1161,10 @@ class FederationEventHandler: return logger.info( - "Persisting %i of %i remaining events", len(roots), len(event_map) + "Persisting %i of %i remaining outliers: %s", + len(roots), + len(event_map), + shortstr(e.event_id for e in roots), ) await self._auth_and_persist_fetched_events_inner(origin, room_id, roots) -- cgit 1.5.1