Re: [CFR] unified rc.firewall

On Monday 23 November 2009 12:27:23 pm Hajimu UMEMOTO wrote:
> Hi,
> 
> >>>>> On Mon, 23 Nov 2009 10:56:14 -0500
> >>>>> John Baldwin <jhb_at_freebsd.org> said:
> 
> jhb> _at__at_ -178,6 +212,16 _at__at_
> jhb>         # Allow any traffic to or from my own net.
> jhb>         ${fwcmd} add pass all from me to ${net}
> jhb>         ${fwcmd} add pass all from ${net} to me
> jhb> +       if [ -n "$net6" ]; then
> jhb> +               ${fwcmd} add pass ip6 from me6 to ${net6}
> jhb> +               ${fwcmd} add pass ip6 from ${net6} to me6
> jhb> +       fi
> jhb> +
> jhb> +       if [ -n "$net6" ]; then
> jhb> +               # Allow any link-local multicast traffic
> jhb> +               ${fwcmd} add pass ip6 from fe80::/10 to ff02::/16
> jhb> +               ${fwcmd} add pass ip6 from ${net6} to ff02::/16
> jhb> +       fi
> 
> jhb> Any reason to not use 'all' here rather than 'ip6' to match the earlier IPv4
> jhb> rules?
> 
> Thank you for the review.
> The rule is only applicable for IPv6.  Rather, I prefer to use 'ip4'
> explicitly over 'all' or 'ip' here.  However, changing 'all' to 'ip4'
> makes the diff complex.  So, I keep 'all' as is.

Hmm, however, using 'all' will work, and while in this case the typing is the
same I find it easier to read 'add pass tcp <...>' vs
'add pass ip <...> proto tcp'.  I do think they should be consistent
regardless.

> jhb>         # For services permitted below.
> jhb>         ${fwcmd} add pass tcp  from me to any established
> jhb> +       if [ $ipv6_available -eq 0 ]; then
> jhb> +               ${fwcmd} add pass ip6 from any to any proto tcp established
> jhb> +       fi
> 
> jhb> I think this extra rule here isn't needed at all as the first rule should
> jhb> already match all of those packets.
> 
> WORKSTATION type rule is fully dynamic.  However, I saw it doesn't
> work for IPv6 as expected.  SSH connection stalls after some period.
> I suspect keepalive timer doesn't work well for IPv6.
> So, I changed to use traditional setup/established rule for TCP/IPv6.
> Further, 'me' doesn't match to IPv6 address.

I had missed the me vs any.  It is true that the equivalent rule would use
me6.  I would rather figure out the IPv6 bug so that TCP is treated the
same for both protocols instead of having a weaker firewall for IPv6 than
IPV4.

> jhb>         # Allow any connection out, adding state for each.
> jhb>         ${fwcmd} add pass tcp  from me to any setup keep-state
> jhb>         ${fwcmd} add pass udp  from me to any       keep-state
> jhb>         ${fwcmd} add pass icmp from me to any       keep-state
> jhb> +       if [ $ipv6_available -eq 0 ]; then
> jhb> +               ${fwcmd} add pass ip6 from me6 to any proto tcp setup
> jhb> +               ${fwcmd} add pass ip6 from me6 to any proto udp keep-state
> jhb> +               ${fwcmd} add pass ip6 from me6 to any proto ipv6-icmp \
> jhb> +                   keep-state
> jhb> +       fi
> 
> jhb> I think it is more consistent to use 'pass tcp from me6 to any' similar to
> jhb> the IPv4 rules here.  It is also shorter and easier to read that way IMO.
> 
> I thought similar thing with 'all' vs 'ip4'.  Rather, I prefer to
> change IPv4 rules.  However, if 'all' is preferable, I'll change so.

I do find the shorter version easier to read, and it matches the existing
style as well as the examples in the manual page, handbook, etc.

-- 
John Baldwin