Clarify how third party invites work #1505

turt2live · 2018-08-13T23:02:09Z

Rendered: see 'docs' commit status. This is highly recommended given the large sequence diagrams in the client-server spec.

This adds several diagrams to the Client-Server API about how invites are handled, including what the server is expected to do. This helps implementors know what they are supposed to do in the common cases, and infer where needed to get the more complex cases correct.

Although lacking in some areas, this is how third party invites work today.

A link to the now-improved client-server documentation for third party invites has been added to the server-server specification, alongside some documentation on how the /3pid/onbind endpoint works.

Fixes #1366
Fixes #1422

This adds several diagrams to the Client-Server API about how invites are handled, including what the server is expected to do. This helps implementors know what they are supposed to do in the common cases, and infer where needed to get the more complex cases correct. Although lacking in some areas, this is how third party invites work today. A link to the now-improved client-server documentation for third party invites has been added to the server-server specification. The existing server-server specification needed no further changes on the subject. Fixes matrix-org#1366

Fixes matrix-org#1422

dbkr

otherwise this looks great!

dbkr · 2018-08-17T10:22:25Z

api/identity/pubkey.yaml

@@ -56,7 +56,7 @@ paths:
    get:
      summary: Check whether a long-term public key is valid.
      description: |-
-        Check whether a long-term public key is valid.
+        Check whether a long-term public key is valid. The request must be idempotent.


Hmm - presumably this is saying it should always return the same data for a given key ID? It's slightly debatable whether idempotency strictly means this since it generally asserts that the operation will not be performed again, not that the resource doesn't change by other means in the intervening time. A GET request should always be idempotent since it should have no side effect.

Might be clearer to just say it should always be the same for a given key ID. :)

turt2live · 2018-08-17T15:50:10Z

@dbkr please take a look at the updated wording. It's slightly different than your suggestion as I want to make sure people don't cache the request as invalid forever, even if the key becomes valid at a later date.

dbkr · 2018-08-29T17:37:38Z

Actually, hang on - I thought the point of this endpoint was mostly to support key revocation, so the point of it is that it might later say a key is not valid that previously was? Also, in your suggestion of it going invalid -> valid at some point, this also would violate your condition of it always being the same?

turt2live · 2018-08-29T17:43:14Z

If the key can become invalid at a later point then third party invites would be rejected by servers as they attempt to verify them.

The updated wording for checking for valid keys should support only the case of the server publishing new keys. If the IS were required to always say the same thing for a given request then someone could easily reserve a ton of keys on the server that it then has to consider as invalid forever. With the condition that it has to consider a key that exists as (in)valid forever, the server isn't open to being abused.

dbkr · 2018-08-29T18:53:14Z

Hmm, the thing is that the intro for that section says:

is, the In the event of key compromise, the identity service may revoke any of its keys. An HTTP API is offered to get public keys, and check whether a particular key is valid.

Which surely is saying it exists so the ISes can revoke keys? I was afraid it might be that subsequent servers validating the 3pid invite would get a different result - this seems a bit broken? Not sure how best to untangle this mess.

turt2live · 2018-08-29T19:03:03Z

The ability to revoke keys is important. I'm sure there's a hole somewhere in this suggestion, but how does altering the auth rules slightly sound? The auth rules could consider the age of the event (by depth or ts or something) as a heuristic for "this was probably valid at one point, but it's so old now that an invalid key isn't unexpected". It should still check the key, and maybe just not send the event to clients if the key is invalid and old, but otherwise accept it?

This does have the problem where if someone does a 3rd party invite and moments later the IS decides it needs to rotate keys - the fresh invite would be considered as invalid by a subset of servers and reject it.

dbkr · 2018-08-29T20:01:40Z

I think the hole may be that a server can come up with an event supposedly from an old fork (but that actually it's just made up) referencing an old 3pid invite with a revoked key, and this will always be accepted. I was about to question why the validity endpoint doesn't return a not-before & not-after like an x.509 cert, but I think this applies here too in that whatever server is making the join can just pick whatever time at which the key was valid.

Might be good to get @erikjohnston / @richvdh 's thoughts here. There's a certain argument for speccing what we have and trying to unravel this later, even if that means something like saying you actually can't ever revoke keys (which presumably is currently the case).

richvdh · 2018-08-30T10:02:46Z

specification/modules/third_party_invites.rst

+        |              |                     | (Federation) Emit m.room.member for invite     |                 |                            |
+        |              |                     |----------------------------------------------->|                 |                            |
+        |              |                     |                                                |                 |                            |
+        |              |                     |                                                | Accept event    |                            |


why is "accept event" drawn in for m.room.member but not m.room.third_party_invite? I'm not sure it adds any value here.

It's a subtle thing where the event in this case must be accepted, which is why it is annotated specifically.

hrm. I'm not sure that this arrow shows that clearly.

I'll just take the arrow out - the surrounding text seems to cover the subtly well enough imo.

richvdh · 2018-08-30T10:04:57Z

specification/modules/third_party_invites.rst

+        |              |                     |                  |                             |                 |                            |
+        |              |                     |<------------------                             |                 |                            |
+        |              |                     |                                                |                 |                            |
+        |              |                     | (Federation) Emit m.room.member for invite     |                 |                            |


does there not need to be a send_join dance before this can happen?

Nope, the user doesn't join. The exchange of the 3rd party invite produces a real invite, which just has to be broadcast to all the other servers

richvdh · 2018-08-30T10:24:48Z

I'm not quite sure I understand the discussion here. If the IS wants to revoke its key, it stops replying yes to "/isvalid". Does that break something?

There's a certain argument for speccing what we have and trying to unravel this later,

+1 to that, I fear

turt2live · 2018-08-30T12:47:08Z

I'm not quite sure I understand the discussion here. If the IS wants to revoke its key, it stops replying yes to "/isvalid". Does that break something?

Yes, otherwise valid 3rd party invite events get rejected by servers that weren't lucky enough to check the event when the key was valid.

richvdh · 2018-08-30T19:55:37Z

Yes, otherwise valid 3rd party invite events get rejected by servers that weren't lucky enough to check the event when the key was valid.

Oh right. I hadn't grokked that other HSes needed to verify the signature at the point they see the 3pid invite - the only time 'isvalid' is mentioned in the spec is at the point H1 turns it into a normal invite. TBH this sounds like a questionable decision, but it's probably not worth debating at this point.

Given what you say, I agree that the correct decision is to spec it as-is, even if that means that keys can't be revoked, and then fix it asap.

richvdh · 2018-08-30T19:56:45Z

specification/modules/third_party_invites.rst

@@ -102,26 +141,85 @@ No other homeservers may reject the joining of the room on the basis of
 the room. They may, however, indicate to their clients that a member's'


They can't because otherwise 3rd party invites can be rejected by homeservers, as per https://github.com/matrix-org/matrix-doc/issues/1633

This doesn't add anything in terms of clarity.

turt2live · 2018-08-30T21:02:25Z

@richvdh and/or @dbkr - please take a look. I've opened https://github.com/matrix-org/matrix-doc/issues/1633 to cover the case of identity services not actually being able to revoke their keys.

richvdh

well, it looks sensible to me.

turt2live added 2 commits August 13, 2018 16:29

Spec /3pid/onbind

a556e33

Fixes matrix-org#1422

turt2live requested a review from a team August 13, 2018 23:02

turt2live mentioned this pull request Aug 13, 2018

Federation API r0 megathread #1464

Closed

35 tasks

dbkr reviewed Aug 17, 2018

View reviewed changes

Clarify how /isvalid is meant to always be truthful

3de50cb

turt2live requested a review from dbkr August 17, 2018 15:50

turt2live assigned dbkr Aug 20, 2018

turt2live mentioned this pull request Aug 30, 2018

Touch up more of the identity service specification #1615

Merged

turt2live changed the title ~~Clarify how third party invites works~~ Clarify how third party invites work Aug 30, 2018

richvdh reviewed Aug 30, 2018

View reviewed changes

turt2live mentioned this pull request Mar 1, 2022

Identity services should be able to revoke all their keys on a whim matrix-org/matrix-spec#362

Open

turt2live added 4 commits August 30, 2018 15:00

Comment out the part where identity services can revoke their keys

5e6a2c3

They can't because otherwise 3rd party invites can be rejected by homeservers, as per https://github.com/matrix-org/matrix-doc/issues/1633

Remove unhelpful arrow from the 3rd party invite sequence dance

3566268

This doesn't add anything in terms of clarity.

Fix typo

1f6499d

Merge branch 'm-master' into travis/general/3pid_invite

d57b977

richvdh approved these changes Aug 30, 2018

View reviewed changes

turt2live merged commit 38ae166 into matrix-org:master Aug 31, 2018

turt2live deleted the travis/general/3pid_invite branch August 31, 2018 14:38

ShadowJonathan mentioned this pull request Mar 1, 2022

Centralize event/object schema checking matrix-org/synapse#8361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify how third party invites work #1505

Clarify how third party invites work #1505

turt2live commented Aug 13, 2018

dbkr left a comment

dbkr Aug 17, 2018

turt2live commented Aug 17, 2018

dbkr commented Aug 29, 2018

turt2live commented Aug 29, 2018

dbkr commented Aug 29, 2018

turt2live commented Aug 29, 2018 •

edited

Loading

dbkr commented Aug 29, 2018

richvdh Aug 30, 2018

turt2live Aug 30, 2018

richvdh Aug 30, 2018

turt2live Aug 30, 2018

richvdh Aug 30, 2018

turt2live Aug 30, 2018

richvdh commented Aug 30, 2018

turt2live commented Aug 30, 2018

richvdh commented Aug 30, 2018

richvdh Aug 30, 2018

turt2live commented Aug 30, 2018

richvdh left a comment

		@@ -102,26 +141,85 @@ No other homeservers may reject the joining of the room on the basis of
		the room. They may, however, indicate to their clients that a member's'

Clarify how third party invites work #1505

Clarify how third party invites work #1505

Conversation

turt2live commented Aug 13, 2018

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live commented Aug 17, 2018

dbkr commented Aug 29, 2018

turt2live commented Aug 29, 2018

dbkr commented Aug 29, 2018

turt2live commented Aug 29, 2018 • edited Loading

dbkr commented Aug 29, 2018

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 Aug 30, 2018

turt2live commented Aug 30, 2018

richvdh commented Aug 30, 2018

Choose a reason for hiding this comment

turt2live commented Aug 30, 2018

richvdh left a comment

Choose a reason for hiding this comment

turt2live commented Aug 29, 2018 •

edited

Loading