Switch from SHA-512 to BLAKE3

[GeorgeHahn]

BLAKE3 is designed to be a very high performance cryptographic hash. The BLAKE3 team has shown significantly higher single-thread performance than SHA-512 on modern server hardware [1].

This change resulted in a minor improvement to observed local build times.

Comparison

cargo check on sccache repo (at commit 32e40bb) with a warm cache for each hash. Ten samples were collected for each hash.

SHA512 mean: 60.06 seconds
BLAKE3 mean: 57.77 seconds

This data shows a mean improvement of 2.29 seconds (about 4%). My statistics skills are rough, but this appears to be a very statistically significant improvement.

This test was performed on an Intel i5-7600K and a fairly ancient SATA3 SSD. It would be interesting to see a test result from a higher performance machine. The BLAKE3 team's performance analysis shows 8.5x faster single threaded performance vs SHA-512 on an AWS c5d.metal instance.

Raw data

sha512 initial cold build: 1m 47s
blake3 initial cold build: 1m 52s
sha512 build times: 57.02s, 1m 03s, 1m 02s, 58.72s, 1m 00s, 59.48s, 1m 02s, 1m 01s, 58.54s, 58.85s
blake3 build times: 54.87s, 59.82s, 56.72s, 56.91s, 1m 00s, 56.46s, 57.85s, 57.13s, 59.35s, 58.57s

Related issues & PRs:

• #504: Reevaluate hash function used
• #310: Use RustCrypto crates
• #108: Faster hash algorithm?
• #109: Switch from SHA-1 to SHA-512

References:

1. Blake3 repo
2. Blake3 paper

[luser]

Per the other discussions I'm generally supportive of this. Two minor notes:

1. You should be able to make ring an optional dependency with this change since only the gcs feature will rely on it. That would fix some of the issues that motivated other people to look into replacing ring (as mentioned in Use RustCrypto crates #310).
2. For completeness' sake you probably ought to bump the instances of CACHE_VERSION to make it very clear that the keys are incompatible, although given that the digest length will change maybe this is overkill:
   

   sccache/src/compiler/c.rs

   Line 611 in 32e40bb

   pub const CACHE_VERSION: &[u8] = b"7";

   
   

   sccache/src/compiler/rust.rs

   Line 190 in 32e40bb

   const CACHE_VERSION: &[u8] = b"4";

[GeorgeHahn]

Done & done, thanks for the comments.

[froydnj]

An actual speedup from this switch sounds fantastic!

Just to play devil's advocate for a bit: my understanding of why SHA-512 was used is that we wanted something with actual collision resistance, and so we used a cryptographically secure hash instead of something like MurmurHash, xxhash, etc. etc.

But now we're going to replace SHA-512 with a hash that was published ~months ago and hasn't been subjected to analysis outside of the designers (AFAICT; I see the paper does include some results for BLAKE2 and why the designers felt comfortable with differing parameters for BLAKE3 based on the BLAKE2 results, but this is not quite the same thing as analysis of BLAKE3 itself)? So we're replacing a known-good hash function with one that hasn't been subjected to nearly as much scrutiny?

I don't think this means that we'd reject this pull request, but I'd like us to be clear on the tradeoffs we're making.

[jrmuizel]

For my curiosity, what's the threat model for a collision attack? A shared untrusted object cache? Do we include a hash of the entire toolchain in our hashes?

[luser]

Do we include a hash of the entire toolchain in our hashes?

For C compiles we only include a hash of the compiler binary but for Rust compiles we include a hash of all the shared libraries under $SYSROOT/lib (since the rustc binary is a shim in the rustup case, and a tiny wrapper around the implementation in the shared libraries in any case).

[GeorgeHahn]

@froydnj That's a great outline of the hash change, thanks.

BLAKE3 aims to be secure against collision attacks and should offer excellent collision resistance for cached objects. However, there are currently no third-party assurances that BLAKE3's collision resistance will hold up to malicious actors.

Is a 'shared untrusted cache' scenario is within the planned threat model? If so, it may be desirable to wait for a cryptanalysis of BLAKE3.

[luser]

Is a 'shared untrusted cache' scenario is within the planned threat model? If so, it may be desirable to wait for a cryptanalysis of BLAKE3.

I really don't think so. sccache fully trusts the cache contents and does no verification of them. A malicious actor with write access to the cache can already easily poison the cache using known hashes without needing to find a hash collision.

[drahnr]

is there any work item that needs to be done for this to be merged?

[chmanchester]

For the reasons mentioned in the thread I don't think any decreased collision resistance has security implications for us. I'll leave the final sign off on this to froydnj, though.

[froydnj]

Thanks!