Bug #35346
auth_oidc : l'attribut jwkset d'un fournisseur OIDC n'est pas toujours une instance jwcrypto.JWKSet
0%
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
Révisions associées
Historique
Mis à jour par Paul Marillonnet il y a plus de 4 ans
- Statut changé de Nouveau à En cours
- Assigné à mis à Paul Marillonnet
Mis à jour par Paul Marillonnet il y a plus de 4 ans
- Fichier 0001-auth_oidc-make-OIDCProvider.jwkset-always-be-a-jwcry.patch 0001-auth_oidc-make-OIDCProvider.jwkset-always-be-a-jwcry.patch ajouté
- Statut changé de En cours à Solution proposée
- Patch proposed changé de Non à Oui
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é
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
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.
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):
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.
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.
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.
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)
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
auth_oidc: make OIDCProvider.jwkset always be a jwcrypto JWKSet obj (#35346)