Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add Nat PMP method to P2P module #11210

Merged
merged 6 commits into from
Dec 19, 2019
Merged

Add Nat PMP method to P2P module #11210

merged 6 commits into from
Dec 19, 2019

Conversation

NamsooCho
Copy link
Contributor

Add Nat PMP method to P2P module.
I am not sure if it is needed and correct.
I will add tests fn to this PR after someone comments that this PR is correct.

@parity-cla-bot
Copy link

It looks like @NamsooCho signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@jam10o-new jam10o-new added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 28, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Oct 28, 2019

My biggest concern here is: why do we need NAT-PMP? What problem does it solve, what is the gain? I know of no NAT related issues our users are having but maybe I'm wrong.

Another curiosity: it seems like NAT-PMP is superseded by NAT-PCP so if there is a NAT problem, why not go for the current standard? Is it because no NAT-PCP libraries exist in Rust? Or...?

@NamsooCho
Copy link
Contributor Author

NamsooCho commented Oct 29, 2019

NAT-PCP is not widely used in real-world.
https://superuser.com/questions/1301857/using-pcp-port-control-protocol-in-practice

There are some security issues in UPnP.
https://www.howtogeek.com/122487/htg-explains-is-upnp-a-security-risk/

NAP-PMP is widely used and has not been reported to have security issues.

If we support NAT PMP then users can disable UPnP.

@NamsooCho NamsooCho changed the title [WIP] Add Nat PMP method to P2P module Add Nat PMP method to P2P module Oct 29, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Oct 29, 2019

NAP-PMP is widely used and has not been reported to have security issues.

I have read about the security concerns regarding upnp (thank you for the link!) but unless I'm missing something the same concerns apply to NAT-PMP in that it also assumes the local application requesting a port mapping is trustworthy? Or are there other security concerns with upnp?

Copy link
Member

@seunlanlege seunlanlege left a comment

Choose a reason for hiding this comment

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

Needs some code style corrections, and other question answered.

util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
@NamsooCho
Copy link
Contributor Author

the same concerns apply to NAT-PMP in that it also assumes the local application requesting a port mapping is trustworthy?

Yes. Sorry for careless response.

I found this article.
https://discussions.apple.com/docs/DOC-9292

According to the article, Apple routers do not support UPnP.

util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

I think we're close, good job!

util/network-devp2p/src/ip_utils.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm requested a review from ngotchac November 13, 2019 10:19
@NamsooCho NamsooCho force-pushed the natpmp branch 2 times, most recently from 3f5121d to d420dfd Compare November 22, 2019 03:31
Comment on lines 416 to 428
match Natpmp::new() {
Ok(mut n) => {
let gw = get_public_addr(&mut n)?;
let tcp_r = get_mapped_tcp_port(&mut n)?;
let udp_r = get_mapped_udp_port(&mut n)?;

Ok(NodeEndpoint {
address: SocketAddr::V4(SocketAddrV4::new(*gw.public_address(), tcp_r.public_port())),
udp_port: udp_r.public_port()
})
},
Err(e) => Err(e)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest using let mut n = Natpmp::new()?; here, and remove the function closures (get_mapped_udp_port and such)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

A few grumbles

Comment on lines 431 to 434
return search_gateway_child.join()
.map(|node| {
node.map_err(|e| debug!("NAT PMP port mapping error: {:?}", e)).ok()
}).ok()?
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do:

return search_gateway_child.join().ok()?
			.map_err(|e| debug!("NAT PMP port mapping error: {:?}", e))
			.ok();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

match n.read_response_or_retry() {
Ok(Response::TCP(tcp)) => Ok(tcp),
Err(e) => {
debug!("Port mapping for TCP error: {}", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a target to the debug and such logs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dvdplm
Copy link
Collaborator

dvdplm commented Dec 5, 2019

@ngotchac @seunlanlege please take a look again.

@dvdplm
Copy link
Collaborator

dvdplm commented Dec 10, 2019

ping @ngotchac @seunlanlege

@ngotchac
Copy link
Contributor

None of my comments have been addressed or answered yet

Copy link
Member

@seunlanlege seunlanlege left a comment

Choose a reason for hiding this comment

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

Just going to approve this, once @ngotchac's comments have been addressed, it can be merged

@NamsooCho NamsooCho force-pushed the natpmp branch 2 times, most recently from d00ca89 to 1796ae4 Compare December 11, 2019 00:36

let mut n = Natpmp::new()?;

let gw = get_public_addr(&mut n)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could replace the function closure with a direct call as you did bellow IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

👍

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
@dvdplm dvdplm merged commit e14d68e into openethereum:master Dec 19, 2019
dvdplm added a commit that referenced this pull request Dec 19, 2019
* master:
  Add Nat PMP method to P2P module (#11210)
  Add randomness contract support to AuthorityRound. (#10946)
  ethcore/res: activate ecip-1061 on kotti and mordor (#11338)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants