Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Ensure that we correctly auth events returned by send_join #11012

Merged
merged 6 commits into from
Oct 25, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 12 additions & 18 deletions synapse/handlers/federation_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,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):
Expand Down Expand Up @@ -446,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")
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Sigh. Inevitably, I now can't repeat this. And I can't understand how it could have happened :/

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree

Copy link
Member Author

Choose a reason for hiding this comment

The 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 = [
Expand Down