e2ee/device verification: clarifications #1830

sumnerevans · 2024-05-28T00:07:15Z

e2ee/device verification: move general error handling under framework

Most of the "Error and exception handling" section is generally applicable to other verification methods besides SAS, so I moved those bullet points under "Key verification framework".
e2ee/device verification/error handling: fix typo start -> request

It used to be possible to create a start request without a request, but this was deprecated.
m.key.verification.start: remove wording that it is typically to-device

It can be either a to-device event or an in-room-event.
e2ee/device verification: normalize all links

Changes the format of all links in the Device Verification section to be:
```
[`m.key.verification.whatever`](#mkeyverificationwhatever)
```
This involved adding links in a bunch of places, but also changing many links that did not have code within them to have the backticks.
e2ee/device verification start: clarify required nature of transaction_id or m.relates_to

Add notes about the fact that the start event may be the first event sent during a verification process and that clients should handle other clients doing so, but not themselves send the start event first.

Signed-off-by: Sumner Evans sumner.evans@automattic.com

Pull Request Checklist

Pull request includes a changelog file
Pull request includes a sign off
Pull request is classified as 'other changes'

Preview: https://pr1830--matrix-spec-previews.netlify.app

content/client-server-api/modules/end_to_end_encryption.md

Most of the "Error and exception handling" section is generally applicable to other verification methods besides SAS, so I moved those bullet points under "Key verification framework". Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

It used to be possible to create a start request without a request, but this was deprecated. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

It can be either a to-device event or an in-room event. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

richvdh

Thanks for this, and thanks for structuring it in a way that makes it easy to review!

content/client-server-api/modules/end_to_end_encryption.md

uhoreg · 2024-06-05T21:20:31Z

content/client-server-api/modules/end_to_end_encryption.md

+-   When a device receives an unknown `transaction_id`, it should send an
+    appropriate `m.key.verification.cancel` message to the other device
+    indicating as such. This does not apply for inbound
+    `m.key.verification.request` or `m.key.verification.cancel` messages.


I just checked, and the spec still allows verifications to start with m.key.verification.start:

When verifying using to-device messages, an m.key.verification.start message can also be sent independently of any request, specifying the verification method to use. This behaviour is deprecated, and new clients should not begin verifications in this way. However, clients should handle such verifications started by other clients.

in https://spec.matrix.org/unstable/client-server-api/#key-verification-framework (second-last paragraph before https://spec.matrix.org/unstable/client-server-api/#mroommessagemkeyverificationrequest).

So this sentence should allow unknown transaction_id for m.key.verification.request, m.key.verification.start, and m.key.verification.cancel.

We should probably fix this documentation, then

Since the m.relates_to is not required if sending the start as the first message, right?

And the transaction_id is just arbitrary if not related to a request.

You can only initiate the verification with a .start if it's done using to-device messages. (Confusing, I know.) So the "Required when sent as an in-room message" for m.relates_to is still correct.

…amework Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

