-
Notifications
You must be signed in to change notification settings - Fork 413
Use signature support from policy servers when available #18934
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
Changes from 11 commits
e7ada24
d68a08f
23550f2
be57543
4054a8e
fb02a5a
5081151
6f16300
c844abb
ef6f93e
83c3af3
4fdf2f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Update [MSC4284: Policy Servers](https://github.com/matrix-org/matrix-spec-proposals/pull/4284) implementation to support signatures when available. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,11 @@ | |
| import logging | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| from signedjson.key import decode_verify_key_bytes | ||
| from unpaddedbase64 import decode_base64 | ||
|
|
||
| from synapse.api.errors import SynapseError | ||
| from synapse.crypto.keyring import VerifyJsonRequest | ||
| from synapse.events import EventBase | ||
| from synapse.types.handlers.policy_server import RECOMMENDATION_OK | ||
| from synapse.util.stringutils import parse_and_validate_server_name | ||
|
|
@@ -26,6 +31,9 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| POLICY_SERVER_EVENT_TYPE = "org.matrix.msc4284.policy" | ||
| POLICY_SERVER_KEY_ID = "ed25519:policy_server" | ||
|
|
||
|
|
||
| class RoomPolicyHandler: | ||
| def __init__(self, hs: "HomeServer"): | ||
|
|
@@ -54,11 +62,11 @@ async def is_event_allowed(self, event: EventBase) -> bool: | |
| Returns: | ||
| bool: True if the event is allowed in the room, False otherwise. | ||
| """ | ||
| if event.type == "org.matrix.msc4284.policy" and event.state_key is not None: | ||
| if event.type == POLICY_SERVER_EVENT_TYPE and event.state_key is not None: | ||
| return True # always allow policy server change events | ||
|
|
||
| policy_event = await self._storage_controllers.state.get_current_state_event( | ||
| event.room_id, "org.matrix.msc4284.policy", "" | ||
| event.room_id, POLICY_SERVER_EVENT_TYPE, "" | ||
| ) | ||
| if not policy_event: | ||
| return True # no policy server == default allow | ||
|
|
@@ -81,6 +89,22 @@ async def is_event_allowed(self, event: EventBase) -> bool: | |
| if not is_in_room: | ||
| return True # policy server not in room == default allow | ||
|
|
||
| # Check if the event has been signed with the public key in the policy server state event. | ||
| # If it is, we can save an HTTP hit. | ||
| # We actually want to get the policy server state event BEFORE THE EVENT rather than | ||
| # the current state value, else changing the public key will cause all of these checks to fail. | ||
| # However, if we are checking outlier events (which we will due to is_event_allowed being called | ||
| # near the edges at _check_sigs_and_hash) we won't know the state before the event, so the | ||
| # only safe option is to use the current state | ||
| public_key = policy_event.content.get("public_key", None) | ||
| if public_key is not None and isinstance(public_key, str): | ||
| valid = await self._verify_policy_server_signature( | ||
| event, policy_server, public_key | ||
| ) | ||
| if valid: | ||
| return True | ||
| # fallthrough to hit /check manually | ||
|
|
||
| # At this point, the server appears valid and is in the room, so ask it to check | ||
| # the event. | ||
| recommendation = await self._federation_client.get_pdu_policy_recommendation( | ||
|
|
@@ -90,3 +114,73 @@ async def is_event_allowed(self, event: EventBase) -> bool: | |
| return False | ||
|
|
||
| return True # default allow | ||
|
|
||
| async def _verify_policy_server_signature( | ||
| self, event: EventBase, policy_server: str, public_key: str | ||
| ) -> bool: | ||
| # check the event is signed with this (via, public_key). | ||
| verify_json_req = VerifyJsonRequest.from_event(policy_server, event, 0) | ||
| try: | ||
| key_bytes = decode_base64(public_key) | ||
| verify_key = decode_verify_key_bytes(POLICY_SERVER_KEY_ID, key_bytes) | ||
| # We would normally use KeyRing.verify_event_for_server but we can't here as we don't | ||
| # want to fetch the server key, and instead want to use the public key in the state event. | ||
| await self._hs.get_keyring().process_json(verify_key, verify_json_req) | ||
| # if the event is correctly signed by the public key in the policy server state event = Allow | ||
| return True | ||
| except Exception as ex: | ||
| logger.warning( | ||
| "failed to verify event using public key in policy server event: %s", ex | ||
| ) | ||
| return False | ||
|
|
||
| async def ask_policy_server_to_sign_event( | ||
| self, event: EventBase, verify: bool = False | ||
| ) -> None: | ||
| """Ask the policy server to sign this event. The signature is added to the event signatures block. | ||
|
|
||
| Does nothing if there is no policy server state event in the room. If the policy server | ||
| refuses to sign the event (as it's marked as spam) does nothing. | ||
|
|
||
| Args: | ||
| event: The event to sign | ||
| verify: If True, verify that the signature is correctly signed by the public_key in the | ||
| policy server state event. | ||
| Raises: | ||
| if verify=True and the policy server signed the event with an invalid signature. Does | ||
| not raise if the policy server refuses to sign the event. | ||
| """ | ||
| policy_event = await self._storage_controllers.state.get_current_state_event( | ||
| event.room_id, POLICY_SERVER_EVENT_TYPE, "" | ||
| ) | ||
| if not policy_event: | ||
| return | ||
| policy_server = policy_event.content.get("via", None) | ||
| if policy_server is None or not isinstance(policy_server, str): | ||
| return | ||
| # Only ask to sign events if the policy state event has a public_key (so they can be subsequently verified) | ||
| public_key = policy_event.content.get("public_key", None) | ||
| if public_key is None or not isinstance(public_key, str): | ||
| return | ||
|
|
||
| # Ask the policy server to sign this event. | ||
| # We set a smallish timeout here as we don't want to block event sending too long. | ||
| signature = await self._federation_client.ask_policy_server_to_sign_event( | ||
| policy_server, | ||
| event, | ||
| timeout=3000, | ||
| ) | ||
| if ( | ||
| # the policy server returns {} if it refuses to sign the event. | ||
| signature and len(signature) > 0 | ||
| ): | ||
| event.signatures.update(signature) | ||
| if verify: | ||
| is_valid = await self._verify_policy_server_signature( | ||
| event, policy_server, public_key | ||
| ) | ||
| if not is_valid: | ||
| raise SynapseError( | ||
| 500, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is an upstream server problem, seems like it should be a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a network issue though - the server returned a result, but it's wrong.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it still makes sense to return a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps something to discuss on the MSC? I think the only necessary action may be just to use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this to the MSC is probably best - I suspect we actually want to consider it a rejection rather than an error, but doing that at this level in the stack needs more consideration. I'm not seeing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
389 results in |
||
| f"policy server {policy_server} failed to sign event correctly", | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the sound-ness of whether this works as intended, etc.
Everything looks optional and this saves us extra requests/checks 👍