bugfix: make msc3967 idempotent (#16943)

MSC3967 was updated recently to make it more robust to network failures:

> there is an existing cross-signing master key and it exactly matches
the cross-signing master key provided in the request body. If there are
any additional keys provided in the request (self signing key, user
signing key) they MUST also match the existing keys stored on the
server. In other words, the request contains no new keys. If there are
new keys, UIA MUST be performed.


https://github.com/matrix-org/matrix-spec-proposals/blob/hughns/device-signing-upload-uia/proposals/3967-device-signing-upload-uia.md#proposal

This covers the case where the 200 OK is lost in transit so the client
retries the upload, only to then get UIA'd.

Complement tests: https://github.com/matrix-org/complement/pull/713 -
passing example
https://github.com/element-hq/synapse/actions/runs/7976948122/job/21778795094?pr=16943#step:7:8820

### Pull Request Checklist

<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->

* [x] Pull request is based on the develop branch
* [x] Pull request includes a [changelog
file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog).
The entry should:
- Be a short description of your change which makes sense to users.
"Fixed a bug that prevented receiving messages from other servers."
instead of "Moved X method from `EventStore` to `EventWorkerStore`.".
  - Use markdown where necessary, mostly for `code blocks`.
  - End with either a period (.) or an exclamation mark (!).
  - Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by
@github_username." or "Contributed by [Your Name]." to the end of the
entry.
* [x] [Code
style](https://element-hq.github.io/synapse/latest/code_style.html) is
correct
(run the
[linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

---------

Co-authored-by: reivilibre <oliverw@matrix.org>
This commit is contained in:
Kegan Dougal 2024-04-15 11:57:56 +01:00 committed by GitHub
parent fe4719a268
commit 259442fa4c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 102 additions and 3 deletions

1
changelog.d/16943.bugfix Normal file
View file

@ -0,0 +1 @@
Make the CSAPI endpoint `/keys/device_signing/upload` idempotent.

View file

@ -102,6 +102,8 @@ experimental_features:
msc3391_enabled: true msc3391_enabled: true
# Filtering /messages by relation type. # Filtering /messages by relation type.
msc3874_enabled: true msc3874_enabled: true
# no UIA for x-signing upload for the first time
msc3967_enabled: true
server_notices: server_notices:
system_mxid_localpart: _server system_mxid_localpart: _server

View file

@ -214,7 +214,7 @@ fi
extra_test_args=() extra_test_args=()
test_packages="./tests/csapi ./tests ./tests/msc3874 ./tests/msc3890 ./tests/msc3391 ./tests/msc3930 ./tests/msc3902" test_packages="./tests/csapi ./tests ./tests/msc3874 ./tests/msc3890 ./tests/msc3391 ./tests/msc3930 ./tests/msc3902 ./tests/msc3967"
# Enable dirty runs, so tests will reuse the same container where possible. # Enable dirty runs, so tests will reuse the same container where possible.
# This significantly speeds up tests, but increases the possibility of test pollution. # This significantly speeds up tests, but increases the possibility of test pollution.

View file

@ -1476,6 +1476,42 @@ class E2eKeysHandler:
else: else:
return exists, self.clock.time_msec() < ts_replacable_without_uia_before return exists, self.clock.time_msec() < ts_replacable_without_uia_before
async def has_different_keys(self, user_id: str, body: JsonDict) -> bool:
"""
Check if a key provided in `body` differs from the same key stored in the DB. Returns
true on the first difference. If a key exists in `body` but does not exist in the DB,
returns True. If `body` has no keys, this always returns False.
Note by 'key' we mean Matrix key rather than JSON key.
The purpose of this function is to detect whether or not we need to apply UIA checks.
We must apply UIA checks if any key in the database is being overwritten. If a key is
being inserted for the first time, or if the key exactly matches what is in the database,
then no UIA check needs to be performed.
Args:
user_id: The user who sent the `body`.
body: The JSON request body from POST /keys/device_signing/upload
Returns:
True if any key in `body` has a different value in the database.
"""
# Ensure that each key provided in the request body exactly matches the one we have stored.
# The first time we see the DB having a different key to the matching request key, bail.
# Note: we do not care if the DB has a key which the request does not specify, as we only
# care about *replacements* or *insertions* (i.e UPSERT)
req_body_key_to_db_key = {
"master_key": "master",
"self_signing_key": "self_signing",
"user_signing_key": "user_signing",
}
for req_body_key, db_key in req_body_key_to_db_key.items():
if req_body_key in body:
existing_key = await self.store.get_e2e_cross_signing_key(
user_id, db_key
)
if existing_key != body[req_body_key]:
return True
return False
def _check_cross_signing_key( def _check_cross_signing_key(
key: JsonDict, user_id: str, key_type: str, signing_key: Optional[VerifyKey] = None key: JsonDict, user_id: str, key_type: str, signing_key: Optional[VerifyKey] = None

View file

@ -409,7 +409,18 @@ class SigningKeyUploadServlet(RestServlet):
# But first-time setup is fine # But first-time setup is fine
elif self.hs.config.experimental.msc3967_enabled: elif self.hs.config.experimental.msc3967_enabled:
# If we already have a master key then cross signing is set up and we require UIA to reset # MSC3967 allows this endpoint to 200 OK for idempotency. Resending exactly the same
# keys should just 200 OK without doing a UIA prompt.
keys_are_different = await self.e2e_keys_handler.has_different_keys(
user_id, body
)
if not keys_are_different:
# FIXME: we do not fallthrough to upload_signing_keys_for_user because confusingly
# if we do, we 500 as it looks like it tries to INSERT the same key twice, causing a
# unique key constraint violation. This sounds like a bug?
return 200, {}
# the keys are different, is x-signing set up? If no, then the keys don't exist which is
# why they are different. If yes, then we need to UIA to change them.
if is_cross_signing_setup: if is_cross_signing_setup:
await self.auth_handler.validate_user_via_ui_auth( await self.auth_handler.validate_user_via_ui_auth(
requester, requester,
@ -420,7 +431,6 @@ class SigningKeyUploadServlet(RestServlet):
can_skip_ui_auth=False, can_skip_ui_auth=False,
) )
# Otherwise we don't require UIA since we are setting up cross signing for first time # Otherwise we don't require UIA since we are setting up cross signing for first time
else: else:
# Previous behaviour is to always require UIA but allow it to be skipped # Previous behaviour is to always require UIA but allow it to be skipped
await self.auth_handler.validate_user_via_ui_auth( await self.auth_handler.validate_user_via_ui_auth(

View file

@ -1101,6 +1101,56 @@ class E2eKeysHandlerTestCase(unittest.HomeserverTestCase):
}, },
) )
def test_has_different_keys(self) -> None:
"""check that has_different_keys returns True when the keys provided are different to what
is in the database."""
local_user = "@boris:" + self.hs.hostname
keys1 = {
"master_key": {
# private key: 2lonYOM6xYKdEsO+6KrC766xBcHnYnim1x/4LFGF8B0
"user_id": local_user,
"usage": ["master"],
"keys": {
"ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk"
},
}
}
self.get_success(self.handler.upload_signing_keys_for_user(local_user, keys1))
is_different = self.get_success(
self.handler.has_different_keys(
local_user,
{
"master_key": keys1["master_key"],
},
)
)
self.assertEqual(is_different, False)
# change the usage => different keys
keys1["master_key"]["usage"] = ["develop"]
is_different = self.get_success(
self.handler.has_different_keys(
local_user,
{
"master_key": keys1["master_key"],
},
)
)
self.assertEqual(is_different, True)
keys1["master_key"]["usage"] = ["master"] # reset
# change the key => different keys
keys1["master_key"]["keys"] = {
"ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unIc0rncs": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unIc0rncs"
}
is_different = self.get_success(
self.handler.has_different_keys(
local_user,
{
"master_key": keys1["master_key"],
},
)
)
self.assertEqual(is_different, True)
def test_query_devices_remote_sync(self) -> None: def test_query_devices_remote_sync(self) -> None:
"""Tests that querying keys for a remote user that we share a room with, """Tests that querying keys for a remote user that we share a room with,
but haven't yet fetched the keys for, returns the cross signing keys but haven't yet fetched the keys for, returns the cross signing keys