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

Use the sha1_smol library for SHA1 #587

Merged
merged 1 commit into from Feb 28, 2022
Merged

Use the sha1_smol library for SHA1 #587

merged 1 commit into from Feb 28, 2022

Conversation

KodrAus
Copy link
Member

@KodrAus KodrAus commented Feb 11, 2022

Closes #582

Follow-up for #581

This PR follows the crate renaming of sha1 so that we default to the zero-dependency sha1_smol crate, but support a fast-sha1 feature that pulls in the rust-crypto sha1 crate with its asm feature. This follows the same approach we've used for RNG where users get a small footprint by default, but can pull in more specialized implementations if they want to.

@KodrAus
Copy link
Member Author

@KodrAus KodrAus commented Feb 11, 2022

I've bumped our MSRV to 1.56.0 to use the new dependency resolver. We might not actually land this way (it's quite a jump), but I wanted to see if that was an option.

@newpavlov
Copy link

@newpavlov newpavlov commented Feb 11, 2022

As I wrote here, I doubt merit of this approach. You only make public surface of uuid more complex this way. And note that you SHOULD NOT enable the asm feature in library crates. It's intended to be enabled only by end application.

Also I would like to add that number of dependencies is a controversial metric to blindly follow. In the case of RustCrypto crates we publish common parts of algorithm implementations (e.g. block-buffer) as separate crates, while in a "zero-dependency" crate you get those parts re-implemented or copy-pasted. Having separate crates not only improves code reuse, but also helps with making our crates "zero panic" (I am quite certain that sha1_smoll contains unreachable panics).

In addition to that, you should also consider general popularity in ecosystem (and thus chances that a dep will be reused in a project dependency tree) and maintenance status (org vs single developer).

@KodrAus
Copy link
Member Author

@KodrAus KodrAus commented Feb 11, 2022

Ah, this could possibly benefit from a note alongside the asm feature in sha1’s Cargo file to suggest it should only be used by applications and not by libraries.

Yeh, number of dependencies doesn’t personally appeal to me much (neither do MSRVs, but that’s a separate thing). It’s a balance. But it comes up enough from users of this library that I’ve taken a stance on it to minimise them. In practice I don’t think sha1 needs a lot of active maintenance, and I wouldn’t expect the supply-chain risk of sha1_smol to be any different to sha1. So the things that matter to uuid when deciding on these internal implementations (assuming they’re of a roughly equal level of quality) are:

  • Who pulls in fewer dependencies?
  • Who is faster?
  • Who is most portable?
  • Who enables more const?

The first two are the ones users make the most noise about.

@KodrAus
Copy link
Member Author

@KodrAus KodrAus commented Feb 12, 2022

I've opened #589 to try clean up our optional dependencies a bit. It uses the rust-crypto sha1 0.10 crate to try keep that refactoring disentangled from this PR, so you don't feel like you need to track multiple places for sha1 / sha1_smol.

@KodrAus KodrAus changed the title add fast-sha1 Use the sha1_smol library for SHA1 Feb 27, 2022
@KodrAus
Copy link
Member Author

@KodrAus KodrAus commented Feb 28, 2022

Ok, at this stage we'll stick with sha1_smol as the private implementation of SHA1 for v5 UUIDs since it aligns more closely with uuid's goals for dependencies (even when those goals aren't necessarily the same as my own personal opinions), but since we keep these dependencies private it's not a decision we're locked into forever and can reconsider in the future.

Thanks for all your input @newpavlov and for your efforts in supporting a full and usable crypto ecosystem 🙇 It's such a cross-cutting concern that it's nice to have you jump in and look to support consumers the way you have.

@KodrAus KodrAus merged commit f0d5956 into main Feb 28, 2022
19 checks passed
@KodrAus KodrAus deleted the feat/fast-sha1 branch Mar 14, 2022
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.

2 participants