Supprimer l'affichage du mot de passe SMTP #10

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,

Cette PR adapte pour Mailshot une PR similaire proposée pour Facteur.

Même après le commit dc11359797, le formulaire de configuration de Mailshot 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 des clés secrètes pour Mailjet et Mandrill, mais il me semble que ce comportement est tout aussi désirable : il s'agit de clés secrètes, après tout.

++
Glop

Bonjour, Cette PR adapte pour Mailshot [une PR similaire proposée pour Facteur](../facteur/pulls/7). Même après le commit [dc11359797](commit/dc1135979714ccfb3efece43fb6a294630404082), le formulaire de configuration de Mailshot 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 des clés secrètes pour Mailjet et Mandrill, mais il me semble que ce comportement est tout aussi désirable : il s'agit de clés secrètes, après tout. ++ Glop
glopglop added 1 commit 2 years ago
glopglop changed title from Supprime l'affichage du mot de passe SMTP to Supprimer l'affichage du mot de passe SMTP 2 years ago
Collaborator

Hum,

l'avantage d'afficher partiellement les clès secretes, c'est de permettre à des gens qui gèrent un portefeuille de plusieurs clè secrete de reperer rapidement si a priori on est sur une bonne clè secrète.

Hum, l'avantage d'afficher partiellement les clès secretes, c'est de permettre à des gens qui gèrent un portefeuille de plusieurs clè secrete de reperer rapidement si _a priori_ on est sur une bonne clè secrète.
b_b commented 2 years ago
Collaborator

Ouep, ça va pas aider les wembrestres à débuguer leur envois de mails quand la personne n'est pas certaine d'avoir la bonne clé... Et puis si mes souvenirs sont bons, ces pages ne sont accessible qu'aux admins avec les droits de webmestre (ce qui limite déjà l'accès à l'info).

Ouep, ça va pas aider les wembrestres à débuguer leur envois de mails quand la personne n'est pas certaine d'avoir la bonne clé... Et puis si mes souvenirs sont bons, ces pages ne sont accessible qu'aux admins avec les droits de webmestre (ce qui limite déjà l'accès à l'info).
Owner

Oué, en matière sécurité, trop de contrainte tue la sécurité.

En l'occurrence, afficher un bout du mot de passe ou de la clé API est très utile avec la configuration des SMTP parce que ça marche rarement du premier coup, ou quand on revient 3 ans après parce que ça marche plus, on passe du temps à chercher pourquoi etc.

Si on peut pas savoir simplement si on a bien le bon mot de passe ou la bonne clé API, on va arriver à des comportement de contournement, comme aller voir le mot de passe ou la clé avec phpmyadmin, qui lui va l'afficher complètement en clair...

Donc c'est une vrai feature, qui rend service, tout en préservant la confidentialité autant que possible.
C'est un compromis, mais on est pas non plus en train de gérer les serveur de la NSA...

Donc je dirai plutot que non, ça doit rester tel quel, sauf a donner l'exemple d'un vrai scénario qui pose problème ?

Oué, en matière sécurité, trop de contrainte tue la sécurité. En l'occurrence, afficher un bout du mot de passe ou de la clé API est très utile avec la configuration des SMTP parce que ça marche rarement du premier coup, ou quand on revient 3 ans après parce que ça marche plus, on passe du temps à chercher pourquoi etc. Si on peut pas savoir simplement si on a bien le bon mot de passe ou la bonne clé API, on va arriver à des comportement de contournement, comme aller voir le mot de passe ou la clé avec phpmyadmin, qui lui va l'afficher complètement en clair... Donc c'est une vrai feature, qui rend service, tout en préservant la confidentialité autant que possible. C'est un compromis, mais on est pas non plus en train de gérer les serveur de la NSA... Donc je dirai plutot que non, ça doit rester tel quel, sauf a donner l'exemple d'un vrai scénario qui pose problème ?
Poster
Collaborator

Merci pour les retours !

Pour répondre rapidement sur la question des droits d'accès : par défaut, tou·tes les admin·es non restreint·es ont accès à ces pages de configuration, pas uniquement les webmestres.

Et si je comprends bien la difficulté qu'il peut y avoir à maintenir un trousseau de clés, il me semble que c'est une fausse bonne idée que de vouloir la résoudre en affichant un bout des infos secrètes pour être sûr·e qu'on a bien mis les bonnes. Lorsqu'on a un doute, j'aurais tendance à penser qu'il vaut mieux remettre la clé que l'on sait être la bonne et revalider le formulaire de configuration pour être certain·e.

Mais bon, n'utilisant pas moi-même les services de type Mailjet ou autre, peut-être que quelque chose m'échappe dans leur usage...

Néanmoins, concernant le mot de passe SMTP, je pense vraiment que ce qu'il y a à gagner à afficher un bout du mot de passe pour être sûr·e qu'on a mis le bon est faible par rapport à ce que ça représente d'en dévoiler au moins 20 % (jusqu'à 36 %, en fait, dans le cas d'un mot de passe de 11 caractères) à tou·tes les admin·es du site.

++
Glop

