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

miniupnpd: Daemon hotfix for 24.10 and revise several upnpd UCI config options #24988

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Self-Hosting-Group
Copy link
Contributor

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

  • Add daemon hotfix to remove nftables rules in upnp_forward and correctly list port maps. See miniupnpd-nftables does not delete forward rules miniupnp/miniupnp#791 and miniupnpd: Port map listing broken since version 2.3.6 miniupnp/miniupnp#768. Backport commits miniupnp/miniupnp@78fbd18 and miniupnp/miniupnp@792757f
  • Fix inconsistent enabled/igdv1 UCI config options default
  • Remove clean_ruleset_interval/threshold UCI config options as not standard/working
  • Rename UCI config option enable_nat_pmp to enable_pcp_pmp as upstream, see miniupnp/miniupnp@02da705
  • Allow third-party PCP (daemon/non-UCI) config option when secure_mode UCI config option is disabled
  • Add (one-line) daemon patch to use secure_mode UCI config also for UPnP IGD with IPv6, previously it was always enabled and the behaviour is undocumented. See miniupnp/miniupnp@c79e25a
  • Convert download/upload UCI config option from KByte/s to kbit/s and rename to *_kbps, and set default to interface link speed (will only be reported)
  • New/clearer UPnP IGD compatibility mode upnp_igd_compat UCI config option accepts igdv1/igdv2, replacing the current igdv1 boolean option, allowing future compatibility modes
  • Rename and invert UCI config option secure_mode to allow_third_party_mapping
  • Better document and reformat default upnpd UCI config file and add (template) ACL entry for low ports (<1024) denied by default, current behaviour
  • Add uci-defaults script to migrate UCI config options
  • Improve configuration generation and deny ACL by default
  • Build daemon with --disable-pppconn to remove legacy workaround no longer included in other implementations since >15y
  • Update PKG_RELEASE

Maintainer:
Compile tested: OpenWrt 24.10.0-rc5
Run tested: daemon init/config generation and uci-defaults migration using OpenWrt 24.10.0-rc5

@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

as not standard/working

Signed-off-by: Self Hosting Group <[email protected]>
`secure_mode` UCI config option is disabled

Signed-off-by: Self Hosting Group <[email protected]>
also for UPnP IGD with IPv6, previously it was always enabled and the
behaviour is undocumented. See miniupnp/miniupnp@c79e25a

Signed-off-by: Self Hosting Group <[email protected]>
kbit/s and rename to `*_kbps`, and set default to interface link speed
(will only be reported)

Signed-off-by: Self Hosting Group <[email protected]>
config option accepts `igdv1`/`igdv2`, replacing the current `igdv1`
boolean option, allowing future compatibility modes

Signed-off-by: Self Hosting Group <[email protected]>
`allow_third_party_mapping`

Signed-off-by: Self Hosting Group <[email protected]>
and add (template) ACL entry for low ports (<1024) denied by default,
current behaviour

Signed-off-by: Self Hosting Group <[email protected]>
workaround no longer included in other implementations since >15y

Signed-off-by: Self Hosting Group <[email protected]>
Signed-off-by: Self Hosting Group <[email protected]>
@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
@Self-Hosting-Group
Copy link
Contributor Author

Self-Hosting-Group commented Jan 6, 2025

@systemcrash @Neustradamus: PR updated to include important daemon hotfix for OpenWrt 24.10 package. See updated PR description for details.

@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.

@Self-Hosting-Group
Copy link
Contributor Author

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.

What do you think about backporting the commit?

@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
Self-Hosting-Group added a commit to Self-Hosting-Group/luci that referenced this pull request Jan 20, 2025
@Self-Hosting-Group
Copy link
Contributor Author

@systemcrash

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

Patches refreshed, corresponding prepared LuCI changes here: openwrt/luci@master...Self-Hosting-Group:luci:adapt-new-uci.

Already at 24.10.0-rc7! I think for security reasons (nftable rule removal) and incorrect listing of port maps, IMHO the PR needs to be included in the final 24.10. And it seems that the hotfix is important for X-WRT: x-wrt/packages@220a092

A comment on the following wording would be helpful as it is suggested in OpenWrt and also in another project:

Allow third-party mapping
Allow adding port maps for non-requesting IP addresses

@Self-Hosting-Group
Copy link
Contributor Author

@systemcrash Should I already create the corresponding LuCI PR for the commit in the above branch?

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.

3 participants