Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2241: Key verification in DMs #2241
MSC2241: Key verification in DMs #2241
Changes from 16 commits
4f65524
fb724bc
0e6286f
0007498
e19fca7
7469198
ab3ed73
1f1d22f
06ee66d
1d165ee
788e987
7466955
f4bad37
a514485
1590ae2
91f51bb
4a77978
affa240
6096416
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It feels a bit sad that we're using m.room.message here. I guess it's for the benefit of clients which don't support this method, but maybe an alternative would be to send two events, one m.room.message for the fallback, which supporting clients can ignore, and another m.key.verification.request which actually starts the process
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.
Yes, it's so that clients that don't support it will show something sensible. Sending two messages seems redundant, and you'd need some markup on the
m.room.message
so that supporting clients will know to ignore it, so it doesn't seem like much of a difference from just sticking the verification request payload in them.room.message
.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.
Why do we need a three-way handshake? Once Alice starts that verification why does Bob need a
m.key.verification.ready
? Can he not just jump tom.key.verification.start
?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.
It allows negotiating a common set of methods and then picking your preferred one. A client needs to send all methods it supports in start, but it may prefer a specific one, if supported. Then the other client can reply with the ones it prefers in "ready" and then the first client can choose its preferred method. If you reduce that to 2 steps, Either the first client sends it preferred methods instead of all of them, which may lead to no matching methods or it the second client picks one that is suboptimal for the first clients screen. Having 3 steps avoids that pretty much.
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.
I see. Would it make sense to make that a bit more clear? It may be helpful to prove a high-level overview of the flow somewhere. Maybe the following can be inserted:
At a high level a successful verification looks like this:
m.room.message
event to the room specifying the target and includes supported methods and a fallback message for clients lacking support.m.key.verification.ready
event and includes their supported methods.m.key.verification.start
event.m.key.verification.done
event.I think this gives a quick overview and can help speeding up the rest of the concrete details.
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 reasoning for
m.key.verification.ready
is given in #2366 so I don't think we need to repeat it here.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.
As noted by @poljar, this makes it very complicated to implement this MSC in Ruma, since previously we could just have a single content struct definition for every event type. With this, we'd have to use different definitions per event "kind" (ephemeral / message / state / to-device / ...).
Is there any chance of unified content objects between the to-device and message events?
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.
I guess this is no longer relevant. This has been implemented, it turned out manageable. I can't mark it as resolved myself though.
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.
Does this mean that clients have to be careful to ignore key verification messages not "directed" at them? Is there a situation where the DMness of a room is inconsistent between servers? E.g. you are in a 3-way room that the one of the other participants thinks is a DM room, but you don't, what happens?
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.
I think that clients should be able to respond to requests in non-DM rooms. So if clients don't agree on which rooms are DM, it should still work. If you try to do a verification in Matrix HQ, you'll just annoy everyone and someone will tell you to do it elsewhere.