Project

General

Profile

Development #35205

OIDC, implémenter Resource Owner Password Credential Grant

Added by Benjamin Dauvergne 6 months ago. Updated 5 days ago.

Status:
En cours
Priority:
Normal
Category:
-
Target version:
-
Start date:
02 Aug 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Voir :

https://tools.ietf.org/html/rfc6749#section-4.3

Il doit être étendu pour prendre en plus le slug ou le nom d'une OU.

La réponse doit contenir seulement un id_token.

Le web-service doit être protégé par du throttling pour éviter de servir aux attaques de vérification de mot de passe par force brute.

0001-idp_oidc-support-oauth2-resource-owner-password-cred.patch View (18.9 KB) Paul Marillonnet, 23 Dec 2019 02:56 PM

0001-idp_oidc-support-oauth2-resource-owner-password-cred.patch View (19.6 KB) Paul Marillonnet, 09 Jan 2020 06:22 PM

0001-idp_oidc-support-oauth2-resource-owner-password-cred.patch View (19.9 KB) Paul Marillonnet, 10 Jan 2020 10:49 AM

History

#1 Updated by Benjamin Dauvergne 6 months ago

On ajoutera un client_id et un client_secret à ce que demande la spec, un appel typique ressemblera à :

https://idp.com/idp/oidc/token/?grant_type=password&username=jean.dupont&password=1234&ou-slug=saint-michel-les-coquillages&client_id=1234&client_secret=4567

#4 Updated by Paul Marillonnet 3 months ago

  • Assignee set to Paul Marillonnet

Ok.

#5 Updated by Benjamin Dauvergne 3 months ago

Du calme c'est pas financé, Mike ?

#6 Updated by Paul Marillonnet 26 days ago

Je verrais bien quelque comme ça, une lecture très directe de la rfc, modulo la condition qu'on s'était fixée au début à savoir ne délivrer qu'un jeton ID.

#7 Updated by Benjamin Dauvergne 25 days ago

  • utiliser django-ratelimit plutôt que exponentialretrytimeout (c'est au client de vérifier si il y a trop d'essais sur un username pas à nous) simplement sur le client_id avec une valeur assez haute ; c'est juste pour limiter le déni de service,
  • utiliser exponentialretrytimeout uniquement sur le username+client_id (la vrai IP du client final on ne l'a pas, pas la peine de s'en préoccuper, le client qui nous interroge devra le faire lui même),
  • plutôt que d'augmenter l'indentation dans token() faire 2 méthodes
  • quel est le besoin d'avoir une fixture cleartext_pw_user ? on a déjà plein de fixture d'utilisateurs (et leur mot de passe est systématiquement égal au username)
  •             return invalid_client(
                    'client authentication failed')
    c'est sur-indenté
  • HttpResponse(json.dumps(
    on peut arrêter d'utiliser json.dumps il y a un JsonResponse en django 1.111
  •             'acr': '0', # XXX Corner cases where the authn context class ref is required?
    ce sera toujours 0 pour du login/mdp (et ce profil ne supporte que ça)

[1]: https://docs.djangoproject.com/fr/1.11/ref/request-response/#jsonresponse-objects

#8 Updated by Benjamin Dauvergne 25 days ago

  • Status changed from Solution proposée to En cours

#9 Updated by Paul Marillonnet 10 days ago

Tu as déjà pu tester django-ratelimit? En mode TDD je n'arrive pas du tout au résultat escompté, je ne parviens tout bonnement pas à faire fonctionner le module (pas de positifs à la rate-limitation, que des (faux ou vrais) négatifs).
Je vais creuser un peu dans le code de django-ratelimit pour comprendre ce à côté de quoi je passe, parce qu'à la lecture de la doc mon code m'a l'air convenable.

#10 Updated by Frédéric Péters 10 days ago

django-ratelimit est utilisé dans combo et w.c.s.

#11 Updated by Paul Marillonnet 10 days ago

Merci, je regarde ça.

#12 Updated by Paul Marillonnet 9 days ago

Je pense qu'on peut faire quelque chose comme ça.

Seule chose qui m'embête : utiliser l'identifiant de client comme clé pour la rate limitation implique de tenter d'authentifier ce client (si on ne le fait pas, il suffit pour un attaquant de connaître l'identifiant valide d'un client sans pour autant connaître son secret pour le bloquer complètement). Est-ce que ça ne défait pas déjà l'objectif de prévention d'une attaque par déni de service.
Si ce n'est pas le cas, une amélioration possible du patch en l'état serait de mettre en cache le résultat d'authentification obtenu lors de la détermination de la clé pour ensuite le réutiliser, sans avoir à authentifier le client une seconde fois – mais je ne suis pas certain de la pertinence de cette amélioration (le dilemme se résume à cette question : est-ce que (i) une authn + une mise en cache de l'id + une requête dans le cache pour retrouver l'id + une requête ORM pour retrouver le client c'est mieux que (ii) deux authn ?).

#13 Updated by Paul Marillonnet 9 days ago

Paul Marillonnet a écrit :

mais je ne suis pas certain de la pertinence de cette amélioration

Et si on me le demande à la relecture, je peux aussi taper un script qui compare en temps d'exécution et en nombre de requêtes sql les versions avec et sans "amélioration".

#14 Updated by Paul Marillonnet 9 days ago

  • Status changed from Solution proposée to En cours

(Des effets de bord entre les tests dûs à l'usage de mêmes clés de limitation, je revois ma copie.)

#15 Updated by Benjamin Dauvergne 9 days ago

Il me semble que dans les tests on utilise le backend locmem, donc un simple cache.clear() avant et après le test devraient suffire.

#16 Updated by Paul Marillonnet 8 days ago

Benjamin Dauvergne a écrit :

Il me semble que dans les tests on utilise le backend locmem, donc un simple cache.clear() avant et après le test devraient suffire.

Bien vu, merci.
Nouveau patch ici (avec ta précédente relecture prise en compte).

#17 Updated by Benjamin Dauvergne 8 days ago

  • il reste encore des HttpResponse(json.dumps...
  • en cas d'erreur la description doit être dans une clé 'error_description', pas 'desc', voir https://openid.net/specs/openid-connect-core-1_0.html#AuthError
  • le cas d'erreur ligne 125 n'est pas testé
  • il n'y a pas de validation des scopes (à factoriser avec le code d'autorisation)
  • tu ne retournes qu'un id_token et pas d'access token, comme on ne propose pas de politique à sujet pour l'instant je renverrai tout
  • je m'aperçois au passage que le token_type est faux dans le code actuel, c'est Bearer dans le code et bearer dans la spec

#18 Updated by Benjamin Dauvergne 8 days ago

  • Status changed from Solution proposée to En cours

#19 Updated by Paul Marillonnet 6 days ago

  • tu ne retournes qu'un id_token et pas d'access token, comme on ne propose pas de politique à sujet pour l'instant je renverrai tout

Oui ok. Le descriptif du ticket indique le contraire, et dans mes souvenirs ça me paraissait logique mais je ne me souviens plus pourquoi :)
Je fais la modification (ainsi que toutes celles demandées dans ta relecture) et je reviens ici.

#20 Updated by Benjamin Dauvergne 5 days ago

Paul Marillonnet a écrit :

Oui ok. Le descriptif du ticket indique le contraire, et dans mes souvenirs ça me paraissait logique mais je ne me souviens plus pourquoi :)

Ouaip c'est vrai, « La réponse doit contenir seulement un id_token » et pareil j'ai oublié la justification alors oublions, c'est juste qu'un access token est plus simple à consommer, mais on pourra imaginer une politique dans le paramétrage pour ce mode (pour les modes "navigateur" c'est dès la demande d'autorisation qu'on précise ce qu'on souhaite comme jeton, le problème c'est que la spécification openidconnect n'est pas très complète sur ce point, en credential grant elle ne précise pas ce qu'on peut ou pas renvoyer ou si ça devrait être configurable).

Also available in: Atom PDF