double hashing implementation #1

noot · 2022-08-30T18:17:03Z

calculate and use sha256 hash of content multihash before advertising/discovering
run golangci-lint and fix lint errors

…into noot/double-hash

…keyspace dep to fork

willscott · 2022-09-08T08:36:08Z

Cool, this is a good first chunk,

Deciding on a migration story is going to be important in figuring out what additional complexity is needed here
The stored PeerID should be encrypted by the MH in the dht record

guillaumemichel

Great progress! This looks all go to me!

Concerning the isHashed variable, I guess that the setting it to true or false will depend on the migration that we choose to go with.

Did you already run some test cases with isHashed = true in lookup.go, or any test case that 1. provides some content to the DHT using double hashing and 2. retrieving this content (from another peer)?

guillaumemichel · 2022-09-27T14:37:25Z

lookup.go

-	// TODO: I can break the interface! return []peer.ID
-	lookupRes, err := dht.runLookupWithFollowup(ctx, key,
+
+	const isHashed = false


How can the host determine whether the requested key is hashed or not?

the recipient of the query will receive Message_FIND_NODE, Message_GET_VALUE or Message_GET_PROVIDERS (see handlers.go). for the first 2, the key is not hashed, but for Message_GET_PROVIDERS the key will be hashed.

guillaumemichel · 2022-09-27T14:48:52Z

query.go

 	q := &query{
 		id:         uuid.New(),
-		key:        target,
+		key:        target, // TODO change this if hashed?


I think this doesn't need to be changed as if isHashed, target should already be the hash

guillaumemichel · 2022-09-27T14:55:06Z

routing.go

@@ -288,7 +288,8 @@ func (dht *IpfsDHT) getValues(ctx context.Context, key string, stopQuery chan st
 	go func() {
 		defer close(valCh)
 		defer close(lookupResCh)
-		lookupRes, err := dht.runLookupWithFollowup(ctx, key,
+		const isHashed = false


Why is isHashed set to false here?

we decided on hashing only the key for the AddProviders/GetProviders path, as it's what's used specifically in IPFS to find content providers. Get/Put keys is only used for IPNS as far as I know

noot · 2022-09-29T13:26:34Z

@guillaumemichel yes, the existing unit tests that use Provide and GetProviders... should cover the double hashing case, for example TestProvides and TestProvidesMany.

araskachoi · 2022-10-03T20:04:24Z

fullrt/dht.go

@@ -777,10 +777,14 @@ func (dht *FullRT) Provide(ctx context.Context, key cid.Cid, brdcst bool) (err e
 		return fmt.Errorf("invalid cid: undefined")
 	}
 	keyMH := key.Hash()
-	logger.Debugw("providing", "cid", key, "mh", internal.LoggableProviderRecordBytes(keyMH))
+	mhHash := internal.Sha256Multihash(keyMH)


do you think it might be a good idea to add a salt for this hash? this way there's another layer of protection that could be used from the provider?
but i can see the other side where if we want to make this easily reproducible, it would be better to leave out the additional step (-:

If required, we could add a constant salt (e.g to specify the purpose of the hash).

However, the secret to access the content is already the CID, adding a random salt means that knowledge of both the CID and the salt is required to find the content. If we add a random salt here, it would have to be shared along with the CID, thus it wouldn't add much security. (in other words, it would only make the secret longer)

We should consider adding a constant salt (or prefix) to the spec and here - so that the derived hashes are in a known specific domain that doesn't easily have collisions with other things.

araskachoi · 2022-10-04T01:04:11Z

lookup.go

-	// TODO: I can break the interface! return []peer.ID
-	lookupRes, err := dht.runLookupWithFollowup(ctx, key,
+
+	const isHashed = false


will we add a check to see if it's hashed later on?

yeah, it's checked in query.go line 152

actually, this got removed since the double hash is now a multihash.Multihash

willscott · 2022-09-08T08:33:58Z

routing.go

 		func(ctx context.Context, p peer.ID) ([]*peer.AddrInfo, error) {
 			// For DHT query command
 			routing.PublishQueryEvent(ctx, &routing.QueryEvent{
 				Type: routing.SendingQuery,
 				ID:   p,
 			})

-			provs, closest, err := dht.protoMessenger.GetProviders(ctx, p, key)
+			provs, closest, err := dht.protoMessenger.GetProviders(ctx, p, multihash.Multihash(mhHash[:])) // TODO: this isn't a multihash anymore, change GetProvider param type instead?


is it worth having a multihash code for these? doublehashed-sha256 or similar and encode the derived hash as a multihash?

yep, updated to use DBL_SHA2_256

willscott · 2022-09-08T08:34:52Z

internal/utils.go

+type Hash [32]byte
+
+func Sha256Multihash(mh multihash.Multihash) Hash {
+	return sha256.Sum256(mh)


This should be a derived hash, so e.g. add a custom prefix like sha256.Sum256("CR_DOUBLEHASH" + mh.Bytes())

willscott · 2022-10-04T14:54:18Z

fullrt/dht.go

@@ -777,10 +777,14 @@ func (dht *FullRT) Provide(ctx context.Context, key cid.Cid, brdcst bool) (err e
 		return fmt.Errorf("invalid cid: undefined")
 	}
 	keyMH := key.Hash()
-	logger.Debugw("providing", "cid", key, "mh", internal.LoggableProviderRecordBytes(keyMH))
+	mhHash := internal.Sha256Multihash(keyMH)


We should consider adding a constant salt (or prefix) to the spec and here - so that the derived hashes are in a known specific domain that doesn't easily have collisions with other things.

willscott · 2022-10-15T16:02:02Z

internal/utils.go

+
+func Sha256Multihash(mh multihash.Multihash) multihash.Multihash {
+	prefix := []byte("CR_DOUBLEHASH")
+	mh, err := multihash.Sum(append(prefix, mh...), multihash.DBL_SHA2_256, 30)


is there a reason we're saying this has a length of 30?

yeah, since the multihash automatically prepends the code and length of the data, the returned multihash is of length 32. If I set the length to 32 (which I initially tried) it causes the returned multihash to be of length 34, which then causes a panic in routingTable.NearestPeers when it calls CommonPrefixLen to compare the multihash with the local peer ID, since the lengths are mismatched.

the other workaround I thought off is slicing off the first 2 bytes of the multihash / only using the digest, but I'm not sure if that would have unintended consequences. any thoughts on what method would be better?

func Encode(buf []byte, code uint64) ([]byte, error) { // FUTURE: this function always causes heap allocs... but when used, this value is almost always going to be appended to another buffer (either as part of CID creation, or etc) -- should this whole function be rethought and alternatives offered? newBuf := make([]byte, varint.UvarintSize(code)+varint.UvarintSize(uint64(len(buf)))+len(buf)) n := varint.PutUvarint(newBuf, code) n += varint.PutUvarint(newBuf[n:], uint64(len(buf))) copy(newBuf[n:], buf) return newBuf, nil }

The Encode() function basically takes the digest and append it to the code, hence returning something of the format: [code, digest]. The code is represented by 2 bytes.

We don't want the digest to start with a constant, as this digest determine the location of the provider records in the DHT. Having a common prefix (code) to all digest would mean that all Double Hashed Provider Records are allocated to a restricted location of the keyspace. We don't need to have the code in the output. We could update the code as follow:

mh, err := multihash.Sum(append(prefix, mh...), multihash.DBL_SHA2_256, KEYSIZE) if err != nil { // this shouldn't ever happen panic(err) } // don't return the code prefix, only return the last KEYSIZE bytes return mh[len(mh)-KEYSIZE:]

KEYSIZE should be a constant value defining the DHT key size. Ideally this constant already exists somewhere in the DHT implementation or it is passed down by kubo. Its value should be 32 for now, but if the DHT keysize was to change this piece of code shouldn't need to be updated, updating the constant should be enough.

@guillaumemichel that works, but the issue is now the function returns a multihash.Multihash that isn't actually a multihash (since the code is gone). the purpose from what I remember of using multihash.Sum was that we could specify the hash was of type DBL_SHA2_256, whereas previously it was just using sha256 (which is effectively the same as doing multihash.Sum then choosing the last 32 bytes). should I just change the function back to do a sha256 sum? because I agree we shouldn't have the hashes all having the same first 2 bytes.

Yes, it should be fine to change the function back to sha256

guillaumemichel · 2022-10-20T10:26:44Z

fullrt/dht.go


 	// add self locally
-	dht.ProviderManager.AddProvider(ctx, keyMH, peer.AddrInfo{ID: dht.h.ID()})
+	err = dht.ProviderManager.AddProvider(ctx, keyMH, peer.AddrInfo{ID: dht.h.ID()})


Shouldn't it be mhHash instead of keyMH?

yep, updated!

guillaumemichel · 2023-01-09T11:34:26Z

@noot we can now probably get rid of this abomination in go-libp2p-kbucket. IIUC it is the result of a preimage bruteforce to find a key that would fit every single small bucket.

It isn't required anymore with double hashing as one doesn't request the preimage of a kad key, but the kad key itself. We can simply forge random keys that fit inside the bucket instead of taking the ones resulting from the brute force.

noot added 12 commits August 23, 2022 16:51

lint and start double hashing impl

cb28c01

go fmt

f2f6a0f

some test fixes

2e84c51

hash multihash in provide/lookup

d855a34

revert some get/put value changes

fc6bce6

fix tests

77e94b5

Merge branch 'master' of https://github.com/libp2p/go-libp2p-kad-dht …

7af690d

…into noot/double-hash

cleanup

c88d1b4

hash multihash before providing in fullrt

3c15441

don't hash target in runQuery if it's already been hashed, update go-…

27545ee

…keyspace dep to fork

update go.mod to use chainsafe/go-keyspace

da599fc

update NearestPeers call not to hash key if it's already hashed

c970ed2

noot marked this pull request as ready for review September 1, 2022 17:04

merge w upstream master

2ecb972

guillaumemichel approved these changes Sep 27, 2022

View reviewed changes

cleanup

ebc521e

araskachoi reviewed Oct 3, 2022

View reviewed changes

araskachoi reviewed Oct 4, 2022

View reviewed changes

willscott reviewed Oct 4, 2022

View reviewed changes

noot added 3 commits October 5, 2022 15:59

add domain prefix to derived hash

c0be529

update double hash to be multihash w code DBL_SHA2_256

8a6cbed

cleanup

96cc75d

willscott reviewed Oct 15, 2022

View reviewed changes

update Sha256Multihash to return last 32 bytes of multihash

4313318

guillaumemichel reviewed Oct 20, 2022

View reviewed changes

update fullrt Provide

8e3db8e

dokterbob mentioned this pull request Nov 10, 2022

IPIP 298: (allow|deny)lists for IPFS Nodes and Gateways ipfs/specs#340

Closed

update Sha256Multihash to use sha256 again

d0b0d88

noot added 3 commits November 17, 2022 09:30

re-add multihash usage, fix length panic

f954b6b

cleanup, likely fix find providers bug

b39aef1

more cleanup

83e8253

umgefahren mentioned this pull request Nov 25, 2022

protocols/kad: Add double hasing for privacy to libp2p-kad libp2p/rust-libp2p#3169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

double hashing implementation #1

double hashing implementation #1

noot commented Aug 30, 2022

willscott commented Sep 8, 2022

guillaumemichel left a comment

guillaumemichel Sep 27, 2022

noot Sep 29, 2022

guillaumemichel Sep 27, 2022

guillaumemichel Sep 27, 2022

noot Sep 29, 2022

noot commented Sep 29, 2022

araskachoi Oct 3, 2022

guillaumemichel Oct 4, 2022

willscott Oct 4, 2022

araskachoi Oct 4, 2022

noot Oct 5, 2022

noot Oct 5, 2022

willscott Sep 8, 2022

noot Oct 5, 2022

willscott Sep 8, 2022

noot Oct 5, 2022

willscott Oct 4, 2022

willscott Oct 15, 2022

noot Oct 18, 2022

guillaumemichel Oct 19, 2022

noot Oct 19, 2022 •

edited

Loading

guillaumemichel Oct 20, 2022

guillaumemichel Oct 20, 2022

noot Oct 21, 2022

guillaumemichel commented Jan 9, 2023

double hashing implementation #1

Are you sure you want to change the base?

double hashing implementation #1

Conversation

noot commented Aug 30, 2022

willscott commented Sep 8, 2022

guillaumemichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumemichel commented Jan 9, 2023

noot Oct 19, 2022 •

edited

Loading