Project

General

Profile

Bug #35346

auth_oidc : l'attribut jwkset d'un fournisseur OIDC n'est pas toujours une instance jwcrypto.JWKSet

Added by Paul Marillonnet 2 months ago. Updated about 1 month ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
09 Aug 2019
Due date:
% Done:

0%

Patch proposed:
No
Planning:
No

Description

Le bout de code fautif :

class OIDCProvider(models.Model):  
    # ...
    @property                                                                                       
    def jwkset(self):                                                                               
        # ...                            
        if self.idtoken_algo == self.ALGO_HMAC:                                                     
            return JWK(kty='oct', k=base64url_encode(self.client_secret.encode('utf-8')))           

0001-auth_oidc-make-OIDCProvider.jwkset-always-be-a-jwcry.patch View (1.04 KB) Paul Marillonnet, 09 Aug 2019 03:33 PM


Related issues

Related to Authentic 2 - Development #31862: authn OIDC : vérifier la signature de l'ID Token reçu Résolu (à déployer) 29 Mar 2019

Associated revisions

Revision 7669f2d6 (diff)
Added by Paul Marillonnet about 2 months ago

auth_oidc: make OIDCProvider.jwkset always be a jwcrypto JWKSet obj (#35346)

History

#1 Updated by Paul Marillonnet 2 months ago

  • Status changed from Nouveau to En cours
  • Assignee set to Paul Marillonnet

#2 Updated by Paul Marillonnet 2 months ago

#3 Updated by Frédéric Péters 2 months ago

  • Related to Development #31862: authn OIDC : vérifier la signature de l'ID Token reçu added

#4 Updated by Paul Marillonnet 2 months ago

  • Patch proposed changed from Yes to No
  • Status changed from Solution proposée to En cours

Ah oui mais non, je ne comprends pas pourquoi le code est en deux parties, en fonction du type d'algo. Et le patch rend ça encore moins propre. Je réessaie

#5 Updated by Paul Marillonnet 2 months ago

Discussions sur le salon :

en mode HMAC il faut ignorer le jwkset_json

Oui, j'y vois plus clair après relecture des spécs à ce sujet. Mais ce qui me gène vraiment c'est qu'on s'attend à ce que la propriété OIDCProvider.jwkset soit absolument raccord avec OIDCProvider.jwkset_json, càd une simple instanciation jwcrypto.jwk.JWKSet d'un dump de l'attribut OIDCProvider.jwkset_json.

Ainsi, si la signature de l'ID Token est basée sur HMAC, ce n'est pas simplement qu'il faut ignorer jwkset_json, c'est que la fonction appelante doit savoir qu'il ne faut pas faire appel à cette propriété OIDC.jwkset, mais plutôt simplement retrouver la clé à partir du client_secret.

#6 Updated by Paul Marillonnet 2 months ago

Le fait qu'un algo de signature HMAC n'empêche à mon avis pas d'avoir un JWKSet ne contenant que la clé correspondante m'amènerait à proposer quelque chose comme ça :

diff --git a/src/authentic2_auth_oidc/models.py b/src/authentic2_auth_oidc/models.py
index 74dd4d086..23149a6d1 100644
--- a/src/authentic2_auth_oidc/models.py
+++ b/src/authentic2_auth_oidc/models.py
@@ -156,11 +156,19 @@ class OIDCProvider(models.Model):
     def jwkset(self):
         from authentic2.crypto import base64url_encode

-        if self.idtoken_algo == self.ALGO_RSA:
-            if self.jwkset_json:
-                return JWKSet.from_json(json.dumps(self.jwkset_json))
-        if self.idtoken_algo == self.ALGO_HMAC:
-            return JWK(kty='oct', k=base64url_encode(self.client_secret.encode('utf-8')))
+        # RSA keys are declared at registration time hence are already present
+        # in the jwkset_json attribute.
+        if self.jwkset_json:
+            return JWKSet.from_json(json.dumps(self.jwkset_json))
+        elif self.idtoken_algo == self.ALGO_HMAC:
+            jwks = JWKSet()
+            key = JWK(kty='oct', k=base64url_encode(self.client_secret.encode('utf-8')))
+            jwks.add_key(key)
+            # Avoid symmetric JWK computation on next access, and adjust
+            # the content of the corresponding JSON model attribute:
+            self.jwkset_json = json.loads(jkws.export())
+            self.save()
+            return jwks
         return None

     def __str__(self):

#7 Updated by Paul Marillonnet 2 months ago

Hmm, ok une seconde relecture des spécs me remet les idées en place : pas de rapport le client_secret et la jwks_uri. Je pense qu'un patch est quand même nécessaire ici, parce qu'un attribut nommé jwkset devrait renvoyer JWKSet quoiqu'il arrive.

#8 Updated by Paul Marillonnet 2 months ago

Et donc plus simplement, répercuter dans le code cette absence de lien logique entre le jwks_uri d'un fournisseur OIDC et le client_secret délivré :

diff --git a/src/authentic2_auth_oidc/models.py b/src/authentic2_auth_oidc/models.py
index 74dd4d086..64bf1c441 100644
--- a/src/authentic2_auth_oidc/models.py
+++ b/src/authentic2_auth_oidc/models.py
@@ -154,13 +154,8 @@ class OIDCProvider(models.Model):

     @property
     def jwkset(self):
-        from authentic2.crypto import base64url_encode
-
-        if self.idtoken_algo == self.ALGO_RSA:
-            if self.jwkset_json:
-                return JWKSet.from_json(json.dumps(self.jwkset_json))
-        if self.idtoken_algo == self.ALGO_HMAC:
-            return JWK(kty='oct', k=base64url_encode(self.client_secret.encode('utf-8')))
+        if self.jwkset_json:
+            return JWKSet.from_json(json.dumps(self.jwkset_json))
         return None

     def __str__(self):

Avec bien sûr toutes les modifications que cela implique dans ce module et dans les tests.

#9 Updated by Benjamin Dauvergne 2 months ago

  • Status changed from En cours to Solution validée

Ok pour le dernier patch.

#10 Updated by Paul Marillonnet about 2 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 7669f2d659235f3f9c59a77d35835437dd998c24
Author: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date:   Fri Aug 9 15:31:27 2019 +0200

    auth_oidc: make OIDCProvider.jwkset always be a jwcrypto JWKSet obj (#35346)

#11 Updated by Frédéric Péters about 1 month ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF