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

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

Merged
merged 9 commits into from
Aug 1, 2018
Merged
46 changes: 29 additions & 17 deletions api/server-server/definitions/keys.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,50 +20,60 @@ properties:
server_name:
type: string
description: DNS name of the homeserver.
required: true # TODO: Verify
required: true
example: "example.org"
verify_keys:
type: object
description: Public keys of the homeserver for verifying digital signatures.
required: true # TODO: Verify
description: |-
Public keys of the homeserver for verifying digital signatures.

The object's key is the algorithm and version combined (``ed25519`` being the
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

algorithm and ``abc123`` being the version in the example below). Together,
this forms the Key ID.
required: true
additionalProperties:
type: object
title: Verify Key
example: {
"ed25519:auto2": {
"key": "Base+64+Encoded+Signature+Verification+Key"
"ed25519:abc123": {
"key": "VGhpcyBzaG91bGQgYmUgYSByZWFsIGVkMjU1MTkgcGF5bG9hZA=="
}
}
properties:
key:
type: string
description: The key
description: The `Unpadded Base64`_ encoded key.
required: true
example: "Base+64+Encoded+Signature+Verification+Key"
example: "VGhpcyBzaG91bGQgYmUgYSByZWFsIGVkMjU1MTkgcGF5bG9hZA=="
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(this is a theme throughout the examples)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

old_verify_keys:
type: object
description: The public keys that the server used to use and when it stopped using them.
description: |-
The public keys that the server used to use and when it stopped using them.

The object's key is the algorithm and version combined (``ed25519`` being the
algorithm and ``0ldK3y`` being the version in the example below). Together,
this forms the Key ID.
additionalProperties:
type: object
title: Old Verify Key
example: {
"ed25519:auto1": {
"ed25519:0ldK3y": {
"expired_ts": 922834800000,
"key": "Base+64+Encoded+Signature+Verification+Key"
"key": "VGhpcyBzaG91bGQgYmUgeW91ciBvbGQga2V5J3MgZWQyNTUxOSBwYXlsb2FkLg=="
}
}
properties:
expired_ts:
type: integer
format: int64
description: The expiration time.
description: POSIX timestamp for when this key expired.
Copy link
Member

Choose a reason for hiding this comment

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

normally we explicitly say it's in ms.

required: true
example: 922834800000
Copy link
Member

Choose a reason for hiding this comment

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

funny-looking timestamp.

Copy link
Member Author

@turt2live turt2live Jul 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

key:
type: string
description: The key.
description: The `Unpadded Base64`_ encoded key.
required: true
example: "Base+64+Encoded+Signature+Verification+Key"
example: "VGhpcyBzaG91bGQgYmUgeW91ciBvbGQga2V5J3MgZWQyNTUxOSBwYXlsb2FkLg=="
signatures:
type: object
description: Digital signatures for this object signed using the ``verify_keys``.
Expand All @@ -72,7 +82,7 @@ properties:
title: Signed Server
example: {
"example.org": {
"ad25519:auto2": "Base+64+Encoded+Signature+Verification+Key"
"ad25519:abc123": "VGhpcyBzaG91bGQgYWN0dWFsbHkgYmUgYSBzaWduYXR1cmU="
}
}
additionalProperties:
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

example: "VGhpcyBpcyBoYXNoIHdoaWNoIHNob3VsZCBiZSBieXRlcw=="
valid_until_ts:
type: integer
format: int64
description: POSIX timestamp when the list of valid keys should be refreshed.
description: |-
POSIX timestamp when the list of valid keys should be refreshed. Keys used beyond this
timestamp are no longer valid.
example: 1052262000000
4 changes: 2 additions & 2 deletions api/server-server/definitions/keys_query_response.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ type: object
description: Server keys
example: {
"server_keys": [{
$ref: "../examples/server_key.json"
$ref: "../examples/server_key_notary_signed.json"
}]
}
properties:
server_keys:
type: array
title: Server Keys
description: The server keys.
description: The queried server's keys, signed by the notary server.
items:
$ref: "keys.yaml"
12 changes: 6 additions & 6 deletions api/server-server/examples/server_key.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"server_name": "example.org",
"verify_keys": {
"ed25519:auto2": {
"key": "Base+64+Encoded+Signature+Verification+Key"
"ed25519:abc123": {
"key": "VGhpcyBzaG91bGQgYmUgYSByZWFsIGVkMjU1MTkgcGF5bG9hZA=="
}
},
"old_verify_keys": {
"ed25519:auto1": {
"ed25519:0ldk3y": {
"expired_ts": 922834800000,
"key": "Base+64+Encoded+Old+Verify+Key"
"key": "VGhpcyBzaG91bGQgYmUgeW91ciBvbGQga2V5J3MgZWQyNTUxOSBwYXlsb2FkLg=="
}
},
"signatures": {
"example.org": {
"ed25519:auto2": "Base+64+Encoded+Signature"
"ed25519:auto2": "VGhpcyBzaG91bGQgYWN0dWFsbHkgYmUgYSBzaWduYXR1cmU="
}
},
"tls_fingerprints": [{
"sha256": "Base+64+Encoded+SHA-256-Fingerprint"
"sha256": "VGhpcyBpcyBoYXNoIHdoaWNoIHNob3VsZCBiZSBieXRlcw=="
}],
"valid_until_ts": 1052262000000
}
11 changes: 11 additions & 0 deletions api/server-server/examples/server_key_notary_signed.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"$ref": "server_key.json",
"signatures": {
"example.org": {
"ed25519:abc123": "VGhpcyBzaG91bGQgYWN0dWFsbHkgYmUgYSBzaWduYXR1cmU="
},
"notary.server.com": {
"ed25519:010203": "VGhpcyBpcyBhbm90aGVyIHNpZ25hdHVyZQ=="
}
}
}
69 changes: 49 additions & 20 deletions api/server-server/keys_query.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,49 +25,60 @@ produces:
paths:
"/query/{serverName}/{keyId}":
get:
summary: Retrieve a server key.
description: Retrieve a server key.
operationId: perspectivesKeyQuery
summary: Query for another server's keys
description: |-
Query for another server's keys. The receiving (notary) server must
sign the keys returned by the queried server.
operationId: getQueryKeys
parameters:
- in: path
name: serverName
type: string
description: Server name.
description: The server's DNS name to query
required: true
x-example: matrix.org
- in: path
name: keyId
type: string
description: Key ID.
required: true
x-example: TODO # No examples in spec so far
description: |-
The key ID to look up. If omitted or empty, all the server's keys
are to be queried for.
required: false
x-example: "ed25519:abc123"
- in: query
name: minimum_valid_until_ts
type: integer
format: int64
description: Minimum Valid Until Milliseconds.
required: true # TODO: Verify
description: |-
A millisecond POSIX timestamp indicating when the returned certificates
will need to be valid until to be useful to the requesting server.

If not supplied, the current time as determined by the notary server is used.
required: false
x-example: 1234567890
responses:
200:
description: The keys for the server
description: |-
The keys for the server, or an empty array if the server could not be reached
and no cached keys were available.
schema:
$ref: "definitions/keys_query_response.yaml"
"/query":
post:
summary: Retrieve a server key
description: Retrieve a server key.
operationId: bulkPerspectivesKeyQuery
summary: Query for several server's keys
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
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a better operationId

parameters:
- in: body
name: body
schema:
type: object
# TODO: Improve example
example: {
"server_keys": {
"{server_name}": {
"{key_id}": {
"example.org": {
"ed25519:abc123": {
"minimum_valid_until_ts": 1234567890
}
}
Expand All @@ -76,24 +87,42 @@ paths:
properties:
server_keys:
type: object
description: The query criteria.
description: |-
The query criteria. The outer ``string`` key on the object is the
server name (eg: ``matrix.org``). The inner ``string`` key is the
Key ID to query for the particular server. If no key IDs are given
to be queried, the notary server should query for all keys. If no
servers are given, the notary server must return an empty ``server_keys``
array in the response.

The notary server may return multiple keys regardless of the Key IDs
given.
additionalProperties:
type: object
name: ServerName
description: The server names to query.
additionalProperties:
type: object
title: Query Criteria
description: The server keys to query.
description: The server key IDs to query.
properties:
minimum_valid_until_ts:
type: integer
format: int64
description: Minimum Valid Until MS.
description: |-
A millisecond POSIX timestamp indicating when the returned
certificates will need to be valid until to be useful to the
requesting server.

If not supplied, the current time as determined by the notary
server is used.
example: 1234567890
required: ['server_keys']
responses:
200:
description: The keys for the server.
description: |-
The keys for the queried servers, signed by the notary server. Servers which
are offline and have no cached keys will not be included in the result. This
may result in an empty array.
schema:
$ref: "definitions/keys_query_response.yaml"
30 changes: 25 additions & 5 deletions api/server-server/keys_server.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,38 @@ produces:
paths:
"/server/{keyId}":
get:
summary: Get the server's key
description: Get the server's key.
summary: Get the homeserver's public key(s)
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

listing all of its keys, regardless of the ``keyId`` path argument.
This is to reduce the number of round trips needed to discover the
relevant keys for a homeserver.

Intermediate notary servers should cache a response for half of its
lifetime to avoid serving a stale response. Originating servers should
avoid returning responses that expire in less than an hour to avoid
repeated reqests for a certificate that is about to expire. Requesting
servers should limit how frequently they query for certificates to
avoid flooding a server with requests.

If the server fails to respond to this request, intermediate notary
servers should continue to return the last response they received
from the server so that the signatures of old events can still be
checked.
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
Copy link
Member

Choose a reason for hiding this comment

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

this conflicts with the text in the description,

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 suppose. Will update alongside deprecation.

to be returned.
required: false
x-example: TODO # No examples in the spec so far
x-example: "ed25519:abc123"
responses:
200:
description: The server's keys.
description: The homeserver's keys
schema:
$ref: "definitions/keys.yaml"