Skip to content

Commit

Permalink
vcc_acl_merge parameter: remove subnets and merge adjacent entries
Browse files Browse the repository at this point in the history 
Function:

With the vcc_acl_merge parameter 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.
  • Loading branch information
@nigoroll
nigoroll committed Mar 27, 2021
1 parent bed0e53 commit 7c53c8a
Show file tree
Hide file tree
Showing 5 changed files with 238 additions and 19 deletions.
108 changes: 108 additions & 0 deletions bin/varnishtest/tests/c00005.vtc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions include/tbl/mgt_vcc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 23 additions & 0 deletions include/tbl/params.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 8 additions & 4 deletions include/tbl/vsl_tags.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down
113 changes: 98 additions & 15 deletions lib/libvcc/vcc_acl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,29 +59,36 @@ 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)

#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
Expand All @@ -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;
Expand All @@ -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++;
}
Expand All @@ -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 *
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down

0 comments on commit 7c53c8a

Please sign in to comment.