Projet

Général

Profil

Bug #35346

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

Ajouté par Paul Marillonnet il y a plus de 4 ans. Mis à jour il y a plus de 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Catégorie:
-
Version cible:
-
Début:
09 août 2019
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Non
Planning:
Non

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')))           


Fichiers


Demandes liées

Lié à Authentic 2 - Development #31862: authn OIDC : vérifier la signature de l'ID Token reçuFermé29 mars 2019

Actions

Révisions associées

Révision 7669f2d6 (diff)
Ajouté par Paul Marillonnet il y a plus de 4 ans

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

Historique

#1

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Nouveau à En cours
  • Assigné à mis à Paul Marillonnet
#2

Mis à jour par Paul Marillonnet il y a plus de 4 ans

#3

Mis à jour par Frédéric Péters il y a plus de 4 ans

  • Lié à Development #31862: authn OIDC : vérifier la signature de l'ID Token reçu ajouté
#4

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Solution proposée à En cours
  • Patch proposed changé de Oui à Non

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

Mis à jour par Paul Marillonnet il y a plus de 4 ans

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

Mis à jour par Paul Marillonnet il y a plus de 4 ans

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

Mis à jour par Paul Marillonnet il y a plus de 4 ans

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

Mis à jour par Paul Marillonnet il y a plus de 4 ans

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

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de En cours à Solution validée

Ok pour le dernier patch.

#10

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Solution validée à 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

Mis à jour par Frédéric Péters il y a plus de 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF