Conversation
Allow truncating message digests (see 5.1 of [Recommendation for Applications Using Approved Hash Algorithms](https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-107r1.pdf)) within sensible limits. Closes #328
achingbrain
changed the title
SgtPooki
left a comment
SgtPooki
left a comment
There was a problem hiding this comment.
do we not want to go with a whitelist approach like is done in
- https://github.com/ipfs/boxo/blob/6a7b42f34e39635e860903f975468e841d70eb81/verifcid/allowlist.go#L46
- https://github.com/ipfs/boxo/blob/6a7b42f34e39635e860903f975468e841d70eb81/verifcid/cid.go#L25-L27
I'm not the expert here but from what I understand, allowing a minimum digest length of 20 bytes on sha512 would result in a hash that is not as collision resistant (160 bit length, 80 bits of collision resistance)?
We have something similar to this already in that Helia only ships with support for sha2-256, sha2-512 and identity hashes. It's up to the user to configure other hashes.
That is correct but if we enforce something different we should change it in Kubo/Boxo too. |
| return new Hasher(name, code, encode, minDigestLength, maxDigestLength) | ||
| } | ||
|
|
||
| export interface DigestOptions { |
There was a problem hiding this comment.
I'd add the unit type in the JS doc so it's clear truncate is in bytes.
There was a problem hiding this comment.
I've added a doc comment, let me know if you think it's clear.
|
What's the upstream workflow this solves that this needs to be done internally? |
|
To be clear - I'm not blocking this, happy for you to land this, just interested in the workflow and what the upstream demands are that are triggering this. |
People truncate hashes to make them less unwieldy but where the smaller hash still results in some sort of acceptable level of collision protection, at least with sha2-512, maybe others. Kubo has a whitelist of allowable hashes and a hardcoded limit of 20-128 bytes for hash size, regardless of the hash used so we need some sort of equivalent functionality. It's probably fair to say that the acceptable lower band for hash length is algorithm-dependant, so this PR is an attempt to allow individual hashes to define what is acceptable and what isn't, rather than having a blanket 20 byte limit.
Pretty much, but with this PR we can offer users a slightly better time in that we can say "this specific algorithm doesn't function under N bytes", without the calling code needing to know which algorithms have which limits - the user can just configured the algorithms they want and get on with things. |
| * | ||
| * @default 20 | ||
| */ | ||
| minDigestLength?: number |
There was a problem hiding this comment.
Without any other customization will this break small identity multihashes?
There was a problem hiding this comment.
shouldn't do, unless you identity.digest(fromString('test'), { truncate: 3 }), this limit doesn't get touched unless you ask to use the new truncate feature so this change should be entirely backward compatible and non-breaking
There was a problem hiding this comment.
Turns out the types wouldn't allow passing options to identity.digest as it doesn't implement the same interface as things returned from from (see comment) - I've fixed this up here, still non-breaking.
|
🎉 This PR is included in version 13.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
Allow truncating message digests (see 5.1 of Recommendation for Applications Using Approved Hash Algorithms) within sensible limits (as defined by https://github.com/ipfs/boxo/blob/main/verifcid/cid.go#L17-L20).
Closes #328