[12.0] auth_oidc: fixes #436

ap-wtioit · 2022-11-03T09:11:43Z

enable handling Open ID Connect without a custom mapping in Odoo
enable handling Open ID Connect ID Tokens without key id

We are running this code for a few months on our servers now, so it was time to try to contribute back. The plan is to forward port it to 13.0, 14.0, 15.0, 16.0 (as we are running it for those versions too)

Info @wt-io-it

OCA-git-bot · 2022-11-03T09:11:47Z

Hi @sbidoul,
some modules you are maintaining are being modified, check this out!

ap-wtioit · 2022-11-03T09:19:15Z

Tests failing for travis_install_nightly because somehow they are still running with python3.5 and pylint-plugin-utils requires: Python >=3.6.2:

DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.

DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.

ERROR: Could not find a version that satisfies the requirement pylint-plugin-utils==0.7 (from -r /home/travis/maintainer-quality-tools/travis/pylint_odoo_requirements.txt (line 1)) (from versions: 0.1, 0.1.1, 0.2, 0.2.1, 0.2.2, 0.2.3, 0.2.4, 0.2.6, 0.3, 0.4, 0.5, 0.6)

ERROR: No matching distribution found for pylint-plugin-utils==0.7 (from -r /home/travis/maintainer-quality-tools/travis/pylint_odoo_requirements.txt (line 1))

The command "travis_install_nightly" failed and exited with 1 during .

* Add module auth_signup_verify_email. * Import module following guidelines. * README typos. * OCA Transbot updated translations from Transifex * Credit creator. * author name correction * [9.0][MIG][auth_signup_verify_email] Migration. Migrate to v9. * [FIX] auth_signup_verify_email: Python library requirement * Add tests, fix xml tags, fix credits. * Fix test.

sbidoul · 2022-11-03T17:25:36Z

Trying to fix ci in #438

sbidoul

Thanks for this contrib. I've a couple of questions.

sbidoul · 2022-11-03T17:27:35Z

auth_oidc/readme/HISTORY.rst

+12.0.2.0.0 2022-11-03
+~~~~~~~~~~~~~~~~~~~~~
+
+* Fix handling OpenID Connect responses without custom mapping


Suggested change

* Fix handling OpenID Connect responses without custom mapping

* Fix handling OpenID Connect responses without custom mapping by using the ``sub`` claim as user id

thanks, added

sbidoul · 2022-11-03T17:28:30Z

auth_oidc/readme/HISTORY.rst

+~~~~~~~~~~~~~~~~~~~~~
+
+* Fix handling OpenID Connect responses without custom mapping
+* Fix handling OpenID Connect ID Tokens without key ids


I'm not sure what that means. Do you have a link to the standard that talks about this?

When a JWKS has only one key it is not required for the signed ID Token to contain a kid parameter. Before the fix one would get an error like this in this case:

