Skip to content

Use RustCrypto crates#310

Closed
newpavlov wants to merge 2 commits intomozilla:masterfrom
newpavlov:rustcrypto
Closed

Use RustCrypto crates#310
newpavlov wants to merge 2 commits intomozilla:masterfrom
newpavlov:rustcrypto

Conversation

@newpavlov
Copy link
Copy Markdown

This PR replaces rust-crypto and ring with RustCrypto crates. Both are overkill for just HMAC and hash functions, and were duplicating each other.

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 15, 2018

One thing I'd like to see is a comparison of the SHA-512 hash speed between these crates. ring has an implementation written in assembly so it's very fast. sccache spends a lot of time calculating SHA-512 hashes, so it's pretty important.

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 15, 2018

Unrelated, but hello from a different town named Moscow. :)

@newpavlov
Copy link
Copy Markdown
Author

newpavlov commented Oct 15, 2018

On my PC it's 170 MB/s vs. 260 MB/s in favour of ring. sha2 has also asm feature, but currently it uses less optimized assembly. (we have plans to improve performance, but currently it's not top of my list) If hashing speed is important how about migrating to BLAKE2? On my PC BLAKE2b from blake2 crate with enabled optimization features results in 615 MB/s and in 508 MB/s without them. (IIRC it should be possible to squeeze from BLAKE2b up to 1 TB/s, and bp variant can go even further)

hello from a different town named Moscow. :)

Hi! ;)

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 15, 2018

Changing the hash function is more invasive because it invalidates all existing caches, but it could be done. (we switched from SHA-1 to SHA-512 a while ago). I can't recall if there was a reason we didn't go with BLAKE2 when we did that. @alexcrichton: you mentioned blake2b in that issue but we decided to go with SHA-512 and I don't see that we ever mentioned why we chose not to use blake2b.

@alexcrichton
Copy link
Copy Markdown
Contributor

I think it may have been that in local testing it was as fast as sha-512, but that may have changed over time! I basically just went with the fastest one that was somewhat secure for this use case

@newpavlov
Copy link
Copy Markdown
Author

newpavlov commented Oct 16, 2018

So should I change hash function to BLAKE2b, revert SHA-256 implementation to ring, or leave as-is for now?

BTW note that some modern CPUs have dedicated SHA instructions, with which you can achieve 2 GB/s. I plan to use them in future in SHA crates. (it's not that difficult, but essentially blocked on lack of hardware on which I could test it)

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 23, 2018

So should I change hash function to BLAKE2b, revert SHA-256 implementation to ring, or leave as-is for now?

I don't have an immediate answer here--I'm open to a compelling argument. We should ensure that this doesn't break anything after all the sccache-dist work has landed (#323 should be the second-to-last PR for that).

@xen0n
Copy link
Copy Markdown

xen0n commented Jul 26, 2019

Hi, is there any more interest in getting this merged? I'd like to setup sccache on MIPS machines, but ring currently doesn't build on MIPS. I'm pretty sure porters of other niche architectures would appreciate this effort, too.

@newpavlov
Copy link
Copy Markdown
Author

I can update this PR after the decision will be made.

@chmanchester
Can you help here?

@apriori
Copy link
Copy Markdown

apriori commented Aug 27, 2019

On the topic of hash functions performance:

https://github.com/rurban/smhasher

@q66
Copy link
Copy Markdown

q66 commented Sep 27, 2019

This would help me significantly as well, as ring doesn't work on 64-bit or 32-bit powerpc (I at one point wrote a complete ppc64+ppc64le implementation, but ppc seemed trickier and not easily possible to implement completely, and I stopped having time to comply with the ring author's requirements of getting it upstreamed)

Fundamentally, I think relying on any (especially crypto) library that has assembly-only implementations with no fallbacks is hardly ideal, and in case of especially crypto, also irresponsible.

@newpavlov
Copy link
Copy Markdown
Author

Looks like all changes already landed as part of other PRs. :)

@newpavlov newpavlov closed this Jun 26, 2020
@newpavlov newpavlov deleted the rustcrypto branch June 26, 2020 03:01
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.

6 participants