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

[identify] Add configurable automatic push on listen addr changes. #2004

Merged
merged 3 commits into from
Mar 22, 2021

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Mar 18, 2021

This PR picks up a suggestion of @mxinden from #1999 (review). When a new or expired listen address is reported by the Swarm, it can result in an active push of an identify message to all connected peers. Two points worth noting:

  1. I've made this behaviour configurable and disabled it by default. I think that having such new features opt-in initially avoids surprises and allows selective experimentation in a more controlled manner. Also, since listen addresses are often not directly reachable due to NAT, it may be better if it is selectively enabled for those nodes who are known to be deployed with publicly reachable listen addresses. Lastly, if there are potentially a lot of connected peers, this kind of active push may cause quite a traffic burst. If it proves beneficial over time without any downsides, it can still be made the default later. However, I generally think that active pushes are more likely to be useful if they are targeted to specific subsets of peers.
  2. I have not included inject_new_external_addr as a trigger for an automatic push. I am somewhat concerned that, given that these addresses are reported by remote peers, it would make it too easy to remotely make a node spam all its connected peers with messages, possibly even with invalid data, since this would essentially provide a remote trigger for a kind of broadcast. One could of course add defensive measures via some form of throttling, but at least a naive implementation, as I've done here for inject_new_listen_addr and inject_expired_listen_addr, seems to be too easy to abuse.

@romanb romanb requested a review from mxinden March 18, 2021 13:48
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up pull request.

I've made this behaviour configurable and disabled it by default. I think that having such new features opt-in initially avoids surprises and allows selective experimentation in a more controlled manner. Also, since listen addresses are often not directly reachable due to NAT, it may be better if it is selectively enabled for those nodes who are known to be deployed with publicly reachable listen addresses. Lastly, if there are potentially a lot of connected peers, this kind of active push may cause quite a traffic burst. If it proves beneficial over time without any downsides, it can still be made the default later. However, I generally think that active pushes are more likely to be useful if they are targeted to specific subsets of peers.

Disabling by default at first and maybe enabling by default in future releases sounds good to me.

I have not included inject_new_external_addr as a trigger for an automatic push. I am somewhat concerned that, given that these addresses are reported by remote peers, it would make it too easy to remotely make a node spam all its connected peers with messages, possibly even with invalid data, since this would essentially provide a remote trigger for a kind of broadcast. One could of course add defensive measures via some form of throttling, but at least a naive implementation, as I've done here for inject_new_listen_addr and inject_expired_listen_addr, seems to be too easy to abuse.

Good point. I did not consider this attack vector in my initial proposal. While in the benign case, pushing of externally observed addresses would likely be most useful, I agree that it is not worth the attack surface. In case people still need it, I would be open to discussing some kind of throttling mechanism.

@romanb romanb merged commit c8d69ab into libp2p:master Mar 22, 2021
@romanb romanb deleted the identify-push-auto branch March 22, 2021 09:53
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.

2 participants