From 9b9a11ba07a3691ca108c706c8be24d76380c102 Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Thu, 17 Jun 2021 16:29:24 +0200 Subject: [PATCH 01/13] add PKCE support --- setup.py | 1 + src/pyop/message.py | 29 +++++++++++++++++++++ src/pyop/provider.py | 60 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 79 insertions(+), 11 deletions(-) create mode 100644 src/pyop/message.py diff --git a/setup.py b/setup.py index da943f6..26a7ec3 100644 --- a/setup.py +++ b/setup.py @@ -12,6 +12,7 @@ description='OpenID Connect Provider (OP) library in Python.', install_requires=[ 'oic >= 1.2.1', + 'nacl', 'pymongo' ] ) diff --git a/src/pyop/message.py b/src/pyop/message.py new file mode 100644 index 0000000..ed3c113 --- /dev/null +++ b/src/pyop/message.py @@ -0,0 +1,29 @@ +from oic.oauth2.message import SINGLE_OPTIONAL_STRING +from oic.oic import message + +class AccessTokenRequest(message.Message): + c_param = message.AccessTokenRequest.c_param.copy() + c_param.update( + { + 'code_verifier': SINGLE_OPTIONAL_STRING + } + ) + +class AuthorizationRequest(message.Message): + c_param = message.AuthorizationRequest.c_param.copy() + c_param.update( + { + 'code_challenge': SINGLE_OPTIONAL_STRING, + 'code_challenge_method': SINGLE_OPTIONAL_STRING + } + ) + + c_allowed_values = message.AuthorizationRequest.c_param.copy() + c_allowed_values.update( + { + "code_challenge_method": [ + "plain", + "S256" + ] + } + ) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 1ff0c8e..d5a34ab 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -6,14 +6,14 @@ from urllib.parse import parse_qsl from urllib.parse import urlparse +import nacl.hash +from nacl.encoding import URLSafeBase64Encoder from jwkest import jws from oic import rndstr from oic.exception import MessageException from oic.oic import PREFERENCE2PROVIDER from oic.oic import scope2claims -from oic.oic.message import AccessTokenRequest from oic.oic.message import AccessTokenResponse -from oic.oic.message import AuthorizationRequest from oic.oic.message import AuthorizationResponse from oic.oic.message import EndSessionRequest from oic.oic.message import EndSessionResponse @@ -24,6 +24,8 @@ from oic.oic.message import RegistrationRequest from oic.oic.message import RegistrationResponse +from .message import AuthorizationRequest +from .message import AccessTokenRequest from .access_token import extract_bearer_token_from_http_request from .client_authentication import verify_client_authentication from .exceptions import AuthorizationError @@ -328,6 +330,49 @@ def handle_token_request(self, request_body, # type: str raise InvalidTokenRequest('grant_type \'{}\' unknown'.format(token_request['grant_type']), token_request, oauth_error='unsupported_grant_type') + def _compute_code_challenge(self, code_verifier): + """ + Given a code verifier compute the code_challenge. This code_challenge is computed as defined (https://datatracker.ietf.org/doc/html/rfc7636#section-4.2): + + code_challenge = BASE64URL-ENCODE(SHA256(ASCII(code_verifier))). + + This shows that the SHA256 of the ascii encoded code_verifier is URLSafe base64 encoded. We have adjusted the encoding to the ISO_8859_1 encoding, + conform to the AppAuth SDK for Android and IOS. Moreover, we remove the base64 padding (=). + + :param code_verifier: the code verifier to transform to the Code Challenge + """ + verifier_hash = nacl.hash.sha256(code_verifier.encode('ISO_8859_1'), encoder=URLSafeBase64Encoder) + return verifier_hash.decode().replace('=', '') + + def _PKCE_verify(self, token_request, authentication_request): + """ + Verify that the given code_verifier complies with the initially supplied code_challenge. + + Only supports the SHA256 code challenge method, plaintext is regarded as unsafe. + + :param cc_cm: the initially supplied Code Challenge Code challenge Method dictionary + :param code_verifier: the code_verfier to check against the code challenge. + :returns: whether the code_verifier is what was expected given the cc_cm + """ + code_challenge_method = authentication_request['code_challenge_method'] + if code_challenge_method == 'plain': + return authentication_request['code_challenge'] == token_request['code_verifier'] + + code_challenge = self._compute_code_challenge(token_request['code_verifier']) + return code_challenge == authentication_request['code_challenge'] + + def _verify_code_exchange_req(self, token_request, authentication_request): + if token_request['client_id'] != authentication_request['client_id']: + logger.info('Authorization code \'%s\' belonging to \'%s\' was used by \'%s\'', + token_request['code'], authentication_request['client_id'], token_request['client_id']) + raise InvalidAuthorizationCode('{} unknown'.format(token_request['code'])) + if token_request['redirect_uri'] != authentication_request['redirect_uri']: + raise InvalidTokenRequest('Invalid redirect_uri: {} != {}'.format(token_request['redirect_uri'], + authentication_request['redirect_uri']), + token_request) + if not self._PKCE_verify(token_request, authentication_request): + raise InvalidTokenRequest('Unexpected Code Verifier: {}'.format(authentication_request['code_challenge'])) + def _do_code_exchange(self, request, # type: Dict[str, str] extra_id_token_claims=None # type: Optional[Union[Mapping[str, Union[str, List[str]]], Callable[[str, str], Mapping[str, Union[str, List[str]]]]] @@ -351,14 +396,7 @@ def _do_code_exchange(self, request, # type: Dict[str, str] authentication_request = self.authz_state.get_authorization_request_for_code(token_request['code']) - if token_request['client_id'] != authentication_request['client_id']: - logger.info('Authorization code \'%s\' belonging to \'%s\' was used by \'%s\'', - token_request['code'], authentication_request['client_id'], token_request['client_id']) - raise InvalidAuthorizationCode('{} unknown'.format(token_request['code'])) - if token_request['redirect_uri'] != authentication_request['redirect_uri']: - raise InvalidTokenRequest('Invalid redirect_uri: {} != {}'.format(token_request['redirect_uri'], - authentication_request['redirect_uri']), - token_request) + self._verify_code_exchange_req(token_request, authentication_request) sub = self.authz_state.get_subject_identifier_for_code(token_request['code']) user_id = self.authz_state.get_user_id_for_subject_identifier(sub) @@ -393,7 +431,7 @@ def _do_token_refresh(self, request): Handles a token request for refreshing an access token (grant_type=refresh_token). :param request: parsed http request parameters :return: a token response containing a new Access Token and possibly a new Refresh Token - :raise InvalidTokenRequest: if the token request is invalid + :raise InvalidTtoken_requestokenRequest: if the token request is invalid """ token_request = RefreshAccessTokenRequest().from_dict(request) try: From 860eb67229d3afd174d6e3f366d62c3296ec8344 Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Thu, 17 Jun 2021 16:50:52 +0200 Subject: [PATCH 02/13] fix wrong value copy --- src/pyop/message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pyop/message.py b/src/pyop/message.py index ed3c113..cf1dac6 100644 --- a/src/pyop/message.py +++ b/src/pyop/message.py @@ -18,7 +18,7 @@ class AuthorizationRequest(message.Message): } ) - c_allowed_values = message.AuthorizationRequest.c_param.copy() + c_allowed_values = message.AuthorizationRequest.c_allowed_values.copy() c_allowed_values.update( { "code_challenge_method": [ From 0fa527c948830c09632f4c68d8c0645edafe599a Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Thu, 17 Jun 2021 16:51:22 +0200 Subject: [PATCH 03/13] add additional check, verifying whether the user requested PKCE validation --- src/pyop/provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index d5a34ab..402407a 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -370,7 +370,7 @@ def _verify_code_exchange_req(self, token_request, authentication_request): raise InvalidTokenRequest('Invalid redirect_uri: {} != {}'.format(token_request['redirect_uri'], authentication_request['redirect_uri']), token_request) - if not self._PKCE_verify(token_request, authentication_request): + if 'code_challenge' in authentication_request and not self._PKCE_verify(token_request, authentication_request): raise InvalidTokenRequest('Unexpected Code Verifier: {}'.format(authentication_request['code_challenge'])) def _do_code_exchange(self, request, # type: Dict[str, str] From dae5a8b1d5c188f8f16885f6b87888f01b045f6b Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Thu, 17 Jun 2021 17:01:08 +0200 Subject: [PATCH 04/13] add types and docstrings --- src/pyop/provider.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 402407a..7ed8a6a 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -330,7 +330,10 @@ def handle_token_request(self, request_body, # type: str raise InvalidTokenRequest('grant_type \'{}\' unknown'.format(token_request['grant_type']), token_request, oauth_error='unsupported_grant_type') - def _compute_code_challenge(self, code_verifier): + def _compute_code_challenge(self, + code_verifier # type: str + ): + # type: (...) -> str """ Given a code verifier compute the code_challenge. This code_challenge is computed as defined (https://datatracker.ietf.org/doc/html/rfc7636#section-4.2): @@ -344,14 +347,18 @@ def _compute_code_challenge(self, code_verifier): verifier_hash = nacl.hash.sha256(code_verifier.encode('ISO_8859_1'), encoder=URLSafeBase64Encoder) return verifier_hash.decode().replace('=', '') - def _PKCE_verify(self, token_request, authentication_request): + def _PKCE_verify(self, + token_request, # type: AccessTokenRequest + authentication_request # type: AuthorizationRequest + ): + # type: (...) -> bool """ Verify that the given code_verifier complies with the initially supplied code_challenge. Only supports the SHA256 code challenge method, plaintext is regarded as unsafe. - :param cc_cm: the initially supplied Code Challenge Code challenge Method dictionary - :param code_verifier: the code_verfier to check against the code challenge. + :param token_request: the token request containing the initially supplied code challenge and code_challenge method. + :param authentication_request: the code_verfier to check against the code challenge. :returns: whether the code_verifier is what was expected given the cc_cm """ code_challenge_method = authentication_request['code_challenge_method'] @@ -361,7 +368,20 @@ def _PKCE_verify(self, token_request, authentication_request): code_challenge = self._compute_code_challenge(token_request['code_verifier']) return code_challenge == authentication_request['code_challenge'] - def _verify_code_exchange_req(self, token_request, authentication_request): + def _verify_code_exchange_req(self, + token_request, # type: AccessTokenRequest + authentication_request # type: AuthorizationRequest + ): + # type: (...) -> None + """ + Verify that the code exchange request is valid. In order to be valid we validate + the expected client and redirect_uri. Finally, if requested by the client, perform a + PKCE check. + + :param token_request: The request asking for a token given a code, and optionally a code_verifier + :param authentication_request: The authentication request belonging to the provided code. + :raises InvalidTokenRequest, InvalidAuthorizationCode: If request is invalid, throw a representing exception. + """ if token_request['client_id'] != authentication_request['client_id']: logger.info('Authorization code \'%s\' belonging to \'%s\' was used by \'%s\'', token_request['code'], authentication_request['client_id'], token_request['client_id']) From eaad3440c3445624615080e34b96ebdfd3d8fa8c Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:12:24 +0200 Subject: [PATCH 05/13] fix wrong nacl import --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 26a7ec3..ad27e12 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ description='OpenID Connect Provider (OP) library in Python.', install_requires=[ 'oic >= 1.2.1', - 'nacl', + 'pynacl', 'pymongo' ] ) From 648637a737b7b81c778898ea55a215947c858b95 Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:17:34 +0200 Subject: [PATCH 06/13] create code_verifier tests --- tests/pyop/test_provider.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/pyop/test_provider.py b/tests/pyop/test_provider.py index 5f85482..c39252d 100644 --- a/tests/pyop/test_provider.py +++ b/tests/pyop/test_provider.py @@ -12,8 +12,9 @@ from oic import rndstr from oic.oauth2.message import MissingRequiredValue, MissingRequiredAttribute from oic.oic import PREFERENCE2PROVIDER -from oic.oic.message import IdToken, AuthorizationRequest, ClaimsRequest, Claims, EndSessionRequest, EndSessionResponse +from oic.oic.message import IdToken, ClaimsRequest, Claims, EndSessionRequest, EndSessionResponse +from pyop.message import AuthorizationRequest from pyop.access_token import BearerTokenError from pyop.authz_state import AuthorizationState from pyop.client_authentication import InvalidClientAuthentication @@ -319,6 +320,19 @@ def test_code_exchange_request(self): self.authn_request_args) @patch('time.time', MOCK_TIME) + def test_pkce_code_exchange_request(self): + self.authorization_code_exchange_request_args['code'] = self.create_authz_code( + { + "code_challenge": "_1f8tFjAtu6D1Df-GOyDPoMjCJdEvaSWsnqR6SLpzsw", + "code_challenge_method": "S256" + } + ) + self.authorization_code_exchange_request_args['code_verifier'] = "SoOEDN-mZKNhw7Mc52VXxyiqTvFB3mod36MwPru253c" + response = self.provider._do_code_exchange(self.authorization_code_exchange_request_args, None) + assert response['access_token'] in self.provider.authz_state.access_tokens + assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, + self.authn_request_args) + @patch('time.time', MOCK_TIME) def test_code_exchange_request_with_claims_requested_in_id_token(self): claims_req = {'claims': ClaimsRequest(id_token=Claims(email=None))} self.authorization_code_exchange_request_args['code'] = self.create_authz_code(extra_auth_req_params=claims_req) @@ -374,6 +388,12 @@ def test_handle_token_request_reject_missing_grant_type(self): with pytest.raises(InvalidTokenRequest): self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) + def test_handle_token_request_reject_invalid_code_verifier(self): + del self.authorization_code_exchange_request_args['grant_type'] + self.authorization_code_exchange_request_args['code'] = self.create_authz_code() + with pytest.raises(InvalidTokenRequest): + self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) + def test_refresh_request(self): self.provider.authz_state = AuthorizationState(HashBasedSubjectIdentifierFactory('salt'), refresh_token_lifetime=600) From 5258c4e5ecb924e4e32ae1b9cbad179b6975f742 Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:40:01 +0200 Subject: [PATCH 07/13] fix inheritance mistake and error response --- src/pyop/message.py | 4 ++-- src/pyop/provider.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/pyop/message.py b/src/pyop/message.py index cf1dac6..2e016b4 100644 --- a/src/pyop/message.py +++ b/src/pyop/message.py @@ -1,7 +1,7 @@ from oic.oauth2.message import SINGLE_OPTIONAL_STRING from oic.oic import message -class AccessTokenRequest(message.Message): +class AccessTokenRequest(message.AccessTokenRequest): c_param = message.AccessTokenRequest.c_param.copy() c_param.update( { @@ -9,7 +9,7 @@ class AccessTokenRequest(message.Message): } ) -class AuthorizationRequest(message.Message): +class AuthorizationRequest(message.AuthorizationRequest): c_param = message.AuthorizationRequest.c_param.copy() c_param.update( { diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 7ed8a6a..8d34af1 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -381,7 +381,7 @@ def _verify_code_exchange_req(self, :param token_request: The request asking for a token given a code, and optionally a code_verifier :param authentication_request: The authentication request belonging to the provided code. :raises InvalidTokenRequest, InvalidAuthorizationCode: If request is invalid, throw a representing exception. - """ + """ if token_request['client_id'] != authentication_request['client_id']: logger.info('Authorization code \'%s\' belonging to \'%s\' was used by \'%s\'', token_request['code'], authentication_request['client_id'], token_request['client_id']) @@ -391,7 +391,8 @@ def _verify_code_exchange_req(self, authentication_request['redirect_uri']), token_request) if 'code_challenge' in authentication_request and not self._PKCE_verify(token_request, authentication_request): - raise InvalidTokenRequest('Unexpected Code Verifier: {}'.format(authentication_request['code_challenge'])) + raise InvalidTokenRequest('Unexpected Code Verifier: {}'.format(authentication_request['code_challenge']), + token_request) def _do_code_exchange(self, request, # type: Dict[str, str] extra_id_token_claims=None From 198814e0cb00cad12c7229706b54219b49212825 Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:40:40 +0200 Subject: [PATCH 08/13] add plaintext and false code_verifier test --- tests/pyop/test_provider.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/pyop/test_provider.py b/tests/pyop/test_provider.py index c39252d..afbb565 100644 --- a/tests/pyop/test_provider.py +++ b/tests/pyop/test_provider.py @@ -332,6 +332,20 @@ def test_pkce_code_exchange_request(self): assert response['access_token'] in self.provider.authz_state.access_tokens assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, self.authn_request_args) + + @patch('time.time', MOCK_TIME) + def test_pkce_code_exchange_request_plaintext(self): + self.authorization_code_exchange_request_args['code'] = self.create_authz_code( + { + "code_challenge": "SoOEDN-mZKNhw7Mc52VXxyiqTvFB3mod36MwPru253c", + "code_challenge_method": "plain" + } + ) + self.authorization_code_exchange_request_args['code_verifier'] = "SoOEDN-mZKNhw7Mc52VXxyiqTvFB3mod36MwPru253c" + response = self.provider._do_code_exchange(self.authorization_code_exchange_request_args, None) + assert response['access_token'] in self.provider.authz_state.access_tokens + assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, + self.authn_request_args) @patch('time.time', MOCK_TIME) def test_code_exchange_request_with_claims_requested_in_id_token(self): claims_req = {'claims': ClaimsRequest(id_token=Claims(email=None))} @@ -389,8 +403,13 @@ def test_handle_token_request_reject_missing_grant_type(self): self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) def test_handle_token_request_reject_invalid_code_verifier(self): - del self.authorization_code_exchange_request_args['grant_type'] - self.authorization_code_exchange_request_args['code'] = self.create_authz_code() + self.authorization_code_exchange_request_args['code'] = self.create_authz_code( + { + "code_challenge": "_1f8tFjAtu6D1Df-GOyDPoMjCJdEvaSWsnqR6SLpzsw=", + "code_challenge_method": "S256" + } + ) + self.authorization_code_exchange_request_args['code_verifier'] = "ThiS Cer_tainly Ain't Valid" with pytest.raises(InvalidTokenRequest): self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) From 23ab1c47f1345f566ee1289e39c5f6fdfda8d3d5 Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:45:09 +0200 Subject: [PATCH 09/13] verify that required request parameters have been supplied --- src/pyop/provider.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 8d34af1..cbcf957 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -361,6 +361,13 @@ def _PKCE_verify(self, :param authentication_request: the code_verfier to check against the code challenge. :returns: whether the code_verifier is what was expected given the cc_cm """ + if not 'code_verifier' in token_request: + return False + + if not 'code_challenge_method' in authentication_request: + raise InvalidTokenRequest("A code_challenge and code_verifier have been supplied" + "but missing code_challenge_method in authentication_request", token_request) + code_challenge_method = authentication_request['code_challenge_method'] if code_challenge_method == 'plain': return authentication_request['code_challenge'] == token_request['code_verifier'] From cdbdbba580da0a02e33f4a65971a19e4fb3414ee Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Thu, 17 Jun 2021 17:45:29 +0200 Subject: [PATCH 10/13] additional testing in case of missing request parameters --- tests/pyop/test_provider.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/pyop/test_provider.py b/tests/pyop/test_provider.py index afbb565..da82699 100644 --- a/tests/pyop/test_provider.py +++ b/tests/pyop/test_provider.py @@ -346,6 +346,7 @@ def test_pkce_code_exchange_request_plaintext(self): assert response['access_token'] in self.provider.authz_state.access_tokens assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, self.authn_request_args) + @patch('time.time', MOCK_TIME) def test_code_exchange_request_with_claims_requested_in_id_token(self): claims_req = {'claims': ClaimsRequest(id_token=Claims(email=None))} @@ -413,6 +414,25 @@ def test_handle_token_request_reject_invalid_code_verifier(self): with pytest.raises(InvalidTokenRequest): self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) + def test_handle_token_request_reject_unsynced_requests(self): + self.authorization_code_exchange_request_args['code'] = self.create_authz_code( + { + "code_challenge": "_1f8tFjAtu6D1Df-GOyDPoMjCJdEvaSWsnqR6SLpzsw=", + "code_challenge_method": "S256" + } + ) + with pytest.raises(InvalidTokenRequest): + self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) + + def test_handle_token_request_reject_missing_code_challenge_method(self): + self.authorization_code_exchange_request_args['code'] = self.create_authz_code( + { + "code_challenge": "_1f8tFjAtu6D1Df-GOyDPoMjCJdEvaSWsnqR6SLpzsw=", + } + ) + with pytest.raises(InvalidTokenRequest): + self.provider.handle_token_request(urlencode(self.authorization_code_exchange_request_args)) + def test_refresh_request(self): self.provider.authz_state = AuthorizationState(HashBasedSubjectIdentifierFactory('salt'), refresh_token_lifetime=600) From 6722733d3cb323b21bfae53066839b4032c0e7dd Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Thu, 17 Jun 2021 17:50:35 +0200 Subject: [PATCH 11/13] swap to the new message instance included in pyop --- README.md | 2 +- src/pyop/authz_state.py | 12 ++++++------ src/pyop/provider.py | 10 +++++----- tests/pyop/test_authz_state.py | 2 +- tests/pyop/test_exceptions.py | 3 +-- tests/pyop/test_util.py | 2 +- 6 files changed, 15 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 02704d3..86187a7 100644 --- a/README.md +++ b/README.md @@ -146,7 +146,7 @@ user data and OpenID Connect claim names. Hence the underlying data source must same names as the [standard claims of OpenID Connect](http://openid.net/specs/openid-connect-core-1_0.html#StandardClaims). ```python -from oic.oic.message import AuthorizationRequest +from pyop.message import AuthorizationRequest from pyop.util import should_fragment_encode diff --git a/src/pyop/authz_state.py b/src/pyop/authz_state.py index 07104e0..acaea9b 100644 --- a/src/pyop/authz_state.py +++ b/src/pyop/authz_state.py @@ -3,8 +3,8 @@ import uuid from oic.extension.message import TokenIntrospectionResponse -from oic.oic.message import AuthorizationRequest +from .message import AuthorizationRequest from .access_token import AccessToken from .exceptions import InvalidAccessToken from .exceptions import InvalidAuthorizationCode @@ -80,7 +80,7 @@ def __init__(self, subject_identifier_factory, authorization_code_db=None, acces self.subject_identifiers = subject_identifier_db if subject_identifier_db is not None else {} def create_authorization_code(self, authorization_request, subject_identifier, scope=None): - # type: (oic.oic.message.AuthorizationRequest, str, Optional[List[str]]) -> str + # type: (AuthorizationRequest, str, Optional[List[str]]) -> str """ Creates an authorization code bound to the authorization request and the authenticated user identified by the subject identifier. @@ -106,7 +106,7 @@ def create_authorization_code(self, authorization_request, subject_identifier, s return authorization_code def create_access_token(self, authorization_request, subject_identifier, scope=None): - # type: (oic.oic.message.AuthorizationRequest, str, Optional[List[str]]) -> se_leg_op.access_token.AccessToken + # type: (AuthorizationRequest, str, Optional[List[str]]) -> se_leg_op.access_token.AccessToken """ Creates an access token bound to the authentication request and the authenticated user identified by the subject identifier. @@ -315,7 +315,7 @@ def get_user_id_for_subject_identifier(self, subject_identifier): raise InvalidSubjectIdentifier('{} unknown'.format(subject_identifier)) def get_authorization_request_for_code(self, authorization_code): - # type: (str) -> oic.oic.message.AuthorizationRequest + # type: (str) -> AuthorizationRequest if authorization_code not in self.authorization_codes: raise InvalidAuthorizationCode('{} unknown'.format(authorization_code)) @@ -323,14 +323,14 @@ def get_authorization_request_for_code(self, authorization_code): self.authorization_codes[authorization_code][self.KEY_AUTHORIZATION_REQUEST]) def get_authorization_request_for_access_token(self, access_token_value): - # type: (str) -> oic.oic.message.AuthorizationRequest + # type: (str) -> if access_token_value not in self.access_tokens: raise InvalidAccessToken('{} unknown'.format(access_token_value)) return AuthorizationRequest().from_dict(self.access_tokens[access_token_value][self.KEY_AUTHORIZATION_REQUEST]) def get_subject_identifier_for_code(self, authorization_code): - # type: (str) -> oic.oic.message.AuthorizationRequest + # type: (str) -> AuthorizationRequest if authorization_code not in self.authorization_codes: raise InvalidAuthorizationCode('{} unknown'.format(authorization_code)) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index cbcf957..4e6160e 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -83,7 +83,7 @@ def __init__(self, signing_key, configuration_information, authz_state, clients, self.userinfo = userinfo self.id_token_lifetime = id_token_lifetime - self.authentication_request_validators = [] # type: List[Callable[[oic.oic.message.AuthorizationRequest], Boolean]] + self.authentication_request_validators = [] # type: List[Callable[[AuthorizationRequest], Boolean]] self.authentication_request_validators.append(authorization_request_verify) self.authentication_request_validators.append( functools.partial(client_id_is_known, self)) @@ -116,7 +116,7 @@ def jwks(self): return {'keys': keys} def parse_authentication_request(self, request_body, http_headers=None): - # type: (str, Optional[Mapping[str, str]]) -> oic.oic.message.AuthorizationRequest + # type: (str, Optional[Mapping[str, str]]) -> AuthorizationRequest """ Parses and verifies an authentication request. @@ -132,7 +132,7 @@ def parse_authentication_request(self, request_body, http_headers=None): logger.debug('parsed authentication_request: %s', auth_req) return auth_req - def authorize(self, authentication_request, # type: oic.oic.message.AuthorizationRequest + def authorize(self, authentication_request, # type: AuthorizationRequest user_id, # type: str extra_id_token_claims=None # type: Optional[Union[Mapping[str, Union[str, List[str]]], Callable[[str, str], Mapping[str, Union[str, List[str]]]]] @@ -218,7 +218,7 @@ def _create_subject_identifier(self, user_id, client_id, redirect_uri): return self.authz_state.get_subject_identifier(subject_type, user_id, sector_identifier) def _get_requested_claims_in(self, authentication_request, response_method): - # type (oic.oic.message.AuthorizationRequest, str) -> Mapping[str, Optional[Mapping[str, Union[str, List[str]]]] + # type (AuthorizationRequest, str) -> Mapping[str, Optional[Mapping[str, Union[str, List[str]]]] """ Parses any claims requested using the 'claims' request parameter, see @@ -286,7 +286,7 @@ def _create_signed_id_token(self, return id_token.to_jwt([self.signing_key], alg) def _check_subject_identifier_matches_requested(self, authentication_request, sub): - # type (oic.message.AuthorizationRequest, str) -> None + # type (AuthorizationRequest, str) -> None """ Verifies the subject identifier against any requested subject identifier using the claims request parameter. :param authentication_request: authentication request diff --git a/tests/pyop/test_authz_state.py b/tests/pyop/test_authz_state.py index 2eb9ade..2812973 100644 --- a/tests/pyop/test_authz_state.py +++ b/tests/pyop/test_authz_state.py @@ -4,8 +4,8 @@ from unittest.mock import patch, Mock import pytest -from oic.oic.message import AuthorizationRequest +from pyop.message import AuthorizationRequest from pyop.authz_state import AccessToken, InvalidScope from pyop.authz_state import AuthorizationState from pyop.exceptions import InvalidSubjectIdentifier, InvalidAccessToken, InvalidAuthorizationCode, InvalidRefreshToken diff --git a/tests/pyop/test_exceptions.py b/tests/pyop/test_exceptions.py index 64fb050..7ec5112 100644 --- a/tests/pyop/test_exceptions.py +++ b/tests/pyop/test_exceptions.py @@ -1,7 +1,6 @@ from urllib.parse import urlparse, parse_qsl -from oic.oic.message import AuthorizationRequest - +from pyop.message import AuthorizationRequest from pyop.exceptions import InvalidAuthenticationRequest diff --git a/tests/pyop/test_util.py b/tests/pyop/test_util.py index be22121..5251838 100644 --- a/tests/pyop/test_util.py +++ b/tests/pyop/test_util.py @@ -1,6 +1,6 @@ import pytest -from oic.oic.message import AuthorizationRequest +from pyop.message import AuthorizationRequest from pyop.util import should_fragment_encode From 173d3af94010745bb11492c2f6ffdb07622de311 Mon Sep 17 00:00:00 2001 From: maxxiefjv Date: Mon, 21 Jun 2021 09:19:37 +0200 Subject: [PATCH 12/13] Update provider.py --- src/pyop/provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 4e6160e..656fa28 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -459,7 +459,7 @@ def _do_token_refresh(self, request): Handles a token request for refreshing an access token (grant_type=refresh_token). :param request: parsed http request parameters :return: a token response containing a new Access Token and possibly a new Refresh Token - :raise InvalidTtoken_requestokenRequest: if the token request is invalid + :raise InvalidTokenRequest: if the token request is invalid """ token_request = RefreshAccessTokenRequest().from_dict(request) try: From 1c642b858347d10e797a8cdf9a57e323ba2333d7 Mon Sep 17 00:00:00 2001 From: Max Vasterd Date: Mon, 9 Aug 2021 16:47:57 +0200 Subject: [PATCH 13/13] remove custom implementation of code challenge computation, reuse oic function instead. Remove nacl dependency --- setup.py | 1 - src/pyop/provider.py | 30 +++++------------------------- tests/pyop/test_provider.py | 14 -------------- 3 files changed, 5 insertions(+), 40 deletions(-) diff --git a/setup.py b/setup.py index ad27e12..da943f6 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,6 @@ description='OpenID Connect Provider (OP) library in Python.', install_requires=[ 'oic >= 1.2.1', - 'pynacl', 'pymongo' ] ) diff --git a/src/pyop/provider.py b/src/pyop/provider.py index 656fa28..e69140c 100644 --- a/src/pyop/provider.py +++ b/src/pyop/provider.py @@ -6,8 +6,6 @@ from urllib.parse import parse_qsl from urllib.parse import urlparse -import nacl.hash -from nacl.encoding import URLSafeBase64Encoder from jwkest import jws from oic import rndstr from oic.exception import MessageException @@ -23,6 +21,7 @@ from oic.oic.message import RefreshAccessTokenRequest from oic.oic.message import RegistrationRequest from oic.oic.message import RegistrationResponse +from oic.extension.provider import Provider as OICProviderExtensions from .message import AuthorizationRequest from .message import AccessTokenRequest @@ -330,23 +329,6 @@ def handle_token_request(self, request_body, # type: str raise InvalidTokenRequest('grant_type \'{}\' unknown'.format(token_request['grant_type']), token_request, oauth_error='unsupported_grant_type') - def _compute_code_challenge(self, - code_verifier # type: str - ): - # type: (...) -> str - """ - Given a code verifier compute the code_challenge. This code_challenge is computed as defined (https://datatracker.ietf.org/doc/html/rfc7636#section-4.2): - - code_challenge = BASE64URL-ENCODE(SHA256(ASCII(code_verifier))). - - This shows that the SHA256 of the ascii encoded code_verifier is URLSafe base64 encoded. We have adjusted the encoding to the ISO_8859_1 encoding, - conform to the AppAuth SDK for Android and IOS. Moreover, we remove the base64 padding (=). - - :param code_verifier: the code verifier to transform to the Code Challenge - """ - verifier_hash = nacl.hash.sha256(code_verifier.encode('ISO_8859_1'), encoder=URLSafeBase64Encoder) - return verifier_hash.decode().replace('=', '') - def _PKCE_verify(self, token_request, # type: AccessTokenRequest authentication_request # type: AuthorizationRequest @@ -368,12 +350,10 @@ def _PKCE_verify(self, raise InvalidTokenRequest("A code_challenge and code_verifier have been supplied" "but missing code_challenge_method in authentication_request", token_request) - code_challenge_method = authentication_request['code_challenge_method'] - if code_challenge_method == 'plain': - return authentication_request['code_challenge'] == token_request['code_verifier'] - - code_challenge = self._compute_code_challenge(token_request['code_verifier']) - return code_challenge == authentication_request['code_challenge'] + # OIC Provider extension returns either a boolean or Response object containing an error. To support + # stricter typing guidelines, return if True. Error handling support should be in encapsulating function. + return OICProviderExtensions.verify_code_challenge(token_request['code_verifier'], + authentication_request['code_challenge'], authentication_request['code_challenge_method']) == True def _verify_code_exchange_req(self, token_request, # type: AccessTokenRequest diff --git a/tests/pyop/test_provider.py b/tests/pyop/test_provider.py index da82699..9c20fa0 100644 --- a/tests/pyop/test_provider.py +++ b/tests/pyop/test_provider.py @@ -333,20 +333,6 @@ def test_pkce_code_exchange_request(self): assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, self.authn_request_args) - @patch('time.time', MOCK_TIME) - def test_pkce_code_exchange_request_plaintext(self): - self.authorization_code_exchange_request_args['code'] = self.create_authz_code( - { - "code_challenge": "SoOEDN-mZKNhw7Mc52VXxyiqTvFB3mod36MwPru253c", - "code_challenge_method": "plain" - } - ) - self.authorization_code_exchange_request_args['code_verifier'] = "SoOEDN-mZKNhw7Mc52VXxyiqTvFB3mod36MwPru253c" - response = self.provider._do_code_exchange(self.authorization_code_exchange_request_args, None) - assert response['access_token'] in self.provider.authz_state.access_tokens - assert_id_token_base_claims(response['id_token'], self.provider.signing_key, self.provider, - self.authn_request_args) - @patch('time.time', MOCK_TIME) def test_code_exchange_request_with_claims_requested_in_id_token(self): claims_req = {'claims': ClaimsRequest(id_token=Claims(email=None))}