Bug 237973 - pf: implement egress keyword to simplify rules across different hardware
Summary: pf: implement egress keyword to simplify rules across different hardware
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-net (Nobody)
URL: https://man.openbsd.org/pf.conf
Keywords: feature, needs-patch
Depends on:
Blocks:
 
Reported: 2019-05-18 18:37 UTC by Dave Cottlehuber
Modified: 2022-11-12 19:39 UTC (History)
7 users (show)

See Also:
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
routing-monitor.c (1.07 KB, text/plain)
2022-08-01 14:19 UTC, Goran Mekić
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Cottlehuber freebsd_committer freebsd_triage 2019-05-18 18:37:09 UTC
OpenBSD 6.5 has an egress keyword, which I believe is a tag/label assigned to each interface that has a default route defined.

pass in on egress proto tcp from any to any port smtp \
	rdr-to 127.0.0.1 port spamd

[see https://man.openbsd.org/pf.conf for details]

// discussed over falafel at BSDCan.
Comment 1 Kristof Provost freebsd_committer freebsd_triage 2019-05-18 21:09:18 UTC
'egress' isn't strictly a pf keyword. It's just another ifgroup. You could emulate it by adding your egress interfaces to the group already.
OpenBSD add any interface with a default route to that group (as I understand it). If we do that too it'll automatically work with pf.

Look for IFG_EGRESS in openbsd/sys/net. It should be straightforward enough to add this to freebsd as well.
Comment 2 Kristof Provost freebsd_committer freebsd_triage 2019-06-04 11:20:13 UTC
(Reassigned to pf@, because this is not on my short-term todo list.)
Comment 3 tech-lists 2022-04-01 14:56:28 UTC
has egress been added? will egress be added?
Comment 4 Kristof Provost freebsd_committer freebsd_triage 2022-04-01 14:59:07 UTC
(In reply to tech-lists from comment #3)
> has egress been added? 

No.

> will egress be added?

Depends. Are you volunteering?
Comment 5 tech-lists 2022-04-01 15:36:22 UTC
(In reply to Kristof Provost from comment #4)

volunteering for what? coding? I can't code in C, don't know how.

I can volunteer to test a code change though.

But what I was asking here is the status of this issue, which you've answered. 
So, why not close down the PR, which has been open since 2019, and give the reason so that we know? 

Please note that "it's not implemented and you/pf haven't got the time" is fine at least from my viewpoint as a "reason". It allows people who mainly just consume freebsd rather than program it to make a decision based on fact.
Comment 6 Chris Hutchinson 2022-04-01 16:13:12 UTC
(In reply to tech-lists from comment #5)
IMHO there is zero reason to close this pr(1) until,
or unless someone/anyone has implemented the egress
label/keyword/modifier. kp has already indicated he
has no objection. :-)
Comment 7 Kristof Provost freebsd_committer freebsd_triage 2022-04-01 16:45:34 UTC
(In reply to tech-lists from comment #5)
No, that's a bad idea.

The PR currently correctly reflects the state of the feature request. That is, it's been requested but has not been implemented and is not currently on the list of items I plan to work on.

It also contains useful pointers for someone who wants to work on it.

If we close this all that will happen is that someone else will come along with the same feature request, who will open a new ticket, without any of the useful information in this one.
Comment 8 Kristof Provost freebsd_committer freebsd_triage 2022-05-09 08:29:04 UTC
Reassign to net@, because this is a feature request for the network stack, not for pf. See comment #1.
Comment 9 Goran Mekić 2022-07-31 21:41:08 UTC
I have a really rough patch in https://github.com/mekanix/freebsd-src/commit/afeff25d15b5d16b6402b36de8d61ee44d229c5b but it needs polishing. I did it only for adding default route for simplicity, so deleting default route is yet to be done.

I've seen subscribers exists, so maybe I should subscribe with a callback? I'm a network noob so if you know better, please help. I am also very glad to receive links for documentation or code that is explanatory, basically anything that can help me learn how to improve this patch. Thank you!
Comment 10 Zhenlei Huang freebsd_committer freebsd_triage 2022-08-01 02:09:57 UTC
I think it is a little complicated.

1. FreeBSD supports multiple FIBs, different FIB may have different default route. Then how can the `egress` group been set?
2. What if it is a router and have multiple interfaces and ECMP default route?
3. If we have dynamic or static route, maybe another interface will be chosen as real egress interface other than the one with default route. If we rely on PF firewall `egress` rules then it may be a security hole.

So I think it is best to let user add `egress` ifgroup to the interface manually or by scripts.
Comment 11 Goran Mekić 2022-08-01 07:36:13 UTC
(In reply to Zhenlei Huang from comment #10)
It is complex and I just started learning about routing implementation in kernel, so this patch is far from perfect, but let me give some of the answers:

1. Until we have group per FIB and not group per interface, we can't do better, unless we already have groups per FIB?
2. That issue is present on OpenBSD and yet they still have egress. I didn't dive into egress edge cases on that operating system, but I assume they have this problem, too
3. People already can set groups on their interfaces,  so that is covered.

My point is that egress is not universally usable. You can always imagine a case where egress is not actually what you want in your pf.conf. That being said, I would argue that egress implementation helps until you get to complex network setups in which deeper understanding is assumed, hence it's assumed that network administrators responsible for it know how they should configure their pf.conf. In short I think there are more people who can use egress than those who can't, so I still think this is useful (not in current state, of course).
Comment 12 Alexander V. Chernikov freebsd_committer freebsd_triage 2022-08-01 08:04:38 UTC
Some notes
1) it can be implemented in PF only relatively easy - it just need to subscribe to the route notifications for the desired fib/family, just as ND6 do (nd6_subscription_cb).
2) I'm not sure about the logic of the feature w.r.t IPv4/IPv6. Should it be 2 keywords (egress/egress6) and 2 groups?
Comment 13 Goran Mekić 2022-08-01 08:22:33 UTC
(In reply to Alexander V. Chernikov from comment #12)
I'm curious about 1. Does that mean if_addgroup() would be removed altogether and PF would handle egress internally?
Comment 14 Alexander V. Chernikov freebsd_committer freebsd_triage 2022-08-01 08:33:50 UTC
(In reply to Goran Mekić from comment #13)
Depends on the implementation.
The code in the mentioned repository ( https://github.com/mekanix/freebsd-src/commit/afeff25d15b5d16b6402b36de8d61ee44d229c5b ) sets IFG_EGRESS based on the default route presence. I'm not familiar with PF internals to say what is a clear way here - to maintain an additional internal data structure with the list of interfaces with default route, or leverage an already existing? machinery for interface groups.

What I'm saying is that PF can instantiate subscription to any fib it desires and handle default default route changes from that hook.
Comment 15 Goran Mekić 2022-08-01 09:26:00 UTC
(In reply to Alexander V. Chernikov from comment #14)
Continuing on what you wrote, I can see PF implementing something internal, then IPFW doing similar and we end up with a need for common implementation, which got me wondering should existing grouping algorithm be altered so that groups are per FIB? Of course, groups like bridge, lagg and epair, just to name the few, should be present in all FIBs on the corresponding interfaces. I know it's far from trivial to do such a thing, but just thinking about it, with the knowledge I currently (don't) have it sounds like it would be beneficial to many apps if groups were per FIB.

What do you all think?
Comment 16 Alexander V. Chernikov freebsd_committer freebsd_triage 2022-08-01 10:26:56 UTC
(In reply to Goran Mekić from comment #15)
IIRC ipfw doesn't do anything with the groups. It does have interface tracker, which is used to maintain efficient lookup for interface-name tables.

Speaking of the grouping - there are many variants of grouping that may take different options into account. For some (bridge, epair) FIB isn't relevant at all. I'd really prefer to have as little business logic in the kernel as possible and leave it to the applications.
For example, nothing stops someone from writing a small rtsock monitor script that tracks per-fib default route and assigns "egress_fibX" groups to the relevant interfaces.
Comment 17 Goran Mekić 2022-08-01 11:04:20 UTC
(In reply to Alexander V. Chernikov from comment #16)
I didn't know you can subscribe from userspace, too. In that case, yes, my patch is completely wrong. I have to do some research now before I write next (probably also rough) patch. Thank you all for the input and I hope I'll have some results soon.
Comment 18 Goran Mekić 2022-08-01 14:19:27 UTC
Created attachment 235606 [details]
routing-monitor.c

This is as far as I could figure out on myself. I obviously receive rt_msghdr (152 bytes) and something else (96 bytes). First question is "what is 'rest' in this code"? Second question is how do I get interface, fib and destination address (assuming '0.0.0.0' is default route)?
Comment 19 Alexander V. Chernikov freebsd_committer freebsd_triage 2022-08-01 15:15:36 UTC
(In reply to Goran Mekić from comment #18)
The rest is the prefix information, in sockaddr form.

Basically, rtm_addrs contains a bitmask of sockaddrs available, and these remaining bytes are 4/8-bytes aligned 'struct sockaddr_in[6]', representing dst,netmask and gateway. You can look at the example parsing code here: https://github.com/freebsd/freebsd-src/blob/main/sys/net/rtsock.c#L1311

Interface is present in rtm_ifindex (and you can use if_indextoname()). rtsock sockets are bound the the fib of the process at the time of socket creation, so the fib is the process fib. If you need to monitor more than one fib, you can either create multiple processes or create multiple rtsock sockets, using setfib() calls prior to calling socket().
Comment 20 Goran Mekić 2022-08-01 21:57:19 UTC
Initial version: https://github.com/mekanix/egress-monitor

I found in ifconfig (https://github.com/freebsd/freebsd-src/blob/main/sbin/ifconfig/ifgroup.c#L60) that group name should not end in a digit, so I altered the group name a little bit: v4egress and v6egress.

TODO:
  * FIB support
  * rc.d service
  * port/package
Comment 21 dfr 2022-11-11 08:29:23 UTC
This is looking very promising. I would love to see your egress-monitor available in ports - it should make writing portable PF rule fragments for e.g. container networking easier.
Comment 22 Goran Mekić 2022-11-11 11:04:34 UTC
I created a port in https://github.com/mekanix/freebsd-ports/tree/port/egress-monitor but it compiles only if you use root account. If anyone knows how to fix that I would be grateful. For now you can try it out but it's not production ready.
Comment 23 dfr 2022-11-11 16:40:29 UTC
For the root account problem, it looks like bsd.prog.mk always specifies root user. I guess you could workaround by overriding do-install in the port?
Comment 24 Goran Mekić 2022-11-11 17:33:18 UTC
I was missing one USES=uidfix, so that's sorted out, and I did create service file for it, but before I submit the port I want the "initial scan" implemented and that is: get the current default route and set/unset groups of interfaces. That way the service can be started after network or before it.
Comment 25 Goran Mekić 2022-11-12 19:39:26 UTC
I created a port for egress-monitor in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=267731 so please take any discussion about that port to that PR.