security groups: Do not use conntrack when it is not required

[phsm]

Description

This PR changes the behavior of Security Groups to disable connection tracking when it is not needed.
The idea is that the VM that have "allow all" rule can have as many connections as they want without straining the host system. This change may be benefitial for VPS hosters, where the VM behavior is not under control of the servers administrator.

The list of changes:
Introduced two new ipsets: cs_notrack for IPv4 and cs_notrack6 for IPv6 that contain the VM IP addresses that do not need to be tracked.
When a security group contains a rule allowing all protocols from 0.0.0.0/0 (IPv4) or ::/0 (IPv6), then all the IPv4 and/or IPv6 addresses of the VM are added to these ipsets.

The following rules are added into iptables table raw chain PREROUTING:

iptables -t raw -A PREROUTING -m set --match-set cs_notrack dst -j NOTRACK
iptables -t raw -A PREROUTING -m set --match-set cs_notrack src -j NOTRACK
ip6tables -t raw -A PREROUTING -m set --match-set cs_notrack6 dst -j NOTRACK
ip6tables -t raw -A PREROUTING -m set --match-set cs_notrack6 src -j NOTRACK

The iptables matchers -m state --state NEW are removed as they are not needed for several reasons:

• they block the allowed traffic if the connection is not tracked
• the rest of the matcher is explicit enough to allow the traffic that was specified in the security group
• the conntrack look up calls can be very expensive on high packet per second rate when the connection tracking table has tens millions of records

The -m state --state ESTABLISHED,RELATED rules are only placed at the end of the VM -def chain, as the last resort rule before the final decision to drop the packet. The goal is to try explicit matchers as much as possible.

The behavior of the -VM chain that contains user-defined rules was modified:

• do not return traffic in the rules, the only possible rule action is ACCEPT. If a packet doesn't match any rules, then it returns back to the -def chain, where it is checked to belong to an existing connection, otherwise dropped.
• the above mentioned -m state NEW are removed.

Since the VM -def chain is populated with rules for each NIC, and there is no place in inject the final unconditional -j DROP in the code, I had to resort to blocking traffic matching each VM network interface in the end of each set of interface-specific rules

A minor refactoring is done:

• The function split_ips_by_family() now takes one or more arguments that can be either a ;-separated string or any other type that can be parsed by Python ipaddress.ip_address() method.
  The function splits ;-separated strings when it encounters them, removes the empty elements and '0' literals (they indicate an empty IP address list for some reason).
  As the result, it returns a tuple containing a list of IPv4 addresses, and IPv6 addresses. Therefore, the function is backwards compatible to the previous behavior.
• Some lines of code that were doing the same functionality as the updated split_ips_by_family(), are removed.
• The function add_to_ipset() uses -! flag that silently ignores addition of a new element if it already exists in the ipset, or its removal if it doesn't exist in the ipset. It will still crash if the requested ipset does not exist. This change makes ipset add calls indempotent.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI
• test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested:

• starting a VM
• changing the rules on the fly: add/remove "allow all" rule, add more specific rules such as allow a specific TCP port range.
• migrating the VM to a host with these changes deployed
• migrating the VM from the host with these changes deployed to a host with "vanilla" security groups script, and back
• both ingress and egress security groups behavior is tested.

How did you try to break this feature and the system with this change?

• Test with only egress traffic allowed (no ingress rules)
• Test with only ingress traffic allowed (only one egress rule allowing traffic to a non-existing IP address, that makes every other egress traffic dropped)
• Test with more-specific rules, e.g. allow specific ports, or allow only IPv6 traffic

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.00%. Comparing base (653b973) to head (f1ff535).

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #10594      +/-   ##
============================================
- Coverage     16.00%   16.00%   -0.01%     
- Complexity    13104    13105       +1     
============================================
  Files          5651     5651              
  Lines        495870   495870              
  Branches      60049    60049              
============================================
- Hits          79370    79361       -9     
- Misses       407638   407652      +14     
+ Partials       8862     8857       -5     

Flag	Coverage Δ
uitests	4.00% <ø> (ø)
unittests	16.84% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wido]

This is really great! I don't have a test env now, but can you confirm that you have tested this in your environment and verified it works?

[phsm]

Well, I tested it somewhat extensively: single ipv4, single ipv4+ipv6, ipv4+ipv6+additional ipv4 and ipv6 ips, multiple NICs, different ipv4+ipv6 rule combinations.. Looks good to me.

However, I wouldn't trust me on this entirely, it is quite hard to not make a mistake with such a complicated script.
I could have missed something. So it would be great if someone could test this extensively too.

[weizhouapache]

@phsm
I think we should not change it

RETURN means rules in other chains will be checked. But they will not be checked if this is changed to ACCEPT

[weizhouapache]

btw: I did not look into the changes. Correct me if I am wrong.

This script is very important for public cloud providers, we have to be very careful

[phsm]

Thanks for scrutinizing my PR, I understand how critical that file is. So the more eyes look at it (and test), the better.
Let me show the examples of the current and proposed security group rules so it becomes more clear.

First, lets have a look how the current rules look like for a single virtual machine with the current security group implementation:

:FORWARD ACCEPT [0:0]
# a set of rules like that for every shared network bridge (VLAN)
# in/out traffic forwarded via this bridge always passes into an individual "per bridge" chain
-A FORWARD -o brbond0-304 -m physdev --physdev-is-bridged -j BF-brbond0-304
-A FORWARD -i brbond0-304 -m physdev --physdev-is-bridged -j BF-brbond0-304
# Everything that was not explicitly accepted or dropped in the "per bridge" chain gets returned here
# and dropped
-A FORWARD -o brbond0-304 -j DROP
-A FORWARD -i brbond0-304 -j DROP

:BF-brbond0-304 - [0:0]
# The early accept of packets belongign to established and related connections.
# The goal of my changes are to eliminate conntrack as much as possible, therefore my changes do not have this rule
-A BF-brbond0-304 -m state --state RELATED,ESTABLISHED -j ACCEPT
-A BF-brbond0-304 -m physdev --physdev-is-in --physdev-is-bridged -j BF-brbond0-304-IN
-A BF-brbond0-304 -m physdev --physdev-is-out --physdev-is-bridged -j BF-brbond0-304-OUT
-A BF-brbond0-304 -m physdev --physdev-out bond0.304 --physdev-is-bridged -j ACCEPT

:BF-brbond0-304-IN - [0:0]
# rules for other VMs are omitted
-A BF-brbond0-304-IN -m physdev --physdev-in vnet152 --physdev-is-bridged -j i-2-104-def

:BF-brbond0-304-OUT - [0:0]
# rules for other VMs are omitted
-A BF-brbond0-304-OUT -m physdev --physdev-out vnet152 --physdev-is-bridged -j i-2-104-def


# At this point we can safely claim that the traffic concerning a single VM always reaches the VM '-def' chain.
# If anything is returned from the '-def' chain, it traverses back to the previous chains until it gets dropped in the FORWARD chain
# Except the outgoing traffic to the Internet due to an unexpected rule '-A BF-brbond0-304 -m physdev --physdev-out bond0.304 --physdev-is-bridged -j ACCEPT'


:i-2-104-def - [0:0]
-A i-2-104-def -m state --state RELATED,ESTABLISHED -j ACCEPT <- unnecesarry, the same rule exists earlier
-A i-2-104-def -p udp -m physdev --physdev-in vnet152 --physdev-is-bridged -m udp --sport 68 --dport 67 -j ACCEPT
-A i-2-104-def -p udp -m physdev --physdev-out vnet152 --physdev-is-bridged -m udp --sport 67 --dport 68 -j ACCEPT
-A i-2-104-def -p udp -m physdev --physdev-in vnet152 --physdev-is-bridged -m udp --sport 67 -j DROP
-A i-2-104-def -m physdev --physdev-in vnet152 --physdev-is-bridged -m set ! --match-set i-2-104-VM src -j DROP
-A i-2-104-def -m physdev --physdev-out vnet152 --physdev-is-bridged -m set ! --match-set i-2-104-VM dst -j DROP
# This traffic in the following 2 rules gets returned into the previous chains, which I believe triggers the rule
# '-A BF-brbond0-304 -m physdev --physdev-out bond0.304 --physdev-is-bridged -j ACCEPT'
# effectively always allowing the DNS, regardless if the security group denies all the traffic or not.
-A i-2-104-def -p udp -m physdev --physdev-in vnet152 --physdev-is-bridged -m set --match-set i-2-104-VM src -m udp --dport 53 -j RETURN
-A i-2-104-def -p tcp -m physdev --physdev-in vnet152 --physdev-is-bridged -m set --match-set i-2-104-VM src -m tcp --dport 53 -j RETURN
-A i-2-104-def -m physdev --physdev-in vnet152 --physdev-is-bridged -m set --match-set i-2-104-VM src -j i-2-104-VM-eg
-A i-2-104-def -m physdev --physdev-out vnet152 --physdev-is-bridged -j i-2-104-VM

:i-2-104-VM - [0:0]
# the incoming traffic to the VM gets into this chain
# The test VM has only one rule there: accept all protocols from 0.0.0.0/0
-A i-2-104-VM -j ACCEPT
-A i-2-104-VM -j RETURN

