New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usages of event ID's domain #4514

Merged
merged 9 commits into from Jan 29, 2019

Conversation

3 participants
@erikjohnston
Copy link
Member

erikjohnston commented Jan 29, 2019

In future room version event IDs won't have a domain part

erikjohnston added some commits Jan 29, 2019

Only check event IDs domain signed event for V1 and V2
Since newer versions of events don't have the same format for event ID.
Remove event ID usage when checking if new room
The event ID is changing, so we can no longer get the domain from it. On
the other hand, the check is unnecessary.
Use snder and not event ID domain to check if ours
The transaction queue only sends out events that we generate. This was
done by checking domain of event ID, but that can no longer be used.
Instead, we may as well use the sender field.
Use event origin for filtering incoming events
We only process events sent to us from a server if the event ID matches
the server, to help guard against federation storms. We replace this
with a check against the event origin.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 29, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 29, 2019

I've realised I've messed up Only check event ID domain for signatures for V1 events, fixing now.

erikjohnston added some commits Jan 29, 2019

Only check event ID domain for signatures for V1 events
In future version events won't have an event ID, so we won't be able to
do this check.

@erikjohnston erikjohnston force-pushed the erikj/remove_event_id branch from ecd6c6b to b40abe0 Jan 29, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 29, 2019

Fixed now

@erikjohnston erikjohnston added this to To Do in Homeserver Task Board via automation Jan 29, 2019

@erikjohnston erikjohnston moved this from To Do to In progress in Homeserver Task Board Jan 29, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #4514 into develop will increase coverage by <.01%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #4514      +/-   ##
===========================================
+ Coverage    74.75%   74.75%   +<.01%     
===========================================
  Files          336      336              
  Lines        34219    34266      +47     
  Branches      5570     5583      +13     
===========================================
+ Hits         25580    25617      +37     
- Misses        7060     7065       +5     
- Partials      1579     1584       +5
@richvdh
Copy link
Member

richvdh left a comment

looks plausible otherwise

# Check the event_id's domain has signed the event
if not event.signatures.get(event_id_domain):
raise AuthError(403, "Event not signed by sending server")
if event.format_version in (RoomVersions.V1, RoomVersions.V2):

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

surely event.format_version should be an event version, not a room version?

# now let's look for events where the sender's domain is different to the
# event id's domain (normally only the case for joins/leaves), and add additional
# checks. Only do this if the room version has a concept of event ID domain
if room_version in KNOWN_ROOM_VERSIONS:

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

does this not need to be different?

This comment has been minimized.

@erikjohnston

erikjohnston Jan 29, 2019

Author Member

It currently gets done in #4515, I'm not sure why I didn't write it out fully here

@@ -243,32 +245,22 @@ def _check_sigs_on_pdus(keyring, pdus):
#
# let's start by getting the domain for each pdu, and flattening the event back
# to JSON.

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

could you update the comment at line 230 about event_id?

@richvdh
Copy link
Member

richvdh left a comment

lgtm

@erikjohnston erikjohnston merged commit 7740edd into develop Jan 29, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@richvdh richvdh moved this from In progress to Done in Homeserver Task Board Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment