Created attachment 191739 [details] Support "return" statements in passing rules when they fail. Normally pf rules are expected to do one of two things: pass the traffic or block it. Blocking can be silent - "drop", or loud - "return", "return-rst", "return-icmp". Yet there is a 3rd category of traffic passing through pf. Packets matching a "pass" rule but when applying the rule fails. This happens when redirection table is empty or when src node or state creation fails. Such rules always fail silently without notifying the sender. Please see proposed patch for adding "return"-like keywords to "pass" rules just as "block" rules do. Other option would be to not change pf.conf's grammar and just make such rules always returning.
Is this compatible with OpenBSD pf? (I doubt we should be adding novel syntax to a mostly unsupported old fork of OpenBSD pf.) If OpenBSD does it, then it may make sense.
I'm sorry but I did not bother to check OpenBSD syntax. Isn't FreeBSD diverted beyond the point of caring about it anyway? There are other ways to handle this without changing rule syntax, but then it would not be tunable per rule: 1. have all "pass" rules always return if they fail 2. add new pf.conf "set" option 3. follow global "set block-policy" option Option 3 is the least invasive one but is not a solution for my particular issue - I want the firewall to silently drop packets when there is no matching rule but be verbose when a rule fails. I will prepare a patch for solution 2. That would mean no change in rule syntax, no change in default behaviour and possibility to enable this fix if anybody finds this to be a bug for them too. To be honest doing it this way also means I can easily implement it in my environment. The patch I prepared yesterday would require me to change how rules are generated depending on FreeBSD release and kernel patch level. Single change in pf.conf is way easier to do as I create the resulting pf.conf from multiple files coming from different sources.
We do care, up to a point, so we (1) don't make importing changes any harder than it needs to be and (2) reduce the syntax difference as much as possible. I'm not quite sure I fully understand your use case though. What's the reason you want this change?
The exact situation looks like this: I use PF for loadbalacing with "route-to" target and also as firewall preventing servers in datacenter from accessing the Internet. Each "route-to" rule has a table of targets for loadbalancing ("pool") and this table is controlled by a tool which runs health checks against servers which can serve traffic. If all servers in a pool are not healthy, there is nobody to serve the traffic. Requests to such pool are "sinking" into the firewall, SYNs are never responded to. It works pretty bad for pools serving various APIs because it causes very long waits on clients. There are other reasons for the behaviour, mainly failed state or src-node creation or insertion. One could argue that my situation is very specific but I still consider this a general bug or at least unexpected behaviour: while a "block" rule can be configured to drop or return, a "pass" rule is always expected to pass. Which is not true. And should such situation happen, outcome is not documented and not configurable.
(In reply to vegeta from comment #4) Okay, I think I understand. It certainly makes sense to follow the block policy for this. If we're aiming for symmetry with the block rules, I'd say we need both: follow the block policy unless overruled per-rule by the pass blockspec. While I'm giving you work it'd be awesome to have a test case for this too. CURRENT now has test (based on VIMAGE) for some pf features, and I'd like many more tests.
*if* we're aiming for symmetry with block rules. I am unsure if we really should. I usually tend to initially create very universal and highly configurable solutions which break all compatibility only to learn that nobody, even me included, needs such thing. Maybe we can agree on a simpler thing, that is adding "set pass-fail-policy [drop|return]"? That would be a bit more configurable than following block-policy but less invasive than per-rule setting.
I was not aware that there are tests for pf. I am still mostly running FreeBSD 10 on my loadbalancers due to amount of custom patches so I am a bit behind the schedule. I will have a look on them nevertheless.
(In reply to vegeta from comment #6) Okay, let's do that. The tests are fairly new. They're only in 12, so those won't get MFCd back to 11 and 10 because they require VIMAGE. Let's start with the 'set pass-fail-policy' change. The tests can be a separate commit (that'll make the MFC easier too).
This is a new feature rather than a bug fix. The way to implement this is with an extra word to reply-to/route-to in the terms of block-return empty or some-such. This is not a very hard addition but i would recommend the userland application doing health checks perform the FIN sending as well by reading the state table. It feels more clean rather than hacking pf to do this.
Any rule can fail like this, not only route-to rules, so it is not specific to them. And I'm taking about responding with RST/ICMP to new connections when redirection table is already empty. Injecting RSTs during killing of existing connections I already have written and it is done using new sysctls, so I always assumed that it would be too much to include in upstream code. Let's not get into that in this bug report, I will be of course happy to share code (it is on GitHub in fact), just email me if you want to discuss it.
I misread the issue you are experiencing. I do not see any impact on this apart of either - overloading the set block-policy global to express the global policy. pf already marks as dropped the packets that go through failure paths. - Introduce a new global policy like set failure-return-policy. In both scenarios the underlying implementation does not differ. I am not sure on the added value of having it be controllable per-rule!
I think I have a final patch. Configuration of behaviour is global via `set fail-policy` but in fact it is assigned as a flag to each rule. So it could be modified to be done per-rule if somebody would ever wish for that.
Created attachment 194089 [details] fail-policy.diff
(In reply to vegeta from comment #14) Thanks for the patch. I think it looks good, but I've got one question. I see that you removed the (r->rule_flag & PFRULE_RETURNRST) || (r->rule_flag & PFRULE_RETURN)) condition from the IPPROTO_TCP case. I think that might result in a subtle behaviour change for rules which have PFRULE_RETURNICMP set (but not one of the other returns). (I.e. it'd return a TCP RST, where it didn't use to do so. Am I missing something, or should those checks be kept?
That is true, it forces returning RST. I will fix it ASAP.
Created attachment 194340 [details] Reject connection when rule matched but state was not created Now it should correctly distinguish between returning ICMP and RST for TCP connections. Please add the usual information when committing to svn: Submitted by: Kajetan Staszkiewicz <vegeta tuxpowered.net> Sponsored by: InnoGames GmbH
I was way too fast. Now block rules work fine but failed-pass rules are not returning again. Please await another patch.
Created attachment 194357 [details] Reject connection when rule matched but state was not created How about this one? Now there is no extra flag (probably better) and "pass" rules get same set of flags as "block" rules. I'm still testing it but I want your opinion on it anyway.
(In reply to Kajetan Staszkiewicz from comment #19) I'm not sure I understand what the changes in 'action : PASS {' (in parse.y) are for. Other than that, I think it's good.
Without this modification only "block" rules would be configured with return-enabling flag and return ICMP codes. Modification in parse.y ensure that "pass" rules are getting this information too.
A commit references this bug: Author: kp Date: Fri Jun 22 21:59:31 UTC 2018 New revision: 335569 URL: https://svnweb.freebsd.org/changeset/base/335569 Log: pf: Support "return" statements in passing rules when they fail. Normally pf rules are expected to do one of two things: pass the traffic or block it. Blocking can be silent - "drop", or loud - "return", "return-rst", "return-icmp". Yet there is a 3rd category of traffic passing through pf: Packets matching a "pass" rule but when applying the rule fails. This happens when redirection table is empty or when src node or state creation fails. Such rules always fail silently without notifying the sender. Allow users to configure this behaviour too, so that pf returns an error packet in these cases. PR: 226850 Submitted by: Kajetan Staszkiewicz <vegeta tuxpowered.net> MFC after: 1 week Sponsored by: InnoGames GmbH Changes: head/sbin/pfctl/parse.y head/share/man/man5/pf.conf.5 head/sys/netpfil/pf/pf.c
A commit references this bug: Author: kp Date: Fri Jun 29 16:46:20 UTC 2018 New revision: 335798 URL: https://svnweb.freebsd.org/changeset/base/335798 Log: MFC r335569: pf: Support "return" statements in passing rules when they fail. Normally pf rules are expected to do one of two things: pass the traffic or block it. Blocking can be silent - "drop", or loud - "return", "return-rst", "return-icmp". Yet there is a 3rd category of traffic passing through pf: Packets matching a "pass" rule but when applying the rule fails. This happens when redirection table is empty or when src node or state creation fails. Such rules always fail silently without notifying the sender. Allow users to configure this behaviour too, so that pf returns an error packet in these cases. PR: 226850 Submitted by: Kajetan Staszkiewicz <vegeta tuxpowered.net> Sponsored by: InnoGames GmbH Changes: _U stable/11/ stable/11/sbin/pfctl/parse.y stable/11/share/man/man5/pf.conf.5 stable/11/sys/netpfil/pf/pf.c