Skip to content

Replace ring with hmac for hmacs#139

Merged
ordian merged 2 commits into
paritytech:masterfrom
tomaka:hmac-no-ring
May 11, 2019
Merged

Replace ring with hmac for hmacs#139
ordian merged 2 commits into
paritytech:masterfrom
tomaka:hmac-no-ring

Conversation

@tomaka
Copy link
Copy Markdown
Contributor

@tomaka tomaka commented May 9, 2019

It's a bit of a painful birth because of #138, but this PR replaces ring with hmac for computing HMACs.
Part of #133

@dvdplm dvdplm requested a review from cheme May 10, 2019 10:46
@tomaka tomaka requested a review from ordian May 10, 2019 10:57
@tomaka tomaka mentioned this pull request May 10, 2019
@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 10, 2019

@tomaka could you clarify if the #138 applies to the current or the proposed implementation? Seems like the latter.

Building one of these types with the wrong slice length panics.

This seems to be different from https://briansmith.org/rustdoc/ring/hmac/struct.SigningKey.html#method.new

As specified in RFC 2104, if key_value is shorter than the digest algorithm's block length (as returned by digest::Algorithm::block_len, not the digest length returned by digest::Algorithm::output_len) then it will be padded with zeros. Similarly, if it is longer than the block length then it will be compressed using the digest algorithm.

Maybe we should use https://docs.rs/hmac/0.7.0/hmac/trait.Mac.html#method.new_varkey?

@tomaka
Copy link
Copy Markdown
Contributor Author

tomaka commented May 10, 2019

#138 applies to both. I decided to not touch the API here.

This seems to be different from https://briansmith.org/rustdoc/ring/hmac/struct.SigningKey.html#method.new

Oh, good remark. I really don't like this implicit behaviour in ring.

@cheme
Copy link
Copy Markdown
Collaborator

cheme commented May 10, 2019

@tomaka I did write some compat for webassembly on most of crypto crate a while back, for your code it looks like I did use Hmac : https://github.com/cheme/parity-common/blob/crypto-compat/parity-crypto/src/hmac_alt.rs there is a test to compare both impl (the branch did keep both impl).

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 10, 2019

I like @cheme's version better, because it can panic with a more meaningful message and uses static dispatch. As a downside it adds a dependency on hmac::Mac for public types though.

Copy link
Copy Markdown
Collaborator

@cheme cheme left a comment

Choose a reason for hiding this comment

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

This seems to work as intended, sorry for my previous comment I did not mean to say we should use this implementation but I misunderstood the previous comment and thought it could help.

It is probably a bit late but you also got secp256 alt implementation and pbkdf2 in the same branch and also lot of compat (not everything is good and I think some are not required anymore) in my wasm-test branch.

@ordian I am not sure having the Hmac is an issue (I did find back my test in ethereum openethereum/parity-ethereum@master...cheme:wasm-tests and it does not look like the api did change except for an import (for some other things it did change a lot eg secp where I add to put some curve math in a trait).
Edit: @ordian I think I misunderstood you, it does add some additional deps indeed (could probably find a way to avoid that).

@ordian
Copy link
Copy Markdown
Contributor

ordian commented May 10, 2019

@ordian I am not sure having the Hmac is an issue (I did find back my test in ethereum paritytech/parity-ethereum@master...cheme:wasm-tests and it does not look like the api did change except for an import (for some other things it did change a lot eg secp where I add to put some curve math in a trait).
Edit: @ordian I think I misunderstood you, it does add some additional deps indeed (could probably find a way to avoid that).

I meant the fact, that we're using hmac crate is an implementation detail that shouldn't be exposed in the public API. That way we can migrate to other crate later w/o introducing breaking changes.

Copy link
Copy Markdown
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Anyways I don't mind merging this PR as is and fixing #138 later.

@ordian ordian merged commit 9bd29e3 into paritytech:master May 11, 2019
@tomaka tomaka deleted the hmac-no-ring branch May 11, 2019 14:09

enum KeyInner {
Sha256(GenericArray<u8, U64>),
Sha512(GenericArray<u8, U128>),
Copy link
Copy Markdown
Contributor

@ordian ordian May 13, 2019

Choose a reason for hiding this comment

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

@tomaka should this be U32 and U64?

(found by @dvdplm)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants