search

etalab / transport-site

Rendre disponible, valoriser et améliorer les données transports
https://transport.data.gouv.fr
194 stars 30 forks source link

URLs proxy cassées si HTTPS + IP, ou certificats "auto-signés" #3631

Closed thbar closed 11 months ago

thbar commented 11 months ago

Je crée ce ticket ici pour donner de la visibilité, car il y aura peut-être des dévs à faire, même si je suis d'avis de demander aux producteurs de corriger leurs serveurs de façon préférentielle.

Voir:

Une ressource proxy ciblant une url qui contient 1) un schéma https et 2) une IP plutôt qu'un nom de domaine va globalement ne pas fonctionner.

Le fait d'intégrer une adresse IP dans un certificat SSL est techniquement possible mais déconseillé et très peu rencontré en pratique.

Il faut par ailleurs également éviter les certificats "auto-signés", qui produisent les mêmes soucis.

Pour que cela fonctionne dans ces cas, il faudra "adoucir" (ou "fragiliser") le requêtage au niveau du serveur (à la curl -k), et je préfère évidemment l'éviter, et si on devait le faire, ne le faire que de façon spécifique et temporaire.

AntoineAugusti commented 11 months ago

Je ne pense pas qu'un fix soit nécessaire sur ce dépôt, mais il faut améliorer les choses côté transport-proxy-config. Je te rejoins sur le fait qu'on ne doit pas accepter ces URLs et demander aux producteurs de donner du HTTP sans certificat (bof) ou du HTTPS avec certificat classique (pas de difficulté particulière en 2023 normalement).

Le problème est survenu parce qu'on n'a pas suivi la procédure : le commit d'ajout de la configuration https://github.com/etalab/transport-proxy-config/commit/dfbb73913e1dfd01264c6a0c6cb055f6f910a465 a été fait directement sur master sans PR et donc sans exécution de tests/review par un tech, ce qui aurait pu permettre d'identifier ce problème.

Un autre problème qu'on a sur ce dépôt c'est certains flux ont des erreurs récurrentes/habituelles et que les tests échouent, sans faire un test complet sur toutes les URLs. Voir par exemple dernière exécution.

Je propose donc de :

Je vais m'en charger

thbar commented 11 months ago

Je ne pense pas qu'un fix soit nécessaire sur ce dépôt, mais il faut améliorer les choses côté proxy.

Je ne suis pas sûr de te comprendre : quand tu parles de "côté proxy", tu parles peut-être de la configuration proxy, et non du code proxy ? (qui est bien ici) ?

Si c'est oui c'est bien mon opinion aussi - autant corriger les choses côté serveurs producteurs et configuration chez nous (yml), toutefois on verra qui arrive à corriger ça dans la vraie vie, versus pas, à suivre !

Le problème est survenu parce qu'on n'a pas suivi la procédure faire en sorte qu'on ne puisse pas faire de commit sur master sans PR (peut-être impossible à faire comme c'est un dépôt privé…)

Alors oui si la procédure est aussi facile à casser, elle va recasser à un moment pour sûr.

Je confirme que c'est (à ma dernière vérification) impossible à faire car c'est un dépôt privé et qu'il y aurait des impacts (de mémoire, c'est flou) côté abonnement etalab GitHub.

Point à vérifier, clairement si on pouvait juste pour ce repo activer cette fonction, ça serait top !

améliorer les tests

