-
Notifications
You must be signed in to change notification settings - Fork 246
Move ethkey crypto utils to parity crypto crate #210
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.
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
Changes from all commits
fd0e2ec
fc1298f
f1cf5dc
a6f8c9f
6940745
3c047ed
cad7824
f6c4702
9b7a0c9
34649b5
c1ba58a
377db06
3a24be0
dc7dc4d
46ece97
7ca69cc
5a77f83
a69d130
2acb917
fedfc96
669933f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| [package] | ||||||
| name = "parity-crypto" | ||||||
| version = "0.4.1" | ||||||
| version = "0.4.2" | ||||||
| authors = ["Parity Technologies <admin@parity.io>"] | ||||||
| repository = "https://github.com/paritytech/parity-common" | ||||||
| description = "Crypto utils used by ethstore and network." | ||||||
|
|
@@ -11,10 +11,13 @@ edition = "2018" | |||||
| [[bench]] | ||||||
| name = "bench" | ||||||
| harness = false | ||||||
|
|
||||||
| required-features = ["publickey"] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||
|
|
||||||
| [dependencies] | ||||||
| tiny-keccak = "1.4" | ||||||
| eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "a96ad75", optional = true } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
another update |
||||||
| ethereum-types = { version = "0.8.0", optional = true } | ||||||
| lazy_static = { version = "1.0", optional = true } | ||||||
| scrypt = { version = "0.2", default-features = false } | ||||||
| ripemd160 = "0.8.0" | ||||||
| sha2 = "0.8.0" | ||||||
|
|
@@ -24,9 +27,17 @@ aes = "0.3.2" | |||||
| aes-ctr = "0.3.0" | ||||||
| block-modes = "0.3.3" | ||||||
| pbkdf2 = "0.3.0" | ||||||
| rand = "0.6" | ||||||
|
dvdplm marked this conversation as resolved.
|
||||||
| rustc-hex = "2.0" | ||||||
| subtle = "2.1" | ||||||
| zeroize = "0.9.1" | ||||||
|
|
||||||
| [dev-dependencies] | ||||||
| criterion = "0.2" | ||||||
| hex-literal = "0.2" | ||||||
|
|
||||||
| [features] | ||||||
| default = [] | ||||||
| # public key crypto utils | ||||||
| # moved from ethkey module in parity ethereum repository | ||||||
| publickey = ["eth-secp256k1", "lazy_static", "ethereum-types"] | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure about the rational of naming the feature 'publickey' , a comment could be good.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to name this feature
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree in general, but this exact name (secp256k1) seems not the best option, because we already have eth-secp256k1 feature and it will be two similar names. Naming is the hardest part of coding :-( |
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| // Copyright 2015-2019 Parity Technologies (UK) Ltd. | ||
| // This file is part of Parity Ethereum. | ||
|
|
||
| // Parity Ethereum is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // Parity Ethereum is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with Parity Ethereum. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| //! Multiple primitives for work with public and secret keys and with secp256k1 curve points | ||
|
|
||
| use super::{SECP256K1, Public, Secret, Error}; | ||
| use secp256k1::key; | ||
| use secp256k1::constants::{CURVE_ORDER as SECP256K1_CURVE_ORDER}; | ||
| use ethereum_types::{BigEndianHash as _, U256, H256}; | ||
| use lazy_static::lazy_static; | ||
|
|
||
| /// Generation point array combined from X and Y coordinates | ||
| /// Equivalent to uncompressed form, see https://tools.ietf.org/id/draft-jivsov-ecc-compact-05.html#rfc.section.3 | ||
| pub const BASE_POINT_BYTES: [u8; 65] = [ | ||
| 0x4, | ||
|
grbIzl marked this conversation as resolved.
|
||
| // The X coordinate of the generator | ||
| 0x79, 0xbe, 0x66, 0x7e, 0xf9, 0xdc, 0xbb, 0xac, | ||
| 0x55, 0xa0, 0x62, 0x95, 0xce, 0x87, 0x0b, 0x07, | ||
| 0x02, 0x9b, 0xfc, 0xdb, 0x2d, 0xce, 0x28, 0xd9, | ||
| 0x59, 0xf2, 0x81, 0x5b, 0x16, 0xf8, 0x17, 0x98, | ||
| // The Y coordinate of the generator | ||
| 0x48, 0x3a, 0xda, 0x77, 0x26, 0xa3, 0xc4, 0x65, | ||
| 0x5d, 0xa4, 0xfb, 0xfc, 0x0e, 0x11, 0x08, 0xa8, | ||
| 0xfd, 0x17, 0xb4, 0x48, 0xa6, 0x85, 0x54, 0x19, | ||
| 0x9c, 0x47, 0xd0, 0x8f, 0xfb, 0x10, 0xd4, 0xb8, | ||
| ]; | ||
|
|
||
| lazy_static! { | ||
| pub static ref CURVE_ORDER: U256 = H256::from_slice(&SECP256K1_CURVE_ORDER).into_uint(); | ||
| } | ||
|
|
||
| /// Whether the public key is valid. | ||
| pub fn public_is_valid(public: &Public) -> bool { | ||
| to_secp256k1_public(public).ok() | ||
| .map_or(false, |p| p.is_valid()) | ||
| } | ||
|
|
||
| /// In-place multiply public key by secret key (EC point * scalar) | ||
| pub fn public_mul_secret(public: &mut Public, secret: &Secret) -> Result<(), Error> { | ||
| let key_secret = secret.to_secp256k1_secret()?; | ||
| let mut key_public = to_secp256k1_public(public)?; | ||
| key_public.mul_assign(&SECP256K1, &key_secret)?; | ||
| set_public(public, &key_public); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// In-place add one public key to another (EC point + EC point) | ||
| pub fn public_add(public: &mut Public, other: &Public) -> Result<(), Error> { | ||
| let mut key_public = to_secp256k1_public(public)?; | ||
| let other_public = to_secp256k1_public(other)?; | ||
| key_public.add_assign(&SECP256K1, &other_public)?; | ||
| set_public(public, &key_public); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// In-place sub one public key from another (EC point - EC point) | ||
| pub fn public_sub(public: &mut Public, other: &Public) -> Result<(), Error> { | ||
| let mut key_neg_other = to_secp256k1_public(other)?; | ||
| key_neg_other.mul_assign(&SECP256K1, &key::MINUS_ONE_KEY)?; | ||
|
|
||
| let mut key_public = to_secp256k1_public(public)?; | ||
| key_public.add_assign(&SECP256K1, &key_neg_other)?; | ||
| set_public(public, &key_public); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Replace a public key with its additive inverse (EC point = - EC point) | ||
| pub fn public_negate(public: &mut Public) -> Result<(), Error> { | ||
| let mut key_public = to_secp256k1_public(public)?; | ||
| key_public.mul_assign(&SECP256K1, &key::MINUS_ONE_KEY)?; | ||
| set_public(public, &key_public); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Return the generation point (aka base point) of secp256k1 | ||
| pub fn generation_point() -> Public { | ||
| let public_key = key::PublicKey::from_slice(&SECP256K1, &BASE_POINT_BYTES) | ||
| .expect("constructed using constants; qed"); | ||
| let mut public = Public::default(); | ||
| set_public(&mut public, &public_key); | ||
| public | ||
| } | ||
|
|
||
| fn to_secp256k1_public(public: &Public) -> Result<key::PublicKey, Error> { | ||
| let public_data = { | ||
| let mut temp = [4u8; 65]; | ||
|
dvdplm marked this conversation as resolved.
|
||
| (&mut temp[1..65]).copy_from_slice(&public[0..64]); | ||
| temp | ||
| }; | ||
|
|
||
| Ok(key::PublicKey::from_slice(&SECP256K1, &public_data)?) | ||
| } | ||
|
|
||
| fn set_public(public: &mut Public, key_public: &key::PublicKey) { | ||
| let key_public_serialized = key_public.serialize_vec(&SECP256K1, false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This intermediate copy is a bit annoying. I wonder if there's a way to avoid it? /cc @ordian
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The upstream API doesn't allow this either https://github.com/rust-bitcoin/rust-secp256k1/blob/f5c8a005f9d8948a16a0cb1c30c991df3e1f4a29/src/key.rs#L276. Unless we want to maintain our own fork ;)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So there's actually another copy in |
||
| public.as_bytes_mut().copy_from_slice(&key_public_serialized[1..65]); | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::super::{Random, Generator, Secret}; | ||
| use super::{public_add, public_sub, public_negate, public_is_valid, generation_point, public_mul_secret}; | ||
| use std::str::FromStr; | ||
|
|
||
| #[test] | ||
| fn public_addition_is_commutative() { | ||
|
grbIzl marked this conversation as resolved.
|
||
| let public1 = Random.generate().unwrap().public().clone(); | ||
| let public2 = Random.generate().unwrap().public().clone(); | ||
|
|
||
| let mut left = public1.clone(); | ||
| public_add(&mut left, &public2).unwrap(); | ||
|
|
||
| let mut right = public2.clone(); | ||
| public_add(&mut right, &public1).unwrap(); | ||
|
|
||
| assert_eq!(left, right); | ||
| } | ||
|
|
||
| #[test] | ||
| fn public_addition_is_reversible_with_subtraction() { | ||
| let public1 = Random.generate().unwrap().public().clone(); | ||
| let public2 = Random.generate().unwrap().public().clone(); | ||
|
|
||
| let mut sum = public1.clone(); | ||
| public_add(&mut sum, &public2).unwrap(); | ||
| public_sub(&mut sum, &public2).unwrap(); | ||
|
|
||
| assert_eq!(sum, public1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn public_negation_is_involutory() { | ||
| let public = Random.generate().unwrap().public().clone(); | ||
| let mut negation = public.clone(); | ||
| public_negate(&mut negation).unwrap(); | ||
| public_negate(&mut negation).unwrap(); | ||
|
|
||
| assert_eq!(negation, public); | ||
| } | ||
|
|
||
| #[test] | ||
| fn known_public_is_valid() { | ||
| let public = Random.generate().unwrap().public().clone(); | ||
| assert!(public_is_valid(&public)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn generation_point_expected() { | ||
| let point = generation_point(); | ||
| // Check the returned value equal to uncompressed form for sec2561k1 | ||
| assert_eq!(format!("{:x}", point), "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8"); | ||
|
grbIzl marked this conversation as resolved.
|
||
| } | ||
|
|
||
| #[test] | ||
| fn public_multiplication_verification() { | ||
| let secret = Secret::from_str("a100df7a048e50ed308ea696dc600215098141cb391e9527329df289f9383f65").unwrap(); | ||
| let mut public = generation_point(); | ||
| public_mul_secret(&mut public, &secret).unwrap(); | ||
| assert_eq!(format!("{:x}", public), "8ce0db0b0359ffc5866ba61903cc2518c3675ef2cf380a7e54bde7ea20e6fa1ab45b7617346cd11b7610001ee6ae5b0155c41cad9527cbcdff44ec67848943a4"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this value is from a spec somewhere it'd be great to document that. :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a randomly created secret |
||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a bunch of tests missing right? valid, multiplication, set_public etc?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added one more test for negation operation, that makes sense for me. For other methods I could not come up with anything better, than check returning value against its expected hardcoded const. So I've left them for now
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't that ok though? Like, these are unit tests, they don't have to be "interesting" in any way, just make sure that the calls do what is expected? Or what am I missing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Added such tests for other methods |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // Copyright 2015-2019 Parity Technologies (UK) Ltd. | ||
| // This file is part of Parity Ethereum. | ||
|
|
||
| // Parity Ethereum is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // Parity Ethereum is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with Parity Ethereum. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| //! ECDH key agreement scheme implemented as a free function. | ||
|
|
||
| use secp256k1::{self, ecdh, key}; | ||
| use super::{Error, Secret, Public, SECP256K1}; | ||
|
|
||
| /// Agree on a shared secret | ||
| pub fn agree(secret: &Secret, public: &Public) -> Result<Secret, Error> { | ||
|
dvdplm marked this conversation as resolved.
|
||
| let context = &SECP256K1; | ||
| let pdata = { | ||
| let mut temp = [4u8; 65]; | ||
| (&mut temp[1..65]).copy_from_slice(&public[0..64]); | ||
| temp | ||
| }; | ||
|
|
||
| let publ = key::PublicKey::from_slice(context, &pdata)?; | ||
| let sec = key::SecretKey::from_slice(context, secret.as_bytes())?; | ||
| let shared = ecdh::SharedSecret::new_raw(context, &publ, &sec); | ||
|
|
||
| Secret::import_key(&shared[0..32]) | ||
| .map_err(|_| Error::Secp(secp256k1::Error::InvalidSecretKey)) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: while it's technically correct that the changes here are not breaking the previous API, I wonder if it's not warranted to go with
0.5here anyway, just to signal the presence of a major new piece of functionality?/cc @ordian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're coming from and don't have a strong opinion on this. Having a minor release makes it easier to ensure other crates could use the latest version of parity-crypto (even via transitive dependencies), but that's a minor concern.