Skip to content

Commit

Permalink
Enable cross-signing key upload without UIA (#17284)
Browse files Browse the repository at this point in the history
Per MSC3967, which is now stable, we should not require UIA when
uploading cross-signing keys for the first time.

Fixes: #17227
  • Loading branch information
richvdh authored Jun 14, 2024
1 parent 2c36a67 commit 3aae60f
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 123 deletions.
1 change: 1 addition & 0 deletions changelog.d/17284.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Do not require user-interactive authentication for uploading cross-signing keys for the first time, per MSC3967.
3 changes: 0 additions & 3 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# 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", {}))
Expand Down
1 change: 0 additions & 1 deletion synapse/rest/admin/experimental_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ class ExperimentalFeature(str, Enum):

MSC3026 = "msc3026"
MSC3881 = "msc3881"
MSC3967 = "msc3967"


class ExperimentalFeaturesRestServlet(RestServlet):
Expand Down
79 changes: 29 additions & 50 deletions synapse/rest/client/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,44 +382,35 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
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

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

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:
# 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, {}

# 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

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,
Expand All @@ -428,18 +419,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
# 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
Expand Down
2 changes: 2 additions & 0 deletions tests/handlers/test_oauth_delegation.py
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,8 @@ def test_cross_signing(self) -> None:

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",
Expand Down
4 changes: 0 additions & 4 deletions tests/rest/admin/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,6 @@ def test_enable_and_disable(self) -> None:
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}"
Expand Down
65 changes: 0 additions & 65 deletions tests/rest/client/test_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,71 +155,6 @@ def make_device_keys(self, user_id: str, device_id: str) -> JsonDict:
}

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)
Expand Down

0 comments on commit 3aae60f

Please sign in to comment.