[TECH] Mise à jour de samlify 2.4.0 à 2.7.6.

[octo-topi]

🦄 Problème

Suite à passage à Node10, le package samlify a été forké pour forcer l'utilisation de xml-crypto à 1.0.2 afin d'avoir un comportement désiré.

Mais plusieurs problèmes de sécurité de niveau haut affectent ce package xml-crypto

🤖 Solution

A ce jour, la nouvelle version 2.75 samlify qui embarque la version 2 de xml-crypto fonctionne dans Pix et ne cause plus de problème de sécurité

🌈 Remarques

Vérifier le choix du package de validation XML

J'ai écarté @authenio/samlify-xsd-schema-validator car il a besoin d'un environnement java
J'ai pris @authenio/samlify-node-xmllint de manière arbitraire

💯 Pour tester

Se connecter sans compte existant avec le GAR, puis se reconnecter

[pix-service]

I'm deploying this PR to these urls:

• App: https://app-pr2296.review.pix.fr
• Orga: https://orga-pr2296.review.pix.fr
• Certif: https://certif-pr2296.review.pix.fr
• Admin: https://admin-pr2296.review.pix.fr
• API: https://api-pr2296.review.pix.fr/api/

Please check it out!

[jonathanperret]

@octo-topi le fork de samlify n'a rien à voir avec le passage à Node 10 mais servait plutôt à éviter une dépendance à Java qui était alors introduite par samlify. Nous avons estimé que l'apport de la validation XSD ne justifiait pas cette dépendance ni le coût en performances d'un appel à un exécutable Java externe.

Depuis samlify a introduit la possibilité de choisir le composant de validation, voire de désactiver la validation. En revanche sélectionner samlify-xsd-schema-validator nous ramènerait à la situation initiale de dépendance à Java ! Ce que montre bien l'échec du déploiement de cette PR…

[octo-topi]

@octo-topi le fork de samlify n'a rien à voir avec le passage à Node 10

Je m'étais basé sur le commentaire du commit 😄
"Pin xml-crypto version to fix samlify breakage"

Concernant la dépendance à Java, j'avai corrigé en local mais oublié de pusher 🤦

[jonathanperret]

Je m'étais basé sur le commentaire du commit 😄
"Pin xml-crypto version to fix samlify breakage"

C'est un message de commit qui mentionne samlify et xml-crypto certes mais ce n'est pas le commit qui introduit le fork de samlify, qui est plutôt celui-ci : 962feb3

Je ne vois pas comment tu arrives à cette conclusion en partant de ce commit 🤷‍♂️

[VincentHardouin]

@HEYGUL avait fait le changement dans une PR de montée de version #1767, que j'ai sans faire exprès écrasé. Cependant j'ai oublié de les reprendre, les jours passent et heureusement les commits restent : ba28472, 785c674

[octo-topi]

Je m'étais basé sur le commentaire du commit smile
"Pin xml-crypto version to fix samlify breakage"

C'est un message de commit qui mentionne samlify et xml-crypto certes mais ce n'est pas le commit qui introduit le fork de samlify, qui est plutôt celui-ci : 962feb3

Je ne vois pas comment tu arrives à cette conclusion en partant de ce commit man_shrugging

🤦 J'ai git blame sur xml-crypto au lieu de samlify
Merci pour l'explication et le commentaire d'issue d'origine !
tngan/samlify#224 (comment)

[octo-topi]

@HEYGUL avait fait le changement dans une PR de montée de version #1767, que j'ai sans faire exprès écrasé. Cependant j'ai oublié de les reprendre, les jours passent et heureusement les commits restent : ba28472, 785c674

J'aime ce proverbe ad-hoc ! merci @VincentHardouin
J'ai comparé les modifs: je vois que xml-crypto est toujours en dépendance directe package.json| (au lieu d'être en transitif - package-lock.json). On est d'accord que je peux enlever la dépendance directe ?

Ensuite, j'ai vu qu'on n'utilisait pas la validation dans la version @HEYGUL

samlify.setSchemaValidator({
  validate() {
    return Promise.resolve('skipped');
  }
});

Comme elle marche avec @authenio/samlify-node-xmllint, quelqu'un gradé en sécurité sait-il quoi préférer ?