[pve-devel] [PATCH firewall 3/5] Create corosync firewall rules independant of localnet

[Fabian Grünbichler]

On Mon, Jul 01, 2019 at 02:36:53PM +0200, Stefan Reiter wrote:
> 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:
> > >       # 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.

I see the following options:

1 drop '-s', adapt commit message
2 move $localnet higher up and preserve old behaviour with '-s'
3 guess some multicast network per ring/link based on configured
  addresses
4 ask Corosync to get multicast network information (every X seconds!)

1/2 are easy and both should not break any existing, working udp setups
(1 just allows more inbound multicast traffic, 2 does not change
anything at all). both could easily be modified to only be active if the
configured transport is udp/multicast.

3 sounds rather brittle, 4 potentially expensive.

for master/Corosync 3.x, 1 or 2 should be fine since udp/multicast is
neither recommended nor really supported (no authentication/encryption
whatsoever).

for stable-5/Corosync 2.x, backporting this patch series with 1 or 2
would mainly affect udpu/unicast setups, and be important for upgrading
to Corosync 3.x/PVE 6.x.

so maybe we could have 1 or 2 always on for stable-5, and 1 or 2 with
condition transport == udp for master?