lib/pmp: Add NAT-PMP support (ref #698)

[AudriusButkevicius]

Purpose

Add NAT-PMP support

Testing

None, someone with a NAT-PMP gateway available needs to test it.

[AudriusButkevicius]

By the way, there is one change in behaviour with the bigger refactor.

Instead of sending 0.0.0.0:(extPort), we now send (extIp):(extPort), but I think that's fine.

[calmh]

Cool. I have a decommissioned airport express somewhere I'll dig out to take this for a spin

[AudriusButkevicius]

Updated my comment above due to markdown errors

[calmh]

[MRIW7] 2016/04/12 21:35:37.861151 service.go:107: INFO: Detected 1 NAT device
[MRIW7] 2016/04/12 21:35:37.861272 service.go:236: DEBUG: Acquiring TCP 0.0.0.0:22002 mapping on NAT-PNP@172.16.32.1
[MRIW7] 2016/04/12 21:35:37.867285 service.go:244: DEBUG: Acquired TCP 0.0.0.0:22002 -> 213.114.37.239:14 mapping on NAT-PNP@172.16.32.1
[MRIW7] 2016/04/12 21:35:37.867319 structs.go:32: INFO: New NAT port mapping: external TCP address 213.114.37.239:14 to local address 0.0.0.0:22002.

\o/

There's a couple of PNP/PMP spellos tho - fix those, and good to go I think!

[calmh]

Eh, wait. 14 is an odd choice of port to request. Not necessarily broken as such, but I think we should probably stick to 1024-65535...

[calmh]

Hm. Looks like we in fact did ask for a good port number. I wonder if this is a bug in miniupnpd's NAT-PMP stack. Packet trace says we are asking for the right thing, and it responds with gobbledygook.

Frame 3: 60 bytes on wire (480 bits), 60 bytes captured (480 bits) on interface 0
...
NAT Port Mapping Protocol, Map TCP Request
    Version: 0
    Opcode: Map TCP Request (2)
    Reserved: 0
    Internal Port: 22002
    Requested External Port: 28174
    Requested Port Mapping Lifetime: 60

Frame 4: 58 bytes on wire (464 bits), 58 bytes captured (464 bits) on interface 0
...
NAT Port Mapping Protocol, Map TCP Response
    Version: 0
    Opcode: Map TCP Response (130)
    Result Code: Success (0)
    Seconds Since Start of Epoch: 252249
    Internal Port: 242
    Mapped External Port: 14
    Port Mapping Lifetime: 60

Indeed 242 == 22002 % 256, 14 == 28174 % 256. So something is only interpreting the low order bits. And I'm not sure about the lifetime, I would expect it to be in seconds, but from our code it looks like we should correctly ask for a 60 minute lease...

[calmh]

I think the bugs are on the other side until someone proves otherwise. Another thing though:

[MRIW7] 2016/04/12 21:52:21.615059 service.go:107: INFO: Detected 2 NAT devices
[MRIW7] 2016/04/12 21:52:21.615136 service.go:236: DEBUG: Acquiring TCP 0.0.0.0:22002 mapping on NAT-PNP@172.16.32.1
[MRIW7] 2016/04/12 21:52:21.619651 service.go:244: DEBUG: Acquired TCP 0.0.0.0:22002 -> 213.114.37.239:14 mapping on NAT-PNP@172.16.32.1
[MRIW7] 2016/04/12 21:52:21.619668 structs.go:32: INFO: New NAT port mapping: external TCP address 213.114.37.239:14 to local address 0.0.0.0:22002.
[MRIW7] 2016/04/12 21:52:21.619692 service.go:236: DEBUG: Acquiring TCP 0.0.0.0:22002 mapping on c9c13192-b5dd-4bb4-827e-585ab9082cd7
[MRIW7] 2016/04/12 21:52:21.675623 service.go:244: DEBUG: Acquired TCP 0.0.0.0:22002 -> 213.114.37.239:22002 mapping on c9c13192-b5dd-4bb4-827e-585ab9082cd7
[MRIW7] 2016/04/12 21:52:21.675648 structs.go:32: INFO: New NAT port mapping: external TCP address 213.114.37.239:22002 to local address 0.0.0.0:22002.

Should we speak both UPnP and NAT-PMP with the same device? Perhaps we should stick to just one protocol per gateway IP?

[calmh]

Oh and we do apparently default to a one minute lease renewal. Is that intentional - it seems a bit excessive?

[AudriusButkevicius]

If the lease time is 0 (permanent) it uses renewal interval. We ask for a specific port, but might get something else if the router decides so. Regarding two port mappings on the same router, I don't think its a problem, it's yet another way to connect.

[calmh]

So I think the only actionable point in my tirade above is a replace PNP -> PMP in a couple of log lines

[AudriusButkevicius]

Fixed the typo

[AudriusButkevicius]

Did you set the renewal interval to 1m to get the 1 minute renewals?

[calmh]

Not intentionally, no. This was on the h1 or h2 config in the test dir, with the listen address opened up to 0.0.0.0 and then auto migrated from whatever version it is now.

[calmh]

@st-review merge. for great justice!

[st-review]

🕐 Build status is pending. I'll wait to see if this becomes successful and then merge!

[st-review]

👌 Merged as c49453c. Thanks, @AudriusButkevicius!

[AudriusButkevicius]

so I am suspicious of the fact that it was only 1 minute.

[calmh]

Yeah I'll look into it again but if anything it comes from the lib/nat commit so doesn't taint this one i think.

[AudriusButkevicius]

Audrius@Audrius syncthing $ cat test/h2/config.xml | grep upnpRenewal
        <upnpRenewalMinutes>1</upnpRenewalMinutes>

[calmh]

Well the mystery solved :)