phsm commented on code in PR #10594:
URL: https://github.com/apache/cloudstack/pull/10594#discussion_r2009751873
##########
scripts/vm/network/security_group.py:
##########
@@ -607,12 +622,16 @@ def default_network_rules(vm_name, vm_id, vm_ip, vm_ip6,
vm_mac, vif, brname, se
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 ")
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
