[libvirt] [PATCH V4] nwfilter: Add support for ipset

This patch adds support for the recent ipset iptables extension to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets' of IP addresses, ports and other packet parameters and allows for faster lookup (in the order of O(1) vs. O(n)) and rule evaluation to achieve higher throughput than what can be achieved with individual iptables rules. On the command line iptables supports ipset using iptables ... -m set --match-set <ipset name> <flags> -j ... where 'ipset name' is the name of a previously created ipset and flags is a comma-separated list of up to 6 flags. Flags use 'src' and 'dst' for selecting IP addresses, ports etc. from the source or destination part of a packet. So a concrete example may look like this: iptables -A INPUT -m set --match-set test src,src -j ACCEPT Since ipset management is quite complex, the idea was to leave ipset management outside of libvirt but still allow users to reference an ipset. The user would have to make sure the ipset is available once the VM is started so that the iptables rule(s) referencing the ipset can be created. Using XML to describe an ipset in an nwfilter rule would then look as follows: <rule action='accept' direction='in'> <all ipset='test' ipsetflags='src,src'/> </rule> The two parameters on the command line are also the two distinct XML attributes 'ipset' and 'ipsetflags'. FYI: Here is the man page for ipset: https://ipset.netfilter.org/ipset.man.html Regards, Stefan --- v4: - followed Eric's review comments v3: - use DATATYPE_IPSETNAME for the name of the ipset - adapted documentation for 0.9.12 v2: - split ipset description into ipset and ipsetflags attribute - improved on documentation --- docs/formatnwfilter.html.in | 65 ++++++++++++++ docs/schemas/nwfilter.rng | 23 +++++ src/conf/nwfilter_conf.c | 136 +++++++++++++++++++++++++++++- src/conf/nwfilter_conf.h | 14 ++- src/nwfilter/nwfilter_ebiptables_driver.c | 79 ++++++++++++++++- tests/nwfilterxml2xmlin/ipset-test.xml | 24 +++++ tests/nwfilterxml2xmlout/ipset-test.xml | 24 +++++ tests/nwfilterxml2xmltest.c | 2 8 files changed, 362 insertions(+), 5 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -183,6 +183,8 @@ static const char dstportstart_str[] = " static const char dstportend_str[] = "dstportend"; static const char dscp_str[] = "dscp"; static const char state_str[] = "state"; +static const char ipset_str[] = "ipset"; +static const char ipsetflags_str[] = "ipsetflags"; #define SRCMACADDR srcmacaddr_str #define SRCMACMASK srcmacmask_str @@ -206,6 +208,8 @@ static const char state_str[] = " #define DSTPORTEND dstportend_str #define DSCP dscp_str #define STATE state_str +#define IPSET ipset_str +#define IPSETFLAGS ipsetflags_str /** @@ -980,6 +984,97 @@ tcpFlagsFormatter(virBufferPtr buf, return true; } +static bool +ipsetValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *val, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item) +{ + const char *errmsg = NULL; + + if (virStrcpy(item->u.ipset.setname, val->c, + sizeof(item->u.ipset.setname)) == NULL) { + errmsg = _("ipset name is too long"); + goto arg_err_exit; + } + + if (item->u.ipset.setname[strspn(item->u.ipset.setname, + VALID_IPSETNAME)] != 0) { + errmsg = _("ipset name contains invalid characters"); + goto arg_err_exit; + } + + return true; + +arg_err_exit: + virNWFilterReportError(VIR_ERR_INVALID_ARG, + "%s", errmsg); + return false; +} + +static bool +ipsetFormatter(virBufferPtr buf, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item) +{ + virBufferAdd(buf, item->u.ipset.setname, -1); + + return true; +} + +static bool +ipsetFlagsValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *val, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, nwItemDesc *item) +{ + const char *errmsg = NULL; + size_t idx = 0; + + item->u.ipset.numFlags = 0; + item->u.ipset.flags = 0; + + errmsg = _("malformed ipset flags"); + + while (item->u.ipset.numFlags < 6) { + if (STRCASEEQLEN(&val->c[idx], "src", 3)) { + item->u.ipset.flags |= (1 << item->u.ipset.numFlags); + } else if (!STRCASEEQLEN(&val->c[idx], "dst", 3)) { + goto arg_err_exit; + } + item->u.ipset.numFlags++; + idx += 3; + if (val->c[idx] != ',') + break; + idx++; + } + + if (val->c[idx] != '\0') + goto arg_err_exit; + + return true; + +arg_err_exit: + virNWFilterReportError(VIR_ERR_INVALID_ARG, + "%s", errmsg); + return false; +} + +static bool +ipsetFlagsFormatter(virBufferPtr buf, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item) +{ + uint8_t ctr; + + for (ctr = 0; ctr < item->u.ipset.numFlags; ctr++) { + if (ctr != 0) + virBufferAddLit(buf, ","); + if ((item->u.ipset.flags & (1 << ctr))) + virBufferAddLit(buf, "src"); + else + virBufferAddLit(buf, "dst"); + } + + return true; +} #define COMMON_MAC_PROPS(STRUCT) \ {\ @@ -1411,6 +1506,20 @@ static const virXMLAttr2Struct ipv6Attri .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataState),\ .validator = stateValidator,\ .formatter = stateFormatter,\ + },\ + {\ + .name = IPSET,\ + .datatype = DATATYPE_IPSETNAME,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataIPSet),\ + .validator = ipsetValidator,\ + .formatter = ipsetFormatter,\ + },\ + {\ + .name = IPSETFLAGS,\ + .datatype = DATATYPE_IPSETFLAGS,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataIPSetFlags),\ + .validator = ipsetFlagsValidator,\ + .formatter = ipsetFlagsFormatter,\ } #define COMMON_PORT_PROPS(STRUCT) \ @@ -1853,6 +1962,8 @@ virNWFilterRuleDetailsParse(xmlNodePtr n break; case DATATYPE_STRING: + case DATATYPE_IPSETFLAGS: + case DATATYPE_IPSETNAME: if (!validator) { /* not supported */ rc = -1; @@ -1964,6 +2075,19 @@ err_exit: goto cleanup; } +static void +virNWFilterRuleDefFixupIPSet(ipHdrDataDefPtr ipHdr) +{ + if (HAS_ENTRY_ITEM(&ipHdr->dataIPSet) && + !HAS_ENTRY_ITEM(&ipHdr->dataIPSetFlags)) { + ipHdr->dataIPSetFlags.flags = NWFILTER_ENTRY_ITEM_FLAG_EXISTS; + ipHdr->dataIPSetFlags.u.ipset.numFlags = 1; + ipHdr->dataIPSetFlags.u.ipset.flags = 1; + } else { + ipHdr->dataIPSet.flags = 0; + ipHdr->dataIPSetFlags.flags = 0; + } +} static void virNWFilterRuleDefFixup(virNWFilterRuleDefPtr rule) @@ -2017,6 +2141,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr); COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataDstIPMask, rule->p.ipHdrFilter.ipHdr.dataDstIPAddr); + virNWFilterRuleDefFixupIPSet(&rule->p.ipHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_IPV6: @@ -2024,6 +2149,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr); COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask, rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr); + virNWFilterRuleDefFixupIPSet(&rule->p.ipv6HdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_ARP: @@ -2047,6 +2173,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.tcpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortEnd, rule->p.tcpHdrFilter.portData.dataSrcPortStart); + virNWFilterRuleDefFixupIPSet(&rule->p.tcpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_UDP: @@ -2065,6 +2192,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.udpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortEnd, rule->p.udpHdrFilter.portData.dataSrcPortStart); + virNWFilterRuleDefFixupIPSet(&rule->p.udpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_UDPLITE: @@ -2077,6 +2205,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.udpliteHdrFilter.ipHdr.dataSrcIPFrom); COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataDstIPTo, rule->p.udpliteHdrFilter.ipHdr.dataDstIPFrom); + virNWFilterRuleDefFixupIPSet(&rule->p.udpliteHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_ESP: @@ -2089,6 +2218,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.espHdrFilter.ipHdr.dataSrcIPFrom); COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataDstIPTo, rule->p.espHdrFilter.ipHdr.dataDstIPFrom); + virNWFilterRuleDefFixupIPSet(&rule->p.espHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_AH: @@ -2101,6 +2231,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.ahHdrFilter.ipHdr.dataSrcIPFrom); COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataDstIPTo, rule->p.ahHdrFilter.ipHdr.dataDstIPFrom); + virNWFilterRuleDefFixupIPSet(&rule->p.ahHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_SCTP: @@ -2119,6 +2250,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.sctpHdrFilter.portData.dataSrcPortStart); COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortEnd, rule->p.sctpHdrFilter.portData.dataSrcPortStart); + virNWFilterRuleDefFixupIPSet(&rule->p.sctpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_ICMP: @@ -2133,6 +2265,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.icmpHdrFilter.ipHdr.dataDstIPFrom); COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPCode, rule->p.icmpHdrFilter.dataICMPType); + virNWFilterRuleDefFixupIPSet(&rule->p.icmpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_ALL: @@ -2156,6 +2289,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.igmpHdrFilter.ipHdr.dataSrcIPFrom); COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataDstIPTo, rule->p.igmpHdrFilter.ipHdr.dataDstIPFrom); + virNWFilterRuleDefFixupIPSet(&rule->p.igmpHdrFilter.ipHdr); break; case VIR_NWFILTER_RULE_PROTOCOL_LAST: @@ -3120,7 +3254,7 @@ virNWFilterRuleDefDetailsFormat(virBuffe virBufferAsprintf(buf, " %s='", att[i].name); - if (att[i].formatter) { + if (att[i].formatter && !(flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { if (!att[i].formatter(buf, def, item)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("formatter for %s %s reported error"), Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -79,6 +79,7 @@ enum virNWFilterEntryItemFlags { # define MAX_COMMENT_LENGTH 256 +# define MAX_IPSET_NAME_LENGTH 32 /* incl. terminating '\0' */ # define HAS_ENTRY_ITEM(data) \ (((data)->flags) & NWFILTER_ENTRY_ITEM_FLAG_EXISTS) @@ -103,8 +104,10 @@ enum attrDatatype { DATATYPE_BOOLEAN = (1 << 12), DATATYPE_UINT32 = (1 << 13), DATATYPE_UINT32_HEX = (1 << 14), + DATATYPE_IPSETNAME = (1 << 15), + DATATYPE_IPSETFLAGS = (1 << 16), - DATATYPE_LAST = (1 << 15), + DATATYPE_LAST = (1 << 17), }; # define NWFILTER_MAC_BGA "01:80:c2:00:00:00" @@ -136,9 +139,16 @@ struct _nwItemDesc { uint8_t mask; uint8_t flags; } tcpFlags; + struct { + char setname[MAX_IPSET_NAME_LENGTH]; + uint8_t numFlags; + uint8_t flags; + } ipset; } u; }; +# define VALID_IPSETNAME \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:-+ " typedef struct _ethHdrDataDef ethHdrDataDef; typedef ethHdrDataDef *ethHdrDataDefPtr; @@ -232,6 +242,8 @@ struct _ipHdrDataDef { nwItemDesc dataState; nwItemDesc dataConnlimitAbove; nwItemDesc dataComment; + nwItemDesc dataIPSet; + nwItemDesc dataIPSetFlags; }; Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -256,10 +256,13 @@ static int _printDataType(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item, - bool asHex) + bool asHex, bool directionIn) { int done; char *data; + uint8_t ctr; + virBuffer vb = VIR_BUFFER_INITIALIZER; + char *flags; if (printVar(vars, buf, bufsize, item, &done) < 0) return -1; @@ -346,6 +349,48 @@ _printDataType(virNWFilterVarCombIterPtr } break; + case DATATYPE_IPSETNAME: + if (virStrcpy(buf, item->u.ipset.setname, bufsize) == NULL) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer to small for ipset name")); + return -1; + } + break; + + case DATATYPE_IPSETFLAGS: + for (ctr = 0; ctr < item->u.ipset.numFlags; ctr++) { + if (ctr != 0) + virBufferAddLit(&vb, ","); + if ((item->u.ipset.flags & (1 << ctr))) { + if (directionIn) + virBufferAddLit(&vb, "dst"); + else + virBufferAddLit(&vb, "src"); + } else { + if (directionIn) + virBufferAddLit(&vb, "src"); + else + virBufferAddLit(&vb, "dst"); + } + } + + if (virBufferError(&vb)) { + virReportOOMError(); + virBufferFreeAndReset(&vb); + return -1; + } + + flags = virBufferContentAndReset(&vb); + + if (snprintf(buf, bufsize, "%s", flags) >= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for IPSETFLAGS type")); + VIR_FREE(flags); + return -1; + } + VIR_FREE(flags); + break; + default: virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Unhandled datatype %x"), item->datatype); @@ -362,16 +407,23 @@ printDataType(virNWFilterVarCombIterPtr char *buf, int bufsize, nwItemDescPtr item) { - return _printDataType(vars, buf, bufsize, item, 0); + return _printDataType(vars, buf, bufsize, item, 0, 0); } +static int +printDataTypeDirection(virNWFilterVarCombIterPtr vars, + char *buf, int bufsize, + nwItemDescPtr item, bool directionIn) +{ + return _printDataType(vars, buf, bufsize, item, 0, directionIn); +} static int printDataTypeAsHex(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item) { - return _printDataType(vars, buf, bufsize, item, 1); + return _printDataType(vars, buf, bufsize, item, 1, 0); } @@ -927,6 +979,7 @@ iptablesHandleIpHdr(virBufferPtr buf, char ipaddr[INET6_ADDRSTRLEN], number[MAX(INT_BUFSIZE_BOUND(uint32_t), INT_BUFSIZE_BOUND(int))]; + char str[MAX_IPSET_NAME_LENGTH]; const char *src = "--source"; const char *dst = "--destination"; const char *srcrange = "--src-range"; @@ -938,6 +991,26 @@ iptablesHandleIpHdr(virBufferPtr buf, dstrange = "--src-range"; } + if (HAS_ENTRY_ITEM(&ipHdr->dataIPSet) && + HAS_ENTRY_ITEM(&ipHdr->dataIPSetFlags)) { + + if (printDataType(vars, + str, sizeof(str), + &ipHdr->dataIPSet) < 0) + goto err_exit; + + virBufferAsprintf(afterStateMatch, + " -m set --match-set \"%s\" ", + str); + + if (printDataTypeDirection(vars, + str, sizeof(str), + &ipHdr->dataIPSetFlags, directionIn) < 0) + goto err_exit; + + virBufferAdd(afterStateMatch, str, -1); + } + if (HAS_ENTRY_ITEM(&ipHdr->dataSrcIPAddr)) { if (printDataType(vars, Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -485,6 +485,14 @@ <ref name="stateflags-type"/> </attribute> </optional> + <optional> + <attribute name="ipset"> + <ref name="ipset-name-type"/> + </attribute> + <attribute name="ipsetflags"> + <ref name="ipset-flags-type"/> + </attribute> + </optional> </interleave> </define> @@ -1060,4 +1068,19 @@ <param name="pattern">((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)/((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)</param> </data> </define> + + <define name='ipset-name-type'> + <choice> + <ref name="variable-name-type"/> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\.:\-\+ ]{1,31}</param> + </data> + </choice> + </define> + + <define name='ipset-flags-type'> + <data type="string"> + <param name="pattern">(src|dst)(,(src|dst)){0,5}</param> + </data> + </define> </grammar> Index: libvirt-acl/tests/nwfilterxml2xmlin/ipset-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/ipset-test.xml @@ -0,0 +1,24 @@ +<filter name='testcase' chain='root'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='accept' direction='out'> + <all ipset='test' ipsetflags='src,dst' /> + </rule> + <rule action='accept' direction='in'> + <all state='NONE' ipset='test' ipsetflags='src,dst' comment='in+NONE'/> + </rule> + <rule action='accept' direction='out'> + <all state='NONE' ipset='test' ipsetflags='src,dst' comment='out+NONE'/> + </rule> + <rule action='accept' direction='in'> + <all ipset='test' ipsetflags='SRC,DST,SRC' /> + </rule> + <rule action='accept' direction='in'> + <all ipset='test:_.-+' ipsetflags='SRC,dSt,SRC' /> + </rule> + <rule action='accept' direction='in'> + <all ipset='$IPSETNAME' ipsetflags='src,dst' /> + </rule> + <rule action='accept' direction='inout'> + <all ipset='$IPSETNAME' ipsetflags='src,dst' comment='inout'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/ipset-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/ipset-test.xml @@ -0,0 +1,24 @@ +<filter name='testcase' chain='root'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='accept' direction='out' priority='500'> + <all ipset='test' ipsetflags='src,dst'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <all state='NONE' ipset='test' ipsetflags='src,dst' comment='in+NONE'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <all state='NONE' ipset='test' ipsetflags='src,dst' comment='out+NONE'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <all ipset='test' ipsetflags='src,dst,src'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <all ipset='test:_.-+' ipsetflags='src,dst,src'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <all ipset='$IPSETNAME' ipsetflags='src,dst'/> + </rule> + <rule action='accept' direction='inout' priority='500'> + <all ipset='$IPSETNAME' ipsetflags='src,dst' comment='inout'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -157,6 +157,8 @@ mymain(void) DO_TEST("iter-test2", false); DO_TEST("iter-test3", false); + DO_TEST("ipset-test", false); + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -528,6 +528,11 @@ <li>IPV6_MASK: IPv6 mask in numbers format (FFFF:FFFF:FC00::) or CIDR mask (0-128)</li> <li>STRING: A string</li> <li>BOOLEAN: 'true', 'yes', '1' or 'false', 'no', '0'</li> + <li>IPSETFLAGS: The source and destination flags of the ipset described + by up to 6 'src' or 'dst' elements selecting features from either + the source or destination part of the packet header; example: + src,src,dst. The number of 'selectors' to provide here depends + on the type of ipset that is referenced.</li> </ul> <p> <br/><br/> @@ -1169,6 +1174,16 @@ <td>STRING</td> <td>TCP-only: format of mask/flags with mask and flags each being a comma separated list of SYN,ACK,URG,PSH,FIN,RST or NONE or ALL</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/> @@ -1269,6 +1284,16 @@ <td>STRING</td> <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/> @@ -1358,6 +1383,16 @@ <td>STRING</td> <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/> @@ -1459,6 +1494,16 @@ <td>STRING</td> <td>TCP-only: format of mask/flags with mask and flags each being a comma separated list of SYN,ACK,URG,PSH,FIN,RST or NONE or ALL</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/> @@ -1545,6 +1590,16 @@ <td>STRING</td> <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/> @@ -1619,6 +1674,16 @@ <td>STRING</td> <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> </tr> + <tr> + <td>ipset <span class="since">(Since 0.9.13)</span></td> + <td>STRING</td> + <td>The name of an IPSet managed outside of libvirt</td> + </tr> + <tr> + <td>ipsetflags <span class="since">(Since 0.9.13)</span></td> + <td>IPSETFLAGS</td> + <td>flags for the IPSet; requires ipset attribute</td> + </tr> </table> <p> <br/><br/>

