Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Secret - from hash function, also validate data #4159

Merged
merged 4 commits into from
Jan 16, 2017
Merged

Secret - from hash function, also validate data #4159

merged 4 commits into from
Jan 16, 2017

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Jan 13, 2017

No description provided.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 13, 2017
@tomusdrw
Copy link
Collaborator

tomusdrw commented Jan 13, 2017 •
edited
Loading

I think we should validate the hash before turning it into secret: https://github.com/apoelstra/rust-secp256k1/blob/master/src/key.rs#L86
That implementation was recently changed to avoid blindly converting hash to secret and rather pass it through Secret::from_slice or Secret::from_hash

More: https://en.bitcoin.it/wiki/Private_key#Range_of_valid_ECDSA_private_keys

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 13, 2017
@NikVolf
Copy link
Contributor Author

NikVolf commented Jan 13, 2017

Hmm
Why Secret::from_slice does not validate anything then?

I thought of this as a shortcut of from_slice actually

@tomusdrw
Copy link
Collaborator

Darn, you're absolutely right. I wanted to use secp256_k1::key::SecretKey::from_slice there, but probably missed that when working on it:
https://github.com/ethcore/parity/blob/master/ethkey/src/secret.rs#L43
It should definitely be verified, good that you've noticed it.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 20be2de on ethkey-from into ** on master**.

@NikVolf
Copy link
Contributor Author

NikVolf commented Jan 13, 2017

@tomusdrw
ok i'll add to both here

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 13, 2017
@NikVolf NikVolf changed the title Secret from hash Secret - from hash function, also validate data Jan 13, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2837bcd on ethkey-from into ** on master**.

tomusdrw
tomusdrw reviewed Jan 13, 2017
View reviewed changes
ethkey/src/secret.rs
@@ -54,17 +63,11 @@ impl FromStr for Secret {

impl From<key::SecretKey> for Secret {
fn from(key: key::SecretKey) -> Self {
Self::from_slice(&key[0..32])
Self::from_slice_unchecked(&key[0..32])
.expect("`key::SecretKey` is valid (no way to construct invalid one); qed")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could get rid of this expect by moving .len() != 32 check to from_slice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced it with just assert
since SecretKey have got this check already

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 82df539 on ethkey-from into ** on master**.

rphmeier
rphmeier reviewed Jan 13, 2017
View reviewed changes
ethkey/src/secret.rs
}

pub fn from_hash(key: &H256) -> Result<Self, Error> {
Self::from_slice(&**key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function necessary? From an abstract standpoint getting a secret from a hash doesn't make a whole lot of sense and from a code standpoint the H256 already has a Deref impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

though it was from what pr started, now indeed there is no need in it

was hoping to use into() in few cases, now not :)

@NikVolf NikVolf closed this Jan 16, 2017
@NikVolf NikVolf reopened this Jan 16, 2017
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 16, 2017
rphmeier
rphmeier reviewed Jan 16, 2017
View reviewed changes
ethkey/src/secret.rs

pub fn from_slice(key: &[u8]) -> Result<Self, Error> {
let secret = key::SecretKey::from_slice(&super::SECP256K1, key)?;
Ok(secret.into())
Copy link
Contributor

Choose a reason for hiding this comment

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

could also .map(Into::into)

Copy link
Collaborator

@tomusdrw tomusdrw Jan 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

with .map_err(Into::into) :/ which doesn't look all that nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yeah. probably cleaner this way then :)

@gavofyork gavofyork merged commit f807aa6 into master Jan 16, 2017
@gavofyork gavofyork deleted the ethkey-from branch January 16, 2017 15:50
@tomusdrw tomusdrw mentioned this pull request Feb 2, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

@tomusdrw tomusdrw tomusdrw left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NikVolf @tomusdrw @coveralls @rphmeier @gavofyork