public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Hanreich <s.hanreich@proxmox.com>
To: Hannes Laimer <h.laimer@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains
Date: Wed, 12 Mar 2025 13:51:27 +0100	[thread overview]
Message-ID: <578b7f6c-376e-4ff0-8a92-99e443e031c0@proxmox.com> (raw)
In-Reply-To: <2e127582-005d-40fd-bf01-0c16586c4ba0@proxmox.com>

On 3/12/25 11:18, Hannes Laimer wrote:
> On 3/4/25 13:24, Stefan Hanreich wrote:
>> default-in is also checking for conntrack status, so we should put this
> 
> I think `default-in` is currently noop'ing[1] ct state invalid, am I
> missing something? I though maybe there's a reason for that, so I
> left it as is, as with the change we'd drop there with invalid ct
> state.

Yes, it is - I think I also remember the reason now for not including
invalid initially. CT has issues with multicast / broadcast traffic,
since it is impossible to know the return IP. We had some issues with
DHCP on bridges with firewall-enabled guests for instance.

There are workarounds / solutions for this (ICMPv6 has explicit
exceptions in the conntrack module for instance, some protocols have
conntrack helpers), but they're not a silver bullet that work for every
conceivable protocol. There were some additional fixes in the kernel for
this in the meanwhile [1].

The old firewall does drop invalid CT traffic going in / out the host
unless the nt_conntrack_allow_invalid setting is set:

  -A PVEFW-HOST-IN -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-HOST-IN -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

  [...]

  -A PVEFW-HOST-OUT -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-HOST-OUT -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

Same for guest traffic:

  -A PVEFW-FORWARD -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-FORWARD -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT


IMO we should explicitly check for CT state invalid everywhere we are
checking CT state, since dropping it at some place but not the other
seems wrong/inconsistent. I'd also definitely default to dropping
invalid traffic everywhere, unless explicitly disabled by the user. I
think it's best for now to stick to what the old firewall is doing? It
should have the same effect in the new firewall, and we don't really
have lots of reports of this behavior causing issues.

For users that *are* encountering issues with this behavior, we have an
escape hatch with nt_conntrack_allow_invalid, which is okay IMO since
that doesn't bypass the firewall (all invalid packets still go through
the ruleset and get either accepted / dropped according to the ruleset).
It only hurts performance for that connection and consumes some CPU cycles.

We need to rework how we are utilizing CT in the near future though. It
would be nice to implement support for notrack [2] in the firewall,
which would help users to handle this more surgically by explicitly
exempting certain connections from conntrack. For EVPN we also need to
revisit CT handling of the firewall. So this is something that's on the
roadmap anyway.


[1]
https://lore.kernel.org/lkml/ZfyeC8mjLnGkqnVT@calendula/t/#m32cb12199c69a77f14786ec17b2df8566f34fe95
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=2441


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


      reply	other threads:[~2025-03-12 12:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-20 15:14 Hannes Laimer
2025-02-20 15:14 ` [pve-devel] [PATCH proxmox-firewall 2/2] firewall: apply `nt_conntrack_allow_invalid` option to host table Hannes Laimer
2025-03-04 12:24 ` [pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains Stefan Hanreich
2025-03-12 10:18   ` Hannes Laimer
2025-03-12 12:51     ` Stefan Hanreich [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=578b7f6c-376e-4ff0-8a92-99e443e031c0@proxmox.com \
    --to=s.hanreich@proxmox.com \
    --cc=h.laimer@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal