-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow blake3
#8650
Comments
See this line: This is needed because of I belive the goal isn't really to protect you from everything because that a lost cause, if sha2 gets broken all non updated nodes will still follow the old thing, that not an effective thing anyway.
As you can see in the list, it's an optin list. If you lookup the last commit date, you can see that this code is older than blake3. Opinions
Optout optionI belive a |
+1 to the reasons given by @Jorropo explaining what's going on with SHA1 and why the output says that blake3 is unsafe (i.e. it was not explicitly marked as safe). Marking blake3 as safe (when it has long enough outputs) seems fine by me per the discussion in multiformats/go-multihash#121. The main thing I'd want to be careful of is making sure that blake3 works properly in relevant edge cases since unlike most other hash functions (and all the ones currently supported) it allows for variable length outputs. If some (sharness) tests are added to go-ipfs exercising the transfer of variable length blake3 hashes that would likely be enough to make it usable by go-ipfs. As an aside I'm looking forward to people exploring the possibilities of how blake3 can be utilized with IPFS. There seem like a number of pretty interesting possibilities since blake3 is itself a merkle-tree based hashing scheme |
Maybe all it needs is to change the misleading error message. Instead of asserting that a hash function is "potentially insecure" if it is not in the Adding a Regarding variable length outputs: blake3 - as per specification - has a default output size of 256 bits. The multicodec code for |
Multihash support variable sized hash functions (note, must be byte alligned, so only thing with multiple of 8 bits). |
Blake3 made decisions that offer performance over security. It's not safe enough to be trusted with data integrity checking on a level that you would expect from IPFS. I did some performance testing and hashing doesn't really seem to be the blocker either. Blake2b should be performant enough for the usual applications - and is considered safe. @RubenKelevra wrote:
|
I understand that security is a sensitive topic. In spite of reducing compression rounds from 10 to 7 BLAKE3 claims to have the same 128-bit security level as BLAKE2s. "That is, 128-bit security against (second-)preimage, collision, or differentiability attacks.". I am not a crypto-analyst and correct me if I am wrong, but SHA-1 is clearly less secure than BLAKE3. My argument is that for clarity and consistency IPFS should either add a warning when using SHA-1 or remove the warning when trying to use BLAKE3. |
I don't think the security debate is particularly usefull. We support SHA1 which has well known complete colisions, see this: shattered.io. Something that might be usefull is a feature where go-ipfs would error or warn you if you have some blocks linking to different hashes. |
Agree. Filled #8703 for this. |
Adding a pro-BLAKE3 voice to the conversation: similar to @titusz, we're looking at implementing the BLAKE3 verified streaming feature over IPFS. Would love if it was supported, that cleans our architecture up quite a bit. |
Just send a PR then 🙂. |
Not sure what needs to be done as far as bumping versions and getting this integrated- if there's anything I can do to make that easier and get this accepted, please let me know. But here's the PR, I think you can just directly import the updated version of the go-verifcid code into go-ipfs and you'll be all set. I can make a PR doing the integration into IPFS as well, but am awaiting advice re: preferences on version bumps so I don't mess up ur system :) edit: wait, don't look at that, i am really confused about these dependencies... do I need to patch go-cid? looking into it now. |
First bump the go-verify cid version (just edit the Then update go-ipfs to include it:
|
It'd also be great to have tests (Go tests or sharness) that deal with different sized blake3 hashes. Blake3 is the only variable length hash function we have other than identity which is frequently treated specially. Would be sad if you could cause me problems downloading DAGs with blake3 hashes. Reiterating #8650 (comment) |
We already have prior work that "just" assume blake3 is a fix sized 32 bytes big hash function, I think that a sane default to begin with and require less refactoring than supporting variable sizes easly. |
If you're sticking with a fixed length, then we'd want to test that behavior (i.e. that different sizes don't work) so that we'll notice if there are changes there. |
true. |
I don't think so. Those have been around for a while and you can check them out and also see why they exist in the multiformats repos. I'm more concerned with really long hashes breaking things than short ones since we have some basic protections in go-verifcid here and these have been possible for a while. |
what do you prefer me to do re: this? honestly i think the correct way to go (and this is edgy, sorry!) is CIDvn+1 where you encode a length as well. I'm adding support for the 32-bytes blake3 into multihash right now (as necessary to add support in verifCID) edit: whoopsies silly me multihashes already have that :) |
will add tests briefly as well |
right now i think go-multihash supports creating one length but will parse any length (but does error handling correctly if the lengths are inconsistent), the right place to validate whether a file "matches" a blake3 length-16 hash (it never should) is probably go-ipfs or go-cid- investigating now. |
@laudiacay I think you are touching a critical point here. Just FYI, the core problem and the thing you need to target is that That is important to remember because that will change where you need to put thoses length checks. |
another two prerequisite PRs to getting this changed in go-ipfs. need the multihash one accepted before any others can be accepted. ok. i think the right place to do this is proooobably in multihash parsing...? like... adding something saying that if the code is for BLAKE3, the length must be 32? you know this stack better than me, let me know if im being silly. |
No, the parser must support insecure hashes, we have consumers (like trie keying), using xxhash64 for example. I would add it to this function: BTW I notice we already have a minimum of 20 bytes in place. |
triage update, some things that we'd like to see. Largely stemming from multihash lengths being used differently by blake3 than the other (non-identity) multihashes. Sharness tests for blake3 (I'd just add a
Thanks @laudiacay for pushing on this 🙏 |
do these still need doing, considering that we're handling length in more or less the same way? |
Closed by #9187. Thanks @laudiacay and @Jorropo 🎉. Currently only 32 byte blake3 hashes are generatable, if demand (and PRs) for that show up then those can get tackled too. You can follow the PR trail for more information. |
Checklist
Installation method
ipfs-update or dist.ipfs.io
Version
Config
irrelevant
Description
This command gives an error message:
While using
sha1
produces a valid CIDv1sha1
is known to be broken,blake3
is not. So it would make sense to either remove the security warning forblake3
or add one forsha1
.The text was updated successfully, but these errors were encountered: