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 RustCrypto crates #6

Merged
merged 1 commit into from May 18, 2022
Merged

Update RustCrypto crates #6

merged 1 commit into from May 18, 2022

Conversation

paolobarbolini
Copy link
Contributor

No description provided.

Comment on lines +76 to +84
H: Update + FixedOutput + CoreProxy,
H::Core: HashMarker
+ UpdateCore
+ FixedOutputCore
+ BufferKindUser<BufferKind = Eager>
+ Default
+ Clone,
<H::Core as BlockSizeUser>::BlockSize: IsLess<U256>,
Le<<H::Core as BlockSizeUser>::BlockSize, U256>: NonZero,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain these new constraints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mainly copy-pasted them from Hmac https://docs.rs/hmac/latest/src/hmac/optim.rs.html#20-36

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks I'll review these.

Comment on lines +118 to +119
let mut mac = <Hmac<H> as Mac>::new_from_slice(secret).unwrap();
<Hmac<H> as Update>::update(&mut mac, &to_bytes(time / step));
Copy link
Owner

Choose a reason for hiding this comment

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

Is update no longer a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still a method, but both the Update and the UpdateCore traits implement update, so I had to let the compiler know which of the two implementations to use

@BlackDex
Copy link

Nice, thanks @paolobarbolini, I was looking through the dependencies of Vaultwarden, and saw an easy update on duplicate crates here. This saves me creating an PR ;).

And @fosskers thanks for this crate, works really nice with Vaultwarden, would be great to have an updated release!

@fosskers fosskers merged commit 2b6409d into fosskers:master May 18, 2022
@fosskers
Copy link
Owner

I just released 1.1.0. Thanks for the PR!

@paolobarbolini
Copy link
Contributor Author

I just released 1.1.0. Thanks for the PR!

Shouldn't it be 2.0.0? I know you re-export the RustCrypto hash functions implementations that can be used for TOTP, but nothing prevents users from not using the re-export and importing the implementations themselves, so this is a breaking change.

@fosskers
Copy link
Owner

Could a break actually occur? Even if they imported say Sha256 from the underlying lib themselves and passed that in, it should still work without issue.

@paolobarbolini
Copy link
Contributor Author

Could a break actually occur? Even if they imported say Sha256 from the underlying lib themselves and passed that in, it should still work without issue.

Nope, because the traits are from a different version of the digest crate, so they won't match

@fosskers
Copy link
Owner

fosskers commented May 19, 2022

deps

totp-lite depends on digest 0.10, as do the three crypto libs, hmac, sha2, and sha-1. I suppose you're implying there would be breaks if the user was manually depending on digest 0.9 (already two years old)?

@paolobarbolini
Copy link
Contributor Author

paolobarbolini commented May 19, 2022

I suppose you're implying there would be breaks if the user was manually depending on digest 0.9 (already two years old)?

It might be improbable, maybe nobody will ever complain about this release, but upgrading a public dependency in a backwards incompatible way is a breaking change. It's annoying sometimes but it gives strong semver guarantees 😃

@fosskers
Copy link
Owner

Well I wouldn't want to break semver. So be it, I just released 2.0.0 and yanked 1.1.0 off crates.io

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

3 participants