[pve-devel] [PATCH firewall 3/5] Create corosync firewall rules independant of localnet
Stefan Reiter
s.reiter at proxmox.com
Mon Jul 1 14:36:53 CEST 2019
Thank you for the in-depth review, I'll follow up with a v2 shortly.
Pretty much agree with everything mentioned, one question about this
patch in particular below:
On 7/1/19 2:05 PM, Fabian Grünbichler wrote:
> s/independant/independently
>
> On Mon, Jul 01, 2019 at 10:49:22AM +0200, Stefan Reiter wrote:
>> "localnet" does not necessarily correspond to the correct network for
>> corosync (e.g. corosync rings/link can be run independently from other PVE
>> cluster service networks).
>>
>> This change uses the previously introduced sub 'for_all_corosync_addresses'
>> to iterate through all nodes in a corosync cluster and create rules for
>> all nodes and all their respective ringX_addr's it finds.
>>
>> The rules are created as strict as possible, there is a specific rule
>> for every remote node and every ring/link. Also, communication "between"
>> different links/rings is not allowed, e.g. a remote ring1_addr cannot
>> contact a local ring0_addr, and vice versa.
>>
>> Multicast is always allowed, for backwards compatibility (as was the
>> case before).
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>
> maybe s/create/generate/ ? creating a rule is what the admin does when
> configuring the firewall, generating is what the firewall code does
> given certain input/config ;)
>
>> ---
>> src/PVE/Firewall.pm | 69 +++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 54 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
>> index 3a3bd11..ea714db 100644
>> --- a/src/PVE/Firewall.pm
>> +++ b/src/PVE/Firewall.pm
>> @@ -14,6 +14,7 @@ use Socket qw(AF_INET6 inet_ntop inet_pton);
>> use Storable qw(dclone);
>>
>> use PVE::Cluster;
>> +use PVE::Corosync;
>> use PVE::Exception qw(raise raise_param_exc);
>> use PVE::INotify;
>> use PVE::JSONSchema qw(register_standard_option get_standard_option);
>> @@ -2395,13 +2396,27 @@ sub generate_tap_rules_direction {
>> }
>>
>> sub enable_host_firewall {
>> - my ($ruleset, $hostfw_conf, $cluster_conf, $ipversion) = @_;
>> + my ($ruleset, $hostfw_conf, $cluster_conf, $ipversion, $corosync_conf) = @_;
>>
>> my $options = $hostfw_conf->{options};
>> my $cluster_options = $cluster_conf->{options};
>> my $rules = $hostfw_conf->{rules};
>> my $cluster_rules = $cluster_conf->{rules};
>>
>> + # corosync preparation
>> + my $corosync_rule = "-p udp --dport 5404:5405";
>> + my $corosync_local_addresses = {};
>> + my $local_hostname = PVE::INotify::nodename();
>> + if (%$corosync_conf) {
>
> nit: this is rather uncommon style, we'd usually use either
>
> if ($corosync_conf)
>
> or
>
> if (defined($corosync_conf))
>
> (depending on whether we want to check truthiness or definedness)
>
>> + for_all_corosync_addresses($corosync_conf, sub {
>> + my ($node_name, $node_ip, $node_ipversion, $key) = @_;
>> +
>> + if ($node_ipversion == $ipversion && $node_name eq $local_hostname) {
>
> you check for matching $node_ipversion in all three calls - maybe it
> would make sense to pass that responsibility to the iterator method?
> e.g., something like this:
>
> for_all_corosync_addresses($conf, $ipversion, sub {
>
> }
>
> with 0/undef meaning both versions.
>
>> + $corosync_local_addresses->{$key} = $node_ip;
>> + }
>> + });
>> + }
>> +
>> # host inbound firewall
>> my $chain = "PVEFW-HOST-IN";
>> ruleset_create_chain($ruleset, $chain);
>> @@ -2446,14 +2461,20 @@ sub enable_host_firewall {
>> ruleset_addrule($ruleset, $chain, "$mngmntsrc -p tcp --dport 3128", "-j $accept_action"); # SPICE Proxy
>> ruleset_addrule($ruleset, $chain, "$mngmntsrc -p tcp --dport 22", "-j $accept_action"); # SSH
>>
>> - my $localnet = $cluster_conf->{aliases}->{local_network}->{cidr};
>> - my $localnet_ver = $cluster_conf->{aliases}->{local_network}->{ipversion};
>> + # corosync inbound rules
>> + if (%$corosync_conf) {
>> + # always allow multicast
>> + ruleset_addrule($ruleset, $chain, "-m addrtype --dst-type MULTICAST $corosync_rule", "-j $accept_action");
>
> this is now less strict then before, as it is missing '-s $localnet'
> (for inbound), which is a bit at odds with the commit message ('same as
> before')
Does this matter at all? Because a different option would be to create a
multicast rule similar to how the unicast rules are created with this
series (since the original "-s" relied on localnet, which we explicitly
don't have now). I think that would work here, although explicit host
rules somehow feel slightly misaligned with using multicast in the first
place.
>
>>
>> - # corosync
>> - if ($localnet && ($ipversion == $localnet_ver)) {
>> - my $corosync_rule = "-p udp --dport 5404:5405";
>> - ruleset_addrule($ruleset, $chain, "-s $localnet -d $localnet $corosync_rule", "-j $accept_action");
>> - ruleset_addrule($ruleset, $chain, "-s $localnet -m addrtype --dst-type MULTICAST $corosync_rule", "-j $accept_action");
>> + for_all_corosync_addresses($corosync_conf, sub {
>> + my ($node_name, $node_ip, $node_ipversion, $key) = @_;
>> +
>> + if ($node_ipversion == $ipversion && $node_name ne $local_hostname) {
>> + my $destination = $corosync_local_addresses->{$key};
>> + ruleset_addrule($ruleset, $chain, "-d $destination -s $node_ip $corosync_rule", "-j $accept_action")
>> + if defined($destination); # accept only traffic on same ring
>> + }
>
> nit: I'd move this comment to its own line above the appropriate code,
> and move the allocation into the condition, e.g. like this:
>
> # accept only traffic on same ring
> if (defined(my $destination = $corosync_local_addresses->{$key})) {
> ruleset_addrule($ruleset, $chain, "-d $destination -s $node_ip $corosync_rule", "-j $accept_action")
> }
>
>> + });
>> }
>>
>> # implement input policy
>> @@ -2496,15 +2517,30 @@ sub enable_host_firewall {
>> }
>>
>> # allow standard traffic on cluster network
>> + my $localnet = $cluster_conf->{aliases}->{local_network}->{cidr};
>> + my $localnet_ver = $cluster_conf->{aliases}->{local_network}->{ipversion};
>> +
>> if ($localnet && ($ipversion == $localnet_ver)) {
>> ruleset_addrule($ruleset, $chain, "-d $localnet -p tcp --dport 8006", "-j $accept_action"); # PVE API
>> ruleset_addrule($ruleset, $chain, "-d $localnet -p tcp --dport 22", "-j $accept_action"); # SSH
>> ruleset_addrule($ruleset, $chain, "-d $localnet -p tcp --dport 5900:5999", "-j $accept_action"); # PVE VNC Console
>> ruleset_addrule($ruleset, $chain, "-d $localnet -p tcp --dport 3128", "-j $accept_action"); # SPICE Proxy
>> + }
>>
>> - my $corosync_rule = "-p udp --dport 5404:5405";
>> - ruleset_addrule($ruleset, $chain, "-d $localnet $corosync_rule", "-j $accept_action");
>> + # corosync outbound rules
>> + if (%$corosync_conf) {
>
> same nit as above
>
>> + # always allow multicast
>> ruleset_addrule($ruleset, $chain, "-m addrtype --dst-type MULTICAST $corosync_rule", "-j $accept_action");
>> +
>> + for_all_corosync_addresses($corosync_conf, sub {
>> + my ($node_name, $node_ip, $node_ipversion, $key) = @_;
>> +
>> + if ($node_ipversion == $ipversion && $node_name ne $local_hostname) {
>> + my $source = $corosync_local_addresses->{$key};
>> + ruleset_addrule($ruleset, $chain, "-s $source -d $node_ip $corosync_rule", "-j $accept_action")
>> + if defined($source); # accept only traffic on same ring
>
> same nit as above
>
>> + }
>> + });
>> }
>>
>> # implement output policy
>> @@ -3476,7 +3512,7 @@ sub save_hostfw_conf {
>> }
>>
>> sub compile {
>> - my ($cluster_conf, $hostfw_conf, $vmdata) = @_;
>> + my ($cluster_conf, $hostfw_conf, $vmdata, $corosync_conf) = @_;
>>
>> my $vmfw_configs;
>>
>> @@ -3497,6 +3533,9 @@ sub compile {
>>
>> $hostfw_conf = load_hostfw_conf($cluster_conf, undef) if !$hostfw_conf;
>>
>> + # cfs_update is handled by daemon or API
>> + $corosync_conf = PVE::Cluster::cfs_read_file("corosync.conf") if !$corosync_conf;
>> +
>> $vmdata = read_local_vm_config();
>> $vmfw_configs = read_vm_firewall_configs($cluster_conf, $vmdata, undef);
>> }
>> @@ -3516,8 +3555,8 @@ sub compile {
>>
>> push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet };
>>
>> - my $ruleset = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, 4);
>> - my $rulesetv6 = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, 6);
>> + my $ruleset = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 4);
>> + my $rulesetv6 = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 6);
>> my $ebtables_ruleset = compile_ebtables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata);
>> my $ipset_ruleset = compile_ipsets($cluster_conf, $vmfw_configs, $vmdata);
>>
>> @@ -3525,7 +3564,7 @@ sub compile {
>> }
>>
>> sub compile_iptables_filter {
>> - my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $ipversion) = @_;
>> + my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, $ipversion) = @_;
>>
>> my $ruleset = {};
>>
>> @@ -3555,7 +3594,7 @@ sub compile_iptables_filter {
>> my $hostfw_enable = !(defined($hostfw_options->{enable}) && ($hostfw_options->{enable} == 0));
>>
>> if ($hostfw_enable) {
>> - eval { enable_host_firewall($ruleset, $hostfw_conf, $cluster_conf, $ipversion); };
>> + eval { enable_host_firewall($ruleset, $hostfw_conf, $cluster_conf, $ipversion, $corosync_conf); };
>> warn $@ if $@; # just to be sure - should not happen
>> }
>>
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list