[libvirt] Appending REJECT rules.

[Daniel P. Berrange]

On Thu, Jun 23, 2011 at 11:00:28AM -0400, Laine Stump wrote:
> On 06/22/2011 08:58 PM, Stefan Berger wrote:
> >On 06/22/2011 06:01 PM, Eric Blake wrote:
> >>On 05/18/2011 03:10 PM, Stephen O'Dor wrote:
> >>>Greetings folks,
> >>Hello, and sorry for the delayed response.  Looks like this fell through
> >>the cracks, because it wasn't in traditional 'git format-patch' style.
> >>
> >>>I've patched the libvirt iptables interface to append it's REJECT
> >>>rules rather than insert at the head. Idea being that I'm not the only
> >>>person who usually puts the REJECTs at the end of a chain.
> >>>
> >>>In my particular case any custom ACCEPT rules involving the bridge
> >>>interfaces would get pushed below the rules that libvirt puts in to
> >>>REJECT everything on the bridge interface.
> >>>
> >>>I'm using the routed network mode, I have no idea if this hurts any
> >>>other network mode.
> >>Stefan is probably the best person to comment on whether this
> >>makes sense.
> >>
> >>>Thanks,
> >>>
> >>>-Steve
> >>>
> >>>
> >>>--- iptables.c  2011-02-28 23:03:32.000000000 -0800
> >>>+++ iptables.c_new      2011-05-18 14:00:59.110855881 -0700
> >>>@@ -51,7 +51,8 @@
> >>>
> >>>  enum {
> >>>      ADD = 0,
> >>>-    REMOVE
> >>>+    REMOVE,
> >>>+    APPEND
> >>>  };
> >>>
> >>>  typedef struct
> >>>@@ -111,7 +112,7 @@
> >>>                          ? IP6TABLES_PATH : IPTABLES_PATH);
> >>>
> >>>      virCommandAddArgList(cmd, "--table", rules->table,
> >>>-                         action == ADD ? "--insert" : "--delete",
> >>>+                         action == ADD ? "--insert" : action ==
> >>>REMOVE ? "--delete" : "--append",
> >>>                           rules->chain, arg, NULL);
> >>>
> >>>      va_start(args, arg);
> >>>@@ -666,7 +667,7 @@
> >>>                              int family,
> >>>                              const char *iface)
> >>>  {
> >>>-    return iptablesForwardRejectOut(ctx, family, iface, ADD);
> >>>+    return iptablesForwardRejectOut(ctx, family, iface, APPEND);
> >>>  }
> >>>
> >>>  /**
> >>>@@ -722,7 +723,7 @@
> >>>                             int family,
> >>>                             const char *iface)
> >>>  {
> >>>-    return iptablesForwardRejectIn(ctx, family, iface, ADD);
> >>>+    return iptablesForwardRejectIn(ctx, family, iface, APPEND);
> >>>  }
> >>>
> >'ADD' caused an 'insertion' at position 0. Now 'APPEND' appends
> >the new rule to the end. To me it makes sense per-se to put the
> >reject rules to the end. There shoudn't be any negative side
> >effects because of this. So I'd give it an ACK.
> 
> This very old bug demonstrates that changing the order of the rules
> can have unintended consequences.
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=453580
> 
> What does this patch do to that situation? A short synopsis - what
> we really want when there are two virtual networks is that the
> guests on the two networks be completely isolated from each other.
> Instead, with the current filter scheme, a guest on network "A" can
> contact a guest on network "B", but "guest B" can't contact " guest
> A". Will changing the ordering of the reject rules make this
> behavior better, worse, or will it remain the same? That question
> needs to be answered before making a decision about this patch.

Yes, appending the REJECT rules is not safe. If there are other existing
rules in the tables, then these rules may accidentally be allowing
traffic to/from the network we just added that is not desired. Of course
the reverse may also be true, so this is a no-win situation. We thus have
to pick the conservative option and insert at the head of the tables.

So NACK to this patch

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|