Re: [CFR] unified rc.firewall
Doug Barton
> Hi,
>
> The ipfw and ip6fw were unified into ipfw2, now. But, we still have
> rc.firewall and rc.firewall6. However, there are conflicts with each
> other, and it confuses the users, IMHO.
> So, I made a patch to unify rc.firewall and rc.firewall6, and obsolete
> rc.firewall6 and rc.d/ip6fw.
> Please review the attached patch. If there is no objection, I'll
> commit it in next weekend.
Overall I think this is good, and I'm definitely in favor of more
integration of IPv6 into the mainstream rather than something that is
glued on.
A few comments:
In rc.firewall you seem to have copied afexists() from network.subr.
Is there a reason that you did not simply source that file? That would
be the preferred method. Also in that file you call "if afexists
inet6" quite a few times. My preference from a performance standpoint
would be to call it once, perhaps in a start_precmd then cache the value.
And of course, you have regression tested this thoroughly, yes? :)
Please include scenarios where there is no INET6 in the kernel as well.
hth,
Doug
--
Improve the effectiveness of your Internet presence with
a domain name makeover! http://SupersetSolutions.com/
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
Hajimu UMEMOTO
>>>>> On Sun, 22 Nov 2009 11:12:33 -0800
>>>>> Doug Barton <do...@FreeBSD.org> said:
dougb> In rc.firewall you seem to have copied afexists() from network.subr.
dougb> Is there a reason that you did not simply source that file? That would
dougb> be the preferred method. Also in that file you call "if afexists
dougb> inet6" quite a few times. My preference from a performance standpoint
dougb> would be to call it once, perhaps in a start_precmd then cache the value.
Thank you for the comments.
Ah, yes, afexists() is only in 9-CURRENT, and is not MFC'ed into 8,
yet. So, I thought the patch should be able to work on both 9 and 8,
for review. I've changed to source network.subr for afexists().
Calling afexists() several times was not good idea. So, I've changed
to call afexists() just once.
The new patch is attached.
dougb> And of course, you have regression tested this thoroughly, yes? :)
dougb> Please include scenarios where there is no INET6 in the kernel as well.
Okay, I've tested it on INET6-less kernel, as well.
Sincerely,
John Baldwin
Some comments I have:
@@ -178,6 +212,16 @@
# Allow any traffic to or from my own net.
${fwcmd} add pass all from me to ${net}
${fwcmd} add pass all from ${net} to me
+ if [ -n "$net6" ]; then
+ ${fwcmd} add pass ip6 from me6 to ${net6}
+ ${fwcmd} add pass ip6 from ${net6} to me6
+ fi
+
+ if [ -n "$net6" ]; then
+ # Allow any link-local multicast traffic
+ ${fwcmd} add pass ip6 from fe80::/10 to ff02::/16
+ ${fwcmd} add pass ip6 from ${net6} to ff02::/16
+ fi
Any reason to not use 'all' here rather than 'ip6' to match the earlier IPv4
rules?
@@ -273,6 +329,55 @@
${fwcmd} add deny all from 224.0.0.0/4 to any via ${oif}
${fwcmd} add deny all from 240.0.0.0/4 to any via ${oif}
+ if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
+ # Stop unique local unicast address on the outside interface
+ ${fwcmd} add deny ip6 from fc00::/7 to any via ${oif6}
+ ${fwcmd} add deny ip6 from any to fc00::/7 via ${oif6}
+
...
Similarly here, why not use 'all' instead of 'ip6'?
@@ -291,7 +396,11 @@
${fwcmd} add pass tcp from any to me 80 setup
# Reject&Log all setup of incoming connections from the outside
- ${fwcmd} add deny log tcp from any to any in via ${oif} setup
+ ${fwcmd} add deny log ip4 from any to any in via ${oif} setup proto
tcp
+ if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
+ ${fwcmd} add deny log ip6 from any to any in via ${oif6} \
+ setup proto tcp
+ fi
I would actually not use separate v6 interfaces for the 'simple' firewall
but just have 'oif', 'onet', and 'onet_ipv6' variables. Then you don't need
this diff at all as the existing rule will work fine.
# For services permitted below.
${fwcmd} add pass tcp from me to any established
+ if [ $ipv6_available -eq 0 ]; then
+ ${fwcmd} add pass ip6 from any to any proto tcp established
+ fi
I think this extra rule here isn't needed at all as the first rule should
already match all of those packets.
# Allow any connection out, adding state for each.
${fwcmd} add pass tcp from me to any setup keep-state
${fwcmd} add pass udp from me to any keep-state
${fwcmd} add pass icmp from me to any keep-state
+ if [ $ipv6_available -eq 0 ]; then
+ ${fwcmd} add pass ip6 from me6 to any proto tcp setup
+ ${fwcmd} add pass ip6 from me6 to any proto udp keep-state
+ ${fwcmd} add pass ip6 from me6 to any proto ipv6-icmp \
+ keep-state
+ fi
I think it is more consistent to use 'pass tcp from me6 to any' similar to
the IPv4 rules here. It is also shorter and easier to read that way IMO.
--
John Baldwin
Bjoern A. Zeeb
I haven't looked at the entire update but as I see this I shall note
unless I missed a fix to ipfw, you need to make that ip and use ip6
and me6 for the new world order.
Please make sure that this works as expected in mixed-world scenarios
as well as legacy IP and IPv6 only worlds.
/bz
--
Bjoern A. Zeeb It will not break if you know what you are doing.
Hajimu UMEMOTO
>>>>> On Mon, 23 Nov 2009 10:56:14 -0500
>>>>> John Baldwin <j...@freebsd.org> said:
jhb> @@ -178,6 +212,16 @@
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.
jhb> @@ -273,6 +329,55 @@
jhb> ${fwcmd} add deny all from 224.0.0.0/4 to any via ${oif}
jhb> ${fwcmd} add deny all from 240.0.0.0/4 to any via ${oif}
jhb>
jhb> + if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
jhb> + # Stop unique local unicast address on the outside interface
jhb> + ${fwcmd} add deny ip6 from fc00::/7 to any via ${oif6}
jhb> + ${fwcmd} add deny ip6 from any to fc00::/7 via ${oif6}
jhb> +
jhb> ....
jhb> Similarly here, why not use 'all' instead of 'ip6'?
Same above.
jhb> @@ -291,7 +396,11 @@
jhb> ${fwcmd} add pass tcp from any to me 80 setup
jhb>
jhb> # Reject&Log all setup of incoming connections from the outside
jhb> - ${fwcmd} add deny log tcp from any to any in via ${oif} setup
jhb> + ${fwcmd} add deny log ip4 from any to any in via ${oif} setup proto
jhb> tcp
jhb> + if [ -n "$oif6" -a -n "$onet6" -a -n "$iif6" -a -n "$inet6" ]; then
jhb> + ${fwcmd} add deny log ip6 from any to any in via ${oif6} \
jhb> + setup proto tcp
jhb> + fi
jhb> I would actually not use separate v6 interfaces for the 'simple' firewall
jhb> but just have 'oif', 'onet', and 'onet_ipv6' variables. Then you don't need
jhb> this diff at all as the existing rule will work fine.
Yup, it should makes rule simpler. However, many sites still use
tunnel for IPv6 connectivity. I think, separating 'oif' and 'oif6'
makes such sites happy. So, this diff should make sense, IMHO.
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.
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.
Sincerely,
--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
u...@mahoroba.org ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/
John Baldwin
> Hi,
>
> >>>>> On Mon, 23 Nov 2009 10:56:14 -0500
> >>>>> John Baldwin <j...@freebsd.org> said:
>
> jhb> @@ -178,6 +212,16 @@
> 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
Kevin Oberman
> Date: Mon, 23 Nov 2009 12:55:25 -0500
> Sender: owner-free...@freebsd.org
FWIW, I have been seeing this since the last update of OpenSSH. I never
saw it until then. It's a real pain and I'd love to see it fixed. Right
now I'm forced to use IPv4 for the jobs that I tunnel in SSH.
--
R. Kevin Oberman, Network Engineer
Energy Sciences Network (ESnet)
Ernest O. Lawrence Berkeley National Laboratory (Berkeley Lab)
E-mail: obe...@es.net Phone: +1 510 486-8634
Key fingerprint:059B 2DDF 031C 9BA3 14A4 EADA 927D EBB3 987B 3751
Hajimu UMEMOTO
>>>>> On Mon, 23 Nov 2009 10:27:43 -0800
>>>>> Benjamin Lee <b...@b1c1l1.com> said:
ben> There is a bug in ipfw send_pkt() that prevents ipfw_tick() from
ben> functioning for IPv6. See PR kern/117234.
I confirmed that the patch fixed the problem. Thank you for letting
me know.
Sincerely,
--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
u...@mahoroba.org ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/
Hajimu UMEMOTO
>>>>> On Mon, 23 Nov 2009 12:55:25 -0500
>>>>> John Baldwin <j...@freebsd.org> said:
I updated the patch.
jhb> I had missed the me vs any. It is true that the equivalent rule would use
jhb> me6. I would rather figure out the IPv6 bug so that TCP is treated the
jhb> same for both protocols instead of having a weaker firewall for IPv6 than
jhb> IPV4.
Yes, it is better, definitely. I thought that we could change to use
dynamic rule, once it was fixed.
Since the PR kern/117234 fixed it, I changed to use dynamic rule for
IPv6 as well. So, it requires the patch in the PR.
jhb> I do find the shorter version easier to read, and it matches the existing
jhb> style as well as the examples in the manual page, handbook, etc.
Okay, I changed 'ip6' to 'all' where we can use it, and stopped use of
'proto xxx'' as possible.
I reconsidered oif vs oif6 and iif vs iif6 issue. Now, if
$firewall_simple_oif_ipv6 is not set, $firewall_simple_oif is assumed
for oif6, and, $firewall_simple_iif_ipv6 is not set,
$firewall_simple_iif is assumed for iif6.
Further, I think we don't assign a global IPv6 address to oif in
usual. So, I made $firewall_simple_onet_ipv6 optional.
One more change is that DHCPv6 is allowed as well as IPv4 DHCP for
WORKSTATION type. I'm using DHCPv6 in usual; L2TP + DHCPv6 PD, DHCPv6
DNS option ...
Sincerely,
John Baldwin
I think you can just remove the ipv6_firewall_* variables from
/etc/defaults/rc.conf completely. Perhaps you can use 'set_rcvar_obsolete'
in /etc/rc.firewall to emit a warning if ipv6_firewall_enable is defined?
Or maybe just emit an explicit warning in /etc/rc.firewall in that case?
Other than that I think this patch looks good. Thanks for fixing this!
Hajimu UMEMOTO
>>>>> On Mon, 30 Nov 2009 13:00:03 -0500
>>>>> John Baldwin <j...@freebsd.org> said:
jhb> I think you can just remove the ipv6_firewall_* variables from
jhb> /etc/defaults/rc.conf completely. Perhaps you can use 'set_rcvar_obsolete'
jhb> in /etc/rc.firewall to emit a warning if ipv6_firewall_enable is defined?
jhb> Or maybe just emit an explicit warning in /etc/rc.firewall in that case?
jhb> Other than that I think this patch looks good. Thanks for fixing this!
Thank you for the comment. I've changed to use 'set_rcvar_obsolete',
and committed it.
Sincerely,
--
Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan
u...@mahoroba.org ume@{,jp.}FreeBSD.org
http://www.imasy.org/~ume/