[TECH] Monter les versions des packages de l'API.

[VincentHardouin]

🦄 Problème

Nous utilisons beaucoup de package qui ne sont pas à jour.

Afin de fiabiliser, sécuriser la plateforme et profiter des dernières fonctionnalité, il faut tenir au maximum à jour les dépendances.

🤖 Solution

Monter les versions des dépendances suivantes :

• @hapi/inert : 6.0.1 -> 6.0.2 https://hapi.dev/module/inert/changelog/
• @hapi/hapi : 19.1.1 -> 19.2.0 20.0.0 Release Notes hapijs/hapi#4138
• @hapi/vision : 6.0.0 -> 6.0.1 https://hapi.dev/module/vision/changelog/
• airtable : 0.8.0 -> 0.9.0 https://github.com/Airtable/airtable.js/blob/master/CHANGELOG.md
• bcrypt : 4.0.1 -> 5.0.0 https://github.com/kelektiv/node.bcrypt.js/blob/master/CHANGELOG.md
• bunyan : 1.8.12 -> 1.8.1 https://github.com/trentm/node-bunyan/blob/master/CHANGES.md
• csv-parse : 4.11.1 -> 4.12.0 https://github.com/adaltas/node-csv-parse/blob/master/CHANGELOG.md
• eslint : 7.0.0 -> 7.6.0 https://github.com/eslint/eslint/blob/master/CHANGELOG.md
• eslint-plugin-mocha : 7.0.1 -> 8.0.0 https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/CHANGELOG.md
• fast-levenshtein : 2.0.6 -> 3.0.0 https://github.com/hiddentao/fast-levenshtein pas de changelog mais il semble qu'ils ont amélioré l'algo 👍
• hapi-swagger : 13.0.1 -> 13.1.0 https://github.com/glennjones/hapi-swagger/releases
• iconv-lite : 0.5.1 -> 0.6.2 https://github.com/ashtuchkin/iconv-lite/blob/master/Changelog.md
• jszip : 3.4.0 -> 3.5.0 https://github.com/Stuk/jszip/blob/master/CHANGES.md
• knex : 0.21.1 -> 0.21.4 https://github.com/knex/knex/blob/master/CHANGELOG.md
• lodash : 4.17.1 -> 4.17.19 https://github.com/lodash/lodash/wiki/Changelog
• mocha : 7.1.2 -> 8.1.1 https://github.com/mochajs/mocha/blob/master/CHANGELOG.md
• moment : 2.26.2 -> 2.27.0 https://github.com/moment/moment/blob/develop/CHANGELOG.md
• node-cache : 5.1.0 -> 5.1.2 https://github.com/node-cache/node-cache/releases
• nock : 12.0.3 -> 13.0.4 https://github.com/nock/nock/releases
• nyc : 15.0.1 -> 15.1.0 https://github.com/istanbuljs/nyc/blob/master/CHANGELOG.md
• pg : 7.18.2 -> 8.3.0 https://github.com/brianc/node-postgres/blob/master/CHANGELOG.md
• request-promise-native : 1.0.8 -> 1.0.9 Pas trouvé
• sib-api-v3-sdk : 7.2.2 -> 7.2.4 https://github.com/sendinblue/APIv3-nodejs-library/releases
• sinon : 9.0.2 -> 9.0.3 https://github.com/sinonjs/sinon/blob/master/CHANGELOG.md
• stream-to-promise : 2.2.0 -> 3.0.0 Pas trouvé

[pix-service]

I'm deploying this PR to these urls:

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

Please check it out!

[laura-bergoens]

Pas encore fait, mais je prends le check de version majeure sur node-pg

[HEYGUL]

Le soucis d'xml-crypto est lié à notre fork de samlify qui date. Il va falloir le mettre à jour également.

[octo-topi]

Pas encore fait, mais je prends le check de version majeure sur node-pg

Il n'y a que 2 entrées

• l'une mineure (Support passing a string of command line options flags)
• l'autre majeure mais rétro-compatible

Switch internal protocol parser & serializer to pg-protocol. The change is backwards compatible

[octo-topi]

J'ai testé un peu partout pix-app / pix-orga /swagger => OK .

[sbedeau]

Ok pour moi
Quelle est l'intention derrière l'hétérogénéité dans la gestion des versions de paquets ? Certains ont gagné un ^, d'autres l'ont perdu.

[VincentHardouin]

Ok pour moi
Quelle est l'intention derrière l'hétérogénéité dans la gestion des versions de paquets ? Certains ont gagné un ^, d'autres l'ont perdu.

Je parle au nom de GUL, je pense qu'il a du écrire les versions à la main : npm install lodash@20.0.0 et donc a perdu l'hétérogénéité.
Après, dans les faits cela change rien le package-lock.json verrouille nos versions. Cela change juste pour la version que npm va installer lors d'un : npm update <nom_du_paquet>. En suivant avec un npm outdated on voit très bien quelle est la dernière version qui doit-être installée.

[laura-bergoens]

Du coup le breaking-change côté node-pg c'est le changement d'un comportement par défaut lors de l'instanciation d'un client ->

Previously we white listed the parameters passed here and did slight massaging of some of them. The main breaking change here is that now if you do this:
const client = new Client({ ssl: true })
Now we will use the default ssl options to tls.connect which includes rejectUnauthorized being enabled. This means your connection attempt may fail if you are using a self-signed cert. To use the old behavior you should do this:
const client = new Client({ ssl: { rejectUnauthorized: false } })

source
Vu qu'on passe par knex, je pense (j'espère) qu'ils ont rendu cette contrainte invisible pour nous 🤞 donc je dirai qu'on n'a rien à faire de notre côté

[laura-bergoens]

@VincentHardouin Je veux aussi signaler qu'on a des petites nouveautés au niveau des logs :

je pense que ça serait bien de savoir de quoi il s'agit

[VincentHardouin]

@VincentHardouin Je veux aussi signaler qu'on a des petites nouveautés au niveau des logs :

je pense que ça serait bien de savoir de quoi il s'agit

Je viens de trouver ça : knex/knex#3921 (comment)

[jonathanperret]

La dernière version de samlify offre cette possiblité, cf ce commit).

Le lien vers le commit est cassé et je ne trouve pas de changement de version de samlify ?

[jonathanperret]

Du coup le breaking-change côté node-pg c'est le changement d'un comportement par défaut lors de l'instanciation d'un client ->

Previously we white listed the parameters passed here and did slight massaging of some of them. The main breaking change here is that now if you do this:
const client = new Client({ ssl: true })
Now we will use the default ssl options to tls.connect which includes rejectUnauthorized being enabled. This means your connection attempt may fail if you are using a self-signed cert. To use the old behavior you should do this:
const client = new Client({ ssl: { rejectUnauthorized: false } })

source
Vu qu'on passe par knex, je pense (j'espère) qu'ils ont rendu cette contrainte invisible pour nous 🤞 donc je dirai qu'on n'a rien à faire de notre côté

@jbuget avait déjà fait une tentative de passage à pg@8 dans #1433 et renoncé… est-ce que quelque chose a changé ?

[jonathanperret]

@jbuget avait déjà fait une tentative de passage à pg@8 dans #1433 et renoncé… est-ce que quelque chose a changé ?

J'avais creusé le sujet à l'époque et trouvé quelques infos supplémentaires mais j'avais mis ça uniquement dans un message Slack privé à @jbuget 🤦 … je remets l'essentiel ici :

Les certificats des serveurs PG de Scalingo sont pas nickels : le nom par lequel on y accède ne figure pas dans le certificat.

Error [ERR_TLS_CERT_ALTNAME_INVALID]: Hostname/IP does not match certificate's altnames: Host: pix-api-revi-8433.postgresql.dbs.scalingo.com. is not in the cert's altnames: DNS:f74cd2aa-6768-4d4f-8e8b-219680cadc65.pix-api-revi-8433.postgresql.dbs.scalingo.com, DNS:ip-10-0-0-214, DNS:f74cd2aa-6768-4d4f-8e8b-219680cadc65

En lisant les sources de node-postgres tu peux voir qu’il est possible d’ignorer ce souci en mettant PGSSLMODE=no-verify dans l’environnement (ce qui remet le comportement qu’on avait jusque là en fait)

En attendant que Scalingo ait corrigé les certificats (bon, faudrait déjà leur signaler… je vais m’en occuper). Dans tous les cas c’est de la config spécifique à l’environnement, ça ne va pas dans le code.

Si tu retournes voir le code de node-postgres (et pg-connection-string) tu verras que les options sont à peu près toutes accessibles via la connectionString ou l’environnement, ce qui est très bien parce que ça permet de ne pas mettre dans le code des spécificités de l’environnement.

du coup, je me demande, dès lors qu’on sera passé en v8, s’il ne faudra pas carrément enlever cette option du knex file, qui est très mal (voire pas du tout) documentée dans knex, maintenant que le mode par défaut est SSL

Aah oui, j’avais oublié cette ligne ssl: ('true' === process.env.DATABASE_SSL_ENABLED) — effectivement elle n’a rien à faire là, on doit pouvoir contrôler tout ça depuis l’environnement. En pg@7 je viens de tester (en exigeant les connexions SSL sur le PG de pix-api-review-pr1433), elle n’a aucun effet parce que l’URL fournie par scalingo contient déjà sslmode=require.

