[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test #30005
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
a263748 to
39593f8
Compare
|
Tried with a router running OPNsense: I probably have to actively turn on some permission. |
|
It may be some router setting to enable it, the more secure routers have these kind of protocols disabled by default. Or maybe it doesn't support it. There isn't any reply, not even 2 NOT_AUTHORIZED (but nice to see the retry code is working 😄 ). |
|
Unfortunately I'm not able to find any documentation for this. There is a UPnP plugin, but that's not what you're using (and probably best left uninstalled). Anyway, people who run OPNsense will now how to manually forward a port, so that's not really the target demographic here. |
Yea, according to this list, UPnP and PCP is the same plugin. It could be that they can be turned on and off seperately in the plugin configuration (it's similar for OpenWRT). The most well-known open source implementation of a PCP server is... ironically, miniupnpd. |
|
Ah, that's confusing. The OPNsense is indeed just MiniUPnPd and described as "Universal Plug and Play (UPnP IGD & PCP/NAT-PMP) Service", in other words: all the things :-) It lets you choose what to enable, in this case I selected "PCP/NAT-PMP Port Mapping". Now your utility is happy. On Ubuntu I additionally had to do I tested that the machine was indeed reachable from the outside world (and that it no longer is after 120 seconds, existing connections are preserved). |
There was a problem hiding this comment.
Concept ACK. RFC 6887 is quite readable and simple for a client to support.
IPv6 always returns multiple addresses. I'm not sure what the logic behind that is, but should we gossiping and pinholing all of them? Or is one "supposed" to be used?
Why not also use PCP for IPv4 NAT? If it's widely supported enough, maybe it lets us drop both upnp and natpmp from the dependencies (immediately or at some point in the future if there's actually a problem with them).
This appears to be trivial:
When the address field holds an IPv4 address, an IPv4-mapped IPv6 address RFC4291 is used (::ffff:0:0/96).
One nice feature of PCP is that it gives us the external IP(v4) address. We could use that in lieu of / in addition to asking our peers for it. But note that section 11.6 recommends against doing this without also requesting a port forward / pinhole.
|
Thanks for the review!
That's a good question. Sometimes routers will give out temporary addresses as well as permanent addresses, i'm not sure it's possible to discover the intent of an address at application level. If so it is going to be OS specific. Currently in master the application Discover()s alll publicly routable IPv6 addresses, which seems like a sensible default. More advanced networking users will hand-configure things anyway. And if specific addresses are being
i agree with this, but it's not as urgent. We have good coverage for IPv4 port mapping methods, i'm kind of hestitant to touch that, given how hard it already is to find people to test these kind of things 😅 So called "dual stack lite" are getting quite popular for ISPs, which essentially means no publicly routable IPv4 ("carrier-grade NAT") only IPv6. So for home users this is a better focus.
AFAIK, UPnP and natpmp also give you this information. In regard to IPv4, natpmp and pcp are essentially the same with slightly different packet layout. |
|
Could you rebase this on master, now that we are using the latest version of miniupnpc (#29707). |
For now we could add support in the test, but stick to IPv6 in the implementation. OTOH both UPnP and NAP-PMP are off by default. Adding another off-by-default checkbox "IPv6 pinhole" to the GUI seems pretty confusing, and probably doesn't increase the chance of users trying this. |
Yes, but first want to address @Sjors's comments. Mind that this adds an utility and is orthogonal to UPnP. Adding IPv6 pinholing through UPnP would be useful too (for routers that support that, and not PCP), so we'll likely want to be able to do both in the eventual design like we do for natpmp/UPnP for IPv4.
Sure, could. But mind that IPv4 will be mostly a different path. Default gateway discovery is different (mind that i still have to implement that for Windows and MacOS), port mapping semantics are slightly different from pinholing, the code will have to handle IPv4 addresses. This added complexity makes it harder to review, too.
i would like it enabled by default, i am not sure people have that much confidence in my code lol. But it's a discussion for the integration PR, we'll probably aim to merge it with disabled-default first to move forward at all. |
~0. Right, I have misunderstood what this is/does. This could be a useful tool (for some % of users of this software), but I don't think this repository is the right place for it to live, or that it fits into the set of things we should be maintaining here (I'm really suprised there isn't software that already exists, for doing the same thing, that we could just point users to?). |
|
This is a temporary PR for people to test this code, i intend to integrate it in bitcoind just like the current IPv4 port mapping stuff. I did not intend it to be a useful tool to keep around. This is why the title says "nomerge". |
Fair enough, I'll -redirect my NACK to this thread: #30005 (comment). |
I see. I guess future UI confusion can ben prevented by calling this "PCP" with a note saying that it works with IPv6 only for now. The GUI could also hide the kitchen sink of methods under some advanced toggle. In any case that doesn't affect testing. |
OK, i see your point now. Let's replace NAT-PMP for IPv4 at the same time, so we keep the same number of options (PCP and UPnP), and lose one dependency (libnatpnp). |
If there's a library that does the same thing, and doesn't have 1 million lines of code, then it seems fine to include it. We can also make it a library under the core repo, which perhaps other projects will find useful for its simplicity. At least now I have a sense of what that library should be doing. I don't think we should ask users to install a separate piece of software. For HWI I understand the argument of not wanting to include USB support in our codebase. But this protocol is much simpler (one UDP message and one reply). Some extra maintenance work seems justified here because there's at least a plausible risk that we're running short on listening nodes, if we turn on Asmap: #16599 (comment) Also, I was under the assumption that the reason we added NAT-PMP originally was to eventually replace UPnP (see #11902). Have those concerns been reduced? Do you see PCP as a potential replacement for both NAT-PMP and UPnP? Or not sure, let's keep them all around for now? |
|
i mean, there's probably some crappy C implementation of PCP out there that we could include, but this one is written in C++, it integrates with bitcoin's networking and logging, well commented, and it's straightforward enough. Besides, any code included from a third party would have to be reviewed just as well. i've already agreed to add IPv4 support so it can replace libnatpmp. @fanquake's comment was based on a misunderstanding. |
Still default to 10, of course. Need this for parsing a hexadecimal integer.
f66e2e8 to
e3d4765
Compare
|
@Sjors It creates a IPv4 as well as IPv6 mappings now. The added complexity isn't even too bad, mostly a matter of using CNetAddr/CService/Sock abstraction that we already have. |
|
Continued in #30043. |
…ntation 5c7cacf ci: Remove natpmp build option and libnatpmp dependency (laanwj) 7e7ec98 doc: Remove mention of natpmp build options (laanwj) 061c3e3 depends: Drop natpmp and associated option from depends (laanwj) 20a18bf build: Drop libnatpmp from build system (laanwj) 7b04709 qt: Changes for built-in PCP+NAT-PMP (laanwj) 52f8ef6 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj) 97c9717 net: Add PCP and NATPMP implementation (laanwj) d72df63 net: Use GetLocalAddresses in Discover (laanwj) e020304 net: Add netif utility (laanwj) 754e425 crypto: Add missing WriteBE16 function (laanwj) Pull request description: Continues #30005. Closes #17012.. This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable. PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse. For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface. To test: ```bash bitcoind -regtest -natpmp=1 -debug=net ``` (most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs) ## TODO - [x] Default gateway discovery on Linux / FreeBSD - [x] Default gateway discovery on Windows - [x] Default gateway discovery on MacOS - [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support ## Things to consider for follow-up PRs - #30043 (comment) avoid unreachable nets (not given to -onlynet=) - #30043 (comment) could announce an addr:port where we do not listen (no -bind) - #30043 (comment) could announce the wrong port because it uses GetListenPort() - #30043 (comment) if we requested one port but another was assigned, then which one to use in the renewal? - #30043 (comment) Use `GetAdapterAddresses` to discover local addresses for Windows ACKs for top commit: Sjors: ACK 5c7cacf achow101: ACK 5c7cacf vasild: ACK 5c7cacf Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
The overarching goal here is to increase the number of connectable nodes that are not in the big datacenters.
Context: IPv6 doesn't have NAT. Computers behind a router tend to get a globally routable address. However, by default this address is usually completely firewalled for incoming connections, as a security measure. See issue #17012.
This is where "pinholing" comes in. By sending a request, a machine on the network can request a port to be opened. This is similar to requesting a port forward for IPv4 but simpler.
This PR implements the so-called PCP (Port Control Protocol) from RFC6887. This is a simple UDP based protocol with fixed-size packets, so is safe to possibly even enable by default. Much simpler than UPnP which also has commands to open a pinhole, but relies on SSDP, HTTP, and XML parsing (and miniupnpc has had serious RCEs in the past). This implementation is self-contained, no external dependency is added.
Before integrating it into Bitcoin Core i would first like to investigate whether this implementation is correct and whether routers support it. So this adds a binary,
ipv6-pinhole-test, which:i've tested this on two routers myself--Turris Omnia and Fritz!Box, where it worked. Please help testing, by just running it behind the router of your choice!
For now, this is for Linux only. Implementing it for other platforms requires implementing a way to get the default route. i'll get to this later.
Be careful with publicly posting the full output of this program-it will contain your IP address information.
[skip ci]