Skip to content
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

net/portmapper: handle multiple UPnP discovery responses #10623

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

andrew-d
Copy link
Member

Instead of taking the first UPnP response we receive and using that to create port mappings, store all received UPnP responses, sort and deduplicate them, and then try all of them to obtain an external address.

Updates #10602

Change-Id: I783ccb1834834ee2a9ecbae2b16d801f2354302f

@andrew-d
Copy link
Member Author

Reviewer note: the diff doesn't do a good job at capturing it, but there's not as much code changed as it seems. Most of tryUPnPPortmapWithDevice is just extracted from getUPnPPortMapping for clarity, and the test changes are mostly whitespace and indenting inside a loop.

@andrew-d andrew-d force-pushed the andrew/upnp-multiple-responses branch 3 times, most recently from ca49466 to 3cce092 Compare December 16, 2023 08:44
Comment on lines +508 to +510
// NOTE: this time might not technically be accurate if we created a
// permanent lease above, but we should still re-check the presence of
// the lease on a regular basis so we use it anyway.
Copy link
Member

Choose a reason for hiding this comment

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

Do we try to make a permanent lease? I think it would be better to always try to make one that expires.

Copy link
Member Author

Choose a reason for hiding this comment

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

We always try a temporary lease first, but some UPnP implementations don't support that. In those cases, we fall back to a permanent lease, though we still explicitly remove that lease when we're not using it.

@andrew-d andrew-d force-pushed the andrew/upnp-multiple-responses branch from 3cce092 to b6ecbf2 Compare December 18, 2023 19:23
@andrew-d
Copy link
Member Author

Switched away from a channel to an atomic.Bool since we didn't actually use the channel for anything other than signalling; mind taking another quick look, @jwhited and/or @sailorfrag?

@andrew-d andrew-d force-pushed the andrew/upnp-multiple-responses branch from b6ecbf2 to 3668c59 Compare December 18, 2023 20:07
Instead of taking the first UPnP response we receive and using that to
create port mappings, store all received UPnP responses, sort and
deduplicate them, and then try all of them to obtain an external
address.

Updates #10602

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I783ccb1834834ee2a9ecbae2b16d801f2354302f
@andrew-d andrew-d force-pushed the andrew/upnp-multiple-responses branch from 3668c59 to 5ec011e Compare December 18, 2023 20:28
@andrew-d
Copy link
Member Author

Testing notes: tested on a networking running an eero device as the main router, along with a network running a Linux server with MiniUPnPd 2.3.3 as the router. Both networks were still able to obtain a portmap, and on the second network, deduplicated the returned UPnP responses successfully:

portmapper: UPnP reply {Location:http://192.168.X.Y:1234/rootDesc.xml Server:Linux/6.99.99 UPnP/1.1 MiniUPnPd/2.3.3 USN:uuid:aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee::urn:schemas-upnp-org:device:InternetGatewayDevice:2}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=120\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:2\r\nUSN: uuid:aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee::urn:schemas-upnp-org:device:InternetGatewayDevice:2\r\nEXT:\r\nSERVER: Linux/6.99.99 UPnP/1.1 MiniUPnPd/2.3.3\r\nLOCATION: http://192.168.X.Y:1234/rootDesc.xml\r\nOPT: \"http://schemas.upnp.org/upnp/1/0/\"; ns=01\r\n01-NLS: 12345\r\nBOOTID.UPNP.ORG: 12345\r\nCONFIGID.UPNP.ORG: 1337\r\n\r\n"
portmapper: UPnP reply {Location:http://192.168.X.Y:1234/rootDesc.xml Server:Linux/6.99.99 UPnP/1.1 MiniUPnPd/2.3.3 USN:uuid:aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=120\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nEXT:\r\nSERVER: Linux/6.99.99 UPnP/1.1 MiniUPnPd/2.3.3\r\nLOCATION: http://192.168.X.Y:1234/rootDesc.xml\r\nOPT: \"http://schemas.upnp.org/upnp/1/0/\"; ns=01\r\n01-NLS: 12345\r\nBOOTID.UPNP.ORG: 12345\r\nCONFIGID.UPNP.ORG: 1337\r\n\r\n"
portmapper: UPnP meta changed: [{Location:http://192.168.X.Y:1234/rootDesc.xml Server:Linux/6.99.99 UPnP/1.1 MiniUPnPd/2.3.3 USN:uuid:aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee::urn:schemas-upnp-org:device:InternetGatewayDevice:2}]

@andrew-d andrew-d merged commit d05a572 into main Dec 18, 2023
46 checks passed
@andrew-d andrew-d deleted the andrew/upnp-multiple-responses branch December 18, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants