OpenCloud
/
MatrixSynapse
mirror of https://github.com/matrix-org/synapse
1
0
Fork 0
Code Issues Releases Wiki Activity

Support RFC7636 PKCE in the OAuth 2.0 flow. (#14750) 

PKCE can protect against certain attacks and is enabled by default. Support
can be controlled manually by setting the pkce_method of each oidc_providers
entry to 'auto' (default), 'always', or 'never'.

This is required by Twitter OAuth 2.0 support.
pull/14786/head
Patrick Cloke 2023-01-04 14:58:08 -05:00 committed by GitHub
parent 747f8eb231
commit 630d0aeaf6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 212 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/14750.feature Normal file
View File

@ -0,0 +1 @@
Support [RFC7636](https://datatracker.ietf.org/doc/html/rfc7636) Proof Key for Code Exchange for OAuth single sign-on.

7
docs/usage/configuration/config_documentation.md
View File

@ -3053,8 +3053,13 @@ Options for each entry include:
values are `client_secret_basic` (default), `client_secret_post` and
`none`.
* `pkce_method`: Whether to use proof key for code exchange when requesting
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"].
scope. Defaults to `["openid"]`.
* `authorization_endpoint`: the oauth2 authorization endpoint. Required if
provider discovery is disabled.

6
synapse/config/oidc.py
View File

@ -117,6 +117,7 @@ OIDC_PROVIDER_CONFIG_SCHEMA = {
# to avoid importing authlib here.
"enum": ["client_secret_basic", "client_secret_post", "none"],
},
"pkce_method": {"type": "string", "enum": ["auto", "always", "never"]},
"scopes": {"type": "array", "items": {"type": "string"}},
"authorization_endpoint": {"type": "string"},
"token_endpoint": {"type": "string"},
@ -289,6 +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"),
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"),
@ -357,6 +359,10 @@ class OidcProviderConfig:
# 'none'.
client_auth_method: str
# Whether to enable PKCE when exchanging the authorization & token.
# Valid values are 'auto', 'always', and 'never'.
pkce_method: str
# list of scopes to request
scopes: Collection[str]

54
synapse/handlers/oidc.py
View File

@ -36,6 +36,7 @@ from authlib.jose import JsonWebToken, JWTClaims
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
@ -475,6 +476,16 @@ class OidcProvider:
)
)
# 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 "S256" not in m["code_challenge_methods_supported"]:
raise ValueError(
'"S256" not in "code_challenge_methods_supported" ({supported!r})'.format(
supported=m["code_challenge_methods_supported"],
)
)
if m.get("response_types_supported") is not None:
m.validate_response_types_supported()
@ -602,6 +613,11 @@ class OidcProvider:
if self._config.jwks_uri:
metadata["jwks_uri"] = self._config.jwks_uri
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)
return metadata
@ -653,7 +669,7 @@ class OidcProvider:
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 @@ class OidcProvider:
Args:
code: The authorization code we got from the callback.
code_verifier: The PKCE code verifier to send, blank if unused.
Returns:
A dict containing various tokens.
@ -696,6 +713,8 @@ class OidcProvider:
"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
@ -914,11 +933,14 @@ class OidcProvider:
- ``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 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.
@ -935,10 +957,25 @@ class OidcProvider:
state = generate_token()
nonce = generate_token()
code_verifier = ""
if not client_redirect_url:
client_redirect_url = b""
metadata = await self.load_metadata()
# Automatically enable PKCE if it is supported.
extra_grant_values = {}
if metadata.get("code_challenge_methods_supported"):
code_verifier = generate_token(48)
# 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),
}
cookie = self._macaroon_generaton.generate_oidc_session_token(
state=state,
session_data=OidcSessionData(
@ -946,6 +983,7 @@ class OidcProvider:
nonce=nonce,
client_redirect_url=client_redirect_url.decode(),
ui_auth_session_id=ui_auth_session_id or "",
code_verifier=code_verifier,
),
)
@ -966,7 +1004,6 @@ class OidcProvider:
)
)
metadata = await self.load_metadata()
authorization_endpoint = metadata.get("authorization_endpoint")
return prepare_grant_uri(
authorization_endpoint,
@ -976,6 +1013,7 @@ class OidcProvider:
scope=self._scopes,
state=state,
nonce=nonce,
**extra_grant_values,
)
async def handle_oidc_callback(
@ -1003,7 +1041,9 @@ class OidcProvider:
# 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)

7
synapse/util/macaroons.py
View File

@ -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 the RFC7636 code challenge ("" if PKCE is not being used)."""
class MacaroonGenerator:
def __init__(self, clock: Clock, location: str, secret_key: bytes):
@ -187,6 +190,7 @@ class MacaroonGenerator:
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 @@ class MacaroonGenerator:
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 @@ class MacaroonGenerator:
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:

152
tests/handlers/test_oidc.py
View File

@ -396,6 +396,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
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,12 +412,117 @@ class OidcHandlerTestCase(HomeserverTestCase):
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})
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")
)
)
# 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": "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:
"""Errors from the provider returned in the callback are displayed."""
@ -601,7 +707,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
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)
@ -615,13 +721,34 @@ class OidcHandlerTestCase(HomeserverTestCase):
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)
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])
self.assertEqual(args["code_verifier"], [code_verifier])
# Test error handling
self.fake_server.post_token_handler.return_value = FakeResponse.json(
code=400, payload={"error": "foo", "error_description": "bar"}
)
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 +756,9 @@ class OidcHandlerTestCase(HomeserverTestCase):
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 +766,27 @@ class OidcHandlerTestCase(HomeserverTestCase):
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 +823,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
# 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 +874,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
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 +1338,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
nonce=nonce,
client_redirect_url=client_redirect_url,
ui_auth_session_id=ui_auth_session_id,
code_verifier="",
),
)

1
tests/util/test_macaroons.py
View File

@ -92,6 +92,7 @@ class MacaroonGeneratorTestCase(TestCase):
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