Use `ed25519` + `signature` interop crates by tarcieri · Pull Request #102 · dalek-cryptography/ed25519-dalek

tarcieri · 2019-11-12T12:26:42Z

The ed25519 crate and the signature crates provide trait-based interoperability between Ed25519 libraries:

https://github.com/RustCrypto/signatures

This commit integrates the ed25519::Signature type, and changes the existing sign and verify methods to use the Signer and Verifier traits from the signature crate. Additionally, it replaces the current error types with an opaque one from the signature crate.

This has the drawback of requiring the Signer and Verifier traits are in scope in order to create signatures, but the benefit of interoperable support for other Ed25519 signers and verifiers. Presently
there are a number of these provided through the Signatory framework, along with support for HSM-backed Ed25519 signatures in the yubihsm crate:

tarcieri · 2019-11-12T12:29:02Z

I think this still is a bit rough around the edges. This is partly due to the crates being named ed25519 and signature, and those clashing with the existing module names. This is a bit easier to deal with in the 2018 edition module system, and I think it might be good to do a 2018 edition upgrade before landing this.

I'll try to leave some line notes about the parts I don't like.

Ideally I think we could get rid of the existing (private) signature module, and re-export the signature crate.

tarcieri · 2019-11-12T12:43:53Z

@@ -9,49 +9,6 @@

 //! Errors which may occur when parsing keys and/or signatures to or from wire formats.


This is perhaps the most invasive change in the current implementation: it eliminates the InternalError type entirely, and uses the opaque signature::Error type as SignatureError. Additionally, it drops failure, and uses the equivalent std::error::Error trait when available.

It's still possible to support InternalError using the current signature crate via its Error::source feature, but only when std is enabled:

https://docs.rs/signature/1.0.0-pre.1/signature/struct.Error.html#method.from_source

However, as noted in the docs above, the signature crate discourages this to avoid sidechannel leakage that might allow the attacker to either recover a private key or forge a signature.

If you'd prefer to keep the InternalError type, I can use Error::from_source to capture it, and expose it as Error::source, but only when the std feature is enabled (since it uses Box to store it internally, and exposes it through the std::error::Error trait)

tarcieri · 2019-11-12T12:49:50Z

+pub use ed25519_crate::{Signature, signature::Signature as SignatureTrait};
+
+/// Extension trait for the `Signature` type providing access to the
+/// `R` and `s` components of the signature.


Since the ed25519 crate provides the ed25519::Signature type (which internally is a 64-byte "octet string"), I opted to define an extension trait to provide the previous behavior. Let me know what you think of this approach. Alternatively I think this code could be factored to the relevant call sites.

tarcieri · 2019-11-12T12:52:44Z

    }
 }

+impl Signer<Signature> for Keypair {


This is the main interop trait for signing. It's generic around the return value (ed25519::Signature, in this case)

tarcieri · 2019-11-12T12:54:18Z


+impl Signer<Signature> for Keypair {
+    /// Sign a message with this keypair's secret key.
+    fn try_sign(&self, message: &[u8]) -> Result<Signature, SignatureError> {


This trait provides an infallible sign method impl'd in terms of try_sign, which exists to support e.g. HSMs which can have communication errors while signing.

In the case of this library, since signing is always infallible, it just returns Ok

tarcieri · 2019-11-12T12:54:44Z

+    }
+}
+
+impl Verifier<Signature> for Keypair {


This is the main interop trait for signature verification

tarcieri · 2019-11-12T12:55:44Z

    }
-}

-#[cfg(feature = "serde")]


The ed25519 crate provides a serde feature, but it uses the bag-of-bytes representation of a signature, and needed some more invasive changes which I didn't complete as part of the initial PR.

tarcieri · 2019-11-12T12:56:50Z

Looks like the tests are failing due to serde support, which I didn't finish in the initial PR. Will circle back on that.

tarcieri · 2019-11-14T18:07:51Z

-#[derive(Copy, Eq, PartialEq)]
-pub struct Signature {
+// Re-export the `ed25519` crate's Signature type
+pub use ed25519_crate::{Signature, signature::Signature as SignatureTrait};


I really dislike naming a trait *Trait, but it was necessary here to avoid a naming collision (partially exacerbated by the pre-2018 module system).

Fortunately importing it is rarely needed: the only methods the itself trait provides are from_bytes and as_slice, with equivalent functionality available through TryFrom<&[u8]> and AsRef<[u8]> impls. Otherwise its main function is to provide a set of other trait bounds.

Ideally I think the signature module could be eliminated and the signature crate publicly re-exported at the toplevel. That would allow this trait to be accessed as:

use ed25519_dalek::signature::Signature as _;

(I'm all ears on suggested alternative naming if that still seems confusing)

Put the trait in a src/signature/prelude.rs maybe? Although I personally find that "prelude" naming pretentious, and have "traits" submodules usually. In code this reads as Signature: traits::Signature, in docs sometimes as just Signature: Signature.

nickray · 2019-12-17T23:50:34Z

I think instead of forcing the "bag of bytes" on ed25519-dalek, either ed25519 should stop specifying the internal representation, or adopt the more mathematical approach from here and specify compressed point / scalar components.

EDIT: see #80 for a lengthy discussion on the mutual merits.

The `ed25519` crate and the `signature` crates provide trait-based interoperability between Ed25519 libraries: https://github.com/RustCrypto/signatures This commit integrates the `ed25519::Signature` type, and changes the existing `sign` and `verify` methods to use the `Signer` and `Verifier` traits from the `signature` crate. Additionally, it replaces the current error types with an opaque one from the `signature` crate. This has the drawback of requiring the `Signer` and `Verifier` traits are in scope in order to create signatures, but the benefit of interoperable support for other Ed25519 signers and verifiers. Presently there are a number of these provided through the Signatory framework, along with support for HSM-backed Ed25519 signatures in the `yubihsm` crate: - https://github.com/tendermint/signatory - https://github.com/tendermint/yubihsm-rs

tarcieri · 2020-03-08T23:17:37Z

Rebased off master, and updated to ed25519 crate v1.0.0-pre.2

Looks like there's still one remaining bug with serde serialization:

failures:

---- serialisation::serialize_deserialize_signature stdout ----
thread 'serialisation::serialize_deserialize_signature' panicked at 'assertion failed: `(left == right)`
  left: `ed25519::Signature([10, 126, 151, 143, 157, 64, 47, 1, 196, 140, 179, 58, 226, 152, 18, 102, 160, 123, 80, 16, 210, 86, 196, 28, 53, 231, 12, 157, 169, 19, 158, 63, 45, 154, 238, 7, 53, 185, 227, 229, 79, 108, 213, 80, 124, 252, 84, 167, 216, 85, 134, 144, 129, 149, 41, 81, 63, 120, 126, 100, 92, 59, 50, 11])`,
 right: `ed25519::Signature([64, 0, 0, 0, 0, 0, 0, 0, 10, 126, 151, 143, 157, 64, 47, 1, 196, 140, 179, 58, 226, 152, 18, 102, 160, 123, 80, 16, 210, 86, 196, 28, 53, 231, 12, 157, 169, 19, 158, 63, 45, 154, 238, 7, 53, 185, 227, 229, 79, 108, 213, 80, 124, 252, 84, 167, 216, 85, 134, 144, 129, 149, 41, 81])`', tests/ed25519.rs:251:9

tarcieri · 2020-03-08T23:25:51Z

-
-#[cfg(not(feature = "legacy_compatibility"))]
-#[inline(always)]
-fn check_scalar(bytes: [u8; 32]) -> Result<Scalar, SignatureError> {


This was unfortunate to lose in the rebase, and perhaps there's something we can still do yet to retain it, but note that the check_scalar check above (for the highest 3 bits being unset) was adapted into the ed25519 crate:

https://github.com/RustCrypto/signatures/blob/master/ed25519/src/lib.rs#L104

tarcieri · 2020-03-09T16:49:41Z

I'm not really happy with the state of this PR after rebasing, as it seems a lot of recent work was lost in the rebase.

So I think I'll redo it from scratch, and try to find a less invasive way of implementing it which preserves all of the current functionality.

tarcieri · 2020-03-17T17:37:26Z

Opened a new, less invasive version of this PR in #124

tarcieri commented Nov 12, 2019

View reviewed changes

tarcieri commented Nov 14, 2019

View reviewed changes

tarcieri mentioned this pull request Nov 17, 2019

signature v1.0 RustCrypto/signatures#55

Closed

tarcieri mentioned this pull request Nov 28, 2019

Provide std::error::Error impl for SignatureError #103

Closed

jamesmunns mentioned this pull request Jan 12, 2020

Deserialization of Keypair with Serde is broken #111

Closed

tarcieri force-pushed the ed25519-crate branch from ac69792 to c762c53 Compare March 8, 2020 23:14

tarcieri commented Mar 8, 2020

View reviewed changes

tarcieri closed this Mar 9, 2020

tarcieri deleted the ed25519-crate branch March 9, 2020 16:49

tarcieri mentioned this pull request Mar 17, 2020

Use ed25519 + signature interop crates #124

Merged

		@@ -9,49 +9,6 @@

		//! Errors which may occur when parsing keys and/or signatures to or from wire formats.

Conversation

tarcieri commented Nov 12, 2019

Uh oh!

tarcieri commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Nov 12, 2019

Uh oh!

tarcieri Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickray Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickray commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri commented Mar 8, 2020

Uh oh!

tarcieri Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Mar 9, 2020

Uh oh!

tarcieri commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tarcieri commented Nov 12, 2019 •

edited

Loading

tarcieri Nov 12, 2019 •

edited

Loading

tarcieri Nov 14, 2019 •

edited

Loading

nickray Dec 17, 2019 •

edited

Loading

nickray commented Dec 17, 2019 •

edited

Loading