Fix: doublehashes can use different hashes #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, we tried to parse double-hashes as CIDv0s, which implies that SHA256 was used to hash the double-hash. Instead we should try to parse a multihash directly, and support any hashing function for the double-hash. The rule14 test has been changed to use a blake3 double-hash, which previously broke at the parsing level.
hsanjuan
commented
Oct 24, 2023
denylist.go
Outdated
Comment on lines
374
to
387
| // attempt to parse a b58btc-encoded multihash | ||
| rule = strings.TrimPrefix(rule, "//") | ||
| c, err := cid.Decode(rule) | ||
| mh, err := multihash.FromB58String(rule) | ||
| if err == nil { | ||
| prefix := c.Prefix() | ||
| if prefix.Version != 0 { | ||
| return fmt.Errorf("double-hash is not a raw-multihash (cidv0) (%s:%d)", dl.Filename, number) | ||
| dmh, err := multihash.Decode(mh) | ||
| if err != nil { | ||
| return fmt.Errorf("what appears to be a multihash b58 string cannot be decoded (%s:%d): %w", dl.Filename, number, err) | ||
| } | ||
| e.Multihash = c.Hash() | ||
|
|
||
| e.Multihash = mh | ||
| // we use the multihash codec to group double-hashes | ||
| // with the same hashing function. | ||
| mhType = c.Prefix().MhType | ||
| mhType = dmh.Code | ||
| } else { // Assume a hex-encoded sha256 string |
There was a problem hiding this comment.
@lidel there is a problem here...
some of the badbits hex-encoded strings parse just fine as B58 strings, resulting in valid multihashes with weird code, which then make the whole error checking break for anything later.
I assume the other way around also is an issue: some b58 strings could perhaps be hex-decoded... ? If you have thoughts, they are welcome.
Assume a double-hash could be parsed as both a b58btc string and hex string, but handle cases when that happens and it's obviously wrong: - Multihashes of type Identity - Multihashes of unknown hash functions - Hex strings of length different than 64 chars
12 tasks
lidel
added a commit
to ipfs/kubo
that referenced
this pull request
Oct 27, 2023
this ensures we test something other than default sha256 Ref. ipfs-shipyard/nopfs#28
lidel
approved these changes
Oct 27, 2023
There was a problem hiding this comment.
Updated tests in ipfs/kubo#10161 pass with this, thanks!
I also updated example in the spec added in ipfs/specs#383
|
(merging to consolidate changes while working on fix for ipfs/kubo#10161 (comment)) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we tried to parse double-hashes as CIDv0s, which implies that SHA256 was used to hash the double-hash. Instead we should try to parse a multihash directly, and support any hashing function for the double-hash.
The rule14 test has been changed to use a blake3 double-hash, which previously broke at the parsing level.