Skip to content
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

Remove DHT signatures #4613

Closed
Stebalien opened this issue Jan 26, 2018 · 10 comments
Closed

Remove DHT signatures #4613

Stebalien opened this issue Jan 26, 2018 · 10 comments
Labels
kind/bug A bug in existing code (including security flaws) topic/dht Topic dht

Comments

@Stebalien
Copy link
Member

Instead, records should (and do) provide their own signatures. We should simply validate them from the DHT record validator function.

Supersedes #4597.

@Stebalien Stebalien added kind/bug A bug in existing code (including security flaws) topic/dht Topic dht labels Jan 30, 2018
@dirkmc
Copy link
Contributor

dirkmc commented Jan 30, 2018

@Stebalien you want me to take a stab at implementing this?

@Stebalien
Copy link
Member Author

@dirkmc that would be awesome! The current thinking is to:

  1. When calling GetValue to resolve an IPNS record, make sure to fetch the associated public key first. Currently, we fetch the public key in parallel.
  2. Construct the IPNS validator with access to the peerstore.

...

And then remove everything related to signatures/authors in DHT records.

@dirkmc
Copy link
Contributor

dirkmc commented Jan 30, 2018

I guess if the DHT record validator checks the signature then the resolver doesn't need to fetch the public key at all when calling GetValue right?

Currently the DHT

Should the DHT pass a hint to the validator to indicate whether or not it should go online to get the public key?

@Stebalien
Copy link
Member Author

I guess if the DHT record validator checks the signature then the resolver doesn't need to fetch the public key at all when calling GetValue right?

The opposite. We don't want the validator fetching anything. The resolver needs to first fetch the key, put it in the peerstore, and then resolve the IPNS record. The validator can use the peerstore which won't go to the network to fetch it.

On Put, we'd have the same behavior as we currently have (as the resolver won't go online).


Eventually, we could add some form of offline/online hint (#4009) but we can cross that bridge later.

@dirkmc
Copy link
Contributor

dirkmc commented Jan 30, 2018

Ah I see what you're saying, ok makes sense

@dirkmc
Copy link
Contributor

dirkmc commented Jan 31, 2018

@Stebalien here's a first pass at the go-ipfs part:
#4628
@vyzo @whyrusleeping you might want to check this out also

@dirkmc
Copy link
Contributor

dirkmc commented Jan 31, 2018

@Stebalien PR to remove Author and Signature from go-libp2p-record: libp2p/go-libp2p-record#15
Is this what you had in mind?

@vyzo
Copy link
Contributor

vyzo commented Feb 1, 2018

Is there anything else that relies in DHT record signatures?

@Stebalien
Copy link
Member Author

@vyzo not that I know of.

We should have @diasdavid chime in here, however, I'm pretty sure JS doesn't actually require this field either. @diasdavid, is JS like go in this? Will it only check this signature if present (so if we remove it, we won't have any issues)? My cursory look through the JS DHT code indicated that this was the case.

@dirkmc
Copy link
Contributor

dirkmc commented Feb 1, 2018

Here's the go-libp2p-kad-dht PR: libp2p/go-libp2p-kad-dht#114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) topic/dht Topic dht
Projects
None yet
Development

No branches or pull requests

3 participants