Bump ed25519-dalek to 2.0.0 #32836

brooksprumo · 2023-08-14T20:10:14Z

Problem

+ cargo audit --ignore RUSTSEC-2020-0071 --ignore RUSTSEC-2023-0001
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 557 security advisories (from /home/.cargo/advisory-db)
    Updating crates.io index
    Scanning Cargo.lock for vulnerabilities (773 crate dependencies)
Crate:     ed25519-dalek
Version:   1.0.1
Title:     Double Public Key Signing Function Oracle Attack on `ed25519-dalek`
Date:      2022-06-11
ID:        RUSTSEC-2022-0093
URL:       https://rustsec.org/advisories/RUSTSEC-2022-0093
Solution:  Upgrade to >=2
Dependency tree:
ed25519-dalek 1.0.1

Summary of Changes

Upgrade ed25519-dalek to 2.0.0.

brooksprumo · 2023-08-14T20:11:02Z

Cargo.toml

@@ -180,7 +180,7 @@ dir-diff = "0.3.2"
 dirs-next = "2.0.0"
 dlopen2 = "0.5.0"
 eager = "0.1.0"
-ed25519-dalek = "=1.0.1"
+ed25519-dalek = "2"


These versions should likely be updated to include minor and update too.

brooksprumo · 2023-08-14T20:12:04Z

sdk/src/signer/keypair.rs

+    let signing_key = ed25519_dalek::SigningKey::from(extended.secret_key.to_bytes());
+    Ok(Keypair(signing_key))


Is this a correct conversion?

t-nelson · 2023-08-14T21:11:23Z

sdk/src/signer/keypair.rs

@@ -22,15 +22,15 @@ use {
 /// A vanilla Ed25519 key pair
 #[wasm_bindgen]
 #[derive(Debug)]
-pub struct Keypair(ed25519_dalek::Keypair);
+pub struct Keypair(ed25519_dalek::SigningKey);


these aren't the same size. seems like as good a time as any to abandon Keypair's crappy interface

these aren't the same size.

Ah, good point. Will the different size be an issue? IOW, is that an API-breaking change for the SDK?

Edit: Looking at the code again, it appears to me that the old ed25519_dalek::Keypair¹ and the new ed25519_dalek::SigningKey² are the same size.

seems like as good a time as any to abandon Keypair's crappy interface

Gotcha. This code and ideal usage is new for me; what would be the right interface for us here?

Footnotes

https://github.com/dalek-cryptography/ed25519-dalek/blob/1042cb60a07cdaacb59ca209716b69f444460f8f/src/keypair.rs#L36-L41 ↩

https://github.com/dalek-cryptography/ed25519-dalek/blob/57a8add0fd0710f53c6fcdf582480155c50bf345/src/signing.rs#L62-L67 ↩

fyi, i'm working in an alternative direction. sneak peek

diff --git a/sdk/src/signer/keypair.rs b/sdk/src/signer/keypair.rs index c1e0803a6b..f247df3329 100644 --- a/sdk/src/signer/keypair.rs +++ b/sdk/src/signer/keypair.rs @@ -1,4 +1,5 @@ #![cfg(feature = "full")] +#![deprecated(since = "1.17.0", note = "it's time")] use { crate::{

@t-nelson How's this going? Since #32871 has been merged, I'll be looking into moving forward with this PR again. However, should I wait on/ignore any Keypair changes until your work is merged?

i have a side pr that restricts the Keypair APIs similarly to dalek 2.0. i think these will be sufficient for 1.16.

i'm planning to totally deprecate the Keypair type from 1.17 forward and shift all interfaces to generics on our Signer trait. there are a few potential complications here due to existing overly constrained APIs, but i don't believe they're widely used

pinkforest · 2023-08-15T19:39:58Z

Cargo.toml

@@ -180,7 +180,7 @@ dir-diff = "0.3.2"
 dirs-next = "2.0.0"
 dlopen2 = "0.5.0"
 eager = "0.1.0"
-ed25519-dalek = "=1.0.1"
+ed25519-dalek = "2.0.0"
 ed25519-dalek-bip32 = "0.2.0"


Suggested change

ed25519-dalek-bip32 = "0.2.0"

ed25519-dalek-bip32 = "0.3.0"

fyi ed25519-dalek-bip32 just bumped out 0.3.0 release aligned to new API as well 🥳

ilya-bobyr · 2023-08-16T03:25:53Z

sdk/src/signer/keypair.rs

@@ -60,8 +59,8 @@ impl Keypair {
    }

    /// Gets this `Keypair`'s SecretKey
-    pub fn secret(&self) -> &ed25519_dalek::SecretKey {
-        &self.0.secret
+    pub fn secret(&self) -> ed25519_dalek::SecretKey {


I think this is a backward incompatible change, because ed25519_dalek::SecretKey is changing its version from 1.0.1 to 2.x.
Maybe it is OK here, but a similar change in PubKey for the borsh derivation from 0.9 to 0.10 created a lot of issues.

Yep, it is an API-breaking change.

This PR is not likely going to be merged, it's me exploring all the mess that needs to happen to update the ed25519_dalek dependency.

Let me know if you want to discuss the transition.
I was helping Leo with the borsh compatibility issue.

ilya-bobyr · 2023-08-16T03:28:16Z

sdk/src/ed25519_instruction.rs

@@ -29,9 +29,9 @@ pub struct Ed25519SignatureOffsets {
    message_instruction_index: u16,    // index of instruction data to get message data
 }

-pub fn new_ed25519_instruction(keypair: &ed25519_dalek::Keypair, message: &[u8]) -> Instruction {
+pub fn new_ed25519_instruction(keypair: &ed25519_dalek::SigningKey, message: &[u8]) -> Instruction {


This might be a backward incompatible change as well.
Even if SigningKey is the same as Keypair, previous version was using ed25519_dalek version 1.0.1, and it would be considered compatible with the same time from 2.x, if version dependencies are using the default SemVer constraints.

0xRigel · 2023-09-10T03:44:36Z

Whats the latest on this? Working on SIMD-0048 which is currently still affected by all the curve25519-dalek / ed25519-dalek/ zeroize dep conflicts.

t-nelson · 2023-09-11T21:36:01Z

Whats the latest on this? Working on SIMD-0048 which is currently still affected by all the curve25519-dalek / ed25519-dalek/ zeroize dep conflicts.

planning to replace the direct exposure of dependency implementation with traits and generics. deprioritized until 1.17 branch is cut because we need to break/deprecate some api.

in the meantime Keypair's interface was hardened to fail construction from illegal bytes.

it's not clear exactly what's in your way. can you make progress based on the information above?

0xRigel · 2023-09-12T19:17:14Z

Whats the latest on this? Working on SIMD-0048 which is currently still affected by all the curve25519-dalek / ed25519-dalek/ zeroize dep conflicts.

planning to replace the direct exposure of dependency implementation with traits and generics. deprioritized until 1.17 branch is cut because we need to break/deprecate some api.

in the meantime Keypair's interface was hardened to fail construction from illegal bytes.

it's not clear exactly what's in your way. can you make progress based on the information above?

Ah gotcha. Yeah will have to see if we can work around it. I think the main culprit is just the zeroize version conflict.
p256 0.13.2 -> elliptic-curve 0.13.5-> zeroize ^1.5. Hopefully we can use an older version of the p256 crate that is compatible. When is the 1.17 branch cut off?

t-nelson · 2023-09-12T20:39:19Z

Ah gotcha. Yeah will have to see if we can work around it. I think the main culprit is just the zeroize version conflict. p256 0.13.2 -> elliptic-curve 0.13.5-> zeroize ^1.5. Hopefully we can use an older version of the p256 crate that is compatible. When is the 1.17 branch cut off?

we'll cut 1.17 once mb is safely on 1.16. that's a bit blocked atm due to a stubborn memory corruption issue that a handful of nodes have exhibited upon upgrading. once that's resolved, we'll resume the cluster upgrade and 1.17 can be cut approximately two epochs after 1.16 has supermajority adoption

0xRigel · 2024-01-04T03:12:30Z

Heyo, just wanted to check in on this again @t-nelson .
Was just preparing a PR for SIMD-48 and realised that clippy is raising a dep issue on a dep that's enfored by ed25519-dalek=1.1.0. Build and tests seem to run just fine, just clippy is complaining.

The zeroize conflict is no longer present as I've found a version of the p256 crate that doesn't conflict.

brooksprumo added 2 commits August 14, 2023 13:53

Bump ed25519-dalek to 2.0.0

4eb6de2

fixup v1 to v2 changes in ed25519-dalek

61ffd99

brooksprumo added work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Aug 14, 2023

brooksprumo commented Aug 14, 2023

View reviewed changes

t-nelson reviewed Aug 14, 2023

View reviewed changes

brooksprumo added 5 commits August 15, 2023 11:36

fixup versions

255ffa7

fixup sdk cargo toml

cfbe40f

bump deps for rand and libsecp256k1

40d02e1

fixup keypair

97f5854

fixups for rand bump

753ed2b

pinkforest reviewed Aug 15, 2023

View reviewed changes

joncinque mentioned this pull request Aug 15, 2023

enable creation of ed25519 instruction without the signing keypair #32806

Closed

ilya-bobyr reviewed Aug 16, 2023

View reviewed changes

This was referenced Aug 16, 2023

v1.16: ci: ignore RUSTSEC-2022-0093 temporarily #32858

Merged

v1.14: ci: ignore RUSTSEC-2022-0093 temporarily #32859

Merged

joncinque mentioned this pull request Aug 17, 2023

Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

Merged

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 5, 2023

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 11, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 27, 2023

github-actions bot closed this Oct 4, 2023

This was referenced Oct 4, 2023

zeroize: Allow versions newer than 1.3 for curve25519-dalek #33516

Merged

Please release (3.2.2?) with relaxed zeroize dependency dalek-cryptography/curve25519-dalek#452

Closed

brooksprumo deleted the deps branch March 25, 2024 16:03

adrian-kong mentioned this pull request Aug 27, 2024

Update ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0 anza-xyz/agave#2752

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump ed25519-dalek to 2.0.0 #32836

Bump ed25519-dalek to 2.0.0 #32836

brooksprumo commented Aug 14, 2023

brooksprumo Aug 14, 2023

brooksprumo Aug 14, 2023

t-nelson Aug 14, 2023

brooksprumo Aug 15, 2023 •

edited

Loading

t-nelson Aug 15, 2023

brooksprumo Aug 21, 2023 •

edited

Loading

t-nelson Aug 21, 2023

pinkforest Aug 15, 2023

ilya-bobyr Aug 16, 2023

brooksprumo Aug 16, 2023

ilya-bobyr Aug 16, 2023

ilya-bobyr Aug 16, 2023

0xRigel commented Sep 10, 2023

t-nelson commented Sep 11, 2023

0xRigel commented Sep 12, 2023

t-nelson commented Sep 12, 2023

0xRigel commented Jan 4, 2024 •

edited

Loading

		let signing_key = ed25519_dalek::SigningKey::from(extended.secret_key.to_bytes());
		Ok(Keypair(signing_key))

Bump ed25519-dalek to 2.0.0 #32836

Bump ed25519-dalek to 2.0.0 #32836

Conversation

brooksprumo commented Aug 14, 2023

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

brooksprumo Aug 21, 2023 • 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

0xRigel commented Sep 10, 2023

t-nelson commented Sep 11, 2023

0xRigel commented Sep 12, 2023

t-nelson commented Sep 12, 2023

0xRigel commented Jan 4, 2024 • edited Loading

brooksprumo Aug 15, 2023 •

edited

Loading

brooksprumo Aug 21, 2023 •

edited

Loading

0xRigel commented Jan 4, 2024 •

edited

Loading