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.

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

update dependencies: digest-0.7, hmac-0.5 #7

Merged
merged 2 commits into from Nov 29, 2017

Conversation

warner
Copy link
Member

@warner warner commented Nov 28, 2017

This involves a few internal API changes. Applications or libraries which use
hkdf will almost certainly need to depend on e.g. sha2-0.7 instead of
sha2-0.6, so when this change is released, we should bump the hkdf version
number (from the current 0.2.1 to something like 0.3.0).

This involves a few internal API changes. Applications or libraries which use
hkdf will almost certainly need to depend on e.g. sha2-0.7 instead of
sha2-0.6, so when this change is released, we should bump the hkdf version
number (from the current 0.2.1 to something like 0.3.0).
@vladikoff
Copy link
Member

@warner this looks good. Something I noticed was that a new release of hmac is coming soon based on commits: https://github.com/RustCrypto/MACs/commits/master

and also there is a new version of sha2 (0.8) used in that.

@vladikoff vladikoff self-requested a review November 28, 2017 23:13
digest = "0.6.*"
hmac = "0.4.*"
digest = "0.7.*"
hmac = "0.5.*"
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/hkdf.rs Outdated
D::OutputSize: ArrayLength<u8>
{
pub fn new(ikm: &[u8], salt: &[u8]) -> Hkdf<D> {
let mut hmac = Hmac::<D>::new(salt);
// Hmac::new() is now defined to return a Result, but as far as I can
// tell, it can only return Ok.
Copy link
Member

Choose a reason for hiding this comment

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

@warner could you update this comment. Maybe Since hmac v0.5.0 'new' returns a Result ? (I think this will change more in the next version of hmac lib)

Copy link
Member

@vladikoff vladikoff left a comment

Choose a reason for hiding this comment

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

r+ with the comment nit

D::OutputSize: ArrayLength<u8>
{
pub prk: GenericArray<u8, D::OutputSize>,
}

impl<D> Hkdf<D>
where D: Digest + Default,
where D: Digest,
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

to be honest, I just removed that to see if it'd work, I'm not 100% positive it's unnecessary now

@warner
Copy link
Member Author

warner commented Nov 29, 2017

Yeah, I couldn't quite tell where they were going with that, and it feels like they incremented Cargo.toml before making a new release (also, I don't think there are git tags on whatever got released, so I'm finding it hard to compare versions).

It seems like the MAC trait is growing an error condition, where some MACs require a specific length of key (e.g. CMAC), so the overall MAC trait must return a Result. But HMAC can accept any length of key, so there's no way for it's new() to return an Error, so the unwrap() seems safe. I'll expand on that comment.

@warner
Copy link
Member Author

warner commented Nov 29, 2017

Thanks for the r+, I'll merge now.

@warner
Copy link
Member Author

warner commented Nov 29, 2017

oops, I can't merge. Could you land this when you get a chance?

@vladikoff vladikoff merged commit 916ae15 into RustCrypto:master Nov 29, 2017
@warner warner deleted the new-rustcrypto branch November 29, 2017 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants