Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps(identity): update ed25519-dalek to 2.0 #4337

Merged
merged 8 commits into from
Aug 18, 2023
Merged

deps(identity): update ed25519-dalek to 2.0 #4337

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
015c28c
deps(identity): update ed25519-dalek to 2.0
jxs Aug 17, 2023
cd37165
update PR number
jxs Aug 17, 2023
abbcea9
update main Cargo.toml
jxs Aug 17, 2023
0555126
revert identity bump on Cargo.toml
jxs Aug 17, 2023
3449fee
address review
jxs Aug 17, 2023
de50a75
address Max review
jxs Aug 17, 2023
bfbeffe
update main Cargo.toml to identity 0.2.3
jxs Aug 17, 2023
6c6d653
Update identity/CHANGELOG.md
mxinden Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions identity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
## 0.2.3 - unreleased
mxinden marked this conversation as resolved.
Show resolved Hide resolved

- Fix [RUSTSEC-2022-0093] by updating `ed25519-dalek` to `2.0`.
See [PR 4337]

[RUSTSEC-2022-0093]: https://rustsec.org/advisories/RUSTSEC-2022-0093
[PR 4337]: https://github.com/libp2p/rust-libp2p/pull/4337

## 0.2.2

- Implement `from_protobuf_encoding` for RSA `Keypair`.
Expand Down
4 changes: 2 additions & 2 deletions identity/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "libp2p-identity"
version = "0.2.2"
version = "0.2.3"
Copy link
Member

@mxinden mxinden Aug 17, 2023 •
edited by thomaseizinger
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a bump of libp2p-identity in the root Cargo.toml. See CI failure: https://github.com/libp2p/rust-libp2p/actions/runs/5891817364/job/15979771326?pr=4337

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I tried it, but if I do it all other semver-check workflows fail, see here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cargo-semver-check does not adhere to the override?

rust-libp2p/Cargo.toml

Lines 111 to 118 in cbdbaa8

[patch.crates-io]
# Patch away `libp2p-idnentity` in our dependency tree with the workspace version.
# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
# we import via `rust-multiaddr`.
# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
libp2p-identity = { path = "identity" }

Copy link
Contributor

@thomaseizinger thomaseizinger Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised that this is showing up now, how did we ever bump this version? Maybe this failure happens now with the recent version change of cargo semver-checks?

The issue with libp2p-identity is that we need to patch it across the workspace and I'd assume that the temporary workspace created by cargo semver-checks does not support that. To properly fix this, we should move libp2p-identity out of the mono-repo. We should very rarely make breaking changes to it anyway because it sits at the very bottom of the dependency chain of the ecosystem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with libp2p-identity is that we need to patch it across the workspace and I'd assume that the temporary workspace created by cargo semver-checks does not support that.

Maybe @obi1kenobi you have seen this issue before, i.e. that cargo-semver-checks does not adhere to the workspace patches. Note that this is only a suspicion. Issue might as well be on the rust-libp2p end.

@thomaseizinger @jxs I am fine merging here as is, i.e. with the failing cargo-semver-checks check on the libp2p-identity run. Once libp2p-identity v0.2.3 is released we can update our root Cargo.toml. Objections?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't believe it copies patches. You can double-check and make sure by looking up the generated manifest inside target/semver-checks. But I'm pretty sure we never read patches from the source manifest so that's probably it.

If running cargo-semver-checks on an intermediate state that isn't right before a cargo publish, checking with patches included makes sense.

But is there risk that pre-publish checking while using patches might falsely say there are no semver issues, only for issues to come up after the crate gets published and used by users that don't have the patches locally?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated main Cargo.toml to identity 0.2.3

edition = "2021"
description = "Data structures and algorithms for identifying peers in libp2p."
rust-version = { workspace = true }
Expand All @@ -14,7 +14,7 @@ categories = ["cryptography"]
[dependencies]
asn1_der = { version = "0.7.6", optional = true }
bs58 = { version = "0.5.0", optional = true }
ed25519-dalek = { version = "1.0.1", optional = true }
ed25519-dalek = { version = "2.0", optional = true, features = ["rand_core"] }
libsecp256k1 = { version = "0.7.0", optional = true }
log = "0.4"
multihash = { version = "0.19.0", optional = true }
Expand Down
74 changes: 24 additions & 50 deletions identity/src/ed25519.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ use core::cmp;
use core::fmt;
use core::hash;
use ed25519_dalek::{self as ed25519, Signer as _, Verifier as _};
use rand::RngCore;
use std::convert::TryFrom;
use zeroize::Zeroize;

/// An Ed25519 keypair.
pub struct Keypair(ed25519::Keypair);
#[derive(Clone)]
pub struct Keypair(ed25519::SigningKey);

impl Keypair {
/// Generate a new random Ed25519 keypair.
Expand All @@ -42,15 +42,18 @@ impl Keypair {
/// of the secret scalar and the compressed public point,
/// an informal standard for encoding Ed25519 keypairs.
pub fn to_bytes(&self) -> [u8; 64] {
self.0.to_bytes()
self.0.to_keypair_bytes()
}

/// Try to parse a keypair from the [binary format](https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.5)
/// produced by [`Keypair::to_bytes`], zeroing the input on success.
///
/// Note that this binary format is the same as `ed25519_dalek`'s and `ed25519_zebra`'s.
pub fn try_from_bytes(kp: &mut [u8]) -> Result<Keypair, DecodingError> {
ed25519::Keypair::from_bytes(kp)
let bytes = <[u8; 64]>::try_from(&*kp)
.map_err(|e| DecodingError::failed_to_parse("Ed25519 keypair", e))?;

ed25519::SigningKey::from_keypair_bytes(&bytes)
.map(|k| {
kp.zeroize();
Keypair(k)
Expand All @@ -65,60 +68,41 @@ impl Keypair {

/// Get the public key of this keypair.
pub fn public(&self) -> PublicKey {
PublicKey(self.0.public)
PublicKey(self.0.verifying_key())
}

/// Get the secret key of this keypair.
pub fn secret(&self) -> SecretKey {
SecretKey::try_from_bytes(&mut self.0.secret.to_bytes())
.expect("ed25519::SecretKey::from_bytes(to_bytes(k)) != k")
SecretKey(self.0.to_bytes())
}
}

impl fmt::Debug for Keypair {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Keypair")
.field("public", &self.0.public)
.field("public", &self.0.verifying_key())
.finish()
}
}

impl Clone for Keypair {
fn clone(&self) -> Keypair {
let mut sk_bytes = self.0.secret.to_bytes();
let secret = SecretKey::try_from_bytes(&mut sk_bytes)
.expect("ed25519::SecretKey::from_bytes(to_bytes(k)) != k")
.0;

Keypair(ed25519::Keypair {
secret,
public: self.0.public,
})
}
}

/// Demote an Ed25519 keypair to a secret key.
impl From<Keypair> for SecretKey {
fn from(kp: Keypair) -> SecretKey {
SecretKey(kp.0.secret)
SecretKey(kp.0.to_bytes())
}
}

/// Promote an Ed25519 secret key into a keypair.
impl From<SecretKey> for Keypair {
fn from(sk: SecretKey) -> Keypair {
let secret: ed25519::ExpandedSecretKey = (&sk.0).into();
let public = ed25519::PublicKey::from(&secret);
Keypair(ed25519::Keypair {
secret: sk.0,
public,
})
let signing = ed25519::SigningKey::from_bytes(&sk.0);
Keypair(signing)
}
}

/// An Ed25519 public key.
#[derive(Eq, Clone)]
pub struct PublicKey(ed25519::PublicKey);
pub struct PublicKey(ed25519::VerifyingKey);

impl fmt::Debug for PublicKey {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Expand Down Expand Up @@ -170,27 +154,22 @@ impl PublicKey {

/// Try to parse a public key from a byte array containing the actual key as produced by `to_bytes`.
pub fn try_from_bytes(k: &[u8]) -> Result<PublicKey, DecodingError> {
ed25519::PublicKey::from_bytes(k)
let k = <[u8; 32]>::try_from(k)
.map_err(|e| DecodingError::failed_to_parse("Ed25519 public key", e))?;
ed25519::VerifyingKey::from_bytes(&k)
.map_err(|e| DecodingError::failed_to_parse("Ed25519 public key", e))
.map(PublicKey)
}
}

/// An Ed25519 secret key.
#[derive(Clone)]
pub struct SecretKey(ed25519::SecretKey);

/// View the bytes of the secret key.
impl AsRef<[u8]> for SecretKey {
fn as_ref(&self) -> &[u8] {
self.0.as_bytes()
}
}

impl Clone for SecretKey {
fn clone(&self) -> SecretKey {
let mut sk_bytes = self.0.to_bytes();
Self::try_from_bytes(&mut sk_bytes)
.expect("ed25519::SecretKey::from_bytes(to_bytes(k)) != k")
&self.0[..]
}
}

Expand All @@ -203,13 +182,8 @@ impl fmt::Debug for SecretKey {
impl SecretKey {
/// Generate a new Ed25519 secret key.
pub fn generate() -> SecretKey {
let mut bytes = [0u8; 32];
rand::thread_rng().fill_bytes(&mut bytes);
SecretKey(
ed25519::SecretKey::from_bytes(&bytes).expect(
"this returns `Err` only if the length is wrong; the length is correct; qed",
),
)
let signing = ed25519::SigningKey::generate(&mut rand::rngs::OsRng);
SecretKey(signing.to_bytes())
}

/// Try to parse an Ed25519 secret key from a byte slice
Expand All @@ -218,7 +192,7 @@ impl SecretKey {
/// returned.
pub fn try_from_bytes(mut sk_bytes: impl AsMut<[u8]>) -> Result<SecretKey, DecodingError> {
let sk_bytes = sk_bytes.as_mut();
let secret = ed25519::SecretKey::from_bytes(&*sk_bytes)
let secret = <[u8; 32]>::try_from(&*sk_bytes)
.map_err(|e| DecodingError::failed_to_parse("Ed25519 secret key", e))?;
sk_bytes.zeroize();
Ok(SecretKey(secret))
Expand All @@ -231,7 +205,7 @@ mod tests {
use quickcheck::*;

fn eq_keypairs(kp1: &Keypair, kp2: &Keypair) -> bool {
kp1.public() == kp2.public() && kp1.0.secret.as_bytes() == kp2.0.secret.as_bytes()
kp1.public() == kp2.public() && kp1.0.to_bytes() == kp2.0.to_bytes()
}

#[test]
Expand All @@ -249,7 +223,7 @@ mod tests {
fn ed25519_keypair_from_secret() {
fn prop() -> bool {
let kp1 = Keypair::generate();
let mut sk = kp1.0.secret.to_bytes();
let mut sk = kp1.0.to_bytes();
let kp2 = Keypair::from(SecretKey::try_from_bytes(&mut sk).unwrap());
eq_keypairs(&kp1, &kp2) && sk == [0u8; 32]
}
Expand Down