:i-2-104-VM-eg - [0:0]
# the outgoing traffic from the VM gets into this chain
# the test VM has no rules there:
-A i-2-104-VM-eg -j ACCEPT

Now, lets look at the rules that my changes produce:

*raw
:PREROUTING ACCEPT [0:0]
# Everything in the cs_notrack ipset gets excluded from conntrack
-A PREROUTING -m set --match-set cs_notrack dst -j NOTRACK
-A PREROUTING -m set --match-set cs_notrack src -j NOTRACK

:FORWARD ACCEPT [0:0]
-A FORWARD -o brbond0-302 -m physdev --physdev-is-bridged -j BF-brbond0-302
-A FORWARD -i brbond0-302 -m physdev --physdev-is-bridged -j BF-brbond0-302
-A FORWARD -o brbond0-302 -j DROP
-A FORWARD -i brbond0-302 -j DROP

:BF-brbond0-302 - [0:0]
-A BF-brbond0-302 -m physdev --physdev-is-in --physdev-is-bridged -j BF-brbond0-302-IN
-A BF-brbond0-302 -m physdev --physdev-is-out --physdev-is-bridged -j BF-brbond0-302-OUT
-A BF-brbond0-302 -m physdev --physdev-out bond0.302 --physdev-is-bridged -j ACCEPT

:BF-brbond0-302-IN - [0:0]
-A BF-brbond0-302-IN -m physdev --physdev-in vnet4 --physdev-is-bridged -j i-2-340-def

:BF-brbond0-302-OUT - [0:0]
-A BF-brbond0-302-OUT -m physdev --physdev-out vnet4 --physdev-is-bridged -j i-2-340-def

:i-2-340-def - [0:0]
-A i-2-340-def -p udp -m physdev --physdev-in vnet4 --physdev-is-bridged -m udp --sport 68 --dport 67 -j ACCEPT
-A i-2-340-def -p udp -m physdev --physdev-out vnet4 --physdev-is-bridged -m udp --sport 67 --dport 68 -j ACCEPT
-A i-2-340-def -p udp -m physdev --physdev-in vnet4 --physdev-is-bridged -m udp --sport 67 -j DROP
-A i-2-340-def -m physdev --physdev-in vnet4 --physdev-is-bridged -m set ! --match-set i-2-340-VM src -j DROP
-A i-2-340-def -m physdev --physdev-out vnet4 --physdev-is-bridged -m set ! --match-set i-2-340-VM dst -j DROP
# In the following 2 rules: instead of passing the traffic all the way back to the BF-brbond0-302 chain to accept it, I accept it here.
-A i-2-340-def -p udp -m physdev --physdev-in vnet4 --physdev-is-bridged -m set --match-set i-2-340-VM src -m udp --dport 53 -j ACCEPT
-A i-2-340-def -p tcp -m physdev --physdev-in vnet4 --physdev-is-bridged -m set --match-set i-2-340-VM src -m tcp --dport 53 -j ACCEPT
-A i-2-340-def -m physdev --physdev-in vnet4 --physdev-is-bridged -m set --match-set i-2-340-VM src -j i-2-340-VM-eg
-A i-2-340-def -m physdev --physdev-out vnet4 --physdev-is-bridged -j i-2-340-VM
# The following 2 rules: the last resort rule before dropping the packet: check the conntrack
-A i-2-340-def -m physdev --physdev-in vnet4 --physdev-is-bridged -m state --state RELATED,ESTABLISHED -j ACCEPT
-A i-2-340-def -m physdev --physdev-out vnet4 --physdev-is-bridged -m state --state RELATED,ESTABLISHED -j ACCEPT
# The following 2 rules: drop any other traffic concerning this vNIC
# this is done to support multi-NIC, as the same set rules will be added below for the other NIC if the VM has it
-A i-2-340-def -m physdev --physdev-in vnet4 --physdev-is-bridged -j DROP
-A i-2-340-def -m physdev --physdev-out vnet4 --physdev-is-bridged -j DROP

My intention to have the logic reworked was to reduce implicit chain returns when you do not know what is going to happen to a packet when it is returned.
So I made the traffic to flow into the VM '-def' chain and never return from it anymore, so it is clear what is happening to a packet by just looking at the '-def' chain.

[weizhouapache]

@phsm
thanks for the reply.
as far as I remember, both ingress rules and egress rules should be checked. this is why I questioned.

[phsm]

Update regarding the missing '-j DROP' rules in the chain FORWARD chain:
I double checked, those drop rules are present in the chain. Seems I just wrongly grep-ed while preparing the listings for the previous post.

Edited the rules listing in the previous to contain these rules.

[weizhouapache]

cc @loth @kriegsmanj