-
Notifications
You must be signed in to change notification settings - Fork 3.8k
miniupnpd: Update, revision, new network access control and UCI options… #24988
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
base: master
Are you sure you want to change the base?
miniupnpd: Update, revision, new network access control and UCI options… #24988
Conversation
618ed1f to
6da251a
Compare
|
@Self-Hosting-Group: Nice PR! cc: @systemcrash |
6da251a to
173ee62
Compare
cfbf68e to
cb7a02d
Compare
cc180f0 to
86f6935
Compare
upnpd UCI configuration options and defaultsupnpd UCI config options
|
A downgrade included in a patchset won't get accepted, since a downgrade may subtly reintroducing bugs for existing users, if we assume that point releases fix bugs only. Better to wait for a new release, and bump to that version. Migrations are probably a more serious matter: those must be carried basically 'forever'. The best way is simply to avoid those. One might introduce a new setting, and deprecate the old one, and change the UI over to use the new one. Still a bit of a bumpy road. I think personally this is minor in the grand scheme of things (rather unimportant settings), but other reviewers may take a much firmer stance on it since you are, after all, changing setting names. |
Acceptable. It just breaks compile at the next release bump when it no longer applies. Minor, I guess. |
86f6935 to
124dd12
Compare
|
Every single test-build failed: Dirty patches detected, please refresh and review the diff |
6eaafdb to
50eda40
Compare
in openwrt/packages#24988 Signed-off-by: Self-Hosting-Group <[email protected]>
in openwrt/packages#24988 Signed-off-by: Self-Hosting-Group <[email protected]>
Inspired/address copilot's PR review for a clearer config by rename UCI section name `config` (v1.0) -> `settings` (v2.0), helps on migration and to distinguish the updated config from the previous one easily (to merge with prior) Signed-off-by: Self-Hosting-Group <[email protected]>
Alternative option to STUN allow-filtered. As requested by AquanJSW, to test with Tailscale. Also adds the required daemon fix. No public IPv4 address detection; issues with multiple clients, e.g. PCP/NAT-PMP (proposed for inclusion, to merge with prior) Signed-off-by: Self-Hosting-Group <[email protected]>
- Note that the custom ACL is now rejected by default, if it is alone used, with no preset or listed accepted ports. Add (ignored) custom ACL template entries on migration - Migrate custom ACL entries to the new section name `acl_entry` - The following custom ACL UCI options been added or changed, and the previous options are migrated on updating: acl_entry UCI options | Change | Previous name ----------------------------|----------------------------|-------------- action | New/updated values (1) | int_port | Remove colon separator (2) | int_ports ext_port | Remove colon separator (2) | ext_ports descr_filter | New option (3) | 1. Allow ignore, and update action option to use the nftables terms (allow/deny -> accept/reject). To avoid adding inverted actions when changing via LuCI, ensure any missing are set, as LuCI and UCI had not matching action defaults. Missing actions are now ignored/logged 2. Ensure that the hyphen (-) is only used as a port range separator by migration, as the colon (:) is not valid in LuCI 3. Add missing UCI option to set a regular expression to check for a UPnP IGD IPv4 port map description, and fix the current collision with the comment field which was not noticed due to a daemon bug https://redirect.github.com/openwrt/packages/pull/24495 https://redirect.github.com/miniupnp/miniupnp/pull/853 Code refactoring: - Add a more universal usable `is_port_or_range` function instead of `upnpd_get_port_range` and check if it has a valid range, and removes a shellcheck warning - Rename `conf_rule_add` function to `upnpd_add_custom_acl_entry` (to merge with prior) Signed-off-by: Self-Hosting-Group <[email protected]>
- Remove `config_foreach upnpd "upnpd"` and replace it with regular function call, as init was not designed for a multi-instance setup, as the same `tmpconf` will be used/overwritten, and non-anonymous section - Move code to make the custom vs. config file generation decision earlier, and only perform external interface detection with the second one, and rename function `upnpd` to `upnpd_generate_config` - Replace unnecessary `if` cases with `elif` in init/hotplug - Exit with 1 on errors to get an inactive service status - Do not restart daemon in hotplug when using a custom config file, as then this file will not be regenerated on restarts - Use `procd_add_reload_trigger "firewall"` instead of listening `/etc/config/firewall` (to merge with prior) Signed-off-by: Self-Hosting-Group <[email protected]>
- Arrange `start_service` and main init functions first - Format `firewall3.include` using shfmt (to merge with prior) Signed-off-by: Self-Hosting-Group <[email protected]>
3c08971 to
314a104
Compare
Wow, nice! But first, we would need to look at the corresponding LuCI PR, and there is also a problem with the PR description to be updated. Can you take a look at it? |
|
3c08971 to
f6f1b8f
Compare
|
@Self-Hosting-Group team: Maybe good to do the missing steps before ;) |
3c08971 to
7991e74
Compare
Unfortunately it's locked and I cannot make any comments. |
Thank you very much for checking. Perhaps someone else with the necessary permissions could correct this. |
7991e74 to
8ecbf4b
Compare
|
I guess this should be fine to go |
|
@Self-Hosting-Group are you going to squash the commits marked with |
|
I've tried this and it resolves a problem with old mappings hanging around. |
ldir-EDB0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Hello @1715173329 @GeorgeSapkin. I'll come back to the commit squashing (already two less than last review), and a few other points. |
|
PR description updated, dependency clearer described, and set this PR as a draft for the moment so that it is not merged unexpectedly without the depending PR. |
8ecbf4b to
2305e47
Compare
Commits
As this PR is extensive, the descriptions of the individual commits are collapsed here:
1. Update to 2.3.9 to fix issues, refresh building
upnp_forwardand return the correct client port. This also resulted in the excessive opening of new portsProject main download mirror
miniupnp.free.frwas down for 20 days miniupnp/miniupnp#770https://stats.uptimerobot.com/DwGDxUB914
--disable-pppconnto remove the old/IGDv1-only extra WANPPPConnection SSDP announcements workaround not included in other implementations since >15y--vendorcfgto allow customisation of the router/friendly name (+5 potential options) displayed in Windows Explorer, 384 bytes extra required on ARMv7 (binary)clean_ruleset_interval/thresholdUCI config options as not standard/working since OpenWrt 22.03, as nftables not supportedFix: openwrt/openwrt#18011
Fix: openwrt/luci#7759
2. Patch for UPnP IGDv2 Microsoft/Apple compat
(to merge with prior)
Link: https://github.com/Self-Hosting-Group/miniupnp/tree/upnp-igdv2-compat
3. Patch to fix description filter option
To fix the non-working description regex filter option
(to merge with prior)
Link: miniupnp/miniupnp#853
4. Package revision and new/updated UCI options
The following settings UCI options been added or changed, and the previous options are migrated on updating:
Notes:
lease_file6=${lease_file}-ipv6so that active IPv6 port maps are not lost when service restarts, e.g. by deleting an active port map. Use /run path, symlinked and appeared in FHS 3.0 in 2015 and remove option if UCI default is setPresets: accept-high-ports/accept-high-ports+web[+dns]/accept-all-ports or 0
Code refactoring:
upnpd_write_bool/etc/config/upnpdUCI config fileDepends on: openwrt/luci#7822
Fix: #17413
5. Group/rearrange config-gen and refactoring
xml_encode(to merge with prior)
6. Rename UCI section name to `settings` (v2.0)
Inspired/address copilot's PR review for a clearer config by rename UCI section name
config(v1.0) ->settings(v2.0), helps on migration and to distinguish the updated config from the previous one easily(to merge with prior)
7. Add second CGNAT UCI option
Alternative option to STUN allow-filtered. As requested by AquanJSW, to test with Tailscale. Also adds the required daemon fix. No public IPv4 address detection; issues with multiple clients, e.g. PCP/NAT-PMP
(proposed for inclusion, to merge with prior)
8. Update custom ACL options, migrate section
acl_entryminiupnpd: Update to 2.3.7 and enable regex filter #24495
miniupnpd: Rewrite permission line parser miniupnp/miniupnp#853
Code refactoring:
is_port_or_rangefunction instead ofupnpd_get_port_rangeand check if it has a valid range, and removes a shellcheck warningconf_rule_addfunction toupnpd_add_custom_acl_entry(to merge with prior)
9. Separate service start and config-gen
config_foreach upnpd "upnpd"and replace it with regular function call, as init was not designed for a multi-instance setup, as the sametmpconfwill be used/overwritten, and non-anonymous sectionupnpdtoupnpd_generate_configifcases withelifin init/hotplugprocd_add_reload_trigger "firewall"instead of listening/etc/config/firewall(to merge with prior)
10. Rearrange init and format `firewall3.include`
start_serviceand main init functions firstfirewall3.includeusing shfmt(to merge with prior)
(The italic commits are intended to be merged with the prior ones after review.)
Screenshots
The new network-wide access control functionality… can best be described using the LuCI screenshots:
Enabled Networks / Access Control (new)
Edit network access control settings (new)
Advanced Settings tab with new CGNAT functionality
UPnP IGD Adjustments tab (new)
LuCI notification if the related package is not updated (new)
Full LuCI screenshot
Depends on LuCI PR: openwrt/luci#7822
The first commit here has no dependencies and is intended for early cherry-picking.
Tested on: OpenWrt 24.10.4 and current snapshot
The Port Control Protocol (PCP) is the successor to NAT-PMP, shares similar protocol concepts and packet formats, but supports IPv6 port mapping and options/extensions. For more information, see:
Port Mapping Protocols Overview and Comparison 2025: About UPnP IGD & PCP/NAT-PMP
https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview