-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Ensure that we correctly auth events returned by send_join
#11012
Changes from 2 commits
d37c283
44a0762
3701fee
50f58a0
32e9237
8c751f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,6 +362,7 @@ async def on_send_membership_event( | |
# need to. | ||
await self._event_creation_handler.cache_joined_hosts_for_event(event, context) | ||
|
||
await self._check_for_soft_fail(event, None, origin=origin) | ||
await self._run_push_actions_and_persist_event(event, context) | ||
return event, context | ||
|
||
|
@@ -407,16 +408,20 @@ async def process_remote_join( | |
Persists the event separately. Notifies about the persisted events | ||
where appropriate. | ||
|
||
Will attempt to fetch missing auth events. | ||
|
||
Args: | ||
origin: Where the events came from | ||
room_id, | ||
room_id: | ||
auth_events | ||
state | ||
event | ||
room_version: The room version we expect this room to have, and | ||
will raise if it doesn't match the version in the create event. | ||
|
||
Returns: | ||
The stream ID after which all events have been persisted. | ||
|
||
Raises: | ||
SynapseError if the response is in some way invalid. | ||
""" | ||
events_to_context = {} | ||
for e in itertools.chain(auth_events, state): | ||
|
@@ -445,24 +450,14 @@ async def process_remote_join( | |
if room_version.identifier != room_version_id: | ||
raise SynapseError(400, "Room version mismatch") | ||
|
||
missing_auth_events = set() | ||
# we should have all of the auth events in the chain. | ||
for e in itertools.chain(auth_events, state, [event]): | ||
for e_id in e.auth_event_ids(): | ||
if e_id not in event_map: | ||
missing_auth_events.add(e_id) | ||
|
||
for e_id in missing_auth_events: | ||
m_ev = await self._federation_client.get_pdu( | ||
[origin], | ||
e_id, | ||
room_version=room_version, | ||
outlier=True, | ||
timeout=10000, | ||
) | ||
if m_ev and m_ev.event_id == e_id: | ||
event_map[e_id] = m_ev | ||
else: | ||
logger.info("Failed to find auth event %r", e_id) | ||
logger.info( | ||
"Auth chain incomplete: %s refers to missing event %s", e, e_id | ||
) | ||
raise SynapseError(400, "Auth chain incomplete") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that you will not be able to join a room via a server that has missing events in its auth chain? Is that a situation we're in right now with Matrix HQ on various servers? Or do we always have all the events, they've just been rejected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. er, good question. I'm not entirely sure how to find out. (My private HS OOMs whenever I try to join HQ) I'm pretty sure that if any servers do have missing auth events, they're going to be pretty broken by not having an auth chain cover. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hrm, well, one problem it does introduce is that it makes old matrix-dev unjoinable, because there are events in there whose signatures cannot be verified. I think they should be dropped before we get here, so I'll dig a little more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sigh. Inevitably, I now can't repeat this. And I can't understand how it could have happened :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, the problem was that I had a copy of one of the now-unverifiable events in my database, but not all of its auth events, apparently. So this raises the question of: what should we do about events returned as part of a send_join whose auth_events are now unverifiable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the answer here is reasonably clear: we should do the same as we do when we fetch the state at a backwards extremity. Which is to say, we should simply exclude any bits of state whose auth events we cannot find. (Whether that is correct behaviour is highly debatable, but it's hard to do much about without substantial MSC1228-style redesign of the protocol). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, this is now done in 8c751f0. |
||
|
||
for e in itertools.chain(auth_events, state, [event]): | ||
auth_for_e = [ | ||
|
@@ -1013,11 +1008,19 @@ async def _process_received_pdu( | |
logger.exception("Unexpected AuthError from _check_event_auth") | ||
raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) | ||
|
||
if not backfilled and not context.rejected: | ||
# For new (non-backfilled and non-outlier) events we check if the event | ||
# passes auth based on the current state. If it doesn't then we | ||
# "soft-fail" the event. | ||
await self._check_for_soft_fail(event, state, origin=origin) | ||
|
||
await self._run_push_actions_and_persist_event(event, context, backfilled) | ||
|
||
if backfilled: | ||
if backfilled or context.rejected: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previously, we were doing the later stuff even if the event was rejected, which I think was a bug. |
||
return | ||
|
||
await self._maybe_kick_guest_users(event) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previously, we were doing this before persisting the event (which, at best, would create an unnecessary fork in the event dag), and also doing it for |
||
|
||
# For encrypted messages we check that we know about the sending device, | ||
# if we don't then we mark the device cache for that user as stale. | ||
if event.type == EventTypes.Encrypted: | ||
|
@@ -1445,10 +1448,6 @@ async def _check_event_auth( | |
except AuthError as e: | ||
logger.warning("Failed auth resolution for %r because %s", event, e) | ||
context.rejected = RejectedReason.AUTH_ERROR | ||
return context | ||
|
||
await self._check_for_soft_fail(event, state, backfilled, origin=origin) | ||
await self._maybe_kick_guest_users(event) | ||
|
||
return context | ||
|
||
|
@@ -1468,7 +1467,6 @@ async def _check_for_soft_fail( | |
self, | ||
event: EventBase, | ||
state: Optional[Iterable[EventBase]], | ||
backfilled: bool, | ||
origin: str, | ||
) -> None: | ||
"""Checks if we should soft fail the event; if so, marks the event as | ||
|
@@ -1477,15 +1475,8 @@ async def _check_for_soft_fail( | |
Args: | ||
event | ||
state: The state at the event if we don't have all the event's prev events | ||
backfilled: Whether the event is from backfill | ||
origin: The host the event originates from. | ||
""" | ||
# For new (non-backfilled and non-outlier) events we check if the event | ||
# passes auth based on the current state. If it doesn't then we | ||
# "soft-fail" the event. | ||
if backfilled or event.internal_metadata.is_outlier(): | ||
return | ||
Comment on lines
-1483
to
-1487
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
|
||
extrem_ids_list = await self._store.get_latest_event_ids_in_room(event.room_id) | ||
extrem_ids = set(extrem_ids_list) | ||
prev_event_ids = set(event.prev_event_ids()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the call to
check_event_auth
is a fair way up, so we're now only doing the soft_fail check if all the other checks pass.