Re: [libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
On 10/12/2011 03:50 PM, David L Stevens wrote: This patch simplifies the table rules by setting the protocol chains policy to be DROP and removes the explicit -j DROP entries that the protocol rules had previously. It also makes no-other-rarp-traffic.xml obsolete. I agree with Daniel's previous comments that this could introduce compatibility problems. It would be best not to change it or if really need be later on introduce an XML attribute for a chain that allows to choose whether the default policy is accept or drop. Stefan Signed-off-by: David L Stevensdlstev...@us.ibm.com --- examples/xml/nwfilter/Makefile.am |1 - examples/xml/nwfilter/no-arpip-spoofing.xml |2 -- examples/xml/nwfilter/no-arpmac-spoofing.xml|2 -- examples/xml/nwfilter/no-ip-spoofing.xml|2 -- examples/xml/nwfilter/no-mac-spoofing.xml |2 -- examples/xml/nwfilter/no-other-rarp-traffic.xml |3 --- examples/xml/nwfilter/qemu-announce-self.xml|1 - src/nwfilter/nwfilter_ebiptables_driver.c | 11 +-- 8 files changed, 1 insertions(+), 23 deletions(-) delete mode 100644 examples/xml/nwfilter/no-other-rarp-traffic.xml diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index 84aaa3c..67085fa 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -18,7 +18,6 @@ FILTERS = \ no-mac-broadcast.xml \ no-mac-spoofing.xml \ no-other-l2-traffic.xml \ - no-other-rarp-traffic.xml \ qemu-announce-self.xml \ qemu-announce-self-rarp.xml diff --git a/examples/xml/nwfilter/no-arpip-spoofing.xml b/examples/xml/nwfilter/no-arpip-spoofing.xml index ee42d40..7ef6f0f 100644 --- a/examples/xml/nwfilter/no-arpip-spoofing.xml +++ b/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -7,6 +7,4 @@ rule action='return' direction='out' priority='410' arp match='yes' arpsrcipaddr='0.0.0.0' / /rule -!-- drop everything else -- -rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-arpmac-spoofing.xml b/examples/xml/nwfilter/no-arpmac-spoofing.xml index 90499d3..3834047 100644 --- a/examples/xml/nwfilter/no-arpmac-spoofing.xml +++ b/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -2,6 +2,4 @@ rule action='return' direction='out' priority='350' arp match='yes' arpsrcmacaddr='$MAC'/ /rule -!-- drop everything else -- -rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index 84e8a5e..2fccd12 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,6 +4,4 @@ rule action='return' direction='out' ip match='yes' srcipaddr='$IP' / /rule -!-- drop any that don't match the source IP list -- -rule action='drop' direction='out' / /filter diff --git a/examples/xml/nwfilter/no-mac-spoofing.xml b/examples/xml/nwfilter/no-mac-spoofing.xml index aee56c7..e2e8c03 100644 --- a/examples/xml/nwfilter/no-mac-spoofing.xml +++ b/examples/xml/nwfilter/no-mac-spoofing.xml @@ -4,6 +4,4 @@ rule action='return' direction='out' priority='350' mac match='yes' srcmacaddr='$MAC'/ /rule -!-- drop everything else -- -rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-other-rarp-traffic.xml b/examples/xml/nwfilter/no-other-rarp-traffic.xml deleted file mode 100644 index 7729996..000 --- a/examples/xml/nwfilter/no-other-rarp-traffic.xml +++ /dev/null @@ -1,3 +0,0 @@ -filter name='no-other-rarp-traffic' chain='rarp' -rule action='drop' direction='inout' priority='1000'/ -/filter diff --git a/examples/xml/nwfilter/qemu-announce-self.xml b/examples/xml/nwfilter/qemu-announce-self.xml index 352db50..12957b5 100644 --- a/examples/xml/nwfilter/qemu-announce-self.xml +++ b/examples/xml/nwfilter/qemu-announce-self.xml @@ -8,6 +8,5 @@ !-- accept if it was changed to rarp -- filterref filter='qemu-announce-self-rarp'/ -filterref filter='no-other-rarp-traffic'/ /filter diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 3c6fca7..e6a4880 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2791,7 +2791,7 @@ ebtablesCreateTmpSubChain(virBufferPtr buf, protostr[0] = '\0'; virBufferAsprintf(buf, - CMD_DEF(%s -t %s -N %s) CMD_SEPARATOR + CMD_DEF(%s -t %s -N %s -P DROP) CMD_SEPARATOR CMD_EXEC %s CMD_DEF(%s -t %s -A %s %s -j %s) CMD_SEPARATOR @@ -3015,15 +3015,6 @@ ebtablesApplyBasicRules(const char *ifname, PRINT_ROOT_CHAIN(chain, chainPrefix, ifname); virBufferAsprintf(buf, -
Re: [libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
Stefan Berger stef...@linux.vnet.ibm.com wrote on 10/17/2011 08:50:14 AM: I agree with Daniel's previous comments that this could introduce compatibility problems. It would be best not to change it or if really need be later on introduce an XML attribute for a chain that allows to choose whether the default policy is accept or drop. This is not a functional change. With the multiple address support, literally all of the chains in question end with -j DROP. By changing the default policy to DROP, it removes the requirement to have the -j DROP _at_the_end_, which makes appending new rules without reference to a priority easier. I think the real sticking point here is whether we consider the existing standard chains as immutable. If, for compatibility's sake, we keep address matching as: if ($IP != ADDRESS) DROP ACCEPT then we cannot support multiple IP addresses. If we change this to: if ($IP == ADDRESS1) RETURN if ($IP == ADDRESS2) RETURN ... DROP all of the standard chains end in -j DROP and making the policy be DROP expresses exactly the same thing with one less rule per chain. This also allows appending more allowed addresses without using a priority to ensure that -j DROP is last. Any customization to the old filters that do DROP or ACCEPT will still work and RETURN and CONTINUE were not supported before, so the default, whether done by -j DROP/ACCEPT or the policy, don't matter-- the custom rule will behave as it does now. It's just that that DROP/ACCEPT will bypass subsequent checks that are now possible if doing RETURN/CONTINUE instead. But, of course, *any* change to the standard filters requires a review of custom filters that link into them or modify them. I think maintaining this level of compatibility simply means we cannot support multiple addresses. Doing that obviously requires changing the existing address matching filters, which therefore can't be linked in in identically the same way. However, I think any existing customization is trivial to apply after, too. Perhaps an example filter that causes a problem? +-DLS -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
On 10/17/2011 01:04 PM, David Stevens wrote: Stefan Bergerstef...@linux.vnet.ibm.com wrote on 10/17/2011 08:50:14 AM: I agree with Daniel's previous comments that this could introduce compatibility problems. It would be best not to change it or if really need be later on introduce an XML attribute for a chain that allows to choose whether the default policy is accept or drop. This is not a functional change. With the multiple address support, literally all of the chains in question end with -j DROP. By changing the default policy to DROP, it removes the requirement to have the -j DROP _at_the_end_, which makes appending new rules without reference to a priority easier. Yes, '_at_the_end_', that's what I thought. I am not sure whether this particular requirement is the best way to proceed since obviously you cannot have any other rules with lesser priority after the ones doing the 'return' -- whatever those rules may be doing. The alternative would be having to instantiate the whole filter chain in the same way as the IP address learning thread does with the 'late' filter instantiation call -- that would at least get all the priorities correct and not introduce a requirement on how one has to write a filter. I think the real sticking point here is whether we consider the existing standard chains as immutable. If, for compatibility's sake, I don't consider them as immutable but should be replaced with something of the same or 'better' functionality. we keep address matching as: if ($IP != ADDRESS) DROP ACCEPT then we cannot support multiple IP addresses. If we change this to: if ($IP == ADDRESS1) RETURN if ($IP == ADDRESS2) RETURN ... DROP all of the standard chains end in -j DROP and making the policy be DROP expresses exactly the same thing with one less rule per chain. This also allows appending more allowed addresses without using a priority to ensure that -j DROP is last. Yes, I understand that but I don't necessarily like the implications of this. Any customization to the old filters that do DROP or ACCEPT will still work and RETURN and CONTINUE were not supported before, so the default, whether done by -j DROP/ACCEPT or the policy, don't matter-- the custom rule will behave as it does now. It's just that that DROP/ACCEPT will bypass subsequent checks that are now possible if doing RETURN/CONTINUE instead. But, of course, *any* change to the standard filters requires a review of custom filters that link into them or modify them. I think maintaining this level of compatibility simply means we cannot support multiple addresses. Doing that obviously requires changing the existing address matching filters, which therefore can't be linked in in identically the same way. However, I think any existing customization is trivial to apply after, too. Perhaps an example filter that causes a problem? I don't have an example filter myself, but I don't know what other people may have written. An alternative would be to say that the existing filters work with the IP address learning thread and we would need to introduce new filters for support of multiple IP addresses. Yes, the current filters aren't prepare for supporting multiple IP addresses per interface. I don't think replacing the existing filters would be a problem per-se, but I don't like that the priority of the rules doing the 'RETURN' is assumed to be the lowest in the chain and we can just append anything to the end of the chain -- the filters we are writing are just examples and someone may come up with a few rules that do something with packets that were not RETURN'ed and thus needs to have rules executing behind those. Again, maybe (the less efficient but more generic way of) instantiating the filters by calling the virNWFilterInstantiateFilterLate() could solve part of the problem. Stefan +-DLS -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
Stefan Berger stef...@linux.vnet.ibm.com wrote on 10/17/2011 10:29:08 AM: Yes, '_at_the_end_', that's what I thought. I am not sure whether this particular requirement is the best way to proceed since obviously you cannot have any other rules with lesser priority after the ones doing the 'return' -- whatever those rules may be doing. Not in the same chain, but if a chain is checking multiple allowed values for the same field, that's all that chain should be doing. Checking some other condition should be done after passing, e.g., the IP address checks and would be done because of the RETURN. Current code does an ACCEPT or REJECT at that point, so *requires* modification of the existing chain. Any current filters are even worse because without RETURN and CONTINUE, they can only accept or drop a packet without further processing. With this patchset, you can apply multiple tests to the same packet with only the restriction that testing other fields in that packet require a different subchain-- something not possible at all today. If, for example, you want to allow 100 ports and a particular IP address, reject everything else, won't that require a DROP rule for 65436 ports so that you don't accept either based on just the port or just the IP address? Or , you'd have to combine the IP address check in every port-check rule and do it before you do the blanket ACCEPT in the standard rule. An alternative would be to say that the existing filters work with the IP address learning thread and we would need to introduce new filters for support of multiple IP addresses. Yes, the current filters aren't prepare for supporting multiple IP addresses per interface. Yes, I thought about this, but it really is just duplicating the entire set with a different name. I think especially without support for RETURN/CONTINUE, as a practical matter the only way to modify the current filters to do interesting things is to replace them. Anyone doing that will not be affected by a change to the standard rules, except for the possibly trivial addition of a -j ACCEPT at the end if they require a default ACCEPT for the modified rules. I don't think replacing the existing filters would be a problem per-se, but I don't like that the priority of the rules doing the 'RETURN' is assumed to be the lowest in the chain and we can just append anything to the end of the chain -- the filters we are writing are just examples and someone may come up with a few rules that do something with packets that were not RETURN'ed and thus needs to have rules executing behind those. Again, maybe (the less efficient but more generic way of) instantiating the filters by calling the virNWFilterInstantiateFilterLate() could solve part of the problem. The only use I've added for addRules is the multiple address check and that should not have other random fields in the same chain. Better would be to have a different chain linked at the top after the address checks. In fact, for compatibility, it'd be possible to change the address checks to a different chain and leave an empty ip and arp chain that does only accept. It's just that it isn't really all that compatible if what's really happening is that a customer is replacing the existing chains with modifications, rather than applying additional rules to it. In the end, any modification whatsoever to the examples directory requires someone who has customized a filter to look at the customization again. Addition of RETURN/CONTINUE makes that easier in the future. With the existing filters, any customization cannot be independent of the internals of the existing filters (it's adding rules in the middle of it), so the only way to maintain compatibility is to never change them. With RETURN/CONTINUE and your suggested user-defined chain(s), you can apply the standard sets with RETURN for acceptable packets and then apply any user-defined checks also to the same packets afterwards. I think that's the right approach, but it requires either changing the existing chains or introducing a completely parallel set and not using the original filters at all, except for sites that want the old stuff only. +-DLS -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
On 10/17/2011 05:22 PM, David Stevens wrote: Stefan Bergerstef...@linux.vnet.ibm.com wrote on 10/17/2011 10:29:08 AM: Yes, '_at_the_end_', that's what I thought. I am not sure whether this particular requirement is the best way to proceed since obviously you cannot have any other rules with lesser priority after the ones doing the 'return' -- whatever those rules may be doing. Not in the same chain, but if a chain is checking multiple allowed values for the same field, that's all that chain should be doing. Sure, and what if I wanted to do something with packets that were not RETURN'ed, where would I treat them besides *after* the RETURN? Checking some other condition should be done after passing, e.g., the IP address checks and would be done because of the RETURN. Current code does an ACCEPT or REJECT at that point, so *requires* modification of the existing chain. Any current filters are even worse because without RETURN and CONTINUE, they can only accept or drop a packet without further processing. Not arguing against that. With this patchset, you can apply multiple tests to the same packet with only the restriction that testing other fields in that packet require a different subchain-- something not possible at all today. If, for example, you want to allow 100 ports and a particular IP address, reject everything else, won't that require a DROP rule for 65436 ports so that you don't accept either based on just the port or just the IP address? Or , you'd have to combine the IP address check in every port-check rule and do it before you do the blanket ACCEPT in the standard rule. An alternative would be to say that the existing filters work with the IP address learning thread and we would need to introduce new filters for support of multiple IP addresses. Yes, the current filters aren't prepare for supporting multiple IP addresses per interface. Yes, I thought about this, but it really is just duplicating the entire set with a different name. I think especially without support for RETURN/CONTINUE, as a practical matter the only way to modify the current filters to do interesting things is to replace them. Anyone doing that will not be affected by a change to the standard rules, except for the possibly trivial addition of a -j ACCEPT at the end if they require a default ACCEPT for the modified rules. I don't think replacing the existing filters would be a problem per-se, but I don't like that the priority of the rules doing the 'RETURN' is assumed to be the lowest in the chain and we can just append anything to the end of the chain -- the filters we are writing are just examples and someone may come up with a few rules that do something with packets that were not RETURN'ed and thus needs to have rules executing behind those. Again, maybe (the less efficient but more generic way of) instantiating the filters by calling the virNWFilterInstantiateFilterLate() could solve part of the problem. The only use I've added for addRules is the multiple address check and that should not have other random fields in the same chain. Better would be to have a different chain linked at the What if in the future we were to support ebtables logging and wanted to log any packet that has not been handled with a RETURN, thus for example logging MAC spoofing -- just as an example. I don't see how one would do that without adding rules into the mac chain *after* the rules that do the RETURN for packets with acceptable source MAC addresses and all others get logged. I would not call these 'random fields' being evaluated, but that the implementation of addRules() requires the filters to be limited in a way that may at some point become a problem for some use cases and someone has to reimplement it then. Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCHv3 04/10] make default chain policy DROP
This patch simplifies the table rules by setting the protocol chains policy to be DROP and removes the explicit -j DROP entries that the protocol rules had previously. It also makes no-other-rarp-traffic.xml obsolete. Signed-off-by: David L Stevens dlstev...@us.ibm.com --- examples/xml/nwfilter/Makefile.am |1 - examples/xml/nwfilter/no-arpip-spoofing.xml |2 -- examples/xml/nwfilter/no-arpmac-spoofing.xml|2 -- examples/xml/nwfilter/no-ip-spoofing.xml|2 -- examples/xml/nwfilter/no-mac-spoofing.xml |2 -- examples/xml/nwfilter/no-other-rarp-traffic.xml |3 --- examples/xml/nwfilter/qemu-announce-self.xml|1 - src/nwfilter/nwfilter_ebiptables_driver.c | 11 +-- 8 files changed, 1 insertions(+), 23 deletions(-) delete mode 100644 examples/xml/nwfilter/no-other-rarp-traffic.xml diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index 84aaa3c..67085fa 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -18,7 +18,6 @@ FILTERS = \ no-mac-broadcast.xml \ no-mac-spoofing.xml \ no-other-l2-traffic.xml \ - no-other-rarp-traffic.xml \ qemu-announce-self.xml \ qemu-announce-self-rarp.xml diff --git a/examples/xml/nwfilter/no-arpip-spoofing.xml b/examples/xml/nwfilter/no-arpip-spoofing.xml index ee42d40..7ef6f0f 100644 --- a/examples/xml/nwfilter/no-arpip-spoofing.xml +++ b/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -7,6 +7,4 @@ rule action='return' direction='out' priority='410' arp match='yes' arpsrcipaddr='0.0.0.0' / /rule - !-- drop everything else -- - rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-arpmac-spoofing.xml b/examples/xml/nwfilter/no-arpmac-spoofing.xml index 90499d3..3834047 100644 --- a/examples/xml/nwfilter/no-arpmac-spoofing.xml +++ b/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -2,6 +2,4 @@ rule action='return' direction='out' priority='350' arp match='yes' arpsrcmacaddr='$MAC'/ /rule - !-- drop everything else -- - rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index 84e8a5e..2fccd12 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,6 +4,4 @@ rule action='return' direction='out' ip match='yes' srcipaddr='$IP' / /rule -!-- drop any that don't match the source IP list -- -rule action='drop' direction='out' / /filter diff --git a/examples/xml/nwfilter/no-mac-spoofing.xml b/examples/xml/nwfilter/no-mac-spoofing.xml index aee56c7..e2e8c03 100644 --- a/examples/xml/nwfilter/no-mac-spoofing.xml +++ b/examples/xml/nwfilter/no-mac-spoofing.xml @@ -4,6 +4,4 @@ rule action='return' direction='out' priority='350' mac match='yes' srcmacaddr='$MAC'/ /rule - !-- drop everything else -- - rule action='drop' direction='out' priority='1000' / /filter diff --git a/examples/xml/nwfilter/no-other-rarp-traffic.xml b/examples/xml/nwfilter/no-other-rarp-traffic.xml deleted file mode 100644 index 7729996..000 --- a/examples/xml/nwfilter/no-other-rarp-traffic.xml +++ /dev/null @@ -1,3 +0,0 @@ -filter name='no-other-rarp-traffic' chain='rarp' -rule action='drop' direction='inout' priority='1000'/ -/filter diff --git a/examples/xml/nwfilter/qemu-announce-self.xml b/examples/xml/nwfilter/qemu-announce-self.xml index 352db50..12957b5 100644 --- a/examples/xml/nwfilter/qemu-announce-self.xml +++ b/examples/xml/nwfilter/qemu-announce-self.xml @@ -8,6 +8,5 @@ !-- accept if it was changed to rarp -- filterref filter='qemu-announce-self-rarp'/ -filterref filter='no-other-rarp-traffic'/ /filter diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 3c6fca7..e6a4880 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2791,7 +2791,7 @@ ebtablesCreateTmpSubChain(virBufferPtr buf, protostr[0] = '\0'; virBufferAsprintf(buf, - CMD_DEF(%s -t %s -N %s) CMD_SEPARATOR + CMD_DEF(%s -t %s -N %s -P DROP) CMD_SEPARATOR CMD_EXEC %s CMD_DEF(%s -t %s -A %s %s -j %s) CMD_SEPARATOR @@ -3015,15 +3015,6 @@ ebtablesApplyBasicRules(const char *ifname, PRINT_ROOT_CHAIN(chain, chainPrefix, ifname); virBufferAsprintf(buf, - CMD_DEF(%s -t %s -A %s -s ! %s -j DROP) CMD_SEPARATOR - CMD_EXEC - %s, - - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - chain, macaddr_str, - CMD_STOPONERR(1)); - -virBufferAsprintf(buf,
