libremesh / lime-packages

LibreMesh packages configuring OpenWrt for wireless mesh networking
https://libremesh.org/
GNU Affero General Public License v3.0
276 stars 95 forks source link

angw, lime-proto-bmx7: use nft includes instead of init.d scripts #1021

Closed pony1k closed 1 year ago

pony1k commented 1 year ago

Using /etc/init.d-scripts for making nftable / xtable rules can lead to problems. See this comment: https://github.com/libremesh/lime-packages/pull/1020#issuecomment-1523609130 . This PR removes the /etc/init.d/-script for anygw and places nft-rules in /etc/firewall.lime.d/. It also places the bmx7-not-over-bat0 rule there. The rules were rewritten nft-compatible, so that they can be included in the firewall config. It makes anygw and lime-proto-bmx7 compatible to firewall4 and nftables, without using ebtables-nft. lime-proto-bmx7 and lime-proto-anygw are now explicitly dependant on firewall4, not implicitly via lime-system like before. A fallback for the mtu-fix in lime-proto-bmx7 for when no firewall package is installed is no longer necessary and was removed.

pony1k commented 1 year ago

see also: https://github.com/libremesh/lime-packages/pull/1020

I tested it on an archer c7 and it seems working:

root@wtw-teilchen-mast-mitte:~# nft list table bridge nat
table bridge nat {
        chain POSTROUTING {
                type filter hook postrouting priority srcnat; policy accept;
        }

        chain postrouting_bmx7_not_over_batadv {
                type filter hook postrouting priority srcnat; policy accept;
                oifname "bat0" ether type ip6 udp sport 6270 udp dport 6270 counter packets 4286 bytes 1039377 drop
        }

        chain postrouting_anygw {
                type filter hook postrouting priority srcnat; policy accept;
                oifname "bat0" ether saddr aa:aa:aa:00:00:00/24 counter packets 840 bytes 63762 drop
                oifname "bat0" icmpv6 type nd-router-solicit counter packets 5 bytes 240 drop
                oifname "bat0" icmpv6 type nd-router-advert counter packets 0 bytes 0 drop
        }
}
root@wtw-teilchen-mast-mitte:~# nft list table bridge filter
table bridge filter {
        chain FORWARD {
                type filter hook forward priority filter; policy accept;
                ether daddr aa:aa:aa:00:00:00/24 counter packets 114096 bytes 16211610 drop
        }
}

We may want to remove the counters after we verified that everything is working as intended.

pony1k commented 1 year ago

@ilario @G10h4ck @dangowrt, I would love to hear your thoughts about this!

dangowrt commented 1 year ago

Very good work, thank you!

G10h4ck commented 1 year ago

Good work, a few questions can't we use either use /etc/nftables.d/ or /usr/share/nftables.d/ (the one which fits better) instead of /etc/firewall.lime.d/ ? In that case I think we should rename those files like lime-proto-bmx-RULE_EXPLAINATION_HERE.nft

G10h4ck commented 1 year ago

ping @pony1k @dangowrt

pony1k commented 1 year ago

If we put the scripts in /usr/share/nftables.d/ or /etc/nftables.d, that would mean that we need to delete and recreate the files instead of just setting enabled to 1 or 0. I don't like to have the nft-files contents embedded inside lua scrips, because it is less readable this way. I would be cool with having the nft-files in another folder and symlink them into /etc/nftables.d or /usr/share/nftables.d/`. I agree with your naming scheme.

I just found out that firewall4 will add duplicates of the inlcuded rules outside table inet fw4 when /etc/init.d firewall restart is called, which must be fixed. This doesn't happen with stop ; start. Don't ask me. See this discussion: https://github.com/openwrt/openwrt/issues/11620.

pony1k commented 1 year ago

Now, both /etc/init.d/firewall stop ; /etc/init.d/firewall start and /etc/init.d/firewall restart works. @G10h4ck Do you insist on putting the .nft files into a nftables.d folder? Would you be okay with symlinks?

G10h4ck commented 1 year ago

I just don't like /etc/firewall.lime.d/ it gives the idea that just by putting any firewall script there get executed, also I don't like actual code from packages ending up under /etc/ even if it is nftables code, so I think something more reasonable would be something under /usr/, and then [un]symlink in the proper directory (either /etc/nftables.d or /usr/share/nftables.d/ depending on what fits better) at configuration time.

maybe a good directory could be /usr/share/lime/ and then put meaningful names for the files inside

dangowrt commented 1 year ago

What about having the files shipped by the package in /usr/share/nftables.d/ and then simply use UCI to add/enable/disabled each to-be-included file as needed? I agree that files which are shipped by a package (unless they are a default configuration which is going to be edited by the user) should not go under /etc

pony1k commented 1 year ago

What about having the files shipped by the package in /usr/share/nftables.d/ and then simply use UCI to add/enable/disabled each to-be-included file as needed?

I think this would be confusing, because putting the files there would suggest that they are automatically included, like the files in i.e. /usr/share/nftables.d/ruleset-pre/*.nft or /etc/nftables.d/*.nft.

I prefer /usr/share/lime/, which is where we also keep the defaults for lime-community and lime-node. I moved the files there and changed the scripts to [un]symlink them into /usr/share/nftables.d/ruleset-post/. I also renamed them to follow the convention <package-name>_<description>.

@dangowrt @G10h4ck Are you fine with this?

dangowrt commented 1 year ago

Maybe squash the commits so it looks nice in lime-packages git history. But apart from that I think this is ready to go in.