This commit changes the format of all links in the Device Verification section to be: [`m.key.verification.whatever`](#mkeyverificationwhatever) This involved adding links in a bunch of places, but also changing many links that did not have code within them to have the backticks. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

…n_id or m.relates_to Add notes about the fact that the start event *may* be the first event sent during a verification process and that clients should handle other clients doing so, but not themselves send the start event first. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

richvdh · 2024-06-11T14:39:39Z

Suddenly there's an awful lot changing in this PR, and it's quite hard to review...

richvdh

Thanks. A few nits.

richvdh · 2024-06-11T14:52:02Z

content/client-server-api/modules/end_to_end_encryption.md

-this example, Bob's device sends the `m.key.verification.start`, Alice's device
-could also send that message. As well, the order of the
-`m.key.verification.done` messages could be reversed.
+send [`m.key.verification.request`](#mkeyverificationrequest) events to all of


For what it's worth, it's not obvious to me that making every single reference to an event into a hyperlink improves readability - as long as the first reference is a hyperlink, I think that would be fine. (Also, it means that all the text in the section is being reformatted, which makes it very hard to see what's actually changing in the text.)

But given it's done now, I'll not insist on it being changed back.

richvdh · 2024-06-11T14:55:20Z

data/event-schemas/schema/m.key.verification.request.yaml

@@ -4,8 +4,9 @@ allOf:

 description: |-
  Requests a key verification using to-device messaging.  When requesting a key
-  verification in a room, a `m.room.message` should be used, with
-  [`m.key.verification.request`](#mroommessagemkeyverificationrequest) as msgtype.
+  verification in a room, a [`m.room.message`](#mroommessage should be used,


Suggested change

verification in a room, a [`m.room.message`](#mroommessage should be used,

verification in a room, a [`m.room.message`](#mroommessage) should be used,

richvdh · 2024-06-11T14:58:17Z

content/client-server-api/modules/end_to_end_encryption.md

-of his other devices may support the request. Instead, Bob's device should tell
-Bob that no supported method was found, and allow him to manually reject the
-request.
+Upon receipt of Alice's [`m.key.verification.request`](#mkeyverificationrequest)


I think linking to #mkeyverificationrequest incorrectly gives the impression that this only applies to to-device messages:

Suggested change

Upon receipt of Alice's [`m.key.verification.request`](#mkeyverificationrequest)

Upon receipt of Alice's `m.key.verification.request`

richvdh · 2024-06-11T15:00:45Z

content/client-server-api/modules/end_to_end_encryption.md

+    [`m.key.verification.request`](#mkeyverificationrequest),
+    [`m.key.verification.start`](#mkeyverificationstart), or
+    [`m.key.verification.cancel`](#mkeyverificationcancel) messages.


Again, this applies as much to in-room m.key.verification.request events as to-device events.

Suggested change

[`m.key.verification.request`](#mkeyverificationrequest),

[`m.key.verification.start`](#mkeyverificationstart), or

[`m.key.verification.cancel`](#mkeyverificationcancel) messages.

`m.key.verification.request`, `m.key.verification.start` or

`m.key.verification.cancel` messages.

richvdh · 2024-06-11T15:01:11Z

content/client-server-api/modules/end_to_end_encryption.md

@@ -643,8 +655,8 @@ success. A failed attack would result in a mismatched Short
 Authentication String, alerting users to the attack.

 To advertise support for this method, clients use the name `m.sas.v1` in the
-`methods` fields of the `m.key.verification.request` and
-`m.key.verification.ready` events.
+`methods` fields of the [`m.key.verification.request`](#mkeyverificationrequest)


richvdh · 2024-06-11T15:11:16Z

data/event-schemas/schema/m.key.verification.m.relates_to.yaml

-      The event ID of the `m.key.verification.request` that this message is
-      related to.
+      The event ID of the
+      [`m.key.verification.request`](#mkeyverificationrequest) that this


Suggested change

[`m.key.verification.request`](#mkeyverificationrequest) that this

[`m.key.verification.request`](#mroommessagemkeyverificationrequest) that this

richvdh · 2024-06-11T15:14:42Z

data/event-schemas/schema/m.key.verification.start.yaml

+          Note that sending a start event without a request is deprecated, and
+          clients should not send a start event without first sending a request
+          event, but clients should handle other clients doing so.


Suggested change

Note that sending a start event without a request is deprecated, and

clients should not send a start event without first sending a request

event, but clients should handle other clients doing so.

Note that sending a `start` event without a `request` is deprecated, and

clients should not send a `start` event without first sending a `request`

event, but clients should handle other clients doing so.

richvdh · 2024-06-11T15:15:05Z

data/event-schemas/schema/m.key.verification.start.yaml

+          Note that sending a start event without a request is deprecated, and
+          clients should not send a start event without first sending a request
+          event, but clients should handle other clients doing so.


Suggested change

Note that sending a start event without a request is deprecated, and

clients should not send a start event without first sending a request

event, but clients should handle other clients doing so.

Note that sending a `start` event without a `request` is deprecated, and

clients should not send a `start` event without first sending a `request`

event, but clients should handle other clients doing so.

richvdh · 2024-06-11T15:15:29Z

data/event-schemas/schema/m.key.verification.start.yaml

+          Required when sent as an in-room message unless the start event is
+          sent without a corresponding
+          [`m.key.verification.request`](#mkeyverificationrequest).
+
+          Indicates the
+          [`m.key.verification.request`](#mkeyverificationrequest) that this
+          message is related to. Note that for encrypted messages, this
+          property should be in the unencrypted portion of the event.


again, these are linking to the wrong m.key.verification.request

richvdh · 2024-06-11T15:17:17Z

data/event-schemas/schema/m.key.verification.start.yaml

+        properties:
+          rel_type:
+            type: string
+            enum:
+              - m.reference
+            description: |-
+              The relationship type. Currently, this can only be an
+              [`m.reference`](/client-server-api/#reference-relations)
+              relationship type.
+          event_id:
+            type: string
+            description: |-
+              The event ID of the
+              [`m.key.verification.request`](#mkeyverificationrequest) that
+              this message is related to.
+        type: object
+        title: VerificationRelatesTo


it should be possible to inherit these, but override the description. Probably something like:

m.relates_to: allOf: - $ref: m.key.verification.m.relates_to.yaml - description: |- ... blah

richvdh · 2024-10-29T14:50:03Z

@sumnerevans are you still interested in working on this?

sumnerevans commented May 28, 2024

View reviewed changes

content/client-server-api/modules/end_to_end_encryption.md Show resolved Hide resolved

richvdh marked this pull request as ready for review May 29, 2024 14:43

richvdh requested a review from a team as a code owner May 29, 2024 14:43

richvdh mentioned this pull request May 29, 2024

Clean up pull request template #1831

Merged

sumnerevans added 4 commits May 29, 2024 22:04

e2ee/device verification/error handling: fix typo start -> request

01a816d

It used to be possible to create a start request without a request, but this was deprecated. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

m.key.verification.start: remove wording that it is typically to-device

8905029

It can be either a to-device event or an in-room event. Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

Add changelog

b8808ca

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

sumnerevans force-pushed the device-verification-clarifications branch from fa53f58 to b8808ca Compare May 30, 2024 04:05

sumnerevans requested a review from uhoreg May 30, 2024 04:05

richvdh requested changes Jun 4, 2024

View reviewed changes

uhoreg reviewed Jun 5, 2024

View reviewed changes

sumnerevans and others added 7 commits June 7, 2024 10:28

fixup! e2ee/device verification: move general error handling under fr…

ea9dc84

…amework Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

fixup! e2ee/device verification: move general error handling under fr…

edc4f38

…amework Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

fixup! e2ee/device verification: move general error handling under fr…

3fbb048

…amework Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

fixup! e2ee/device verification: move general error handling under fr…

d6c6a03

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

fixup! e2ee/device verification: move general error handling under fr…

2ea4e13

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

fixup! e2ee/device verification: move general error handling under fr…

d66a805

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

fixup! e2ee/device verification: move general error handling under fr…

55375f4

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

sumnerevans force-pushed the device-verification-clarifications branch from 3aad3c5 to 55375f4 Compare June 7, 2024 16:41

sumnerevans added 2 commits June 7, 2024 10:44

fixup! e2ee/device verification: move general error handling under fr…

d10deb0

…amework Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

sumnerevans force-pushed the device-verification-clarifications branch from 17f9944 to b87ffa6 Compare June 7, 2024 17:11

sumnerevans added 3 commits June 7, 2024 11:18

fixup! Add changelog

a79d5a6

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

fixup! e2ee/device verification: normalize all links

a9350fc

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

sumnerevans requested review from richvdh and uhoreg June 7, 2024 17:37

richvdh requested changes Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2ee/device verification: clarifications #1830

e2ee/device verification: clarifications #1830

sumnerevans commented May 28, 2024 •

edited by github-actions bot

Loading

richvdh left a comment

uhoreg Jun 5, 2024

sumnerevans Jun 7, 2024

sumnerevans Jun 7, 2024

uhoreg Jun 11, 2024

richvdh commented Jun 11, 2024

richvdh left a comment

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh Jun 11, 2024

richvdh commented Oct 29, 2024

	verification in a room, a [`m.room.message`](#mroommessage should be used,
	verification in a room, a [`m.room.message`](#mroommessage) should be used,

	Upon receipt of Alice's [`m.key.verification.request`](#mkeyverificationrequest)
	Upon receipt of Alice's `m.key.verification.request`

	[`m.key.verification.request`](#mkeyverificationrequest) that this
	[`m.key.verification.request`](#mroommessagemkeyverificationrequest) that this

e2ee/device verification: clarifications #1830

Are you sure you want to change the base?

e2ee/device verification: clarifications #1830

Conversation

sumnerevans commented May 28, 2024 • edited by github-actions bot Loading

Pull Request Checklist

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jun 11, 2024

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Oct 29, 2024

sumnerevans commented May 28, 2024 •

edited by github-actions bot

Loading