[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test

laanwj commented at 11:35 am on April 30, 2024: member

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:

Enumerates local publicly routable IPv6 addresses
Gets the default gateway to get the PCP endpoint
Requests pinholes for 100 seconds to port 1234 on all addreses, and prints the result.

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]

laanwj added the label Linux/Unix on Apr 30, 2024

laanwj added the label P2P on Apr 30, 2024

DrahtBot commented at 11:35 am on April 30, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	fanquake
Concept ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30040 (util, refactor: Switch to value-initialization by hebasto)

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.

DrahtBot added the label CI failed on Apr 30, 2024

laanwj force-pushed on Apr 30, 2024

laanwj requested review from Sjors on Apr 30, 2024

Sjors commented at 12:18 pm on April 30, 2024: member

Tried with a router running OPNsense:

 02024-04-30T12:16:13Z [net:debug] pcp6: gateway: ...
 12024-04-30T12:16:13Z [net:warning] pcp6: Opening pinhole for addr: ...
 22024-04-30T12:16:14Z [net:debug] pcp6: Timeout
 32024-04-30T12:16:14Z [net:debug] pcp6: Retrying (1)
 42024-04-30T12:16:15Z [net:debug] pcp6: Timeout
 52024-04-30T12:16:15Z [net:debug] pcp6: Retrying (2)
 62024-04-30T12:16:16Z [net:debug] pcp6: Timeout
 72024-04-30T12:16:16Z [net:debug] pcp6: Giving up after 3 tries
 82024-04-30T12:16:16Z [net:warning] pcp6: Opening pinhole for addr: ...
 92024-04-30T12:16:17Z [net:debug] pcp6: Timeout
102024-04-30T12:16:17Z [net:debug] pcp6: Retrying (1)
112024-04-30T12:16:18Z [net:debug] pcp6: Timeout
122024-04-30T12:16:18Z [net:debug] pcp6: Retrying (2)
132024-04-30T12:16:19Z [net:debug] pcp6: Timeout
142024-04-30T12:16:19Z [net:debug] pcp6: Giving up after 3 tries
152024-04-30T12:16:19Z [net:warning] pcp6: Opening pinhole for addr: ...
162024-04-30T12:16:20Z [net:debug] pcp6: Timeout
172024-04-30T12:16:20Z [net:debug] pcp6: Retrying (1)
182024-04-30T12:16:21Z [net:debug] pcp6: Timeout
192024-04-30T12:16:21Z [net:debug] pcp6: Retrying (2)
202024-04-30T12:16:22Z [net:debug] pcp6: Timeout
212024-04-30T12:16:22Z [net:debug] pcp6: Giving up after 3 tries

I probably have to actively turn on some permission.

laanwj commented at 12:23 pm on April 30, 2024: member

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 😄 ).

Sjors commented at 1:10 pm on April 30, 2024: member

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.

laanwj commented at 7:02 pm on April 30, 2024: member

There is a UPnP plugin, but that’s not what you’re using (and probably best left uninstalled).

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.

Sjors commented at 8:53 am on May 2, 2024: member

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 sudo ufw allow 1234 though for the nc connection to succeed (ufw is disabled by default on new systems).

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).

in src/Makefile.am:805 in fb499f555e outdated

800@@ -801,6 +801,15 @@ bitcoind_CXXFLAGS = $(bitcoin_bin_cxxflags)
801 bitcoind_LDFLAGS = $(bitcoin_bin_ldflags)
802 bitcoind_LDADD = $(LIBBITCOIN_NODE) $(bitcoin_bin_ldadd)
803 
804+#################################################################
805+bin_PROGRAMS += ipv6-pinhole-test

Sjors commented at 9:08 am on May 2, 2024:

You could make this a permanent debugging utility by adding it to bitcoin-util.

laanwj commented at 1:20 pm on May 2, 2024:

It might be useful for troubleshooting, though i’m not sure how happy people will be to add networking stuff to bitcoin-util. Currently it’s pure-function stuff only 😄

fanquake commented at 1:49 pm on May 2, 2024:

You could make this a permanent debugging utility by adding it to bitcoin-util.

Concept NACK.

Sjors commented at 3:20 pm on May 2, 2024:

No need to add networking indeed. As long as the eventual implementation (library or otherwise) has proper debug logging, we probably won’t need a standalone tool for that.

laanwj commented at 4:40 pm on May 2, 2024:

Right. Let’s not do this. Such a stand-alone tool, if it is useful, doesn’t need to be part of bitcoin.

in src/ipv6-pinhole-test.cpp:117 in fb499f555e outdated

112+    }
113+
114+    sockaddr_in6 dest_addr = gateway;
115+    dest_addr.sin6_port = htons(PCP_SERVER_PORT);
116+    if (sock.Connect((struct sockaddr*)&dest_addr, sizeof(sockaddr_in6)) != 0) {
117+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp6: Could not bind to connect to gateway: %s\n", NetworkErrorString(WSAGetLastError()));

Sjors commented at 10:35 am on May 2, 2024:

Did you mean pcp6: Could not connect to gateway?

in src/ipv6-pinhole-test.cpp:97 in fb499f555e outdated

92+bool OpenPinhole(const sockaddr_in6 &gateway, const struct in6_addr &my_addr, uint16_t port, uint32_t lifetime, int num_tries = 3)
93+{
94+    LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp6: Opening pinhole for addr: %s\n", CNetAddr(my_addr).ToStringAddr());
95+
96+    // Create IPv6 UDP socket.
97+    SOCKET sock_fd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);

Sjors commented at 10:39 am on May 2, 2024:

See also the mockackable sockets introduced by @vasild in #26812, which - if this goes beyond just PoC - can be used to test how we handle the various failure modes.

laanwj commented at 1:17 pm on May 2, 2024:

Sock is used below, which is the mockable socket. The only exception is construction, so yea for testing we’d want a function where the Sock is passed in.

in src/ipv6-pinhole-test.cpp:36 in fb499f555e outdated

31+//! Version byte. 0 is NAT-PMP, 1 is forbidden, 2 for PCP RFC-6887.
32+constexpr uint8_t PCP_VERSION = 2;
33+//! Map request opcode. See sections 19.2, 7.1.
34+constexpr uint8_t PCP_OP_MAP_REQUEST = 0x01;
35+//! Map response opcode. See sections 19.2, 7.2.
36+constexpr uint8_t PCP_OP_MAP_RESPONSE = 0x01 | 0x80;

Sjors commented at 12:09 pm on May 2, 2024:

To make it easier to understand that 0x80 refers to R, which happens to be the most significant bit of second byte of the message, and 0x01 is the actual Opcode:

0// PCP Request Header. See section 7.1
1constexpr uint8_t PCP_REQUEST = 0x00; // R = 0
2// PCP Response Header. See section 7.2
3constexpr uint8_t PCP_RESPONSE = 0x80; // R = 1
4//! Map opcode. See section 19.2
5constexpr uint8_t PCP_OP_MAP = 0x01;
6//! Map request opcode.
7constexpr uint8_t PCP_OP_MAP_REQUEST = PCP_REQUEST | PCP_OP_MAP;
8//! Map response opcode.
9constexpr uint8_t PCP_OP_MAP_RESPONSE = PCP_RESPONSE | PCP_OP_MAP;

in src/ipv6-pinhole-test.cpp:221 in 4395a42c5d outdated

216+    uint32_t lifetime_ret = ReadBE32(response + 4);
217+    uint16_t external_port = ReadBE16(response + PCP_RESPONSE_HDR_SIZE + 18);
218+    struct in6_addr external_addr;
219+    memcpy(&external_addr, response + PCP_RESPONSE_HDR_SIZE + 20, ADDR_IPV6_SIZE);
220+    if (result_code != PCP_RESULT_SUCCESS) {
221+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp6: Mapping failed with result code %d\n", result_code);

Sjors commented at 12:18 pm on May 2, 2024:

In a real implementation we should log the meaning of each result code, if known. Especially UNSUPP_OPTION and CANNOT_PROVIDE_EXTERNAL seems useful to know about.

laanwj commented at 1:14 pm on May 2, 2024:

Agree. There’s not that many anyway.

in src/ipv6-pinhole-test.cpp:149 in 4395a42c5d outdated

144+
145+    ofs += PCP_MAP_REQUEST_SIZE;
146+
147+    // Fill in option header. See Figure 4.
148+    // Prefer failure to a different external address mapping than we expect.
149+    request[ofs] = PCP_OPTION_PREFER_FAILURE;

Sjors commented at 12:26 pm on May 2, 2024:

We should explicitly set that we want this option to be mandatory (if we do indeed care that much):

Its most significant bit indicates if this option is mandatory (0) or optional (1) to process.

laanwj commented at 1:13 pm on May 2, 2024:

As i understand, mandatory or optional is a property of the opcode. PCP_OPTION_PREFER_FAILURE is 0x02, so It’s always mandatory.

We want the address and port that we request, if not possible, it’s better to fail fast, instead of having to deal with having to cancel unwanted mappings etc.

Sjors commented at 1:15 pm on May 2, 2024:

Maybe I misread it, but I think we’re implicitly doing:

0PCP_OPTION_MANDATORY = 0x00
1... PCP_OPTION_PREFER_FAILURE | PCP_OPTION_MANDATORY

laanwj commented at 4:32 pm on May 2, 2024:

i don’t think that’s the case. The mandatory flag is not a user choice, it’s part of the RFC definition of the option:

Figure 14 says “Option Code=2” only.
Section 19.4 says “The values 0-127 are mandatory to process, and 128-255 are optional to process.” This implies these are disjunct ranges.

In any case, we could be more lenient about this and leave out the option if we’re willing to accept different addresses (or maybe even ports) and advertise them (which is necessary for IPv4 anyway).

Sjors commented at 4:55 pm on May 2, 2024:

I was looking at 7.3:

Option Code: 8 bits. Its most significant bit indicates if this option is mandatory (0) or optional (1) to process.

But that’s implied by using these integer ranges.

Tend to agree we should drop this option, though we should pay attention to its return value. Otherwise we’d potentially gossip the wrong port (in practice this likely only happens if you have two nodes in the same home).

in src/ipv6-pinhole-test.cpp:151 in 4395a42c5d outdated

146+
147+    // Fill in option header. See Figure 4.
148+    // Prefer failure to a different external address mapping than we expect.
149+    request[ofs] = PCP_OPTION_PREFER_FAILURE;
150+
151+    ofs += PCP_OPTION_HDR_SIZE;

Sjors commented at 12:30 pm on May 2, 2024:

So this has the effect of expanding the message by 4 bytes, where we set the first byte to PCP_OPTION_PREFER_FAILURE and the rest to zero (since there’s no option data)?

laanwj commented at 1:14 pm on May 2, 2024:

Correct. The rest of the fields can be left at 0. i’ll add a comment.

in src/ipv6-pinhole-test.cpp:92 in 4395a42c5d outdated

87+    }
88+    return std::nullopt;
89+}
90+
91+//! Try to open a IPv6 pinhole port using RFC 6887 Port Control Protocol (PCP).
92+bool OpenPinhole(const sockaddr_in6 &gateway, const struct in6_addr &my_addr, uint16_t port, uint32_t lifetime, int num_tries = 3)

Sjors commented at 12:33 pm on May 2, 2024:

IIUC this might as well be called RequestPortMap, with the clarification that this boils down to a pinhole for IPv6.

in src/ipv6-pinhole-test.cpp:224 in 4395a42c5d outdated

219+    memcpy(&external_addr, response + PCP_RESPONSE_HDR_SIZE + 20, ADDR_IPV6_SIZE);
220+    if (result_code != PCP_RESULT_SUCCESS) {
221+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp6: Mapping failed with result code %d\n", result_code);
222+        return false;
223+    } else {
224+        // Mapping was successful. Just to be sure, check that the external port and address match what we expect.

Sjors commented at 12:41 pm on May 2, 2024:

For IPv4 we should print the external address regardless, since we had no expectation.

Sjors commented at 12:45 pm on May 2, 2024: member

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.

laanwj commented at 1:02 pm on May 2, 2024: member

Thanks for the review!

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?

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 -listened on, we should heed that.

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).

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.

One nice feature of PCP is that it gives us the external IP(v4) address.

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.

fanquake commented at 1:05 pm on May 2, 2024: member

Could you rebase this on master, now that we are using the latest version of miniupnpc (#29707).

Sjors commented at 1:21 pm on May 2, 2024: member

i’m kind of hestitant to touch that, given how hard it already is to find people to test these kind of things

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.

laanwj commented at 1:31 pm on May 2, 2024: member

Could you rebase this on master, now that we are using the latest version of miniupnpc (https://github.com/bitcoin/bitcoin/pull/29707).

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.

For now we could add support in the test, but stick to IPv6 in the implementation.

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.

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.

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.

fanquake commented at 1:36 pm on May 2, 2024: member

Mind that this adds an utility and is orthogonal to UPnP.

~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?).

laanwj commented at 1:47 pm on May 2, 2024: member

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”.

fanquake commented at 1:49 pm on May 2, 2024: member

I did not intend it to be a useful tool to keep around.

Fair enough, I’ll -redirect my NACK to this thread: #30005 (review).

Sjors commented at 2:02 pm on May 2, 2024: member

Sure, could. But mind that IPv4 will be mostly a different path.

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.

laanwj commented at 2:23 pm on May 2, 2024: member

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.

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).

Sjors commented at 3:19 pm on May 2, 2024: member

I’m really suprised there isn’t software that already exists, for doing the same thing, that we could just point users to?

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?

laanwj commented at 4:18 pm on May 2, 2024: member

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.

DrahtBot removed the label CI failed on May 3, 2024

crypto: Add missing WriteBE16 function d404f9b137

util: Allow overriding base for ToIntegral

Still default to 10, of course. Need this for parsing a hexadecimal integer.

bc896f6534

PoC PCP e3d4765972

laanwj force-pushed on May 4, 2024

DrahtBot added the label CI failed on May 4, 2024

laanwj renamed this:
~~[PoC, nomerge] IPv6 PCP pinhole test~~
[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test
on May 4, 2024

laanwj commented at 3:48 pm on May 4, 2024: member

@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.

laanwj commented at 6:35 pm on May 5, 2024: member

Continued in #30043.

laanwj closed this on May 5, 2024

[PoC, nomerge] PCP IPv4 portmap+IPv6 pinhole test #30005

Code Coverage

Reviews

Conflicts