From 8ecf6be1e1a737a09f51137302ad0d9ae4ed519b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 15 Jun 2022 19:48:22 +0100 Subject: Move some event auth checks out to a different method (#13065) * Add auth events to events used in tests * Move some event auth checks out to a different method Some of the event auth checks apply to an event's auth_events, rather than the state at the event - which means they can play no part in state resolution. Move them out to a separate method. * Rename check_auth_rules_for_event Now it only checks the state-dependent auth rules, it needs a better name. --- synapse/event_auth.py | 108 +++++++++++++++++++++++++---------- synapse/handlers/event_auth.py | 8 ++- synapse/handlers/federation_event.py | 27 +++++---- synapse/state/v1.py | 4 +- synapse/state/v2.py | 2 +- 5 files changed, 105 insertions(+), 44 deletions(-) (limited to 'synapse') diff --git a/synapse/event_auth.py b/synapse/event_auth.py index e23503c1e0..360a50cc71 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -15,11 +15,12 @@ import logging import typing -from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union +from typing import Any, Collection, Dict, Iterable, List, Optional, Set, Tuple, Union from canonicaljson import encode_canonical_json from signedjson.key import decode_verify_key_bytes from signedjson.sign import SignatureVerifyException, verify_signed_json +from typing_extensions import Protocol from unpaddedbase64 import decode_base64 from synapse.api.constants import ( @@ -35,7 +36,8 @@ from synapse.api.room_versions import ( EventFormatVersions, RoomVersion, ) -from synapse.types import StateMap, UserID, get_domain_from_id +from synapse.storage.databases.main.events_worker import EventRedactBehaviour +from synapse.types import MutableStateMap, StateMap, UserID, get_domain_from_id if typing.TYPE_CHECKING: # conditional imports to avoid import cycle @@ -45,6 +47,17 @@ if typing.TYPE_CHECKING: logger = logging.getLogger(__name__) +class _EventSourceStore(Protocol): + async def get_events( + self, + event_ids: Collection[str], + redact_behaviour: EventRedactBehaviour, + get_prev_content: bool = False, + allow_rejected: bool = False, + ) -> Dict[str, "EventBase"]: + ... + + def validate_event_for_room_version(event: "EventBase") -> None: """Ensure that the event complies with the limits, and has the right signatures @@ -112,47 +125,52 @@ def validate_event_for_room_version(event: "EventBase") -> None: raise AuthError(403, "Event not signed by authorising server") -def check_auth_rules_for_event( +async def check_state_independent_auth_rules( + store: _EventSourceStore, event: "EventBase", - auth_events: Iterable["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 + """Check that an event complies with auth rules that are independent of room state - - when we're doing the initial event auth, it is only suitable in combination with - a bunch of other tests. + Runs through the first few auth rules, which are independent of room state. (Which + means that we only need to them once for each received event) Args: + store: the datastore; used to fetch the auth events for validation event: the event being checked. - auth_events: the room state to check the events against. Raises: AuthError if the checks fail """ - # 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. + # Check the auth events. + auth_events = await store.get_events( + event.auth_event_ids(), + redact_behaviour=EventRedactBehaviour.as_is, + allow_rejected=True, + ) room_id = event.room_id - for auth_event in auth_events: + auth_dict: MutableStateMap[str] = {} + for auth_event_id in event.auth_event_ids(): + auth_event = auth_events.get(auth_event_id) + + # we should have all the auth events by now, so if we do not, that suggests + # a synapse programming error + if auth_event is None: + raise RuntimeError( + f"Event {event.event_id} has unknown auth event {auth_event_id}" + ) + + # 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. if auth_event.room_id != room_id: raise AuthError( 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), + % (event.event_id, room_id, auth_event_id, auth_event.room_id), ) + + # We also need to check that the auth event itself is not rejected. if auth_event.rejected_reason: raise AuthError( 403, @@ -160,6 +178,8 @@ def check_auth_rules_for_event( % (event.event_id, auth_event.event_id), ) + auth_dict[(auth_event.type, auth_event.state_key)] = auth_event_id + # Implementation of https://matrix.org/docs/spec/rooms/v1#authorization-rules # # 1. If type is m.room.create: @@ -181,16 +201,46 @@ def check_auth_rules_for_event( "room appears to have unsupported version %s" % (room_version_prop,), ) - logger.debug("Allowing! %s", event) return - auth_dict = {(e.type, e.state_key): e for e in auth_events} - # 3. If event does not have a m.room.create in its auth_events, reject. creation_event = auth_dict.get((EventTypes.Create, ""), None) if not creation_event: raise AuthError(403, "No create event in auth events") + +def check_state_dependent_auth_rules( + event: "EventBase", + auth_events: Iterable["EventBase"], +) -> None: + """Check that an event complies with auth rules that depend on room state + + Runs through the parts of the auth rules that check an event against bits of room + state. + + Note: + + - 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 (including, but not limited to, check_state_independent_auth_rules). + + Args: + event: the event being checked. + auth_events: the room state to check the events against. + + Raises: + AuthError if the checks fail + """ + # there are no state-dependent auth rules for create events. + if event.type == EventTypes.Create: + logger.debug("Allowing! %s", event) + return + + auth_dict = {(e.type, e.state_key): e for e in auth_events} + # additional check for m.federate creating_domain = get_domain_from_id(event.room_id) originating_domain = get_domain_from_id(event.sender) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index ed4149bd58..a2dd9c7efa 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -23,7 +23,10 @@ from synapse.api.constants import ( ) from synapse.api.errors import AuthError, Codes, SynapseError from synapse.api.room_versions import RoomVersion -from synapse.event_auth import check_auth_rules_for_event +from synapse.event_auth import ( + check_state_dependent_auth_rules, + check_state_independent_auth_rules, +) from synapse.events import EventBase from synapse.events.builder import EventBuilder from synapse.events.snapshot import EventContext @@ -52,9 +55,10 @@ class EventAuthHandler: context: EventContext, ) -> None: """Check an event passes the auth rules at its own auth events""" + await check_state_independent_auth_rules(self._store, event) auth_event_ids = event.auth_event_ids() auth_events_by_id = await self._store.get_events(auth_event_ids) - check_auth_rules_for_event(event, auth_events_by_id.values()) + check_state_dependent_auth_rules(event, auth_events_by_id.values()) def compute_auth_events( self, diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 6c9e6a00b5..565ffd7cfd 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -50,7 +50,8 @@ from synapse.api.errors import ( from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion, RoomVersions from synapse.event_auth import ( auth_types_for_event, - check_auth_rules_for_event, + check_state_dependent_auth_rules, + check_state_independent_auth_rules, validate_event_for_room_version, ) from synapse.events import EventBase @@ -1430,7 +1431,9 @@ class FederationEventHandler: allow_rejected=True, ) - def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: + events_and_contexts_to_persist: List[Tuple[EventBase, EventContext]] = [] + + async def prep(event: EventBase) -> None: with nested_logging_context(suffix=event.event_id): auth = [] for auth_event_id in event.auth_event_ids(): @@ -1444,7 +1447,7 @@ class FederationEventHandler: event, auth_event_id, ) - return None + return auth.append(ae) # we're not bothering about room state, so flag the event as an outlier. @@ -1453,17 +1456,20 @@ class FederationEventHandler: context = EventContext.for_outlier(self._storage_controllers) try: validate_event_for_room_version(event) - check_auth_rules_for_event(event, auth) + await check_state_independent_auth_rules(self._store, event) + check_state_dependent_auth_rules(event, auth) except AuthError as e: logger.warning("Rejecting %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR - return event, context + events_and_contexts_to_persist.append((event, context)) + + for event in fetched_events: + await prep(event) - 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), + events_and_contexts_to_persist, # Mark these events backfilled as they're historic events that will # eventually be backfilled. For example, missing events we fetch # during backfill should be marked as backfilled as well. @@ -1515,7 +1521,8 @@ class FederationEventHandler: # ... and check that the event passes auth at those auth events. try: - check_auth_rules_for_event(event, claimed_auth_events) + await check_state_independent_auth_rules(self._store, event) + check_state_dependent_auth_rules(event, claimed_auth_events) except AuthError as e: logger.warning( "While checking auth of %r against auth_events: %s", event, e @@ -1563,7 +1570,7 @@ class FederationEventHandler: auth_events_for_auth = calculated_auth_event_map try: - check_auth_rules_for_event(event, auth_events_for_auth.values()) + check_state_dependent_auth_rules(event, auth_events_for_auth.values()) except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -1663,7 +1670,7 @@ class FederationEventHandler: ) try: - check_auth_rules_for_event(event, current_auth_events) + check_state_dependent_auth_rules(event, current_auth_events) except AuthError as e: logger.warning( "Soft-failing %r (from %s) because %s", diff --git a/synapse/state/v1.py b/synapse/state/v1.py index 8bbb4ce41c..500e384695 100644 --- a/synapse/state/v1.py +++ b/synapse/state/v1.py @@ -330,7 +330,7 @@ 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_auth_rules_for_event( + event_auth.check_state_dependent_auth_rules( event, auth_events.values(), ) @@ -347,7 +347,7 @@ def _resolve_normal_events( for event in _ordered_events(events): try: # The signatures have already been checked at this point - event_auth.check_auth_rules_for_event( + event_auth.check_state_dependent_auth_rules( event, auth_events.values(), ) diff --git a/synapse/state/v2.py b/synapse/state/v2.py index 6a16f38a15..7db032203b 100644 --- a/synapse/state/v2.py +++ b/synapse/state/v2.py @@ -573,7 +573,7 @@ async def _iterative_auth_checks( auth_events[key] = event_map[ev_id] try: - event_auth.check_auth_rules_for_event( + event_auth.check_state_dependent_auth_rules( event, auth_events.values(), ) -- cgit 1.4.1