| Message ID | 20220627105920.2317450-1-fe@dev.tdt.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [firewall4] fw4: add support for include.d dir | expand |
Hi,
instead of introducing uci includes that configure nft includes, why not
encode the chain/position etc. values directly into the path/filename and
directly include the file if it exists at the expected location?
A potential pattern could be
"[0-9][0-9]_{ruleset_pre,ruleset_post,table_pre,table_post,chain_pre_*,chain_post_*}_*.nft".
Taking the example from your mail, these *.nft includes would be stored at
/usr/share/firewall4/include.d/01_chain_pre_input_strongswan.nft
/usr/share/firewall4/include.d/02_chain_pre_output_strongswan.nft
/usr/share/firewall4/include.d/03_chain_pre_forward_strongswan.nft
Alternatively, the hooks could be moved into a subdirectory structure for
better clarity:
/usr/share/firewall4/includes.d/
+ ruleset-pre/
+ 99_custom_named_set_declarations.nft
+ ruleset-post/
+ ...
+ table-pre/
+ ...
+ table-post/
+ ...
+ chain-pre/
+ input/
+ 29_strongswan.nft
+ output/
+ 29_strongswan.nft
+ forward/
+ 29_strongswan.nft
+ chain-post/
+ mangle_output/
+ 99_custom_dscp_fiddling.nft
(The numeric prefixes carry no semantic meaning in this structure, they'd just
be there to enforce a certain order within a given hook directory)
I think the above would be a lot more manageable since you'd just have to
place partial .nft files which are then folded into the final ruleset on fw4
start/reload.
~ Jo
Hi, Sorry for the late reply > instead of introducing uci includes that configure nft includes, why > not > encode the chain/position etc. values directly into the path/filename > and > directly include the file if it exists at the expected location? I was just wondering why not use the new feature you added to give other packages the ability to add rules to fw4. The include feature was exactly what was missing for fw4 to add the posibility for other package adding firewall rules to fw4(nftables=. > I think the above would be a lot more manageable since you'd just have > to > place partial .nft files which are then folded into the final ruleset > on fw4 > start/reload. 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. 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). Also the new include add by you already does have the state file feature. Which is relevant on reloading the ruleset. In addition, it would make the ruleset.uc file confusing if I added the same feature again. 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. We also use the include to add the helpers. Last but not leased, if we now add an other possibility, then I don't think anyone knows where which file adds which rule! 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. Best Regards Florian
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
diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc index 1b4764c..f46caa2 100644 --- a/root/usr/share/ucode/fw4.uc +++ b/root/usr/share/ucode/fw4.uc @@ -731,6 +731,16 @@ return { // this.cursor.foreach("firewall", "include", i => self.parse_include(i)); + let dir = fs.opendir("/usr/share/firewall4/include.d"); + if (dir) { + let file; + while ((file = dir.read()) != null) { + if ((file == '.') || (file == '..')) + continue; + this.cursor.load("/usr/share/firewall4/include.d/" + file); + this.cursor.foreach(file, "include", i => self.parse_include(i)); + } + } if (use_statefile) {
Creating a uci configuration is cumbersome and unmaintainable if other packages want to use the new include feature. If another package wants to use the include feature of fw4, it must copy the uci configuration options to '/usr/share/firewall4/include.d'. This include will then be used in fw4 without having to modify the uci config under '/etc/config/firewall'. In my case, this is about the firewall rules for the Strongswan. This feature allows me to have the firewall add the strongswan rules on reload or startup without having to change the firewall's uci configuration. Content of the include file for firewall rules needed by strongswan. The content of the files are update by the strongswan updown script. '/usr/share/firewall4/include.d/strongswan': config include option path '/tmp/strongswan/nftables.d/pre-input.nft' option type 'nftables' option position 'chain-pre' option chain 'input' config include option path '/tmp/strongswan/nftables.d/pre-output.nft' option type 'nftables' option position 'chain-pre' option chain 'output' config include option path '/tmp/strongswan/nftables.d/pre-forward.nft' option type 'nftables' option position 'chain-pre' option chain 'forward'includes Signed-off-by: Florian Eckert <fe@dev.tdt.de> --- root/usr/share/ucode/fw4.uc | 10 ++++++++++ 1 file changed, 10 insertions(+)