crypto: replace ed25519-dalek with ed25519-consensus #624

joyqvq · 2022-07-29T18:38:08Z

crypto/src/ed25519.rs

crypto/src/tests/ed25519_tests.rs

crypto/src/ed25519.rs

joyqvq · 2022-07-30T17:19:07Z

config/tests/snapshots/config_tests__committee.snap

@@ -4,7 +4,7 @@ expression: committee
 ---
 {
  "authorities": {
-    "IP26ybELdYe7p7W8FjvOaeeW1x5O1EwQ/LRIhon3oUQ=": {
+    "ildi4hrBzbOHBELHe0w69Yx87bh3nQJw5tTx4vc2fXQ=": {


this is expected as rand 0.8.5 uses ChaChaRng for 12 rounds whereas rand 0.7.3 uses 20 rounds with reference to rust-random/rand#932

from 0.7.3:

/// The current algorithm used is the ChaCha block cipher with 20 rounds. /// This may change as new evidence of cipher security and performance /// becomes available.

from 0.8.5:

/// The current algorithm used is the ChaCha block cipher with 12 rounds. Please /// see this relevant [rand issue] for the discussion. This may change as new /// evidence of cipher security and performance becomes available. /// [rand issue]: https://github.com/rust-random/rand/issues/932

i can't think of a way to preserve the key files as the upgrade would affect all rand lib usage in narwhal

Thanks for the context! I expect this would change procedurally generated keys / formats only, correct?

yeah, only for the generate function in KeyPair trait that takes in the Rng

joyqvq · 2022-07-30T17:21:15Z

crypto/src/ed25519.rs

+    }
+}
+
+impl Serialize for Ed25519Signature {


as discussed serde_as would produce {"sig": base64_encoded_string} instead of a flattened base64_encoded_string, removed related functions in serder_helper

crypto/src/ed25519.rs

joyqvq · 2022-07-30T17:23:39Z

crypto/src/tests/ed25519_tests.rs

@@ -41,7 +41,7 @@ fn serialize_deserialize() {

    let private_key = kpref.private();
    let bytes = bincode::serialize(&private_key).unwrap();
-    let privkey = bincode::deserialize::<Ed25519PublicKey>(&bytes).unwrap();
+    let privkey = bincode::deserialize::<Ed25519PrivateKey>(&bytes).unwrap();


i dont know why this test passed on main branch.

It is indeed worrrisome, I suspect an abuse of ToFromBytes since ed25519-dalek and ed25519-consensus share the same underlying EdwardsPoint representation for the decompressed form of the key. Could we add a test for deserializing Ed25519PublicKey with a point known to not be on curve?

joyqvq · 2022-07-30T17:25:37Z

node/tests/staged/narwhal.yaml

@@ -43,8 +43,6 @@ Committee:
    - epoch: U64
 Ed25519PublicKey:
  NEWTYPESTRUCT: STR
-Ed25519Signature:
-  NEWTYPESTRUCT: BYTES


is there any downstream impact on this file? it seems like just a documentation

Yes, it lets us set up the binary representation manifest for our project. In fact, this makes me think we should open an issue to inspect the key types and use serde annotations to make sure the OnceCell is elided when serializing, and reset when deserializing. We should open an issue.

huitseeker

Thanks @joyqvq ! This looks good, I just have 3 questions:

Is this change sufficient to fix Use input Rng across all KeyPair impl #544 as well?
Can we write one negative test for invalid EdDSA serialization? The full issue is #500, no need to solve it, I just want to see a non-regression on the unit test you fix: you fix the code, I would like to see the non-fixed code fail.
We should open an issue to inspect our now pervasive use of OnceCell in key/signature material and make sure it complies with serde best practices.

huitseeker · 2022-08-03T12:43:50Z

config/tests/snapshots/config_tests__committee.snap

@@ -4,7 +4,7 @@ expression: committee
 ---
 {
  "authorities": {
-    "IP26ybELdYe7p7W8FjvOaeeW1x5O1EwQ/LRIhon3oUQ=": {
+    "ildi4hrBzbOHBELHe0w69Yx87bh3nQJw5tTx4vc2fXQ=": {


Thanks for the context! I expect this would change procedurally generated keys / formats only, correct?

huitseeker · 2022-08-03T12:50:52Z

crypto/src/tests/ed25519_tests.rs

@@ -41,7 +41,7 @@ fn serialize_deserialize() {

    let private_key = kpref.private();
    let bytes = bincode::serialize(&private_key).unwrap();
-    let privkey = bincode::deserialize::<Ed25519PublicKey>(&bytes).unwrap();
+    let privkey = bincode::deserialize::<Ed25519PrivateKey>(&bytes).unwrap();


It is indeed worrrisome, I suspect an abuse of ToFromBytes since ed25519-dalek and ed25519-consensus share the same underlying EdwardsPoint representation for the decompressed form of the key. Could we add a test for deserializing Ed25519PublicKey with a point known to not be on curve?

huitseeker · 2022-08-03T12:57:38Z

node/tests/staged/narwhal.yaml

@@ -43,8 +43,6 @@ Committee:
    - epoch: U64
 Ed25519PublicKey:
  NEWTYPESTRUCT: STR
-Ed25519Signature:
-  NEWTYPESTRUCT: BYTES


Yes, it lets us set up the binary representation manifest for our project. In fact, this makes me think we should open an issue to inspect the key types and use serde annotations to make sure the OnceCell is elided when serializing, and reset when deserializing. We should open an issue.

joyqvq · 2022-08-04T02:50:14Z

Thanks @joyqvq ! This looks good, I just have 3 questions:

Is this change sufficient to fix Use input Rng across all KeyPair impl #544 as well?

Can we write one negative test for invalid EdDSA serialization? The full issue is [cryptography] Integrate Wycheproof tests in unit testing fastcrypto#15, no need to solve it, I just want to see a non-regression on the unit test you fix: you fix the code, I would like to see the non-fixed code fail.

We should open an issue to inspect our now pervasive use of OnceCell in key/signature material and make sure it complies with serde best practices.

yes - although i only touched ed25519 files to keep the scope for review, will follow up a diff to upgrade rand across the board and eliminate the workaround
👍 good point, test added, will pick up full issue next
opened

huitseeker

Tip top! thanks a bunch, @joyqvq !

* crypto: replace ed25519-dalek with ed25519-consensus * cargo xclippy and hakari generate * serialize test negative case

…hal#624) * crypto: replace ed25519-dalek with ed25519-consensus * cargo xclippy and hakari generate * serialize test negative case

joyqvq requested review from huitseeker and kchalkias as code owners July 29, 2022 18:38

joyqvq commented Jul 29, 2022

View reviewed changes

crypto/src/ed25519.rs Show resolved Hide resolved

joyqvq requested a review from punwai July 29, 2022 18:41

joyqvq commented Jul 29, 2022

View reviewed changes

crypto/src/tests/ed25519_tests.rs Show resolved Hide resolved

joyqvq commented Jul 29, 2022

View reviewed changes

crypto/src/ed25519.rs Outdated Show resolved Hide resolved

joyqvq force-pushed the use-25519-consensus branch from 3e57023 to 7758e77 Compare July 30, 2022 17:16

joyqvq commented Jul 30, 2022

View reviewed changes

joyqvq force-pushed the use-25519-consensus branch from 7758e77 to adbcadf Compare July 30, 2022 17:26

joyqvq requested review from mwtian and removed request for punwai July 30, 2022 17:27

joyqvq force-pushed the use-25519-consensus branch from adbcadf to 438b36d Compare July 30, 2022 17:28

joyqvq requested a review from punwai July 31, 2022 20:40

joyqvq force-pushed the use-25519-consensus branch 3 times, most recently from 9f77c69 to 96cd44a Compare August 1, 2022 21:19

huitseeker reviewed Aug 3, 2022

View reviewed changes

joyqvq added 2 commits August 3, 2022 21:33

crypto: replace ed25519-dalek with ed25519-consensus

94e1357

cargo xclippy and hakari generate

055b0e6

joyqvq mentioned this pull request Aug 23, 2022

crypto: Inspect key type and make sure the OnceCell bytes are dealt with properly MystenLabs/fastcrypto#16

Closed

serialize test negative case

d129011

joyqvq force-pushed the use-25519-consensus branch from 96cd44a to d129011 Compare August 4, 2022 02:50

joyqvq requested a review from huitseeker August 4, 2022 02:50

huitseeker approved these changes Aug 4, 2022

View reviewed changes

joyqvq merged commit f067539 into main Aug 4, 2022

joyqvq deleted the use-25519-consensus branch August 4, 2022 13:46

huitseeker mentioned this pull request Aug 4, 2022

Check memory zeroization on the allocation of private keys #688

Closed

huitseeker pushed a commit that referenced this pull request Aug 5, 2022

crypto: replace ed25519-dalek with ed25519-consensus (#624)

419e096

* crypto: replace ed25519-dalek with ed25519-consensus * cargo xclippy and hakari generate * serialize test negative case

huitseeker mentioned this pull request Aug 5, 2022

chore: Update the Narwhal pointer MystenLabs/sui#3790

Merged

huitseeker mentioned this pull request Aug 5, 2022

chore: Update the Narwhal pointer MystenLabs/sui#3795

Merged

joyqvq mentioned this pull request Aug 12, 2022

crypto: ensure same length of the aggregated sigs and pubkeys #750

Merged

huitseeker mentioned this pull request Aug 12, 2022

chore(deps): Update the narwhal pointer to devnet branch MystenLabs/sui#3947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: replace ed25519-dalek with ed25519-consensus #624

crypto: replace ed25519-dalek with ed25519-consensus #624

joyqvq commented Jul 29, 2022 •

edited

Loading

joyqvq Jul 30, 2022 •

edited

Loading

huitseeker Aug 3, 2022

joyqvq Aug 4, 2022

joyqvq Jul 30, 2022

joyqvq Jul 30, 2022

huitseeker Aug 3, 2022

joyqvq Jul 30, 2022

huitseeker Aug 3, 2022

huitseeker left a comment

huitseeker Aug 3, 2022

huitseeker Aug 3, 2022

huitseeker Aug 3, 2022

joyqvq commented Aug 4, 2022

huitseeker left a comment

crypto: replace ed25519-dalek with ed25519-consensus #624

crypto: replace ed25519-dalek with ed25519-consensus #624

Conversation

joyqvq commented Jul 29, 2022 • edited Loading

joyqvq Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyqvq commented Aug 4, 2022

huitseeker left a comment

Choose a reason for hiding this comment

joyqvq commented Jul 29, 2022 •

edited

Loading

joyqvq Jul 30, 2022 •

edited

Loading