Produits Entr'ouvert » Authentic 2

Pour l'instant on se contente de la sécurité apportée par le token_endpoint mais ce serait bien aussi qu'on valide la signature.

Bon, j'en suis à la partie pas très drôle : j'ai ma procédure de vérification en place, qui se base sur l'implémentation de JWS par python-jwcrypto, mais qui ne fonctionne pas encore sur les jeux de données témoin présents dans les tests d'a2.

Je me creuse la tête pour savoir si (i) je m'y prends mal, en dépit des spécifications claires de vérification de la signature de l'ID Token, ou si (ii) à la lecture du code-jwcrypto, je vois que cette implémentation fait des choses hors-spécs qu'il faudrait prendre en compte ici.

Je pense que tu peux tenter de remplacer toute le code parse_idtoken par la classe JWT de jwcrypto; mais il faut vérifier aussi les contraintes de validation propres à OIDC : https://openid.net/specs/openid-connect-core-1_0.html#SigEnc

Benjamin Dauvergne a écrit :

Je pense que tu peux tenter de remplacer toute le code parse_idtoken par la classe JWT de jwcrypto; mais il faut vérifier aussi les contraintes de validation propres à OIDC : https://openid.net/specs/openid-connect-core-1_0.html#SigEnc

Oui c'est encore ce qui me paraît le plus simple. Je suis sur le coup, merci pour le tuyau.

Il faudrait remplace ça

        id_token = str(id_token)

try:
        id_token = id_token.encode('ascii')
except UnicodeError:
    ....

C'est quoi le souci qui fait que tu conserves quasiment tout le code au lieu de faire direct jwt.deserialize() ?

Mon avis, d'après la doc¹, c'est que tu peux directement faire :

if symetric:
   algs = ['HS256', 'HS384', 'HS512']
   key = JWK(typ='oct', ...)
else:
   algs = ['RS256', ...]
   key = get_jwkset()
jwt = JWT(jwt=id_token, algs=algs, key=key)

¹ https://jwcrypto.readthedocs.io/en/latest/jwt.html

Benjamin Dauvergne a écrit :

C'est quoi le souci qui fait que tu conserves quasiment tout le code au lieu de faire direct jwt.deserialize() ?

Alors voilà oui, (par mesure de sécurité j'imagine) l'interface de jwcrypto ne laisse pas accéder à la charge utile du jeton si la signature n'a pas été vérifiée :

(Pdb) jwt.claims       
*** KeyError: "'claims' not set" 
(Pdb) jwt.token.payload
*** InvalidJWSOperation: Payload not verified

Or pour vérifier la signature il faut (en plus l'identifiant de clé dans l'entête JOSE) l'identifiant de l'émetteur qui lui est fourni par la revendication 'iss' qui se trouve … dans la charge utile.

D'où le code non supprimé.
J'ai laissé aussi le test sur le nombre de parties du jeton encodé, car je voudrais par la suite y implémenter (dans un autre ticket) le support des jetons chiffrés.

Benjamin Dauvergne a écrit :

Mon avis, d'après la doc¹, c'est que tu peux directement faire :

Hmm, je ne crois pas que ce soit applicable dans notre modèle de donnée. Dans le cas où l'algo de signature est symétrique, il faut connaître le client_secret associé au fournisseur OIDC ; et dans le cas où l'algo est asymétrique, il faut aussi retrouver le fournisseur OIDC pour retomber sur la bonne clé publique.

Dans tous les cas, je crois, ça impose de décoder des bouts de jeton (et d'appliquer les validations manuelles induites) pour retrouver ce fournisseur.

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Mon avis, d'après la doc¹, c'est que tu peux directement faire :

Hmm, je ne crois pas que ce soit applicable dans notre modèle de donnée. Dans le cas où l'algo de signature est symétrique, il faut connaître le client_secret associé au fournisseur OIDC ; et dans le cas où l'algo est asymétrique, il faut aussi retrouver le fournisseur OIDC pour retomber sur la bonne clé publique.

Dans tous les cas, je crois, ça impose de décoder des bouts de jeton (et d'appliquer les validations manuelles induites) pour retrouver ce fournisseur.

Le payload est accessible via jwt.token.objects.get('payload') mais je me demande si c'est la bonne manière de faire, normalement on connaît déjà l'issuer à ce moment là (on est dans le backend, après un appel à authenticate()) il suffirait de passer le provider à authenticate.

Paul Marillonnet a écrit :

Okay, je vois maintenant ce que tu veux dire. Ça fonctionne mais ça donne du code spaghetti. Je serais pour refactoriser en s'inspirant de l'interface des jetons jwcrypto.jwt.JWT : on implémente une fonction IDToken.deserialize qui prend en paramètre l'identifiant de l'émetteur du jeton, voire carrément l'objet OIDCProvider, et qui entre autres vérifie la signature.

Je ne sais pas ce que tu appelles spaghetti, et oui tu peux changer toutes les APIs que tu veux, je dis juste que c'est pas la peine de retrouver une donnée qu'on a déjà et que c'est même mieux en fait (on attend un jeton de tel IdP on ne devrait pas en accepter un autre).

Benjamin Dauvergne a écrit :

Je ne sais pas ce que tu appelles spaghetti, et oui tu peux changer toutes les APIs que tu veux, je dis juste que c'est pas la peine de retrouver une donnée qu'on a déjà et que c'est même mieux en fait (on attend un jeton de tel IdP on ne devrait pas en accepter un autre).

En l'état du code l'initialisateur de la classe IDToken doit recevoir d'une façon ou d'une autre un identifiant d'émetteur du jeton, et du coup l'héritage depuis le type str n'est à mon avis plus justifié. Et le cloisonnement entre parse_id_token appelé par l'initialisateur et le reste de du code ce cet initialisateur qui effectue lui aussi des vérifications sur la valeur du jeton ne l'est plus non plus.

J'ai en tête quelque chose comme :

token = IDToken(encoded) # <- pas de vérification à ce moment, on se contente de stocker l'encodé
try:
    token.deserialize(issuer) # <- on vérifie tout y compris la signature
except ValueError: # <- problème lors de la désérialisation ou de la vérification de la signature
    logger.error("Error on ...")
    return
claims = token.claims # <- devient accessible si la desérialisation s'est déroulée sans erreur
header = token.jose_header # <- pareil

Tout me va.

Paul Marillonnet a écrit :

Ok, je comprends la nécessité de recourir à des exceptions plus spécifiques. Je propose ici une nouvelle version du patch qui reprend les exceptions de jwcrypto (et non pas DeserializationError, qui appartenant au DRF n'a à mon avis pas sa place ici).

On s'est pas compris, je parlais d'une nouvelle exception pas de celle de DRF que je ne connaissais même pas :)

Edit: Le patch retire la vérification de la présence et de la cohérence des claims temporelles 'exp' et 'iat', maintenant prises en charges par jwcrypto.

Ok.

Benjamin Dauvergne a écrit :

On s'est pas compris, je parlais d'une nouvelle exception pas de celle de DRF que je ne connaissais même pas :)

Ok, j'ai dit ça après l'avoir vu utilisée dans le code de authentic.serializers.
Est-ce que le patch en l'état, réutilisant des exceptions jwcrypto plus spécifiques qu'une DeserializationError te convient ?