[libvirt] [patch 0/5] nwfilter: add a 'state' attribute to protocols
The following patch series introduces an attribute 'state' for iptables- supported protocols. This gives the user more control over the 'state match' of the underlying ip(6)tables implementation and allows to create filtering rules that are more efficient to evaluate. TCK test cases will be posted later. Each rule containing a state attribute with either one of the values NEW, ESTABLISHED, RELATED, INVALID or NONE, gets one iptables rule created for the direction of the rule. The keyword 'NONE' does the same, but doesn't generate a rule with the 'state match'. If no state attribute is used, a symmetric rule in the incoming and outgoing direction is generated (as was done previously). The patches do the following: - extend the parser and XML generator to parse and create XML with the state attribute - instantiate the state in case of ip(6)tables - extend the nwfilter.rng schema with the state attribute's possible values - add information about the new state attribute to the web docs - add a test case for the XML parser/generator to be run during 'make check' I valgrind'ed this patch and it looks all memory is freed appropriately. Regards, Stefan
The patch below extends the XML parser and generator so that every l3 protocol now can have a state attribute. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/conf/nwfilter_conf.c | 166 +++++++++++++++++++++++++++++++++++++++++++---- src/conf/nwfilter_conf.h | 21 +++++ src/libvirt_private.syms | 1 3 files changed, 175 insertions(+), 13 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -44,6 +44,7 @@ #include "nwfilter_params.h" #include "nwfilter_conf.h" #include "domain_conf.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -157,6 +158,7 @@ static const char srcportend_str[] = " static const char dstportstart_str[] = "dstportstart"; static const char dstportend_str[] = "dstportend"; static const char dscp_str[] = "dscp"; +static const char state_str[] = "state"; #define SRCMACADDR srcmacaddr_str #define SRCMACMASK srcmacmask_str @@ -179,6 +181,7 @@ static const char dscp_str[] = " #define DSTPORTSTART dstportstart_str #define DSTPORTEND dstportend_str #define DSCP dscp_str +#define STATE state_str /** @@ -414,9 +417,11 @@ union data { }; typedef bool (*valueValidator)(enum attrDatatype datatype, union data *valptr, - virNWFilterRuleDefPtr nwf); + virNWFilterRuleDefPtr nwf, + nwItemDesc *item); typedef bool (*valueFormatter)(virBufferPtr buf, - virNWFilterRuleDefPtr nwf); + virNWFilterRuleDefPtr nwf, + nwItemDesc *item); typedef struct _virXMLAttr2Struct virXMLAttr2Struct; struct _virXMLAttr2Struct @@ -441,7 +446,8 @@ static const struct int_map macProtoMap[ static bool checkMacProtocolID(enum attrDatatype datatype, union data *value, - virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED) + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item ATTRIBUTE_UNUSED) { int32_t res = -1; @@ -468,7 +474,8 @@ checkMacProtocolID(enum attrDatatype dat static bool macProtocolIDFormatter(virBufferPtr buf, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { const char *str = NULL; bool asHex = true; @@ -519,7 +526,8 @@ checkValidMask(unsigned char *data, int static bool checkMACMask(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *macMask, - virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED) + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item ATTRIBUTE_UNUSED) { return checkValidMask(macMask->uc, 6); } @@ -545,7 +553,8 @@ static const struct int_map arpOpcodeMap static bool arpOpcodeValidator(enum attrDatatype datatype, union data *value, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { int32_t res = -1; @@ -569,7 +578,8 @@ arpOpcodeValidator(enum attrDatatype dat static bool arpOpcodeFormatter(virBufferPtr buf, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { const char *str = NULL; @@ -604,7 +614,8 @@ static const struct int_map ipProtoMap[] static bool checkIPProtocolID(enum attrDatatype datatype, union data *value, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { int32_t res = -1; @@ -628,7 +639,8 @@ static bool checkIPProtocolID(enum attrD static bool formatIPProtocolID(virBufferPtr buf, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { const char *str = NULL; bool asHex = true; @@ -649,7 +661,8 @@ formatIPProtocolID(virBufferPtr buf, static bool dscpValidator(enum attrDatatype datatype, union data *val, - virNWFilterRuleDefPtr nwf) + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { uint8_t dscp = val->ui; if (dscp > 63) @@ -660,6 +673,128 @@ dscpValidator(enum attrDatatype datatype return 1; } + +static const struct int_map stateMatchMap[] = { + INTMAP_ENTRY(RULE_FLAG_STATE_NEW , "NEW"), + INTMAP_ENTRY(RULE_FLAG_STATE_ESTABLISHED , "ESTABLISHED"), + INTMAP_ENTRY(RULE_FLAG_STATE_RELATED , "RELATED"), + INTMAP_ENTRY(RULE_FLAG_STATE_INVALID , "INVALID"), + INTMAP_ENTRY(RULE_FLAG_STATE_NONE , "NONE"), + INTMAP_ENTRY_LAST, +}; + + +static int +parseStringItems(const struct int_map *int_map, + const char *input, int32_t *flags, char sep) +{ + int rc = 0; + unsigned int i, j; + bool found; + + i = 0; + while (input[i]) { + found = false; + while (c_isspace(input[i]) || input[i] == sep) + i++; + if (!input[i]) + break; + for (j = 0; int_map[j].val; j++) { + if (STRCASEEQLEN(&input[i], int_map[j].val, + strlen(int_map[j].val))) { + *flags |= int_map[j].attr; + i += strlen(int_map[j].val); + found = true; + break; + } + } + if (!found) { + rc = 1; + break; + } + } + return rc; +} + + +static int +printStringItems(virBufferPtr buf, const struct int_map *int_map, + int32_t flags, const char *sep) +{ + unsigned int i, c = 0; + int32_t last_attr = 0; + + for (i = 0; int_map[i].val; i++) { + if (last_attr != int_map[i].attr && + flags & int_map[i].attr) { + if (c >= 1) + virBufferVSprintf(buf, "%s", sep); + virBufferVSprintf(buf, "%s", int_map[i].val); + c++; + } + last_attr = int_map[i].attr; + } + + return 0; +} + + +static int +parseStateMatch(const char *statematch, int32_t *flags) +{ + int rc = parseStringItems(stateMatchMap, statematch, flags, ','); + + if ((*flags & RULE_FLAG_STATE_NONE)) + *flags = RULE_FLAG_STATE_NONE; + + return rc; +} + + +void +virNWFilterPrintStateMatchFlags(virBufferPtr buf, const char *prefix, + int32_t flags, bool disp_none) +{ + if (!disp_none && (flags & RULE_FLAG_STATE_NONE)) + return; + + virBufferVSprintf(buf, "%s", prefix); + + printStringItems(buf, stateMatchMap, flags, ","); +} + + +static bool +stateValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *val, + virNWFilterRuleDefPtr nwf, + nwItemDesc *item) +{ + char *input = val->c; + int32_t flags = 0; + + if (parseStateMatch(input, &flags)) + return 0; + + item->u.u16 = flags; + nwf->flags |= flags; + + item->datatype = DATATYPE_UINT16; + + return 1; +} + + +static bool +stateFormatter(virBufferPtr buf, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item) +{ + virNWFilterPrintStateMatchFlags(buf, "", item->u.u16, true); + + return true; +} + + #define COMMON_MAC_PROPS(STRUCT) \ {\ .name = SRCMACADDR,\ @@ -926,6 +1061,13 @@ static const virXMLAttr2Struct ipv6Attri .name = "connlimit-above",\ .datatype = DATATYPE_UINT16,\ .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataConnlimitAbove),\ + },\ + {\ + .name = STATE,\ + .datatype = DATATYPE_STRING,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataState),\ + .validator = stateValidator,\ + .formatter = stateFormatter,\ } #define COMMON_PORT_PROPS(STRUCT) \ @@ -1422,7 +1564,7 @@ virNWFilterRuleDetailsParse(xmlNodePtr n *flags = NWFILTER_ENTRY_ITEM_FLAG_EXISTS | flags_set; item->datatype = datatype >> 1; if (validator) { - if (!validator(datatype >> 1, &data, nwf)) { + if (!validator(datatype >> 1, &data, nwf, item)) { rc = -1; *flags = 0; } @@ -2533,7 +2675,7 @@ virNWFilterRuleDefDetailsFormat(virBuffe virBufferVSprintf(buf, " %s='", att[i].name); if (att[i].formatter) { - if (!att[i].formatter(buf, def)) { + if (!att[i].formatter(buf, def, item)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("formatter for %s %s reported error"), type, Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -28,11 +28,14 @@ # include <stdint.h> # include <stddef.h> +# include <stdbool.h> # include "internal.h" + # include "util.h" # include "hash.h" # include "xml.h" +# include "buf.h" # include "network.h" /* XXX @@ -179,6 +182,7 @@ struct _ipHdrDataDef { nwItemDesc dataDstIPFrom; nwItemDesc dataDstIPTo; nwItemDesc dataDSCP; + nwItemDesc dataState; nwItemDesc dataConnlimitAbove; nwItemDesc dataComment; }; @@ -353,10 +357,25 @@ enum virNWFilterEbtablesTableType { # define MAX_RULE_PRIORITY 1000 enum virNWFilterRuleFlags { - RULE_FLAG_NO_STATEMATCH = (1 << 0), + RULE_FLAG_NO_STATEMATCH = (1 << 0), + RULE_FLAG_STATE_NEW = (1 << 1), + RULE_FLAG_STATE_ESTABLISHED = (1 << 2), + RULE_FLAG_STATE_RELATED = (1 << 3), + RULE_FLAG_STATE_INVALID = (1 << 4), + RULE_FLAG_STATE_NONE = (1 << 5), }; +# define IPTABLES_STATE_FLAGS \ + (RULE_FLAG_STATE_NEW | \ + RULE_FLAG_STATE_ESTABLISHED | \ + RULE_FLAG_STATE_RELATED | \ + RULE_FLAG_STATE_INVALID | \ + RULE_FLAG_STATE_NONE) + +void virNWFilterPrintStateMatchFlags(virBufferPtr buf, const char *prefix, + int32_t flags, bool disp_none); + typedef struct _virNWFilterRuleDef virNWFilterRuleDef; typedef virNWFilterRuleDef *virNWFilterRuleDefPtr; struct _virNWFilterRuleDef { Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -534,6 +534,7 @@ virNWFilterConfLayerInit; virNWFilterConfLayerShutdown; virNWFilterLockFilterUpdates; virNWFilterUnlockFilterUpdates; +virNWFilterPrintStateMatchFlags; # nwfilter_params.h
On Fri, Oct 01, 2010 at 08:28:50PM -0400, Stefan Berger wrote:
The patch below extends the XML parser and generator so that every l3 protocol now can have a state attribute.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
Hum, I don't see that much parsing in there, certainly some XML formatter but the patch loos fine to me, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/06/2010 11:38 AM, Daniel Veillard wrote:
The patch below extends the XML parser and generator so that every l3 protocol now can have a state attribute.
Signed-off-by: Stefan Berger<stefanb@us.ibm.com> Hum, I don't see that much parsing in there, certainly some XML
On Fri, Oct 01, 2010 at 08:28:50PM -0400, Stefan Berger wrote: formatter but the patch loos fine to me,
ACK,
Pushed this and the rest of the series. Stefan
In this patch I am extending the rule instantiator to create the state match according to the state attribute in the XML. Only one iptables rule in the incoming or outgoing direction will be created for a rule in direction 'in' or 'out' respectively. A rule in direction 'inout' does get iptables rules in both directions. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 163 +++++++++++++++++++++++++++++- 1 file changed, 158 insertions(+), 5 deletions(-) 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 @@ -1129,7 +1129,7 @@ _iptablesCreateRuleInstance(int directio const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - const char *match, + const char *match, bool defMatch, const char *accept_target, bool isIPv6, bool maySkipICMP) @@ -1488,7 +1488,7 @@ _iptablesCreateRuleInstance(int directio target = accept_target; else { target = "DROP"; - skipMatch = true; + skipMatch = defMatch; } if (match && !skipMatch) @@ -1548,6 +1548,149 @@ exit_no_error: static int +printStateMatchFlags(int32_t flags, char **bufptr) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virNWFilterPrintStateMatchFlags(&buf, + "-m state --state ", + flags, + false); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return 1; + } + *bufptr = virBufferContentAndReset(&buf); + return 0; +} + +static int +iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, + virNWFilterRuleDefPtr rule, + const char *ifname, + virNWFilterHashTablePtr vars, + virNWFilterRuleInstPtr res, + bool isIPv6) +{ + int rc; + int directionIn = 0, directionOut = 0; + char chainPrefix[2]; + bool maySkipICMP, inout = false; + char *matchState = NULL; + bool create; + + if ((rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN) || + (rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT)) { + directionIn = 1; + directionOut = inout = (rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT); + } else + directionOut = 1; + + chainPrefix[0] = 'F'; + + maySkipICMP = directionIn || inout; + + create = true; + matchState = NULL; + + if (directionIn && !inout) { + if ((rule->flags & IPTABLES_STATE_FLAGS)) + create = false; + } + + if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { + if (printStateMatchFlags(rule->flags, &matchState)) + return 1; + } + + chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + if (create) { + rc = _iptablesCreateRuleInstance(directionIn, + chainPrefix, + nwfilter, + rule, + ifname, + vars, + res, + matchState, false, + "RETURN", + isIPv6, + maySkipICMP); + + VIR_FREE(matchState); + if (rc) + return rc; + } + + maySkipICMP = !directionIn || inout; + create = true; + + if (!directionIn) { + if ((rule->flags & IPTABLES_STATE_FLAGS)) + create = false; + } + + if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { + if (printStateMatchFlags(rule->flags, &matchState)) + return 1; + } + + chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; + if (create) { + rc = _iptablesCreateRuleInstance(!directionIn, + chainPrefix, + nwfilter, + rule, + ifname, + vars, + res, + matchState, false, + "ACCEPT", + isIPv6, + maySkipICMP); + + VIR_FREE(matchState); + + if (rc) + return rc; + } + + maySkipICMP = directionIn; + + create = true; + + if (directionIn && !inout) { + if ((rule->flags & IPTABLES_STATE_FLAGS)) + create = false; + } else { + if ((rule->flags & IPTABLES_STATE_FLAGS)) { + if (printStateMatchFlags(rule->flags, &matchState)) + return 1; + } + } + + if (create) { + chainPrefix[0] = 'H'; + chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + rc = _iptablesCreateRuleInstance(directionIn, + chainPrefix, + nwfilter, + rule, + ifname, + vars, + res, + matchState, false, + "RETURN", + isIPv6, + maySkipICMP); + VIR_FREE(matchState); + } + + return rc; +} + + +static int iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, @@ -1562,6 +1705,16 @@ iptablesCreateRuleInstance(virNWFilterDe bool maySkipICMP, inout = false; const char *matchState; + if (!(rule->flags & RULE_FLAG_NO_STATEMATCH) && + (rule->flags & IPTABLES_STATE_FLAGS)) { + return iptablesCreateRuleInstanceStateCtrl(nwfilter, + rule, + ifname, + vars, + res, + isIPv6); + } + if ((rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN) || (rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT)) { directionIn = 1; @@ -1590,7 +1743,7 @@ iptablesCreateRuleInstance(virNWFilterDe ifname, vars, res, - matchState, + matchState, true, "RETURN", isIPv6, maySkipICMP); @@ -1612,7 +1765,7 @@ iptablesCreateRuleInstance(virNWFilterDe ifname, vars, res, - matchState, + matchState, true, "ACCEPT", isIPv6, maySkipICMP); @@ -1630,7 +1783,7 @@ iptablesCreateRuleInstance(virNWFilterDe ifname, vars, res, - NULL, + NULL, true, "ACCEPT", isIPv6, maySkipICMP);
On Fri, Oct 01, 2010 at 08:28:51PM -0400, Stefan Berger wrote:
In this patch I am extending the rule instantiator to create the state match according to the state attribute in the XML. Only one iptables rule in the incoming or outgoing direction will be created for a rule in direction 'in' or 'out' respectively. A rule in direction 'inout' does get iptables rules in both directions.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
Looks fine, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This patch adds a test case for testing the XML parser's and instantiator's support of the state attribute. The other test case tests existing capabilities. Both test cases will be used in TCK again. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- tests/nwfilterxml2xmlin/example-1.xml | 24 +++++++++++++++++++++ tests/nwfilterxml2xmlin/example-2.xml | 37 +++++++++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/example-1.xml | 15 +++++++++++++ tests/nwfilterxml2xmlout/example-2.xml | 21 ++++++++++++++++++ tests/nwfilterxml2xmltest.c | 3 ++ 5 files changed, 100 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/example-1.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/example-1.xml @@ -0,0 +1,24 @@ +<filter name='testcase'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + + <!-- allow incoming ssh connections --> + <rule action='accept' direction='in' priority='100'> + <tcp dstportstart='22'/> + </rule> + + <!-- allow incoming ICMP (ping) packets --> + <rule action='accept' direction='in' priority='200'> + <icmp/> + </rule> + + <!-- allow all outgoing traffic --> + <rule action='accept' direction='in' priority='300'> + <all/> + </rule> + + <!-- drop all other traffic --> + <rule action='drop' direction='inout' priority='1000'> + <all/> + </rule> + +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/example-1.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/example-1.xml @@ -0,0 +1,15 @@ +<filter name='testcase' chain='root'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + <rule action='accept' direction='in' priority='100'> + <tcp dstportstart='22'/> + </rule> + <rule action='accept' direction='in' priority='200'> + <icmp/> + </rule> + <rule action='accept' direction='in' priority='300'> + <all/> + </rule> + <rule action='drop' direction='inout' priority='1000'> + <all/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -126,6 +126,9 @@ mymain(int argc, char **argv) DO_TEST("comment-test"); + DO_TEST("example-1"); + DO_TEST("example-2"); + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } Index: libvirt-acl/tests/nwfilterxml2xmlin/example-2.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/example-2.xml @@ -0,0 +1,37 @@ +<filter name='testcase'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + + <!-- VM outgoing: allow all established and related connections --> + <rule action='accept' direction='out' priority='100'> + <all state='ESTABLISHED,RELATED' + comment='out: existing and related (ftp) connections'/> + </rule> + + <!-- VM incoming: allow all established connections --> + <rule action='accept' direction='in' priority='100'> + <all state='ESTABLISHED' + comment='in: existing connections'/> + </rule> + + <!-- allow incoming ssh and ftp traffic --> + <rule action='accept' direction='in' priority='200'> + <tcp dstportstart='21' dstportend='22' state='NEW' + comment='in: ftp and ssh'/> + </rule> + + <!-- allow incoming ICMP (ping) packets --> + <rule action='accept' direction='in' priority='300'> + <icmp state='NEW' comment='in: icmp'/> + </rule> + + <!-- allow outgong DNS lookups --> + <rule action='accept' direction='out' priority='300'> + <udp dstportstart='53' state='NEW' comment='out: DNS lookups'/> + </rule> + + <!-- drop all other traffic --> + <rule action='drop' direction='inout' priority='1000'> + <all comment='inout: drop all non-accepted traffic'/> + </rule> + +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/example-2.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/example-2.xml @@ -0,0 +1,21 @@ +<filter name='testcase' chain='root'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + <rule action='accept' direction='out' priority='100'> + <all state='ESTABLISHED,RELATED' comment='out: existing and related (ftp) connections'/> + </rule> + <rule action='accept' direction='in' priority='100'> + <all state='ESTABLISHED' comment='in: existing connections'/> + </rule> + <rule action='accept' direction='in' priority='200'> + <tcp state='NEW' dstportstart='21' dstportend='22' comment='in: ftp and ssh'/> + </rule> + <rule action='accept' direction='in' priority='300'> + <icmp state='NEW' comment='in: icmp'/> + </rule> + <rule action='accept' direction='out' priority='300'> + <udp state='NEW' dstportstart='53' comment='out: DNS lookups'/> + </rule> + <rule action='drop' direction='inout' priority='1000'> + <all comment='inout: drop all non-accepted traffic'/> + </rule> +</filter>
On Fri, Oct 01, 2010 at 08:28:52PM -0400, Stefan Berger wrote:
This patch adds a test case for testing the XML parser's and instantiator's support of the state attribute. The other test case tests existing capabilities. Both test cases will be used in TCK again.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- tests/nwfilterxml2xmlin/example-1.xml | 24 +++++++++++++++++++++ tests/nwfilterxml2xmlin/example-2.xml | 37 +++++++++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/example-1.xml | 15 +++++++++++++ tests/nwfilterxml2xmlout/example-2.xml | 21 ++++++++++++++++++ tests/nwfilterxml2xmltest.c | 3 ++
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Extend the nwfilter.rng schema to accept state attributes. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- docs/schemas/nwfilter.rng | 11 +++++++++++ 1 file changed, 11 insertions(+) Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -429,6 +429,11 @@ <ref name="uint16range"/> </attribute> </optional> + <optional> + <attribute name="state"> + <ref name="stateflags-type"/> + </attribute> + </optional> </interleave> </define> @@ -860,4 +865,10 @@ <define name='comment-type'> <data type="string"/> </define> + + <define name='stateflags-type'> + <data type="string"> + <param name="pattern">((NEW|ESTABLISHED|RELATED|INVALID)(,(NEW|ESTABLISHED|RELATED|INVALID))*|NONE)</param> + </data> + </define> </grammar>
On Fri, Oct 01, 2010 at 08:28:53PM -0400, Stefan Berger wrote:
Extend the nwfilter.rng schema to accept state attributes.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com> [...] + + <define name='stateflags-type'> + <data type="string"> + <param name="pattern">((NEW|ESTABLISHED|RELATED|INVALID)(,(NEW|ESTABLISHED|RELATED|INVALID))*|NONE)</param> + </data> + </define> </grammar>
Hum, we really want to accept something like NEW,NEW,NEW,NEW ? I understand that we may want to add RELATED to another state, but that regexp could probably be refined, isn't it ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard <veillard@redhat.com> wrote on 10/06/2010 12:00:04 PM:
Re: [libvirt] [patch 4/5] nwfilter: Extend schema to accept state attribute
On Fri, Oct 01, 2010 at 08:28:53PM -0400, Stefan Berger wrote:
Extend the nwfilter.rng schema to accept state attributes.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com> [...] + + <define name='stateflags-type'> + <data type="string"> + <param name="pattern">((NEW|ESTABLISHED|RELATED|INVALID)(, (NEW|ESTABLISHED|RELATED|INVALID))*|NONE)</param> + </data> + </define> </grammar>
Hum, we really want to accept something like NEW,NEW,NEW,NEW ? I understand that we may want to add RELATED to another state, but that regexp could probably be refined, isn't it ?
The only solution that I could come up with is to explicitly enumerate all possible combinations of the above 4 words (15 combinations). This solution would then also force the user to provide them in a particular order: (A)|(B)|(C)|(D)|(A,B)|(A,C)|(A,D)|(B,C)|(B,D)|(C,D)|(A,B,C)|(A,B,D)|(A,C,D)|(B,C,D)|(A,B,C,D) When not forcing the user into a sequence we'd need something like this here: (A)|(B)|(C)|(D)|(A,B)|(A,C)|(A,D)|(B,C)|(B,D)|(C,D)|(A,B,C)|(A,B,D)|(A,C,D)|(B,C,D)|(A,B,C,D)| (B,A)|(C,A),(D,A),(C,B),(D,B),(D,C),(A,C,B)|(A,D,B)|(A,D,C)|(B,D,C)|(A,B,D,C)| ... I think the proposed solution is not as 'strict' as it should be but compared to the other two solutions I think it is 'ok' -- unless there is a fundamentally different way of writing this type of regex. Regards, Stefan
On 10/06/2010 11:18 AM, Stefan Berger wrote:
Hum, we really want to accept something like NEW,NEW,NEW,NEW ? I understand that we may want to add RELATED to another state, but that regexp could probably be refined, isn't it ?
I think the proposed solution is not as 'strict' as it should be but compared to the other two solutions I think it is 'ok' -- unless there is a fundamentally different way of writing this type of regex.
I agree with Stefan's point of view: the .rng should be a syntax check and outlaw completely unrecognized words, but to be concise, it is necessarily loose and requires that we then perform additional semantic checks to reject strings like NEW,NEW. And this is evident in my recent <vcpu cpuset=...> patch, where domain.rng accepts strings like cpuset='4-3' as syntactically valid (a range of integers) that are later rejected by semantic checks (the integer range is backwards). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Oct 06, 2010 at 01:18:47PM -0400, Stefan Berger wrote:
Daniel Veillard <veillard@redhat.com> wrote on 10/06/2010 12:00:04 PM:
Re: [libvirt] [patch 4/5] nwfilter: Extend schema to accept state attribute
On Fri, Oct 01, 2010 at 08:28:53PM -0400, Stefan Berger wrote:
Extend the nwfilter.rng schema to accept state attributes.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com> [...] + + <define name='stateflags-type'> + <data type="string"> + <param name="pattern">((NEW|ESTABLISHED|RELATED|INVALID)(, (NEW|ESTABLISHED|RELATED|INVALID))*|NONE)</param> + </data> + </define> </grammar>
Hum, we really want to accept something like NEW,NEW,NEW,NEW ? I understand that we may want to add RELATED to another state, but that regexp could probably be refined, isn't it ?
The only solution that I could come up with is to explicitly enumerate all possible combinations of the above 4 words (15 combinations). This solution would then also force the user to provide them in a particular order:
(A)|(B)|(C)|(D)|(A,B)|(A,C)|(A,D)|(B,C)|(B,D)|(C,D)|(A,B,C)|(A,B,D)|(A,C,D)|(B,C,D)|(A,B,C,D)
When not forcing the user into a sequence we'd need something like this here:
(A)|(B)|(C)|(D)|(A,B)|(A,C)|(A,D)|(B,C)|(B,D)|(C,D)|(A,B,C)|(A,B,D)|(A,C,D)|(B,C,D)|(A,B,C,D)|
(B,A)|(C,A),(D,A),(C,B),(D,B),(D,C),(A,C,B)|(A,D,B)|(A,D,C)|(B,D,C)|(A,B,D,C)| ...
I think the proposed solution is not as 'strict' as it should be but compared to the other two solutions I think it is 'ok' -- unless there is a fundamentally different way of writing this type of regex.
yeah, that's not possible as far as I know, I was somehow hoping that not all sets made sense ((NEW|ESTABLISHED|RELATED|INVALID)(,(NEW|ESTABLISHED|RELATED|INVALID)){0,3}|NONE) would at bound the regexp, but that's in no way simpler... Let's kee the current version unless someone gets a brilliant idea in the meantime :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
I am adding a row with information about the newly supported state attribute to each of the tables describing supported attributes of protocols. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- docs/formatnwfilter.html.in | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -748,6 +748,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br> @@ -843,6 +848,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br> @@ -927,6 +937,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br> @@ -1018,6 +1033,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br> @@ -1099,6 +1119,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br> @@ -1168,6 +1193,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table> <p> <br><br>
On Fri, Oct 01, 2010 at 08:28:54PM -0400, Stefan Berger wrote:
I am adding a row with information about the newly supported state attribute to each of the tables describing supported attributes of protocols.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- docs/formatnwfilter.html.in | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -748,6 +748,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of NEW,ESTABLISHED,RELATED,INVALID or NONE</td> + </tr> </table>
Hum, I think the doc really ought to describe the semantic of the values, not just the allowed values, so IMHO that need to be refined explaining for example what "ESTABLISHED,RELATED" actually means would be useful :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard <veillard@redhat.com> wrote on 10/06/2010 12:02:39 PM:
Please respond to veillard
On Fri, Oct 01, 2010 at 08:28:54PM -0400, Stefan Berger wrote:
I am adding a row with information about the newly supported state attribute to each of the tables describing supported attributes
ofprotocols.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- docs/formatnwfilter.html.in | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -748,6 +748,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of
NEW,ESTABLISHED,RELATED,INVALID or NONE</td>
+ </tr> </table>
Hum, I think the doc really ought to describe the semantic of the values, not just the allowed values, so IMHO that need to be refined explaining for example what "ESTABLISHED,RELATED" actually means would be useful :-)
I have another patch that I could merge with this one here (or post separately) that shows two solutions for an example filter having the requirement to support an ftp connection's related data connection in ftp active mode. The two solutions make a lot of use of the state attribute and explain a little what it means - not a whole tutorial but the general idea of it. Stefan
On Wed, Oct 06, 2010 at 01:25:35PM -0400, Stefan Berger wrote:
Daniel Veillard <veillard@redhat.com> wrote on 10/06/2010 12:02:39 PM:
Please respond to veillard
On Fri, Oct 01, 2010 at 08:28:54PM -0400, Stefan Berger wrote:
I am adding a row with information about the newly supported state attribute to each of the tables describing supported attributes
ofprotocols.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- docs/formatnwfilter.html.in | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -748,6 +748,11 @@ <td>STRING</td> <td>text with max. 256 characters</td> </tr> + <tr> + <td>state <span class="since">(Since 0.8.5)</span></td> + <td>STRING</td> + <td>comma separated list of
NEW,ESTABLISHED,RELATED,INVALID or NONE</td>
+ </tr> </table>
Hum, I think the doc really ought to describe the semantic of the values, not just the allowed values, so IMHO that need to be refined explaining for example what "ESTABLISHED,RELATED" actually means would be useful :-)
I have another patch that I could merge with this one here (or post separately) that shows two solutions for an example filter having the requirement to support an ftp connection's related data connection in ftp active mode. The two solutions make a lot of use of the state attribute and explain a little what it means - not a whole tutorial but the general idea of it.
That would be really nicer, yes ! Commit and post separately, but I doubt anybody would object to more documentation... at best trying to cleanup. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel Veillard -
Eric Blake -
Stefan Berger -
Stefan Berger