Avec pg@8 sans cette ligne c’est un poil plus compliqué tant que les certificats ne sont pas corrigés, parce qu’il faut à la fois forcer le SSL et désactiver la validation du certificat. Malheureusement pg@8 interprète sslmode=require (ou prefer) comme sslmode=verify-full (ce qui n’est pas conforme avec ce que fait un outil comme psql), et si on met PGSSLMODE=no-verify il désactive la vérification mais sans activer le SSL! Mais on peut s’en sortir avec : PGSSLMODE=no-verify et DATABASE_URL="$SCALINGO_POSTGRESQL_URL&sslmode=require …

[jonathanperret]

Quelques tests supplémentaire avec pg@8 du coup :

✅ Scalingo a corrigé le problème de certificats

$ </dev/null scalingo -a pix-api-review-pr1767 run -s S 'openssl s_client -starttls postgres -connect pix-api-revi-396.postgresql.dbs.scalingo.com:34164 | openssl x509 -noout -text' | grep DNS:
-----> Connecting to container [one-off-851]...
-----> Process 'openssl s_client -starttls postgres -connect pix-api-revi-396.postgresql.dbs.scalingo.com:34164 | openssl x509 -noout -text' is starting...

                DNS:25728d5e-515b-4a7e-839a-31721ec2ad9f.pix-api-revi-396.postgresql.dbs.scalingo.com, DNS:ip-10-0-0-89, DNS:25728d5e-515b-4a7e-839a-31721ec2ad9f, DNS:pix-api-revi-396.postgresql.dbs.scalingo.com

(on voit bien apparaître le nom d'hôte indiqué dans SCALINGO_POSTGRESQL_URL, à savoir pix-api-revi-396.postgresql.dbs.scalingo.com, dans la liste des noms d'hôte acceptables par le certificat).

✅ La connection SSL à la base fonctionne, grâce à PGSSLMODE=require

Attention, pour avoir un test comparable à ce qui se passe en production, il faut aller sur le dashboard de base de données Scalingo de la review app et bien forcer les connexions SSL!

Une fois ceci fait, on constate que l'application arrive à se connecter à PG:

Scalingo:pix-api-review-pr1767 ~ $ node -e "require('./db/knex-database-connection').knex('users').count().then(console.log).then(process.exit)"
[ { count: 53 } ]

Mais attention, ça ne marche que parce que la variable PGSSLMODE=require est présente dans l'environnement. Si on essaie sans :

Scalingo:pix-api-review-pr1767 ~ $ PGSSLMODE= node -e "require('./db/knex-database-connection').knex('users').count().then(console.log).then(process.exit)"
[ { count: 53 } ]
(node:81) UnhandledPromiseRejectionWarning: error: pg_hba.conf rejects connection for host "192.168.100.2", user "pix_api_revi_396", database "pix_api_revi_396", SSL off
    at createQueryBuilder (/app/node_modules/knex/lib/util/make-knex.js:313:26)
    at Object.knex (/app/node_modules/knex/lib/util/make-knex.js:99:12)
    at [eval]:1:42
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
…

Et ce bien que l'URL fournie par Scalingo (SCALINGO_POSTGRESQL_URL) se termine par sslmode=prefer.

Même si on force l'URL à contenir sslmode=require, ce n'est pas mieux :

Scalingo:pix-api-review-pr1767 ~ $ PGSSLMODE= DATABASE_URL=${SCALINGO_POSTGRESQL_URL/prefer/require} node -e "require('./db/knex-database-connection').knex('users').count().then(console.log).then(process.exit)"
(node:103) UnhandledPromiseRejectionWarning: error: pg_hba.conf rejects connection for host "192.168.100.2", user "pix_api_revi_396", database "pix_api_revi_396", SSL off

Bref, en l'état il faudra bien s'assurer d'avoir la variable d'environnement PGSSLMODE=require sur toutes les applications. Après une vérification rapide, c'est déjà le cas y compris en production. D'ailleurs, j'ai aussi vérifié : le comportement ci-dessus se produisait déjà avec pg@7, ce qui explique probablement la présence de cette variable.

✅ DATABASE_SSL_ENABLED n'a pas d'effet

DATABASE_SSL_ENABLED est à true sur toutes les applications mais la connexion s'établit correctement même si on l'enlève ou qu'on la passe à false. Cette variable (et donc cette ligne ) devraient donc pouvoir être supprimées.

[jonathanperret]

Un dernier petit résultat : si on met ssl=true dans l'URL de connexion (ex. DATABASE_URL=$SCALINGO_POSTGRESQL_URL&ssl=true) alors ça déclenche bien la connexion SSL même sans PGSSLMODE=require… pas sûr que ce soit une meilleure solution.