MSC2366: Key verification flow additions: m.key.verification.ready and m.key.verification.done #2366
Conversation
turt2live
changed the title
m.key.verification.acceptm.key.verification.accept
uhoreg
changed the title
m.key.verification.accept
turt2live
changed the title
|
@anoadragon453 I've added an example. |
turt2live
changed the title
uhoreg
changed the title
uhoreg
changed the title
|
@mscbot fcp merge |
|
Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
mscbot
added
disposition-merge
proposed-final-comment-period
uhoreg
changed the title
| and in to-device messages to accept verifications requested using an | ||
| `m.key.verification.request` event. | ||
|
|
||
| The second event type is `m.key.verification.done`, which has no fields other |
There was a problem hiding this comment.
How long should the other client wait until it receives a done, as old clients won't send that? The normal timeout of 10min sounds pretty long.
There was a problem hiding this comment.
I have the same question. Riot doesn't send this, when you are not verifying in DMs it seems. So we kinda need a behaviour for the transition phase, when a client follows the current spec.
There was a problem hiding this comment.
It definitely should be sent in DMs.
There was a problem hiding this comment.
when you are not verifying in DMs
It seems to be only sent, when verifying via DMs. So it seems like it is not sent in to_device messaging. This proposal suggests, that clients would be sending them already, but they don't. Unless I am missing something.
There was a problem hiding this comment.
Ah, sorry. I read your sentence several times, and managed to miss the "not" every time. :-/
There was a problem hiding this comment.
I just tried in Element Web in a non-DM self-verification, and it does send an m.key.verification.ready in response to m.key.verification.request. Trying to directly verify another user's device seems to send an m.key.verification.start directly, so it is not affected by this case.
I haven't tried Element Android or Element iOS yet.
There was a problem hiding this comment.
Yes, that matches my tests as well. It can deal with request, but if you start it from Element, it uses start (for to_device verifications).
There was a problem hiding this comment.
I just tested Element Web, and it sent an m.key.verification.done after verifying using to-device. I haven't tested Android or iOS. In response to the original question in this thread:
How long should the other client wait until it receives a done, as old clients won't send that?
I think that we're at the point where we don't need to worry about "old" clients (it looks like Element is doing this, and just waiting until the normal timeout), but if you do want to worry about old clients, you could 1) wait for the .done, but allow the user to exit the verification UI, or 2) exit the verification UI once it's completed on your side, without waiting for the .done to come in from the other user.
There was a problem hiding this comment.
Does element-web also send a .done if you don't initiate verification via popup but via the device menu on the right? The device menu on the right also skips the .request and starts directly with a .start, so yeah
There was a problem hiding this comment.
Yes. I tested initialting verification both via the popup and via the device menu. In both situations, it sent the .done in my tests. (I also noticed that initiating from the device menu skipped the .request step.)
| `m.key.verification.start` event to begin the verification. If both parties | ||
| send an `m.key.verification.start` event, and they both specify the same | ||
| verification method, then the event sent by the user whose user ID is the | ||
| lexicographically smallest is used, and the other `m.key.verification.start` event is ignored. |
There was a problem hiding this comment.
It's not immediately obvious that this algorithm solves the glare proble. I think it is robust, but when it lands in the specit would be good to clairify exactly what "used" means here, and to add some walk throughs and/or sequence diagrams.
There was a problem hiding this comment.
An implementation of this proposal is currently available in Element Web.
LGTM.
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
mscbot
added
final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
mscbot
added
finished-final-comment-period
and removed
disposition-merge
final-comment-period
turt2live
added
spec-pr-missing
turt2live
added
spec-pr-in-review
uhoreg
added
merged
|
Merged! 🎉 |
Rendered