diff --git a/changelog.d/18374.misc b/changelog.d/18374.misc new file mode 100644 index 0000000000..a8efca68d0 --- /dev/null +++ b/changelog.d/18374.misc @@ -0,0 +1 @@ +Don't validate the `at_hash` (access token hash) field in OIDC ID Tokens if we don't end up actually using the OIDC Access Token. \ No newline at end of file diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index c4cf0636a3..fb759172b3 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -586,6 +586,24 @@ class OidcProvider: or self._user_profile_method == "userinfo_endpoint" ) + @property + def _uses_access_token(self) -> bool: + """Return True if the `access_token` will be used during the login process. + + This is useful to determine whether the access token + returned by the identity provider, and + any related metadata (such as the `at_hash` field in + the ID token), should be validated. + """ + # Currently, Synapse only uses the access_token to fetch user metadata + # from the userinfo endpoint. Therefore we only have a single criteria + # to check right now but this may change in the future and this function + # should be updated if more usages are introduced. + # + # For example, if we start to use the access_token given to us by the + # IdP for more things, such as accessing Resource Server APIs. + return self._uses_userinfo + @property def issuer(self) -> str: """The issuer identifying this provider.""" @@ -957,9 +975,16 @@ class OidcProvider: "nonce": nonce, "client_id": self._client_auth.client_id, } - if "access_token" in token: + if self._uses_access_token and "access_token" in token: # If we got an `access_token`, there should be an `at_hash` claim - # in the `id_token` that we can check against. + # in the `id_token` that we can check against. Setting this + # instructs authlib to check the value of `at_hash` in the + # ID token. + # + # We only need to verify the access token if we actually make + # use of it. Which currently only happens when we need to fetch + # the user's information from the userinfo_endpoint. Thus, this + # check is also gated on self._uses_userinfo. claims_params["access_token"] = token["access_token"] claims_options = {"iss": {"values": [metadata["issuer"]]}} diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index a7cead83d0..e5f31d57ca 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1029,6 +1029,50 @@ class OidcHandlerTestCase(HomeserverTestCase): args = parse_qs(kwargs["data"].decode("utf-8")) self.assertEqual(args["redirect_uri"], [TEST_REDIRECT_URI]) + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "redirect_uri": TEST_REDIRECT_URI, + } + } + ) + def test_code_exchange_ignores_access_token(self) -> None: + """ + Code exchange completes successfully and doesn't validate the `at_hash` + (access token hash) field of an ID token when the access token isn't + going to be used. + + The access token won't be used in this test because Synapse (currently) + only needs it to fetch a user's metadata if it isn't included in the ID + token itself. + + Because we have included "openid" in the requested scopes for this IdP + (see `SCOPES`), user metadata is be included in the ID token. Thus the + access token isn't needed, and it's unnecessary for Synapse to validate + the access token. + + This is a regression test for a situation where an upstream identity + provider was providing an invalid `at_hash` value, which Synapse errored + on, yet Synapse wasn't using the access token for anything. + """ + # Exchange the code against the fake IdP. + userinfo = { + "sub": "foo", + "username": "foo", + "phone": "1234567", + } + with self.fake_server.id_token_override( + { + "at_hash": "invalid-hash", + } + ): + request, _ = self.start_authorization(userinfo) + self.get_success(self.handler.handle_oidc_callback(request)) + + # If no error was rendered, then we have success. + self.render_error.assert_not_called() + @override_config( { "oidc_config": { diff --git a/tests/test_utils/oidc.py b/tests/test_utils/oidc.py index 6c4be1c1f8..5bf5e5cb0c 100644 --- a/tests/test_utils/oidc.py +++ b/tests/test_utils/oidc.py @@ -20,7 +20,9 @@ # +import base64 import json +from hashlib import sha256 from typing import Any, ContextManager, Dict, List, Optional, Tuple from unittest.mock import Mock, patch from urllib.parse import parse_qs @@ -154,10 +156,23 @@ class FakeOidcServer: json_payload = json.dumps(payload) return jws.serialize_compact(protected, json_payload, self._key).decode("utf-8") - def generate_id_token(self, grant: FakeAuthorizationGrant) -> str: + def generate_id_token( + self, grant: FakeAuthorizationGrant, access_token: str + ) -> str: + # Generate a hash of the access token for the optional + # `at_hash` field in an ID Token. + # + # 3.1.3.6. ID Token, https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken + at_hash = ( + base64.urlsafe_b64encode(sha256(access_token.encode("ascii")).digest()[:16]) + .rstrip(b"=") + .decode("ascii") + ) + now = int(self._clock.time()) id_token = { **grant.userinfo, + "at_hash": at_hash, "iss": self.issuer, "aud": grant.client_id, "iat": now, @@ -243,7 +258,7 @@ class FakeOidcServer: } if "openid" in grant.scope: - token["id_token"] = self.generate_id_token(grant) + token["id_token"] = self.generate_id_token(grant, access_token) return dict(token)