Merci pour les retours ! Pour répondre rapidement sur la question des droits d'accès : par défaut, tou·tes les admin·es non restreint·es ont accès à ces pages de configuration, pas uniquement les webmestres. Et si je comprends bien la difficulté qu'il peut y avoir à maintenir un trousseau de clés, il me semble que c'est une fausse bonne idée que de vouloir la résoudre en affichant un bout des infos secrètes pour être sûr·e qu'on a bien mis les bonnes. Lorsqu'on a un doute, j'aurais tendance à penser qu'il vaut mieux remettre la clé que l'on sait être la bonne et revalider le formulaire de configuration pour être certain·e. Mais bon, n'utilisant pas moi-même les services de type Mailjet ou autre, peut-être que quelque chose m'échappe dans leur usage... Néanmoins, concernant le mot de passe SMTP, je pense vraiment que ce qu'il y a à gagner à afficher un bout du mot de passe pour être sûr·e qu'on a mis le bon est faible par rapport à ce que ça représente d'en dévoiler au moins 20 % (jusqu'à 36 %, en fait, dans le cas d'un mot de passe de 11 caractères) à tou·tes les admin·es du site. ++ Glop
Collaborator

bah c'est ce point là qu'il faudrait régler : il y a pas de raison que les admins aient accès à cette page. Cela devrait être reservé aux webmestres.

Quant à la question du taux de masquage : pourquoi pas une constante indiquant le pourcentage.

Comme cela les ultra secure mettraient à 100.

C'est ce que j'ai mis dans la fonction du plugin saisies
https://git.spip.net/spip-contrib-extensions/saisies/src/branch/master/saisies_fonctions.php#L306

bah c'est ce point là qu'il faudrait régler : il y a pas de raison que les admins aient accès à cette page. Cela devrait être reservé aux webmestres. Quant à la question du taux de masquage : pourquoi pas une constante indiquant le pourcentage. Comme cela les ultra secure mettraient à 100. C'est ce que j'ai mis dans la fonction du plugin saisies https://git.spip.net/spip-contrib-extensions/saisies/src/branch/master/saisies_fonctions.php#L306
Poster
Collaborator

Effectivement, la question de l'accès aux pages de configuration par tou·tes les admin·es se pose, mais elle semble plus globale, comme soulevé par ce message sur la PR côté Facteur.

En attendant, la solution proposée par @maieul d'utiliser saisies_masquer_cle_secrete me semble être un bon compromis. Je peux proposer une modification de ma PR en ce sens, si ça vous convient.

Vaut-il mieux que je fasse directement appel à ce filtre dans les squelettes du formulaire (en rajoutant le plugin Saisies dans les dépendances de Mailshot) ? Ou bien que je duplique le code de la fonction saisies_masquer_cle_secrete() pour l'intégrer à mailshot_affiche_password_masque() sans rajouter de dépendance au plugin ? (Je pencherais pour la première solution, intuitivement, mais je préfère m'en remettre à votre avis pour cela.)

Merci !

++
Glop

Effectivement, la question de l'accès aux pages de configuration par tou·tes les admin·es se pose, mais elle semble plus globale, comme soulevé par [ce message](../facteur/pulls/7#issuecomment-2966) sur la PR côté Facteur. En attendant, la solution proposée par @maieul d'utiliser `saisies_masquer_cle_secrete` me semble être un bon compromis. Je peux proposer une modification de ma PR en ce sens, si ça vous convient. Vaut-il mieux que je fasse directement appel à ce filtre dans les squelettes du formulaire (en rajoutant le plugin Saisies dans les dépendances de Mailshot) ? Ou bien que je duplique le code de la fonction `saisies_masquer_cle_secrete()` pour l'intégrer à `mailshot_affiche_password_masque()` sans rajouter de dépendance au plugin ? (Je pencherais pour la première solution, intuitivement, mais je préfère m'en remettre à votre avis pour cela.) Merci ! ++ Glop
Collaborator

@glopglop tu viens de soulever un troll. Sache que le plugin saisies est très conflictuelle. Et que @cerdic est plutôt opposé au fait de faire dépendre des plugins de saisies. Donc je dirais : a priori duplique. mais attend les avis des autres, ma solutin ne fait pas forcément consensus non plus...

@glopglop tu viens de soulever un troll. Sache que le plugin saisies est très conflictuelle. Et que @cerdic est plutôt opposé au fait de faire dépendre des plugins de saisies. Donc je dirais : a priori duplique. mais attend les avis des autres, ma solutin ne fait pas forcément consensus non plus...
Owner

Non on ne va pas ajouter une dépendance à saisies, et ça sert à rien de dupliquer le code de la fonction de saisies, qui elle même est une variante de la fonction de ce plugin :)

Je vais ajouter une constante et reserver l'affichage partiel aux webmestres, ça me semble le meilleur compromis, indépendamment des autorisations d'accès à la page de configuration

Non on ne va pas ajouter une dépendance à saisies, et ça sert à rien de dupliquer le code de la fonction de saisies, qui elle même est une variante de la fonction de ce plugin :) Je vais ajouter une constante et reserver l'affichage partiel aux webmestres, ça me semble le meilleur compromis, indépendamment des autorisations d'accès à la page de configuration
Poster
Collaborator

OK, ça marche, merci !

OK, ça marche, merci !
Owner

Idem facteur : 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.
442009220e

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 :)

Idem facteur : 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. 442009220ee00666a054c0d88d2449607368e8ea 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.