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

Don’t join DHT when behind a NAT #778

Closed
jacobheun opened this issue Jan 24, 2020 · 8 comments
Closed

Don’t join DHT when behind a NAT #778

jacobheun opened this issue Jan 24, 2020 · 8 comments
Assignees
Labels
Epic kind/enhancement A net-new feature or improvement to an existing feature

Comments

@jacobheun
Copy link
Contributor

jacobheun commented Jan 24, 2020

Design notes

  • Use AutoNAT to not join the DHT when behind a NAT.
    • Make AutoNAT emit events on the eventbus when perceived routability changes (public => private => unknown).
    • DHT to subscribe to AutoNAT events and add/remove DHT protocol handler accordingly.
    • Evaluate routing table addition based on protocols advertised in IDENTIFY.
      • Risk/edge case: a malicious actor could advertise the DHT protocol and never actually speak it. Make sure to cover this case by evicting the peer.
    • Since the node will now attach/detach the DHT protocol handler dynamically, we need to update connected peers.
      • Introduce a new protocol: IDENTIFY DELTA, to send only protocols now enlisted or delisted.
      • Host emits an event on the eventbus when local protocols change.
      • IDENTIFY DELTA subscribes to those events and broadcasts the update to all connected peers.
    • When a connected peer changes its DHT protocols, evaluate addition to, or eviction from, the routing table.

Testing mechanics

Testing with the code from the disjoint lookups and correctly terminating queries, dial up the number of undialable nodes to 95% and make sure everything works.

Success Criteria

  • Content and peer routing work when 90% of the network is undialable.
@jacobheun jacobheun added the kind/enhancement A net-new feature or improvement to an existing feature label Jan 24, 2020
@jacobheun jacobheun added this to the Working Kademlia milestone Jan 24, 2020
@jacobheun jacobheun added the Epic label Jan 24, 2020
@jacobheun jacobheun changed the title Remove NAT’ed nodes from routing tables Don’t join DHT when behind a NAT Jan 27, 2020
@aschmahmann
Copy link
Collaborator

Updated libp2p/go-libp2p-autonat#37 to make it easy to merge when we're ready.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Feb 7, 2020

@aschmahmann @jacobheun

Evaluate routing table addition based on protocols advertised in IDENTIFY.
Risk/edge case: a malicious actor could advertise the DHT protocol and never actually speak it. Make sure to cover this case by evicting the peer.

How do we detect that the peer dosen't actually speak the DHT protocol ? Should we open a DHT Stream to it with our protocol and verify that it's successful ?

@jacobheun
Copy link
Contributor Author

How do we detect that the peer dosen't actually speak the DHT protocol ? Should we open a DHT Stream to it with our protocol and verify that it's successful ?

Closing the loop here from a conversation in chat.

From @raulk: "We can either trust them or open a stream to verify. An attacker could easily circumvent any of those measures though, so it might not be worth opening the stream, just adding them to the routing table and doing failure counting to evict them if they misbehave or mislead us"

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Feb 9, 2020

Design notes

  • Use AutoNAT to not join the DHT when behind a NAT.

    • Make AutoNAT emit events on the eventbus when perceived routability changes (public => private => unknown).

Merged in libp2p/go-libp2p-autonat#37

  • DHT to subscribe to AutoNAT events and add/remove DHT protocol handler accordingly.

Will be in stablize once libp2p/go-libp2p-kad-dht#436 goes in.

  • Evaluate routing table addition based on protocols advertised in IDENTIFY.

This is already in stabilize here.

* Risk/edge case: a malicious actor could advertise the DHT protocol and never actually speak it. Make sure to cover this case by evicting the peer.

I'd like to separate this out into it's own stream of work, possibly as a part of libp2p/go-libp2p-kad-dht#283 as we will also need some form of failure counting(count failed connectivity attempts) there & I could possibly merge the failure counting of this one with that.

  • Since the node will now attach/detach the DHT protocol handler dynamically, we need to update connected peers.

    • Introduce a new protocol: IDENTIFY DELTA, to send only protocols now enlisted or delisted.
    • Host emits an event on the eventbus when local protocols change.
    • IDENTIFY DELTA subscribes to those events and broadcasts the update to all connected peers.
  • When a connected peer changes its DHT protocols, evaluate addition to, or eviction from, the routing table.

Already in stabilize here.

@jacobheun @aschmahmann

If we are okay with doing the work for evicting misbehaving peers separately as I've mentioned in the comment, I think we should be good with this epic once libp2p/go-libp2p-kad-dht#448 goes in and start testing.

@jacobheun
Copy link
Contributor Author

jacobheun commented Feb 10, 2020

@aarshkshah1992 what about libp2p/go-libp2p-kad-dht#367? That PR is still open and is not included in your summary above. Has it been completed elsewhere?

I'd like to separate this out into it's own stream of work, possibly as a part of libp2p/go-libp2p-kad-dht#283 as we will also need some form of failure counting(count failed connectivity attempts) there & I could possibly merge the failure counting of this one with that.

I think it makes sense to separate this out and complete it in a subsequent sprint. Let's not add it to libp2p/go-libp2p-kad-dht#283 for now. Depending on the approach that work might handle this case, but I'd prefer to move this to a followup sprint (as needed), where we focus on hardening and performance. If that sounds reasonable I will split that out into its own issue and reprioritize it accordingly.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Feb 10, 2020

@aarshkshah1992 what about libp2p/go-libp2p-kad-dht#367? That PR is still open and is not included in your summary above. Has it been completed elsewhere?

Code from it has been absorbed in libp2p/go-libp2p-kad-dht#436 & libp2p/go-libp2p-kad-dht#448 combined. That PR can now be closed. I've added a comment to it.

I'd like to separate this out into it's own stream of work, possibly as a part of libp2p/go-libp2p-kad-dht#283 as we will also need some form of failure counting(count failed connectivity attempts) there & I could possibly merge the failure counting of this one with that.

I think it makes sense to separate this out and complete it in a subsequent sprint. Let's not add it to libp2p/go-libp2p-kad-dht#283 for now. Depending on the approach that work might handle this case, but I'd prefer to move this to a followup sprint (as needed), where we focus on hardening and performance. If that sounds reasonable I will split that out into its own issue and reprioritize it accordingly.

I agree. We should split it into it's own issue as it would be part of larger stream of work where we harden the DHT against misbehaving peers/Sybil attacks.

@jacobheun
Copy link
Contributor Author

I agree. We should split it into it's own issue as it would be part of larger stream of work where we harden the DHT against misbehaving peers/Sybil attacks.

👍 I will get that moved to its own / another issue today.

@jacobheun
Copy link
Contributor Author

Added libp2p/go-libp2p-kad-dht#808 to track false dht protocol advertisements, this issue can be resolved now as libp2p/go-libp2p-kad-dht#448 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants