Improve the server key exchange portion of the s2s specification #1423

turt2live · 2018-07-16T22:17:11Z

Rendered: See "docs" commit status.

Most of the text has been shuffled into the swagger definitions to bring it closer to where it matters.

This also attempts to clarify what is out in the wild. Most importantly, the first version of the key exchange is outright removed from the specification. Other research points/questions are:

What is a "Key ID"?
- https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/local_key_resource.py#L81-L83
- https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/local_key_resource.py#L88-L91
Returning a cached response if the server throws a 400, 500, or otherwise not-offline status code
- https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/remote_key_resource.py#L227-L229
minimum_valid_until_ts default
- This branch of the ladder: https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/remote_key_resource.py#L192
Returning empty arrays when querying offline/no servers
- Queried by hand against matrix.org as a notary server with a fake domain name to query
Returning all keys even when querying for specific keys
- Queried by hand using matrix.org as a notary server against a server publishing multiple keys.

The examples and descriptions were also improved as part of this commit.

Most of the text has been shuffled into the swagger definitions to bring it closer to where it matters. This also attempts to clarify what is out in the wild. Most importantly, the first version of the key exchange is outright removed from the specification. Other research points/questions are: * What is a "Key ID"? * https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/local_key_resource.py#L81-L83 * https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/local_key_resource.py#L88-L91 * Returning a cached response if the server throws a 400, 500, or otherwise not-offline status code * https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/remote_key_resource.py#L227-L229 * `minimum_valid_until_ts` default * This branch of the ladder: https://github.com/matrix-org/synapse/blob/1241156c82644d5609f45659607a356af5d8fe08/synapse/rest/key/v2/remote_key_resource.py#L192 * Returning empty arrays when querying offline/no servers * Queried by hand against matrix.org as a notary server with a fake domain name to query * Returning all keys even when querying for specific keys * Queried by hand using matrix.org as a notary server against a server publishing multiple keys. The examples and descriptions were also improved as part of this commit.

richvdh · 2018-07-26T21:57:33Z

api/server-server/definitions/keys.yaml

          required: true
-          example: "Base+64+Encoded+Signature+Verification+Key"
+          example: "VGhpcyBzaG91bGQgYmUgYSByZWFsIGVkMjU1MTkgcGF5bG9hZA=="


if it's unpadded base64... why is this padded?

(this is a theme throughout the examples)

because I did the examples before the docs in this case. Will fix.

richvdh · 2018-07-26T22:00:20Z

api/server-server/definitions/keys.yaml

        }
      }
      properties:
        expired_ts:
          type: integer
          format: int64
-          description: The expiration time.
+          description: POSIX timestamp for when this key expired.


normally we explicitly say it's in ms.

richvdh · 2018-07-26T22:01:23Z

api/server-server/definitions/keys.yaml

        }
      }
      properties:
        expired_ts:
          type: integer
          format: int64
-          description: The expiration time.
+          description: POSIX timestamp for when this key expired.
          required: true
          example: 922834800000


funny-looking timestamp.

It's from 1999. Considering this is the "expired" parameter, it seems fine? (it's also ported from ancient docs)

Edit: will make more modern.

Yeah, the problem is that I expect timestamps to start with 14 or 15 or so; when I see one starting with a 9 I wonder if it is actually a posix timestamp.

richvdh · 2018-07-26T22:03:47Z

api/server-server/keys_query.yaml

+      description: |-
+        Query for keys from multiple servers in a batch format. The receiving (notary)
+        server must sign the keys returned by the queried servers.
+      operationId: postQueryKeys


I'm not sure this is a better operationId

richvdh · 2018-07-26T22:07:34Z

api/server-server/keys_server.yaml

+      description: |-
+        Gets the homeserver's published TLS fingerprints and signing keys.
+        The homeserver may have any number of active keys and may have a
+        number of old keys. Homeservers SHOULD return a single JSON object


I'm not sure they SHOULD... tbh I think it's a bug in synapse that they do.

maybe we should deprecate the keyId parameter?

Deprecating seems best, given both synapse and dendrite ignore it.

richvdh · 2018-07-26T22:08:32Z

api/server-server/keys_server.yaml

      operationId: getServerKey
      parameters:
        - in: path
          name: keyId
          type: string
-          description: Key ID
+          description: |-
+            The key ID to look up. If omitted or empty, all server keys are


this conflicts with the text in the description,

How so? The argument is optional, so the last bit here is to reinforce what should happen when the parameter isn't provided.

well, it's incorrect by omission. Apparently it should be ignored?

I suppose. Will update alongside deprecation.

richvdh · 2018-07-26T22:09:46Z

specification/server_server_api.rst


-Each homeserver publishes its public keys under ``/_matrix/key/v2/server/``.
-Homeservers query for keys by either getting ``/_matrix/key/v2/server/``
+Each homeserver publishes its public keys under ``/_matrix/key/v2/server/{keyId}`.


richvdh · 2018-07-26T22:16:16Z

api/server-server/definitions/keys.yaml

@@ -87,10 +97,12 @@ properties:
      properties:
        sha256:
          type: string
-          description: The encoded fingerprint.
-          example: Base+64+Encoded+SHA-256-Fingerprint
+          description: The `Unpadded Base64`_ encoded fingerprint


given we now say this is unpadded base64, we can probably remove it from the description on tls_fingerprints.

richvdh · 2018-07-26T22:22:22Z

api/server-server/definitions/keys.yaml

+    description: |-
+      Public keys of the homeserver for verifying digital signatures. 
+
+      The object's key is the algorithm and version combined (``ed25519`` being the 


We should have some words here about what constitutes a valid version. I don't know if you want to grasp that particular nettle right now though.

Looks like synapse uses [a-zA-Z0-9_] - is it fair to say that it must match that regex?

Source: https://github.com/matrix-org/synapse/blob/f7b133fc267f92734b88000f0f0450b3118ca713/synapse/config/key.py#L159

yeah, that sounds good. let's go with it.

(cf https://github.com/matrix-org/matrix-doc/issues/666)

turt2live · 2018-07-27T14:23:54Z

@richvdh should be updated. Please take another look.

richvdh

looks amazing. sorry for forgetting to review it.

turt2live · 2018-08-01T13:44:53Z

\o/ one down, half a billion to go :)

Thanks for taking a look

Fixes matrix-org#1407

turt2live mentioned this pull request Jul 16, 2018

Convert server keys to swagger #1405

Merged

turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Jul 16, 2018

turt2live force-pushed the travis/s2s/keys branch 2 times, most recently from debc062 to 5fb786b Compare July 18, 2018 16:51

turt2live force-pushed the travis/s2s/keys branch from 5fb786b to 87d1672 Compare July 24, 2018 20:58

turt2live force-pushed the travis/s2s/keys branch from 87d1672 to 8e97b0c Compare July 26, 2018 14:41

turt2live requested a review from a team July 26, 2018 14:47

turt2live removed the blocked Something needs to be done before action can be taken on this PR/issue. label Jul 26, 2018

richvdh reviewed Jul 26, 2018

View reviewed changes

turt2live added 7 commits July 26, 2018 16:42

Deprecate key ID querying

2f71703

Undo accidental changes to the operation ID

9dbb627

Unpadded means unpadded

cad1db2

Timestamps should be in milliseconds

bdccfca

Formatting

8f1a4ae

Key versions must be [0-9a-zA-Z_]

e27f4a6

Use more modern timestamps

cafd1a9

richvdh assigned turt2live Jul 27, 2018

turt2live assigned richvdh and unassigned turt2live Jul 27, 2018

Remove more padding

73b0a03

turt2live mentioned this pull request Jul 31, 2018

Federation API r0 megathread #1464

Closed

35 tasks

richvdh approved these changes Aug 1, 2018

View reviewed changes

richvdh assigned turt2live and unassigned richvdh Aug 1, 2018

turt2live merged commit b96ee3e into matrix-org:master Aug 1, 2018

turt2live deleted the travis/s2s/keys branch August 1, 2018 13:46

richvdh mentioned this pull request Nov 22, 2022

Synapse uses deprecated keyId path param on /_matrix/key/v2/server matrix-org/synapse#14523

Closed

RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023

Fix anchor for room-knocking heading (matrix-org#1423)

0562091

Fixes matrix-org#1407

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the server key exchange portion of the s2s specification #1423

Improve the server key exchange portion of the s2s specification #1423

turt2live commented Jul 16, 2018 •

edited

richvdh Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018

richvdh Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018 •

edited

richvdh Jul 27, 2018

richvdh Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018

richvdh Jul 26, 2018

richvdh Jul 26, 2018

richvdh Jul 26, 2018

turt2live Jul 26, 2018

richvdh Jul 27, 2018

richvdh Jul 27, 2018

turt2live commented Jul 27, 2018

richvdh left a comment

turt2live commented Aug 1, 2018

Improve the server key exchange portion of the s2s specification #1423

Improve the server key exchange portion of the s2s specification #1423

Conversation

turt2live commented Jul 16, 2018 • edited

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

turt2live Jul 26, 2018 • edited

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

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

turt2live commented Jul 27, 2018

richvdh left a comment

Choose a reason for hiding this comment

turt2live commented Aug 1, 2018

turt2live commented Jul 16, 2018 •

edited

turt2live Jul 26, 2018 •

edited