Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Move the "email unsubscribe" resource, refactor the macaroon generato…
Browse files Browse the repository at this point in the history 
…r & 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.
  • Loading branch information
@sandhose
sandhose authored Jun 14, 2022
1 parent 09a3c5c commit fe1daad
Show file tree
Hide file tree
Showing 16 changed files with 619 additions and 441 deletions.
1 change: 1 addition & 0 deletions changelog.d/12986.misc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Refactor macaroon tokens generation and move the unsubscribe link in notification emails to `/_synapse/client/unsubscribe`.
193 changes: 45 additions & 148 deletions synapse/api/auth.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
from synapse.logging.opentracing import active_span, force_tracing, start_active_span
from synapse.storage.databases.main.registration import TokenLookupResult
from synapse.types import Requester, UserID, create_requester
from synapse.util.caches.lrucache import LruCache
from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand All @@ -46,10 +44,6 @@
GUEST_DEVICE_ID = "guest_device"


class _InvalidMacaroonException(Exception):
pass


class Auth:
"""
This class contains functions for authenticating users of our client-server API.
Expand All @@ -61,14 +55,10 @@ def __init__(self, hs: "HomeServer"):
self.store = hs.get_datastores().main
self._account_validity_handler = hs.get_account_validity_handler()
self._storage_controllers = hs.get_storage_controllers()

self.token_cache: LruCache[str, Tuple[str, bool]] = LruCache(
10000, "token_cache"
)
self._macaroon_generator = hs.get_macaroon_generator()

self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips
self._track_puppeted_user_ips = hs.config.api.track_puppeted_user_ips
self._macaroon_secret_key = hs.config.key.macaroon_secret_key
self._force_tracing_for_users = hs.config.tracing.force_tracing_for_users

async def check_user_in_room(
Expand Down Expand Up @@ -123,7 +113,6 @@ async def get_user_by_req(
self,
request: SynapseRequest,
allow_guest: bool = False,
rights: str = "access",
allow_expired: bool = False,
) -> Requester:
"""Get a registered user's ID.
Expand All @@ -132,7 +121,6 @@ async def get_user_by_req(
request: An HTTP request with an access_token query parameter.
allow_guest: If False, will raise an AuthError if the user making the
request is a guest.
rights: The operation being performed; the access token must allow this
allow_expired: If True, allow the request through even if the account
is expired, or session token lifetime has ended. Note that
/login will deliver access tokens regardless of expiration.
Expand All @@ -147,7 +135,7 @@ async def get_user_by_req(
parent_span = active_span()
with start_active_span("get_user_by_req"):
requester = await self._wrapped_get_user_by_req(
request, allow_guest, rights, allow_expired
request, allow_guest, allow_expired
)

if parent_span:
Expand All @@ -173,7 +161,6 @@ async def _wrapped_get_user_by_req(
self,
request: SynapseRequest,
allow_guest: bool,
rights: str,
allow_expired: bool,
) -> Requester:
"""Helper for get_user_by_req
Expand Down Expand Up @@ -211,7 +198,7 @@ async def _wrapped_get_user_by_req(
return requester

user_info = await self.get_user_by_access_token(
access_token, rights, allow_expired=allow_expired
access_token, allow_expired=allow_expired
)
token_id = user_info.token_id
is_guest = user_info.is_guest
Expand Down Expand Up @@ -391,15 +378,12 @@ async def _get_appservice_user_id_and_device_id(
async def get_user_by_access_token(
self,
token: str,
rights: str = "access",
allow_expired: bool = False,
) -> TokenLookupResult:
"""Validate access token and get user_id from it
Args:
token: The access token to get the user by
rights: The operation being performed; the access token must
allow this
allow_expired: If False, raises an InvalidClientTokenError
if the token is expired
Expand All @@ -410,70 +394,55 @@ async def get_user_by_access_token(
is invalid
"""

if rights == "access":
# First look in the database to see if the access token is present
# as an opaque token.
r = await self.store.get_user_by_access_token(token)
if r:
valid_until_ms = r.valid_until_ms
if (
not allow_expired
and valid_until_ms is not None
and valid_until_ms < self.clock.time_msec()
):
# there was a valid access token, but it has expired.
# soft-logout the user.
raise InvalidClientTokenError(
msg="Access token has expired", soft_logout=True
)
# First look in the database to see if the access token is present
# as an opaque token.
r = await self.store.get_user_by_access_token(token)
if r:
valid_until_ms = r.valid_until_ms
if (
not allow_expired
and valid_until_ms is not None
and valid_until_ms < self.clock.time_msec()
):
# there was a valid access token, but it has expired.
# soft-logout the user.
raise InvalidClientTokenError(
msg="Access token has expired", soft_logout=True
)

return r
return r

# If the token isn't found in the database, then it could still be a
# macaroon, so we check that here.
# macaroon for a guest, so we check that here.
try:
user_id, guest = self._parse_and_validate_macaroon(token, rights)

if rights == "access":
if not guest:
# non-guest access tokens must be in the database
logger.warning("Unrecognised access token - not in store.")
raise InvalidClientTokenError()

# Guest access tokens are not stored in the database (there can
# only be one access token per guest, anyway).
#
# In order to prevent guest access tokens being used as regular
# user access tokens (and hence getting around the invalidation
# process), we look up the user id and check that it is indeed
# a guest user.
#
# It would of course be much easier to store guest access
# tokens in the database as well, but that would break existing
# guest tokens.
stored_user = await self.store.get_user_by_id(user_id)
if not stored_user:
raise InvalidClientTokenError("Unknown user_id %s" % user_id)
if not stored_user["is_guest"]:
raise InvalidClientTokenError(
"Guest access token used for regular user"
)

ret = TokenLookupResult(
user_id=user_id,
is_guest=True,
# all guests get the same device id
device_id=GUEST_DEVICE_ID,
user_id = self._macaroon_generator.verify_guest_token(token)

# Guest access tokens are not stored in the database (there can
# only be one access token per guest, anyway).
#
# In order to prevent guest access tokens being used as regular
# user access tokens (and hence getting around the invalidation
# process), we look up the user id and check that it is indeed
# a guest user.
#
# It would of course be much easier to store guest access
# tokens in the database as well, but that would break existing
# guest tokens.
stored_user = await self.store.get_user_by_id(user_id)
if not stored_user:
raise InvalidClientTokenError("Unknown user_id %s" % user_id)
if not stored_user["is_guest"]:
raise InvalidClientTokenError(
"Guest access token used for regular user"
)
elif rights == "delete_pusher":
# We don't store these tokens in the database

ret = TokenLookupResult(user_id=user_id, is_guest=False)
else:
raise RuntimeError("Unknown rights setting %s", rights)
return ret
return TokenLookupResult(
user_id=user_id,
is_guest=True,
# all guests get the same device id
device_id=GUEST_DEVICE_ID,
)
except (
_InvalidMacaroonException,
pymacaroons.exceptions.MacaroonException,
TypeError,
ValueError,
Expand All @@ -485,78 +454,6 @@ async def get_user_by_access_token(
)
raise InvalidClientTokenError("Invalid access token passed.")

def _parse_and_validate_macaroon(
self, token: str, rights: str = "access"
) -> Tuple[str, bool]:
"""Takes a macaroon and tries to parse and validate it. This is cached
if and only if rights == access and there isn't an expiry.
On invalid macaroon raises _InvalidMacaroonException
Returns:
(user_id, is_guest)
"""
if rights == "access":
cached = self.token_cache.get(token, None)
if cached:
return cached

try:
macaroon = pymacaroons.Macaroon.deserialize(token)
except Exception: # deserialize can throw more-or-less anything
# The access token doesn't look like a macaroon.
raise _InvalidMacaroonException()

try:
user_id = get_value_from_macaroon(macaroon, "user_id")

guest = False
for caveat in macaroon.caveats:
if caveat.caveat_id == "guest = true":
guest = True

self.validate_macaroon(macaroon, rights, user_id=user_id)
except (
pymacaroons.exceptions.MacaroonException,
KeyError,
TypeError,
ValueError,
):
raise InvalidClientTokenError("Invalid macaroon passed.")

if rights == "access":
self.token_cache[token] = (user_id, guest)

return user_id, guest

def validate_macaroon(
self, macaroon: pymacaroons.Macaroon, type_string: str, user_id: str
) -> None:
"""
validate that a Macaroon is understood by and was signed by this server.
Args:
macaroon: The macaroon to validate
type_string: The kind of token required (e.g. "access", "delete_pusher")
user_id: The user_id required
"""
v = pymacaroons.Verifier()

# the verifier runs a test for every caveat on the macaroon, to check
# that it is met for the current request. Each caveat must match at
# least one of the predicates specified by satisfy_exact or
# specify_general.
v.satisfy_exact("gen = 1")
v.satisfy_exact("type = " + type_string)
v.satisfy_exact("user_id = %s" % user_id)
v.satisfy_exact("guest = true")
satisfy_expiry(v, self.clock.time_msec)

# access_tokens include a nonce for uniqueness: any value is acceptable
v.satisfy_general(lambda c: c.startswith("nonce = "))

v.verify(macaroon, self._macaroon_secret_key)

def get_appservice_by_req(self, request: SynapseRequest) -> ApplicationService:
token = self.get_access_token_from_request(request)
service = self.store.get_app_service_by_token(token)
Expand Down
6 changes: 4 additions & 2 deletions synapse/config/key.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,16 +159,18 @@ def read_config(
)
)

self.macaroon_secret_key = config.get(
macaroon_secret_key: Optional[str] = config.get(
"macaroon_secret_key", self.root.registration.registration_shared_secret
)

if not self.macaroon_secret_key:
if not macaroon_secret_key:
# Unfortunately, there are people out there that don't have this
# set. Lets just be "nice" and derive one from their secret key.
logger.warning("Config is missing macaroon_secret_key")
seed = bytes(self.signing_key[0])
self.macaroon_secret_key = hashlib.sha256(seed).digest()
else:
self.macaroon_secret_key = macaroon_secret_key.encode("utf-8")

# a secret which is used to calculate HMACs for form values, to stop
# falsification of values
Expand Down
Loading

0 comments on commit fe1daad

Please sign in to comment.