search

opnsense / plugins

OPNsense plugin collection
https://opnsense.org/
BSD 2-Clause "Simplified" License
849 stars 644 forks source link

net/upnp - Allow for setting arbitrary number of UPnP permissions #4305

Closed Kreeblah closed 1 month ago

Kreeblah commented 1 month ago

I'm investigating migrating to OPNsense, but one issue I ran into was that there's a limit of 8 UPnP/NAT-PMP permissions in OPNsense, but I need more than that in order to migrate over my config.

So, this allows a user to set a number of permissions to use, and then writes them to the config. It's not ideal, as the number of permissions needs to be set, and then a save needs to happen, but it at least allows for the functionality now.

I also have it defaulting to 8 permissions in order to match current behavior, so people don't lose any existing permissions they have with the upgrade.

AdSchellevis commented 1 month ago

@Kreeblah did you test this (on OPNsense)? The manual doesn't seem to mention the parameter in question https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/miniupnpd.8

Kreeblah commented 1 month ago

I did, yes (on 24.7.6), and it's properly modifying the OPNsense config, as well as the miniupnpd.conf file. Which parameter are you referring to?

Kreeblah commented 1 month ago

Oh, I see what you mean. Yes, that shouldn't be adding the num_permuser line to the miniupnpd.conf file. I'll remove that.

AdSchellevis commented 1 month ago

My mistake, I misread it seems, the 8 seems to be rather a trivial choice from the past (https://github.com/opnsense/plugins/issues/1062)

fichtner commented 1 month ago

@Kreeblah @AdSchellevis ready for merge?

AdSchellevis commented 1 month ago

@fichtner looks good to me 👍

Kreeblah commented 1 month ago

Yep, I don't see anything else. Thank you both for the reviews!

fichtner commented 1 month ago

Merged, thanks. Hope this moves the migration along nicely. 😊