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

Record the SSO Auth Provider in the login token #9510

Merged
merged 14 commits into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ files =
synapse/util/async_helpers.py,
synapse/util/caches,
synapse/util/metrics.py,
synapse/util/macaroons.py,
synapse/util/stringutils.py,
tests/replication,
tests/test_utils,
Expand Down
41 changes: 9 additions & 32 deletions synapse/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from synapse.storage.databases.main.registration import TokenLookupResult
from synapse.types import StateMap, UserID
from synapse.util.caches.lrucache import LruCache
from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry
from synapse.util.metrics import Measure

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -408,43 +409,27 @@ def _parse_and_validate_macaroon(self, token, rights="access"):
raise _InvalidMacaroonException()

try:
user_id = self.get_user_id_from_macaroon(macaroon)
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, TypeError, ValueError):
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 get_user_id_from_macaroon(self, macaroon):
"""Retrieve the user_id given by the caveats on the macaroon.

Does *not* validate the macaroon.

Args:
macaroon (pymacaroons.Macaroon): The macaroon to validate

Returns:
(str) user id

Raises:
InvalidClientCredentialsError if there is no user_id caveat in the
macaroon
"""
user_prefix = "user_id = "
for caveat in macaroon.caveats:
if caveat.caveat_id.startswith(user_prefix):
return caveat.caveat_id[len(user_prefix) :]
raise InvalidClientTokenError("No user caveat in macaroon")

