Mais tu n’as pas abstrait les fonctions de signature / vérification ? Tout cela est encapsulé dans keyring.ts dans Duniter, l’objet se construit en fournissant la clé privée (et publique) puis on ne la redemande jamais.
Avec cela le coût de refactoring est pratiquement nul.
Tu parles là d’un utilisateur qui veut faire une synchro, Inso parle de celui qui veut valider la blockchain relativement au protocole. Celui-là veut justement vérifier chaque bloc un à un, selon les règles établies pour la version dudit bloc.
Si bien sûr on à une grosse couche d’abstraction dans ed25519.rs, et même une autre couche d’abstraction par dessus dans le module keys pour être agnostique de l’algo de crypto.
Le problème que j’ai rencontré quand jai migré sous ring c’est que la couche d’abstraction partait du postulat que le Keypair était Clonable, ce que ring ne permet pas. J’ai donc été obligé de repenser entièrement la couche d’abstraction pour permettre que le KeyPair ne soit pas clonable, ce qui est bien plus compliqué qu’il n’y parait si l’on veut rester safe et multithread.
Il est vrai que passer d’un lib de crypto très contraignante a une lib moins contraignante devrait pouvoir ce faire en gardant la même API pour la couche d’abstraction, donc sans avoir a refactorer toute la codebase, et je pense que c’est le cas. Mais ça reste tout de même du dev a faire, je n’ai pas modifié la couche d’abstraction. Pour tester TweetNaCl j’ai fait un binding manuel très salement dans un TU temporaire, ce n’est pas utilisable “tel quel” dans tout le projet.
Migrer ma couche d’abstraction de crypto sur la version buggée de tweetnacl me demanderais certes moins de boulot a court terme que de dev Duniter 1.7.20 selon ma proposition plus haut.
Mais ce serait me binder sur du C (ce que j’essaye d’éviter au maximum), moins performant et moins secure que ring, perdre en perf, perdre en sécurité et perdre en simplicité (a cause du binding C) c’est un coût que j’estime bien plus cher que de maj la lib de crypto de Duniter
De plus, a long terme ça représenterai plus de boulot au global, car se serait censé être temporaire et tôt ou tard il m’aurait fallu remigrer sur ring une fois la lib de crypto de Duniter mise a jours. Cela équivaudrait a creuser un trou pour le reboucher.
Oui vous avez raison, donc il faut cette liste, toutefois je n’en ai pas besoin pour dev Duniter 1.7.20 car il comportera encore les 2 lib de crypto et sera donc toujours en mesure de vérifier tous les blocs.
J’indiquerai dans la doc du protocole que les blocs v10 et v11 doivent être vérifiées avec une version précise de TweetNaCl.
J’aimerais bien @Moul et @Inso (ou d 'autres) s’occupent de faire cette liste, afin que je n’ai pas tout a faire sur ce dossier
Désolé si je te fais répéter, mais pour être bien clair : Dunitrust ne vérifiera pas la signature de ces blocs ? Et quid de la vérification du reste des règles du protocole v10 et v11 ?
Au contraire, tu a raison, tout changement de protocole doit être parfaitement clair, je formaliserai ça dans la RFC v12 (et j’en profiterai pour déplacer au préalable la RFC v12 actuelle en v13).
Dans un premier temps non, si une liste exhaustive des blocs v10 et v11 invalide est faite alors je l’inscrirai en dur dans Dunitrust et il vérifiera alors tout les blocs v10 et v11 qui ne sont pas dans cette liste.
Toutes les autres règles seront toujours vérifiées, il s’agit de ne skipper que la vérification de la signature et que pour les blocs dont le champ version est <= 11
D’après le billet détaillé sur le bug, ce dernier ne présente pas de risque de sécurité, on peut donc en parler publiquement sans crainte :
However, beyond the wrongness of the resulting x-coordinate3, and because this error happens at the end of computations steps there is no risk it could lead to greater damages like for instance revealing bits of user’s secret.
Merci @Moul, a noter que (et je l’ai indiqué dans la RFC v12) ont a potentiellement le même problème avec les transactions signées par Duniter, je pense notamment aux transactions émises par remuniter.
Et je sais pour avoir eu le problème qu’il existe au moins 1 transaction invalide dans la blockchain g1
C’est valable pour tout ce qui est signé, notamment identités, certifications, adhésions. Et vu les volumes, il est tout à fait possible que ce soit déjà arrivé sur un document de WoT.
Les documents signés par cette bibliothèque boguée intégrée dans Duniter sont les blocs et potentiellement les transactions de Remuniter. Je sais pas quelle bibliothèque Remuniter utilise pour signer ses transactions.
La signature des autres documents devraient être valides, étant donné que les clients utilisent libsodium (j’ai pas vérifié pour Césium).
Cesium Android utilise la libsodium (la lib C).
Pour Cesium desktop et web, je dois vérifier. Le nom du fichier JS est nacl_factory, mais je ne sais plus si cela provenait de TweetNacl ou nacl-js.
Je regarde dès que j’ai machine sous la main.
OK, donc pour la version web, Cesium utilise js-nacl v1.1.0
Dans lea prochaine version, je vais faire la mise à jour en version > 1.3.0 qui est basé sur libsodium 1.0.11.
J’ai découpé les changements en une quinzaine de petits commit pour plus de lisibilité
D’abord 3 commits pour rétablir la CI sur la branche 1.7 (la CI n’avais pas tournée depuis plus de 6 mois sur 1.7 et ne fonctionnais plus).
Puis 11 commits pour le dev a proprement parlé, j’ai pris l’initiative de renommer certaines constantes pour plus de clarté.
Pour reviewer, je vous invite a regarder les changements commit par commit et à lire les nom des commits, ça vous aidera a comprendre les changements plus rapidement.
J’ai également rajouté un test d’intégration pour tester les sauts de version du protocole (test/integration/protocol-version-jump.ts), ce qui m’a parmi de constater un bug dans la formule suivante :
// More than 70% of the computing network converted? Let's go to next version.
if (Math.floor(nbNoncesWithNextVersionCode / uniqIssuersInFrame.length) > 0.6) {
return constants.BLOCK_NEW_GENERATED_VERSION
}
La présence de Math.floor impliquait que le saut de version ne pouvait se faire que si l’on avait 100% des issuers de la fenêtre courante qui étaient prêt, ce qu’on a du avoir pour le passage en V11 du coup on ne s’en ai pas aperçu
@cgeek peut tu reviewer cette MR quand tu aura un peu de temps ? N’hésite pas a me faire part de tes retours
Merci Eloïs, je regarde dès que je trouve un créneau. Il faut bien me laisser une semaine je dirais.
Oui je me rappelle de ce point. Il y avait bien des tests pourtant, mais ceux-ci simulaient un accord à 100% sans que je m’en sois rendu compte. Heureusement c’est passé quand même, mais je crois que ça s’est fait un petit peu dans la douleur (fork).
@Moul c’est bon j’ai traité tous les retours de ta review, peut tu passer tes remarques en résolues ou apporter des précisions si elles en le sont pas ?
@cgeek en traitant les retours de moul j’ai un test qui est passé au rouge alors qu’il était vert lors de mon dev initial. J’ai rejoué ce test sans mon dev (branche test-ci), et le test est rouge quand même, il n’a donc a priori pas de lien avec mon dev et est devenu rouge pour une autre raison inconnue.
Il s’agit du test test/integration/protocol/v0.5-transactions.js. Il échoue à la ligne 41 :
await cat.sendMoney(51, tac);
Après analyse en debug, il s’avère que cat n’a aucune source de monnaie. La cause : aucun DU n’a été créé dans les 3 blocs commités. J’ai l’intuition qu’il y a un lien avec le « temps » du premier DU mais je n’ai rien trouvé après 1h d’investigation, peut tu regarder ce test ? Merci
Oui je n’ai pas compris non plus pourquoi ce test échoue soudainement, mais ce n’est rien d’important car ça concerne un très vieux test. Je pense que c’est un problème d’isolation des tests entre eux.
