From 01d153eb8004b8f0cb6af63b5c40713b79d4b820 Mon Sep 17 00:00:00 2001 From: Renan Rodrigues dos Santos Date: Sat, 28 Sep 2024 15:19:52 +0000 Subject: [PATCH 1/5] Add PKCE --- oauthenticator/oauth2.py | 47 +++++++++++++++++++++++- oauthenticator/tests/test_oauth2.py | 55 +++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index 8c62a67c..45332a0c 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -5,11 +5,14 @@ """ import base64 +import hashlib import json import os +import secrets import uuid from functools import reduce from inspect import isawaitable +from typing import cast from urllib.parse import quote, urlencode, urlparse, urlunparse import jwt @@ -115,7 +118,22 @@ def get(self): state_id = self._generate_state_id() next_url = self._get_next_url() - cookie_state = _serialize_state({"state_id": state_id, "next_url": next_url}) + state = {"state_id": state_id, "next_url": next_url} + + if self.authenticator.pkce: + # https://datatracker.ietf.org/doc/html/rfc7636#section-4 + code_verifier = secrets.token_urlsafe(43) + code_challenge = hashlib.sha256(code_verifier.encode("utf-8")).digest() + code_challenge_base64 = ( + base64.urlsafe_b64encode(code_challenge).decode("utf-8").rstrip("=") + ) + + token_params["code_challenge"] = code_challenge_base64 + token_params["code_challenge_method"] = "S256" + + state["code_verifier"] = code_verifier + + cookie_state = _serialize_state(state) self.set_state_cookie(cookie_state) authorize_state = _serialize_state({"state_id": state_id}) @@ -663,6 +681,19 @@ def _allowed_scopes_validation(self, proposal): """, ) + pkce = Bool( + os.environ.get("OAUTH2_PKCE", "False").lower() in {"true", "1"}, + config=True, + help=""" + Whether to use PKCE (Proof Key for Code Exchange) for the OAuth2 flow. + + When using PKCE, the client secret is not required to be sent to the + token endpoint. + Only the S256 method is supported. + `RFC 7636 `. + """, + ) + client_id_env = "" client_id = Unicode( config=True, @@ -980,6 +1011,20 @@ def build_access_tokens_request_params(self, handler, data=None): "data": data, } + if self.pkce: + # https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 + callback_handler = cast(OAuthCallbackHandler, handler) + + cookie_state = callback_handler.get_state_cookie() + if not cookie_state: + raise web.HTTPError(400, "OAuth state missing from cookies") + + code_verifier = _deserialize_state(cookie_state).get("code_verifier") + if not code_verifier: + raise web.HTTPError(400, "Missing code_verifier") + + params.update([("code_verifier", code_verifier)]) + # the client_id and client_secret should not be included in the access token request params # when basic authentication is used # ref: https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1 diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py index 4386e92c..16fd506e 100644 --- a/oauthenticator/tests/test_oauth2.py +++ b/oauthenticator/tests/test_oauth2.py @@ -31,6 +31,7 @@ async def test_serialize_state(): TEST_STATE_ID = '123' TEST_NEXT_URL = '/ABC' +TEST_CODE_VERIFIER = 'code_verifier123' async def test_login_states(): @@ -206,3 +207,57 @@ async def test_add_user_override( assert added_user.name in authenticator.allowed_users else: assert added_user.name not in authenticator.allowed_users + + +async def test_login_handler_pkce(): + authenticator = OAuthenticator(pkce=True) + login_handler = mock_handler(OAuthLoginHandler, authenticator=authenticator) + + login_handler._generate_state_id = Mock(return_value=TEST_STATE_ID) + login_handler.set_state_cookie = Mock() + login_handler.authorize_redirect = Mock() + + login_handler.get() # no await, since authorize_redirect is mocked + + # Check that PKCE parameters are included + assert ( + "code_challenge" + in login_handler.authorize_redirect.call_args.kwargs["extra_params"] + ) + assert ( + login_handler.authorize_redirect.call_args.kwargs["extra_params"][ + "code_challenge_method" + ] + == "S256" + ) + + +async def test_callback_handler_pkce(): + url_state = _serialize_state({'state_id': TEST_STATE_ID}) + callback_request_uri = f"http://myhost/callback?code=123&state={url_state}" + + cookie_state = _serialize_state( + { + 'state_id': TEST_STATE_ID, + 'next_url': TEST_NEXT_URL, + 'code_verifier': TEST_CODE_VERIFIER, + } + ) + + authenticator = OAuthenticator(pkce=True) + callback_handler = mock_handler( + OAuthCallbackHandler, + uri=callback_request_uri, + authenticator=authenticator, + ) + + callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8')) + callback_handler.login_user = Mock(return_value=mock_login_user_coro()) + callback_handler.redirect = Mock() + + await callback_handler.get() + + callback_handler.redirect.assert_called_once_with('/ABC') + params = authenticator.build_access_tokens_request_params(callback_handler) + + assert params['code_verifier'] == TEST_CODE_VERIFIER From d9f96d24341b9ff57d69d01d4bfc675dc14c0769 Mon Sep 17 00:00:00 2001 From: Renan Rodrigues dos Santos Date: Sat, 28 Sep 2024 20:48:57 +0000 Subject: [PATCH 2/5] Address PR comments --- oauthenticator/oauth2.py | 9 ++----- oauthenticator/tests/test_oauth2.py | 41 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index 45332a0c..e62a21c3 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -12,7 +12,6 @@ import uuid from functools import reduce from inspect import isawaitable -from typing import cast from urllib.parse import quote, urlencode, urlparse, urlunparse import jwt @@ -682,13 +681,11 @@ def _allowed_scopes_validation(self, proposal): ) pkce = Bool( - os.environ.get("OAUTH2_PKCE", "False").lower() in {"true", "1"}, + False, config=True, help=""" Whether to use PKCE (Proof Key for Code Exchange) for the OAuth2 flow. - When using PKCE, the client secret is not required to be sent to the - token endpoint. Only the S256 method is supported. `RFC 7636 `. """, @@ -1013,9 +1010,7 @@ def build_access_tokens_request_params(self, handler, data=None): if self.pkce: # https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 - callback_handler = cast(OAuthCallbackHandler, handler) - - cookie_state = callback_handler.get_state_cookie() + cookie_state = handler.get_state_cookie() if not cookie_state: raise web.HTTPError(400, "OAuth state missing from cookies") diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py index 16fd506e..1bcf4c19 100644 --- a/oauthenticator/tests/test_oauth2.py +++ b/oauthenticator/tests/test_oauth2.py @@ -261,3 +261,44 @@ async def test_callback_handler_pkce(): params = authenticator.build_access_tokens_request_params(callback_handler) assert params['code_verifier'] == TEST_CODE_VERIFIER + + +async def test_pkce_not_supported(monkeypatch): + url_state = _serialize_state({'state_id': TEST_STATE_ID}) + callback_request_uri = f"http://myhost/callback?code=123&state={url_state}" + + cookie_state = _serialize_state( + { + 'state_id': TEST_STATE_ID, + 'next_url': TEST_NEXT_URL, + 'code_verifier': TEST_CODE_VERIFIER, + } + ) + + authenticator = OAuthenticator(pkce=True) + callback_handler = mock_handler( + OAuthCallbackHandler, + uri=callback_request_uri, + authenticator=authenticator, + ) + + # Mock the statsd setting + monkeypatch.setitem(callback_handler.settings, 'statsd', Mock()) + callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8')) + + # Mock the token request to return an error indicating PKCE is not supported + error_response = { + "error": "invalid_request", + "error_description": "PKCE not supported", + } + + async def mock_httpfetch(*args, **kwargs): + return error_response + + authenticator.httpfetch = mock_httpfetch + + with raises(HTTPError) as exc_info: + await callback_handler.get() + + assert exc_info.value.status_code == 403 + assert "An access token was not returned: PKCE not supported" in str(exc_info.value) From f65accec08dffce3d16a28e8b6b67ff4c30ceb25 Mon Sep 17 00:00:00 2001 From: Renan Rodrigues dos Santos Date: Sun, 29 Sep 2024 23:43:58 +0000 Subject: [PATCH 3/5] Rename pkce config to require_pkce Remove pkce not supported test --- oauthenticator/oauth2.py | 12 ++++---- oauthenticator/tests/test_oauth2.py | 48 ++--------------------------- 2 files changed, 8 insertions(+), 52 deletions(-) diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index e62a21c3..705d4eda 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -119,7 +119,7 @@ def get(self): state = {"state_id": state_id, "next_url": next_url} - if self.authenticator.pkce: + if self.authenticator.require_pkce: # https://datatracker.ietf.org/doc/html/rfc7636#section-4 code_verifier = secrets.token_urlsafe(43) code_challenge = hashlib.sha256(code_verifier.encode("utf-8")).digest() @@ -680,13 +680,13 @@ def _allowed_scopes_validation(self, proposal): """, ) - pkce = Bool( + require_pkce = Bool( False, config=True, help=""" - Whether to use PKCE (Proof Key for Code Exchange) for the OAuth2 flow. - - Only the S256 method is supported. + Require Proof Key for Code Exchange (PKCE) for OAuth2 authorization code flow + + Only the S256 code challenge method is supported. `RFC 7636 `. """, ) @@ -1008,7 +1008,7 @@ def build_access_tokens_request_params(self, handler, data=None): "data": data, } - if self.pkce: + if self.require_pkce: # https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 cookie_state = handler.get_state_cookie() if not cookie_state: diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py index 1bcf4c19..0339722d 100644 --- a/oauthenticator/tests/test_oauth2.py +++ b/oauthenticator/tests/test_oauth2.py @@ -210,11 +210,8 @@ async def test_add_user_override( async def test_login_handler_pkce(): - authenticator = OAuthenticator(pkce=True) + authenticator = OAuthenticator(require_pkce=True) login_handler = mock_handler(OAuthLoginHandler, authenticator=authenticator) - - login_handler._generate_state_id = Mock(return_value=TEST_STATE_ID) - login_handler.set_state_cookie = Mock() login_handler.authorize_redirect = Mock() login_handler.get() # no await, since authorize_redirect is mocked @@ -244,7 +241,7 @@ async def test_callback_handler_pkce(): } ) - authenticator = OAuthenticator(pkce=True) + authenticator = OAuthenticator(require_pkce=True) callback_handler = mock_handler( OAuthCallbackHandler, uri=callback_request_uri, @@ -261,44 +258,3 @@ async def test_callback_handler_pkce(): params = authenticator.build_access_tokens_request_params(callback_handler) assert params['code_verifier'] == TEST_CODE_VERIFIER - - -async def test_pkce_not_supported(monkeypatch): - url_state = _serialize_state({'state_id': TEST_STATE_ID}) - callback_request_uri = f"http://myhost/callback?code=123&state={url_state}" - - cookie_state = _serialize_state( - { - 'state_id': TEST_STATE_ID, - 'next_url': TEST_NEXT_URL, - 'code_verifier': TEST_CODE_VERIFIER, - } - ) - - authenticator = OAuthenticator(pkce=True) - callback_handler = mock_handler( - OAuthCallbackHandler, - uri=callback_request_uri, - authenticator=authenticator, - ) - - # Mock the statsd setting - monkeypatch.setitem(callback_handler.settings, 'statsd', Mock()) - callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8')) - - # Mock the token request to return an error indicating PKCE is not supported - error_response = { - "error": "invalid_request", - "error_description": "PKCE not supported", - } - - async def mock_httpfetch(*args, **kwargs): - return error_response - - authenticator.httpfetch = mock_httpfetch - - with raises(HTTPError) as exc_info: - await callback_handler.get() - - assert exc_info.value.status_code == 403 - assert "An access token was not returned: PKCE not supported" in str(exc_info.value) From 2ee67aef56712d68889aa5c8839318c8daf0250a Mon Sep 17 00:00:00 2001 From: Renan Rodrigues dos Santos Date: Sun, 29 Sep 2024 23:48:07 +0000 Subject: [PATCH 4/5] fix docs --- oauthenticator/oauth2.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index 705d4eda..07149d08 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -684,10 +684,10 @@ def _allowed_scopes_validation(self, proposal): False, config=True, help=""" - Require Proof Key for Code Exchange (PKCE) for OAuth2 authorization code flow + Require Proof Key for Code Exchange (PKCE) for OAuth2 authorization code flow. Only the S256 code challenge method is supported. - `RFC 7636 `. + `RFC 7636 `_. """, ) From 73c23d12c9f7e7658cccf18f45aeca50d090ff03 Mon Sep 17 00:00:00 2001 From: Renan Rodrigues dos Santos Date: Mon, 30 Sep 2024 17:53:26 +0000 Subject: [PATCH 5/5] Rename config from require_pkce to enable_pkce Change enable_pkce default to True Factor out code_verifier and code_challenge generation Improve docs Update tests --- oauthenticator/oauth2.py | 60 +++++++++++++------- oauthenticator/tests/mocks.py | 7 +++ oauthenticator/tests/test_oauth2.py | 88 +++++++---------------------- 3 files changed, 69 insertions(+), 86 deletions(-) diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index 07149d08..bdf9f285 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -93,6 +93,18 @@ def set_state_cookie(self, state_cookie_value): STATE_COOKIE_NAME, state_cookie_value, expires_days=1, httponly=True ) + def _generate_pkce_params(self): + # https://datatracker.ietf.org/doc/html/rfc7636#section-4 + # It is recommended that the output of the random number generator creates + # a 32-octet sequence which is base64url-encoded to produce a 43-octet URL + # safe string to use as the code verifier. + code_verifier = secrets.token_urlsafe(32) + code_challenge = hashlib.sha256(code_verifier.encode("utf-8")).digest() + code_challenge_base64 = ( + base64.urlsafe_b64encode(code_challenge).decode("utf-8").rstrip("=") + ) + return code_verifier, code_challenge_base64 + def _generate_state_id(self): return uuid.uuid4().hex @@ -119,18 +131,11 @@ def get(self): state = {"state_id": state_id, "next_url": next_url} - if self.authenticator.require_pkce: - # https://datatracker.ietf.org/doc/html/rfc7636#section-4 - code_verifier = secrets.token_urlsafe(43) - code_challenge = hashlib.sha256(code_verifier.encode("utf-8")).digest() - code_challenge_base64 = ( - base64.urlsafe_b64encode(code_challenge).decode("utf-8").rstrip("=") - ) - - token_params["code_challenge"] = code_challenge_base64 - token_params["code_challenge_method"] = "S256" - + if self.authenticator.enable_pkce: + code_verifier, code_challenge = self._generate_pkce_params() state["code_verifier"] = code_verifier + token_params["code_challenge"] = code_challenge + token_params["code_challenge_method"] = "S256" cookie_state = _serialize_state(state) self.set_state_cookie(cookie_state) @@ -680,15 +685,32 @@ def _allowed_scopes_validation(self, proposal): """, ) - require_pkce = Bool( - False, + enable_pkce = Bool( + True, config=True, help=""" - Require Proof Key for Code Exchange (PKCE) for OAuth2 authorization code flow. - - Only the S256 code challenge method is supported. - `RFC 7636 `_. - """, + Enable Proof Key for Code Exchange (PKCE) for the OAuth2 authorization code flow. + For more information, see `RFC 7636 `_. + + PKCE can be used even if the authorization server does not support it. According to + `section 3.1 of RFC 6749 `_: + + The authorization server MUST ignore unrecognized request parameters. + + Additionally, `section 5 of RFC 7636 `_ states: + + As the OAuth 2.0 [RFC6749] server responses are unchanged by this + specification, client implementations of this specification do not + need to know if the server has implemented this specification or not + and SHOULD send the additional parameters as defined in Section 4 to + all servers. + + Note that S256 is the only code challenge method supported. As per `section 4.2 of RFC 6749 + `_: + + If the client is capable of using "S256", it MUST use "S256", as + "S256" is Mandatory To Implement (MTI) on the server. + """, ) client_id_env = "" @@ -1008,7 +1030,7 @@ def build_access_tokens_request_params(self, handler, data=None): "data": data, } - if self.require_pkce: + if self.enable_pkce: # https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 cookie_state = handler.get_state_cookie() if not cookie_state: diff --git a/oauthenticator/tests/mocks.py b/oauthenticator/tests/mocks.py index efeac575..d9174604 100644 --- a/oauthenticator/tests/mocks.py +++ b/oauthenticator/tests/mocks.py @@ -15,6 +15,8 @@ from tornado.log import app_log from tornado.simple_httpclient import SimpleAsyncHTTPClient +from ..oauth2 import _serialize_state + RegExpType = type(re.compile('.')) @@ -222,6 +224,11 @@ def handler_for_user(user): method="GET", uri=f"https://hub.example.com?code={code}" ) handler.hub = Mock(server=Mock(base_url='/hub/'), base_url='/hub/') + handler.get_state_cookie = Mock( + return_value=_serialize_state( + {"state_id": "123", "next_url": "/ABC", "code_verifier": "123"} + ) + ) return handler client.handler_for_user = handler_for_user diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py index 0339722d..e0c2fde8 100644 --- a/oauthenticator/tests/test_oauth2.py +++ b/oauthenticator/tests/test_oauth2.py @@ -31,12 +31,12 @@ async def test_serialize_state(): TEST_STATE_ID = '123' TEST_NEXT_URL = '/ABC' -TEST_CODE_VERIFIER = 'code_verifier123' -async def test_login_states(): +@mark.parametrize("enable_pkce", [True, False]) +async def test_login_states(enable_pkce): login_request_uri = f"http://myhost/login?next={TEST_NEXT_URL}" - authenticator = OAuthenticator() + authenticator = OAuthenticator(enable_pkce=enable_pkce) login_handler = mock_handler( OAuthLoginHandler, uri=login_request_uri, @@ -44,30 +44,35 @@ async def test_login_states(): ) login_handler._generate_state_id = Mock(return_value=TEST_STATE_ID) - + code_verifier, code_challenge = login_handler._generate_pkce_params() + login_handler._generate_pkce_params = Mock( + return_value=(code_verifier, code_challenge) + ) login_handler.set_state_cookie = Mock() login_handler.authorize_redirect = Mock() login_handler.get() # no await, we've mocked the authorizer_redirect to NOT be async - expected_cookie_value = _serialize_state( - { - 'state_id': TEST_STATE_ID, - 'next_url': TEST_NEXT_URL, - } - ) + expected_state = { + 'state_id': TEST_STATE_ID, + 'next_url': TEST_NEXT_URL, + } + if enable_pkce: + expected_state['code_verifier'] = code_verifier + expected_cookie_value = _serialize_state(expected_state) login_handler.set_state_cookie.assert_called_once_with(expected_cookie_value) - expected_state_param_value = _serialize_state( - { - 'state_id': TEST_STATE_ID, - } - ) + expected_state_param_value = { + 'state': _serialize_state({'state_id': TEST_STATE_ID}) + } + if enable_pkce: + expected_state_param_value['code_challenge'] = code_challenge + expected_state_param_value['code_challenge_method'] = 'S256' login_handler.authorize_redirect.assert_called_once() assert ( - login_handler.authorize_redirect.call_args.kwargs['extra_params']['state'] + login_handler.authorize_redirect.call_args.kwargs['extra_params'] == expected_state_param_value ) @@ -207,54 +212,3 @@ async def test_add_user_override( assert added_user.name in authenticator.allowed_users else: assert added_user.name not in authenticator.allowed_users - - -async def test_login_handler_pkce(): - authenticator = OAuthenticator(require_pkce=True) - login_handler = mock_handler(OAuthLoginHandler, authenticator=authenticator) - login_handler.authorize_redirect = Mock() - - login_handler.get() # no await, since authorize_redirect is mocked - - # Check that PKCE parameters are included - assert ( - "code_challenge" - in login_handler.authorize_redirect.call_args.kwargs["extra_params"] - ) - assert ( - login_handler.authorize_redirect.call_args.kwargs["extra_params"][ - "code_challenge_method" - ] - == "S256" - ) - - -async def test_callback_handler_pkce(): - url_state = _serialize_state({'state_id': TEST_STATE_ID}) - callback_request_uri = f"http://myhost/callback?code=123&state={url_state}" - - cookie_state = _serialize_state( - { - 'state_id': TEST_STATE_ID, - 'next_url': TEST_NEXT_URL, - 'code_verifier': TEST_CODE_VERIFIER, - } - ) - - authenticator = OAuthenticator(require_pkce=True) - callback_handler = mock_handler( - OAuthCallbackHandler, - uri=callback_request_uri, - authenticator=authenticator, - ) - - callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8')) - callback_handler.login_user = Mock(return_value=mock_login_user_coro()) - callback_handler.redirect = Mock() - - await callback_handler.get() - - callback_handler.redirect.assert_called_once_with('/ABC') - params = authenticator.build_access_tokens_request_params(callback_handler) - - assert params['code_verifier'] == TEST_CODE_VERIFIER