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

Port UPnPUtil to use Jupnp #5124

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

fishface60
Copy link
Contributor

@fishface60 fishface60 commented Dec 27, 2024

Identify the Bug or Feature request

closes #4049 because timing out is guaranteed using CompletableFutures.
closes #4902 because jupnp filters interfaces and addresses by whether they are non-loopback IPv4.
closes #5123

Description of the Change

This replaces the implementation of UPnPUtil from upnplib to jupnp.

Considered implementations were:

  1. MapTool's existing upnplib fork. There is no wider development community around it to contribute fixes so it will be a lot of effort to keep up to date.
  2. PortMapper. I ported the implementation to this first, but it did not work with my router and in the issue tracker the developer admitted to abandoning it because there are too many routers to keep up with.
  3. upnpc's java bindings were recommended by the PortMapper developer, but the code for building them was broken, and has been broken for 3 years so appears to have no active users.
  4. jupnp, which was suggested in comments as a potential alternative.

Possible Drawbacks

jupnp is licensed CDDL. The top answer on https://opensource.stackexchange.com/questions/2094/are-cddl-and-gpl-really-incompatible argues that linking to CDDL from GPL is fine on the grounds that distributing an application's bundled libraries is a combined work and only requires that parts remain available under their original licenses.

This sounds reasonable to me, but there is not currently a dependency on any CDDL Jars.

javax.ws.rs has a few libraries that are CDDL licensed but javax.ws.rs-api is not one of them, and the only other CDDL licensed library was javax.servlet:servlet-api which is dual-licensed GPL and CDDL so is compatible as GPL.

Release Notes

  • Ported our UPnP implementation to jupnp in order to support newer Routers.

This change is Reviewable

This uses the jupnp stack to discover and map ports.
It does not use jupnp's built-in PortMappingListener due to
shortcomings:

1. We need to determine which address to request mappings to dynamically
   instead of specifying the address to map in advance
   since we can only know which address is appropriate after we
   determine which interface has an IGD on.
   We instead use the address that the IGD was discovered from.
   This may not be the most preferred address to use, but it does
   guarantee that it works with routers that reject requests to map ports
   to addresses that the request didn't come from.
2. We need some additional callbacks to be able to wait for some device
   to be discovered and some port to be mapped in order to present
   appropriate error messages.
3. My router doesn't support InternetGatewayDevice:1,
   only InternetGatewayDevice:2, so I needed to add additional checks.
   This is fine because PortMappingAdd is supported on both.

The jupnp data model isn't ideal, since it broadcasts from every IPv4
address and some routers (i.e. mine) will respond to discovery
broadcasts from every address it has, including a few IPv6 ones,
and it assumes that if it gets a response from a different address
then the UPnP device has changed address and so is a new device,
triggering device removed and new device callbacks,
which we have to request a new port mapping for because we can't tell if
it's a new device or not.

As a result, port mapping involves a number of port mapping requests
equal to the number of non-loopback IPv4 addresses you have
multiplied by the number of addresses the IGD responds from.
@fishface60 fishface60 marked this pull request as ready for review December 28, 2024 19:15
@fishface60
Copy link
Contributor Author

Ubuntu sets a precedent that including CDDL code in GPL licensed projects is acceptable if it's not in the source tree by shipping the OpenZFS kernel module alongside the kernel.
It's not uncontroversial but there doesn't seem to have been any consequences.

I can have another go at fixing the upnplib fork, but it may take me some time to get around to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug performance A performance or quality of life improvement
Projects
None yet
1 participant