odoo_1 | 2022-11-04 13:16:12,248 1 ERROR devel odoo.addons.auth_oauth.controllers.main: OAuth2: 'kid' odoo_1 | Traceback (most recent call last): odoo_1 | File "/opt/odoo/custom/src/odoo/odoo/tools/cache.py", line 88, in lookup odoo_1 | r = d[key] odoo_1 | File "/opt/odoo/custom/src/odoo/odoo/tools/func.py", line 69, in wrapper odoo_1 | return func(self, *args, **kwargs) odoo_1 | File "/opt/odoo/custom/src/odoo/odoo/tools/lru.py", line 44, in __getitem__ odoo_1 | a = self.d[obj].me odoo_1 | KeyError: ('auth.oauth.provider', <function AuthOauthProvider._get_key at 0x7f4d8d0d5950>, 'http://redacted.doodba/redacted/jwks', None) odoo_1 | odoo_1 | During handling of the above exception, another exception occurred: odoo_1 | odoo_1 | Traceback (most recent call last): odoo_1 | File "/opt/odoo/auto/addons/auth_oauth/controllers/main.py", line 170, in signin odoo_1 | credentials = env['res.users'].sudo().auth_oauth(provider, kw) odoo_1 | File "/opt/odoo/auto/addons/auth_oidc/models/res_users.py", line 65, in auth_oauth odoo_1 | validation = oauth_provider._parse_id_token(id_token, access_token) odoo_1 | File "/opt/odoo/auto/addons/auth_oidc/models/auth_oauth_provider.py", line 75, in _parse_id_token odoo_1 | self._get_key(header.get("kid", None)), odoo_1 | File "<decorator-gen-116>", line 2, in _get_key odoo_1 | File "/opt/odoo/custom/src/odoo/odoo/tools/cache.py", line 93, in lookup odoo_1 | value = d[key] = self.method(*args, **kwargs) odoo_1 | File "/opt/odoo/auto/addons/auth_oidc/models/auth_oauth_provider.py", line 55, in _get_key odoo_1 | if key["kid"] == kid: odoo_1 | KeyError: 'kid' odoo_1 | 2022-11-04 13:16:12,250 1 INFO devel werkzeug: 172.16.3.129 - - [04/Nov/2022 13:16:12] "GET /auth_oauth/signin?code=redacted&state=redacted HTTP/1.0" 303 - 2 0.001 0.362

I updated the fix to handle only the case where a ID Token without kid in header is used with a JWKS that contains a single key.

from OpenID Connect Core Section 10.1

If there are multiple keys in the referenced JWK Set document, a kid value MUST be provided in the JOSE Header

sbidoul · 2022-11-03T17:34:48Z

auth_oidc/models/auth_oauth_provider.py

-        )
-
+        error = None
+        for key in self._get_keys(header.get("kid")):


Do you have link to the spec that shows that kid can contain a list?
I don't find that in https://openid.net/specs/openid-connect-core-1_0.html for instance.

i interpreted the following:

If there are multiple keys in the referenced JWK Set document, a kid value MUST be provided in the JOSE Header

from OpenID Connect Core Section 10.1

As the kid is optional. And missed the as long as there is only one key in the JWK Set (jwks_uri) part. So with your question in mind, it should be unique and not a list. (An our current provider implementation is wrong). I will update the merge request.

i updated the commit to only handle cases for JWKS with a single key

Well, as i started checking our implementation of JWK, i checked the docs again

OpenID ID Token Validation
Would be our starting point.
Step 6:

If the ID Token is received via direct communication between the Client and the Token Endpoint (which it is in this flow), the TLS server validation MAY be used to validate the issuer in place of checking the token signature. The Client MUST validate the signature of all other ID Tokens according to JWS [JWS] using the algorithm specified in the JWT alg Header Parameter. The Client MUST use the keys provided by the Issuer.

Would be the one that defines we MUST validate the signature according to JWS

JWS Note on Key Seelection
Defines which keys should be considered.
Step 2:

Filter the set of collected keys. For instance, some applications will use only keys referenced by "kid" (key ID) or "x5t" (X.509 certificate SHA-1 thumbprint) parameters. If the application uses the "alg" (algorithm), "use" (public key use), or "key_ops" (key operations) parameters, keys with keys with inappropriate values of those parameters would be excluded. Additionally, keys might be filtered to include or exclude keys with certain other member values in an application specific manner. For some applications, no filtering will be applied

And the following sections all sound like we should allow multiple keys (so the initial for loop was more correct than the current implementation)

So it seems for OpenID Connect:

when no kid is specified in ID Token, the JWKS can only contain one key (According to the OpenID Connect Spec)

when kid is specified in ID Token we need to find the matching Key from JWKS because there can be multiple keys for the same kid. The note at the end of
JWS Note on Key Seelection explicitly allow the behaviour of checking the signature against all keys with the same kid:

Note that it is reasonable for some applications to perform signature
or MAC validation prior to making a trust decision about a key, since
keys for which the validation fails need no trust decision.

