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

Deterministically de-dupe mdns connections #112

Merged
merged 3 commits into from
Jun 6, 2023
Merged

Deterministically de-dupe mdns connections #112

merged 3 commits into from
Jun 6, 2023

Conversation

tomasciccola
Copy link
Contributor

This should close #66
This is done comparing the remote and local publicKeys using b4a.compare that sorts the keys.
On this branch there's also other changes, mainly:

  • close mdns connection if not from a private ip (there's also this PR related to this, which does it differently...)
  • destroy connections instead of closing them (following hypercore implementation)
  • setting event handlers for errors as noop
  • replace throwing errors that can be handled from outside (like socket errors) with logging.
  • PeerInfo had the host and port binded to local host and port, now they are binded to remote host and port (also, don't know why we are tracking the port...) @sethvincent does this change make sense??

Tomás Ciccola added 2 commits June 6, 2023 11:49
We added a check to always close the same connection from both sides,
using `b4a.compare(publicKey, remotePublicKey)`
additionally we:
* destroy the socket instead of closing to match hypercore
implementation, and take care of handling posible error events
* close mdns connection if its not from a localAddress
* add the remote address and port to PeerInfo (instead of local address
  and port)
Basically, only allow local connections on mdns, using `private-ip`
@sethvincent
Copy link
Contributor

sethvincent commented Jun 6, 2023

@tomasciccola is this meant as an alternative to #109?

It looks like the only issue right now is package-lock.json needs to be updated by running npm install locally and pushing the updated package-lock.json.

PeerInfo had the host and port binded to local host and port, now they are binded to remote host and port (also, don't know why we are tracking the port...) @sethvincent does this change make sense??

Yeah, I think that does makes sense!

@sethvincent sethvincent merged commit b918ce1 into main Jun 6, 2023
@gmaclennan
Copy link
Member

@tomasciccola I'm concerned about merging work like this without any tests. Can we add tests in a follow-up PR? Also at a quick glance the private-ip package looks to check IPs in a very inefficient way. It may not be a huge bottleneck, but bogon should be a lot more efficient, and has a similar API

}
if (!isIpPrivate(socket.remoteAddress)) socket.destroy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomasciccola this should return here. Also there are no error handlers on socket at this point so this will crash with an unhandled error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, smth like?

    if (!isIpPrivate(socket.remoteAddress)) {
      socket.on('error', noop)
      socket.destroy()
      return
    }

@gmaclennan
Copy link
Member

the private-ip package looks to check IPs in a very inefficient way.

I ran a quick benchmark and private-ip is >100 times slower than bogon for evaluating non-private IPs. Still negligible at 0.01ms per check, but would be better to avoid having that code in our codebase and stick to bogon.

@gmaclennan
Copy link
Member

@tomasciccola I'm concerned about merging work like this without any tests.

Obviously the check on remote ips is hard to test, but it would be good to simulate some situations where we can test the dedupe code. Running coverage checks can ensure that the dedupe code paths are being tested.

@tomasciccola
Copy link
Contributor Author

You're right, I'll open an issue to add tests to the dedup code. Additionally, change private-ip to bogon

@gmaclennan gmaclennan deleted the dedupMdns branch October 26, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deterministically de-dupe mdns connections
3 participants