Supprimer l'affichage du mot de passe SMTP #7

Closed
glopglop wants to merge 1 commits from dev/supprimer-affichage-du-mot-de-passe-smtp into master
glopglop commented 2 years ago
Collaborator

Bonjour,

Même après le commit 5358fa1bba, le formulaire de configuration de Facteur affiche toujours en clair 20 % du mot de passe du compte SMTP utilisé pour l'envoi de mails.

Même si c'est toujours mieux qu'un affichage complètement en clair du mot de passe, il me semble que l'intérêt de cet affichage (d'une partie) du mot de passe reste assez discutable, surtout au regard du risque ce que cela représente d'un point de vue sécurité.

La modification proposée ici supprime l'affichage du mot de passe et le remplace par une chaîne d'astérisques * de même longueur que le mot de passe. Ce dernier point reste lui-même discutable, et on pourrait très bien envisager d'afficher uniquement une chaîne de longueur fixe afin de ne même pas révéler la longueur du mot de passe.

Il est à noter que cette modification supprime aussi par la même occasion l'affichage de la clé secrète de la configuration Mailjet, mais il me semble que ce comportement est tout aussi désirable : il s'agit d'une clé secrète, après tout.

++
Glop

Bonjour, Même après le commit [5358fa1bba](commit/5358fa1bba82489e502de183e376c0d70d5584c5), le formulaire de configuration de Facteur affiche toujours en clair 20 % du mot de passe du compte SMTP utilisé pour l'envoi de mails. Même si c'est toujours mieux qu'un affichage complètement en clair du mot de passe, il me semble que l'intérêt de cet affichage (d'une partie) du mot de passe reste assez discutable, surtout au regard du risque ce que cela représente d'un point de vue sécurité. La modification proposée ici supprime l'affichage du mot de passe et le remplace par une chaîne d'astérisques `*` de même longueur que le mot de passe. Ce dernier point reste lui-même discutable, et on pourrait très bien envisager d'afficher uniquement une chaîne de longueur fixe afin de ne même pas révéler la longueur du mot de passe. Il est à noter que cette modification supprime aussi par la même occasion l'affichage de la clé secrète de la configuration Mailjet, mais il me semble que ce comportement est tout aussi désirable : il s'agit d'une clé secrète, après tout. ++ Glop
glopglop added 1 commit 2 years ago

Je suis d'accord que plusieurs personnes ayant accès relativement facilement à ces pages, ça donne accès à des choses qui peuvent permettre des problèmes plus graves que juste interne au site web (surtout que souvent le mot de passe SMTP est le même que celui pour la lecture aussi). Donc +1 pour vraiment le masquer. Mais quitte à le masquer oui je vois pas l'intérêt de laisser le bon nombre de caractères, ça n'apporte rien.

Je suis d'accord que plusieurs personnes ayant accès relativement facilement à ces pages, ça donne accès à des choses qui peuvent permettre des problèmes plus graves que juste interne au site web (surtout que souvent le mot de passe SMTP est le même que celui pour la lecture aussi). Donc +1 pour vraiment le masquer. Mais quitte à le masquer oui je vois pas l'intérêt de laisser le bon nombre de caractères, ça n'apporte rien.
glopglop changed title from Supprime l'affichage du mot de passe SMTP to Supprimer l'affichage du mot de passe SMTP 2 years ago
Poster
Collaborator

Merci pour le retour rapide !

Mais quitte à le masquer oui je vois pas l'intérêt de laisser le bon nombre de caractères, ça n'apporte rien.

Effectivement. Afin de pouvoir malgré tout faire la différence entre le cas où il y a un mot de passe et celui où il n'y en a pas, on peut remplacer la fonction facteur_affiche_password_masque() par :

function facteur_affiche_password_masque($pass){
        if ($pass === '') {
                return '';
        } else {
                return '****************';
        }
}

Voire même carrément remplacer dans les squelettes les occurrences de la forme

        [placeholder="(#ENV**{_smtp_password}|facteur_affiche_password_masque|attribut_html)"]

par

        [(#ENV**{_smtp_password}|strlen|oui)placeholder="****************"]

par exemple.

Merci pour le retour rapide ! > Mais quitte à le masquer oui je vois pas l'intérêt de laisser le bon nombre de caractères, ça n'apporte rien. Effectivement. Afin de pouvoir malgré tout faire la différence entre le cas où il y a un mot de passe et celui où il n'y en a pas, on peut remplacer la fonction `facteur_affiche_password_masque()` par : ```php function facteur_affiche_password_masque($pass){ if ($pass === '') { return ''; } else { return '****************'; } } ``` Voire même carrément remplacer dans les squelettes les occurrences de la forme ``` [placeholder="(#ENV**{_smtp_password}|facteur_affiche_password_masque|attribut_html)"] ``` par ``` [(#ENV**{_smtp_password}|strlen|oui)placeholder="****************"] ``` par exemple.
Owner

Moi je trouve pas que ce soit une bonne idée de le masquer, il faut pouvoir savoir si on a le bon mot de passe ou la bonne clé, cf mon commentaire dans spip-contrib-extensions/mailshot#10

Ces trucs là c'est toujours galère à debug quand ça marche plus ou même à faire marcher la première fois, si en plus on doit gérer les yeux bandés...

plusieurs personnes ayant accès relativement facilement à ces pages

Il me semble pas non, ce sont des pages sur autorisations, réservées aux webmestres non ?

Ou alors si ce n'est pas le cas peut-être qu'il faut masquer a tout le monde mais garder la feature d'affichage partiel pour les webmestres ?

Moi je trouve pas que ce soit une bonne idée de le masquer, il faut pouvoir savoir si on a le bon mot de passe ou la bonne clé, cf mon commentaire dans https://git.spip.net/spip-contrib-extensions/mailshot/pulls/10 Ces trucs là c'est toujours galère à debug quand ça marche plus ou même à faire marcher la première fois, si en plus on doit gérer les yeux bandés... > plusieurs personnes ayant accès relativement facilement à ces pages Il me semble pas non, ce sont des pages sur autorisations, réservées aux webmestres non ? Ou alors si ce n'est pas le cas peut-être qu'il faut masquer a tout le monde mais garder la feature d'affichage partiel pour les webmestres ?
Collaborator

Ou alors si ce n'est pas le cas peut-être qu'il faut masquer a tout le monde mais garder la feature d'affichage partiel pour les webmestres ?

si ce n'est pas le cas, c'est qu'il y a un problème, non ?

> Ou alors si ce n'est pas le cas peut-être qu'il faut masquer a tout le monde mais garder la feature d'affichage partiel pour les webmestres ? si ce n'est pas le cas, c'est qu'il y a un problème, non ?

Alors l'état c'est :

  • il n'y a aucune définition d'autorisation propre au plugin
  • dans le squelette, ça demande "autoriser(configurer, facteur)" donc ça utilise "configurer" tout court vu qu'il n'y a rien de plus précis
  • "configurer" tout court c'est pour TOUS les admins complets, pas juste 1 ou 2 webmestres, d'où le fait que je disais que c'était accessible à plus de monde (et pas des gens "technique", des gens qui s'occupent que de l'éditorial, etc aussi)
  • par ailleurs l'autorisation n'est que dans le squelette : si on insère le formulaire autre part, et n'importe qui peut le faire avec un modèle dans un contenu, alors ça ne sera même pas protégé par ça et n'importe quel rédac peut le voir (<formulaire|configurer_trucmuche> dans un brouillon d'article)

Donc

  1. Il faudrait que le formulaire lui-même demande une autorisation, pas juste le contexte parent.
  2. Il faudrait préciser une autorisation spécifique à "configurer, facteur", uniquement pour les webmestres et non pas les admins ?
Alors l'état c'est : - il n'y a aucune définition d'autorisation propre au plugin - dans le squelette, ça demande "autoriser(configurer, facteur)" donc ça utilise "configurer" tout court vu qu'il n'y a rien de plus précis - "configurer" tout court c'est pour TOUS les admins complets, pas juste 1 ou 2 webmestres, d'où le fait que je disais que c'était accessible à plus de monde (et pas des gens "technique", des gens qui s'occupent que de l'éditorial, etc aussi) - par ailleurs l'autorisation n'est que dans le squelette : si on insère le formulaire autre part, et n'importe qui peut le faire avec un modèle dans un contenu, alors ça ne sera même pas protégé par ça et n'importe quel rédac peut le voir (`<formulaire|configurer_trucmuche>` dans un brouillon d'article) Donc 1) Il faudrait que le formulaire lui-même demande une autorisation, pas juste le contexte parent. 2) Il faudrait préciser une autorisation spécifique à "configurer, facteur", uniquement pour les webmestres et non pas les admins ?
Collaborator

Là à chaud, je vois 2 failles de SPIP

  1. Il me semble que configurer devrait être reservé aux webmestres, quelque soit le formulaire, et pas seulement pour facteur. Normalement un formulaire de config c'est une fois pour toute, c'est fait par des gens qui ont une vision d'ensemble du site, etc. Mais bon, ca peut de discuter (notamment poru les formulaires de config de l'identité du site, mais même cela...)
  2. Le truc du formulaire de config insérable par raccourci. Hum. Je sais pas. A mon avis tout les raccourcis <formulaire_configurer_trucmuche> devrait automatiquement passer par la vérif d'autorisation.
Là à chaud, je vois 2 failles de SPIP 1. Il me semble que configurer devrait être reservé aux webmestres, quelque soit le formulaire, et pas seulement pour facteur. Normalement un formulaire de config c'est une fois pour toute, c'est fait par des gens qui ont une vision d'ensemble du site, etc. Mais bon, ca peut de discuter (notamment poru les formulaires de config de l'identité du site, mais même cela...) 2. Le truc du formulaire de config insérable par raccourci. Hum. Je sais pas. A mon avis tout les raccourcis `<formulaire_configurer_trucmuche>` devrait automatiquement passer par la vérif d'autorisation.

@maieul Je ne suis pas d'accord, il y a plein de configurations qui ne sont pas du tout propres aux webmestres, qui sont les personnes vraiment "techniques" qui installent le site physiquement, ont accès aux fichiers du serveur, etc. Je pense plutôt qu'effectivement la majeure partie des configs sont bien des choix de fonctionnement, donc à laisser aux admins complets : qui peuvent bien chercher, installer et activer des plugins, sans être webmestres. Un⋅e admin peut parfaitement décider d'avoir des téléformulaires, donc installer et activer Formidable, sans rien y connaitre de technique, c'est un choix éditorial et fonctionnel d'avoir des téléformulaires, et pareil pour la config du plugin, une fois activé, cette personne non technique ne doit pas être obligée d'appeler une personne qui a plus de droit dans la hiérarchie pour pouvoir configurer le plugin. Donc non non, par défaut c'est très bien "admins non restreints" pour "configurer".

En revanche, certains plugins où il y a des infos sensibles doivent être restreints spécifiquement uniquement aux webmestres. Pour ça lors des appels à autoriser() il faut bien penser à donner une info en plus, pas juste autoriser(configurer) mais toujours autoriser(configurer, truc) même quand on ne le définit pas et que ça retombe par défaut sur "configurer" seul. Mais ça permet de le définir plus précis pour les cas où on veut.

Après je ne sais pas si c'est à SPIP dans sa gestion particulère des formulaires "configurer_XXX" d'appeler de base une autorisation autoriser(configurer, XXX). Mais oui ça pourrait, c'est un gros changement par défaut mais ça pourrait et ça forcerait à une sécurité plus importante par défaut (actuellement toute personne même rédac etc peut voir le formulaire). Ça ce serait à discuter dans un ticket à faire du noyau.

@maieul Je ne suis pas d'accord, il y a plein de configurations qui ne sont pas du tout propres aux webmestres, qui sont les personnes vraiment "techniques" qui installent le site physiquement, ont accès aux fichiers du serveur, etc. Je pense plutôt qu'effectivement la majeure partie des configs sont bien des choix de fonctionnement, donc à laisser aux admins complets : qui peuvent bien chercher, installer et activer des plugins, sans être webmestres. Un⋅e admin peut parfaitement décider d'avoir des téléformulaires, donc installer et activer Formidable, sans rien y connaitre de technique, c'est un choix éditorial et fonctionnel d'avoir des téléformulaires, et pareil pour la config du plugin, une fois activé, cette personne non technique ne doit pas être obligée d'appeler une personne qui a plus de droit dans la hiérarchie pour pouvoir configurer le plugin. Donc non non, par défaut c'est très bien "admins non restreints" pour "configurer". En revanche, certains plugins où il y a des infos sensibles doivent être restreints spécifiquement uniquement aux webmestres. Pour ça lors des appels à `autoriser()` il faut bien penser à donner une info en plus, pas juste `autoriser(configurer)` mais toujours `autoriser(configurer, truc)` même quand on ne le définit pas et que ça retombe par défaut sur "configurer" seul. Mais ça permet de le définir plus précis pour les cas où on veut. Après je ne sais pas si c'est à SPIP dans sa gestion particulère des formulaires "configurer_XXX" d'appeler de base une autorisation `autoriser(configurer, XXX)`. Mais oui ça pourrait, c'est un gros changement par défaut mais ça pourrait et ça forcerait à une sécurité plus importante par défaut (actuellement toute personne même rédac etc peut voir le formulaire). Ça ce serait à discuter dans un ticket à faire du noyau.
Collaborator

