security groups: Do not use conntrack when it is not required #10594
Conversation
The conntrack is disabled if the security group allows all traffic. Also, refactored the code a little.
boring-cyborg
bot
added
component:networking
Python
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wido
left a comment
wido
left a comment
There was a problem hiding this comment.
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?
|
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. |
| if vm_ip: | ||
| execute("iptables -A " + vmchain_default + " -m physdev --physdev-is-bridged --physdev-in " + vif + " -m set ! --match-set " + vmipsetName + " src -j DROP") | ||
| execute("iptables -A " + vmchain_default + " -m physdev --physdev-is-bridged --physdev-out " + vif + " -m set ! --match-set " + vmipsetName + " dst -j DROP") | ||
| execute("iptables -A " + vmchain_default + " -m physdev --physdev-is-bridged --physdev-in " + vif + " -m set --match-set " + vmipsetName + " src -p udp --dport 53 -j RETURN ") |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@phsm
thanks for the reply.
as far as I remember, both ingress rules and egress rules should be checked. this is why I questioned.
There was a problem hiding this comment.
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.
|
cc @loth @kriegsmanj |
|
Moving to 4.23 milestone |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16023 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-14977) |
I see the test had failed here. Is there a way for me to see the details? |
this is not a problem with the tests but a capacity problem in the backend lab. I restarted the test job. |
|
[SF] Trillian test result (tid-14995)
|
|
For those who're wiling to test: This script can be placed on one of the nodes in Key points to test:
During the test, running
The outgoing connections from the VM, e.g. "apt update" shall still work |
| if type(i) is str: | ||
| ips += [ip for ip in i.split(';') if ip != '' and ip != '0'] | ||
| else: | ||
| ips.append(str(i)) |
There was a problem hiding this comment.
This is just me, but I would try to check if it's a valid IPv4/IPv6 here prior to adding it to the list. Not check the list afterwards, but that's just me.
If it's valid, add the IP object to the list
There was a problem hiding this comment.
I've rechecked this function.
It has 2 for loops:
The first does the initial parse and splits some of the elements. The result is stored in the intermediate ips list that is not returned back. This is the forloop you're referring to.
The second for loop does the precise sort into lists ip4s and ip6s using python ipaddress library. And those are the lists returned by the function.
Of course I could redo it into a single forloop, but back then I decided to have it more verbose for the readability.
There was a problem hiding this comment.
It's just me how I would code it. I wouldn't put a str() in the list, I would already cast it to an IP object, but that's just me.
You can leave it as-is.
|
This is really great work and much needed for many situations! Now, I would love to test this, but I am currently lacking a testing environment on which I can do so easily. This will take me a couple of weeks before I get a new environment delivered on which I can test. Looking at the code I don't see any major problems to be honest, seems like well thought of and tested. |
|
@wido , that sounds like an LGTM from you and a “let’s trust the submitter’s tests” ..? cc @weizhouapache should we merge as is? |
|
I certainly don't mind waiting for couple more weeks before Wido is able to test it. So, unless there's a rush, we could wait more.. |
|
@DaanHoogland I see no objections in merging it. As it looks like it right now I will not have the new test environment before end of Jan 2026 (hardware deliveries take a long time....) and that would be long. I am OK with merging this based on the code. I know this script really well and these changes are sane. |
|
Whenever you feel like @wido . this will be in main and no new release will be out till half of next year (alternatively it can be rebased on 4.20 or 4.22) |
I am ok with merging it. Code seems good and IF (IF!) there are issues they will be caught during other tests as well |
;) that was an invitation to merge. Any objections @weizhouapache ? |
|
If @weizhouapache agrees this can be merged |
|
@DaanHoogland @wido |
6 participants
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_notrackfor IPv4 andcs_notrack6for 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
rawchainPREROUTING:The iptables matchers
-m state --state NEWare removed as they are not needed for several reasons:The
-m state --state ESTABLISHED,RELATEDrules are only placed at the end of the VM-defchain, 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
-VMchain that contains user-defined rules was modified:-defchain, where it is checked to belong to an existing connection, otherwise dropped.-m state NEWare removed.Since the VM
-defchain is populated with rules for each NIC, and there is no place in inject the final unconditional-j DROPin the code, I had to resort to blocking traffic matching each VM network interface in the end of each set of interface-specific rulesA minor refactoring is done:
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 Pythonipaddress.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.
split_ips_by_family(), are removed.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 makesipset addcalls indempotent.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested:
How did you try to break this feature and the system with this change?