arno-iptables-firewall / aif

GNU General Public License v2.0
149 stars 24 forks source link

changed: Workaround non-working + interface wildcard in nftables's iptables binary #56

Closed arnova closed 5 years ago

arnova commented 5 years ago

@abelbeck : Please review/comment

abelbeck commented 5 years ago

As I see it ... the environment function get_ifs is where the + gets added as an interface placeholder as an interface wildcard.

Only parse_rule calls get_ifs which sets the interfaces variable.

So, for parse_rule results using any form of interfaces- (as interfaces:EXT_IF- ever contains a single +) now call a function in a subshell to eliminate -i + and -o + iptables args.

Given that, @arnova it looks like you implemented this perfectly.

I'm not sure how much CPU cost there is with all the extra subshell calls, but I will test.

It appears this is an iptables 1.8.x bug, but I can't argue against eliminating -i + and -o + iptables args.

Alternatively:

$(ipt_in_if "$interface")
$(ipt_out_if "$interface")

could be

$(ipt_if -i "$interface")
$(ipt_if -o "$interface")

ipt_if()
{
  if [ -n "$2" -a "$2" != "+" ]; then
    echo "$1 $2"
  fi
}
arnova commented 5 years ago

@abelbeck : Lonnie, good idea! Updated it, if ok please hit "green".

abelbeck commented 5 years ago

Great ... I'll do some testing over the weekend and merge it if all looks good.

abelbeck commented 5 years ago

Nice solution @arnova

abelbeck commented 5 years ago

@arnova Followup, it looks like plugin scripts: 90dmz-dnat.plugin and dyndns-host-open-helper call parse-rule with interfaces-

I know you don't assume the environment tracks plugins, so you decide.

arnova commented 5 years ago

@abelbeck : I think those should be updated with the next point release.

abelbeck commented 5 years ago

I think those should be updated with the next point release.

@arnova OK, I'll make a PR for fixing the two plugins and you can merge it when you want.

abelbeck commented 5 years ago

@arnova I created PR #57

arnova commented 5 years ago

Thanks @abelbeck

arnova commented 5 years ago

@abelbeck : It turns out $(ipt_if -i "$interface") isn't working properly. With eg. $interface="net0, I get "Warning: weird character in interface ` net0' ('/' and ' ' are not allowed by the kernel)." It's as if the shell is not passing the arguments separately. Ideas on how to fix (I vaguely recall we had this issue before in the past with iptables somewhere)?

arnova commented 5 years ago

@abelbeck : It turns out $(ipt_if -i "$interface") isn't working properly. With eg. $interface="net0, I get "Warning: weird character in interface ` net0' ('/' and ' ' are not allowed by the kernel)." It's as if the shell is not passing the arguments separately. Ideas on how to fix (I vaguely recall we had this issue before in the past with iptables somewhere)?

Fixed it in https://github.com/arno-iptables-firewall/aif/commit/a5a3d92d7a08dfc7cf085168815942d10fc1fb3e . You ok with it? To be honest I don't understand why one works and the other does not...

abelbeck commented 5 years ago

@arnova No, that is not a fix.

I think the problem is with the IFS=',', IFS=' ,' would probably fix it, but let me work on this.

abelbeck commented 5 years ago

@arnova While I have never used $IFS in output before, this seems to provide a general fix:

--- /usr/share/arno-iptables-firewall/environment
+++ ./environment
@@ -1067,7 +1067,7 @@
 ipt_if()
 {
   if [ -n "$2" -a "$2" != "+" ]; then
-    echo "$1 $2"
+    echo "$1${IFS:- }$2"
   fi
 }

Alternately, replacing all the IFS=',' with IFS=' ,' where ipt_if() is called and look to see if that has any side effects.

arnova commented 5 years ago

I like your fix with "echo "$1${IFS:- }$2"" better than modifying IFS all over the place (which may also lead to regressions). Could you PR this?

abelbeck commented 5 years ago

Sure

abelbeck commented 5 years ago

Added PR #64