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.
This commit is contained in:
Richard van der Hoff 2021-09-29 18:59:15 +01:00 committed by GitHub
parent a19aa8b162
commit 428174f902
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 177 additions and 172 deletions

1
changelog.d/10940.misc Normal file
View File

@ -0,0 +1 @@
Clean up some of the federation event authentication code for clarity.

View File

@ -41,52 +41,38 @@ 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)
if not hasattr(event, "room_id"):
raise AuthError(500, "Event has no room_id: %s" % event)
room_id = event.room_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.
for auth_event in auth_events.values():
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),
)
if do_sig_check:
# check that the event has the correct signatures
sender_domain = get_domain_from_id(event.sender)
is_invite_via_3pid = (
@ -125,6 +111,51 @@ def check(
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(
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),
)
# Implementation of https://matrix.org/docs/spec/rooms/v1#authorization-rules
#
# 1. If type is m.room.create:

View File

@ -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,

View File

@ -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)

View File

@ -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",

View File

@ -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)

View File

@ -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(

View File

@ -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:

View File

@ -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

View File

@ -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,
)