Project

General

Profile

Development #41949

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

Added by Benjamin Dauvergne 7 months ago. Updated 5 months ago.

Status:
Solution déployé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

Screenshot_2020-06-02 Portail - Connexion.png View (33.1 KB) Paul Marillonnet, 02 Jun 2020 04:12 PM

45077

Related issues

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

Associated revisions

Revision be52f6c2 (diff)
Added by Benjamin Dauvergne 5 months ago

misc: sign logout message when using PRIVATE_KEYS instead of PRIVATE_KEY (#41949)

Revision 7b5ad08a (diff)
Added by Benjamin Dauvergne 5 months ago

models: add SessionIndex model (#41949)

Revision 2c6a051b (diff)
Added by Benjamin Dauvergne 5 months ago

misc: cache SAML identifier model on logged user (#41949)

Revision 65cbdcef (diff)
Added by Benjamin Dauvergne 5 months ago

misc: support asynchronous logout (#41949)

It means that will lookup for other Django sessions linked to the
received logout request; logout request can specify session indexes or
ask for logout of all sessions of the user targeted by the NameID.

Revision 482aa09f (diff)
Added by Benjamin Dauvergne 5 months ago

misc: add support for SOAP SLO (#41949)

Revision 3c696c60 (diff)
Added by Benjamin Dauvergne 5 months ago

tox.ini: add env for running makemigrations (#41949)

Revision e1deb96f (diff)
Added by Benjamin Dauvergne 5 months ago

tests: clear caplog between sessions (#41949)

History

#1 Updated by Benjamin Dauvergne 7 months ago

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

#2 Updated by Benjamin Dauvergne 7 months ago

  • Assignee set to Benjamin Dauvergne

#3 Updated by Benjamin Dauvergne 7 months 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 7 months 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 7 months 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 7 months ago

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

#7 Updated by Benjamin Dauvergne 7 months ago

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

#8 Updated by Benjamin Dauvergne 7 months ago

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

#9 Updated by Paul Marillonnet 6 months 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 6 months 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 6 months 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 6 months 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 6 months 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 6 months 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 6 months 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.

#16 Updated by Paul Marillonnet 6 months ago

Pour info, je suis en train de jouer en local avec les patches pour voir si le slo soap fonctionne comme attendu, mais je me mange une TypeError lorsque je change les métadonnées dans /admin/saml/libertyprovider/<id>/change/, je regarde ça.

#17 Updated by Paul Marillonnet 6 months ago

Et je pensais qu'ajouter le bout de métadonnées introduit dans le patch slo soap 0005, à l'autre bout dans l'admin d'authentic pour les SP concernés, serait suffisant. Mais, sans réussir à comprendre si c'est dû aux patches ou non, on dirait que les clés ont changé entre temps. Par exemple :

$ tail -13 /var/log/combo/stderr.log 
2020-06-02 15:16:14 (xml.c/:1451) Matching node NameID vs snippet CertificateFile: FAILURE names don't match
2020-06-02 15:16:14 (xml.c/:1451) Matching node NameID vs snippet BaseID: FAILURE names don't match
2020-06-02 15:16:14 (xml.c/:1461) Matching node NameID vs snippet NameID: SUCCESS namespace URIs match
2020-06-02 15:16:14 (xml.c/:2577) Processing node 'NameID' with type '(null)'
2020-06-02 15:16:14 (xml.c/:2577) Processing node 'NameID' with type 'LassoSaml2NameID'
2020-06-02 15:16:14 (xml.c/:1492) lasso_node_impl_init_from_xml <NameID>
2020-06-02 15:16:14 (xml.c/:1901) lasso_node_impl_init_from_xml </NameID> rc=0
2020-06-02 15:16:14 (xml.c/:1456) Matching node SessionIndex vs snippet SessionIndex: SUCCESS namespace prefixes match
2020-06-02 15:16:14 (xml.c/:1901) lasso_node_impl_init_from_xml </LogoutRequest> rc=0
2020-06-02 15:16:14 (xml.c/:2577) Processing node 'Session' with type 'LassoSession'
error validating logout request: ProfileMissingAssertionError()
2020-06-02 15:16:14 (xml.c/:2577) Processing node 'Session' with type 'LassoSession'
error validating logout request: ProfileMissingAssertionError()

Je continue de creuser.

#18 Updated by Paul Marillonnet 6 months ago

45077
Dans un test devinst avec, côté a2, le règlement des options de fournisseur de service configuré pour le Binding de SLO SOAP :
  • Lorsque le SLO est initié par l'idp, nickel.
  • Lorsqu'initié par un des SP, on reste bloqué sur la page de déconnexion A2 (cf capture) et les logs a2 renvoient à nouveau une erreur de ce genre :
    2020-06-02 16:06:57 (xml.c/:1492) lasso_node_impl_init_from_xml <LogoutResponse>
    2020-06-02 16:06:57 (xml.c/:1461) Matching node Issuer vs snippet Issuer: SUCCESS namespace URIs match
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'Issuer' with type 'LassoSaml2NameID'
    2020-06-02 16:06:57 (xml.c/:1492) lasso_node_impl_init_from_xml <Issuer>
    2020-06-02 16:06:57 (xml.c/:1901) lasso_node_impl_init_from_xml </Issuer> rc=0
    2020-06-02 16:06:57 (xml.c/:1461) Matching node Signature vs snippet Signature: SUCCESS namespace URIs match
    2020-06-02 16:06:57 (xml.c/:1451) Matching node Status vs snippet Extensions: FAILURE names don't match
    2020-06-02 16:06:57 (xml.c/:1456) Matching node Status vs snippet Status: SUCCESS namespace prefixes match
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'Status' with type '(null)'
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'Status' with type 'LassoSamlp2Status'
    2020-06-02 16:06:57 (xml.c/:1492) lasso_node_impl_init_from_xml <Status>
    2020-06-02 16:06:57 (xml.c/:1456) Matching node StatusCode vs snippet StatusCode: SUCCESS namespace prefixes match
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'StatusCode' with type '(null)'
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'StatusCode' with type 'LassoSamlp2StatusCode'
    2020-06-02 16:06:57 (xml.c/:1492) lasso_node_impl_init_from_xml <StatusCode>
    2020-06-02 16:06:57 (xml.c/:1456) Matching node StatusCode vs snippet StatusCode: SUCCESS namespace prefixes match
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'StatusCode' with type '(null)'
    2020-06-02 16:06:57 (xml.c/:2577) Processing node 'StatusCode' with type 'LassoSamlp2StatusCode'
    2020-06-02 16:06:57 (xml.c/:1492) lasso_node_impl_init_from_xml <StatusCode>
    2020-06-02 16:06:57 (xml.c/:1901) lasso_node_impl_init_from_xml </StatusCode> rc=0
    2020-06-02 16:06:57 (xml.c/:1901) lasso_node_impl_init_from_xml </StatusCode> rc=0
    2020-06-02 16:06:57 (xml.c/:1901) lasso_node_impl_init_from_xml </Status> rc=0
    2020-06-02 16:06:57 (xml.c/:1901) lasso_node_impl_init_from_xml </LogoutResponse> rc=0
    https://passerelle.dev.publik.love/accounts/mellon/metadata/ denied the logout request
    

    Je vais chercher ce que signifie cette erreur, i.e. Matching node Status vs snippet Extensions: FAILURE names don't match.

#19 Updated by Paul Marillonnet 6 months ago

Ce n'est pas spécifique au SLO SOAP, j'ai créé #43613.

#20 Updated by Benjamin Dauvergne 6 months ago

Matching node Status vs snippet Extensions: FAILURE names don't match.

Ce n'est pas une erreur c'est un log normal du parser XSD dans Lasso, il matche progressivement et quand il y a des alternatives il faut au noeud suivant en cas d'erreur. On voit qu'il réessaye justement pour le même noeud Status et ça passe :

2020-06-02 16:06:57 (xml.c/:1451) Matching node Status vs snippet Extensions: FAILURE names don't match
2020-06-02 16:06:57 (xml.c/:1456) Matching node Status vs snippet Status: SUCCESS namespace prefixes match

Il faut voir pourquoi la requête de logout est refusé, il doit y avoir un code d'erreur lasso.

2020-06-02 15:16:14 (xml.c/:2577) Processing node 'Session' with type 'LassoSession'

error validating logout request: ProfileMissingAssertionError()

2020-06-02 15:16:14 (xml.c/:2577) Processing node 'Session' with type 'LassoSession'

error validating logout request: ProfileMissingAssertionError()

Là il y a un souci pour retrouver la bonne assertion/session dans LassoSession.

#21 Updated by Paul Marillonnet 6 months ago

Après clôture de #43613 je reproduis le bogue décrit ici selon le protocole suivant :
- déploiement du tenant sur devinst, le SLO fonctionne tranquille.
- git-checkout de mellon sur la branche de ce ticket, réinstallation devinst en clone_repo: False et redéploiement du tenant.
- le SLO fonctionne encore, la balise de métadonnées SAML correspondant au nouveau binding est bien présente.
- basculement sur le binding de SLO SOAP dans les options de réglement de fournisseur SAML, ce qui casse le SLO de la façon décrite ici plus haut.
- retour sur le binding HTTP-Redirect, ce qui ne rétablit pas le bon fonctionnement du SLO.

#22 Updated by Benjamin Dauvergne 6 months ago

  • Status changed from Solution proposée to En cours

Je vais regarder ça.

#23 Updated by Benjamin Dauvergne 5 months ago

Je suppose que tu n'as pas rafraîchi les métadonnées coté authentic entre temps (dans admin, listing des fournisseurs, action "update metadata") et donc tu as eu des Unsupported protocol profile (j'ai eu pareil), si tu rafraîchis tout marche correctement.

#24 Updated by Benjamin Dauvergne 5 months ago

  • Status changed from En cours to Solution proposée

#25 Updated by Paul Marillonnet 5 months ago

  • Status changed from Solution proposée to Information nécessaire

Benjamin Dauvergne a écrit :

Je suppose que tu n'as pas rafraîchi les métadonnées coté authentic entre temps (dans admin, listing des fournisseurs, action "update metadata") et donc tu as eu des Unsupported protocol profile (j'ai eu pareil), si tu rafraîchis tout marche correctement.

Je ne comprends pas, si les deux bindings sont supportés et tous deux déclarés dans les métadonnées, pourquoi passer de l'un à l'autre dans les options de règlement de fournisseurs SAML implique de mettre à jour les métadonnées des SPs concernés ?!

#26 Updated by Benjamin Dauvergne 5 months ago

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Je suppose que tu n'as pas rafraîchi les métadonnées coté authentic entre temps (dans admin, listing des fournisseurs, action "update metadata") et donc tu as eu des Unsupported protocol profile (j'ai eu pareil), si tu rafraîchis tout marche correctement.

Je ne comprends pas, si les deux bindings sont supportés et tous deux déclarés dans les métadonnées, pourquoi passer de l'un à l'autre dans les options de règlement de fournisseurs SAML implique de mettre à jour les métadonnées des SPs concernés ?!

Si tu demandes à authentic d'envoyer un logout par SOAP mais que les métadonnées du SP concerné n'indique pas que SOAP est concerné, Lasso lève l'exception UnsupportedProfile ce qui est normal (après on pourrait vérifier que c'est possible au moment de la configuration, ce n'est pas fait, ce n'est pas très important actuellement).

#27 Updated by Paul Marillonnet 5 months ago

  • Status changed from Information nécessaire to Solution validée

Benjamin Dauvergne a écrit :

Si tu demandes à authentic d'envoyer un logout par SOAP mais que les métadonnées du SP concerné n'indique pas que SOAP est concerné, Lasso lève l'exception UnsupportedProfile ce qui est normal (après on pourrait vérifier que c'est possible au moment de la configuration, ce n'est pas fait, ce n'est pas très important actuellement).

Ok, je capte le truc, mais le supposé bogue que je décrivais semblait survenir en présence des deux balises SingleLogoutService (une pour le binding par redirections HTTP et l'autre pour le binding SOAP), et donc je ne voyais la nécessité de recharger les MD (qui d'ailleurs restent inchangées après un "update metadata", même après changement du binding utilisé dans les options de règlement de fournisseur SAML).
Toujours est-il que je ne reproduis plus le problème décrit dans #41949-21, qui venait je pense d'un mauvais déploiement sur mon devinst, alors que je creusais pour rien dans le code de lasso.
Mes excuses pour le faux négatif, ack.

#28 Updated by Benjamin Dauvergne 5 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit e1deb96f8cd9f0df58b6288bdf2bdf30118e04b9
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Wed May 27 13:10:10 2020 +0200

    tests: clear caplog between sessions (#41949)

commit 3c696c60a2fa062d1cbb487b0d31950157b78a99
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 12:49:53 2020 +0200

    tox.ini: add env for running makemigrations (#41949)

commit 24f96bb6c9ee3848e34ea6eea9f504036e570bda
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 13:08:09 2020 +0200

    misc: style (#41949)

commit 482aa09f9218ec92d8b575dfddabf4e9392ee44e
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 13:01:17 2020 +0200

    misc: add support for SOAP SLO (#41949)

commit 65cbdcefc31f32b6438616e183257df61dc3b4a5
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 12:52:34 2020 +0200

    misc: support asynchronous logout (#41949)

    It means that will lookup for other Django sessions linked to the
    received logout request; logout request can specify session indexes or
    ask for logout of all sessions of the user targeted by the NameID.

commit 2c6a051b4a16ca174da67eab4a3535faebbfc2be
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 12:48:00 2020 +0200

    misc: cache SAML identifier model on logged user (#41949)

commit 7b5ad08ad827e6ef8fde04820d8cb980bf97f879
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Apr 24 12:49:10 2020 +0200

    models: add SessionIndex model (#41949)

commit be52f6c2eca815e0ae1ff9b9bfe1a86a4b13eaaf
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Wed Apr 22 04:11:20 2020 +0200

    misc: sign logout message when using PRIVATE_KEYS instead of PRIVATE_KEY (#41949)

#29 Updated by Frédéric Péters 5 months ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF