Skip to content
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

Merged
merged 19 commits into from
Apr 18, 2021

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Aug 22, 2019

@uhoreg uhoreg changed the title [WIP] MSCxxxx: key verification in DMs [WIP] MSC2241: key verification in DMs Aug 22, 2019
@uhoreg uhoreg added the proposal A matrix spec change proposal label Aug 22, 2019
@uhoreg
Copy link
Member Author

uhoreg commented Sep 5, 2019

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Sep 5, 2019

Team member @uhoreg 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 none object), 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 info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Sep 5, 2019
@uhoreg uhoreg changed the title [WIP] MSC2241: key verification in DMs MSC2241: key verification in DMs Sep 5, 2019
@turt2live turt2live self-requested a review September 5, 2019 19:59
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Show resolved Hide resolved

Users might have multiple Direct Messaging rooms with other users. In this
case, clients will need to prompt the user to select the room in which they
want to perform the verification.
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this works fine if a client gets a key verification method via a non-DM room? One of the things with the current canonical DM proposal is that it doesn't actually guarantee there will be a single DM room at any given point (given glare and upgrading rooms and servers not agreeing etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, nothing here actually requires the room to be a DM, but clients should use a DM to avoid cluttering unrelated rooms.

Copy link
Member

Choose a reason for hiding this comment

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

One of the things with the current canonical DM proposal is that it doesn't actually guarantee there will be a single DM room at any given point (given glare and upgrading rooms and servers not agreeing etc)

This is a bug in the proposal intending to be fixed. There will only ever be one DM by the time the proposal gets merged.

visible to other users in the room. However, key verification does not rely on
secrecy, so this will no affect the security of the key verification. This may
reveal to others in the room that Alice and Bob know each other, but this is
already revealed by the fact that they share a Direct Messaging room.
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 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?

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

@uhoreg uhoreg requested a review from dbkr October 11, 2019 19:13
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

I very much like the idea of the MSC, but the chosen event definition looks unnecessarily heavy to me.

proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
proposals/2241-e2e-verification-in-dms.md Outdated Show resolved Hide resolved
uhoreg and others added 2 commits October 23, 2019 15:30
Co-Authored-By: Kitsune Ral <Kitsune-Ral@users.sf.net>
Co-Authored-By: David Baker <dbkr@users.noreply.github.com>
@richvdh
Copy link
Member

richvdh commented Apr 9, 2021

well, as @deepbluev7 pointed out, I was being a massive crank, so

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Apr 9, 2021

An FCP proposal is already in progress.

@richvdh
Copy link
Member

richvdh commented Apr 9, 2021

oh I'm still being a crank. The outstanding concern is:

The question of m.relationship vs m.relates_to in MSC1849 should be resolved.

it feels like we've resolved this by doing nothing about it. Changing the decision now is going to be a major upheaval. @uhoreg can we resolve this?

proposals/2241-e2e-verification-in-dms.md Show resolved Hide resolved
started
- `method`: the key verification method that is being used. This should be a
method that both Alice's and Bob's devices support.
- `from_device`: The user's device ID.
Copy link
Contributor

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 to m.key.verification.start?

Copy link
Contributor

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.

Copy link
Contributor

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:

  1. Alice sends a m.room.message event to the room specifying the target and includes supported methods and a fallback message for clients lacking support.
  2. Bob accepts the verification request with a m.key.verification.ready event and includes their supported methods.
  3. Either Alice or Bob selects a method and initiates the verification with a m.key.verification.start event.
  4. Verification proceeds according to the selected method.
  5. Both Alice and Bob send a m.key.verification.done event.

I think this gives a quick overview and can help speeding up the rest of the concrete details.

Copy link
Member Author

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.

@uhoreg
Copy link
Member Author

uhoreg commented Apr 13, 2021

I've decoupled this MSC from the aggregations MSC by just saying that we're basing the property shape on what the aggregations MSC currently says, but we're going to be independent. If the aggregations MSC changes the property shape in the future, and we really want to re-sync, then we can introduce a new MSC to do so.

So...

@mscbot resolve The question of m.relationship vs m.relates_to in MSC1849 should be resolved.

@mscbot
Copy link
Collaborator

mscbot commented Apr 13, 2021

Unknown concern 'The question of m.relationship vs m.relates_to in MSC1849 should be resolved.'.

@mscbot
Copy link
Collaborator

mscbot commented Apr 13, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 13, 2021
@anoadragon453
Copy link
Member

(Concern resolved manually)

@mscbot
Copy link
Collaborator

mscbot commented Apr 18, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Apr 18, 2021
@turt2live turt2live merged commit 058ce53 into matrix-org:master Apr 18, 2021
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Apr 18, 2021
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Apr 21, 2021
@uhoreg
Copy link
Member Author

uhoreg commented Apr 22, 2021

Merged! 🎉

@uhoreg uhoreg added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 22, 2021
@bwindels
Copy link
Contributor

bwindels commented Jul 5, 2021

For posterity, I've written #3267 to define how relations are used in this MSC (e.g. rel_type of m.reference)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal unresolved-concerns This proposal has at least one outstanding concern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSC for DMs as the channel for SAS verification