def validate_macaroon(self, macaroon, type_string, user_id):
"""
validate that a Macaroon is understood by and was signed by this server.
Expand All @@ -465,21 +450,13 @@ def validate_macaroon(self, macaroon, type_string, user_id):
v.satisfy_exact("type = " + type_string)
v.satisfy_exact("user_id = %s" % user_id)
v.satisfy_exact("guest = true")
v.satisfy_general(self._verify_expiry)
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 _verify_expiry(self, caveat):
prefix = "time < "
if not caveat.startswith(prefix):
return False
expiry = int(caveat[len(prefix) :])
now = self.hs.get_clock().time_msec()
return now < expiry

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
71 changes: 61 additions & 10 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
from synapse.types import JsonDict, Requester, UserID
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.threepids import canonicalise_email

Expand Down Expand Up @@ -170,6 +171,16 @@ class SsoLoginExtraAttributes:
extra_attributes = attr.ib(type=JsonDict)


@attr.s(slots=True, frozen=True)
class LoginTokenAttributes:
"""Data we store in a short-term login token"""

user_id = attr.ib(type=str)

# the SSO Identity Provider that the user authenticated with, to get this token
auth_provider_id = attr.ib(type=Optional[str])


class AuthHandler(BaseHandler):
SESSION_EXPIRE_MS = 48 * 60 * 60 * 1000

Expand Down Expand Up @@ -1164,18 +1175,16 @@ async def _check_local_password(self, user_id: str, password: str) -> Optional[s
return None
return user_id

async def validate_short_term_login_token_and_get_user_id(self, login_token: str):
auth_api = self.hs.get_auth()
user_id = None
async def validate_short_term_login_token(
self, login_token: str
) -> LoginTokenAttributes:
try:
macaroon = pymacaroons.Macaroon.deserialize(login_token)
user_id = auth_api.get_user_id_from_macaroon(macaroon)
auth_api.validate_macaroon(macaroon, "login", user_id)
res = self.macaroon_gen.verify_short_term_login_token(login_token)
except Exception:
raise AuthError(403, "Invalid token", errcode=Codes.FORBIDDEN)

await self.auth.check_auth_blocking(user_id)
return user_id
await self.auth.check_auth_blocking(res.user_id)
return res

async def delete_access_token(self, access_token: str):
"""Invalidate a single access token
Expand Down Expand Up @@ -1397,6 +1406,7 @@ async def start_sso_ui_auth(self, request: SynapseRequest, session_id: str) -> s
async def complete_sso_login(
self,
registered_user_id: str,
auth_provider_id: str,
request: Request,
client_redirect_url: str,
extra_attributes: Optional[JsonDict] = None,
Expand All @@ -1406,6 +1416,9 @@ async def complete_sso_login(

Args:
registered_user_id: The registered user ID to complete SSO login for.
auth_provider_id: The id of the SSO Identity provider that was used for
login. This will be stored in the login token for future tracking in
prometheus metrics.
request: The request to complete.
client_redirect_url: The URL to which to redirect the user at the end of the
process.
Expand All @@ -1427,6 +1440,7 @@ async def complete_sso_login(

self._complete_sso_login(
registered_user_id,
auth_provider_id,
request,
client_redirect_url,
extra_attributes,
Expand All @@ -1437,6 +1451,7 @@ async def complete_sso_login(
def _complete_sso_login(
self,
registered_user_id: str,
auth_provider_id: str,
request: Request,
client_redirect_url: str,
extra_attributes: Optional[JsonDict] = None,
Expand All @@ -1463,7 +1478,7 @@ def _complete_sso_login(

# Create a login token
login_token = self.macaroon_gen.generate_short_term_login_token(
registered_user_id
registered_user_id, auth_provider_id=auth_provider_id
)

# Append the login token to the original redirect URL (i.e. with its query
Expand Down Expand Up @@ -1569,15 +1584,51 @@ def generate_access_token(
return macaroon.serialize()

def generate_short_term_login_token(
self, user_id: str, duration_in_ms: int = (2 * 60 * 1000)
self,
user_id: str,
duration_in_ms: int = (2 * 60 * 1000),
auth_provider_id: Optional[str] = None,
) -> str:
macaroon = self._generate_base_macaroon(user_id)
macaroon.add_first_party_caveat("type = login")
now = self.hs.get_clock().time_msec()
expiry = now + duration_in_ms
macaroon.add_first_party_caveat("time < %d" % (expiry,))
if auth_provider_id is not None:
macaroon.add_first_party_caveat(
"auth_provider_id = %s" % (auth_provider_id,)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A macaroon that doesn't have an auth_provider_id caveat allows clients to add any auth_provider_id themselves. I think we want to add a auth_provider_id = None (or equivalent) when auth_provider_id is None?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of storing this information in a macaroon instead of recording it internally in the database?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A macaroon that doesn't have an auth_provider_id caveat allows clients to add any auth_provider_id themselves. I think we want to add a auth_provider_id = None (or equivalent) when auth_provider_id is None?

we were actually setting it to something not-None on all code paths. I've updated the method signature to enforce this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of storing this information in a macaroon instead of recording it internally in the database?

it's just easier! To store it in the database, we'd have to add a table, do a bunch of sql to insert and select rows, and do a bunch more sql to delete rows once they expire. Not having to do the database queries seems more efficient too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just easier!

/me looks at the size of this PR :p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-p

This PR is mostly moving existing stuff about. Storing it in the DB would be a bunch of new code.

return macaroon.serialize()

def verify_short_term_login_token(self, token: str) -> LoginTokenAttributes:
"""Verify a short-term-login macaroon

Checks that the given token is a valid, unexpired short-term-login token
minted by this server.

Args:
token: the login token to verify

Returns:
the user_id that this token is valid for

Raises:
MacaroonVerificationFailedException if the verification failed
"""
macaroon = pymacaroons.Macaroon.deserialize(token)
user_id = get_value_from_macaroon(macaroon, "user_id")
auth_provider_id = get_value_from_macaroon(macaroon, "auth_provider_id", None)

v = pymacaroons.Verifier()
v.satisfy_exact("gen = 1")
v.satisfy_exact("type = login")
v.satisfy_general(lambda c: c.startswith("user_id = "))
v.satisfy_general(lambda c: c.startswith("auth_provider_id = "))
satisfy_expiry(v, self.hs.get_clock().time_msec)
v.verify(macaroon, self.hs.config.key.macaroon_secret_key)

return LoginTokenAttributes(user_id=user_id, auth_provider_id=auth_provider_id)

def generate_delete_pusher_token(self, user_id: str) -> str:
macaroon = self._generate_base_macaroon(user_id)
macaroon.add_first_party_caveat("type = delete_pusher")
Expand Down
46 changes: 9 additions & 37 deletions synapse/handlers/oidc_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
from synapse.util import json_decoder
from synapse.util.caches.cached_call import RetryOnExceptionCachedCall
from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand Down Expand Up @@ -211,7 +212,7 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
session_data = self._token_generator.verify_oidc_session_token(
session, state
)
except (MacaroonDeserializationException, ValueError) as e:
except (MacaroonDeserializationException, KeyError) as e:
logger.exception("Invalid session for OIDC callback")
self._sso_handler.render_error(request, "invalid_session", str(e))
return
Expand Down Expand Up @@ -1046,7 +1047,7 @@ def verify_oidc_session_token(
The data extracted from the session cookie

Raises:
ValueError if an expected caveat is missing from the macaroon.
KeyError if an expected caveat is missing from the macaroon.
"""
macaroon = pymacaroons.Macaroon.deserialize(session)

Expand All @@ -1060,21 +1061,19 @@ def verify_oidc_session_token(
# Sometimes there's a UI auth session ID, it seems to be OK to attempt
# to always satisfy this.
v.satisfy_general(lambda c: c.startswith("ui_auth_session_id = "))
v.satisfy_general(self._verify_expiry)
satisfy_expiry(v, self._clock.time_msec)

v.verify(macaroon, self._macaroon_secret_key)

# Extract the session data from the token.
nonce = self._get_value_from_macaroon(macaroon, "nonce")
idp_id = self._get_value_from_macaroon(macaroon, "idp_id")
client_redirect_url = self._get_value_from_macaroon(
macaroon, "client_redirect_url"
)
nonce = get_value_from_macaroon(macaroon, "nonce")
idp_id = get_value_from_macaroon(macaroon, "idp_id")
client_redirect_url = get_value_from_macaroon(macaroon, "client_redirect_url")
try:
ui_auth_session_id = self._get_value_from_macaroon(
ui_auth_session_id = get_value_from_macaroon(
macaroon, "ui_auth_session_id"
) # type: Optional[str]
except ValueError:
except KeyError:
ui_auth_session_id = None

return OidcSessionData(
Expand All @@ -1084,33 +1083,6 @@ def verify_oidc_session_token(
ui_auth_session_id=ui_auth_session_id,
)

def _get_value_from_macaroon(self, macaroon: pymacaroons.Macaroon, key: str) -> str:
"""Extracts a caveat value from a macaroon token.

Args:
macaroon: the token
key: the key of the caveat to extract

Returns:
The extracted value

Raises:
ValueError: if the caveat was not in the macaroon
"""
prefix = key + " = "
for caveat in macaroon.caveats:
if caveat.caveat_id.startswith(prefix):
return caveat.caveat_id[len(prefix) :]
raise ValueError("No %s caveat in macaroon" % (key,))

def _verify_expiry(self, caveat: str) -> bool:
prefix = "time < "
if not caveat.startswith(prefix):
return False
expiry = int(caveat[len(prefix) :])
now = self._clock.time_msec()
return now < expiry


@attr.s(frozen=True, slots=True)
class OidcSessionData:
Expand Down
2 changes: 2 additions & 0 deletions synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ async def complete_sso_login_request(

await self._auth_handler.complete_sso_login(
user_id,
auth_provider_id,
request,
client_redirect_url,
extra_login_attributes,
Expand Down Expand Up @@ -886,6 +887,7 @@ async def register_sso_user(self, request: Request, session_id: str) -> None:

await self._auth_handler.complete_sso_login(
user_id,
session.auth_provider_id,
request,
session.client_redirect_url,
session.extra_login_attributes,
Expand Down
Loading