From fe1daad67237c2154a3d8d8cdf6c603f0d33682e Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 14 Jun 2022 15:12:08 +0200 Subject: Move the "email unsubscribe" resource, refactor the macaroon generator & simplify the access token verification logic. (#12986) This simplifies the access token verification logic by removing the `rights` parameter which was only ever used for the unsubscribe link in email notifications. The latter has been moved under the `/_synapse` namespace, since it is not a standard API. This also makes the email verification link more secure, by embedding the app_id and pushkey in the macaroon and verifying it. This prevents the user from tampering the query parameters of that unsubscribe link. Macaroon generation is refactored: - Centralised all macaroon generation and verification logic to the `MacaroonGenerator` - Moved to `synapse.utils` - Changed the constructor to require only a `Clock`, hostname, and a secret key (instead of a full `Homeserver`). - Added tests for all methods. --- tests/api/test_auth.py | 15 ++--- tests/handlers/test_oidc.py | 7 +-- tests/test_state.py | 11 +++- tests/unittest.py | 2 +- tests/util/test_macaroons.py | 146 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 164 insertions(+), 17 deletions(-) create mode 100644 tests/util/test_macaroons.py (limited to 'tests') diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 54af9089e9..dfcfaf79b6 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -313,9 +313,7 @@ class AuthTestCase(unittest.HomeserverTestCase): self.assertEqual(self.store.insert_client_ip.call_count, 2) def test_get_user_from_macaroon(self): - self.store.get_user_by_access_token = simple_async_mock( - TokenLookupResult(user_id="@baldrick:matrix.org", device_id="device") - ) + self.store.get_user_by_access_token = simple_async_mock(None) user_id = "@baldrick:matrix.org" macaroon = pymacaroons.Macaroon( @@ -323,17 +321,14 @@ class AuthTestCase(unittest.HomeserverTestCase): identifier="key", key=self.hs.config.key.macaroon_secret_key, ) + # "Legacy" macaroons should not work for regular users not in the database macaroon.add_first_party_caveat("gen = 1") macaroon.add_first_party_caveat("type = access") macaroon.add_first_party_caveat("user_id = %s" % (user_id,)) - user_info = self.get_success( - self.auth.get_user_by_access_token(macaroon.serialize()) + serialized = macaroon.serialize() + self.get_failure( + self.auth.get_user_by_access_token(serialized), InvalidClientTokenError ) - self.assertEqual(user_id, user_info.user_id) - - # TODO: device_id should come from the macaroon, but currently comes - # from the db. - self.assertEqual(user_info.device_id, "device") def test_get_guest_user_from_macaroon(self): self.store.get_user_by_id = simple_async_mock({"is_guest": True}) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 1231aed944..e6cd3af7b7 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -25,7 +25,7 @@ from synapse.handlers.sso import MappingException from synapse.server import HomeServer from synapse.types import JsonDict, UserID from synapse.util import Clock -from synapse.util.macaroons import get_value_from_macaroon +from synapse.util.macaroons import OidcSessionData, get_value_from_macaroon from tests.test_utils import FakeResponse, get_awaitable_result, simple_async_mock from tests.unittest import HomeserverTestCase, override_config @@ -1227,7 +1227,7 @@ class OidcHandlerTestCase(HomeserverTestCase): ) -> str: from synapse.handlers.oidc import OidcSessionData - return self.handler._token_generator.generate_oidc_session_token( + return self.handler._macaroon_generator.generate_oidc_session_token( state=state, session_data=OidcSessionData( idp_id="oidc", @@ -1251,7 +1251,6 @@ async def _make_callback_with_userinfo( userinfo: the OIDC userinfo dict client_redirect_url: the URL to redirect to on success. """ - from synapse.handlers.oidc import OidcSessionData handler = hs.get_oidc_handler() provider = handler._providers["oidc"] @@ -1260,7 +1259,7 @@ async def _make_callback_with_userinfo( provider._fetch_userinfo = simple_async_mock(return_value=userinfo) # type: ignore[assignment] state = "state" - session = handler._token_generator.generate_oidc_session_token( + session = handler._macaroon_generator.generate_oidc_session_token( state=state, session_data=OidcSessionData( idp_id="oidc", diff --git a/tests/test_state.py b/tests/test_state.py index 95f81bebae..b005dd8d0f 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Collection, Dict, List, Optional +from typing import Collection, Dict, List, Optional, cast from unittest.mock import Mock from twisted.internet import defer @@ -22,6 +22,8 @@ from synapse.api.room_versions import RoomVersions from synapse.events import make_event_from_dict from synapse.events.snapshot import EventContext from synapse.state import StateHandler, StateResolutionHandler +from synapse.util import Clock +from synapse.util.macaroons import MacaroonGenerator from tests import unittest @@ -190,13 +192,18 @@ class StateTestCase(unittest.TestCase): "get_clock", "get_state_resolution_handler", "get_account_validity_handler", + "get_macaroon_generator", "hostname", ] ) + clock = cast(Clock, MockClock()) hs.config = default_config("tesths", True) hs.get_datastores.return_value = Mock(main=self.dummy_store) hs.get_state_handler.return_value = None - hs.get_clock.return_value = MockClock() + hs.get_clock.return_value = clock + hs.get_macaroon_generator.return_value = MacaroonGenerator( + clock, "tesths", b"verysecret" + ) hs.get_auth.return_value = Auth(hs) hs.get_state_resolution_handler = lambda: StateResolutionHandler(hs) hs.get_storage_controllers.return_value = storage_controllers diff --git a/tests/unittest.py b/tests/unittest.py index e7f255b4fa..c645dd3563 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -315,7 +315,7 @@ class HomeserverTestCase(TestCase): "is_guest": False, } - async def get_user_by_req(request, allow_guest=False, rights="access"): + async def get_user_by_req(request, allow_guest=False): assert self.helper.auth_user_id is not None return create_requester( UserID.from_string(self.helper.auth_user_id), diff --git a/tests/util/test_macaroons.py b/tests/util/test_macaroons.py new file mode 100644 index 0000000000..32125f7bb7 --- /dev/null +++ b/tests/util/test_macaroons.py @@ -0,0 +1,146 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from pymacaroons.exceptions import MacaroonVerificationFailedException + +from synapse.util.macaroons import MacaroonGenerator, OidcSessionData + +from tests.server import get_clock +from tests.unittest import TestCase + + +class MacaroonGeneratorTestCase(TestCase): + def setUp(self): + self.reactor, hs_clock = get_clock() + self.macaroon_generator = MacaroonGenerator(hs_clock, "tesths", b"verysecret") + self.other_macaroon_generator = MacaroonGenerator( + hs_clock, "tesths", b"anothersecretkey" + ) + + def test_guest_access_token(self): + """Test the generation and verification of guest access tokens""" + token = self.macaroon_generator.generate_guest_access_token("@user:tesths") + user_id = self.macaroon_generator.verify_guest_token(token) + self.assertEqual(user_id, "@user:tesths") + + # Raises with another secret key + with self.assertRaises(MacaroonVerificationFailedException): + self.other_macaroon_generator.verify_guest_token(token) + + # Check that an old access token without the guest caveat does not work + macaroon = self.macaroon_generator._generate_base_macaroon("access") + macaroon.add_first_party_caveat(f"user_id = {user_id}") + macaroon.add_first_party_caveat("nonce = 0123456789abcdef") + token = macaroon.serialize() + + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_guest_token(token) + + def test_delete_pusher_token(self): + """Test the generation and verification of delete_pusher tokens""" + token = self.macaroon_generator.generate_delete_pusher_token( + "@user:tesths", "m.mail", "john@example.com" + ) + user_id = self.macaroon_generator.verify_delete_pusher_token( + token, "m.mail", "john@example.com" + ) + self.assertEqual(user_id, "@user:tesths") + + # Raises with another secret key + with self.assertRaises(MacaroonVerificationFailedException): + self.other_macaroon_generator.verify_delete_pusher_token( + token, "m.mail", "john@example.com" + ) + + # Raises when verifying for another pushkey + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_delete_pusher_token( + token, "m.mail", "other@example.com" + ) + + # Raises when verifying for another app_id + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_delete_pusher_token( + token, "somethingelse", "john@example.com" + ) + + # Check that an old token without the app_id and pushkey still works + macaroon = self.macaroon_generator._generate_base_macaroon("delete_pusher") + macaroon.add_first_party_caveat("user_id = @user:tesths") + token = macaroon.serialize() + user_id = self.macaroon_generator.verify_delete_pusher_token( + token, "m.mail", "john@example.com" + ) + self.assertEqual(user_id, "@user:tesths") + + def test_short_term_login_token(self): + """Test the generation and verification of short-term login tokens""" + token = self.macaroon_generator.generate_short_term_login_token( + user_id="@user:tesths", + auth_provider_id="oidc", + auth_provider_session_id="sid", + duration_in_ms=2 * 60 * 1000, + ) + + info = self.macaroon_generator.verify_short_term_login_token(token) + self.assertEqual(info.user_id, "@user:tesths") + self.assertEqual(info.auth_provider_id, "oidc") + self.assertEqual(info.auth_provider_session_id, "sid") + + # Raises with another secret key + with self.assertRaises(MacaroonVerificationFailedException): + self.other_macaroon_generator.verify_short_term_login_token(token) + + # Wait a minute + self.reactor.pump([60]) + # Shouldn't raise + self.macaroon_generator.verify_short_term_login_token(token) + # Wait another minute + self.reactor.pump([60]) + # Should raise since it expired + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_short_term_login_token(token) + + def test_oidc_session_token(self): + """Test the generation and verification of OIDC session cookies""" + state = "arandomstate" + session_data = OidcSessionData( + idp_id="oidc", + nonce="nonce", + client_redirect_url="https://example.com/", + ui_auth_session_id="", + ) + token = self.macaroon_generator.generate_oidc_session_token( + state, session_data, duration_in_ms=2 * 60 * 1000 + ).encode("utf-8") + info = self.macaroon_generator.verify_oidc_session_token(token, state) + self.assertEqual(session_data, info) + + # Raises with another secret key + with self.assertRaises(MacaroonVerificationFailedException): + self.other_macaroon_generator.verify_oidc_session_token(token, state) + + # Should raise with another state + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_oidc_session_token(token, "anotherstate") + + # Wait a minute + self.reactor.pump([60]) + # Shouldn't raise + self.macaroon_generator.verify_oidc_session_token(token, state) + # Wait another minute + self.reactor.pump([60]) + # Should raise since it expired + with self.assertRaises(MacaroonVerificationFailedException): + self.macaroon_generator.verify_oidc_session_token(token, state) -- cgit 1.4.1