This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Change the format of access tokens away from macaroons #5588
Merged
Merged
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
31d15c6
Move get_or_create_user to test code
richvdh 0163b45
use opaque strings as access tokens
richvdh 912fbea
changelog
richvdh 2953aa7
fix changelog
richvdh 6b562ce
access token length
richvdh 91ade0c
Merge remote-tracking branch 'origin/develop' into rav/soft_logout/ki…
richvdh 25d9aa8
back out extraneous changes
richvdh 57c73c2
fix bad merge
richvdh abda8cd
lint
richvdh 19ebc19
fix tests
richvdh 7843281
lint
richvdh 55265e8
Extend token format
richvdh 7bfa66d
fix mau tests
richvdh 3b66b11
fix registration test
richvdh b0c61c5
Update 5588.misc
richvdh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import time | ||
import unicodedata | ||
import urllib.parse | ||
from binascii import crc32 | ||
from typing import ( | ||
TYPE_CHECKING, | ||
Any, | ||
|
@@ -34,6 +35,7 @@ | |
import attr | ||
import bcrypt | ||
import pymacaroons | ||
import unpaddedbase64 | ||
|
||
from twisted.web.server import Request | ||
|
||
|
@@ -66,6 +68,7 @@ | |
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,18 +811,20 @@ async def get_access_token_for_user_id( | |
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 | ||
or self.hs.config.appservice.track_appservice_user_ips | ||
): | ||
await self.auth.check_auth_blocking(user_id) | ||
|
||
access_token = self.generate_access_token() | ||
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,9 +1197,18 @@ async def _check_local_password(self, user_id: str, password: str) -> Optional[s | |
return None | ||
return user_id | ||
|
||
def generate_access_token(self): | ||
def generate_access_token(self, for_user: UserID) -> str: | ||
"""Generates an opaque string, for use as an access token""" | ||
return stringutils.random_string(20) | ||
|
||
# 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) | ||
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. Why are we using base62 for the crc? Is that just how its typically done? 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. 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. wfm |
||
return f"{base}_{crc}" | ||
|
||
async def validate_short_term_login_token( | ||
self, login_token: str | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
a thought occurs. The use of base64 here means that access tokens will have to be correctly url-encoded when used in a query param.
maybe we should use a url-safe base64 encoding.
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.
Oh, hmm. Good point.
I had a quick look at haproxy support and it looks like
b64dec
only handles standard padded base64 (2.4 will have support for padded url safe base64 decoding). Don't that is a huge concern though, since I think conversion is relatively easy with simple string replacement?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.
of course, we could just say that only people with spec-compliant mxids are allowed to use matrix, so it doesn't need encoding at all 😈
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 love it :D
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 don't think this needs to particularly influence us; we should be deprecating and removing GET based access token use, as it's just a bad idea for http access logs outside of synapse, perhaps the effort of ensuring your access tokens are urlencoded correctly will push developers towards doing the right thing.
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 think that's fair. it's really not that hard to do it right, even from a shell command.