Order first by port range and only then by swidth

This change makes the subnets with the most specific port ranges come
before subnets with larger, least specific, port ranges. Before this
change subnets with smaller swidth would always come first and only for
subnets with the same width would the size of the port range be
considered.

Example:
188.0.0.0/8 -x 0.0.0.0/0:443
Before: 188.0.0.0/8 would come first meaning that all ports would be
routed through the VPN for the subnet 188.0.0.0/8
After: 0.0.0.0/0:443 comes first, meaning that port 443 will be
excluded for all subnets, including 188.0.0.0/8. All other ports of
188.0.0.0/8 will be routed.

2 files changed, 36 insertions, 4 deletions

diff --git a/sshuttle/firewall.py b/sshuttle/firewall.py
index 4f37735..b275d08 100644
--- a/sshuttle/firewall.py
+++ b/sshuttle/firewall.py
@@ -75,12 +75,13 @@ def setup_daemon():
 
 
 # Note that we're sorting in a very particular order:
-# we need to go from most-specific (largest swidth) to least-specific,
-# and at any given level of specificity, smaller port ranges come
-# before larger port ranges. On ties excludes come first.
+# we need to go from smaller, more specific, port ranges, to larger,
+# less-specific, port ranges. At each level, we order by subnet
+# width, from most-specific subnets (largest swidth) to
+# least-specific. On ties, excludes come first.
 # s:(inet, subnet width, exclude flag, subnet, first port, last port)
 def subnet_weight(s):
-    return (s[1], s[-2] or -65535 - s[-1], s[2])
+    return (-s[-1] + (s[-2] or -65535), s[1], s[2])
 
 
 # This is some voodoo for setting up the kernel's transparent
diff --git a/sshuttle/tests/client/test_firewall.py b/sshuttle/tests/client/test_firewall.py
index b61a23e..6201601 100644
--- a/sshuttle/tests/client/test_firewall.py
+++ b/sshuttle/tests/client/test_firewall.py
@@ -1,5 +1,6 @@
 from mock import Mock, patch, call
 import io
+import socket
 
 import sshuttle.firewall
 
@@ -58,6 +59,36 @@ def test_rewrite_etc_hosts(tmpdir):
     assert orig_hosts.computehash() == new_hosts.computehash()
 
 
+def test_subnet_weight():
+    subnets = [
+        (socket.AF_INET, 16, 0, '192.168.0.0', 0, 0),
+        (socket.AF_INET, 24, 0, '192.168.69.0', 0, 0),
+        (socket.AF_INET, 32, 0, '192.168.69.70', 0, 0),
+        (socket.AF_INET, 32, 1, '192.168.69.70', 0, 0),
+        (socket.AF_INET, 32, 1, '192.168.69.70', 80, 80),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 0, 0),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 9000),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 8500),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 8000),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 400, 450)
+    ]
+    subnets_sorted = [
+        (socket.AF_INET, 32, 1, '192.168.69.70', 80, 80),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 8000),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 400, 450),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 8500),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 8000, 9000),
+        (socket.AF_INET, 32, 1, '192.168.69.70', 0, 0),
+        (socket.AF_INET, 32, 0, '192.168.69.70', 0, 0),
+        (socket.AF_INET, 24, 0, '192.168.69.0', 0, 0),
+        (socket.AF_INET, 16, 0, '192.168.0.0', 0, 0),
+        (socket.AF_INET, 0, 1, '0.0.0.0', 0, 0)
+    ]
+    
+    assert subnets_sorted == \
+            sorted(subnets, key=sshuttle.firewall.subnet_weight, reverse=True)
+
+
 @patch('sshuttle.firewall.rewrite_etc_hosts')
 @patch('sshuttle.firewall.setup_daemon')
 @patch('sshuttle.firewall.get_method')