From 49fe26e6141d16f0bf6d495c7c594362acd413ee Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 16:21:59 +0200 Subject: [PATCH 01/19] Support back-channel logouts from OIDC providers Signed-off-by: Quentin Gliech --- changelog.d/11414.feature | 1 + synapse/config/oidc.py | 5 + synapse/handlers/oidc.py | 261 +++++++++++++++++- synapse/rest/synapse/client/oidc/__init__.py | 4 + .../oidc/backchannel_logout_resource.py | 35 +++ 5 files changed, 305 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11414.feature create mode 100644 synapse/rest/synapse/client/oidc/backchannel_logout_resource.py diff --git a/changelog.d/11414.feature b/changelog.d/11414.feature new file mode 100644 index 000000000000..fc035e50a741 --- /dev/null +++ b/changelog.d/11414.feature @@ -0,0 +1 @@ +Support back-channel logouts from OpenID Connect providers. diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 5418a332da14..b6c35ac495a6 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -123,6 +123,7 @@ def oidc_enabled(self) -> bool: "userinfo_endpoint": {"type": "string"}, "jwks_uri": {"type": "string"}, "skip_verification": {"type": "boolean"}, + "backchannel_logout_enabled": {"type": "boolean"}, "user_profile_method": { "type": "string", "enum": ["auto", "userinfo_endpoint"], @@ -292,6 +293,7 @@ def _parse_oidc_config_dict( token_endpoint=oidc_config.get("token_endpoint"), userinfo_endpoint=oidc_config.get("userinfo_endpoint"), jwks_uri=oidc_config.get("jwks_uri"), + backchannel_logout_enabled=oidc_config.get("backchannel_logout_enabled", False), skip_verification=oidc_config.get("skip_verification", False), user_profile_method=oidc_config.get("user_profile_method", "auto"), allow_existing_users=oidc_config.get("allow_existing_users", False), @@ -368,6 +370,9 @@ class OidcProviderConfig: # "openid" scope is used. jwks_uri: Optional[str] + # Whether Synapse should react to backchannel logouts + backchannel_logout_enabled: bool + # Whether to skip metadata verification skip_verification: bool diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 9759daf043ad..e6503583a78b 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -12,14 +12,18 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import binascii import inspect +import json import logging from typing import TYPE_CHECKING, Any, Dict, Generic, List, Optional, TypeVar, Union from urllib.parse import urlencode, urlparse import attr +import unpaddedbase64 from authlib.common.security import generate_token -from authlib.jose import JsonWebToken, jwt +from authlib.jose import JsonWebToken, JWTClaims, jwt +from authlib.jose.errors import InvalidClaimError, MissingClaimError from authlib.oauth2.auth import ClientAuth from authlib.oauth2.rfc6749.parameters import prepare_grant_uri from authlib.oidc.core import CodeIDToken, UserInfo @@ -35,9 +39,12 @@ from twisted.web.client import readBody from twisted.web.http_headers import Headers +from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.config.oidc import OidcProviderClientSecretJwtKey, OidcProviderConfig from synapse.handlers.sso import MappingException, UserAttributes +from synapse.http.server import finish_request +from synapse.http.servlet import parse_string from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart @@ -247,6 +254,78 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None: await oidc_provider.handle_oidc_callback(request, session_data, code) + async def handle_backchannel_logout(self, request: SynapseRequest) -> None: + """Handle an incoming request to /_synapse/client/oidc/backchannel_logout + + This extracts the logout_token from the request and try to figure out + from whom it is coming from. This works by matching the iss claim with + the issuer and the aud claim with the client_id. + + Since at this point we don't know who signed the JWT, we can't just + decode it automatically, we have to manually parse the JWT to extract + both claims. + + Args: + request: the incoming request from the browser. + """ + logout_token = parse_string(request, "logout_token") + if logout_token is None: + raise SynapseError(400, "Missing logout_token in request") + + # A JWT looks like this: + # header.payload.signature + # where all parts are encoded with urlsafe base64. + # The aud and iss claims we care about are in the payload part, which + # is a JSON object. + try: + # This raises if there are too many or not enough segments in the token + header, payload, signature = logout_token.rsplit(".", 4) + except ValueError: + raise SynapseError(400, "Invalid logout_token in request") + + try: + payload_bytes = unpaddedbase64.decode_base64(payload) + claims = json.loads(payload_bytes) + except (json.JSONDecodeError, binascii.Error): + raise SynapseError(400, "Invalid logout_token payload in request") + + try: + # Let's extract the iss and aud claim + iss = claims["iss"] + aud = claims["aud"] + # The aud claim can be either a string or a list of string. Here we + # normalize it as a list of strings. + if isinstance(aud, str): + aud = [aud] + + # Check that we have the right types for the aud and the iss claims + assert isinstance(iss, str) + assert isinstance(aud, list) + for a in aud: + assert isinstance(a, str) + + # At this point we properly checked both claims types + issuer: str = iss + audience: List[str] = aud + except (TypeError, KeyError, AssertionError): + raise SynapseError(400, "Invalid issuer/audience in logout_token") + + # Now that we know the audience and the issuer, we can figure out from + # what provider it is coming from + oidc_provider: Optional[OidcProvider] = None + for provider in self._providers.values(): + if provider.issuer == issuer and any( + a == provider.client_id for a in audience + ): + oidc_provider = provider + break + + if oidc_provider is None: + raise SynapseError(400, "Could not find the OP that issued this event") + + # We now figured out what provider should handle the logout request + await oidc_provider.handle_backchannel_logout(request, logout_token) + class OidcError(Exception): """Used to catch errors when calling the token_endpoint""" @@ -342,6 +421,7 @@ def __init__( self.idp_brand = provider.idp_brand self._sso_handler = hs.get_sso_handler() + self._device_handler = hs.get_device_handler() self._sso_handler.register_identity_provider(self) @@ -400,6 +480,22 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: # If we're not using userinfo, we need a valid jwks to validate the ID token m.validate_jwks_uri() + if self._config.backchannel_logout_enabled: + if not m.get("backchannel_logout_supported", False): + logger.warning( + "OIDC Back-Channel Logout is enabled for issuer %r" + "but it does not advertise support for it", + self.issuer, + ) + + if not m.get("backchannel_logout_session_supported", False): + logger.warning( + "OIDC Back-Channel Logout is enabled and supported " + "by issuer %r but it might not send session ID with " + "logout tokens, which is required for the logouts to work", + self.issuer, + ) + @property def _uses_userinfo(self) -> bool: """Returns True if the ``userinfo_endpoint`` should be used. @@ -415,6 +511,14 @@ def _uses_userinfo(self) -> bool: or self._user_profile_method == "userinfo_endpoint" ) + @property + def issuer(self) -> str: + return self._config.issuer + + @property + def client_id(self) -> str: + return self._config.client_id + async def load_metadata(self, force: bool = False) -> OpenIDProviderMetadata: """Return the provider metadata. @@ -1043,6 +1147,161 @@ def _remote_id_from_userinfo(self, userinfo: UserInfo) -> str: # to be strings. return str(remote_user_id) + async def handle_backchannel_logout( + self, request: SynapseRequest, logout_token: str + ) -> None: + """Handle an incoming request to /_synapse/client/oidc/backchannel_logout + + The OIDC Provider posts a logout token to this endpoint when a user + session ends. That token is a JWT signed with the same keys as + ID tokens. The OpenID Connect Back-Channel Logout draft explains how to + validate the JWT and figure out what session to end. + + Args: + request: The request to respond to + logout_token: The logout token (a JWT) extracted from the request body + """ + # Back-Channel Logout can be disabled in the config, hence this check. + # This is not that important for now since Synapse is registered + # manually to the OP, so not specifying the backchannel-logout URI is + # as effective than disabling it here. It might make more sense if we + # support dynamic registration in Synapse at some point. + if not self._config.backchannel_logout_enabled: + logger.warning( + f"Received an OIDC Back-Channel Logout request from issuer {self.issuer!r} but it is disabled in config" + ) + raise SynapseError( + 400, "OpenID Connect Back-Channel Logout is disabled for this provider" + ) + + metadata = await self.load_metadata() + + # As per OIDC Back-Channel Logout 1.0 sec. 2.4: + # A Logout Token MUST be signed and MAY also be encrypted. The same + # keys are used to sign and encrypt Logout Tokens as are used for ID + # Tokens. If the Logout Token is encrypted, it SHOULD replicate the + # iss (issuer) claim in the JWT Header Parameters, as specified in + # Section 5.3 of [JWT]. + alg_values = metadata.get("id_token_signing_alg_values_supported", ["RS256"]) + jwt = JsonWebToken(alg_values) + + # As per sec. 2.6: + # 3. Validate the iss, aud, and iat Claims in the same way they are + # validated in ID Tokens. + # Which means the audience should contain Synapse's client_id and the + # issuer should be the IdP issuer + claim_options = { + "iss": {"values": [self.issuer]}, + "aud": {"values": [self.client_id]}, + } + + logger.debug("Attempting to decode JWT logout_token %r", logout_token) + + # Try to decode the keys in cache first, then retry by forcing the keys + # to be reloaded + jwk_set = await self.load_jwks() + try: + claims = jwt.decode( + logout_token, + key=jwk_set, + claims_cls=LogoutToken, + claims_options=claim_options, + ) + except ValueError: + logger.info("Reloading JWKS after decode error") + jwk_set = await self.load_jwks(force=True) # try reloading the jwks + claims = jwt.decode( + logout_token, + key=jwk_set, + claims_cls=LogoutToken, + claims_options=claim_options, + ) + + logger.debug("Decoded logout_token JWT %r; validating", claims) + claims.validate(leeway=120) # allows 2 min of clock skew + + # As per sec. 2.6: + # 4. Verify that the Logout Token contains a sub Claim, a sid Claim, + # or both. + # 5. Verify that the Logout Token contains an events Claim whose + # value is JSON object containing the member name + # http://schemas.openid.net/event/backchannel-logout. + # 6. Verify that the Logout Token does not contain a nonce Claim. + # This is all verified by the LogoutToken claims class, so at this + # point the `sid` claim exists and is a string. + sid: str = claims.get("sid") + + # Find in the store if we have a device associated with that SID + devices = await self._store.get_devices_by_auth_provider_session_id( + auth_provider_id=self.idp_id, + auth_provider_session_id=sid, + ) + for device in devices: + user_id = device["user_id"] + device_id = device["device_id"] + logger.info( + "Logging out %r (device %r) via OIDC backchannel logout (sid %r).", + user_id, + device_id, + sid, + ) + await self._device_handler.delete_devices(user_id, [device_id]) + + request.setResponseCode(200) + request.setHeader(b"Cache-Control", b"no-cache, no-store") + request.setHeader(b"Pragma", b"no-cache") + finish_request(request) + + +class LogoutToken(JWTClaims): + """ + Holds and verify claims of a logout token, as per + https://openid.net/specs/openid-connect-backchannel-1_0.html#LogoutToken + """ + + REGISTERED_CLAIMS = ["iss", "sub", "aud", "iat", "jti", "events", "sid"] + + def validate(self, now: Optional[int] = None, leeway: int = 0) -> None: + """Validate everything in claims payload.""" + super(LogoutToken, self).validate(now, leeway) + self.validate_sid() + self.validate_events() + self.validate_nonce() + + def validate_exp(self, now: Optional[int] = None, leeway: int = 0) -> None: + """Do not validate the exp claim""" + pass + + def validate_nbf(self, now: Optional[int] = None, leeway: int = 0) -> None: + """Do not validate the nbf claim""" + pass + + def validate_sid(self) -> None: + """Ensure the sid claim is present""" + sid = self.get("sid") + if not sid: + raise MissingClaimError("sid") + + if not isinstance(sid, str): + raise InvalidClaimError("sid") + + def validate_nonce(self) -> None: + """Ensure the nonce claim is absent""" + if "nonce" in self: + raise InvalidClaimError("nonce") + + def validate_events(self) -> None: + """Ensure the events is present and with the right value""" + events = self.get("events") + if not events: + raise MissingClaimError("events") + + if not isinstance(events, dict): + raise InvalidClaimError("events") + + if "http://schemas.openid.net/event/backchannel-logout" not in events: + raise InvalidClaimError("events") + # number of seconds a newly-generated client secret should be valid for CLIENT_SECRET_VALIDITY_SECONDS = 3600 diff --git a/synapse/rest/synapse/client/oidc/__init__.py b/synapse/rest/synapse/client/oidc/__init__.py index 81fec39659fc..e4b28ce3dfc9 100644 --- a/synapse/rest/synapse/client/oidc/__init__.py +++ b/synapse/rest/synapse/client/oidc/__init__.py @@ -17,6 +17,9 @@ from twisted.web.resource import Resource +from synapse.rest.synapse.client.oidc.backchannel_logout_resource import ( + OIDCBackchannelLogoutResource, +) from synapse.rest.synapse.client.oidc.callback_resource import OIDCCallbackResource if TYPE_CHECKING: @@ -29,6 +32,7 @@ class OIDCResource(Resource): def __init__(self, hs: "HomeServer"): Resource.__init__(self) self.putChild(b"callback", OIDCCallbackResource(hs)) + self.putChild(b"backchannel_logout", OIDCBackchannelLogoutResource(hs)) __all__ = ["OIDCResource"] diff --git a/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py b/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py new file mode 100644 index 000000000000..2a14f6f36dbf --- /dev/null +++ b/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py @@ -0,0 +1,35 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import logging +from typing import TYPE_CHECKING + +from synapse.http.server import DirectServeJsonResource +from synapse.http.site import SynapseRequest + +if TYPE_CHECKING: + from synapse.server import HomeServer + +logger = logging.getLogger(__name__) + + +class OIDCBackchannelLogoutResource(DirectServeJsonResource): + isLeaf = 1 + + def __init__(self, hs: "HomeServer"): + super().__init__() + self._oidc_handler = hs.get_oidc_handler() + + async def _async_render_POST(self, request: SynapseRequest) -> None: + await self._oidc_handler.handle_backchannel_logout(request) From a0bb53df8e1822b563f0e6094470541eb10fa404 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 7 Dec 2021 10:14:22 +0100 Subject: [PATCH 02/19] Apply suggestions from code review Co-authored-by: Patrick Cloke --- synapse/handlers/oidc.py | 41 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index e6503583a78b..5b2eda07d880 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -22,7 +22,7 @@ import attr import unpaddedbase64 from authlib.common.security import generate_token -from authlib.jose import JsonWebToken, JWTClaims, jwt +from authlib.jose import JsonWebToken, JWTClaims from authlib.jose.errors import InvalidClaimError, MissingClaimError from authlib.oauth2.auth import ClientAuth from authlib.oauth2.rfc6749.parameters import prepare_grant_uri @@ -257,17 +257,18 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None: async def handle_backchannel_logout(self, request: SynapseRequest) -> None: """Handle an incoming request to /_synapse/client/oidc/backchannel_logout - This extracts the logout_token from the request and try to figure out - from whom it is coming from. This works by matching the iss claim with - the issuer and the aud claim with the client_id. + This extracts the logout_token from the request and tries to figure out + which account it is associated with. This works by matching the iss claim + with the issuer and the aud claim with the client_id. Since at this point we don't know who signed the JWT, we can't just - decode it automatically, we have to manually parse the JWT to extract - both claims. + decode it using authlib since it will always verifies the signature. We + have to decode it manually without validating the signature. Args: request: the incoming request from the browser. """ + logger.warn("OHAI") logout_token = parse_string(request, "logout_token") if logout_token is None: raise SynapseError(400, "Missing logout_token in request") @@ -279,18 +280,18 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: # is a JSON object. try: # This raises if there are too many or not enough segments in the token - header, payload, signature = logout_token.rsplit(".", 4) + _, payload, _ = logout_token.rsplit(".", 4) except ValueError: raise SynapseError(400, "Invalid logout_token in request") try: payload_bytes = unpaddedbase64.decode_base64(payload) - claims = json.loads(payload_bytes) - except (json.JSONDecodeError, binascii.Error): + claims = json_decoder.decode(payload_bytes.decode("utf-8")) + except (json.JSONDecodeError, binascii.Error, UnicodeError): raise SynapseError(400, "Invalid logout_token payload in request") try: - # Let's extract the iss and aud claim + # Let's extract the iss and aud claims iss = claims["iss"] aud = claims["aud"] # The aud claim can be either a string or a list of string. Here we @@ -299,15 +300,16 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: aud = [aud] # Check that we have the right types for the aud and the iss claims - assert isinstance(iss, str) - assert isinstance(aud, list) + if not isinstance(iss, str) or not isinstance(aud, list): + raise TypeError() for a in aud: - assert isinstance(a, str) + if not isinstance(a, str): + raise TypeError() # At this point we properly checked both claims types issuer: str = iss audience: List[str] = aud - except (TypeError, KeyError, AssertionError): + except (TypeError, KeyError): raise SynapseError(400, "Invalid issuer/audience in logout_token") # Now that we know the audience and the issuer, we can figure out from @@ -323,7 +325,7 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: if oidc_provider is None: raise SynapseError(400, "Could not find the OP that issued this event") - # We now figured out what provider should handle the logout request + # Ask the provider to handle the logout request. await oidc_provider.handle_backchannel_logout(request, logout_token) @@ -488,7 +490,7 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: self.issuer, ) - if not m.get("backchannel_logout_session_supported", False): + elif not m.get("backchannel_logout_session_supported", False): logger.warning( "OIDC Back-Channel Logout is enabled and supported " "by issuer %r but it might not send session ID with " @@ -1231,7 +1233,7 @@ async def handle_backchannel_logout( # point the `sid` claim exists and is a string. sid: str = claims.get("sid") - # Find in the store if we have a device associated with that SID + # Fetch any device(s) in the store associated with the session ID. devices = await self._store.get_devices_by_auth_provider_session_id( auth_provider_id=self.idp_id, auth_provider_session_id=sid, @@ -1263,7 +1265,7 @@ class LogoutToken(JWTClaims): def validate(self, now: Optional[int] = None, leeway: int = 0) -> None: """Validate everything in claims payload.""" - super(LogoutToken, self).validate(now, leeway) + super().validate(now, leeway) self.validate_sid() self.validate_events() self.validate_nonce() @@ -1291,7 +1293,7 @@ def validate_nonce(self) -> None: raise InvalidClaimError("nonce") def validate_events(self) -> None: - """Ensure the events is present and with the right value""" + """Ensure the events claim is present and with the right value""" events = self.get("events") if not events: raise MissingClaimError("events") @@ -1371,6 +1373,7 @@ def _get_secret(self) -> bytes: logger.info( "Generating new JWT for %s: %s %s", self._oauth_issuer, header, payload ) + jwt = JsonWebToken(header["alg"]) self._cached_secret = jwt.encode(header, payload, self._key.key) self._cached_secret_replacement_time = ( expires_at - CLIENT_SECRET_MIN_VALIDITY_SECONDS From a5d74faf8e386158a42514d626ce126b00097038 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 7 Dec 2021 11:27:13 +0100 Subject: [PATCH 03/19] Validate `exp` and `nbf` claims in logout tokens if present Signed-off-by: Quentin Gliech --- synapse/handlers/oidc.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 5b2eda07d880..e1ed9f7d6d98 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1270,14 +1270,6 @@ def validate(self, now: Optional[int] = None, leeway: int = 0) -> None: self.validate_events() self.validate_nonce() - def validate_exp(self, now: Optional[int] = None, leeway: int = 0) -> None: - """Do not validate the exp claim""" - pass - - def validate_nbf(self, now: Optional[int] = None, leeway: int = 0) -> None: - """Do not validate the nbf claim""" - pass - def validate_sid(self) -> None: """Ensure the sid claim is present""" sid = self.get("sid") From 51f3cd8c5218e11b135d75804a020b3a644bf0b6 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 7 Dec 2021 11:31:05 +0100 Subject: [PATCH 04/19] Docstrings on issuer/client_id properties Signed-off-by: Quentin Gliech --- synapse/handlers/oidc.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index e1ed9f7d6d98..cd8bbe0a7e9f 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -515,10 +515,12 @@ def _uses_userinfo(self) -> bool: @property def issuer(self) -> str: + """The issuer identifying this provider.""" return self._config.issuer @property def client_id(self) -> str: + """The client_id used when interacting with this provider.""" return self._config.client_id async def load_metadata(self, force: bool = False) -> OpenIDProviderMetadata: From 1fd365887f8365362e45e8a02e26684621a84942 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 18:31:15 +0200 Subject: [PATCH 05/19] Misc changes Mainly adds a warning if the user_mapping_provider is not using the `sub` claim as subject Signed-off-by: Quentin Gliech --- synapse/handlers/oidc.py | 167 ++++++++++++++++++++++++--------------- 1 file changed, 104 insertions(+), 63 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index cd8bbe0a7e9f..ff0625b85093 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -16,7 +16,17 @@ import inspect import json import logging -from typing import TYPE_CHECKING, Any, Dict, Generic, List, Optional, TypeVar, Union +from typing import ( + TYPE_CHECKING, + Any, + Dict, + Generic, + List, + Optional, + Type, + TypeVar, + Union, +) from urllib.parse import urlencode, urlparse import attr @@ -95,6 +105,8 @@ class Token(TypedDict): #: there is no real point of doing this in our case. JWK = Dict[str, str] +C = TypeVar("C") + #: A JWK Set, as per RFC7517 sec 5. class JWKS(TypedDict): @@ -258,7 +270,7 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: """Handle an incoming request to /_synapse/client/oidc/backchannel_logout This extracts the logout_token from the request and tries to figure out - which account it is associated with. This works by matching the iss claim + which OpenID Provider it is comming from. This works by matching the iss claim with the issuer and the aud claim with the client_id. Since at this point we don't know who signed the JWT, we can't just @@ -268,7 +280,6 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: Args: request: the incoming request from the browser. """ - logger.warn("OHAI") logout_token = parse_string(request, "logout_token") if logout_token is None: raise SynapseError(400, "Missing logout_token in request") @@ -498,6 +509,22 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: self.issuer, ) + # If OIDC backchannel logouts are enabled, the provider mapping provider + # should use the `sub` claim. We verify that by mapping a dumb user and see + # if we get back the sub claim + user = UserInfo({"sub": "thisisasubject"}) + try: + subject = self._user_mapping_provider.get_remote_user_id(user) + if subject != user["sub"]: + raise Exception() + except Exception: + logger.warning( + "OIDC Back-Channel Logout is enabled for issuer %r but it looks " + "like the configured `user_mapping_provider` does not use the " + "`sub` claim as subject, which may be resuired for logouts to work", + self.issuer, + ) + @property def _uses_userinfo(self) -> bool: """Returns True if the ``userinfo_endpoint`` should be used. @@ -770,6 +797,59 @@ async def _fetch_userinfo(self, token: Token) -> UserInfo: return UserInfo(resp) + async def _verify_jwt( + self, + alg_values: List[str], + token: str, + claims_cls: Type[C], + claims_options: Optional[dict] = None, + claims_params: Optional[dict] = None, + ) -> C: + """Decode and validate a JWT, re-fetching the JWKS as needed. + + Args: + alg_values: list of `alg` values allowed when verifying the JWT. + token: the JWT. + claims_cls: the JWTClaims class to use to validate the claims. + claims_options: dict of options passed to the `claims_cls` constructor. + claims_params: dict of params passed to the `claims_cls` constructor. + + Returns: + The decoded claims in the JWT. + """ + jwt = JsonWebToken(alg_values) + + logger.debug("Attempting to decode JWT (%s) %r", claims_cls.__name__, token) + + # Try to decode the keys in cache first, then retry by forcing the keys + # to be reloaded + jwk_set = await self.load_jwks() + try: + claims = jwt.decode( + token, + key=jwk_set, + claims_cls=claims_cls, + claims_options=claims_options, + claims_params=claims_params, + ) + except ValueError: + logger.info("Reloading JWKS after decode error") + jwk_set = await self.load_jwks(force=True) # try reloading the jwks + claims = jwt.decode( + token, + key=jwk_set, + claims_cls=claims_cls, + claims_options=claims_options, + claims_params=claims_params, + ) + + logger.debug("Decoded JWT (%s) %r; validating", claims_cls.__name__, claims) + + claims.validate( + now=self._clock.time(), leeway=120 + ) # allows 2 min of clock skew + return claims + async def _parse_id_token(self, token: Token, nonce: str) -> CodeIDToken: """Return an instance of UserInfo from token's ``id_token``. @@ -783,13 +863,13 @@ async def _parse_id_token(self, token: Token, nonce: str) -> CodeIDToken: The decoded claims in the ID token. """ id_token = token.get("id_token") - logger.debug("Attempting to decode JWT id_token %r", id_token) # That has been theoritically been checked by the caller, so even though # assertion are not enabled in production, it is mainly here to appease mypy assert id_token is not None metadata = await self.load_metadata() + claims_params = { "nonce": nonce, "client_id": self._client_auth.client_id, @@ -799,38 +879,17 @@ async def _parse_id_token(self, token: Token, nonce: str) -> CodeIDToken: # in the `id_token` that we can check against. claims_params["access_token"] = token["access_token"] - alg_values = metadata.get("id_token_signing_alg_values_supported", ["RS256"]) - jwt = JsonWebToken(alg_values) - - claim_options = {"iss": {"values": [metadata["issuer"]]}} + claims_options = {"iss": {"values": [metadata["issuer"]]}} - # Try to decode the keys in cache first, then retry by forcing the keys - # to be reloaded - jwk_set = await self.load_jwks() - try: - claims = jwt.decode( - id_token, - key=jwk_set, - claims_cls=CodeIDToken, - claims_options=claim_options, - claims_params=claims_params, - ) - except ValueError: - logger.info("Reloading JWKS after decode error") - jwk_set = await self.load_jwks(force=True) # try reloading the jwks - claims = jwt.decode( - id_token, - key=jwk_set, - claims_cls=CodeIDToken, - claims_options=claim_options, - claims_params=claims_params, - ) - - logger.debug("Decoded id_token JWT %r; validating", claims) + alg_values = metadata.get("id_token_signing_alg_values_supported", ["RS256"]) - claims.validate( - now=self._clock.time(), leeway=120 - ) # allows 2 min of clock skew + claims = await self._verify_jwt( + alg_values=alg_values, + token=id_token, + claims_cls=CodeIDToken, + claims_options=claims_options, + claims_params=claims_params, + ) return claims @@ -1187,42 +1246,23 @@ async def handle_backchannel_logout( # iss (issuer) claim in the JWT Header Parameters, as specified in # Section 5.3 of [JWT]. alg_values = metadata.get("id_token_signing_alg_values_supported", ["RS256"]) - jwt = JsonWebToken(alg_values) # As per sec. 2.6: # 3. Validate the iss, aud, and iat Claims in the same way they are # validated in ID Tokens. # Which means the audience should contain Synapse's client_id and the # issuer should be the IdP issuer - claim_options = { + claims_options = { "iss": {"values": [self.issuer]}, "aud": {"values": [self.client_id]}, } - logger.debug("Attempting to decode JWT logout_token %r", logout_token) - - # Try to decode the keys in cache first, then retry by forcing the keys - # to be reloaded - jwk_set = await self.load_jwks() - try: - claims = jwt.decode( - logout_token, - key=jwk_set, - claims_cls=LogoutToken, - claims_options=claim_options, - ) - except ValueError: - logger.info("Reloading JWKS after decode error") - jwk_set = await self.load_jwks(force=True) # try reloading the jwks - claims = jwt.decode( - logout_token, - key=jwk_set, - claims_cls=LogoutToken, - claims_options=claim_options, - ) - - logger.debug("Decoded logout_token JWT %r; validating", claims) - claims.validate(leeway=120) # allows 2 min of clock skew + claims = await self._verify_jwt( + alg_values=alg_values, + token=logout_token, + claims_cls=LogoutToken, + claims_options=claims_options, + ) # As per sec. 2.6: # 4. Verify that the Logout Token contains a sub Claim, a sid Claim, @@ -1240,6 +1280,10 @@ async def handle_backchannel_logout( auth_provider_id=self.idp_id, auth_provider_session_id=sid, ) + + # We have no guarantee that all the devices of that session are for the same + # `user_id`. Hence, we have to iterate over the list of devices and log them out + # one by one. for device in devices: user_id = device["user_id"] device_id = device["device_id"] @@ -1382,9 +1426,6 @@ class UserAttributeDict(TypedDict): emails: List[str] -C = TypeVar("C") - - class OidcMappingProvider(Generic[C]): """A mapping provider maps a UserInfo object to user attributes. From 17580ae4667d50261ad2c33203bd8724b697aefc Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 21 Sep 2022 17:49:05 +0200 Subject: [PATCH 06/19] Check the `sub` claim when receiving a logout token --- synapse/config/oidc.py | 7 + synapse/handlers/oidc.py | 77 ++++++++--- tests/rest/client/test_auth.py | 241 ++++++++++++++++++++++++++++++++- tests/rest/client/utils.py | 27 +++- tests/test_utils/oidc.py | 27 +++- 5 files changed, 350 insertions(+), 29 deletions(-) diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index b6c35ac495a6..1cabc04b3aac 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -124,6 +124,7 @@ def oidc_enabled(self) -> bool: "jwks_uri": {"type": "string"}, "skip_verification": {"type": "boolean"}, "backchannel_logout_enabled": {"type": "boolean"}, + "backchannel_logout_ignore_sub": {"type": "boolean"}, "user_profile_method": { "type": "string", "enum": ["auto", "userinfo_endpoint"], @@ -294,6 +295,9 @@ def _parse_oidc_config_dict( userinfo_endpoint=oidc_config.get("userinfo_endpoint"), jwks_uri=oidc_config.get("jwks_uri"), backchannel_logout_enabled=oidc_config.get("backchannel_logout_enabled", False), + backchannel_logout_ignore_sub=oidc_config.get( + "backchannel_logout_ignore_sub", False + ), skip_verification=oidc_config.get("skip_verification", False), user_profile_method=oidc_config.get("user_profile_method", "auto"), allow_existing_users=oidc_config.get("allow_existing_users", False), @@ -373,6 +377,9 @@ class OidcProviderConfig: # Whether Synapse should react to backchannel logouts backchannel_logout_enabled: bool + # Whetever Synapse should ignore the `sub` claim in backchannel logouts or not + backchannel_logout_ignore_sub: bool + # Whether to skip metadata verification skip_verification: bool diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index ff0625b85093..1bd506162347 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -33,7 +33,7 @@ import unpaddedbase64 from authlib.common.security import generate_token from authlib.jose import JsonWebToken, JWTClaims -from authlib.jose.errors import InvalidClaimError, MissingClaimError +from authlib.jose.errors import InvalidClaimError, JoseError, MissingClaimError from authlib.oauth2.auth import ClientAuth from authlib.oauth2.rfc6749.parameters import prepare_grant_uri from authlib.oidc.core import CodeIDToken, UserInfo @@ -509,21 +509,24 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: self.issuer, ) - # If OIDC backchannel logouts are enabled, the provider mapping provider - # should use the `sub` claim. We verify that by mapping a dumb user and see - # if we get back the sub claim - user = UserInfo({"sub": "thisisasubject"}) - try: - subject = self._user_mapping_provider.get_remote_user_id(user) - if subject != user["sub"]: - raise Exception() - except Exception: - logger.warning( - "OIDC Back-Channel Logout is enabled for issuer %r but it looks " - "like the configured `user_mapping_provider` does not use the " - "`sub` claim as subject, which may be resuired for logouts to work", - self.issuer, - ) + if not self._config.backchannel_logout_ignore_sub: + # If OIDC backchannel logouts are enabled, the provider mapping provider + # should use the `sub` claim. We verify that by mapping a dumb user and + # see if we get back the sub claim + user = UserInfo({"sub": "thisisasubject"}) + try: + subject = self._user_mapping_provider.get_remote_user_id(user) + if subject != user["sub"]: + raise Exception() + except Exception: + logger.warning( + f"OIDC Back-Channel Logout is enabled for issuer {self.issuer!r} " + f"but it looks like the configured `user_mapping_provider` " + f"does not use the `sub` claim as subject. If it is the case, " + f"and you want Synapse to ignore the `sub` claim in OIDC " + f"Back-Channel Logouts, set `backchannel_logout_ignore_sub` " + f"to `true` in the issuer config." + ) @property def _uses_userinfo(self) -> bool: @@ -1257,12 +1260,16 @@ async def handle_backchannel_logout( "aud": {"values": [self.client_id]}, } - claims = await self._verify_jwt( - alg_values=alg_values, - token=logout_token, - claims_cls=LogoutToken, - claims_options=claims_options, - ) + try: + claims = await self._verify_jwt( + alg_values=alg_values, + token=logout_token, + claims_cls=LogoutToken, + claims_options=claims_options, + ) + except JoseError: + logger.exception("Invalid logout_token") + raise SynapseError(400, "Invalid logout_token") # As per sec. 2.6: # 4. Verify that the Logout Token contains a sub Claim, a sid Claim, @@ -1275,6 +1282,16 @@ async def handle_backchannel_logout( # point the `sid` claim exists and is a string. sid: str = claims.get("sid") + # If the `sub` claim was included in the logout token, we check that it matches + # that it matches the right user. We can have cases where the `sub` claim is not + # the ID saved in database, so we let admins disable this check in config. + sub: Optional[str] = claims.get("sub") + expected_user_id: Optional[str] = None + if sub is not None and not self._config.backchannel_logout_ignore_sub: + expected_user_id = await self._store.get_user_by_external_id( + self.idp_id, sub + ) + # Fetch any device(s) in the store associated with the session ID. devices = await self._store.get_devices_by_auth_provider_session_id( auth_provider_id=self.idp_id, @@ -1287,6 +1304,22 @@ async def handle_backchannel_logout( for device in devices: user_id = device["user_id"] device_id = device["device_id"] + + # If the user_id associated with that device/session is not the one we got + # out of the `sub` claim, skip that device and show log an error. + if expected_user_id is not None and user_id != expected_user_id: + logger.error( + f"Received an OIDC Back-Channel Logout request " + f"from issuer {self.issuer!r}, " + f"for the user {expected_user_id!r} (sub: {sub!r}), " + f"but with a session ID ({sid!r}) which belongs to {user_id!r}. " + f"This may happen when the OIDC user mapper uses something else " + f"than the `sub` claim as subject claim. " + f"Set `backchannel_logout_ignore_sub` to `true` " + f"in the provider config it that is the case." + ) + continue + logger.info( "Logging out %r (device %r) via OIDC backchannel logout (sid %r).", user_id, diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index ebf653d018f6..dd0bd95961ba 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -32,7 +32,7 @@ from tests import unittest from tests.handlers.test_oidc import HAS_OIDC -from tests.rest.client.utils import TEST_OIDC_CONFIG +from tests.rest.client.utils import TEST_OIDC_CONFIG, TEST_OIDC_ISSUER from tests.server import FakeChannel from tests.unittest import override_config, skip_unless @@ -1165,3 +1165,242 @@ def _txn(txn: LoggingTransaction) -> int: # and no refresh token self.assertEqual(_table_length("access_tokens"), 0) self.assertEqual(_table_length("refresh_tokens"), 0) + + +@skip_unless(HAS_OIDC, "Requires OIDC") +class OidcBackchannelLogoutTests(unittest.HomeserverTestCase): + servlets = [ + account.register_servlets, + login.register_servlets, + ] + + def default_config(self) -> Dict[str, Any]: + config = super().default_config() + + # public_baseurl uses an http:// scheme because FakeChannel.isSecure() returns + # False, so synapse will see the requested uri as http://..., so using http in + # the public_baseurl stops Synapse trying to redirect to https. + config["public_baseurl"] = "http://synapse.test" + + return config + + def create_resource_dict(self) -> Dict[str, Resource]: + resource_dict = super().create_resource_dict() + resource_dict.update(build_synapse_client_resource_tree(self.hs)) + return resource_dict + + def is_access_token_valid(self, access_token: str) -> bool: + """ + Checks whether an access token is valid, returning whether it is or not. + """ + code = self.make_request( + "GET", "/_matrix/client/v3/account/whoami", access_token=access_token + ).code + + # Either 200 or 401 is what we get back; anything else is a bug. + assert code in {HTTPStatus.OK, HTTPStatus.UNAUTHORIZED} + + return code == HTTPStatus.OK + + def submit_logout_token(self, logout_token: str) -> FakeChannel: + return self.make_request( + "POST", + "/_synapse/client/oidc/backchannel_logout", + content=f"logout_token={logout_token}", + content_is_form=True, + ) + + @override_config( + { + "oidc_providers": [ + { + "idp_id": "oidc", + "idp_name": "OIDC", + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": True, + } + ] + } + ) + def test_simple_logout(self) -> None: + fake_oidc_provider = self.helper.fake_oidc_server() + user = "john" + + login_resp, first_grant = self.helper.login_via_oidc( + fake_oidc_provider, user, with_sid=True + ) + first_access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(first_access_token)) + + login_resp, second_grant = self.helper.login_via_oidc( + fake_oidc_provider, user, with_sid=True + ) + second_access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(second_access_token)) + + self.assertNotEqual(first_grant.sid, second_grant.sid) + self.assertEqual(first_grant.userinfo["sub"], second_grant.userinfo["sub"]) + + # Logging out of the first session + logout_token = fake_oidc_provider.generate_logout_token(first_grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + self.assertFalse(self.is_access_token_valid(first_access_token)) + self.assertTrue(self.is_access_token_valid(second_access_token)) + + # Logging out of the second session + logout_token = fake_oidc_provider.generate_logout_token(second_grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + self.assertFalse(self.is_access_token_valid(second_access_token)) + + @override_config( + { + "oidc_providers": [ + { + "idp_id": "oidc", + "idp_name": "OIDC", + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": False, + } + ] + } + ) + def test_disabled(self) -> None: + fake_oidc_provider = self.helper.fake_oidc_server() + user = "john" + + login_resp, grant = self.helper.login_via_oidc( + fake_oidc_provider, user, with_sid=True + ) + access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(access_token)) + + # Logging out shouldn't work + logout_token = fake_oidc_provider.generate_logout_token(grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 400) + + # And the token should still be valid + self.assertTrue(self.is_access_token_valid(access_token)) + + @override_config( + { + "oidc_providers": [ + { + "idp_id": "oidc", + "idp_name": "OIDC", + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": True, + } + ] + } + ) + def test_no_sid(self) -> None: + fake_oidc_provider = self.helper.fake_oidc_server() + user = "john" + + login_resp, grant = self.helper.login_via_oidc( + fake_oidc_provider, user, with_sid=False + ) + access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(access_token)) + + # Logging out shouldn't work + logout_token = fake_oidc_provider.generate_logout_token(grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 400) + + # And the token should still be valid + self.assertTrue(self.is_access_token_valid(access_token)) + + @override_config( + { + "oidc_providers": [ + { + "idp_id": "first", + "idp_name": "First", + "issuer": "https://first-issuer.com/", + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": True, + }, + { + "idp_id": "second", + "idp_name": "Second", + "issuer": "https://second-issuer.com/", + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": True, + }, + ] + } + ) + def test_multiple_providers(self) -> None: + first_provider = self.helper.fake_oidc_server( + issuer="https://first-issuer.com/" + ) + second_provider = self.helper.fake_oidc_server( + issuer="https://second-issuer.com/" + ) + user = "john" + + login_resp, first_grant = self.helper.login_via_oidc( + first_provider, user, with_sid=True, idp_id="oidc-first" + ) + first_access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(first_access_token)) + + login_resp, second_grant = self.helper.login_via_oidc( + second_provider, user, with_sid=True, idp_id="oidc-second" + ) + second_access_token: str = login_resp["access_token"] + self.assertTrue(self.is_access_token_valid(second_access_token)) + + # `sid` in the fake providers are generated by a counter, so the first grant of + # each provider should give the same SID + self.assertEqual(first_grant.sid, second_grant.sid) + self.assertEqual(first_grant.userinfo["sub"], second_grant.userinfo["sub"]) + + # Logging out of the first session + logout_token = first_provider.generate_logout_token(first_grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + self.assertFalse(self.is_access_token_valid(first_access_token)) + self.assertTrue(self.is_access_token_valid(second_access_token)) + + # Logging out of the second session + logout_token = second_provider.generate_logout_token(second_grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + self.assertFalse(self.is_access_token_valid(second_access_token)) diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index 967d229223ab..0d5819de9b10 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -572,6 +572,7 @@ def login_via_oidc( fake_server: FakeOidcServer, remote_user_id: str, with_sid: bool = False, + idp_id: Optional[str] = None, expected_status: int = 200, ) -> Tuple[JsonDict, FakeAuthorizationGrant]: """Log in (as a new user) via OIDC @@ -588,7 +589,11 @@ def login_via_oidc( client_redirect_url = "https://x" userinfo = {"sub": remote_user_id} channel, grant = self.auth_via_oidc( - fake_server, userinfo, client_redirect_url, with_sid=with_sid + fake_server, + userinfo, + client_redirect_url, + with_sid=with_sid, + idp_id=idp_id, ) # expect a confirmation page @@ -623,6 +628,7 @@ def auth_via_oidc( client_redirect_url: Optional[str] = None, ui_auth_session_id: Optional[str] = None, with_sid: bool = False, + idp_id: Optional[str] = None, ) -> Tuple[FakeChannel, FakeAuthorizationGrant]: """Perform an OIDC authentication flow via a mock OIDC provider. @@ -648,6 +654,7 @@ def auth_via_oidc( ui_auth_session_id: if set, we will perform a UI Auth flow. The session id of the UI auth. with_sid: if True, generates a random `sid` (OIDC session ID) + idp_id: if set, explicitely chooses one specific IDP Returns: A FakeChannel containing the result of calling the OIDC callback endpoint. @@ -665,7 +672,9 @@ def auth_via_oidc( oauth_uri = self.initiate_sso_ui_auth(ui_auth_session_id, cookies) else: # otherwise, hit the login redirect endpoint - oauth_uri = self.initiate_sso_login(client_redirect_url, cookies) + oauth_uri = self.initiate_sso_login( + client_redirect_url, cookies, idp_id=idp_id + ) # we now have a URI for the OIDC IdP, but we skip that and go straight # back to synapse's OIDC callback resource. However, we do need the "state" @@ -742,7 +751,10 @@ def complete_oidc_auth( return channel, grant def initiate_sso_login( - self, client_redirect_url: Optional[str], cookies: MutableMapping[str, str] + self, + client_redirect_url: Optional[str], + cookies: MutableMapping[str, str], + idp_id: Optional[str] = None, ) -> str: """Make a request to the login-via-sso redirect endpoint, and return the target @@ -753,6 +765,7 @@ def initiate_sso_login( client_redirect_url: the client redirect URL to pass to the login redirect endpoint cookies: any cookies returned will be added to this dict + idp_id: if set, explicitely chooses one specific IDP Returns: the URI that the client gets redirected to (ie, the SSO server) @@ -761,6 +774,12 @@ def initiate_sso_login( if client_redirect_url: params["redirectUrl"] = client_redirect_url + uri = "/_matrix/client/r0/login/sso/redirect" + if idp_id is not None: + uri = f"{uri}/{idp_id}" + + uri = f"{uri}?{urllib.parse.urlencode(params)}" + # hit the redirect url (which should redirect back to the redirect url. This # is the easiest way of figuring out what the Host header ought to be set to # to keep Synapse happy. @@ -768,7 +787,7 @@ def initiate_sso_login( self.hs.get_reactor(), self.site, "GET", - "/_matrix/client/r0/login/sso/redirect?" + urllib.parse.urlencode(params), + uri, ) assert channel.code == 302 diff --git a/tests/test_utils/oidc.py b/tests/test_utils/oidc.py index de134bbc893b..1461d23ee823 100644 --- a/tests/test_utils/oidc.py +++ b/tests/test_utils/oidc.py @@ -51,6 +51,8 @@ class FakeOidcServer: get_userinfo_handler: Mock post_token_handler: Mock + sid_counter: int = 0 + def __init__(self, clock: Clock, issuer: str): from authlib.jose import ECKey, KeySet @@ -146,7 +148,7 @@ def _sign(self, payload: dict) -> str: return jws.serialize_compact(protected, json_payload, self._key).decode("utf-8") def generate_id_token(self, grant: FakeAuthorizationGrant) -> str: - now = self._clock.time() + now = int(self._clock.time()) id_token = { **grant.userinfo, "iss": self.issuer, @@ -166,6 +168,26 @@ def generate_id_token(self, grant: FakeAuthorizationGrant) -> str: return self._sign(id_token) + def generate_logout_token(self, grant: FakeAuthorizationGrant) -> str: + now = int(self._clock.time()) + logout_token = { + "iss": self.issuer, + "aud": grant.client_id, + "iat": now, + "jti": random_string(10), + "events": { + "http://schemas.openid.net/event/backchannel-logout": {}, + }, + } + + if grant.sid is not None: + logout_token["sid"] = grant.sid + + if "sub" in grant.userinfo: + logout_token["sub"] = grant.userinfo["sub"] + + return self._sign(logout_token) + def id_token_override(self, overrides: dict): """Temporarily patch the ID token generated by the token endpoint.""" return patch.object(self, "_id_token_overrides", overrides) @@ -183,7 +205,8 @@ def start_authorization( code = random_string(10) sid = None if with_sid: - sid = random_string(10) + sid = str(self.sid_counter) + self.sid_counter += 1 grant = FakeAuthorizationGrant( userinfo=userinfo, From 42df126e8e6da343f7e37ca8f7b9289a1f42ee5d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 26 Oct 2022 13:52:22 +0200 Subject: [PATCH 07/19] Revoke in-flight logins --- synapse/handlers/oidc.py | 12 ++ synapse/handlers/sso.py | 26 +++ .../storage/databases/main/registration.py | 21 +++ tests/rest/client/test_auth.py | 174 +++++++++++++++--- 4 files changed, 211 insertions(+), 22 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 1bd506162347..d839bf5f3cc3 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1292,6 +1292,18 @@ async def handle_backchannel_logout( self.idp_id, sub ) + # Invalidate any running user-mapping sessions + self._sso_handler.revoke_mapping_sessions_for_provider_session_id( + auth_provider_id=self.idp_id, + auth_provider_session_id=sid, + ) + + # Invalidate any in-flight login tokens + await self._store.invalidate_login_tokens_by_session_id( + auth_provider_id=self.idp_id, + auth_provider_session_id=sid, + ) + # Fetch any device(s) in the store associated with the session ID. devices = await self._store.get_devices_by_auth_provider_session_id( auth_provider_id=self.idp_id, diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 5943f08e916e..828e63b6d3f4 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -790,6 +790,32 @@ def get_mapping_session(self, session_id: str) -> UsernameMappingSession: logger.info("Couldn't find session id %s", session_id) raise SynapseError(400, "unknown session") + def revoke_mapping_sessions_for_provider_session_id( + self, auth_provider_id: str, auth_provider_session_id: str + ) -> None: + """Revoke username mapping sessions with the given IdP session ID. + + This is useful to revoke in-flight mapping sessions when a logout notification + arrives from the IdP. + + Args: + auth_provider_id: A unique identifier for this SSO provider, e.g. + "oidc" or "saml". + auth_provider_session_id: The session ID got during login from the SSO IdP. + """ + to_delete = [] + + for session_id, session in self._username_mapping_sessions.items(): + if ( + session.auth_provider_id == auth_provider_id + and session.auth_provider_session_id == auth_provider_session_id + ): + to_delete.append(session_id) + + for session_id in to_delete: + logger.info("Expiring mapping session %s", session_id) + del self._username_mapping_sessions[session_id] + async def check_username_availability( self, localpart: str, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 0255295317f9..5167089e03b4 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1920,6 +1920,27 @@ async def consume_login_token(self, token: str) -> LoginTokenLookupResult: self._clock.time_msec(), ) + async def invalidate_login_tokens_by_session_id( + self, auth_provider_id: str, auth_provider_session_id: str + ) -> None: + """Invalidate login tokens with the given IdP session ID. + + Args: + auth_provider_id: The SSO Identity Provider that the user authenticated with + to get this token + auth_provider_session_id: The session ID advertised by the SSO Identity + Provider + """ + await self.db_pool.simple_update( + table="login_tokens", + keyvalues={ + "auth_provider_id": auth_provider_id, + "auth_provider_session_id": auth_provider_session_id, + }, + updatevalues={"used_ts": self._clock.time_msec()}, + desc="invalidate_login_tokens_by_session_id", + ) + @cached() async def is_guest(self, user_id: str) -> bool: res = await self.db_pool.simple_select_one_onecol( diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index dd0bd95961ba..07734adc303f 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -12,6 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import re from http import HTTPStatus from typing import Any, Dict, List, Optional, Tuple, Union @@ -21,7 +22,7 @@ import synapse.rest.admin from synapse.api.constants import ApprovalNoticeMedium, LoginType -from synapse.api.errors import Codes +from synapse.api.errors import Codes, SynapseError from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker from synapse.rest.client import account, auth, devices, login, logout, register from synapse.rest.synapse.client import build_synapse_client_resource_tree @@ -33,7 +34,7 @@ from tests import unittest from tests.handlers.test_oidc import HAS_OIDC from tests.rest.client.utils import TEST_OIDC_CONFIG, TEST_OIDC_ISSUER -from tests.server import FakeChannel +from tests.server import FakeChannel, make_request from tests.unittest import override_config, skip_unless @@ -1229,17 +1230,20 @@ def submit_logout_token(self, logout_token: str) -> FakeChannel: } ) def test_simple_logout(self) -> None: - fake_oidc_provider = self.helper.fake_oidc_server() + """ + Receiving a logout token should logout the user + """ + fake_oidc_server = self.helper.fake_oidc_server() user = "john" login_resp, first_grant = self.helper.login_via_oidc( - fake_oidc_provider, user, with_sid=True + fake_oidc_server, user, with_sid=True ) first_access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(first_access_token)) login_resp, second_grant = self.helper.login_via_oidc( - fake_oidc_provider, user, with_sid=True + fake_oidc_server, user, with_sid=True ) second_access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(second_access_token)) @@ -1248,7 +1252,7 @@ def test_simple_logout(self) -> None: self.assertEqual(first_grant.userinfo["sub"], second_grant.userinfo["sub"]) # Logging out of the first session - logout_token = fake_oidc_provider.generate_logout_token(first_grant) + logout_token = fake_oidc_server.generate_logout_token(first_grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) @@ -1256,11 +1260,130 @@ def test_simple_logout(self) -> None: self.assertTrue(self.is_access_token_valid(second_access_token)) # Logging out of the second session - logout_token = fake_oidc_provider.generate_logout_token(second_grant) + logout_token = fake_oidc_server.generate_logout_token(second_grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) - self.assertFalse(self.is_access_token_valid(second_access_token)) + @override_config( + { + "oidc_providers": [ + { + "idp_id": "oidc", + "idp_name": "OIDC", + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": { + "config": {"localpart_template": "{{ user.sub }}"} + }, + "backchannel_logout_enabled": True, + } + ] + } + ) + def test_logout_during_login(self) -> None: + """ + It should revoke login tokens when receiving a logout token + """ + fake_oidc_server = self.helper.fake_oidc_server() + user = "john" + + # Get an authentication, and logout before submitting the logout token + client_redirect_url = "https://x" + userinfo = {"sub": user} + channel, grant = self.helper.auth_via_oidc( + fake_oidc_server, + userinfo, + client_redirect_url, + with_sid=True, + ) + + # expect a confirmation page + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + # fish the matrix login token out of the body of the confirmation page + m = re.search( + 'a href="%s.*loginToken=([^"]*)"' % (client_redirect_url,), + channel.text_body, + ) + assert m, channel.text_body + login_token = m.group(1) + + # Submit a logout + logout_token = fake_oidc_server.generate_logout_token(grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + # Now try to exchange the login token + channel = make_request( + self.hs.get_reactor(), + self.site, + "POST", + "/login", + content={"type": "m.login.token", "token": login_token}, + ) + # It should have failed + self.assertEqual(channel.code, 403) + + @override_config( + { + "oidc_providers": [ + { + "idp_id": "oidc", + "idp_name": "OIDC", + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + "user_mapping_provider": {"config": {}}, + "backchannel_logout_enabled": True, + } + ] + } + ) + def test_logout_during_mapping(self) -> None: + """ + It should stop ongoing user mapping session when receiving a logout token + """ + fake_oidc_server = self.helper.fake_oidc_server() + user = "john" + + # Get an authentication, and logout before submitting the logout token + client_redirect_url = "https://x" + userinfo = {"sub": user} + channel, grant = self.helper.auth_via_oidc( + fake_oidc_server, + userinfo, + client_redirect_url, + with_sid=True, + ) + + # Expect a user mapping page + self.assertEqual(channel.code, HTTPStatus.FOUND, channel.result) + + # We should have a user_mapping_session cookie + cookie_headers = channel.headers.getRawHeaders("Set-Cookie") + assert cookie_headers + cookies: Dict[str, str] = {} + for h in cookie_headers: + key, value = h.split(";")[0].split("=", maxsplit=1) + cookies[key] = value + + user_mapping_session_id = cookies["username_mapping_session"] + + # Getting that session should not raise + session = self.hs.get_sso_handler().get_mapping_session(user_mapping_session_id) + self.assertIsNotNone(session) + + # Submit a logout + logout_token = fake_oidc_server.generate_logout_token(grant) + channel = self.submit_logout_token(logout_token) + self.assertEqual(channel.code, 200) + + # Now it should raise + with self.assertRaises(SynapseError): + self.hs.get_sso_handler().get_mapping_session(user_mapping_session_id) @override_config( { @@ -1281,17 +1404,20 @@ def test_simple_logout(self) -> None: } ) def test_disabled(self) -> None: - fake_oidc_provider = self.helper.fake_oidc_server() + """ + Receiving a logout token should do nothing if it is disabled in the config + """ + fake_oidc_server = self.helper.fake_oidc_server() user = "john" login_resp, grant = self.helper.login_via_oidc( - fake_oidc_provider, user, with_sid=True + fake_oidc_server, user, with_sid=True ) access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(access_token)) # Logging out shouldn't work - logout_token = fake_oidc_provider.generate_logout_token(grant) + logout_token = fake_oidc_server.generate_logout_token(grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 400) @@ -1317,17 +1443,20 @@ def test_disabled(self) -> None: } ) def test_no_sid(self) -> None: - fake_oidc_provider = self.helper.fake_oidc_server() + """ + Receiving a logout token without `sid` during the login should do nothing + """ + fake_oidc_server = self.helper.fake_oidc_server() user = "john" login_resp, grant = self.helper.login_via_oidc( - fake_oidc_provider, user, with_sid=False + fake_oidc_server, user, with_sid=False ) access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(access_token)) # Logging out shouldn't work - logout_token = fake_oidc_provider.generate_logout_token(grant) + logout_token = fake_oidc_server.generate_logout_token(grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 400) @@ -1365,22 +1494,23 @@ def test_no_sid(self) -> None: } ) def test_multiple_providers(self) -> None: - first_provider = self.helper.fake_oidc_server( - issuer="https://first-issuer.com/" - ) - second_provider = self.helper.fake_oidc_server( + """ + It should be able to distinguish login tokens from two different IdPs + """ + first_server = self.helper.fake_oidc_server(issuer="https://first-issuer.com/") + second_server = self.helper.fake_oidc_server( issuer="https://second-issuer.com/" ) user = "john" login_resp, first_grant = self.helper.login_via_oidc( - first_provider, user, with_sid=True, idp_id="oidc-first" + first_server, user, with_sid=True, idp_id="oidc-first" ) first_access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(first_access_token)) login_resp, second_grant = self.helper.login_via_oidc( - second_provider, user, with_sid=True, idp_id="oidc-second" + second_server, user, with_sid=True, idp_id="oidc-second" ) second_access_token: str = login_resp["access_token"] self.assertTrue(self.is_access_token_valid(second_access_token)) @@ -1391,7 +1521,7 @@ def test_multiple_providers(self) -> None: self.assertEqual(first_grant.userinfo["sub"], second_grant.userinfo["sub"]) # Logging out of the first session - logout_token = first_provider.generate_logout_token(first_grant) + logout_token = first_server.generate_logout_token(first_grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) @@ -1399,7 +1529,7 @@ def test_multiple_providers(self) -> None: self.assertTrue(self.is_access_token_valid(second_access_token)) # Logging out of the second session - logout_token = second_provider.generate_logout_token(second_grant) + logout_token = second_server.generate_logout_token(second_grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) From 55076e1b22b9a8a3479864629ec46af19ac44894 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 26 Oct 2022 17:25:50 +0200 Subject: [PATCH 08/19] Fix unit tests on old version of Twisted There was an issue before Twisted 20.3.0, where request.args would not be filled if the `Content-Length` header was missing. I fixed some unit tests by adding this header in `make_request`. --- tests/server.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/server.py b/tests/server.py index 8b1d18621951..b1730fcc8dd5 100644 --- a/tests/server.py +++ b/tests/server.py @@ -362,6 +362,12 @@ def make_request( # Twisted expects to be at the end of the content when parsing the request. req.content.seek(0, SEEK_END) + # Old version of Twisted (<20.3.0) have issues with parsing x-www-form-urlencoded + # bodies if the Content-Length header is missing + req.requestHeaders.addRawHeader( + b"Content-Length", str(len(content)).encode("ascii") + ) + if access_token: req.requestHeaders.addRawHeader( b"Authorization", b"Bearer " + access_token.encode("ascii") From eb7537ab220ab6abce1ff97d285d7130ca8a836d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 26 Oct 2022 18:04:47 +0200 Subject: [PATCH 09/19] Docs --- docs/openid.md | 14 ++++++++++++++ docs/usage/configuration/config_documentation.md | 6 ++++++ 2 files changed, 20 insertions(+) diff --git a/docs/openid.md b/docs/openid.md index 87ebea4c296f..37c5eb244da6 100644 --- a/docs/openid.md +++ b/docs/openid.md @@ -49,6 +49,13 @@ setting in your configuration file. See the [configuration manual](usage/configuration/config_documentation.md#oidc_providers) for some sample settings, as well as the text below for example configurations for specific providers. +## OIDC Back-Channel Logout + +Synapse supports receiving [OpenID Connect Back-Channel Logout](https://openid.net/specs/openid-connect-backchannel-1_0.html) notifications. + +This lets the OpenID Connect Provider notify Synapse when a user logs out, so that Synapse can end that user session. +This feature can be enabled by setting the `backchannel_logout_enabled` property to `true` in the provider configuration, and setting the following URL as destination for Back-Channel Logout notifications in your OpenID Connect Provider: `[synapse public baseurl]/_synapse/client/oidc/backchannel_logout` + ## Sample configs Here are a few configs for providers that should work with Synapse. @@ -123,6 +130,9 @@ oidc_providers: [Keycloak][keycloak-idp] is an opensource IdP maintained by Red Hat. +Keycloak supports OIDC Back-Channel Logout, which sends logout notification to Synapse, so that Synapse users get logged out when they log out from Keycloak. +This can be optionally enabled by setting `backchannel_logout_enabled` to `true` in the Synapse configuration, and by setting the "Backchannel Logout URL" in Keycloak. + Follow the [Getting Started Guide](https://www.keycloak.org/getting-started) to install Keycloak and set up a realm. 1. Click `Clients` in the sidebar and click `Create` @@ -144,6 +154,8 @@ Follow the [Getting Started Guide](https://www.keycloak.org/getting-started) to | Client Protocol | `openid-connect` | | Access Type | `confidential` | | Valid Redirect URIs | `[synapse public baseurl]/_synapse/client/oidc/callback` | +| Backchannel Logout URL (optional) | `[synapse public baseurl]/_synapse/client/oidc/backchannel_logout` | +| Backchannel Logout Session Required (optional) | `On` | 5. Click `Save` 6. On the Credentials tab, update the fields: @@ -167,7 +179,9 @@ oidc_providers: config: localpart_template: "{{ user.preferred_username }}" display_name_template: "{{ user.name }}" + backchannel_logout_enabled: true # Optional ``` + ### Auth0 [Auth0][auth0] is a hosted SaaS IdP solution. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index d81eda52c156..26f6aa6b381d 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3014,6 +3014,12 @@ Options for each entry include: which is set to the claims returned by the UserInfo Endpoint and/or in the ID Token. +* `backchannel_logout_enabled`: set to true to process OIDC Back-Channel Logout notifications. + Those notifications are expected to be received on `/_synapse/client/oidc/backchannel_logout`. + +* `backchannel_logout_ignore_sub`: by default, the OIDC Back-Channel Logout feature checks that the + `sub` claim matches the subject claim received during login. This check can be disabled by setting + this to `false`. It is possible to configure Synapse to only allow logins if certain attributes match particular values in the OIDC userinfo. The requirements can be listed under From d19bc836f6548748cbf55fb1aa52f0441cf65bdf Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 Oct 2022 17:31:22 +0200 Subject: [PATCH 10/19] Apply suggestions from code review Co-authored-by: Patrick Cloke --- synapse/config/oidc.py | 2 +- synapse/handlers/sso.py | 2 +- synapse/rest/synapse/client/oidc/backchannel_logout_resource.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 1cabc04b3aac..0bd83f40100b 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -377,7 +377,7 @@ class OidcProviderConfig: # Whether Synapse should react to backchannel logouts backchannel_logout_enabled: bool - # Whetever Synapse should ignore the `sub` claim in backchannel logouts or not + # Whether Synapse should ignore the `sub` claim in backchannel logouts or not. backchannel_logout_ignore_sub: bool # Whether to skip metadata verification diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 828e63b6d3f4..a22eea7b2f74 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -813,7 +813,7 @@ def revoke_mapping_sessions_for_provider_session_id( to_delete.append(session_id) for session_id in to_delete: - logger.info("Expiring mapping session %s", session_id) + logger.info("Revoking mapping session %s", session_id) del self._username_mapping_sessions[session_id] async def check_username_availability( diff --git a/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py b/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py index 2a14f6f36dbf..e07e76855a1f 100644 --- a/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py +++ b/synapse/rest/synapse/client/oidc/backchannel_logout_resource.py @@ -1,4 +1,4 @@ -# Copyright 2021 The Matrix.org Foundation C.I.C. +# Copyright 2022 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 0f22917d5f185df0b9e3fb7f738c028208790c90 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 Oct 2022 17:33:56 +0200 Subject: [PATCH 11/19] Docs --- docs/usage/configuration/config_documentation.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 26f6aa6b381d..8277dd280ebf 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3014,12 +3014,13 @@ Options for each entry include: which is set to the claims returned by the UserInfo Endpoint and/or in the ID Token. -* `backchannel_logout_enabled`: set to true to process OIDC Back-Channel Logout notifications. +* `backchannel_logout_enabled`: set to `true` to process OIDC Back-Channel Logout notifications. Those notifications are expected to be received on `/_synapse/client/oidc/backchannel_logout`. + Defaults to `false`. * `backchannel_logout_ignore_sub`: by default, the OIDC Back-Channel Logout feature checks that the `sub` claim matches the subject claim received during login. This check can be disabled by setting - this to `false`. + this to `true`. Defaults to `false`. It is possible to configure Synapse to only allow logins if certain attributes match particular values in the OIDC userinfo. The requirements can be listed under From 99f58d015f729dccbf577df3ab29bf5ef515b095 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 28 Oct 2022 21:19:29 +0200 Subject: [PATCH 12/19] Apply suggestions from code review Co-authored-by: Patrick Cloke --- docs/usage/configuration/config_documentation.md | 2 ++ synapse/handlers/oidc.py | 16 +++++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 8277dd280ebf..ef86fdffcfae 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3022,6 +3022,8 @@ Options for each entry include: `sub` claim matches the subject claim received during login. This check can be disabled by setting this to `true`. Defaults to `false`. + You might want to disable this if the `subject_claim` returned by the mapping provider is not `sub`. + It is possible to configure Synapse to only allow logins if certain attributes match particular values in the OIDC userinfo. The requirements can be listed under `attribute_requirements` as shown here: diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index d839bf5f3cc3..10b0c3ad141f 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -327,9 +327,7 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: # what provider it is coming from oidc_provider: Optional[OidcProvider] = None for provider in self._providers.values(): - if provider.issuer == issuer and any( - a == provider.client_id for a in audience - ): + if provider.issuer == issuer and provider.client_id in audience: oidc_provider = provider break @@ -504,7 +502,7 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: elif not m.get("backchannel_logout_session_supported", False): logger.warning( "OIDC Back-Channel Logout is enabled and supported " - "by issuer %r but it might not send session ID with " + "by issuer %r but it might not send a session ID with " "logout tokens, which is required for the logouts to work", self.issuer, ) @@ -521,11 +519,11 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: except Exception: logger.warning( f"OIDC Back-Channel Logout is enabled for issuer {self.issuer!r} " - f"but it looks like the configured `user_mapping_provider` " - f"does not use the `sub` claim as subject. If it is the case, " - f"and you want Synapse to ignore the `sub` claim in OIDC " - f"Back-Channel Logouts, set `backchannel_logout_ignore_sub` " - f"to `true` in the issuer config." + "but it looks like the configured `user_mapping_provider` " + "does not use the `sub` claim as subject. If it is the case, " + "and you want Synapse to ignore the `sub` claim in OIDC " + "Back-Channel Logouts, set `backchannel_logout_ignore_sub` " + "to `true` in the issuer config." ) @property From 48777f3d2bb632e995525ed2120531a209ace5b7 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 09:37:32 +0100 Subject: [PATCH 13/19] Refactor OIDC backchannel logout test config --- tests/rest/client/test_auth.py | 155 ++++++++++++++++----------------- 1 file changed, 73 insertions(+), 82 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 07734adc303f..d322fcc8f60a 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -1168,6 +1168,42 @@ def _txn(txn: LoggingTransaction) -> int: self.assertEqual(_table_length("refresh_tokens"), 0) +def oidc_config( + id: str, with_localpart_template: bool, **kwargs: Any +) -> Dict[str, Any]: + """Sample OIDC provider config used in backchannel logout tests. + + Args: + id: IDP ID for this provider + with_localpart_template: Set to `true` to have a default localpart_template in + the `user_mapping_provider` config and skip the user mapping session + **kwargs: rest of the config + + Returns: + A dict suitable for the `oidc_config` or the `oidc_providers[]` parts of + the HS config + """ + config: Dict[str, Any] = { + "idp_id": id, + "idp_name": id, + "issuer": TEST_OIDC_ISSUER, + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "scopes": ["openid"], + } + + if with_localpart_template: + config["user_mapping_provider"] = { + "config": {"localpart_template": "{{ user.sub }}"} + } + else: + config["user_mapping_provider"] = {"config": {}} + + config.update(kwargs) + + return config + + @skip_unless(HAS_OIDC, "Requires OIDC") class OidcBackchannelLogoutTests(unittest.HomeserverTestCase): servlets = [ @@ -1214,18 +1250,11 @@ def submit_logout_token(self, logout_token: str) -> FakeChannel: @override_config( { "oidc_providers": [ - { - "idp_id": "oidc", - "idp_name": "OIDC", - "issuer": TEST_OIDC_ISSUER, - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": True, - } + oidc_config( + id="oidc", + with_localpart_template=True, + backchannel_logout_enabled=True, + ) ] } ) @@ -1267,18 +1296,11 @@ def test_simple_logout(self) -> None: @override_config( { "oidc_providers": [ - { - "idp_id": "oidc", - "idp_name": "OIDC", - "issuer": TEST_OIDC_ISSUER, - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": True, - } + oidc_config( + id="oidc", + with_localpart_template=True, + backchannel_logout_enabled=True, + ) ] } ) @@ -1329,16 +1351,11 @@ def test_logout_during_login(self) -> None: @override_config( { "oidc_providers": [ - { - "idp_id": "oidc", - "idp_name": "OIDC", - "issuer": TEST_OIDC_ISSUER, - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": {"config": {}}, - "backchannel_logout_enabled": True, - } + oidc_config( + id="oidc", + with_localpart_template=False, + backchannel_logout_enabled=True, + ) ] } ) @@ -1388,18 +1405,11 @@ def test_logout_during_mapping(self) -> None: @override_config( { "oidc_providers": [ - { - "idp_id": "oidc", - "idp_name": "OIDC", - "issuer": TEST_OIDC_ISSUER, - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": False, - } + oidc_config( + id="oidc", + with_localpart_template=True, + backchannel_logout_enabled=False, + ) ] } ) @@ -1427,18 +1437,11 @@ def test_disabled(self) -> None: @override_config( { "oidc_providers": [ - { - "idp_id": "oidc", - "idp_name": "OIDC", - "issuer": TEST_OIDC_ISSUER, - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": True, - } + oidc_config( + id="oidc", + with_localpart_template=True, + backchannel_logout_enabled=True, + ) ] } ) @@ -1466,30 +1469,18 @@ def test_no_sid(self) -> None: @override_config( { "oidc_providers": [ - { - "idp_id": "first", - "idp_name": "First", - "issuer": "https://first-issuer.com/", - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": True, - }, - { - "idp_id": "second", - "idp_name": "Second", - "issuer": "https://second-issuer.com/", - "client_id": "test-client-id", - "client_secret": "test-client-secret", - "scopes": ["openid"], - "user_mapping_provider": { - "config": {"localpart_template": "{{ user.sub }}"} - }, - "backchannel_logout_enabled": True, - }, + oidc_config( + "first", + issuer="https://first-issuer.com/", + with_localpart_template=True, + backchannel_logout_enabled=True, + ), + oidc_config( + "second", + issuer="https://second-issuer.com/", + with_localpart_template=True, + backchannel_logout_enabled=True, + ), ] } ) From e3c2db61c84485c9168fd48cf25911bcede2268c Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 10:04:31 +0100 Subject: [PATCH 14/19] Move the `is_access_token_valid` method to the `RestHelper` --- tests/rest/client/test_auth.py | 76 +++++++++++++--------------------- tests/rest/client/utils.py | 28 +++++++++++++ 2 files changed, 57 insertions(+), 47 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index d322fcc8f60a..847294dc8e07 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -639,19 +639,6 @@ def use_refresh_token(self, refresh_token: str) -> FakeChannel: {"refresh_token": refresh_token}, ) - def is_access_token_valid(self, access_token: str) -> bool: - """ - Checks whether an access token is valid, returning whether it is or not. - """ - code = self.make_request( - "GET", "/_matrix/client/v3/account/whoami", access_token=access_token - ).code - - # Either 200 or 401 is what we get back; anything else is a bug. - assert code in {HTTPStatus.OK, HTTPStatus.UNAUTHORIZED} - - return code == HTTPStatus.OK - def test_login_issue_refresh_token(self) -> None: """ A login response should include a refresh_token only if asked. @@ -848,29 +835,37 @@ def test_different_expiry_for_refreshable_and_nonrefreshable_access_tokens( self.reactor.advance(59.0) # Both tokens should still be valid. - self.assertTrue(self.is_access_token_valid(refreshable_access_token)) - self.assertTrue(self.is_access_token_valid(nonrefreshable_access_token)) + self.helper.whoami(refreshable_access_token, expect_code=HTTPStatus.OK) + self.helper.whoami(nonrefreshable_access_token, expect_code=HTTPStatus.OK) # Advance to 61 s (just past 1 minute, the time of expiry) self.reactor.advance(2.0) # Only the non-refreshable token is still valid. - self.assertFalse(self.is_access_token_valid(refreshable_access_token)) - self.assertTrue(self.is_access_token_valid(nonrefreshable_access_token)) + self.helper.whoami( + refreshable_access_token, expect_code=HTTPStatus.UNAUTHORIZED + ) + self.helper.whoami(nonrefreshable_access_token, expect_code=HTTPStatus.OK) # Advance to 599 s (just shy of 10 minutes, the time of expiry) self.reactor.advance(599.0 - 61.0) # It's still the case that only the non-refreshable token is still valid. - self.assertFalse(self.is_access_token_valid(refreshable_access_token)) - self.assertTrue(self.is_access_token_valid(nonrefreshable_access_token)) + self.helper.whoami( + refreshable_access_token, expect_code=HTTPStatus.UNAUTHORIZED + ) + self.helper.whoami(nonrefreshable_access_token, expect_code=HTTPStatus.OK) # Advance to 601 s (just past 10 minutes, the time of expiry) self.reactor.advance(2.0) # Now neither token is valid. - self.assertFalse(self.is_access_token_valid(refreshable_access_token)) - self.assertFalse(self.is_access_token_valid(nonrefreshable_access_token)) + self.helper.whoami( + refreshable_access_token, expect_code=HTTPStatus.UNAUTHORIZED + ) + self.helper.whoami( + nonrefreshable_access_token, expect_code=HTTPStatus.UNAUTHORIZED + ) @override_config( {"refreshable_access_token_lifetime": "1m", "refresh_token_lifetime": "2m"} @@ -1226,19 +1221,6 @@ def create_resource_dict(self) -> Dict[str, Resource]: resource_dict.update(build_synapse_client_resource_tree(self.hs)) return resource_dict - def is_access_token_valid(self, access_token: str) -> bool: - """ - Checks whether an access token is valid, returning whether it is or not. - """ - code = self.make_request( - "GET", "/_matrix/client/v3/account/whoami", access_token=access_token - ).code - - # Either 200 or 401 is what we get back; anything else is a bug. - assert code in {HTTPStatus.OK, HTTPStatus.UNAUTHORIZED} - - return code == HTTPStatus.OK - def submit_logout_token(self, logout_token: str) -> FakeChannel: return self.make_request( "POST", @@ -1269,13 +1251,13 @@ def test_simple_logout(self) -> None: fake_oidc_server, user, with_sid=True ) first_access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(first_access_token)) + self.helper.whoami(first_access_token, expect_code=HTTPStatus.OK) login_resp, second_grant = self.helper.login_via_oidc( fake_oidc_server, user, with_sid=True ) second_access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(second_access_token)) + self.helper.whoami(second_access_token, expect_code=HTTPStatus.OK) self.assertNotEqual(first_grant.sid, second_grant.sid) self.assertEqual(first_grant.userinfo["sub"], second_grant.userinfo["sub"]) @@ -1285,8 +1267,8 @@ def test_simple_logout(self) -> None: channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) - self.assertFalse(self.is_access_token_valid(first_access_token)) - self.assertTrue(self.is_access_token_valid(second_access_token)) + self.helper.whoami(first_access_token, expect_code=HTTPStatus.UNAUTHORIZED) + self.helper.whoami(second_access_token, expect_code=HTTPStatus.OK) # Logging out of the second session logout_token = fake_oidc_server.generate_logout_token(second_grant) @@ -1424,7 +1406,7 @@ def test_disabled(self) -> None: fake_oidc_server, user, with_sid=True ) access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(access_token)) + self.helper.whoami(access_token, expect_code=HTTPStatus.OK) # Logging out shouldn't work logout_token = fake_oidc_server.generate_logout_token(grant) @@ -1432,7 +1414,7 @@ def test_disabled(self) -> None: self.assertEqual(channel.code, 400) # And the token should still be valid - self.assertTrue(self.is_access_token_valid(access_token)) + self.helper.whoami(access_token, expect_code=HTTPStatus.OK) @override_config( { @@ -1456,7 +1438,7 @@ def test_no_sid(self) -> None: fake_oidc_server, user, with_sid=False ) access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(access_token)) + self.helper.whoami(access_token, expect_code=HTTPStatus.OK) # Logging out shouldn't work logout_token = fake_oidc_server.generate_logout_token(grant) @@ -1464,7 +1446,7 @@ def test_no_sid(self) -> None: self.assertEqual(channel.code, 400) # And the token should still be valid - self.assertTrue(self.is_access_token_valid(access_token)) + self.helper.whoami(access_token, expect_code=HTTPStatus.OK) @override_config( { @@ -1498,13 +1480,13 @@ def test_multiple_providers(self) -> None: first_server, user, with_sid=True, idp_id="oidc-first" ) first_access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(first_access_token)) + self.helper.whoami(first_access_token, expect_code=HTTPStatus.OK) login_resp, second_grant = self.helper.login_via_oidc( second_server, user, with_sid=True, idp_id="oidc-second" ) second_access_token: str = login_resp["access_token"] - self.assertTrue(self.is_access_token_valid(second_access_token)) + self.helper.whoami(second_access_token, expect_code=HTTPStatus.OK) # `sid` in the fake providers are generated by a counter, so the first grant of # each provider should give the same SID @@ -1516,12 +1498,12 @@ def test_multiple_providers(self) -> None: channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) - self.assertFalse(self.is_access_token_valid(first_access_token)) - self.assertTrue(self.is_access_token_valid(second_access_token)) + self.helper.whoami(first_access_token, expect_code=HTTPStatus.UNAUTHORIZED) + self.helper.whoami(second_access_token, expect_code=HTTPStatus.OK) # Logging out of the second session logout_token = second_server.generate_logout_token(second_grant) channel = self.submit_logout_token(logout_token) self.assertEqual(channel.code, 200) - self.assertFalse(self.is_access_token_valid(second_access_token)) + self.helper.whoami(second_access_token, expect_code=HTTPStatus.UNAUTHORIZED) diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index 0d5819de9b10..706399fae548 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -553,6 +553,34 @@ def upload_media( return channel.json_body + def whoami( + self, + access_token: str, + expect_code: Literal[HTTPStatus.OK, HTTPStatus.UNAUTHORIZED] = HTTPStatus.OK, + ) -> JsonDict: + """Perform a 'whoami' request, which can be a quick way to check for access + token validity + + Args: + access_token: The user token to use during the request + expect_code: The return code to expect from attempting the whoami request + """ + channel = make_request( + self.hs.get_reactor(), + self.site, + "GET", + "account/whoami", + access_token=access_token, + ) + + assert channel.code == expect_code, "Exepcted: %d, got %d, resp: %r" % ( + expect_code, + channel.code, + channel.result["body"], + ) + + return channel.json_body + def fake_oidc_server(self, issuer: str = TEST_OIDC_ISSUER) -> FakeOidcServer: """Create a ``FakeOidcServer``. From 9cc5d401ec4ab69e792fe5f946132c16f04d5e02 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 10:12:54 +0100 Subject: [PATCH 15/19] Clarification around logout token verification --- synapse/handlers/oidc.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 10b0c3ad141f..bc19364220e0 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -275,7 +275,9 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: Since at this point we don't know who signed the JWT, we can't just decode it using authlib since it will always verifies the signature. We - have to decode it manually without validating the signature. + have to decode it manually without validating the signature. The actual JWT + verification is done in the `OidcProvider.handler_backchannel_logout` method, + once we figured out which provider sent the request. Args: request: the incoming request from the browser. From c0b498664a57268237cd2937f223e227eb5cf2bc Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 10:17:51 +0100 Subject: [PATCH 16/19] Clarify the string split while manually parsing the JWT --- synapse/handlers/oidc.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index bc19364220e0..284da5c5754f 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -292,8 +292,10 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: # The aud and iss claims we care about are in the payload part, which # is a JSON object. try: - # This raises if there are too many or not enough segments in the token - _, payload, _ = logout_token.rsplit(".", 4) + # By splitting a maximum of 3 times and destructuring the resulting array, + # we ensure that we have exactly 3 segments, while avoiding doing + # unnecessary splits. + _, payload, _ = logout_token.rsplit(".", 3) except ValueError: raise SynapseError(400, "Invalid logout_token in request") From 6deef8d412073427c208f7abc18ad756dd0f1afa Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 11:01:47 +0100 Subject: [PATCH 17/19] Move the IDP session revocation logic to the SSO handler --- synapse/handlers/oidc.py | 48 ++------------------ synapse/handlers/sso.py | 97 +++++++++++++++++++++++++++++----------- 2 files changed, 75 insertions(+), 70 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 284da5c5754f..a6ee2d25b81b 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1294,54 +1294,14 @@ async def handle_backchannel_logout( self.idp_id, sub ) - # Invalidate any running user-mapping sessions - self._sso_handler.revoke_mapping_sessions_for_provider_session_id( + # Invalidate any running user-mapping sessions, in-flight login tokens and + # active devices + await self._sso_handler.revoke_sessions_for_provider_session_id( auth_provider_id=self.idp_id, auth_provider_session_id=sid, + expected_user_id=expected_user_id, ) - # Invalidate any in-flight login tokens - await self._store.invalidate_login_tokens_by_session_id( - auth_provider_id=self.idp_id, - auth_provider_session_id=sid, - ) - - # Fetch any device(s) in the store associated with the session ID. - devices = await self._store.get_devices_by_auth_provider_session_id( - auth_provider_id=self.idp_id, - auth_provider_session_id=sid, - ) - - # We have no guarantee that all the devices of that session are for the same - # `user_id`. Hence, we have to iterate over the list of devices and log them out - # one by one. - for device in devices: - user_id = device["user_id"] - device_id = device["device_id"] - - # If the user_id associated with that device/session is not the one we got - # out of the `sub` claim, skip that device and show log an error. - if expected_user_id is not None and user_id != expected_user_id: - logger.error( - f"Received an OIDC Back-Channel Logout request " - f"from issuer {self.issuer!r}, " - f"for the user {expected_user_id!r} (sub: {sub!r}), " - f"but with a session ID ({sid!r}) which belongs to {user_id!r}. " - f"This may happen when the OIDC user mapper uses something else " - f"than the `sub` claim as subject claim. " - f"Set `backchannel_logout_ignore_sub` to `true` " - f"in the provider config it that is the case." - ) - continue - - logger.info( - "Logging out %r (device %r) via OIDC backchannel logout (sid %r).", - user_id, - device_id, - sid, - ) - await self._device_handler.delete_devices(user_id, [device_id]) - request.setResponseCode(200) request.setHeader(b"Cache-Control", b"no-cache, no-store") request.setHeader(b"Pragma", b"no-cache") diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index a22eea7b2f74..749d7e93b0f6 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -191,6 +191,7 @@ def __init__(self, hs: "HomeServer"): self._server_name = hs.hostname self._registration_handler = hs.get_registration_handler() self._auth_handler = hs.get_auth_handler() + self._device_handler = hs.get_device_handler() self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() @@ -790,32 +791,6 @@ def get_mapping_session(self, session_id: str) -> UsernameMappingSession: logger.info("Couldn't find session id %s", session_id) raise SynapseError(400, "unknown session") - def revoke_mapping_sessions_for_provider_session_id( - self, auth_provider_id: str, auth_provider_session_id: str - ) -> None: - """Revoke username mapping sessions with the given IdP session ID. - - This is useful to revoke in-flight mapping sessions when a logout notification - arrives from the IdP. - - Args: - auth_provider_id: A unique identifier for this SSO provider, e.g. - "oidc" or "saml". - auth_provider_session_id: The session ID got during login from the SSO IdP. - """ - to_delete = [] - - for session_id, session in self._username_mapping_sessions.items(): - if ( - session.auth_provider_id == auth_provider_id - and session.auth_provider_session_id == auth_provider_session_id - ): - to_delete.append(session_id) - - for session_id in to_delete: - logger.info("Revoking mapping session %s", session_id) - del self._username_mapping_sessions[session_id] - async def check_username_availability( self, localpart: str, @@ -1052,6 +1027,76 @@ def check_required_attributes( return True + async def revoke_sessions_for_provider_session_id( + self, + auth_provider_id: str, + auth_provider_session_id: str, + expected_user_id: Optional[str] = None, + ) -> None: + """Revoke any devices and in-flight logins tied to a provider session. + + Args: + auth_provider_id: A unique identifier for this SSO provider, e.g. + "oidc" or "saml". + auth_provider_session_id: The session ID from the provider to logout + expected_user_id: The user we're expecting to logout. If set, it will ignore + sessions belonging to other users and log an error. + """ + # Invalidate any running user-mapping sessions + to_delete = [] + for session_id, session in self._username_mapping_sessions.items(): + if ( + session.auth_provider_id == auth_provider_id + and session.auth_provider_session_id == auth_provider_session_id + ): + to_delete.append(session_id) + + for session_id in to_delete: + logger.info("Revoking mapping session %s", session_id) + del self._username_mapping_sessions[session_id] + + # Invalidate any in-flight login tokens + await self._store.invalidate_login_tokens_by_session_id( + auth_provider_id=auth_provider_id, + auth_provider_session_id=auth_provider_session_id, + ) + + # Fetch any device(s) in the store associated with the session ID. + devices = await self._store.get_devices_by_auth_provider_session_id( + auth_provider_id=auth_provider_id, + auth_provider_session_id=auth_provider_session_id, + ) + + # We have no guarantee that all the devices of that session are for the same + # `user_id`. Hence, we have to iterate over the list of devices and log them out + # one by one. + for device in devices: + user_id = device["user_id"] + device_id = device["device_id"] + + # If the user_id associated with that device/session is not the one we got + # out of the `sub` claim, skip that device and show log an error. + if expected_user_id is not None and user_id != expected_user_id: + logger.error( + "Received a logout notification from SSO provider " + f"{auth_provider_id!r} for the user {expected_user_id!r}, but with " + f"a session ID ({auth_provider_session_id!r}) which belongs to " + f"{user_id!r}. This may happen when the SSO provider user mapper " + "uses something else than the standard attribute as mapping ID. " + "For OIDC providers, set `backchannel_logout_ignore_sub` to `true` " + "in the provider config if that is the case." + ) + continue + + logger.info( + "Logging out %r (device %r) via SSO (%r) logout notification (session %r).", + user_id, + device_id, + auth_provider_id, + auth_provider_session_id, + ) + await self._device_handler.delete_devices(user_id, [device_id]) + def get_username_mapping_session_cookie_from_request(request: IRequest) -> str: """Extract the session ID from the cookie From e0748f60b2ebee227f7c3e49b60a400aee047830 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 11:05:43 +0100 Subject: [PATCH 18/19] Use the issuer from the metadata, not the config when verifying logout tokens --- synapse/handlers/oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index a6ee2d25b81b..4fb1cdef0a58 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1258,7 +1258,7 @@ async def handle_backchannel_logout( # Which means the audience should contain Synapse's client_id and the # issuer should be the IdP issuer claims_options = { - "iss": {"values": [self.issuer]}, + "iss": {"values": [metadata["issuer"]]}, "aud": {"values": [self.client_id]}, } From 8d3279662747e361429334e66f683d68f14e2c73 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 31 Oct 2022 16:18:38 +0100 Subject: [PATCH 19/19] Fix remaining issues raised in code review --- synapse/handlers/oidc.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 4fb1cdef0a58..867973dcca4a 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -292,10 +292,9 @@ async def handle_backchannel_logout(self, request: SynapseRequest) -> None: # The aud and iss claims we care about are in the payload part, which # is a JSON object. try: - # By splitting a maximum of 3 times and destructuring the resulting array, - # we ensure that we have exactly 3 segments, while avoiding doing - # unnecessary splits. - _, payload, _ = logout_token.rsplit(".", 3) + # By destructuring the list after splitting, we ensure that we have + # exactly 3 segments + _, payload, _ = logout_token.split(".") except ValueError: raise SynapseError(400, "Invalid logout_token in request") @@ -519,7 +518,7 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: try: subject = self._user_mapping_provider.get_remote_user_id(user) if subject != user["sub"]: - raise Exception() + raise ValueError("Unexpected subject") except Exception: logger.warning( f"OIDC Back-Channel Logout is enabled for issuer {self.issuer!r} " @@ -1238,6 +1237,12 @@ async def handle_backchannel_logout( logger.warning( f"Received an OIDC Back-Channel Logout request from issuer {self.issuer!r} but it is disabled in config" ) + + # TODO: this responds with a 400 status code, which is what the OIDC + # Back-Channel Logout spec expects, but spec also suggests answering with + # a JSON object, with the `error` and `error_description` fields set, which + # we are not doing here. + # See https://openid.net/specs/openid-connect-backchannel-1_0.html#BCResponse raise SynapseError( 400, "OpenID Connect Back-Channel Logout is disabled for this provider" )