-
Notifications
You must be signed in to change notification settings - Fork 23
manager: Expose option to forbid dialing private IP addresses #412
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
One consequence might be that mDNS won't work with Actually, this brings us back to the discussion of Kademlia manual routing table update mode — ideally, private IPs should not be added to the local routing table (and distributed further) in the first place, and this can be done completely in client code (polkadot-sdk) like it's done with libp2p backend. Just one thing to consider is we should also ensure we don't add private addresses to the transport manager. This way we will "fail early" and not even try to dial such peers. |
This is not the case luckily — mDNS directly uses UDP sockets instead of relying on Line 129 in 8618470
|
src/transport/manager/mod.rs
Outdated
| Some(Protocol::Dns(_)) | Some(Protocol::Dns4(_)) | Some(Protocol::Dns6(_)) => | ||
| return true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak addresses like /dns4/10.10.10.10/tcp/30333/..., and also dns records can point to local IP addresses.
src/transport/manager/handle.rs
Outdated
| let has_dialable_address = address_store | ||
| .addresses | ||
| .iter() | ||
| .any(|(address, _record)| self.ip_dialing_mode.allows_address(address)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we check if the address is global two times — in TransportManagerHandle, and later in TransportManager. May be it's enough to keep the check only in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we could keep the check in TransportManager. I think I wanted to propagate the dial error as soon as possible (not entirely sure since itsbeen a while)
|
What about this PR? This issue is still present on polkadot nodes. |
|
Hey @la10736, we reprioritized the efforts towards a high-priority objective. I'll handle this in the following few weeks as this is still important 🙏 |
Signed-off-by: Alexandru Vasile <[email protected]>
…-dials Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
…-dials Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
From the testing done locally with a patched polkadot binary, this PR ensures that no dials will be made on private IP addresses. This was tested similarly to the linked issue above |
This PR exposes an option to forbid or allow dialing private IP addresses.
For test networks started locally, it is recommended to use private IP addresses to establish connections.
However, for running production validators, the validator node should never attempt to dial a private IP address.
This behavior can be detected by some cloud providers as port scanning. The downstream effect of that depends on the cloud provider, but most likely, the node will become unreachable or the instance will be terminated.
Before this PR, litep2p might receive a local (private) IP address on either the Kademlia or the Identify protocol from a libp2p node. Now, the transport manager will not forward the dialing request to the installed protocols if the private IP option is disabled.
This may be the case of paritytech/polkadot-sdk#8915, where a node operator running a validator using litep2p on the Polkadot network loses connectivity after a few days.
Closes: paritytech/polkadot-sdk#8631
Next Steps