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

Upstream this crate to RustCrypto? #38

Closed
tarcieri opened this issue May 5, 2021 · 14 comments
Closed

Upstream this crate to RustCrypto? #38

tarcieri opened this issue May 5, 2021 · 14 comments

Comments

@tarcieri
Copy link

tarcieri commented May 5, 2021

Hello 👋 I'm one of the devs working on the RustCrypto Project.

I was curious if you were interested in upstreaming this crate to our GitHub Org:

https://github.com/rustcrypto/hashes

Though we presently maintain a separate crate called sha-1, if you upstreamed this crate we would be happy to abandon our crate and maintain yours instead. We're doing something somewhat similar with the blake2 crate.

Anyway, if you're interested, let us know. Thanks!

@mitsuhiko
Copy link
Owner

Happy to assist. I wrote this crate out of a necessity at the time and I haven't spent much time with it since. Let me know what you need me to do.

@tarcieri
Copy link
Author

tarcieri commented May 5, 2021

A tentative plan would look something like:

  1. Import the latest version of the code into https://github.com/rustcrypto/hashes
  2. Do some housekeeping/upkeep on it
  3. Add impls of the digest traits
  4. Cut a new release

I'd be happy to do the work on the initial steps

@mitsuhiko
Copy link
Owner

I will try to have a look at this on the weekend.

@tarcieri
Copy link
Author

@mitsuhiko I was doing some work on RustCrypto/hashes and looked at potentially upstreaming this crate, but I wanted to ask you something first.

We've done a lot of work on the sha-1 crate lately, including ARMv8 optimizations which work on e.g. the Apple M1 now, and are getting ecstatic reviews:

RustCrypto/hashes#289 (comment)

Would you consider giving us the sha1 crate name and letting us continue working from the sha-1 codebase?

cc @newpavlov

@mitsuhiko
Copy link
Owner

@tarcieri still interested in this?

@tarcieri
Copy link
Author

Sure!

@mitsuhiko
Copy link
Owner

Was just trying to figure out if it's feasible to make a shimmed version of this to sha-1 and I have to say that the API of hashes is incredible complex for some current uses of this crate. Also pulls in the whole kitchen sink. I'm wondering if it makes sense to keep this crate around under a different name for people who don't need the whole rust crypto shebang.

@tarcieri
Copy link
Author

@mitsuhiko we can probably shim at least parts of the old API with deprecations that provide upgrade instructions.

I tried to do something similar here: RustCrypto/hashes#228

@mitsuhiko
Copy link
Owner

I might have a look tomorrow at how much work this is. But the old crate had quite a few trait bounds that apparently digests do not have in that form. (eg: it does not provide a regular PartialEq etc.) and the string formatting or digest parsing does not appear to be a concern of digests/sha-1.

@newpavlov
Copy link

newpavlov commented Jan 16, 2022

I think it will be difficult to have inherent methods on the sha1::Sha1 type like you have in sha1 v0.6. To reduce code duplication almost all hashes in RustCrypto implement only block-level traits, while slice-based traits are implemented on the buffering wrapper. It would've been really nice to be able implement inherent methods on Wrapper<ConcreteType> in a crate which defines ConcreteType, but, unfortunately, it's not possible today.

it does not provide a regular PartialEq etc.

Are those needed in practice? I think we could add them, but no one has asked us for them even though crates like sha2 are quite popular, so...

the API of hashes is incredible complex

I agree, but it comes from:

  • Supporting a number of less common use-cases (variable output hashes, XOFs, MACs).
  • Support of block-level mode of operation (rarely needed, but sometimes can be a useful optimization).
  • Code de-duplication across implementation crates.

Luckily, needs of almost all users are served by the simple Digest trait and the rare few have to venture deeper.

pulls in the whole kitchen sink

I respectfully disagree. :) Here is the list of its deps:

  • generic-array and its deps: inevitable since we provide common interfaces for different hash functions. Hopefully, it will be removed in an year or so, maybe even sooner.
  • cfg-if: can be removed, but it will make cfgs a bit too painfull. Hopefully, it (or cfg_match) will be included into std one day.
  • cpufeatures: needed for intrinsics-based backends. Since we support no_std, we can not rely on is_x86_feature_detected. Also it's a tiny bit more efficient than the std macro.
  • block-buffer: provides panic-free buffering shared across implementation crates.
  • crypto-common: a very simple crate with a bunch of common traits and convenience aliases.
  • digest: traits for hashes and MACs, and a bit of common code for them (the buffering wrapper and dev-utilities).

the string formatting or digest parsing does not appear to be a concern of digests/sha-1

Yes, we intentionally do not provide any built-in formatting and instead recommend using specialized crates directly. One of the reasons is to reduce number of pulled dependencies.

@mitsuhiko
Copy link
Owner

Are those needed in practice?

Unclear, but I can't just remove stuff from a crate that's popular and not consider existing users at all. The digest type for sure is used for comparisons right now and it's also commonly used as key in maps for content addressable stuff.

I respectfully disagree. :) Here is the list of its deps:

You can't argue that there it does not pull in the whole kitchen sink and then enumerate the kitchen sink. Your list of deps is not even complete. It also pulls in libc because cpufeatures depends on it. For people who are conscious on dependencies that's a bit deal.

Yes, we intentionally do not provide any built-in formatting and instead recommend using specialized crates directly. One of the reasons is to reduce number of pulled dependencies.

Well in that effort you failed already :)

Look I'm fine with you having the crate name but I'm not okay denying the existence of the code that was there before or existing users.

@mitsuhiko
Copy link
Owner

So my new plan is to publish another version of sha1 0.6.1 that depends on sha1_smol which I will release today which is just a re-release of 0.6.0. That way old users will continue to have a migration within what they are used to.

@newpavlov
Copy link

newpavlov commented Jan 16, 2022

The digest type for sure is used for comparisons right now and it's also commonly used as key in maps for content addressable stuff.

I think GenericArray handles such cases without any issues.

It also pulls in libc because cpufeatures depends on it.

It only pulls it on ARM targets, since it's the only way to detect target features with no_std support. Ideally, we would not pull cpufeatures with a disabled neon feature, but unfortunately IIRC gating dependencies on crate features does not work properly with the current version of cargo.

You can't argue that there it does not pull in the whole kitchen sink and then enumerate the kitchen sink.

My point is that this "kitchen sink" is mostly just stuff which which people usually copy-paste or reinvent (sometimes poorly) into their "zero-deps" crates. This is why I think that number of dependencies is a misleading metric.

I think it's worth to continue discussion about migration in the PR.

@mitsuhiko
Copy link
Owner

I'm going to close this issue now. This crate move to sha1-smol.

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

No branches or pull requests

3 participants