Project

General

Profile

Development #41949

Gérer correctement le SessionIndex dans les requêtes de Logout

Added by Benjamin Dauvergne about 1 month ago. Updated 1 day ago.

Status:
Solution proposée
Priority:
Normal
Target version:
-
Start date:
22 Apr 2020
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

  • Si SessionIndex, le comparer à celui connu, si différent logger un warning, dans tous les cas arrêter la session en cours (donc là il manque juste un log)
  • Si pas de SessionIndex, recherche en base la totalité des sessions pour l'utilisateur, et les supprimer

0002-misc-add-support-for-IDP-initiated-SOAP-SLO-41949.patch View (3.34 KB) Benjamin Dauvergne, 22 Apr 2020 04:27 AM

0001-misc-sign-logout-message-when-using-PRIVATE_KEYS-ins.patch View (854 Bytes) Benjamin Dauvergne, 22 Apr 2020 04:27 AM

0004-tests-add-tests-on-IdP-intiated-SLO-41949.patch View (13.3 KB) Benjamin Dauvergne, 22 Apr 2020 04:27 AM

0003-misc-logout-all-sessions-when-SLO-does-not-contain-a.patch View (2.7 KB) Benjamin Dauvergne, 22 Apr 2020 04:27 AM

0001-misc-sign-logout-message-when-using-PRIVATE_KEYS-ins.patch View (854 Bytes) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0007-tox.ini-add-env-for-running-makemigrations-41949.patch View (744 Bytes) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0002-models-add-SessionIndex-model-41949.patch View (3.18 KB) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0004-misc-support-asynchronous-logout-41949.patch View (19.7 KB) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0003-misc-pass-SAML-identifier-model-on-login-41949.patch View (1.81 KB) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0005-misc-add-support-for-SOAP-SLO-41949.patch View (10.3 KB) Benjamin Dauvergne, 24 Apr 2020 01:22 PM

0006-misc-style-41949.patch View (3.13 KB) Benjamin Dauvergne, 24 Apr 2020 01:22 PM


Related issues

Related to Authentic 2 - Development #41948: À la suppression d'un compte, fermer toutes les sessions ouvertes Nouveau 22 Apr 2020

History

#1 Updated by Benjamin Dauvergne about 1 month ago

  • Related to Development #41948: À la suppression d'un compte, fermer toutes les sessions ouvertes added

#2 Updated by Benjamin Dauvergne about 1 month ago

  • Assignee set to Benjamin Dauvergne

#3 Updated by Benjamin Dauvergne about 1 month ago

J'avais oublié que le binding SOAP n'était pas implémenté dans mellon, c'est chose faite.

Le patch 0001 est nécessaire pour les tests, sinon les réponses de logout n'étaient pas signées.

Ça a levé un petit bug dans l'implémentation de lasso_samlp2_logout_request_set_session_indexes, quand la liste passée est vide, le champ LogoutRequest->SessionIndex n'est pas vidé.

#4 Updated by Benjamin Dauvergne about 1 month ago

  • Status changed from Solution proposée to En cours

Le test SOAP n'est pas bon, la requête doit être reçu sans session en cours.

#5 Updated by Benjamin Dauvergne about 1 month ago

  • 0001: correction d'un bug qui faisait que les réponses de logout n'étaient pas signées dans les tests (utilisation de PRIVATE_KEYS et pas de PRIVATE_KEY)
  • 0002: ajout d'un modèle pour conserver le lien entre les authentifications SAML et les sessions, utile pour tuer toutes les sessions Django d'une même session Django mais aussi pour tuer toutes les sessions d'un même utilisateur (cas ou le message de logout contient une liste de SessionIndex vide)
  • 0003: je m'arrange pour passer le SAMLUserIdentifier obtenu au login d'un utilisateur à la vue de login, pour pouvoir plus loin le lier à l'objet SessionIndex
  • 0004: le coeur du truc; le logout devient un poil plus compliqué et on vérifie des choses qu'on aurait du vérifier avant :
    • je retrouve les SessionIndex par rapport au contenu de la requête de logout (sur base du NameID et des éventuels SessionIndex),
    • je reconstruis proprement un dump d'un objet LassoSession pour permettre à Lasso de savoir tout ça (et plus par rapport à ce qui est stocké en session),
    • ensuite lors de la déconnexion effective des sessions :
      • si la session en cours concerne le même utilisateur que la requête, je la déconnecte toujours (même si le SessionIndex ne correspond pas c'est plus sûr), par contre si la requête concerne un autre utilisateur on ne fait rien (sinon un utilisateur pour conserver une requête pour lui, la faire cliquer à un autre et le déconnecter),
      • si la requête contient des SessionIndex qui vise d'autres sessions ou indique de tuer toutes les sessions, je les supprime directement via le backend de session
        0005: implémentation du mode SOAP, parce que c'est généralement par là que viendront les requêtes de déconnexion globale (désactivation / suppression de compte)

#6 Updated by Benjamin Dauvergne about 1 month ago

  • corrigé l'usage de is_authenticated()
  • corrigé l'invariant invalide dans _link_user()

#7 Updated by Benjamin Dauvergne about 1 month ago

  • Status changed from Solution proposée to Solution validée

#8 Updated by Benjamin Dauvergne about 1 month ago

  • Status changed from Solution validée to Solution proposée

#9 Updated by Paul Marillonnet 3 days ago

Déjà une première chose qui saute aux yeux, dans les deux tests ajoutés par le patch 0005, il manque un caplog.clear() entre l'établissement des deux sessions différentes, pour tester correctement ce qui est loggé (ici on teste plusieurs fois la même chose sans effacer les logs, la seconde phase d'assert sur caplog ne sert à rien, elle ne teste rien de nouveau).

#10 Updated by Benjamin Dauvergne 3 days ago

Je ne vois pas de caplog.clear() sauf dans tests/test_default_adapter.py, en fait tu veux dire que tous les tests du genre 'x' in caplog sont foireux ? C'est possible mais c'est pas bien clair de la façon dont tu l'écris.

#11 Updated by Paul Marillonnet 3 days ago

Benjamin Dauvergne a écrit :

Je ne vois pas de caplog.clear() sauf dans tests/test_default_adapter.py, en fait tu veux dire que tous les tests du genre 'x' in caplog sont foireux ? C'est possible mais c'est pas bien clair de la façon dont tu l'écris.

J'ai oublié le verbe dans ma phrase, forcément c'est un peu moins percutant, mes excuses :/ (c'est corrigé).
Et oui on teste plusieurs fois la présence de chaînes loggées entre différentes actions, sans réinitialiser les logs entre temps.

#12 Updated by Benjamin Dauvergne 3 days ago

Ok j'ai poussé une correction sur ce point, j'ai viré au passage la fonction reset_log() qui datait de quand caplog.clear() n'existait pas.

#13 Updated by Paul Marillonnet 2 days ago

Benjamin Dauvergne a écrit :

Ok j'ai poussé une correction sur ce point, j'ai viré au passage la fonction reset_log() qui datait de quand caplog.clear() n'existait pas.

Merci, je continue ma relecture, et je regarde comment paramétrer si possible devinst pour utiliser le slo soap du patch 0005.

#14 Updated by Paul Marillonnet 2 days ago

Autre question : dans 0004 la cbv LogoutView est exemptée de vérification du jeton csrf. Tu crois pas qu'on gagnerait à faire en sorte d'appliquer cette exemption aux tentatives de SLO asynchrones seulement, et continuer la vérification pour toutes les autres ?

#15 Updated by Benjamin Dauvergne 1 day ago

Paul Marillonnet a écrit :

Autre question : dans 0004 la cbv LogoutView est exemptée de vérification du jeton csrf. Tu crois pas qu'on gagnerait à faire en sorte d'appliquer cette exemption aux tentatives de SLO asynchrones seulement, et continuer la vérification pour toutes les autres ?

Le truc c'est que par simplification la vue s'occupe des logouts initiées par le SP (la oui il faudrait passer à une initiation par POST avec CSRF check, histoire qu'on ne puisse pas délogger quelqu'un sur un GET venant de n'importe où), et les logouts initiés par l'IdP qui peuvent venir en GET ou POST-SOAP (ou même POST tout court mais on ne le gère pas et on a pas d'obligation de le faire je pense, enfin on peut toujours regarder SAML Conformance, profil light SP pour s'en assurer). Ici de toute façon on accepte logout par GET donc laisser le check CSRF ne change rien à l'affaire, les checks CSRF c'est que sur les POST, par contre ça devrait aller dans le patch 0005 plutôt que le 0004, c'est une erreur de découpage de ma part je pense.

Also available in: Atom PDF