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: bump version, drop igdv1 variant, add nftables support #17094

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

stintel
Copy link
Member

@stintel stintel commented Nov 7, 2021

Maintainer: NA
Compile/run tested: OpenWrt master r18045-71af55ae2a on qoriq

Please do not merge this before someone tested the iptables variant. I have currently only tested the nftables variant.

@stintel stintel force-pushed the miniupnpd branch 4 times, most recently from f52ed65 to 182cf31 Compare November 7, 2021 21:02
@dangowrt
Copy link
Member

dangowrt commented Nov 7, 2021

libnetfilter-conntrack3 still pulls in kmod-ipt-conntrack... Apparently libnetfilter-conntrack3 isn't even needed for the nftables variant:

root@bpi-r64:/# ldd /usr/sbin/miniupnpd
	/lib/ld-musl-aarch64.so.1 (0x7f802fe000)
	libcap-ng.so.0 => /usr/lib/libcap-ng.so.0 (0x7f802ad000)
	libnftnl.so.11 => /usr/lib/libnftnl.so.11 (0x7f8026b000)
	libmnl.so.0 => /usr/lib/libmnl.so.0 (0x7f80255000)
	libuuid.so.1 => /usr/lib/libuuid.so.1 (0x7f8023e000)
	libc.so => /lib/ld-musl-aarch64.so.1 (0x7f802fe000)
	libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x7f8021b000)

@stintel
Copy link
Member Author

stintel commented Nov 8, 2021

libnetfilter-conntrack3 still pulls in kmod-ipt-conntrack... Apparently libnetfilter-conntrack3 isn't even needed for the nftables variant:

Thanks. I've incorrectly moved back the dependencies due to miniupnpd hardcoding iptables when detecting OpenWrt. Didn't properly check the dependencies anymore after fixing that.

net/miniupnpd/Makefile Outdated Show resolved Hide resolved
@dangowrt
Copy link
Member

Tested the nftables variant and while it does seem to work and also create appropriate forwarding rules in it's tables, there is still something missing because the rules there don't have any effect.

root@OpenWrt:/# nft list tables
table inet fw4
table inet miniupnpd
table ip miniupnpd
table ip6 miniupnpd
root@OpenWrt:/# nft list table inet miniupnpd
table inet miniupnpd {
	chain forward {
		type filter hook forward priority -25; policy accept;
		iif "wwan0" th dport 1234 @nh,128,32 3232236975 @nh,72,8 6 accept
		iif "wwan0" th dport 1234 @nh,128,32 3232236975 @nh,72,8 17 accept
	}
}
root@OpenWrt:/# nft list table ip miniupnpd
table ip miniupnpd {
	chain prerouting {
		type nat hook prerouting priority dstnat; policy accept;
		iif "wwan0" tcp dport 1234 dnat to 192.168.5.175:1234
		iif "wwan0" udp dport 1234 dnat to 192.168.5.175:1234
	}

	chain postrouting {
		type nat hook postrouting priority srcnat; policy accept;
	}
}

I assume we still need a hook in fw4 to jump to the miniupnpd table. This can provide some hints maybe...

@stintel
Copy link
Member Author

stintel commented Nov 14, 2021

Tested the nftables variant and while it does seem to work and also create appropriate forwarding rules in it's tables, there is still something missing because the rules there don't have any effect.

The added rules work fine for me.

@dangowrt
Copy link
Member

The added rules work fine for me.

With fw4 running at the same time?

@stintel
Copy link
Member Author

stintel commented Nov 14, 2021

With fw4 running at the same time?

Yes. My main router is fully migrated to fw4. My Xbox (the reason for using miniupnpd) has "open NAT". How I understand it: nft works quite a bit differently than iptables; the "type X hook Y priority -25" already hooks the table/chain in the right place, with priority -25 causing these rules to be tried before all the fw4 rules.

@stintel
Copy link
Member Author

stintel commented Nov 14, 2021

What does seem to be a problem is that these miniupnpd rules get duplicated every time a device requests a upnp mapping:

table inet miniupnpd {
        chain forward {
                type filter hook forward priority -25; policy accept;
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
                iif "wan" th dport 3074 @nh,128,32 3232249379 @nh,72,8 17 accept
        }
}
table ip miniupnpd {
        chain prerouting {
                type nat hook prerouting priority dstnat; policy accept;
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
                iif "wan" udp dport 3074 dnat to 192.168.54.35:3074
        }

        chain postrouting {
                type nat hook postrouting priority srcnat; policy accept;
        }
}

@aparcar
Copy link
Member

aparcar commented Dec 29, 2021

Is this work in progress? If so please mark it as a draft.

@stintel
Copy link
Member Author

stintel commented Dec 30, 2021

I do not consider this a work in progress. Yes, the nftables variant might have some bugs, but the iptables variant should not change behaviour. I am using the nftables variant on my main router for more than a month and it is usable. I created an upstream issue, these fixes can be added later imo.

@stintel
Copy link
Member Author

stintel commented Dec 31, 2021

Fixed a missing PROVIDES for the iptables variant and did basic runtime test on my backup router, which is still running firewall3. If no objections, I will merge this soon.

Since version 2.2.3, miniupnpd will detect MS clients and force IGDv1.

This reverts commit 7f5534a.

Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
The next OpenWrt stable release aims to use firewall4 by default. As
this uses nftables as backend, miniupnpd will no longer work. Create an
iptables and nftables variant of the miniupnpd package so that miniupnpd
can be used with either firewall variant.

See openwrt#16818 for more info.

Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
@stintel
Copy link
Member Author

stintel commented Jan 5, 2022

Dropped the bump as apparently that was merged from a later PR.

@stintel stintel merged commit 59600bd into openwrt:master Jan 5, 2022
@theAeon
Copy link

theAeon commented Jan 6, 2022

Thoughts on backporting the version bump to 21.02 for the igd fix?

@Dopam-IT
Copy link

Dopam-IT commented Jan 10, 2022

is what this problem is solved ? i'm use a belkin rt3200 with upnp and firewall 4,

ok i has found a little solution i has install miniupnpd iptables thanks for all

@hnyman
Copy link
Contributor

hnyman commented Jan 25, 2022

@stintel @dangowrt @aparcar

Now that firewall4 with nftables is the default in master, should we declare the nftables variant as the DEFAULT_VARIANT for miniupnpd?

Otherwise opkg may find the iptables variant first and install it.
(The iptables variant may also be acidentally selected by dependent packages, e.g. the LuCI app.)

@aparcar
Copy link
Member

aparcar commented Jan 25, 2022

Sounds good to me.

@coudu
Copy link
Contributor

coudu commented Jan 30, 2022

Tested the nftables variant and while it does seem to work and also create appropriate forwarding rules in it's tables, there is still something missing because the rules there don't have any effect.

same for me, rules are there but have no effect...
Also the init script has some iptables commands, wouldn't the nftables variant need a different init script? or does it need iptables-nft?

@hnyman
Copy link
Contributor

hnyman commented Jan 30, 2022

The Nftables dependencies are still screwed to some extent.
See pr openwrt/openwrt#5004 and openwrt/openwrt#5007
Iptables-nft will probably be a seamless drop-in replacement in a few days/weeks, once those PRs from @champtar and @tiagogaspar8 are merged.

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.

7 participants