From 1d4b3ed6ffb8d794992e6ebba4fec0ca310e4eaf Mon Sep 17 00:00:00 2001 From: Guillaume Pujol Date: Tue, 27 Jun 2023 16:27:19 +0200 Subject: [PATCH] hopefully last fixes before v1.2.0 (#17) * fix ID Token validation when signature verification key does not contain an "alg" param * fix validation of empty at_hash, c_hash, s_hash values * do not validate the ID Token if there is no ID Token --- requests_oauth2client/client.py | 2 +- requests_oauth2client/tokens.py | 8 ++--- tests/unit_tests/test_oidc.py | 53 +++++++++++++++++++-------------- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/requests_oauth2client/client.py b/requests_oauth2client/client.py index ddddef5..14c73e5 100644 --- a/requests_oauth2client/client.py +++ b/requests_oauth2client/client.py @@ -364,7 +364,7 @@ def authorization_code( data = dict(grant_type="authorization_code", code=code, **token_kwargs) token = self.token_request(data, **requests_kwargs) - if isinstance(azr, AuthorizationResponse) and validate: + if validate and token.id_token and isinstance(azr, AuthorizationResponse): token.validate_id_token(self, azr) return token diff --git a/requests_oauth2client/tokens.py b/requests_oauth2client/tokens.py index 7f6f282..8af2976 100644 --- a/requests_oauth2client/tokens.py +++ b/requests_oauth2client/tokens.py @@ -222,7 +222,7 @@ def validate_id_token( # noqa: C901 id_token.verify_signature(verification_jwk, alg=id_token_alg) alg_class = jwskate.select_alg_class( - verification_jwk.SIGNATURE_ALGORITHMS, jwk_alg=verification_jwk.alg + verification_jwk.SIGNATURE_ALGORITHMS, jwk_alg=id_token_alg ) if alg_class == jwskate.EdDsa: if verification_jwk.crv == "Ed25519": @@ -244,7 +244,7 @@ def validate_id_token( # noqa: C901 ) at_hash = id_token.get_claim("at_hash") - if at_hash: + if at_hash is not None: expected_at_hash = hash_function(self.access_token) if expected_at_hash != at_hash: raise InvalidIdToken( @@ -252,7 +252,7 @@ def validate_id_token( # noqa: C901 ) c_hash = id_token.get_claim("c_hash") - if c_hash: + if c_hash is not None: expected_c_hash = hash_function(azr.code) if expected_c_hash != c_hash: raise InvalidIdToken( @@ -260,7 +260,7 @@ def validate_id_token( # noqa: C901 ) s_hash = id_token.get_claim("s_hash") - if s_hash: + if s_hash is not None: if azr.state is None: raise InvalidIdToken( "ID Token has a 's_hash' claim but no state was included in the request." diff --git a/tests/unit_tests/test_oidc.py b/tests/unit_tests/test_oidc.py index ecc4b6f..e9a8f39 100644 --- a/tests/unit_tests/test_oidc.py +++ b/tests/unit_tests/test_oidc.py @@ -23,41 +23,49 @@ MissingIdToken, ) -ID_TOKEN = ( - "eyJraWQiOiIxZTlnZGs3IiwiYWxnIjoiUlMyNTYifQ.ewogIml" - "zcyI6ICJodHRwOi8vc2VydmVyLmV4YW1wbGUuY29tIiwKICJzdWIiOiAiMjQ" - "4Mjg5NzYxMDAxIiwKICJhdWQiOiAiczZCaGRSa3F0MyIsCiAibm9uY2UiOiA" - "ibi0wUzZfV3pBMk1qIiwKICJleHAiOiAxMzExMjgxOTcwLAogImlhdCI6IDE" - "zMTEyODA5NzAsCiAiY19oYXNoIjogIkxEa3RLZG9RYWszUGswY25YeENsdEE" - "iCn0.XW6uhdrkBgcGx6zVIrCiROpWURs-4goO1sKA4m9jhJIImiGg5muPUcN" - "egx6sSv43c5DSn37sxCRrDZZm4ZPBKKgtYASMcE20SDgvYJdJS0cyuFw7Ijp" - "_7WnIjcrl6B5cmoM6ylCvsLMwkoQAxVublMwH10oAxjzD6NEFsu9nipkszWh" - "sPePf_rM4eMpkmCbTzume-fzZIi5VjdWGGEmzTg32h3jiex-r5WTHbj-u5HL" - "7u_KP3rmbdYNzlzd1xWRYTUs4E8nOTgzAUwvwXkIQhOh5TPcSMBYy6X3E7-_" - "gr9Ue6n4ND7hTFhtjYs3cjNKIA08qm5cpVYFMFMG6PkhzLQ" -) - @freeze_time("2011-07-21 20:42:55") @pytest.mark.parametrize( - "kwargs, at_hash", + "kwargs, at_hash, c_hash, s_hash", ( - ({"alg": "PS256"}, "xsZZrUssMXjL3FBlzoSh2g"), - ({"alg": "PS384"}, "adt46pcdiB-l6eTNifgoVM-5AIJAxq84"), - ({"alg": "PS512"}, "p2LHG4H-8pYDc0hyVOo3iIHvZJUqe9tbj3jESOuXbkY"), + ( + {"alg": "PS256"}, + "xsZZrUssMXjL3FBlzoSh2g", + "LDktKdoQak3Pk0cnXxCltA", + "GAsHrlqowzjqTW8xW7lyMw", + ), + ( + {"alg": "PS384"}, + "adt46pcdiB-l6eTNifgoVM-5AIJAxq84", + "Mq-knyaEMtWGfnBi2POEZb1kiLx10_DF", + "IIXrx5-tK3fM7Q80_DbXTWRb6ty48rOd", + ), + ( + {"alg": "PS512"}, + "p2LHG4H-8pYDc0hyVOo3iIHvZJUqe9tbj3jESOuXbkY", + "E9z1C-c0Az4eTEzE0Nm3OQ3BS2BhMgxuP7x5JAQj1_4", + "aVrO6_zIGuPg0pvBhlmB9jnpmFoY6MXEt1nJeHp1pmI", + ), ( {"alg": "EdDSA", "crv": "Ed448"}, "sB_U72jyb0WgtX8TsVoqJnm6CD295W9gfSDRxkilB3LAL7REi9JYutRW_s1yE4lD8cOfMZf83gi4", + "07UgYISe6yaAzmTIBr_f2vchFCIs6bAGk1-36iEH00fq4B3eBih5g0r_kEPHpuYLqbXOq7gDBVpr", + "ZPaPdOYbQ2dUGsQZHaSIcIveQMwWh4yG8lMT9Cfa_cSKSO8KGjx4rqI4zwmAfYJ6bPIxZWeUwvUn", ), ), ) -def test_validate_id_token(kwargs: Dict[str, str], at_hash: str) -> None: +def test_validate_id_token( + kwargs: Dict[str, str], at_hash: str, c_hash: str, s_hash: str +) -> None: signing_key = jwskate.Jwk.generate(**kwargs).with_kid_thumbprint() + jwks = signing_key.public_jwk().minimize().as_jwks() client_id = "s6BhdRkqt3" access_token = ( "YmJiZTAwYmYtMzgyOC00NzhkLTkyOTItNjJjNDM3MGYzOWIy9sFhvH8K_x8UIHj1osisS57f5DduL" ) code = "Qcb0Orv1zh30vL1MPRsbm-diHiMwcLyZvn1arpZv-Jxf_11jnpEX3Tgfvk" + state = "qu2pNLwFWBjakH2x4OxivEVtjKiM27SHrPdY3McJN4g" + nonce = "n-0S6_WzA2Mj" id_token = IdToken.sign( { @@ -67,8 +75,9 @@ def test_validate_id_token(kwargs: Dict[str, str], at_hash: str) -> None: "nonce": nonce, "exp": 1311281970, "iat": 1311280970, - # "c_hash": BinaPy(code).to("sha256")[:16].to("b64u").ascii(), + "c_hash": c_hash, "at_hash": at_hash, + "s_hash": s_hash, "auth_time": 1311280970, }, signing_key, @@ -81,10 +90,10 @@ def test_validate_id_token(kwargs: Dict[str, str], at_hash: str) -> None: client=OAuth2Client( "https://myas.local/token", client_id=client_id, - authorization_server_jwks=signing_key.public_jwk().as_jwks(), + authorization_server_jwks=jwks, id_token_signed_response_alg=kwargs["alg"], ), - azr=AuthorizationResponse(code=code, nonce=nonce, max_age=0), + azr=AuthorizationResponse(code=code, nonce=nonce, max_age=0, state=state), )