[pve-devel] pve-firewall : iptables V2

[Alexandre DERUMIER]

>>I do not really like the use of a global variable @ruleset to store rules. 

Ok, I'll change that. (I was a bit lazy ;)

>>Also, It is a bit unclear to me how you plan to do updates. There is code like: 
>>
>>if(!iptables_rule_exist($rule)){ 
>>iptables_addrule("-I $rule"); 
>>} 

It's mainly to not add a rule twice,mainly in bridge chains or other parent chains.
But never in a tap chain.


>>but how do you remove rules for removed VMs (or removed network interfaces) then? 

I flush the tap chain, then remove link in bridge chain, then delete the chain

here the code with comments

sub flush_tap_rules_direction {
    my ($iface, $bridge, $direction) = @_;

    my $tapchain = "$iface-$direction";

    if(iptables_chain_exist($tapchain)){
        iptables_addrule("-F $tapchain");   >> flush the chain

        my $physdevdirection = $direction eq 'IN' ? "out":"in";
        my $rule = "$bridge-$direction -m physdev --physdev-$physdevdirection $iface --physdev-is-bridged -j $tapchain";
        if(iptables_rule_exist($rule)){       >> test if the tapchain is linked in the bridge chain
            iptables_addrule("-D $rule");     >> if yes, delete it
        }

        if($direction eq 'OUT'){
            my $rule = "proxmoxfw-INPUT -m physdev --physdev-$physdevdirection $iface -j $tapchain";
            if(iptables_rule_exist($rule)){  >> test if the tapchain is linked in the bridge chain
                iptables_addrule("-D $rule");  >> if yes, delete it
            }
        }

        iptables_addrule("-X $tapchain"); >> finally, delete the chain
    }
}



>>Also, the order of rules inside a ruleset is important, so how do you track ordering changes? 

For the tap rules, host rules, group rules, I rewrite the full chain. So no problem for ordering.

>>IMHO this is really impossible using code like "if(!iptables_rule_exist($rule))" 
I don't use iptables_rule_exist in the tapchain.

inline comments :


sub generate_tap_rules_direction {
    my ($iface, $netid, $rules, $bridge, $direction) = @_;

    my $tapchain = "$iface-$direction";

    iptables_addrule(":$tapchain - [0:0]");  >> create the tap chain

    iptables_addrule("-A $tapchain -m state --state INVALID -j DROP");  >> add default drop to tap  chain
    iptables_addrule("-A $tapchain -m state --state RELATED,ESTABLISHED -j ACCEPT"); >> add established accept

    >> now, process the rules from the VMID.FW file
    if (scalar(@$rules)) {
        foreach my $rule (@$rules) {
            next if $rule->{iface} && $rule->{iface} ne $netid;
            if($rule->{action}  =~ m/^(GROUP-(\S+))$/){
                    $rule->{action} .= "-$direction";
                    #generate empty group rule if don't exist
                    if(!iptables_chain_exist($rule->{action})){
                        generate_group_rules($2);
                    }
            }
            #we go to vmbr-IN if accept in out rules
            $rule->{action} = "$bridge-IN" if $rule->{action} eq 'ACCEPT' && $direction eq 'OUT';
            iptables_generate_rule($tapchain, $rule);
        }
    }
    >> add drop to tap chain
    iptables_addrule("-A $tapchain -j LOG --log-prefix \"$tapchain-dropped: \" --log-level 4");
    iptables_addrule("-A $tapchain -j DROP");

    >>now the tap chain is defined, we need to link it to bridge

    #plug the tap chain to bridge chain
    my $physdevdirection = $direction eq 'IN' ? "out":"in";
    my $rule = "$bridge-$direction -m physdev --physdev-$physdevdirection $iface --physdev-is-bridged -j $tapchain";

    >> test if the tap is already linked to bridge. (If we update the rules for exemple, it's already exist)
    if(!iptables_rule_exist($rule)){
        iptables_addrule("-I $rule");   >> insert the link for tapchain in the bridge chain
    }

    if($direction eq 'OUT'){
        #add tap->host rules
        my $rule = "proxmoxfw-INPUT -m physdev --physdev-$physdevdirection $iface -j $tapchain";

        if(!iptables_rule_exist($rule)){
            iptables_addrule("-A $rule");
        }
    }
}



>>Wouldn't it be easier to always restore the full ruleset? 

Yes, It's not a problem.
But we need to read all the vmid.fw files, check if the tap interface plugged/unplugged on the bridge,...)
In a cluster, I would like to have only vm tap running on the host in the firewall.
So, my main idea was to add hooks in tap_plug/ tap_unplug


It is more clear ?


----- Mail original ----- 

De: "Dietmar Maurer" <dietmar at proxmox.com> 
À: "Alexandre DERUMIER" <aderumier at odiso.com>, pve-devel at pve.proxmox.com 
Envoyé: Jeudi 13 Février 2014 11:49:52 
Objet: RE: [pve-devel] pve-firewall : iptables V2 

> any comments for theses patches ? 

OK, I have now committed your patches, and removed shorewall specific code. 

I do not really like the use of a global variable @ruleset to store rules. 

Also, It is a bit unclear to me how you plan to do updates. There is code like: 

if(!iptables_rule_exist($rule)){ 
iptables_addrule("-I $rule"); 
} 

but how do you remove rules for removed VMs (or removed network interfaces) then? 

Also, the order of rules inside a ruleset is important, so how do you track ordering changes? 
IMHO this is really impossible using code like "if(!iptables_rule_exist($rule))" 

Wouldn't it be easier to always restore the full ruleset?