From 0bf47e1e8e7bac471429e0ec6cae6f9d34f84e12 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Sat, 27 Mar 2021 13:56:25 +0100 Subject: [PATCH] vcc_acl_merge parameter: remove subnets and merge adjacent entries Function: With the merge acl flag enabled (default: disabled, keeping the existing behavior), 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). Skip and merge operations on VCL entries are output as warnings during VCL compilation as entries from the VCL are processed in order. 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". Negated ACL entries are excluded from merges. Implementation: The sort functions are changed such that the previous semantics are preserved: negative return values signify "a < b", positive return values signify "a > b". But additionally the values -2/2 and -3/3 are introduced (and given enums) to signify "contained in supernet" and "directly adjacent to". This allows for mostly unchanged code with vcc_acl_merge disabled. For the "subnet contained in supernet" case, all contained subnets are removed. By sort order, caontained subnets are always to be found left of supernets. For the "merge adjacent" case, the netmask of the entry with the smaller network number is decreased by one and the other entry removed. Because changing the netmask might affect sort order, we reinsert the changed entry. --- bin/varnishtest/tests/c00005.vtc | 105 +++++++++++++++++++++++++++ doc/sphinx/reference/vcl.rst | 21 ++++++ include/tbl/vsl_tags.h | 23 +++--- lib/libvcc/vcc_acl.c | 120 ++++++++++++++++++++++++++----- 4 files changed, 243 insertions(+), 26 deletions(-) diff --git a/bin/varnishtest/tests/c00005.vtc b/bin/varnishtest/tests/c00005.vtc index 255f422324a..04679cfccd0 100644 --- a/bin/varnishtest/tests/c00005.vtc +++ b/bin/varnishtest/tests/c00005.vtc @@ -162,3 +162,108 @@ varnish v1 -errvcl {Non-zero bits in masked part} { if (client.ip ~ acl1) {} } } + +# this is both an OK test for pedantic and merge +varnish v1 -vcl { + import std; + + backend dummy None; + + acl acl1 +log +pedantic +merge { + # 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/doc/sphinx/reference/vcl.rst b/doc/sphinx/reference/vcl.rst index 3513f0eb642..183475465b6 100644 --- a/doc/sphinx/reference/vcl.rst +++ b/doc/sphinx/reference/vcl.rst @@ -292,6 +292,27 @@ individually: However, if the name resolves to both IPv4 and IPv6 you will still get an error. +* `+merge` - Merge ACL supernets and adjacent networks. + + 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). + + Skip and merge operations on VCL entries are output as warnings + during VCL compilation as entries from the VCL are processed in + order. + + 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``. + + Negated ACL entries are never merged. + VCL objects ----------- diff --git a/include/tbl/vsl_tags.h b/include/tbl/vsl_tags.h index 2d3d3cf8656..ccbc1777512 100644 --- a/include/tbl/vsl_tags.h +++ b/include/tbl/vsl_tags.h @@ -289,15 +289,20 @@ SLTM(Fetch_Body, 0, "Body fetched from backend", SLTM(VCL_acl, 0, "VCL ACL check results", "ACLs with the `+log` flag emits this record with the result.\n\n" "The format is::\n\n" - "\t%s %s [%s [fixed: %s]]\n" - "\t| | | |\n" - "\t| | | +- Fixed entry (see vcc_acl_pedantic parameter)\n" - "\t| | +------------ Matching entry (only for MATCH)\n" - "\t| +---------------- Name of the ACL\n" - "\t+-------------------- MATCH or NO_MATCH\n" - "\n" - "MATCH denotes an ACL match\n" - "NO_MATCH denotes that a checked ACL has not matched\n" + "\t%s [%s [%s [fixed: %s]]]\n" + "\t| | | |\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" + "* 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 5ec82105766..182133ed0d6 100644 --- a/lib/libvcc/vcc_acl.c +++ b/lib/libvcc/vcc_acl.c @@ -53,6 +53,7 @@ struct acl { #define VCC_ACL_MAGIC 0xb9fb3cd0 int flag_log; + int flag_merge; int flag_pedantic; int flag_table; @@ -74,19 +75,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) @@ -94,9 +102,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 @@ -109,7 +117,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; @@ -125,7 +133,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++; } @@ -135,14 +143,14 @@ 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); } static int @@ -160,20 +168,22 @@ vcl_acl_disjoint(const struct acl_e *ae1, const struct acl_e *ae2) if (ae2->mask < m) m = ae2->mask; for (; m >= 8; m -= 8) { - CMP(*p1, *p2); + CMPA(*p1, *p2); p1++; p2++; } if (m) { m = 0xff00 >> m; m &= 0xff; - CMP(*p1 & m, *p2 & m); + CMPA(*p1 & m, *p2 & m); } return (0); } 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_NEXT(acl_tree, acl_e, branch, static) VRBT_GENERATE_PREV(acl_tree, acl_e, branch, static) @@ -232,10 +242,66 @@ vcc_acl_chk(struct vcc *tl, const struct acl_e *ae, const int l, return (r); } +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->acl_tree, *l); + FREE_OBJ(*l); + *l = VRBT_PREV(acl_tree, &tl->acl->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->acl_tree, *r); + FREE_OBJ(*r); + VRBT_REMOVE(acl_tree, &tl->acl->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->acl_tree, *aenp); @@ -248,7 +314,24 @@ vcc_acl_insert_entry(struct vcc *tl, struct acl_e **aenp) } return; } + + r = *aenp; *aenp = NULL; + + if (tl->acl->flag_merge == 0) + return; + + l = VRBT_PREV(acl_tree, &tl->acl->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->acl_tree, l); + if (r == NULL) + return; + vcl_acl_merge(tl, &l, &r); } static void @@ -752,6 +835,9 @@ vcc_ParseAcl(struct vcc *tl) if (vcc_IdIs(tl->t, "log")) { acl->flag_log = sign; vcc_NextToken(tl); + } else if (vcc_IdIs(tl->t, "merge")) { + acl->flag_merge = sign; + vcc_NextToken(tl); } else if (vcc_IdIs(tl->t, "pedantic")) { acl->flag_pedantic = sign; vcc_NextToken(tl);