Change the format of access tokens away from macaroons (#5588)

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