Un truc qui serait pas mal si tu les changes un peu en profondeur, c'est de passer en Elixir pour les vérifications. On n'a jamais fait mais ça homogénéiserait les stacks et ça peut avoir certains avantages (même si il y a un petit coût d'entrée, setup GitHub action minimal et ré-écriture).

Je vais m'en charger

Ca marche merci !

Pour info, en anticipation, au cas où ça ne soit pas réglable côté producteurs (ça peut prendre du temps même si globalement dans le passé on a plutôt obtenu de bons résultats), j'ai regardé un peu du côté de la librairie utilisée dans le proxy, et ouvert un ticket pour savoir comment on pouvait faire ça et si c'était faisable:

thbar commented 11 months ago

Un truc qui serait pas mal si tu les changes un peu en profondeur, c'est de passer en Elixir pour les vérifications. On n'a jamais fait mais ça homogénéiserait les stacks et ça peut avoir certains avantages (même si il y a un petit coût d'entrée, setup GitHub action minimal et ré-écriture).

J'ajoute un cas concret tout de même: on pourrait directement utiliser Finch pour nos vérifications, et ainsi avoir plus de chances d'intercepter exactement les problèmes qui se poseront en production.

AntoineAugusti commented 11 months ago

Tu parles peut-être de la configuration proxy, et non du code proxy ?

Oui tout à fait, navré d'avoir manqué de clarté.

Un truc qui serait pas mal si tu les changes un peu en profondeur, c'est de passer en Elixir pour les vérifications.

Je vais peut-être me laisser tenter 😉

thbar commented 11 months ago

Le problème est survenu parce qu'on n'a pas suivi la procédure : le commit d'ajout de la configuration https://github.com/etalab/transport-proxy-config/commit/dfbb73913e1dfd01264c6a0c6cb055f6f910a465 a été fait directement sur master sans PR et donc sans exécution de tests/review par un tech, ce qui aurait pu permettre d'identifier ce problème.

Tu pointes vers un commit de @ChristinaLaumond mais je remarque que tu l'as fait aussi plusieurs fois:

CleanShot 2023-11-29 at 09 37 14@2x

Ceci pas pour te blâmer (et la dernière occurrence ne concerne que du TTL), mais pour indiquer qu'il y a un intérêt à faire en sorte que ça ne soit pas faisable du tout, si on peut, et ne pas le faire nous-même, pour ne pas ajouter à la confusion (ah oui mais moi je peux bypass la procédure parce que je sais ce que je fais etc, même si dans le passé on a nous même touché à des urls en direct aussi, voir l'historique).

Et à défaut, il faut recommuniquer sur cette procédure, surtout que @ChristinaLaumond était en congé maternité pendant un moment et que c'est un détail assez sioux à retenir quand on n'est pas dév, je trouve.

Mes suggestions supplémentaires donc dans l'interim d'un fix plus solide (protection de branche / corrections des urls / adaptation du proxy si pas possible d'avancer assez vite etc)

thbar commented 11 months ago

Oui tout à fait, navré d'avoir manqué de clarté.

Non non pas de souci, c'était pour être sûr. Merci de la clarification.

Je vais peut-être me laisser tenter 😉

Il y a des GitHub actions bien fichues normalement de mémoire (https://github.com/erlef/setup-beam, pas testé mais ça semble être le truc canonique actuellement et je l'ai repéré depuis un moment).

AntoineAugusti commented 11 months ago

J'ai déjà mis ça en place dans du GitHub Actions 😄

thbar commented 11 months ago

J'ai déjà mis ça en place dans du GitHub Actions 😄

Parfait ! C'est par ailleurs dans 100% des repositories Elixir que j'ai pu cloner / forker sur l'année, donc je pense qu'on peut y aller solidement.

thbar commented 11 months ago

Le problème est survenu parce qu'on n'a pas suivi la procédure : le commit d'ajout de la configuration etalab/transport-proxy-config@dfbb739 a été fait directement sur master sans PR et donc sans exécution de tests/review par un tech, ce qui aurait pu permettre d'identifier ce problème.

Ah et j'ajoute "the elephant in the room" car c'est quand même le gros truc:

En vrai, le problème "racine" est survenu pas là, mais en amont... parce que le fournisseur donne une url qui ne fonctionne absolument pas, même si tu la colles dans un navigateur ! Donc vérification simple à faire de leur côté en premier lieu.

thbar commented 11 months ago

@AntoineAugusti j'ai alerté Christina en DM sur ce point pour éviter qu'on n'ajoute d'autres urls problématiques ! Ainsi ça limitera la prolifération du problème.

thbar commented 11 months ago

Je réponds ici à un message en DM de @AntoineAugusti car ça a un intérêt en terme de comm entre nous (devs ou biz-devs) et les producteurs:

https://$$UNE_IP:PORT$$/XYZ/TripUpdate.pb ne passe pas chez toi ? Il y a un warning HTTPS mais après tu peux accéder aux données

J'ai répondu non et c'est assez classique car c'est une IP avec du SSL et qu'à 99,99% ça ne fonctionne pas dans la vraie vie.

Voilà le rendu dans deux navigateurs différents (avec la vraie url):

Safari

CleanShot 2023-11-29 at 10 59 56@2x

Chrome

C'est encore plus net:

CleanShot 2023-11-29 at 11 03 24@2x

Donc au final il n'y a zéro ambiguité, et non ça ne passe pas chez moi.

@AntoineAugusti est-ce que c'est passé à un moment chez toi ? Je suis étonné si c'est le cas.

Ca renforce l'idée qu'il faut renforcer notre processus, pas nous fouetter en interne.

On peut améliorer aussi le processus en recommandant (sauf pour le SIRI) de faire des tests basiques en navigateur, que ça soit chez le producteur ou chez nous.

AntoineAugusti commented 11 months ago

@thbar Je te confirme que j'ai bien eu le warning que tu montres dans Chrome, le problème est que si tu bypass une seule fois le warning tu peux y accéder sans problème par la suite.

thbar commented 11 months ago

Je te confirme que j'ai bien eu le warning que tu montres dans Chrome, le problème est que si tu bypass une seule fois le warning tu peux y accéder sans problème par la suite.

Ok alors on est sur du nominal - si tu acceptes c'est noté quelque part (dans la keychain sur Mac), et après c'est plié :smile:

AntoineAugusti commented 11 months ago

Je passerai les tests sur proxy-config en Elixir avec Finch d'ici fin 2023 (mark my words ™️)

thbar commented 11 months ago

Héhé nice :-)

Et de mon côté, j'ai confirmation qu'il vaut mieux qu'on déplace ce repository de config sur notre propre organisation.

Je gèrerai ce côté car il me semble (semble) que c'était moi qui avait créé un truc là dessus.

thbar commented 11 months ago

Je passerai les tests sur proxy-config en Elixir avec Finch d'ici fin 2023 (mark my words ™️)

You did it!

AntoineAugusti commented 11 months ago

Je ferme ce ticket, les tests ayant été adaptés dans l'autre dépôt. La suite dans #3648