From 3cd886ce97ee77e843680ecd9592633bb5e85e2e Mon Sep 17 00:00:00 2001 From: Andreas Perhab Date: Tue, 9 Aug 2022 14:10:41 +0200 Subject: [PATCH 1/3] [FIX] auth_oidc: set user_id for auth_oauth --- auth_oidc/models/res_users.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/auth_oidc/models/res_users.py b/auth_oidc/models/res_users.py index 56e92fbdc9..231c0cd519 100644 --- a/auth_oidc/models/res_users.py +++ b/auth_oidc/models/res_users.py @@ -70,7 +70,12 @@ def auth_oauth(self, provider, params): ).json() validation.update(data) # required check - if not validation.get("user_id"): + if "sub" in validation and "user_id" not in validation: + # set user_id for auth_oauth, user_id is not an OpenID Connect standard + # claim: + # https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims + validation["user_id"] = validation["sub"] + elif not validation.get("user_id"): _logger.error("user_id claim not found in id_token (after mapping).") raise AccessDenied() # retrieve and sign in user From 0d15649acb159fa1ba834837f9597922fd91daa9 Mon Sep 17 00:00:00 2001 From: Andreas Perhab Date: Tue, 9 Aug 2022 14:07:28 +0200 Subject: [PATCH 2/3] [FIX] auth_oidc: support keys without kid --- auth_oidc/models/auth_oauth_provider.py | 44 ++-- auth_oidc/tests/test_auth_oidc_auth_code.py | 245 ++++++++++++++++++-- 2 files changed, 254 insertions(+), 35 deletions(-) diff --git a/auth_oidc/models/auth_oauth_provider.py b/auth_oidc/models/auth_oauth_provider.py index 8f4cd9a853..40478c5932 100644 --- a/auth_oidc/models/auth_oauth_provider.py +++ b/auth_oidc/models/auth_oauth_provider.py @@ -12,6 +12,7 @@ try: from jose import jwt + from jose.exceptions import JWTError, JWSError except ImportError: logging.getLogger(__name__).debug("jose library not installed") @@ -51,14 +52,15 @@ class AuthOauthProvider(models.Model): ) @tools.ormcache("self.jwks_uri", "kid") - def _get_key(self, kid): + def _get_keys(self, kid): r = requests.get(self.jwks_uri) r.raise_for_status() response = r.json() - for key in response["keys"]: - if key["kid"] == kid: - return key - return {} + # the keys returned here should follow + # JWS Notes on Key Selection + # https://datatracker.ietf.org/doc/html/draft-ietf-jose-json-web-signature#appendix-D + return [key for key in response["keys"] + if kid is None or key.get("kid", None) == kid] def _map_token_values(self, res): if self.token_map: @@ -72,19 +74,31 @@ def _parse_id_token(self, id_token, access_token): self.ensure_one() res = {} header = jwt.get_unverified_header(id_token) - res.update( - jwt.decode( - id_token, - self._get_key(header.get("kid")), - algorithms=["RS256"], - audience=self.client_id, - access_token=access_token, - ) - ) - + res.update(self._decode_id_token(access_token, id_token, header.get("kid"))) res.update(self._map_token_values(res)) return res + def _decode_id_token(self, access_token, id_token, kid): + keys = self._get_keys(kid) + if len(keys) > 1 and kid is None: + # https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.10.1 + # If there are multiple keys in the referenced JWK Set document, a kid + # value MUST be provided in the JOSE Header. + raise JWTError("OpenID Connect requires kid to be set if there is more" + " than one key in the JWKS") + error = None + # we accept multiple keys with the same kid in case a key gets rotated. + for key in keys: + try: + values = jwt.decode(id_token, key, algorithms=["RS256"], + audience=self.client_id, access_token=access_token) + return values + except (JWTError, JWSError) as e: + error = e + if error: + raise error + return {} + class AuthOauthProviderGroupLine(models.Model): _name = 'auth.oauth.provider.group_line' diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index c3c89c37cf..baebd6d38c 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -8,9 +8,12 @@ from cryptography.hazmat.primitives.asymmetric import rsa from urllib.parse import parse_qs, urlparse from jose import jwt +from jose.exceptions import JWTError +from jose.utils import long_to_base64 import odoo from odoo.tests import common +from odoo.exceptions import AccessDenied from odoo.addons.auth_oidc.controllers.main import OpenIDLogin from odoo.addons.website.tools import MockRequest as _MockRequest @@ -30,20 +33,34 @@ class TestAuthOIDCAuthorizationCodeFlow(common.HttpCase): @classmethod def setUpClass(cls): super().setUpClass() - cls.rsa_key = rsa.generate_private_key( + cls.rsa_key_pem, cls.rsa_key_public_pem, cls.rsa_key_public_jwk =\ + cls._generate_key() + _, cls.second_key_public_pem, _ = cls._generate_key() + + @staticmethod + def _generate_key(): + rsa_key = rsa.generate_private_key( public_exponent=65537, key_size=4096, ) - cls.rsa_key_pem = cls.rsa_key.private_bytes( + rsa_key_pem = rsa_key.private_bytes( serialization.Encoding.PEM, serialization.PrivateFormat.TraditionalOpenSSL, serialization.NoEncryption(), ).decode("utf8") - cls.rsa_key_public = cls.rsa_key.public_key() - cls.rsa_key_public_pem = cls.rsa_key_public.public_bytes( + rsa_key_public = rsa_key.public_key() + rsa_key_public_pem = rsa_key_public.public_bytes( serialization.Encoding.PEM, serialization.PublicFormat.SubjectPublicKeyInfo, ).decode("utf8") + jwk = { + # https://datatracker.ietf.org/doc/html/rfc7518#section-6.1 + "kty": "RSA", + "use": "sig", + "n": long_to_base64(rsa_key_public.public_numbers().n).decode("utf-8"), + "e": long_to_base64(rsa_key_public.public_numbers().e).decode("utf-8"), + } + return rsa_key_pem, rsa_key_public_pem, jwk def setUp(self): super().setUp() @@ -74,42 +91,65 @@ def test_auth_link(self): self.assertTrue(params["state"]) self.assertEqual(params["redirect_uri"], [BASE_URL + "/auth_oauth/signin"]) - def test_group_expression(self): - """Test that group expressions evaluate correctly""" - group_line = self.env.ref('auth_oidc.local_keycloak').group_line_ids[:1] - group_line.expression = 'token["test"]["test"] == 1' - self.assertFalse(group_line._eval_expression(self.env.user, {})) - - @responses.activate - def test_login(self): - """Test that login works""" + def _prepare_login_test_user(self): user = self.env.ref("base.user_demo") user.write({ 'oauth_provider_id': self.provider_rec.id, 'oauth_uid': user.login, }) + return user + + def _prepare_login_test_responses(self, + access_token="42", + id_token_body=None, + id_token_headers=None, + keys=None): + if id_token_body is None: + id_token_body = {} + if id_token_headers is None: + id_token_headers = {"kid": "the_key_id"} responses.add( responses.POST, "http://localhost:8080/auth/realms/master/protocol/openid-connect/token", json={ - "access_token": "42", + "access_token": access_token, "id_token": jwt.encode( - {"user_id": user.login}, + id_token_body, self.rsa_key_pem, algorithm="RS256", - headers={"kid": "the_key_id"}, + headers=id_token_headers, ), }, ) + if keys is None: + if "kid" in id_token_headers: + keys = [{ + "kid": "the_key_id", + "keys": [self.rsa_key_public_pem], + }] + else: + keys = [{ + "keys": [self.rsa_key_public_pem], + }] responses.add( responses.GET, "http://localhost:8080/auth/realms/master/protocol/openid-connect/certs", json={ - "keys": [{ - "kid": "the_key_id", - "keys": [self.rsa_key_public_pem], - }], + "keys": keys, }, ) + + def test_group_expression(self): + """Test that group expressions evaluate correctly""" + group_line = self.env.ref('auth_oidc.local_keycloak').group_line_ids[:1] + group_line.expression = 'token["test"]["test"] == 1' + self.assertFalse(group_line._eval_expression(self.env.user, {})) + + @responses.activate + def test_login(self): + """Test that login works""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses(id_token_body={"user_id": user.login}) + params = {"state": json.dumps({})} with MockRequest(self.env): db, login, token = self.env["res.users"].auth_oauth( @@ -117,3 +157,168 @@ def test_login(self): ) self.assertEqual(token, "42") self.assertEqual(login, user.login) + + @responses.activate + def test_login_without_kid(self): + """Test that login works when ID Token has no kid in header""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + id_token_headers={}, + access_token=chr(42) + ) + + params = {"state": json.dumps({})} + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, params, + ) + self.assertEqual(token, "*") + self.assertEqual(login, user.login) + + @responses.activate + def test_login_with_sub_claim(self): + """Test that login works when ID Token contains only standard claims""" + self.provider_rec.token_map = False + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"sub": user.login}, + access_token="1764" + ) + + params = {"state": json.dumps({})} + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, params, + ) + self.assertEqual(token, "1764") + self.assertEqual(login, user.login) + + @responses.activate + def test_login_without_kid_multiple_keys_in_jwks(self): + """ + Test that login fails if no kid is provided in ID Token and JWKS has multiple + keys + """ + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + id_token_headers={}, + access_token="6*7", + keys=[{ + "kid": "other_key_id", + "keys": [self.second_key_public_pem], + }, { + "kid": "the_key_id", + "keys": [self.rsa_key_public_pem], + }], + ) + + with self.assertRaises(JWTError, + msg="OpenID Connect requires kid to be set if there is" + " more than one key in the JWKS"): + with MockRequest(self.env): + self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + + @responses.activate + def test_login_without_matching_key(self): + """Test that login fails if no matching key can be found""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + id_token_headers={}, + access_token="168/4", + keys=[{ + "kid": "other_key_id", + "keys": [self.second_key_public_pem], + }], + ) + + with self.assertRaises(JWTError): + with MockRequest(self.env): + self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + + @responses.activate + def test_login_without_any_key(self): + """Test that login fails if no key is provided by JWKS""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + id_token_headers={}, + access_token="168/4", + keys=[], + ) + + with self.assertRaises(AccessDenied): + with MockRequest(self.env): + self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + + @responses.activate + def test_login_with_multiple_keys_in_jwks(self): + """Test that login works with multiple keys present in jwks""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + access_token="2*3*7", + keys=[{ + "kid": "other_key_id", + "keys": [self.second_key_public_pem], + }, { + "kid": "the_key_id", + "keys": [self.rsa_key_public_pem], + }], + ) + + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + self.assertEqual(token, "2*3*7") + self.assertEqual(login, user.login) + + @responses.activate + def test_login_with_multiple_keys_in_jwks_same_kid(self): + """Test that login works with multiple keys with the same kid present in jwks""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + access_token="84/2", + keys=[{ + "kid": "the_key_id", + "keys": [self.second_key_public_pem], + }, { + "kid": "the_key_id", + "keys": [self.rsa_key_public_pem], + }], + ) + + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + self.assertEqual(token, "84/2") + self.assertEqual(login, user.login) + + @responses.activate + def test_login_with_jwk_format(self): + """Test that login works with proper jwks format""" + user = self._prepare_login_test_user() + self.rsa_key_public_jwk["kid"] = "the_key_id" + self._prepare_login_test_responses( + id_token_body={"user_id": user.login}, + keys=[self.rsa_key_public_jwk], + access_token="122/3" + ) + + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, {"state": json.dumps({})}, + ) + self.assertEqual(token, "122/3") + self.assertEqual(login, user.login) From 00dc64884fcb88f977fffd3875d6e8339db3bb9c Mon Sep 17 00:00:00 2001 From: Andreas Perhab Date: Thu, 3 Nov 2022 10:09:23 +0100 Subject: [PATCH 3/3] [IMP] auth_oidc: update documentation for 12.0.1.2.0 --- auth_oidc/__manifest__.py | 2 +- auth_oidc/readme/CONTRIBUTORS.rst | 1 + auth_oidc/readme/HISTORY.rst | 21 +++++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/auth_oidc/__manifest__.py b/auth_oidc/__manifest__.py index 1778440ec1..44a58bb683 100644 --- a/auth_oidc/__manifest__.py +++ b/auth_oidc/__manifest__.py @@ -4,7 +4,7 @@ { "name": "Authentication OpenID Connect", - "version": "12.0.1.1.0", + "version": "12.0.1.2.0", "license": "AGPL-3", "author": ( "ICTSTUDIO, André Schenkels, " diff --git a/auth_oidc/readme/CONTRIBUTORS.rst b/auth_oidc/readme/CONTRIBUTORS.rst index d721552d86..95b87f0a78 100644 --- a/auth_oidc/readme/CONTRIBUTORS.rst +++ b/auth_oidc/readme/CONTRIBUTORS.rst @@ -1,2 +1,3 @@ * Alexandre Fayolle * Stéphane Bidoul +* Andreas Perhab diff --git a/auth_oidc/readme/HISTORY.rst b/auth_oidc/readme/HISTORY.rst index f831ce0e6d..e4442847a9 100644 --- a/auth_oidc/readme/HISTORY.rst +++ b/auth_oidc/readme/HISTORY.rst @@ -1,3 +1,24 @@ +12.0.1.2.0 2022-11-22 +~~~~~~~~~~~~~~~~~~~~~ + +* Fix handling OpenID Connect responses without custom mapping by using the ``sub`` claim as user id +* Fix handling OpenID Connect ID Tokens without Key ID (``kid``) + +12.0.1.1.0 2022-11-22 +~~~~~~~~~~~~~~~~~~~~~ + +* Enable allowing assigning groups from token claims + +12.0.1.0.1 2022-02-28 +~~~~~~~~~~~~~~~~~~~~~ + +* Updated readme and pot + +12.0.1.0.0 2022-02-12 +~~~~~~~~~~~~~~~~~~~~~ + +* Backport to Odoo 12 + 13.0.1.0.0 2020-04-10 ~~~~~~~~~~~~~~~~~~~~~