As JWS only specifies key selection i would conclude that if ID Token gives no kid it is just a missing hint for the correct key so it should still be allowed to have an ID Token without a kid and a JWKS with a single key that has a kid and validate against it.

sbidoul · 2022-11-03T17:35:25Z

auth_oidc/models/res_users.py

+        if "sub" in validation and not "user_id" 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"]


This looks like a useful improvement, thanks.

sbidoul · 2022-11-04T14:13:51Z

auth_oidc/models/auth_oauth_provider.py

                return key
+


I think it would be easier to read (by me at least :) if it was if kid is None: return somekey else: return key["kid"].

And, do you have a standard reference that says which key should be used in that case? Here you take the "first" one. But why the first?

Well, no i do not take the first one. I only take the key that has no kid specified. As this is only allowed when only one key is specified in the JWKS it should be also the first (and last) one.

Your proposed code would get rid of the check that if ID Token has no kid specified in header the JWKS also needs to have no kid specified for the key.

As per my findings in #436 (comment) i would roll back the roll back of the for loop (So we have a _get_keys() again).

And i would also change the code so an ID Token without a kid can match a single key in JWKS.

@sbidoul Let me know if this approach looks good to you.

Yes, sounds good, thanks!

hbrunn · 2022-11-22T13:31:53Z

@ap-wtioit please rebase

ap-wtioit · 2022-11-22T14:01:20Z

@hbrunn i rebased the pull request

* Add module auth_signup_verify_email. * Import module following guidelines. * README typos. * OCA Transbot updated translations from Transifex * Credit creator. * author name correction * [9.0][MIG][auth_signup_verify_email] Migration. Migrate to v9. * [FIX] auth_signup_verify_email: Python library requirement * Add tests, fix xml tags, fix credits. * Fix test.

hbrunn · 2022-12-13T06:51:39Z

@sbidoul could you re-review?

* Add module auth_signup_verify_email. * Import module following guidelines. * README typos. * OCA Transbot updated translations from Transifex * Credit creator. * author name correction * [9.0][MIG][auth_signup_verify_email] Migration. Migrate to v9. * [FIX] auth_signup_verify_email: Python library requirement * Add tests, fix xml tags, fix credits. * Fix test.

github-actions · 2023-04-16T12:10:20Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

hbrunn · 2023-04-17T02:49:10Z

@sbidoul can you have another look?

* Add module auth_signup_verify_email. * Import module following guidelines. * README typos. * OCA Transbot updated translations from Transifex * Credit creator. * author name correction * [9.0][MIG][auth_signup_verify_email] Migration. Migrate to v9. * [FIX] auth_signup_verify_email: Python library requirement * Add tests, fix xml tags, fix credits. * Fix test.

github-actions · 2023-08-27T12:09:53Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

ap-wtioit · 2023-08-28T05:31:44Z

@sbidoul do you have some time to review this again?

auth_oidc/models/auth_oauth_provider.py

sbidoul · 2023-11-16T09:02:51Z

Tests are red due to auth_saml which has a dependency on lasso which is not available on PyPI.
So merging manually, as I don't see an easy way out for lasso.

* Add module auth_signup_verify_email. * Import module following guidelines. * README typos. * OCA Transbot updated translations from Transifex * Credit creator. * author name correction * [9.0][MIG][auth_signup_verify_email] Migration. Migrate to v9. * [FIX] auth_signup_verify_email: Python library requirement * Add tests, fix xml tags, fix credits. * Fix test.

ap-wtioit changed the title ~~12.0 auth OIDC fixes~~ [12.0] auth_oidc: fixes Nov 3, 2022

sbidoul reviewed Nov 3, 2022

View reviewed changes

ap-wtioit force-pushed the 12.0-auth_oidc_fixes branch 2 times, most recently from 2a53bdf to a622a40 Compare November 4, 2022 13:18

