diff options
| -rw-r--r-- | changelog.d/5588.misc | 1 | ||||
| -rwxr-xr-x | scripts-dev/dump_macaroon.py | 2 | ||||
| -rw-r--r-- | synapse/handlers/auth.py | 28 | ||||
| -rw-r--r-- | synapse/handlers/register.py | 4 | ||||
| -rw-r--r-- | synapse/util/stringutils.py | 20 | ||||
| -rw-r--r-- | tests/api/test_auth.py | 63 | ||||
| -rw-r--r-- | tests/handlers/test_auth.py | 43 | ||||
| -rw-r--r-- | tests/handlers/test_register.py | 12 | ||||
| -rw-r--r-- | tests/util/test_stringutils.py | 8 |
9 files changed, 78 insertions, 103 deletions
diff --git a/changelog.d/5588.misc b/changelog.d/5588.misc new file mode 100644 index 0000000000..b8f52a212c --- /dev/null +++ b/changelog.d/5588.misc @@ -0,0 +1 @@ +Reduce the length of Synapse's access tokens. diff --git a/scripts-dev/dump_macaroon.py b/scripts-dev/dump_macaroon.py index 980b5e709f..0ca75d3fe1 100755 --- a/scripts-dev/dump_macaroon.py +++ b/scripts-dev/dump_macaroon.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python2 +#!/usr/bin/env python import sys diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 36f2450e2e..8a6666a4ad 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -17,6 +17,7 @@ import logging import time import unicodedata import urllib.parse +from binascii import crc32 from typing import ( TYPE_CHECKING, Any, @@ -34,6 +35,7 @@ from typing import ( import attr import bcrypt import pymacaroons +import unpaddedbase64 from twisted.web.server import Request @@ -66,6 +68,7 @@ from synapse.util import stringutils as stringutils from synapse.util.async_helpers import maybe_awaitable from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry from synapse.util.msisdn import phone_number_to_msisdn +from synapse.util.stringutils import base62_encode from synapse.util.threepids import canonicalise_email if TYPE_CHECKING: @@ -808,10 +811,12 @@ class AuthHandler(BaseHandler): logger.info( "Logging in user %s as %s%s", user_id, puppets_user_id, fmt_expiry ) + target_user_id_obj = UserID.from_string(puppets_user_id) else: logger.info( "Logging in user %s on device %s%s", user_id, device_id, fmt_expiry ) + target_user_id_obj = UserID.from_string(user_id) if ( not is_appservice_ghost @@ -819,7 +824,7 @@ class AuthHandler(BaseHandler): ): await self.auth.check_auth_blocking(user_id) - access_token = self.macaroon_gen.generate_access_token(user_id) + access_token = self.generate_access_token(target_user_id_obj) await self.store.add_access_token_to_user( user_id=user_id, token=access_token, @@ -1192,6 +1197,19 @@ class AuthHandler(BaseHandler): return None return user_id + def generate_access_token(self, for_user: UserID) -> str: + """Generates an opaque string, for use as an access token""" + + # we use the following format for access tokens: + # syt_<base64 local part>_<random string>_<base62 crc check> + + b64local = unpaddedbase64.encode_base64(for_user.localpart.encode("utf-8")) + random_string = stringutils.random_string(20) + base = f"syt_{b64local}_{random_string}" + + crc = base62_encode(crc32(base.encode("ascii")), minwidth=6) + return f"{base}_{crc}" + async def validate_short_term_login_token( self, login_token: str ) -> LoginTokenAttributes: @@ -1585,10 +1603,7 @@ class MacaroonGenerator: hs = attr.ib() - def generate_access_token( - self, user_id: str, extra_caveats: Optional[List[str]] = None - ) -> str: - extra_caveats = extra_caveats or [] + def generate_guest_access_token(self, user_id: str) -> str: macaroon = self._generate_base_macaroon(user_id) macaroon.add_first_party_caveat("type = access") # Include a nonce, to make sure that each login gets a different @@ -1596,8 +1611,7 @@ class MacaroonGenerator: macaroon.add_first_party_caveat( "nonce = %s" % (stringutils.random_string_with_symbols(16),) ) - for caveat in extra_caveats: - macaroon.add_first_party_caveat(caveat) + macaroon.add_first_party_caveat("guest = true") return macaroon.serialize() def generate_short_term_login_token( diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 007fb12840..4ceef3fab3 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -722,9 +722,7 @@ class RegistrationHandler(BaseHandler): ) if is_guest: assert valid_until_ms is None - access_token = self.macaroon_gen.generate_access_token( - user_id, ["guest = true"] - ) + access_token = self.macaroon_gen.generate_guest_access_token(user_id) else: access_token = await self._auth_handler.get_access_token_for_user_id( user_id, diff --git a/synapse/util/stringutils.py b/synapse/util/stringutils.py index cd82777f80..4f25cd1d26 100644 --- a/synapse/util/stringutils.py +++ b/synapse/util/stringutils.py @@ -220,3 +220,23 @@ def strtobool(val: str) -> bool: return False else: raise ValueError("invalid truth value %r" % (val,)) + + +_BASE62 = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + + +def base62_encode(num: int, minwidth: int = 1) -> str: + """Encode a number using base62 + + Args: + num: number to be encoded + minwidth: width to pad to, if the number is small + """ + res = "" + while num: + num, rem = divmod(num, 62) + res = _BASE62[rem] + res + + # pad to minimum width + pad = "0" * (minwidth - len(res)) + return pad + res diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index c0ed64f784..1b0a815757 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -21,13 +21,11 @@ from synapse.api.constants import UserTypes from synapse.api.errors import ( AuthError, Codes, - InvalidClientCredentialsError, InvalidClientTokenError, MissingClientTokenError, ResourceLimitError, ) from synapse.storage.databases.main.registration import TokenLookupResult -from synapse.types import UserID from tests import unittest from tests.test_utils import simple_async_mock @@ -253,67 +251,6 @@ class AuthTestCase(unittest.HomeserverTestCase): self.assertTrue(user_info.is_guest) self.store.get_user_by_id.assert_called_with(user_id) - def test_cannot_use_regular_token_as_guest(self): - USER_ID = "@percy:matrix.org" - self.store.add_access_token_to_user = simple_async_mock(None) - self.store.get_device = simple_async_mock(None) - - token = self.get_success( - self.hs.get_auth_handler().get_access_token_for_user_id( - USER_ID, "DEVICE", valid_until_ms=None - ) - ) - self.store.add_access_token_to_user.assert_called_with( - user_id=USER_ID, - token=token, - device_id="DEVICE", - valid_until_ms=None, - puppets_user_id=None, - ) - - async def get_user(tok): - if token != tok: - return None - return TokenLookupResult( - user_id=USER_ID, - is_guest=False, - token_id=1234, - device_id="DEVICE", - ) - - self.store.get_user_by_access_token = get_user - self.store.get_user_by_id = simple_async_mock({"is_guest": False}) - - # check the token works - request = Mock(args={}) - request.args[b"access_token"] = [token.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - requester = self.get_success( - self.auth.get_user_by_req(request, allow_guest=True) - ) - self.assertEqual(UserID.from_string(USER_ID), requester.user) - self.assertFalse(requester.is_guest) - - # add an is_guest caveat - mac = pymacaroons.Macaroon.deserialize(token) - mac.add_first_party_caveat("guest = true") - guest_tok = mac.serialize() - - # the token should *not* work now - request = Mock(args={}) - request.args[b"access_token"] = [guest_tok.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - - cm = self.get_failure( - self.auth.get_user_by_req(request, allow_guest=True), - InvalidClientCredentialsError, - ) - - self.assertEqual(401, cm.value.code) - self.assertEqual("Guest access token used for regular user", cm.value.msg) - - self.store.get_user_by_id.assert_called_with(USER_ID) - def test_blocking_mau(self): self.auth_blocking._limit_usage_by_mau = False self.auth_blocking._max_mau_value = 50 diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py index fe7e9484fd..5f3350e490 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -16,12 +16,17 @@ from unittest.mock import Mock import pymacaroons from synapse.api.errors import AuthError, ResourceLimitError +from synapse.rest import admin from tests import unittest from tests.test_utils import make_awaitable class AuthTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets, + ] + def prepare(self, reactor, clock, hs): self.auth_handler = hs.get_auth_handler() self.macaroon_generator = hs.get_macaroon_generator() @@ -35,16 +40,10 @@ class AuthTestCase(unittest.HomeserverTestCase): self.small_number_of_users = 1 self.large_number_of_users = 100 - def test_token_is_a_macaroon(self): - token = self.macaroon_generator.generate_access_token("some_user") - # Check that we can parse the thing with pymacaroons - macaroon = pymacaroons.Macaroon.deserialize(token) - # The most basic of sanity checks - if "some_user" not in macaroon.inspect(): - self.fail("some_user was not in %s" % macaroon.inspect()) + self.user1 = self.register_user("a_user", "pass") def test_macaroon_caveats(self): - token = self.macaroon_generator.generate_access_token("a_user") + token = self.macaroon_generator.generate_guest_access_token("a_user") macaroon = pymacaroons.Macaroon.deserialize(token) def verify_gen(caveat): @@ -59,19 +58,23 @@ class AuthTestCase(unittest.HomeserverTestCase): def verify_nonce(caveat): return caveat.startswith("nonce =") + def verify_guest(caveat): + return caveat == "guest = true" + v = pymacaroons.Verifier() v.satisfy_general(verify_gen) v.satisfy_general(verify_user) v.satisfy_general(verify_type) v.satisfy_general(verify_nonce) + v.satisfy_general(verify_guest) v.verify(macaroon, self.hs.config.macaroon_secret_key) def test_short_term_login_token_gives_user_id(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", "", 5000 + self.user1, "", 5000 ) res = self.get_success(self.auth_handler.validate_short_term_login_token(token)) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) self.assertEqual("", res.auth_provider_id) # when we advance the clock, the token should be rejected @@ -83,22 +86,22 @@ class AuthTestCase(unittest.HomeserverTestCase): def test_short_term_login_token_gives_auth_provider(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", auth_provider_id="my_idp" + self.user1, auth_provider_id="my_idp" ) res = self.get_success(self.auth_handler.validate_short_term_login_token(token)) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) self.assertEqual("my_idp", res.auth_provider_id) def test_short_term_login_token_cannot_replace_user_id(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", "", 5000 + self.user1, "", 5000 ) macaroon = pymacaroons.Macaroon.deserialize(token) res = self.get_success( self.auth_handler.validate_short_term_login_token(macaroon.serialize()) ) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) # add another "user_id" caveat, which might allow us to override the # user_id. @@ -114,7 +117,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # Ensure does not throw exception self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) @@ -132,7 +135,7 @@ class AuthTestCase(unittest.HomeserverTestCase): self.get_failure( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ), ResourceLimitError, ) @@ -160,7 +163,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # If not in monthly active cohort self.get_failure( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ), ResourceLimitError, ) @@ -177,7 +180,7 @@ class AuthTestCase(unittest.HomeserverTestCase): ) self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) self.get_success( @@ -195,7 +198,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # Ensure does not raise exception self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) @@ -210,6 +213,6 @@ class AuthTestCase(unittest.HomeserverTestCase): def _get_macaroon(self): token = self.macaroon_generator.generate_short_term_login_token( - "user_a", "", 5000 + self.user1, "", 5000 ) return pymacaroons.Macaroon.deserialize(token) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 608f8f3d33..bd43190523 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -48,10 +48,6 @@ class RegistrationTestCase(unittest.HomeserverTestCase): self.mock_distributor = Mock() self.mock_distributor.declare("registered_user") self.mock_captcha_client = Mock() - self.macaroon_generator = Mock( - generate_access_token=Mock(return_value="secret") - ) - self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) self.handler = self.hs.get_registration_handler() self.store = self.hs.get_datastore() self.lots_of_users = 100 @@ -67,8 +63,8 @@ class RegistrationTestCase(unittest.HomeserverTestCase): self.get_or_create_user(requester, frank.localpart, "Frankie") ) self.assertEquals(result_user_id, user_id) - self.assertTrue(result_token is not None) - self.assertEquals(result_token, "secret") + self.assertIsInstance(result_token, str) + self.assertGreater(len(result_token), 20) def test_if_user_exists(self): store = self.hs.get_datastore() @@ -500,7 +496,7 @@ class RegistrationTestCase(unittest.HomeserverTestCase): user_id = self.get_success(self.handler.register_user(localpart="user")) # Get an access token. - token = self.macaroon_generator.generate_access_token(user_id) + token = "testtok" self.get_success( self.store.add_access_token_to_user( user_id=user_id, token=token, device_id=None, valid_until_ms=None @@ -577,7 +573,7 @@ class RegistrationTestCase(unittest.HomeserverTestCase): user = UserID(localpart, self.hs.hostname) user_id = user.to_string() - token = self.macaroon_generator.generate_access_token(user_id) + token = self.hs.get_auth_handler().generate_access_token(user) if need_register: await self.handler.register_with_store( diff --git a/tests/util/test_stringutils.py b/tests/util/test_stringutils.py index f7fecd9cf3..ad4dd7f007 100644 --- a/tests/util/test_stringutils.py +++ b/tests/util/test_stringutils.py @@ -13,7 +13,7 @@ # limitations under the License. from synapse.api.errors import SynapseError -from synapse.util.stringutils import assert_valid_client_secret +from synapse.util.stringutils import assert_valid_client_secret, base62_encode from .. import unittest @@ -45,3 +45,9 @@ class StringUtilsTestCase(unittest.TestCase): for client_secret in bad: with self.assertRaises(SynapseError): assert_valid_client_secret(client_secret) + + def test_base62_encode(self): + self.assertEqual("0", base62_encode(0)) + self.assertEqual("10", base62_encode(62)) + self.assertEqual("1c", base62_encode(100)) + self.assertEqual("001c", base62_encode(100, minwidth=4)) |