From 33b10b9112f2f51df049315438204efec7a5434c Mon Sep 17 00:00:00 2001

From: Eric Garver <eric@garver.life>

Date: Tue, 25 Jan 2022 09:29:32 -0500

Subject: [PATCH 51/51] fix(ipset): further reduce cost of entry overlap

 detection

This makes the complexity linear by sorting the networks ahead of time.

Fixes: #881

Fixes: rhbz2043289

(cherry picked from commit 36c170db265265e838a089858be4b20dbbd582eb)

---

 src/firewall/core/ipset.py    | 59 ++++++++++++++++++++++++++++++++---

 src/tests/regression/gh881.at | 42 ++++++++++++++++++++++---

 2 files changed, 92 insertions(+), 9 deletions(-)

diff --git a/src/firewall/core/ipset.py b/src/firewall/core/ipset.py

index 66ea4335536d..b160d8345669 100644

--- a/src/firewall/core/ipset.py

+++ b/src/firewall/core/ipset.py

@@ -327,8 +327,57 @@ def check_for_overlapping_entries(entries):

         # at least one entry can not be parsed

         return

-    while entries:

-        entry = entries.pop()

-        for itr in entries:

-            if entry.overlaps(itr):

-                raise FirewallError(errors.INVALID_ENTRY, "Entry '{}' overlaps entry '{}'".format(entry, itr))

+    # We can take advantage of some facts of IPv4Network/IPv6Network and

+    # how Python sorts the networks to quickly detect overlaps.

+    #

+    # Facts:

+    #

+    #   1. IPv{4,6}Network are normalized to remove host bits, e.g.

+    #     10.1.1.0/16 will become 10.1.0.0/16.

+    #

+    #   2. IPv{4,6}Network objects are sorted by:

+    #     a. IP address (network bits)

+    #   then

+    #     b. netmask (significant bits count)

+    #

+    # Because of the above we have these properties:

+    #

+    #   1. big networks (netA) are sorted before smaller networks (netB)

+    #      that overlap the big network (netA)

+    #     - e.g. 10.1.128.0/17 (netA) sorts before 10.1.129.0/24 (netB)

+    #   2. same value addresses (network bits) are grouped together even

+    #      if the number of network bits vary. e.g. /16 vs /24

+    #     - recall that address are normalized to remove host bits

+    #     - e.g. 10.1.128.0/17 (netA) sorts before 10.1.128.0/24 (netC)

+    #   3. non-overlapping networks (netD, netE) are always sorted before or

+    #      after networks that overlap (netB, netC) the current one (netA)

+    #     - e.g. 10.1.128.0/17 (netA) sorts before 10.2.128.0/16 (netD)

+    #     - e.g. 10.1.128.0/17 (netA) sorts after 9.1.128.0/17 (netE)

+    #     - e.g. 9.1.128.0/17 (netE) sorts before 10.1.129.0/24 (netB)

+    #

+    # With this we know the sorted list looks like:

+    #

+    #   list: [ netE, netA, netB, netC, netD ]

+    #

+    #   netE = non-overlapping network

+    #   netA = big network

+    #   netB = smaller network that overlaps netA (subnet)

+    #   netC = smaller network that overlaps netA (subnet)

+    #   netD = non-overlapping network

+    #

+    #   If networks netB and netC exist in the list, they overlap and are

+    #   adjacent to netA.

+    #

+    # Checking for overlaps on a sorted list is thus:

+    #

+    #   1. compare adjacent elements in the list for overlaps

+    #

+    # Recall that we only need to detect a single overlap. We do not need to

+    # detect them all.

+    #

+    entries.sort()

+    prev_network = entries.pop(0)

+    for current_network in entries:

+        if prev_network.overlaps(current_network):

+            raise FirewallError(errors.INVALID_ENTRY, "Entry '{}' overlaps entry '{}'".format(prev_network, current_network))

+        prev_network = current_network

diff --git a/src/tests/regression/gh881.at b/src/tests/regression/gh881.at

index c7326805b555..a5cf7e4eb912 100644

--- a/src/tests/regression/gh881.at

+++ b/src/tests/regression/gh881.at

@@ -5,21 +5,55 @@ dnl build a large ipset

 dnl

 AT_DATA([./deny_cidr], [])

 NS_CHECK([sh -c '

-for I in $(seq 10); do

+for I in $(seq 250); do

   for J in $(seq 250); do

     echo "10.${I}.${J}.0/24" >> ./deny_cidr

   done

 done

 '])

+NS_CHECK([echo "10.254.0.0/16" >> ./deny_cidr])

 dnl verify non-overlapping does not error

 dnl

 FWD_CHECK([--permanent --new-ipset=deny_set --type=hash:net --option=family=inet --option=hashsize=16384 --option=maxelem=20000], 0, [ignore])

-NS_CHECK([time timeout 300 firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore])

+

+dnl still no overlap

+dnl

+AT_DATA([./deny_cidr], [

+9.0.0.0/8

+11.1.0.0/16

+])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore])

 dnl verify overlap detection actually detects an overlap

 dnl

-NS_CHECK([echo "10.1.0.0/16" >> ./deny_cidr])

-NS_CHECK([time timeout 300 firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore])

+AT_DATA([./deny_cidr], [

+10.1.0.0/16

+10.2.0.0/16

+10.250.0.0/16

+])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore])

+

+AT_DATA([./deny_cidr], [

+10.253.0.0/16

+10.253.128.0/17

+])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore])

+

+AT_DATA([./deny_cidr], [

+10.1.1.1/32

+])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore])

+

+AT_DATA([./deny_cidr], [

+10.0.0.0/8

+10.0.0.0/25

+])

+NS_CHECK([time firewall-cmd --permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 136, [ignore], [ignore])

+

+dnl empty file, no additions, but previous ones will remain

+AT_DATA([./deny_cidr], [])

+FWD_CHECK([--permanent --ipset=deny_set --add-entries-from-file=./deny_cidr], 0, [ignore], [ignore])

 FWD_END_TEST()

-- 

2.31.1