Hi,

> [...]
> 
> Sorry, but I don't agree with the following reasons. Let me briefly explain
> why.
> 
> All files under '/usr/share/firewall4/includes.d' are uci files. I can see 
> all relevant options.

One problem with your suggested implementation is that you introduce a generic
directory (/usr/share/firewall4/includes.d) containing uci partials that do
look like a full /etc/config/firewall but actually ignore all section types
except `config include` ones, that will lead to confusion.

> I can set in the includes my own path. This is
> relevant for packages that updates the ruleset on events. If I do not want
> to be this rules persistent saved I could use a tmp file in the system
> (strongswan).

Using my proposal you could achieve the same by placing a symlink to a
temporary file path

> Also the new include add by you already does have the state file feature. 
> Which is relevant on reloading the ruleset.

Not sure what you mean with that.

> In addition, it would make the ruleset.uc file confusing if I added the
> same feature again.

The ruleset.uc file wouldn't change at all. The implementation would be
confined to fw4.uc, like your approach.

> Also, I would have to add two sections on include to support temporary
> rules, which I would not have to store under 
> '/usr/share/firewall4/includes.d/' but under '/tmp/firewall4/includes.d/'
> for example to support the not persistent feature.

ln -s /tmp/strongswan/nftables.d/pre-input.nft \
  /usr/share/nftables.d/chain-pre/input/10_strongswan.nft

ln -s /tmp/strongswan/nftables.d/pre-output.nft \
  /usr/share/nftables.d/chain-pre/output/10_strongswan.nft

ln -s /tmp/strongswan/nftables.d/pre-forward.nft \
  /usr/share/nftables.d/chain-pre/forward/10_strongswan.nft

> We also use the include to add the helpers.

Can you elaborate?

> Last but not leased, if we now add an other possibility, then I don't
> think anyone knows where which file adds which rule!

The same applies to /usr/share/firewall4/includes.d/ as well. You do need to
be aware of it to know that includes can stem from there.

> From my point of view, it makes sense to use the include function from you 
> with my extension. So I think the include feature is the better and
> unified solution.

What I do not like about it are several facts:

 - It introduces uci in a non-standard location
 - It implies configurability where there isn't any (it is package managed
   resource files, like init scripts. Technically you can edit init scripts
   but they're certainly not configuration but package integration code).
 - It introduces overhead; you do have to read and parse a uci file to extract
   a path from it which you then write into the ruleset so that nftables can
   include it
 - It creates the false impression of being a general-purpose uci partial
   solution to be able to modularize /etc/config/firewall which it isn't since
   it'll silently ignore any non-include section type being put there


I have attached my proposal as patch for reference.


Kind regards,
Jo
From 5ab0f61350f02590c5e6c1981bce4531510517de Mon Sep 17 00:00:00 2001
From: Jo-Philipp Wich <j...@mein.io>
Date: Thu, 11 Aug 2022 13:48:14 +0200
Subject: [PATCH] fw4: support automatic includes

Introduce a new directory tree /usr/share/nftables/includes.d/ which may
contain partial nftables files being included into the rendered ruleset.

The include position is derived from the file path;

 - Files in .../includes.d/table-pre/ and .../includes.d/table-post/ are
   included before and after the `table inet fw4 { ... }` declaration
   respectively

 - Files in .../includes.d/ruleset-pre/ and .../includes.d/ruleset-post/
   are included before the first chain and after the last chain
   declaration within the fw4 table respectively

 - Files in .../includes.d/chain-pre/${chain}/ and .../chain-post/${chain}/
   are included before the first and after the last rule within the mentioned
   chain of the fw4 table respectively

Automatic includes can be disabled by setting the `auto_includes` option to
`0` in the global defaults section.

Signed-off-by: Jo-Philipp Wich <j...@mein.io>
---
 root/usr/share/ucode/fw4.uc | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc
index 2dc44ac..86e3331 100644
--- a/root/usr/share/ucode/fw4.uc
+++ b/root/usr/share/ucode/fw4.uc
@@ -733,6 +733,19 @@ return {
 		this.cursor.foreach("firewall", "include", i => self.parse_include(i));
 
 
+		//
+		// Discover automatic includes
+		//
+
+		if (this.default_option("auto_includes")) {
+			for (let position in [ 'ruleset-pre', 'ruleset-post', 'table-pre', 'table-post', 'chain-pre', 'chain-post' ])
+				for (let chain in (position in [ 'chain-pre', 'chain-post' ]) ? fs.lsdir(`/usr/share/nftables/includes.d/${position}`) : [ null ])
+					for (let path in fs.glob(`/usr/share/nftables/includes.d/${position}/${chain ?? ''}/*.nft`))
+						if (fs.access(path))
+							this.parse_include({ type: 'nftables', position, chain, path });
+		}
+
+
 		if (use_statefile) {
 			let fd = fs.open(STATEFILE, "w");
 
@@ -1876,7 +1889,9 @@ return {
 			custom_chains: [ "bool", null, UNSUPPORTED ],
 			disable_ipv6: [ "bool", null, UNSUPPORTED ],
 			flow_offloading: [ "bool", "0" ],
-			flow_offloading_hw: [ "bool", "0" ]
+			flow_offloading_hw: [ "bool", "0" ],
+
+			auto_includes: [ "bool", "1" ]
 		});
 
 		if (defs.synflood_protect === null)
-- 
2.35.1

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to