diff --git a/bin/varnishtest/tests/c00005.vtc b/bin/varnishtest/tests/c00005.vtc index 448d6f80cf5..948b6bda809 100644 --- a/bin/varnishtest/tests/c00005.vtc +++ b/bin/varnishtest/tests/c00005.vtc @@ -164,3 +164,111 @@ varnish v1 -errvcl {Address/Netmask mismatch, need be 1.2.3.0/24} { if (client.ip ~ acl1) {} } } + +varnish v1 -cliok "param.set vcc_acl_merge on" + +# this is both an OK test for vcc_acl_pedantic and a +# test for vcc_acl_merge +varnish v1 -vcl { + import std; + + backend dummy None; + + acl acl1 { + # bad notation (confusing) + "1.2.3.0"/24; + "1.2.3.64"/26; + + # all contained in 1.3.0.0/21 and 1.4.4.0/22 + "1.4.4.0"/22; + "1.3.4.0"/23; + "1.3.5.0"/26; + "1.3.6.0"/25; + "1.3.6.128"/25; + "1.3.0.0"/21; + "1.4.7"; + "1.4.6.0"/24; + + # right,left adjacent + "2.3.2.0"/23; + "2.3.0.0"/23; + # left,right adjacent + "2.3.4.0"/23; + "2.3.6.0"/23; + } + + sub vcl_recv { + return (synth(200)); + } + sub t { + if (std.ip(req.http.ip) ~ acl1) { } + } + sub vcl_synth { + # variables would be nice, but not in core (yet?) + set req.http.ip = "1.2.3.0"; call t; + set req.http.ip = "1.2.3.63"; call t; + set req.http.ip = "1.2.3.64"; call t; + + set req.http.ip = "1.3.4.255"; call t; + set req.http.ip = "1.3.5.0"; call t; + set req.http.ip = "1.3.5.255"; call t; + set req.http.ip = "1.3.6.0"; call t; + set req.http.ip = "1.3.6.140"; call t; + set req.http.ip = "1.3.7.255"; call t; + + set req.http.ip = "1.4.5.255"; call t; + set req.http.ip = "1.4.6.64"; call t; + set req.http.ip = "1.4.7.64"; call t; + + set req.http.ip = "2.3.0.0"; call t; + set req.http.ip = "2.3.5.255"; call t; + + set req.http.ip = "2.2.255.255";call t; + set req.http.ip = "2.3.8.0"; call t; +} +} + +logexpect l1 -v v1 -g raw { + expect * 1009 ReqHeader {^\Qip: 1.2.3.0\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.2.3.0"/24\E$} + expect 1 = ReqHeader {^\Qip: 1.2.3.63\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.2.3.0"/24\E$} + expect 1 = ReqHeader {^\Qip: 1.2.3.64\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.2.3.0"/24\E$} + + expect 1 = ReqHeader {^\Qip: 1.3.4.255\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + expect 1 = ReqHeader {^\Qip: 1.3.5.0\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + expect 1 = ReqHeader {^\Qip: 1.3.5.255\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + expect 1 = ReqHeader {^\Qip: 1.3.6.0\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + expect 1 = ReqHeader {^\Qip: 1.3.6.140\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + expect 1 = ReqHeader {^\Qip: 1.3.7.255\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.3.0.0"/21\E$} + + expect 1 = ReqHeader {^\Qip: 1.4.5.255\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.4.4.0"/22\E$} + expect 1 = ReqHeader {^\Qip: 1.4.6.64\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.4.4.0"/22\E$} + expect 1 = ReqHeader {^\Qip: 1.4.7.64\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "1.4.4.0"/22\E$} + + expect 1 = ReqHeader {^\Qip: 2.3.0.0\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "2.3.0.0"/21 fixed: merged\E} + expect 1 = ReqHeader {^\Qip: 2.3.5.255\E$} + expect 0 = VCL_acl {^\QMATCH acl1 "2.3.0.0"/21 fixed: merged\E} + expect 1 = ReqHeader {^\Qip: 2.2.255.255\E$$} + expect 0 = VCL_acl {^\QNO_MATCH acl1\E$} + expect 1 = ReqHeader {^\Qip: 2.3.8.0\E$} + expect 0 = VCL_acl {^\QNO_MATCH acl1\E$} +} -start + +client c1 { + txreq + rxresp +} -run + +logexpect l1 -wait diff --git a/include/tbl/mgt_vcc.h b/include/tbl/mgt_vcc.h index 9f828679a5d..ed0ef3df9b0 100644 --- a/include/tbl/mgt_vcc.h +++ b/include/tbl/mgt_vcc.h @@ -3,6 +3,7 @@ * */ +MGT_VCC(unsigned, acl_merge, Acl_Merge) MGT_VCC(unsigned, acl_pedantic, Acl_Pedantic) MGT_VCC(unsigned, allow_inline_c, Allow_InlineC) MGT_VCC(unsigned, err_unref, Err_Unref) diff --git a/include/tbl/params.h b/include/tbl/params.h index cca420cb8e5..1496de8f9da 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1588,6 +1588,29 @@ PARAM_VCC( "Unreferenced VCL objects result in error." ) +PARAM_VCC( + /* name */ vcc_acl_merge, + /* def */ "off", // REL_20210915 XXX change to on in 7.x ? + /* descr */ + "Merge ACL supernets and adjacent networks.\n\n" + "With this parameter set to on, ACLs are optimized in that " + "subnets contained in other entries are skipped (e.g. " + "if 1.2.3.0/24 is part of the ACL, an entry for 1.2.3.128/25 " + "will not be added) and adjacent entries get merged (e.g. " + "if both 1.2.3.0/25 and 1.2.3.128/25 are added, they will " + "be merged to 1.2.3.0/24).\n" + "Skip and merge operations on VCL entries are output as " + "warnings during VCL compilation as entries from the VCL are " + "processed in order.\n" + "Logging under the VCL_acl tag can change with this parameter " + "enabled: Matches on skipped subnet entries are now logged as " + "matches on the respective supernet entry. Matches on merged " + "entries are logged with a shorter netmask which might not be " + "contained in the original ACL as defined in VCL. Such log " + "entries are marked by \"fixed: merged\".\n" + "Negated ACL entries are never merged." +) + PARAM_VCC( /* name */ vcc_acl_pedantic, /* def */ "off", diff --git a/include/tbl/vsl_tags.h b/include/tbl/vsl_tags.h index c98646d2613..488d5eb44e8 100644 --- a/include/tbl/vsl_tags.h +++ b/include/tbl/vsl_tags.h @@ -291,14 +291,18 @@ SLTM(VCL_acl, 0, "VCL ACL check results", "The format is::\n\n" "\t%s [%s [%s [fixed: %s]]]\n" "\t| | | |\n" - "\t| | | +- Fixed entry (see vcc_acl_pedantic parameter)\n" + "\t| | | +- Fix info (see below)\n" "\t| | +------------ Matching entry (only for MATCH)\n" "\t| +---------------- Name of the ACL for MATCH or NO_MATCH\n" "\t+-------------------- MATCH, NO_MATCH or NO_FAM\n" "\n" - "MATCH denotes an ACL match\n" - "NO_MATCH denotes that a checked ACL has not matched\n" - "NO_FAM denotes a missing address family and should not occur.\n" + "* Fix info: either contains network/mask for non-canonical entries " + "(see the ``vcc_acl_pedantic`` parameter) or ``merged`` for entries " + "which were the result of a merge operation (see the ``vcc_acl_merge`` " + "parameter.\n" + "* ``MATCH`` denotes an ACL match\n" + "* ``NO_MATCH`` denotes that a checked ACL has not matched\n" + "* ``NO_FAM`` denotes a missing address family and should not occur.\n" "\n" ) diff --git a/lib/libvcc/vcc_acl.c b/lib/libvcc/vcc_acl.c index c784c3a9921..7c114e38a4b 100644 --- a/lib/libvcc/vcc_acl.c +++ b/lib/libvcc/vcc_acl.c @@ -59,19 +59,26 @@ struct acl_e { struct token *t_mask; }; +enum acl_cmp_e { + ACL_EQ = 0, + ACL_LT = -1, // a < b + ACL_GT = 1, // b > a + ACL_CONTAINED = -2, // b contains a + ACL_CONTAINS = 2, // a contains b + ACL_LEFT = -3, // a + 1 == b + ACL_RIGHT = 3 // a == b + 1 +}; + +static void vcc_acl_insert_entry(struct vcc *, struct acl_e **); + /* - * Compare two acl rules for ordering - * returns: - * 0 same - * -1/1 strictly less/greater - * -2/2 b contains a / a contains b - * -3/3 a left of b / b left of a + * Compare two acl rules for relation */ #define CMP(n, a, b) \ do { \ if ((a) < (b)) \ - return (-n); \ + return (enum acl_cmp_e)(-n); \ else if ((b) < (a)) \ return (n); \ } while (0) @@ -79,9 +86,9 @@ struct acl_e { #define CMPA(a, b) \ do { \ if ((a) + 1 == (b)) \ - return (-3); \ + return (ACL_LEFT); \ else if ((b) + 1 == (a)) \ - return (3); \ + return (ACL_RIGHT); \ } while (0) static void @@ -94,7 +101,7 @@ vcl_acl_free(struct acl_e **aep) FREE_OBJ(*aep); } -static int +static enum acl_cmp_e vcl_acl_cmp(const struct acl_e *ae1, const struct acl_e *ae2) { const unsigned char *p1, *p2; @@ -110,7 +117,7 @@ vcl_acl_cmp(const struct acl_e *ae1, const struct acl_e *ae2) if (ae2->mask < m) m = ae2->mask; for (; m >= 8; m -= 8) { - CMP(1, *p1, *p2); + CMP(ACL_GT, *p1, *p2); p1++; p2++; } @@ -120,19 +127,22 @@ vcl_acl_cmp(const struct acl_e *ae1, const struct acl_e *ae2) a2 = *p2 >> (8 - m); if (ae1->mask == ae2->mask) CMPA(a1, a2); - CMP(1, a1, a2); + CMP(ACL_GT, a1, a2); } else if (ae1->mask == ae2->mask) { CMPA(*p1, *p2); } /* Long mask is less than short mask */ - CMP(2, ae2->mask, ae1->mask); + CMP(ACL_CONTAINS, ae2->mask, ae1->mask); - return (0); + return (ACL_EQ); } VRBT_GENERATE_INSERT_COLOR(acl_tree, acl_e, branch, static) VRBT_GENERATE_INSERT(acl_tree, acl_e, branch, vcl_acl_cmp, static) +VRBT_GENERATE_REMOVE_COLOR(acl_tree, acl_e, branch, static) +VRBT_GENERATE_REMOVE(acl_tree, acl_e, branch, static) VRBT_GENERATE_MINMAX(acl_tree, acl_e, branch, static) +VRBT_GENERATE_PREV(acl_tree, acl_e, branch, static) VRBT_GENERATE_NEXT(acl_tree, acl_e, branch, static) static char * @@ -190,10 +200,66 @@ vcc_acl_chk(struct vcc *tl, const struct acl_e *ae, const int l, return (strdup(t)); } +static void +vcl_acl_merge(struct vcc *tl, struct acl_e **l, struct acl_e **r) +{ + enum acl_cmp_e cmp; + + AN(l); + AN(r); + CHECK_OBJ_NOTNULL(*l, VCC_ACL_E_MAGIC); + CHECK_OBJ_NOTNULL(*r, VCC_ACL_E_MAGIC); + + if ((*l)->not || (*r)->not) + return; + + cmp = vcl_acl_cmp(*l, *r); + + assert(cmp < 0); + if (cmp == ACL_LT) + return; + + do { + switch (cmp) { + case ACL_CONTAINED: + VSB_cat(tl->sb, "ACL entry:\n"); + vcc_ErrWhere(tl, (*r)->t_addr); + VSB_cat(tl->sb, "supersedes / removes:\n"); + vcc_ErrWhere(tl, (*l)->t_addr); + vcc_Warn(tl); + VRBT_REMOVE(acl_tree, &tl->acl_tree, *l); + FREE_OBJ(*l); + *l = VRBT_PREV(acl_tree, &tl->acl_tree, *r); + break; + case ACL_LEFT: + (*l)->mask--; + (*l)->fixed = "merged"; + VSB_cat(tl->sb, "ACL entry:\n"); + vcc_ErrWhere(tl, (*l)->t_addr); + VSB_cat(tl->sb, "left of:\n"); + vcc_ErrWhere(tl, (*r)->t_addr); + VSB_printf(tl->sb, "removing the latter and expanding " + "mask of the former by one to /%d\n", + (*l)->mask - 8); + vcc_Warn(tl); + VRBT_REMOVE(acl_tree, &tl->acl_tree, *r); + FREE_OBJ(*r); + VRBT_REMOVE(acl_tree, &tl->acl_tree, *l); + vcc_acl_insert_entry(tl, l); + return; + default: + INCOMPL(); + } + if (*l == NULL || *r == NULL) + break; + cmp = vcl_acl_cmp(*l, *r); + } while (cmp != ACL_LT); +} + static void vcc_acl_insert_entry(struct vcc *tl, struct acl_e **aenp) { - struct acl_e *ae2; + struct acl_e *ae2, *l, *r; CHECK_OBJ_NOTNULL(*aenp, VCC_ACL_E_MAGIC); ae2 = VRBT_INSERT(acl_tree, &tl->acl_tree, *aenp); @@ -206,7 +272,24 @@ vcc_acl_insert_entry(struct vcc *tl, struct acl_e **aenp) } return; } + + r = *aenp; *aenp = NULL; + + if (tl->acl_merge == 0) + return; + + l = VRBT_PREV(acl_tree, &tl->acl_tree, r); + if (l != NULL) { + vcl_acl_merge(tl, &l, &r); + } + if (r == NULL) + return; + l = r; + r = VRBT_NEXT(acl_tree, &tl->acl_tree, l); + if (r == NULL) + return; + vcl_acl_merge(tl, &l, &r); } static void