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

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

Merged
merged 1 commit into from
May 2, 2023

Conversation

pony1k
Copy link
Contributor

@pony1k pony1k commented Apr 27, 2023

Using /etc/init.d-scripts for making nftable / xtable rules can lead to problems. See this comment: #1020 (comment) .
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
Copy link
Contributor Author

pony1k commented Apr 27, 2023

see also: #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
Copy link
Contributor Author

pony1k commented Apr 27, 2023

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

@dangowrt
Copy link
Member

Very good work, thank you!

@G10h4ck
Copy link
Member

G10h4ck commented Apr 27, 2023

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
Copy link
Member

G10h4ck commented Apr 28, 2023

ping @pony1k @dangowrt

@pony1k
Copy link
Contributor Author

pony1k commented Apr 28, 2023

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: openwrt/openwrt#11620.

@pony1k
Copy link
Contributor Author

pony1k commented Apr 30, 2023

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
Copy link
Member

G10h4ck commented May 1, 2023

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
Copy link
Member

dangowrt commented May 1, 2023

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
Copy link
Contributor Author

pony1k commented May 2, 2023

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
Copy link
Member

dangowrt commented May 2, 2023

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.

…bles, use nft include instead, fix bmx7 not over bat0, update dependencies
@pony1k pony1k force-pushed the translate-xtables-to-nft branch from 272c89f to 758a26b Compare May 2, 2023 16:05
@G10h4ck G10h4ck merged commit 482bee1 into libremesh:master May 2, 2023
1 check failed
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.

None yet

3 participants