From 3aae60f17b97078b2fd4bde64be063f9d34c6352 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 14 Jun 2024 11:14:56 +0100 Subject: [PATCH] Enable cross-signing key upload without UIA (#17284) Per MSC3967, which is now stable, we should not require UIA when uploading cross-signing keys for the first time. Fixes: #17227 --- changelog.d/17284.feature | 1 + synapse/config/experimental.py | 3 - synapse/rest/admin/experimental_features.py | 1 - synapse/rest/client/keys.py | 75 ++++++++------------- tests/handlers/test_oauth_delegation.py | 2 + tests/rest/admin/test_admin.py | 4 -- tests/rest/client/test_keys.py | 65 ------------------ 7 files changed, 30 insertions(+), 121 deletions(-) create mode 100644 changelog.d/17284.feature diff --git a/changelog.d/17284.feature b/changelog.d/17284.feature new file mode 100644 index 0000000000..015d925e7c --- /dev/null +++ b/changelog.d/17284.feature @@ -0,0 +1 @@ +Do not require user-interactive authentication for uploading cross-signing keys for the first time, per MSC3967. \ No newline at end of file diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index d9ad5fc32d..24546171e5 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -393,9 +393,6 @@ class ExperimentalConfig(Config): # MSC3391: Removing account data. self.msc3391_enabled = experimental.get("msc3391_enabled", False) - # MSC3967: Do not require UIA when first uploading cross signing keys - self.msc3967_enabled = experimental.get("msc3967_enabled", False) - # MSC3861: Matrix architecture change to delegate authentication via OIDC try: self.msc3861 = MSC3861(**experimental.get("msc3861", {})) diff --git a/synapse/rest/admin/experimental_features.py b/synapse/rest/admin/experimental_features.py index 52eb9e62db..c5a00c490c 100644 --- a/synapse/rest/admin/experimental_features.py +++ b/synapse/rest/admin/experimental_features.py @@ -41,7 +41,6 @@ class ExperimentalFeature(str, Enum): MSC3026 = "msc3026" MSC3881 = "msc3881" - MSC3967 = "msc3967" class ExperimentalFeaturesRestServlet(RestServlet): diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 306db07b86..67de634eab 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -382,44 +382,35 @@ class SigningKeyUploadServlet(RestServlet): master_key_updatable_without_uia, ) = await self.e2e_keys_handler.check_cross_signing_setup(user_id) - # Before MSC3967 we required UIA both when setting up cross signing for the - # first time and when resetting the device signing key. With MSC3967 we only - # require UIA when resetting cross-signing, and not when setting up the first - # time. Because there is no UIA in MSC3861, for now we throw an error if the - # user tries to reset the device signing key when MSC3861 is enabled, but allow - # first-time setup. - if self.hs.config.experimental.msc3861.enabled: - # The auth service has to explicitly mark the master key as replaceable - # without UIA to reset the device signing key with MSC3861. - if is_cross_signing_setup and not master_key_updatable_without_uia: - config = self.hs.config.experimental.msc3861 - if config.account_management_url is not None: - url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset" - else: - url = config.issuer + # 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: + return 200, {} - raise SynapseError( - HTTPStatus.NOT_IMPLEMENTED, - "To reset your end-to-end encryption cross-signing identity, " - f"you first need to approve it at {url} and then try again.", - Codes.UNRECOGNIZED, - ) - # But first-time setup is fine + # The keys are different; is x-signing set up? If no, then this is first-time + # setup, and that is allowed without UIA, per MSC3967. + # If yes, then we need to authenticate the change. + if is_cross_signing_setup: + # With MSC3861, UIA is not possible. Instead, the auth service has to + # explicitly mark the master key as replaceable. + if self.hs.config.experimental.msc3861.enabled: + if not master_key_updatable_without_uia: + config = self.hs.config.experimental.msc3861 + if config.account_management_url is not None: + url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset" + else: + url = config.issuer - elif self.hs.config.experimental.msc3967_enabled: - # 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: + raise SynapseError( + HTTPStatus.NOT_IMPLEMENTED, + "To reset your end-to-end encryption cross-signing identity, " + f"you first need to approve it at {url} and then try again.", + Codes.UNRECOGNIZED, + ) + else: + # Without MSC3861, we require UIA. await self.auth_handler.validate_user_via_ui_auth( requester, request, @@ -428,18 +419,6 @@ class SigningKeyUploadServlet(RestServlet): # Do not allow skipping of UIA auth. can_skip_ui_auth=False, ) - # Otherwise we don't require UIA since we are setting up cross signing for first time - else: - # Previous behaviour is to always require UIA but allow it to be skipped - await self.auth_handler.validate_user_via_ui_auth( - requester, - request, - body, - "add a device signing key to your account", - # Allow skipping of UI auth since this is frequently called directly - # after login and it is silly to ask users to re-auth immediately. - can_skip_ui_auth=True, - ) result = await self.e2e_keys_handler.upload_signing_keys_for_user(user_id, body) return 200, result diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py index 9387d07de8..036c539db2 100644 --- a/tests/handlers/test_oauth_delegation.py +++ b/tests/handlers/test_oauth_delegation.py @@ -541,6 +541,8 @@ class MSC3861OAuthDelegation(HomeserverTestCase): self.assertEqual(channel.code, 200, channel.json_body) + # Try uploading *different* keys; it should cause a 501 error. + keys_upload_body = self.make_device_keys(USER_ID, DEVICE) channel = self.make_request( "POST", "/_matrix/client/v3/keys/device_signing/upload", diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 22106eb786..5f6f7213b3 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -435,10 +435,6 @@ class ExperimentalFeaturesTestCase(unittest.HomeserverTestCase): True, channel.json_body["features"]["msc3881"], ) - self.assertEqual( - False, - channel.json_body["features"]["msc3967"], - ) # test nothing blows up if you try to disable a feature that isn't already enabled url = f"{self.url}/{self.other_user}" diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 5f0c005576..8bbd109092 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -155,71 +155,6 @@ class KeyQueryTestCase(unittest.HomeserverTestCase): } def test_device_signing_with_uia(self) -> None: - """Device signing key upload requires UIA.""" - password = "wonderland" - device_id = "ABCDEFGHI" - alice_id = self.register_user("alice", password) - alice_token = self.login("alice", password, device_id=device_id) - - content = self.make_device_keys(alice_id, device_id) - - channel = self.make_request( - "POST", - "/_matrix/client/v3/keys/device_signing/upload", - content, - alice_token, - ) - - self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, channel.result) - # Grab the session - session = channel.json_body["session"] - # Ensure that flows are what is expected. - self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) - - # add UI auth - content["auth"] = { - "type": "m.login.password", - "identifier": {"type": "m.id.user", "user": alice_id}, - "password": password, - "session": session, - } - - channel = self.make_request( - "POST", - "/_matrix/client/v3/keys/device_signing/upload", - content, - alice_token, - ) - - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - - @override_config({"ui_auth": {"session_timeout": "15m"}}) - def test_device_signing_with_uia_session_timeout(self) -> None: - """Device signing key upload requires UIA buy passes with grace period.""" - password = "wonderland" - device_id = "ABCDEFGHI" - alice_id = self.register_user("alice", password) - alice_token = self.login("alice", password, device_id=device_id) - - content = self.make_device_keys(alice_id, device_id) - - channel = self.make_request( - "POST", - "/_matrix/client/v3/keys/device_signing/upload", - content, - alice_token, - ) - - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - - @override_config( - { - "experimental_features": {"msc3967_enabled": True}, - "ui_auth": {"session_timeout": "15s"}, - } - ) - def test_device_signing_with_msc3967(self) -> None: - """Device signing key follows MSC3967 behaviour when enabled.""" password = "wonderland" device_id = "ABCDEFGHI" alice_id = self.register_user("alice", password)