sbidoul reviewed Nov 4, 2022

View reviewed changes

ap-wtioit marked this pull request as draft November 7, 2022 10:08

ap-wtioit force-pushed the 12.0-auth_oidc_fixes branch 3 times, most recently from 4f462fc to 3cb0ab2 Compare November 8, 2022 13:42

ap-wtioit marked this pull request as ready for review November 8, 2022 14:34

hbrunn approved these changes Nov 22, 2022

View reviewed changes

ap-wtioit force-pushed the 12.0-auth_oidc_fixes branch from 3cb0ab2 to 4513359 Compare November 22, 2022 13:57

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 16, 2023

github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 23, 2023

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 27, 2023

hbrunn added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Aug 28, 2023

wtaferner approved these changes Sep 4, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 4, 2023

ap-wtioit commented Nov 6, 2023

View reviewed changes

auth_oidc/models/auth_oauth_provider.py Show resolved Hide resolved

[FIX] auth_oidc: set user_id for auth_oauth

3cd886c

ap-wtioit force-pushed the 12.0-auth_oidc_fixes branch from 4513359 to 96882df Compare November 7, 2023 09:05

ap-wtioit added 2 commits November 7, 2023 10:06

[FIX] auth_oidc: support keys without kid

0d15649

[IMP] auth_oidc: update documentation for 12.0.1.2.0

00dc648

ap-wtioit force-pushed the 12.0-auth_oidc_fixes branch from 96882df to 00dc648 Compare November 7, 2023 09:06

sbidoul merged commit 092473a into OCA:12.0 Nov 16, 2023
2 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0] auth_oidc: fixes #436

[12.0] auth_oidc: fixes #436

ap-wtioit commented Nov 3, 2022 •

edited

Loading

OCA-git-bot commented Nov 3, 2022

ap-wtioit commented Nov 3, 2022

sbidoul commented Nov 3, 2022

sbidoul left a comment

sbidoul Nov 3, 2022

ap-wtioit Nov 4, 2022

sbidoul Nov 3, 2022

ap-wtioit Nov 4, 2022 •

edited

Loading

sbidoul Nov 3, 2022

ap-wtioit Nov 4, 2022 •

edited

Loading

ap-wtioit Nov 4, 2022

ap-wtioit Nov 7, 2022

sbidoul Nov 3, 2022

sbidoul Nov 4, 2022

ap-wtioit Nov 7, 2022

ap-wtioit Nov 7, 2022

sbidoul Nov 7, 2022

hbrunn commented Nov 22, 2022

ap-wtioit commented Nov 22, 2022

hbrunn commented Dec 13, 2022

github-actions bot commented Apr 16, 2023

hbrunn commented Apr 17, 2023

github-actions bot commented Aug 27, 2023

ap-wtioit commented Aug 28, 2023

sbidoul commented Nov 16, 2023

	* Fix handling OpenID Connect responses without custom mapping
	* Fix handling OpenID Connect responses without custom mapping by using the ``sub`` claim as user id

[12.0] auth_oidc: fixes #436

[12.0] auth_oidc: fixes #436

Conversation

ap-wtioit commented Nov 3, 2022 • edited Loading

OCA-git-bot commented Nov 3, 2022

ap-wtioit commented Nov 3, 2022

sbidoul commented Nov 3, 2022

sbidoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ap-wtioit Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ap-wtioit Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbrunn commented Nov 22, 2022

ap-wtioit commented Nov 22, 2022

hbrunn commented Dec 13, 2022

github-actions bot commented Apr 16, 2023

hbrunn commented Apr 17, 2023

github-actions bot commented Aug 27, 2023

ap-wtioit commented Aug 28, 2023

sbidoul commented Nov 16, 2023

ap-wtioit commented Nov 3, 2022 •

edited

Loading

ap-wtioit Nov 4, 2022 •

edited

Loading

ap-wtioit Nov 4, 2022 •

edited

Loading