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

This commit is contained in:
Richard van der Hoff 2021-05-12 15:04:51 +01:00 committed by GitHub
parent affaffb0ab
commit 7562d887e1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 78 additions and 103 deletions

1
changelog.d/5588.misc Normal file
View File

@ -0,0 +1 @@
Reduce the length of Synapse's access tokens.

View File

@ -1,4 +1,4 @@
#!/usr/bin/env python2 #!/usr/bin/env python
import sys import sys

View File

@ -17,6 +17,7 @@ import logging
import time import time
import unicodedata import unicodedata
import urllib.parse import urllib.parse
from binascii import crc32
from typing import ( from typing import (
TYPE_CHECKING, TYPE_CHECKING,
Any, Any,
@ -34,6 +35,7 @@ from typing import (
import attr import attr
import bcrypt import bcrypt
import pymacaroons import pymacaroons
import unpaddedbase64
from twisted.web.server import Request 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.async_helpers import maybe_awaitable
from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry
from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.msisdn import phone_number_to_msisdn
from synapse.util.stringutils import base62_encode
from synapse.util.threepids import canonicalise_email from synapse.util.threepids import canonicalise_email
if TYPE_CHECKING: if TYPE_CHECKING:
@ -808,10 +811,12 @@ class AuthHandler(BaseHandler):
logger.info( logger.info(
"Logging in user %s as %s%s", user_id, puppets_user_id, fmt_expiry "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: else:
logger.info( logger.info(
"Logging in user %s on device %s%s", user_id, device_id, fmt_expiry "Logging in user %s on device %s%s", user_id, device_id, fmt_expiry
) )
target_user_id_obj = UserID.from_string(user_id)
if ( if (
not is_appservice_ghost not is_appservice_ghost
@ -819,7 +824,7 @@ class AuthHandler(BaseHandler):
): ):
await self.auth.check_auth_blocking(user_id) 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( await self.store.add_access_token_to_user(
user_id=user_id, user_id=user_id,
token=access_token, token=access_token,
@ -1192,6 +1197,19 @@ class AuthHandler(BaseHandler):
return None return None
return user_id 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( async def validate_short_term_login_token(
self, login_token: str self, login_token: str
) -> LoginTokenAttributes: ) -> LoginTokenAttributes:
@ -1585,10 +1603,7 @@ class MacaroonGenerator:
hs = attr.ib() hs = attr.ib()
def generate_access_token( def generate_guest_access_token(self, user_id: str) -> str:
self, user_id: str, extra_caveats: Optional[List[str]] = None
) -> str:
extra_caveats = extra_caveats or []
macaroon = self._generate_base_macaroon(user_id) macaroon = self._generate_base_macaroon(user_id)
macaroon.add_first_party_caveat("type = access") macaroon.add_first_party_caveat("type = access")
# Include a nonce, to make sure that each login gets a different # Include a nonce, to make sure that each login gets a different
@ -1596,8 +1611,7 @@ class MacaroonGenerator:
macaroon.add_first_party_caveat( macaroon.add_first_party_caveat(
"nonce = %s" % (stringutils.random_string_with_symbols(16),) "nonce = %s" % (stringutils.random_string_with_symbols(16),)
) )
for caveat in extra_caveats: macaroon.add_first_party_caveat("guest = true")
macaroon.add_first_party_caveat(caveat)
return macaroon.serialize() return macaroon.serialize()
def generate_short_term_login_token( def generate_short_term_login_token(

View File

@ -722,9 +722,7 @@ class RegistrationHandler(BaseHandler):
) )
if is_guest: if is_guest:
assert valid_until_ms is None assert valid_until_ms is None
access_token = self.macaroon_gen.generate_access_token( access_token = self.macaroon_gen.generate_guest_access_token(user_id)
user_id, ["guest = true"]
)
else: else:
access_token = await self._auth_handler.get_access_token_for_user_id( access_token = await self._auth_handler.get_access_token_for_user_id(
user_id, user_id,

View File

@ -220,3 +220,23 @@ def strtobool(val: str) -> bool:
return False return False
else: else:
raise ValueError("invalid truth value %r" % (val,)) 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

View File

@ -21,13 +21,11 @@ from synapse.api.constants import UserTypes
from synapse.api.errors import ( from synapse.api.errors import (
AuthError, AuthError,
Codes, Codes,
InvalidClientCredentialsError,
InvalidClientTokenError, InvalidClientTokenError,
MissingClientTokenError, MissingClientTokenError,
ResourceLimitError, ResourceLimitError,
) )
from synapse.storage.databases.main.registration import TokenLookupResult from synapse.storage.databases.main.registration import TokenLookupResult
from synapse.types import UserID
from tests import unittest from tests import unittest
from tests.test_utils import simple_async_mock from tests.test_utils import simple_async_mock
@ -253,67 +251,6 @@ class AuthTestCase(unittest.HomeserverTestCase):
self.assertTrue(user_info.is_guest) self.assertTrue(user_info.is_guest)
self.store.get_user_by_id.assert_called_with(user_id) 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): def test_blocking_mau(self):
self.auth_blocking._limit_usage_by_mau = False self.auth_blocking._limit_usage_by_mau = False
self.auth_blocking._max_mau_value = 50 self.auth_blocking._max_mau_value = 50

View File

@ -16,12 +16,17 @@ from unittest.mock import Mock
import pymacaroons import pymacaroons
from synapse.api.errors import AuthError, ResourceLimitError from synapse.api.errors import AuthError, ResourceLimitError
from synapse.rest import admin
from tests import unittest from tests import unittest
from tests.test_utils import make_awaitable from tests.test_utils import make_awaitable
class AuthTestCase(unittest.HomeserverTestCase): class AuthTestCase(unittest.HomeserverTestCase):
servlets = [
admin.register_servlets,
]
def prepare(self, reactor, clock, hs): def prepare(self, reactor, clock, hs):
self.auth_handler = hs.get_auth_handler() self.auth_handler = hs.get_auth_handler()
self.macaroon_generator = hs.get_macaroon_generator() self.macaroon_generator = hs.get_macaroon_generator()
@ -35,16 +40,10 @@ class AuthTestCase(unittest.HomeserverTestCase):
self.small_number_of_users = 1 self.small_number_of_users = 1
self.large_number_of_users = 100 self.large_number_of_users = 100
def test_token_is_a_macaroon(self): self.user1 = self.register_user("a_user", "pass")
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())
def test_macaroon_caveats(self): 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) macaroon = pymacaroons.Macaroon.deserialize(token)
def verify_gen(caveat): def verify_gen(caveat):
@ -59,19 +58,23 @@ class AuthTestCase(unittest.HomeserverTestCase):
def verify_nonce(caveat): def verify_nonce(caveat):
return caveat.startswith("nonce =") return caveat.startswith("nonce =")
def verify_guest(caveat):
return caveat == "guest = true"
v = pymacaroons.Verifier() v = pymacaroons.Verifier()
v.satisfy_general(verify_gen) v.satisfy_general(verify_gen)
v.satisfy_general(verify_user) v.satisfy_general(verify_user)
v.satisfy_general(verify_type) v.satisfy_general(verify_type)
v.satisfy_general(verify_nonce) v.satisfy_general(verify_nonce)
v.satisfy_general(verify_guest)
v.verify(macaroon, self.hs.config.macaroon_secret_key) v.verify(macaroon, self.hs.config.macaroon_secret_key)
def test_short_term_login_token_gives_user_id(self): def test_short_term_login_token_gives_user_id(self):
token = self.macaroon_generator.generate_short_term_login_token( 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)) 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) self.assertEqual("", res.auth_provider_id)
# when we advance the clock, the token should be rejected # 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): def test_short_term_login_token_gives_auth_provider(self):
token = self.macaroon_generator.generate_short_term_login_token( 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)) 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) self.assertEqual("my_idp", res.auth_provider_id)
def test_short_term_login_token_cannot_replace_user_id(self): def test_short_term_login_token_cannot_replace_user_id(self):
token = self.macaroon_generator.generate_short_term_login_token( token = self.macaroon_generator.generate_short_term_login_token(
"a_user", "", 5000 self.user1, "", 5000
) )
macaroon = pymacaroons.Macaroon.deserialize(token) macaroon = pymacaroons.Macaroon.deserialize(token)
res = self.get_success( res = self.get_success(
self.auth_handler.validate_short_term_login_token(macaroon.serialize()) 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 # add another "user_id" caveat, which might allow us to override the
# user_id. # user_id.
@ -114,7 +117,7 @@ class AuthTestCase(unittest.HomeserverTestCase):
# Ensure does not throw exception # Ensure does not throw exception
self.get_success( self.get_success(
self.auth_handler.get_access_token_for_user_id( 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.get_failure(
self.auth_handler.get_access_token_for_user_id( 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, ResourceLimitError,
) )
@ -160,7 +163,7 @@ class AuthTestCase(unittest.HomeserverTestCase):
# If not in monthly active cohort # If not in monthly active cohort
self.get_failure( self.get_failure(
self.auth_handler.get_access_token_for_user_id( 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, ResourceLimitError,
) )
@ -177,7 +180,7 @@ class AuthTestCase(unittest.HomeserverTestCase):
) )
self.get_success( self.get_success(
self.auth_handler.get_access_token_for_user_id( 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( self.get_success(
@ -195,7 +198,7 @@ class AuthTestCase(unittest.HomeserverTestCase):
# Ensure does not raise exception # Ensure does not raise exception
self.get_success( self.get_success(
self.auth_handler.get_access_token_for_user_id( 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): def _get_macaroon(self):
token = self.macaroon_generator.generate_short_term_login_token( token = self.macaroon_generator.generate_short_term_login_token(
"user_a", "", 5000 self.user1, "", 5000
) )
return pymacaroons.Macaroon.deserialize(token) return pymacaroons.Macaroon.deserialize(token)

View File

@ -48,10 +48,6 @@ class RegistrationTestCase(unittest.HomeserverTestCase):
self.mock_distributor = Mock() self.mock_distributor = Mock()
self.mock_distributor.declare("registered_user") self.mock_distributor.declare("registered_user")
self.mock_captcha_client = Mock() 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.handler = self.hs.get_registration_handler()
self.store = self.hs.get_datastore() self.store = self.hs.get_datastore()
self.lots_of_users = 100 self.lots_of_users = 100
@ -67,8 +63,8 @@ class RegistrationTestCase(unittest.HomeserverTestCase):
self.get_or_create_user(requester, frank.localpart, "Frankie") self.get_or_create_user(requester, frank.localpart, "Frankie")
) )
self.assertEquals(result_user_id, user_id) self.assertEquals(result_user_id, user_id)
self.assertTrue(result_token is not None) self.assertIsInstance(result_token, str)
self.assertEquals(result_token, "secret") self.assertGreater(len(result_token), 20)
def test_if_user_exists(self): def test_if_user_exists(self):
store = self.hs.get_datastore() store = self.hs.get_datastore()
@ -500,7 +496,7 @@ class RegistrationTestCase(unittest.HomeserverTestCase):
user_id = self.get_success(self.handler.register_user(localpart="user")) user_id = self.get_success(self.handler.register_user(localpart="user"))
# Get an access token. # Get an access token.
token = self.macaroon_generator.generate_access_token(user_id) token = "testtok"
self.get_success( self.get_success(
self.store.add_access_token_to_user( self.store.add_access_token_to_user(
user_id=user_id, token=token, device_id=None, valid_until_ms=None 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 = UserID(localpart, self.hs.hostname)
user_id = user.to_string() 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: if need_register:
await self.handler.register_with_store( await self.handler.register_with_store(

View File

@ -13,7 +13,7 @@
# limitations under the License. # limitations under the License.
from synapse.api.errors import SynapseError 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 from .. import unittest
@ -45,3 +45,9 @@ class StringUtilsTestCase(unittest.TestCase):
for client_secret in bad: for client_secret in bad:
with self.assertRaises(SynapseError): with self.assertRaises(SynapseError):
assert_valid_client_secret(client_secret) 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))