mais les admin ne peuvent actuellement pas installer des plugins, ils peuvent justent les activer. Du coup il y a un truc pas forcément hyper cohérent non plus de ce côté là.

Mais en fait la question c'est "c'est quoi un statut de webmestre ?". Pour mon expérience, il y aussi l'idée que la personne s'assure que les besoins éditoriaux (qu'elle ne définit pas elle même) soient répondus de manière cohérente techniquement (ce qui implique notamment que les configs ne changent pas tous les 36 matins). Mais je suis d'accord que cela peut se discuter. Donc restons en à la situation actuel, où la config est autorisé pour les admin (sauf exception).

mais les admin ne peuvent actuellement pas installer des plugins, ils peuvent justent les activer. Du coup il y a un truc pas forcément hyper cohérent non plus de ce côté là. Mais en fait la question c'est "c'est quoi un statut de webmestre ?". Pour mon expérience, il y aussi l'idée que la personne s'assure que les besoins éditoriaux (qu'elle ne définit pas elle même) soient répondus de manière cohérente techniquement (ce qui implique notamment que les configs ne changent pas tous les 36 matins). Mais je suis d'accord que cela peut se discuter. Donc restons en à la situation actuel, où la config est autorisé pour les admin (sauf exception).
Owner

Pour info, tous les #FORMULAIRE_CONFIGURER_xxx sont protégés au charger() par une vérification d'autorisation automatique
https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/cvt_configurer.php#L39

Pour info, tous les `#FORMULAIRE_CONFIGURER_xxx` sont protégés au charger() par une vérification d'autorisation automatique https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/cvt_configurer.php#L39

Ok super c'est totomatique pour tous les configurer, donc pour Facteur il suffirait juste de dédier une fonction plus spécifique à "configurer, facteur" pour cloisonner aux webmestres par défaut, pas seulement admins.

Ok super c'est totomatique pour tous les configurer, donc pour Facteur il suffirait juste de dédier une fonction plus spécifique à "configurer, facteur" pour cloisonner aux webmestres par défaut, pas seulement admins.
Owner

J'ai donc ajouté une implémentation dans le core d'une fonction standard pour ce besoin récurrent
8916270302

et dupliqué l'implémentation ici pour la compat des anciens SPIP avec un appel automatique vers la fonction SPIP si elle existe.
b9910c38e9

Dans le squelette on demande un affichage partiel pour les webmestre uniquement, et une constante _SPIP_AFFICHE_MOT_DE_PASSE_MASQUE_PERCENT permet de definir la portion qu'on veut afficher pour les affichages partiels, dans le cas où ce n'est pas renseigné dans l'appel au filtre.

On doit donc couvrir tous les besoins et desiderata :)

J'ai donc ajouté une implémentation dans le core d'une fonction standard pour ce besoin récurrent https://git.spip.net/spip/spip/commit/8916270302c5dcea1a1c178e0fb2db8ad8cdcae8 et dupliqué l'implémentation ici pour la compat des anciens SPIP avec un appel automatique vers la fonction SPIP si elle existe. https://git.spip.net/spip-contrib-extensions/facteur/commit/b9910c38e9b8742ae004e9a994943a06bf262e21 Dans le squelette on demande un affichage partiel pour les webmestre uniquement, et une constante `_SPIP_AFFICHE_MOT_DE_PASSE_MASQUE_PERCENT` permet de definir la portion qu'on veut afficher pour les affichages partiels, dans le cas où ce n'est pas renseigné dans l'appel au filtre. On doit donc couvrir tous les besoins et desiderata :)
cerdic closed this pull request 2 years ago
cerdic deleted branch dev/supprimer-affichage-du-mot-de-passe- 2 years ago
Poster
Collaborator

Bonjour,

Oui, c'est parfait, en ce qui me concerne. Merci beaucoup !

++
Glop

Bonjour, Oui, c'est parfait, en ce qui me concerne. Merci beaucoup ! ++ Glop
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.