On 05/14/2012 09:00 PM, Stefan Berger wrote:
This patch adds support for the recent ipset iptables extension to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets' of IP addresses, ports and other packet parameters and allows for faster lookup (in the order of O(1) vs. O(n)) and rule evaluation to achieve higher throughput than what can be achieved with individual iptables rules.
On the command line iptables supports ipset using
iptables ... -m set --match-set <ipset name> <flags> -j ...
where 'ipset name' is the name of a previously created ipset and flags is a comma-separated list of up to 6 flags. Flags use 'src' and 'dst' for selecting IP addresses, ports etc. from the source or destination part of a packet. So a concrete example may look like this:
iptables -A INPUT -m set --match-set test src,src -j ACCEPT
Since ipset management is quite complex, the idea was to leave ipset management outside of libvirt but still allow users to reference an ipset. The user would have to make sure the ipset is available once the VM is started so that the iptables rule(s) referencing the ipset can be created.
I think that at least a subset of it needs to be in libvirt, if only so that the use of ipset makes sense when it isn't available natively on a host. On that topic - could the new XML you're proposing be made useful/applicable with a packet filtering system other than iptables (or, for that matter, on a system with iptables but no ipset?). Or is it specific to ipset? (I haven't thought about it enough to come up with my own answer; I'm just at the stage of questions). I'm concerned that we might add something by definition iptables-specific, and even within that requiring a particular version of iptables, which would then be difficult/impossible to translate to some other filtering system. If there are too many features like that, we can be guaranteed that we'll never see any support other than iptables. (Yes, I know we currently only have a driver for iptables, but it seems like a good idea to not lock us into that. I've got this thought in the back of my head that maybe someday firewalld could (should?) be supported as a separate driver, rather than just using its passthrough mode (which bypasses some of the automatic management it's trying to provide, for better or for worse)) (It's really too bad nobody has written an ipf (or whatever) filter driver for libvirt - that would help keep us honest when adding new features :-) I may be worrying about nothing, as it does seem like a simple addition, but thought I should bring it up anyway, to make sure we go through due diligence.
Using XML to describe an ipset in an nwfilter rule would then look as follows:
<rule action='accept' direction='in'> <all ipset='test' ipsetflags='src,src'/> </rule>
The two parameters on the command line are also the two distinct XML attributes 'ipset' and 'ipsetflags'.
FYI: Here is the man page for ipset:
(That link fails for me unless I change https to http)

On 05/15/2012 11:47 AM, Laine Stump wrote:
Since ipset management is quite complex, the idea was to leave ipset management outside of libvirt but still allow users to reference an ipset. The user would have to make sure the ipset is available once the VM is started so that the iptables rule(s) referencing the ipset can be created.
I think that at least a subset of it needs to be in libvirt, if only so that the use of ipset makes sense when it isn't available natively on a host. On that topic - could the new XML you're proposing be made useful/applicable with a packet filtering system other than iptables (or, for that matter, on a system with iptables but no ipset?). Or is it specific to ipset? (I haven't thought about it enough to come up with my own answer; I'm just at the stage of questions).
I'm concerned that we might add something by definition iptables-specific, and even within that requiring a particular version of iptables, which would then be difficult/impossible to translate to some other filtering system. If there are too many features like that, we can be guaranteed that we'll never see any support other than iptables.
I'm a bit worried about this too. In an IRC chat with Stefan, he mentioned to me the possibility of adding a new virIpsetPtr object (similar to virNwfilterPtr) with operations to construct the set in that manner; and where if the underlying iptables or other firewall solution doesn't support sets natively, then maybe libvirt can still use the virIpsetPtr object to manually expand into one rule per combination of the set. Obviously not as efficient as using the iptables ipset functionality, but that description sounds to me like ipset is a way to optimize existing firewall technology, rather than a new form of filtering. So I think we have room to expand libvirt XML in the future to expose an ipset object, even if we don't support it right away, and in the meantime, the optimizations are worth supporting to make nwfilter faster. So I'm leaning 70-30 towards including this patch even if we don't yet have a way to create ipset objects from within libvirt. Any other opinions?
(Yes, I know we currently only have a driver for iptables, but it seems like a good idea to not lock us into that. I've got this thought in the back of my head that maybe someday firewalld could (should?) be supported as a separate driver, rather than just using its passthrough mode (which bypasses some of the automatic management it's trying to provide, for better or for worse)) (It's really too bad nobody has written an ipf (or whatever) filter driver for libvirt - that would help keep us honest when adding new features :-)
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/14/2012 07:00 PM, Stefan Berger wrote:
This patch adds support for the recent ipset iptables extension to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets' of IP addresses, ports and other packet parameters and allows for faster lookup (in the order of O(1) vs. O(n)) and rule evaluation to achieve higher throughput than what can be achieved with individual iptables rules.
FYI: Here is the man page for ipset:
s/https/http/
+static bool +ipsetValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *val,
Not sure why this line wrapped in my reply, but I don't think it is a problem in the real patch.
+static bool +ipsetFlagsFormatter(virBufferPtr buf, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item) +{ + uint8_t ctr; + + for (ctr = 0; ctr < item->u.ipset.numFlags; ctr++) { + if (ctr != 0) + virBufferAddLit(buf, ",");
I would have used this, but I don't think it makes any difference in speed: virBufferAddChar(buf, ',')
+ case DATATYPE_IPSETFLAGS:
+ + flags = virBufferContentAndReset(&vb); + + if (snprintf(buf, bufsize, "%s", flags) >= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for IPSETFLAGS type"));
Missed an instance of virStrncpy being nicer than snprintf. Other than that, you hit all my review points, so you have my: ACK. If by Tuesday, no one speaks up with a counter-argument against this patch as-is, then I say go ahead and apply with the nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/18/2012 04:19 PM, Eric Blake wrote:
On 05/14/2012 07:00 PM, Stefan Berger wrote:
+ + flags = virBufferContentAndReset(&vb); + + if (snprintf(buf, bufsize, "%s", flags)>= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for IPSETFLAGS type")); Missed an instance of virStrncpy being nicer than snprintf.
Other than that, you hit all my review points, so you have my:
ACK.
If by Tuesday, no one speaks up with a counter-argument against this patch as-is, then I say go ahead and apply with the nits fixed.
I pushed this now. If necessary, we may have to revert it. Stefan
participants (3)
-
Eric Blake
-
Laine Stump
-
Stefan Berger