From 32475f065daecc51d02e2588fd671ae220a35d8c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 29 Dec 2022 10:41:56 -0500 Subject: [PATCH 1/7] Support OIDC code challenges. --- .../configuration/config_documentation.md | 6 +- synapse/config/oidc.py | 17 +++ synapse/handlers/oidc.py | 47 +++++++- synapse/util/macaroons.py | 7 ++ tests/handlers/test_oidc.py | 100 ++++++++++++++++-- tests/util/test_macaroons.py | 1 + 6 files changed, 167 insertions(+), 11 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 67e0acc9104f..60f7364ca754 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3053,8 +3053,12 @@ Options for each entry include: values are `client_secret_basic` (default), `client_secret_post` and `none`. +* `code_challenge_method`: proof key for code exchange method used when requesting + and exchanging the token. Valid values are `null` (default, disables support), + `plain`, and `S256`. + * `scopes`: list of scopes to request. This should normally include the "openid" - scope. Defaults to ["openid"]. + scope. Defaults to `["openid"]`. * `authorization_endpoint`: the oauth2 authorization endpoint. Required if provider discovery is disabled. diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 0bd83f40100b..7986eb987790 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -117,6 +117,18 @@ def oidc_enabled(self) -> bool: # to avoid importing authlib here. "enum": ["client_secret_basic", "client_secret_post", "none"], }, + "code_challenge_method": { + "oneOf": [ + { + "type": "string", + # the following list is the same as the keys of + # authlib.oauth2.auth.rfc7636.challenge.CodeChallenge.SUPPORTED_CODE_CHALLENGE_METHOD. + # We inline it to avoid importing authlib here. + "enum": ["plain", "S256"], + }, + {"type": "null"}, + ], + }, "scopes": {"type": "array", "items": {"type": "string"}}, "authorization_endpoint": {"type": "string"}, "token_endpoint": {"type": "string"}, @@ -289,6 +301,7 @@ def _parse_oidc_config_dict( client_secret=oidc_config.get("client_secret"), client_secret_jwt_key=client_secret_jwt_key, client_auth_method=oidc_config.get("client_auth_method", "client_secret_basic"), + code_challenge_method=oidc_config.get("code_challenge_method"), scopes=oidc_config.get("scopes", ["openid"]), authorization_endpoint=oidc_config.get("authorization_endpoint"), token_endpoint=oidc_config.get("token_endpoint"), @@ -357,6 +370,10 @@ class OidcProviderConfig: # 'none'. client_auth_method: str + # code challenge method to use when exchanging the authorization & token. + # Valid values are None (to disable use of code challenges), 'plain', and 'S256'. + code_challenge_method: Optional[str] + # list of scopes to request scopes: Collection[str] diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 03de6a4ba637..328d57c10296 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -36,6 +36,7 @@ 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.oauth2.rfc7636.challenge import create_s256_code_challenge from authlib.oidc.core import CodeIDToken, UserInfo from authlib.oidc.discovery import OpenIDProviderMetadata, get_well_known_url from jinja2 import Environment, Template @@ -403,6 +404,7 @@ def __init__( provider.client_auth_method, ) self._client_auth_method = provider.client_auth_method + self._code_challenge_method = provider.code_challenge_method # cache of metadata for the identity provider (endpoint uris, mostly). This is # loaded on-demand from the discovery endpoint (if discovery is enabled), with @@ -475,6 +477,20 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: ) ) + # Validate code_challenge_methods_supported only if it is enabled. + if ( + m.get("code_challenge_methods_supported") is not None + and self._code_challenge_method is not None + ): + m.validate_code_challenge_methods_supported() + if self._code_challenge_method not in m["code_challenge_methods_supported"]: + raise ValueError( + '"{code_challenge_method}" not in "code_challenge_methods_supported" ({supported!r})'.format( + code_challenge_method=self._code_challenge_method, + supported=m["code_challenge_methods_supported"], + ) + ) + if m.get("response_types_supported") is not None: m.validate_response_types_supported() @@ -653,7 +669,7 @@ async def _load_jwks(self) -> JWKS: return jwk_set - async def _exchange_code(self, code: str) -> Token: + async def _exchange_code(self, code: str, code_verifier: str) -> Token: """Exchange an authorization code for a token. This calls the ``token_endpoint`` with the authorization code we @@ -666,6 +682,7 @@ async def _exchange_code(self, code: str) -> Token: Args: code: The authorization code we got from the callback. + code_verifier: The code verifier to send, blank if unused. Returns: A dict containing various tokens. @@ -696,6 +713,8 @@ async def _exchange_code(self, code: str) -> Token: "code": code, "redirect_uri": self._callback_url, } + if code_verifier: + args["code_verifier"] = code_verifier body = urlencode(args, True) # Fill the body/headers with credentials @@ -935,10 +954,30 @@ async def handle_redirect_request( state = generate_token() nonce = generate_token() + code_verifier = "" if not client_redirect_url: client_redirect_url = b"" + # If code challenge is supported and configured, use it. + extra_grant_values = {} + if self._code_challenge_method is not None: + code_verifier = generate_token(48) + + # Default to the values for plain. + extra_grant_values = {"code_challenge_method": self._code_challenge_method} + + if self._code_challenge_method == "S256": + extra_grant_values["code_challenge"] = create_s256_code_challenge( + code_verifier + ) + elif self._code_challenge_method == "plain": + extra_grant_values["code_challenge"] = code_verifier + else: + raise RuntimeError( + f"Unexpeced code challenege method: {self._code_challenge_method!r}" + ) + cookie = self._macaroon_generaton.generate_oidc_session_token( state=state, session_data=OidcSessionData( @@ -946,6 +985,7 @@ async def handle_redirect_request( nonce=nonce, client_redirect_url=client_redirect_url.decode(), ui_auth_session_id=ui_auth_session_id or "", + code_verifier=code_verifier, ), ) @@ -976,6 +1016,7 @@ async def handle_redirect_request( scope=self._scopes, state=state, nonce=nonce, + **extra_grant_values, ) async def handle_oidc_callback( @@ -1003,7 +1044,9 @@ async def handle_oidc_callback( # Exchange the code with the provider try: logger.debug("Exchanging OAuth2 code for a token") - token = await self._exchange_code(code) + token = await self._exchange_code( + code, code_verifier=session_data.code_verifier + ) except OidcError as e: logger.warning("Could not exchange OAuth2 code: %s", e) self._sso_handler.render_error(request, e.error, e.error_description) diff --git a/synapse/util/macaroons.py b/synapse/util/macaroons.py index 5df03d3ddcdd..0a2c8b72f2fb 100644 --- a/synapse/util/macaroons.py +++ b/synapse/util/macaroons.py @@ -110,6 +110,9 @@ class OidcSessionData: ui_auth_session_id: str """The session ID of the ongoing UI Auth ("" if this is a login)""" + code_verifier: str + """The random string used in code challenges ("" if code challenges are not used).""" + class MacaroonGenerator: def __init__(self, clock: Clock, location: str, secret_key: bytes): @@ -187,6 +190,7 @@ def generate_oidc_session_token( macaroon.add_first_party_caveat( f"ui_auth_session_id = {session_data.ui_auth_session_id}" ) + macaroon.add_first_party_caveat(f"code_verifier = {session_data.code_verifier}") macaroon.add_first_party_caveat(f"time < {expiry}") return macaroon.serialize() @@ -278,6 +282,7 @@ def verify_oidc_session_token(self, session: bytes, state: str) -> OidcSessionDa v.satisfy_general(lambda c: c.startswith("idp_id = ")) v.satisfy_general(lambda c: c.startswith("client_redirect_url = ")) v.satisfy_general(lambda c: c.startswith("ui_auth_session_id = ")) + v.satisfy_general(lambda c: c.startswith("code_verifier = ")) satisfy_expiry(v, self._clock.time_msec) v.verify(macaroon, self._secret_key) @@ -287,11 +292,13 @@ def verify_oidc_session_token(self, session: bytes, state: str) -> OidcSessionDa idp_id = get_value_from_macaroon(macaroon, "idp_id") client_redirect_url = get_value_from_macaroon(macaroon, "client_redirect_url") ui_auth_session_id = get_value_from_macaroon(macaroon, "ui_auth_session_id") + code_verifier = get_value_from_macaroon(macaroon, "code_verifier") return OidcSessionData( nonce=nonce, idp_id=idp_id, client_redirect_url=client_redirect_url, ui_auth_session_id=ui_auth_session_id, + code_verifier=code_verifier, ) def _generate_base_macaroon(self, type: MacaroonType) -> pymacaroons.Macaroon: diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 49a1842b5ced..de788861a5a5 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -396,6 +396,7 @@ def test_redirect_request(self) -> None: self.assertEqual(params["client_id"], [CLIENT_ID]) self.assertEqual(len(params["state"]), 1) self.assertEqual(len(params["nonce"]), 1) + self.assertNotIn("code_challenge", params) # Check what is in the cookies self.assertEqual(len(req.cookies), 2) # two cookies @@ -411,10 +412,63 @@ def test_redirect_request(self) -> None: macaroon = pymacaroons.Macaroon.deserialize(cookie) state = get_value_from_macaroon(macaroon, "state") nonce = get_value_from_macaroon(macaroon, "nonce") + code_verifier = get_value_from_macaroon(macaroon, "code_verifier") redirect = get_value_from_macaroon(macaroon, "client_redirect_url") self.assertEqual(params["state"], [state]) self.assertEqual(params["nonce"], [nonce]) + self.assertEqual(code_verifier, "") + self.assertEqual(redirect, "http://client/redirect") + + @override_config( + {"oidc_config": {**DEFAULT_CONFIG, "code_challenge_method": "S256"}} + ) + def test_redirect_request_with_code_challenge(self) -> None: + """The redirect request has the right arguments & generates a valid session cookie.""" + req = Mock(spec=["cookies"]) + req.cookies = [] + + url = urlparse( + self.get_success( + self.provider.handle_redirect_request(req, b"http://client/redirect") + ) + ) + auth_endpoint = urlparse(self.fake_server.authorization_endpoint) + + self.assertEqual(url.scheme, auth_endpoint.scheme) + self.assertEqual(url.netloc, auth_endpoint.netloc) + self.assertEqual(url.path, auth_endpoint.path) + + params = parse_qs(url.query) + self.assertEqual(params["redirect_uri"], [CALLBACK_URL]) + self.assertEqual(params["response_type"], ["code"]) + self.assertEqual(params["scope"], [" ".join(SCOPES)]) + self.assertEqual(params["client_id"], [CLIENT_ID]) + self.assertEqual(len(params["state"]), 1) + self.assertEqual(len(params["nonce"]), 1) + self.assertEqual(len(params["code_challenge"]), 1) + + # Check what is in the cookies + self.assertEqual(len(req.cookies), 2) # two cookies + cookie_header = req.cookies[0] + + # The cookie name and path don't really matter, just that it has to be coherent + # between the callback & redirect handlers. + parts = [p.strip() for p in cookie_header.split(b";")] + self.assertIn(b"Path=/_synapse/client/oidc", parts) + name, cookie = parts[0].split(b"=") + self.assertEqual(name, b"oidc_session") + + macaroon = pymacaroons.Macaroon.deserialize(cookie) + state = get_value_from_macaroon(macaroon, "state") + nonce = get_value_from_macaroon(macaroon, "nonce") + code_verifier = get_value_from_macaroon(macaroon, "code_verifier") + redirect = get_value_from_macaroon(macaroon, "client_redirect_url") + + self.assertEqual(params["state"], [state]) + self.assertEqual(params["nonce"], [nonce]) + # Ensure the code verifier is not blank. + self.assertNotEqual(code_verifier, "") self.assertEqual(redirect, "http://client/redirect") @override_config({"oidc_config": DEFAULT_CONFIG}) @@ -601,7 +655,25 @@ def test_exchange_code(self) -> None: payload=token ) code = "code" - ret = self.get_success(self.provider._exchange_code(code)) + ret = self.get_success(self.provider._exchange_code(code, code_verifier="")) + kwargs = self.fake_server.request.call_args[1] + + self.assertEqual(ret, token) + self.assertEqual(kwargs["method"], "POST") + self.assertEqual(kwargs["uri"], self.fake_server.token_endpoint) + + args = parse_qs(kwargs["data"].decode("utf-8")) + self.assertEqual(args["grant_type"], ["authorization_code"]) + self.assertEqual(args["code"], [code]) + self.assertEqual(args["client_id"], [CLIENT_ID]) + self.assertEqual(args["client_secret"], [CLIENT_SECRET]) + self.assertEqual(args["redirect_uri"], [CALLBACK_URL]) + + # Test providing a code verifier. + code_verifier = "code_verifier" + ret = self.get_success( + self.provider._exchange_code(code, code_verifier=code_verifier) + ) kwargs = self.fake_server.request.call_args[1] self.assertEqual(ret, token) @@ -614,6 +686,7 @@ def test_exchange_code(self) -> None: self.assertEqual(args["client_id"], [CLIENT_ID]) self.assertEqual(args["client_secret"], [CLIENT_SECRET]) self.assertEqual(args["redirect_uri"], [CALLBACK_URL]) + self.assertEqual(args["code_verifier"], [code_verifier]) # Test error handling self.fake_server.post_token_handler.return_value = FakeResponse.json( @@ -621,7 +694,9 @@ def test_exchange_code(self) -> None: ) from synapse.handlers.oidc import OidcError - exc = self.get_failure(self.provider._exchange_code(code), OidcError) + exc = self.get_failure( + self.provider._exchange_code(code, code_verifier=""), OidcError + ) self.assertEqual(exc.value.error, "foo") self.assertEqual(exc.value.error_description, "bar") @@ -629,7 +704,9 @@ def test_exchange_code(self) -> None: self.fake_server.post_token_handler.return_value = FakeResponse( code=500, body=b"Not JSON" ) - exc = self.get_failure(self.provider._exchange_code(code), OidcError) + exc = self.get_failure( + self.provider._exchange_code(code, code_verifier=""), OidcError + ) self.assertEqual(exc.value.error, "server_error") # Internal server error with JSON body @@ -637,21 +714,27 @@ def test_exchange_code(self) -> None: code=500, payload={"error": "internal_server_error"} ) - exc = self.get_failure(self.provider._exchange_code(code), OidcError) + exc = self.get_failure( + self.provider._exchange_code(code, code_verifier=""), OidcError + ) self.assertEqual(exc.value.error, "internal_server_error") # 4xx error without "error" field self.fake_server.post_token_handler.return_value = FakeResponse.json( code=400, payload={} ) - exc = self.get_failure(self.provider._exchange_code(code), OidcError) + exc = self.get_failure( + self.provider._exchange_code(code, code_verifier=""), OidcError + ) self.assertEqual(exc.value.error, "server_error") # 2xx error with "error" field self.fake_server.post_token_handler.return_value = FakeResponse.json( code=200, payload={"error": "some_error"} ) - exc = self.get_failure(self.provider._exchange_code(code), OidcError) + exc = self.get_failure( + self.provider._exchange_code(code, code_verifier=""), OidcError + ) self.assertEqual(exc.value.error, "some_error") @override_config( @@ -688,7 +771,7 @@ def test_exchange_code_jwt_key(self) -> None: # timestamps. self.reactor.advance(1000) start_time = self.reactor.seconds() - ret = self.get_success(self.provider._exchange_code(code)) + ret = self.get_success(self.provider._exchange_code(code, code_verifier="")) self.assertEqual(ret, token) @@ -739,7 +822,7 @@ def test_exchange_code_no_auth(self) -> None: payload=token ) code = "code" - ret = self.get_success(self.provider._exchange_code(code)) + ret = self.get_success(self.provider._exchange_code(code, code_verifier="")) self.assertEqual(ret, token) @@ -1203,6 +1286,7 @@ def _generate_oidc_session_token( nonce=nonce, client_redirect_url=client_redirect_url, ui_auth_session_id=ui_auth_session_id, + code_verifier="", ), ) diff --git a/tests/util/test_macaroons.py b/tests/util/test_macaroons.py index f68377a05ac4..e56ec2c86099 100644 --- a/tests/util/test_macaroons.py +++ b/tests/util/test_macaroons.py @@ -92,6 +92,7 @@ def test_oidc_session_token(self) -> None: nonce="nonce", client_redirect_url="https://example.com/", ui_auth_session_id="", + code_verifier="", ) token = self.macaroon_generator.generate_oidc_session_token( state, session_data, duration_in_ms=2 * 60 * 1000 From 4aad25e7ea6343600b89beedd4a9f00dec34d65a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 29 Dec 2022 11:13:44 -0500 Subject: [PATCH 2/7] Newsfragment --- changelog.d/14750.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14750.feature diff --git a/changelog.d/14750.feature b/changelog.d/14750.feature new file mode 100644 index 000000000000..cfed64ee80f0 --- /dev/null +++ b/changelog.d/14750.feature @@ -0,0 +1 @@ +Support [RFC7636](https://datatracker.ietf.org/doc/html/rfc7636) Proof Key for Code Exchange for OAuth single sign-on. From 5e910b8d76e539fb9e2c02799cd9c258395f9c96 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 29 Dec 2022 15:35:37 -0500 Subject: [PATCH 3/7] Changes from review. * Drop support for 'plain' method. * Simplify configuration. --- .../configuration/config_documentation.md | 7 ++-- synapse/config/oidc.py | 21 +++------- synapse/handlers/oidc.py | 39 +++++++------------ tests/handlers/test_oidc.py | 4 +- 4 files changed, 25 insertions(+), 46 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 60f7364ca754..b485e37de48a 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3053,9 +3053,10 @@ Options for each entry include: values are `client_secret_basic` (default), `client_secret_post` and `none`. -* `code_challenge_method`: proof key for code exchange method used when requesting - and exchanging the token. Valid values are `null` (default, disables support), - `plain`, and `S256`. +* `pkce_method`: Whether to use proof key for code exchange when requesting + and exchanging the token. Valid values are: `auto` or `always`. Defaults to + `auto`, which uses PKCE if supported during metadata discovery. Set to `always` + to always enable PKCE. * `scopes`: list of scopes to request. This should normally include the "openid" scope. Defaults to `["openid"]`. diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 7986eb987790..d505ed4e5bdb 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -117,18 +117,7 @@ def oidc_enabled(self) -> bool: # to avoid importing authlib here. "enum": ["client_secret_basic", "client_secret_post", "none"], }, - "code_challenge_method": { - "oneOf": [ - { - "type": "string", - # the following list is the same as the keys of - # authlib.oauth2.auth.rfc7636.challenge.CodeChallenge.SUPPORTED_CODE_CHALLENGE_METHOD. - # We inline it to avoid importing authlib here. - "enum": ["plain", "S256"], - }, - {"type": "null"}, - ], - }, + "pkce_method": {"type": "string", "enum": ["auto", "always"]}, "scopes": {"type": "array", "items": {"type": "string"}}, "authorization_endpoint": {"type": "string"}, "token_endpoint": {"type": "string"}, @@ -301,7 +290,7 @@ def _parse_oidc_config_dict( client_secret=oidc_config.get("client_secret"), client_secret_jwt_key=client_secret_jwt_key, client_auth_method=oidc_config.get("client_auth_method", "client_secret_basic"), - code_challenge_method=oidc_config.get("code_challenge_method"), + pkce_method=oidc_config.get("pkce_method", "auto"), scopes=oidc_config.get("scopes", ["openid"]), authorization_endpoint=oidc_config.get("authorization_endpoint"), token_endpoint=oidc_config.get("token_endpoint"), @@ -370,9 +359,9 @@ class OidcProviderConfig: # 'none'. client_auth_method: str - # code challenge method to use when exchanging the authorization & token. - # Valid values are None (to disable use of code challenges), 'plain', and 'S256'. - code_challenge_method: Optional[str] + # Whether to enable PKCE when exchanging the authorization & token. + # Valid values are 'auto', and 'always'. + pkce_method: str # list of scopes to request scopes: Collection[str] diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 328d57c10296..0c1c368cbfee 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -404,7 +404,6 @@ def __init__( provider.client_auth_method, ) self._client_auth_method = provider.client_auth_method - self._code_challenge_method = provider.code_challenge_method # cache of metadata for the identity provider (endpoint uris, mostly). This is # loaded on-demand from the discovery endpoint (if discovery is enabled), with @@ -477,16 +476,12 @@ def _validate_metadata(self, m: OpenIDProviderMetadata) -> None: ) ) - # Validate code_challenge_methods_supported only if it is enabled. - if ( - m.get("code_challenge_methods_supported") is not None - and self._code_challenge_method is not None - ): + # If PKCE support is advertised ensure the wanted method is available. + if m.get("code_challenge_methods_supported") is not None: m.validate_code_challenge_methods_supported() - if self._code_challenge_method not in m["code_challenge_methods_supported"]: + if "S256" not in m["code_challenge_methods_supported"]: raise ValueError( - '"{code_challenge_method}" not in "code_challenge_methods_supported" ({supported!r})'.format( - code_challenge_method=self._code_challenge_method, + '"S256" not in "code_challenge_methods_supported" ({supported!r})'.format( supported=m["code_challenge_methods_supported"], ) ) @@ -618,6 +613,9 @@ async def _load_metadata(self) -> OpenIDProviderMetadata: if self._config.jwks_uri: metadata["jwks_uri"] = self._config.jwks_uri + if self._config.pkce_method == "always": + metadata["code_challenge_methods_supported"] = ["S256"] + self._validate_metadata(metadata) return metadata @@ -959,24 +957,18 @@ async def handle_redirect_request( if not client_redirect_url: client_redirect_url = b"" - # If code challenge is supported and configured, use it. + metadata = await self.load_metadata() + + # Automatically enable PKCE if it is supported. (Note we verified it contains S256 earlier.) extra_grant_values = {} - if self._code_challenge_method is not None: + if metadata.get("code_challenge_methods_supported"): code_verifier = generate_token(48) # Default to the values for plain. - extra_grant_values = {"code_challenge_method": self._code_challenge_method} - - if self._code_challenge_method == "S256": - extra_grant_values["code_challenge"] = create_s256_code_challenge( - code_verifier - ) - elif self._code_challenge_method == "plain": - extra_grant_values["code_challenge"] = code_verifier - else: - raise RuntimeError( - f"Unexpeced code challenege method: {self._code_challenge_method!r}" - ) + extra_grant_values = { + "code_challenge_method": "S256", + "code_challenge": create_s256_code_challenge(code_verifier), + } cookie = self._macaroon_generaton.generate_oidc_session_token( state=state, @@ -1006,7 +998,6 @@ async def handle_redirect_request( ) ) - metadata = await self.load_metadata() authorization_endpoint = metadata.get("authorization_endpoint") return prepare_grant_uri( authorization_endpoint, diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index de788861a5a5..9558ac039961 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -420,9 +420,7 @@ def test_redirect_request(self) -> None: self.assertEqual(code_verifier, "") self.assertEqual(redirect, "http://client/redirect") - @override_config( - {"oidc_config": {**DEFAULT_CONFIG, "code_challenge_method": "S256"}} - ) + @override_config({"oidc_config": {**DEFAULT_CONFIG, "pkce_method": "always"}}) def test_redirect_request_with_code_challenge(self) -> None: """The redirect request has the right arguments & generates a valid session cookie.""" req = Mock(spec=["cookies"]) From 1e82afd7e88d9194b368519ecd696adbca4c856f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 3 Jan 2023 09:08:14 -0500 Subject: [PATCH 4/7] Fix outdated comment. --- synapse/handlers/oidc.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 0c1c368cbfee..8632bb5f1063 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -932,10 +932,12 @@ async def handle_redirect_request( - ``state``: a random string - ``nonce``: a random string - In addition generating a redirect URL, we are setting a cookie with - a signed macaroon token containing the state, the nonce and the - client_redirect_url params. Those are then checked when the client - comes back from the provider. + In addition to generating a redirect URL, we are setting a cookie with + a signed macaroon token containing the state, the nonce, the + client_redirect_url, and (optionally) the code_verifier params. The state, + nonce, and client_redirect_url are then checked when the client comes back + from the provider. The code_verifier is passed back to the server during + the token exchange and compared to the code_challenge sent in this request. Args: request: the incoming request from the browser. @@ -959,12 +961,13 @@ async def handle_redirect_request( metadata = await self.load_metadata() - # Automatically enable PKCE if it is supported. (Note we verified it contains S256 earlier.) + # Automatically enable PKCE if it is supported. extra_grant_values = {} if metadata.get("code_challenge_methods_supported"): code_verifier = generate_token(48) - # Default to the values for plain. + # Note that we verified the server supports S256 earlier (in + # OidcProvider._validate_metadata). extra_grant_values = { "code_challenge_method": "S256", "code_challenge": create_s256_code_challenge(code_verifier), From 45d3bbaa501a17bd7d1acc68a66fba37b9e7374a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 3 Jan 2023 10:37:08 -0500 Subject: [PATCH 5/7] Clarify comments. Co-authored-by: Denis Kasak --- synapse/handlers/oidc.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 8632bb5f1063..864b37e56d3d 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -680,7 +680,7 @@ async def _exchange_code(self, code: str, code_verifier: str) -> Token: Args: code: The authorization code we got from the callback. - code_verifier: The code verifier to send, blank if unused. + code_verifier: The PKCE code verifier to send, blank if unused. Returns: A dict containing various tokens. @@ -931,6 +931,7 @@ async def handle_redirect_request( - ``scope``: the list of scopes set in ``oidc_config.scopes`` - ``state``: a random string - ``nonce``: a random string + - ``code_challenge``: a RFC7636 code challenge (if PKCE is supported) In addition to generating a redirect URL, we are setting a cookie with a signed macaroon token containing the state, the nonce, the From 1ebd2248c877d0f5a8986b3e50af7c94f603f25b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 3 Jan 2023 10:38:43 -0500 Subject: [PATCH 6/7] Clarify comment. --- synapse/util/macaroons.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/macaroons.py b/synapse/util/macaroons.py index 0a2c8b72f2fb..644c341e8cd2 100644 --- a/synapse/util/macaroons.py +++ b/synapse/util/macaroons.py @@ -111,7 +111,7 @@ class OidcSessionData: """The session ID of the ongoing UI Auth ("" if this is a login)""" code_verifier: str - """The random string used in code challenges ("" if code challenges are not used).""" + """The random string used in the RFC7636 code challenge ("" if PKCE is not being used).""" class MacaroonGenerator: From d504e474851c0becad16ee0b1bea24a9b787edbb Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 4 Jan 2023 10:00:44 -0500 Subject: [PATCH 7/7] Support disabling PKCE. --- .../configuration/config_documentation.md | 6 +- synapse/config/oidc.py | 4 +- synapse/handlers/oidc.py | 2 + tests/handlers/test_oidc.py | 94 +++++++++++++++---- 4 files changed, 81 insertions(+), 25 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 49e2a4335f3a..ec8403c7e99b 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3054,9 +3054,9 @@ Options for each entry include: `none`. * `pkce_method`: Whether to use proof key for code exchange when requesting - and exchanging the token. Valid values are: `auto` or `always`. Defaults to - `auto`, which uses PKCE if supported during metadata discovery. Set to `always` - to always enable PKCE. + and exchanging the token. Valid values are: `auto`, `always`, or `never`. Defaults + to `auto`, which uses PKCE if supported during metadata discovery. Set to `always` + to force enable PKCE or `never` to force disable PKCE. * `scopes`: list of scopes to request. This should normally include the "openid" scope. Defaults to `["openid"]`. diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index d505ed4e5bdb..df8c42204392 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -117,7 +117,7 @@ def oidc_enabled(self) -> bool: # to avoid importing authlib here. "enum": ["client_secret_basic", "client_secret_post", "none"], }, - "pkce_method": {"type": "string", "enum": ["auto", "always"]}, + "pkce_method": {"type": "string", "enum": ["auto", "always", "never"]}, "scopes": {"type": "array", "items": {"type": "string"}}, "authorization_endpoint": {"type": "string"}, "token_endpoint": {"type": "string"}, @@ -360,7 +360,7 @@ class OidcProviderConfig: client_auth_method: str # Whether to enable PKCE when exchanging the authorization & token. - # Valid values are 'auto', and 'always'. + # Valid values are 'auto', 'always', and 'never'. pkce_method: str # list of scopes to request diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 003b4f5d9715..0fc829acf77d 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -615,6 +615,8 @@ async def _load_metadata(self) -> OpenIDProviderMetadata: if self._config.pkce_method == "always": metadata["code_challenge_methods_supported"] = ["S256"] + elif self._config.pkce_method == "never": + metadata.pop("code_challenge_methods_supported", None) self._validate_metadata(metadata) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 9558ac039961..adddbd002f50 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -420,30 +420,55 @@ def test_redirect_request(self) -> None: self.assertEqual(code_verifier, "") self.assertEqual(redirect, "http://client/redirect") - @override_config({"oidc_config": {**DEFAULT_CONFIG, "pkce_method": "always"}}) + @override_config({"oidc_config": DEFAULT_CONFIG}) def test_redirect_request_with_code_challenge(self) -> None: """The redirect request has the right arguments & generates a valid session cookie.""" req = Mock(spec=["cookies"]) req.cookies = [] + with self.metadata_edit({"code_challenge_methods_supported": ["S256"]}): + url = urlparse( + self.get_success( + self.provider.handle_redirect_request( + req, b"http://client/redirect" + ) + ) + ) + + # Ensure the code_challenge param is added to the redirect. + params = parse_qs(url.query) + self.assertEqual(len(params["code_challenge"]), 1) + + # Check what is in the cookies + self.assertEqual(len(req.cookies), 2) # two cookies + cookie_header = req.cookies[0] + + # The cookie name and path don't really matter, just that it has to be coherent + # between the callback & redirect handlers. + parts = [p.strip() for p in cookie_header.split(b";")] + self.assertIn(b"Path=/_synapse/client/oidc", parts) + name, cookie = parts[0].split(b"=") + self.assertEqual(name, b"oidc_session") + + # Ensure the code_verifier is set in the cookie. + macaroon = pymacaroons.Macaroon.deserialize(cookie) + code_verifier = get_value_from_macaroon(macaroon, "code_verifier") + self.assertNotEqual(code_verifier, "") + + @override_config({"oidc_config": {**DEFAULT_CONFIG, "pkce_method": "always"}}) + def test_redirect_request_with_forced_code_challenge(self) -> None: + """The redirect request has the right arguments & generates a valid session cookie.""" + req = Mock(spec=["cookies"]) + req.cookies = [] + url = urlparse( self.get_success( self.provider.handle_redirect_request(req, b"http://client/redirect") ) ) - auth_endpoint = urlparse(self.fake_server.authorization_endpoint) - - self.assertEqual(url.scheme, auth_endpoint.scheme) - self.assertEqual(url.netloc, auth_endpoint.netloc) - self.assertEqual(url.path, auth_endpoint.path) + # Ensure the code_challenge param is added to the redirect. params = parse_qs(url.query) - self.assertEqual(params["redirect_uri"], [CALLBACK_URL]) - self.assertEqual(params["response_type"], ["code"]) - self.assertEqual(params["scope"], [" ".join(SCOPES)]) - self.assertEqual(params["client_id"], [CLIENT_ID]) - self.assertEqual(len(params["state"]), 1) - self.assertEqual(len(params["nonce"]), 1) self.assertEqual(len(params["code_challenge"]), 1) # Check what is in the cookies @@ -457,17 +482,46 @@ def test_redirect_request_with_code_challenge(self) -> None: name, cookie = parts[0].split(b"=") self.assertEqual(name, b"oidc_session") + # Ensure the code_verifier is set in the cookie. macaroon = pymacaroons.Macaroon.deserialize(cookie) - state = get_value_from_macaroon(macaroon, "state") - nonce = get_value_from_macaroon(macaroon, "nonce") code_verifier = get_value_from_macaroon(macaroon, "code_verifier") - redirect = get_value_from_macaroon(macaroon, "client_redirect_url") - - self.assertEqual(params["state"], [state]) - self.assertEqual(params["nonce"], [nonce]) - # Ensure the code verifier is not blank. self.assertNotEqual(code_verifier, "") - self.assertEqual(redirect, "http://client/redirect") + + @override_config({"oidc_config": {**DEFAULT_CONFIG, "pkce_method": "never"}}) + def test_redirect_request_with_disabled_code_challenge(self) -> None: + """The redirect request has the right arguments & generates a valid session cookie.""" + req = Mock(spec=["cookies"]) + req.cookies = [] + + # The metadata should state that PKCE is enabled. + with self.metadata_edit({"code_challenge_methods_supported": ["S256"]}): + url = urlparse( + self.get_success( + self.provider.handle_redirect_request( + req, b"http://client/redirect" + ) + ) + ) + + # Ensure the code_challenge param is added to the redirect. + params = parse_qs(url.query) + self.assertNotIn("code_challenge", params) + + # Check what is in the cookies + self.assertEqual(len(req.cookies), 2) # two cookies + cookie_header = req.cookies[0] + + # The cookie name and path don't really matter, just that it has to be coherent + # between the callback & redirect handlers. + parts = [p.strip() for p in cookie_header.split(b";")] + self.assertIn(b"Path=/_synapse/client/oidc", parts) + name, cookie = parts[0].split(b"=") + self.assertEqual(name, b"oidc_session") + + # Ensure the code_verifier is blank in the cookie. + macaroon = pymacaroons.Macaroon.deserialize(cookie) + code_verifier = get_value_from_macaroon(macaroon, "code_verifier") + self.assertEqual(code_verifier, "") @override_config({"oidc_config": DEFAULT_CONFIG}) def test_callback_error(self) -> None: