Skip to content

Conversation

@Self-Hosting-Group
Copy link
Contributor

@Self-Hosting-Group Self-Hosting-Group commented Sep 18, 2024

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
  • Update daemon to 2.3.9 to fix removal of nftables rules in upnp_forward and return the correct client port. This also resulted in the excessive opening of new ports
  • Build from GitHub releases to get a reliable HTTPS server, as the HTTP-only/HTTPS mirror were only available ~85%/77% over 3 months
    Project main download mirror miniupnp.free.fr was down for 20 days miniupnp/miniupnp#770
    https://stats.uptimerobot.com/DwGDxUB914
  • Build daemon with --disable-pppconn to remove the old/IGDv1-only extra WANPPPConnection SSDP announcements workaround not included in other implementations since >15y
  • Build daemon with --vendorcfg to allow customisation of the router/friendly name (+5 potential options) displayed in Windows Explorer, 384 bytes extra required on ARMv7 (binary)
  • Remove old (iptables variant only) patches, as no longer needed
  • Remove clean_ruleset_interval/threshold UCI config options as not standard/working since OpenWrt 22.03, as nftables not supported

Fix: openwrt/openwrt#18011
Fix: openwrt/luci#7759

7	3	net/miniupnpd/Makefile
1	6	net/miniupnpd/files/miniupnpd.init
0	24	net/miniupnpd/patches/200-remove-default-cflags.patch
0	22	net/miniupnpd/patches/300-macos-compat.patch
2. Patch for UPnP IGDv2 Microsoft/Apple compat
  • Add workaround to list port maps with the Windows IGDv2-incompatible client by returning an infinite (0) lease duration. To fix listing and editing via GUI (Explorer/Network), if daemon was compiled with IGDv2
  • Extend detection to older versions of Windows and add Xbox
  • Detect Apple IGDv2-incompatible clients and apply existing workaround, that only caused problems if PCP/NAT-PMP (prioritised) was disabled

(to merge with prior)

Link: https://github.com/Self-Hosting-Group/miniupnp/tree/upnp-igdv2-compat

75	0	net/miniupnpd/patches/010-upnp-igdv2-compat.patch
3. Patch to fix description filter option

To fix the non-working description regex filter option

(to merge with prior)

Link: miniupnp/miniupnp#853

511	0	net/miniupnpd/patches/020-descr-filter-fix.patch
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:

upnpd.config UCI options Change Previous name
enabled Match default (1)
enabled_protocols=upnp-igd Combined option enable_upnp=1
enabled_protocols=pcp+nat-pmp Combined option enable_natpmp=1
allow_cgnat Allow allow-filtered (2) use_stun
stun_host Allow port inclusion (3)
stun_port Removed, included in host
allow_third_party_mapping=0 Inverted/extended to PCP secure_mode=1
log_output Allow info log level
lease_file Set by default + IPv6 (4) upnp_lease_file
upnp_igd_compat=igdv1 Renamed/match default (1) igdv1=1
download_kbps In kbit/s and renamed (5) download
upload_kbps In kbit/s and renamed (5) upload
friendly_name New option, router name
http_port Renamed, rem. if default port
notify_interval Removed if <900s, minimum
internal_iface Migrated, new section
internal_network UCI options Change Previous name
interface New option
access_preset New option (6)
accept_ports New option (6)
reject_ports New option (7)
custom_acl_before New option (6)

Notes:

  1. Init UCI default now matches LuCI and initial config file defaults for: enabled=0 and upnp_igd_compat=igdv1
  2. Allow allow-filtered for IPv4 CGNAT use and migrate option from X-Wrt and only use STUN when necessary with a private/CGNAT external IPv4
  3. Remove known incompatible STUN servers and set compatible by default
  4. Configure undocumented daemon option lease_file6=${lease_file}-ipv6 so 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 set
  5. Gets converted, config file now defaults to interface link speed instead of 8/4 Mbit/s, which is removed on migration
  6. New options added to select a preset for all devices on the network and decide if the custom ACL should be checked before the preset. Extra ports can also be set that are accepted/rejected.
    Presets: accept-high-ports/accept-high-ports+web[+dns]/accept-all-ports or 0
  7. Reject ports regardless of other settings. By default reject unsafe: 21 (FTP), 23 (Telnet), DCE/NetBIOS/SMB (135/137-139/445), RDP (3389)

Code refactoring:

  • Add a function for logging and output to stderr, and extend logging
  • Revise daemon init/config-gen slightly by declare all UCI options (incl. booleans) according to the same principle and remove upnpd_write_bool
  • Document and reformat default /etc/config/upnpd UCI config file

Depends on: openwrt/luci#7822

Fix: #17413

2	0	net/miniupnpd/Makefile
142	64	net/miniupnpd/files/miniupnpd.init
155	0	net/miniupnpd/files/upnpd-migration.uci-defaults
32	24	net/miniupnpd/files/upnpd.config
5. Group/rearrange config-gen and refactoring
  • Group and rearrange UCI option declaration and config-gen by function/LuCI UI, and comment
  • Encode required XML entities of text UPnP IGD config options until the daemon does so using the created function xml_encode
  • Only generate UPnP IGD config if the protocol is enabled

(to merge with prior)

65	60	net/miniupnpd/files/miniupnpd.init
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)

2	2	net/miniupnpd/files/firewall3.include
3	3	net/miniupnpd/files/miniupnpd.hotplug
25	25	net/miniupnpd/files/miniupnpd.init
7	0	net/miniupnpd/files/upnpd-migration.uci-defaults
1	1	net/miniupnpd/files/upnpd.config
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)

1	1	net/miniupnpd/files/miniupnpd.init
18	0	net/miniupnpd/patches/030-allow-private-fix.patch
8. Update custom ACL options, migrate section
  • 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
    miniupnpd: Update to 2.3.7 and enable regex filter #24495
    miniupnpd: Rewrite permission line parser miniupnp/miniupnp#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)

29	52	net/miniupnpd/files/miniupnpd.init
82	0	net/miniupnpd/files/upnpd-migration.uci-defaults
11	10	net/miniupnpd/files/upnpd.config
9. Separate service start and config-gen
  • 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)

13	21	net/miniupnpd/files/miniupnpd.hotplug
62	70	net/miniupnpd/files/miniupnpd.init
10. Rearrange init and format `firewall3.include`
  • Arrange start_service and main init functions first
  • Format firewall3.include using shfmt

(to merge with prior)

22	22	net/miniupnpd/files/firewall3.include
78	78	net/miniupnpd/files/miniupnpd.init

(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) luci-network-list
Edit network access control settings (new) luci-network-edit
Advanced Settings tab with new CGNAT functionality luci-advanced
UPnP IGD Adjustments tab (new) luci-igd
LuCI notification if the related package is not updated (new) luci-notification
Full LuCI screenshot 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

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 3 times, most recently from 618ed1f to 6da251a Compare October 1, 2024 08:21
@Self-Hosting-Group Self-Hosting-Group marked this pull request as ready for review October 1, 2024 09:06
@Neustradamus
Copy link

@Self-Hosting-Group: Nice PR!

cc: @systemcrash

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 3 times, most recently from cfbf68e to cb7a02d Compare December 17, 2024 07:14
@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 2 times, most recently from cc180f0 to 86f6935 Compare January 6, 2025 09:53
@Self-Hosting-Group Self-Hosting-Group changed the title miniupnpd: Revise several upnpd UCI configuration options and defaults miniupnpd: Daemon hotfix for 24.10 and revise several upnpd UCI config options Jan 6, 2025
@systemcrash
Copy link
Contributor

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.

@systemcrash
Copy link
Contributor

What do you think about backporting the commit?

Acceptable. It just breaks compile at the next release bump when it no longer applies. Minor, I guess.

@systemcrash
Copy link
Contributor

Every single test-build failed: Dirty patches detected, please refresh and review the diff

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 10 times, most recently from 6eaafdb to 50eda40 Compare January 17, 2025 13:45
Self-Hosting-Group added a commit to Self-Hosting-Group/luci that referenced this pull request Jan 17, 2025
Self-Hosting-Group added a commit to Self-Hosting-Group/luci that referenced this pull request Jan 20, 2025
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]>
@Self-Hosting-Group
Copy link
Contributor Author

@1715173329

LGTM - Thank you!

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?
openwrt/luci#7822

@Self-Hosting-Group
Copy link
Contributor Author

@hnyman

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? openwrt/luci#7822

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 6 times, most recently from 3c08971 to f6f1b8f Compare November 27, 2025 18:30
@Neustradamus
Copy link

@Self-Hosting-Group team: Maybe good to do the missing steps before ;)

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 2 times, most recently from 3c08971 to 7991e74 Compare November 27, 2025 22:03
@1715173329
Copy link
Member

1715173329 commented Nov 28, 2025

Can you take a look at it? openwrt/luci#7822

Unfortunately it's locked and I cannot make any comments.
I'm not an expert in LuCI, and I don't have time to check it recently.

@Self-Hosting-Group
Copy link
Contributor Author

Unfortunately it's locked and I cannot make any comments. I'm not an expert in LuCI, and I don't have time to check it recently.

Thank you very much for checking. Perhaps someone else with the necessary permissions could correct this.

@Self-Hosting-Group Self-Hosting-Group force-pushed the miniupnpd-uci-revision branch 3 times, most recently from 7991e74 to 8ecbf4b Compare December 2, 2025 07:30
@1715173329
Copy link
Member

I guess this should be fine to go

@GeorgeSapkin
Copy link
Member

@Self-Hosting-Group are you going to squash the commits marked with (to merge with prior)?

@ldir-EDB0
Copy link
Contributor

I've tried this and it resolves a problem with old mappings hanging around.

Copy link
Contributor

@ldir-EDB0 ldir-EDB0 left a comment

Choose a reason for hiding this comment

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

LGTM

@Self-Hosting-Group
Copy link
Contributor Author

Hello @1715173329 @GeorgeSapkin. I'll come back to the commit squashing (already two less than last review), and a few other points.

@Self-Hosting-Group Self-Hosting-Group marked this pull request as draft December 8, 2025 07:01
@Self-Hosting-Group
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

10 participants