pf: Kill connections by schedule

[kp]

Summary

Mark states with what is essentially a second label to make it easy to
kill them all.
The intended use case is to allow certain hosts access based on a
schedule (for which userspace must add/remove the pass/block rule
according to the desired schedule).
However, this doesn't affect established connections (which could
potentially be long-lived and used to bypass the schedule policy).
The 'schedule' keyword allows these states to be marked, and terminated.

Obtained from: pfSense
MFC after: 4 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

AFAIU, the goal is to distinguish states created by certain rule(s). And in this particular case to kill those states. However, states already reference rules, so internally we can always attribute them. The problem is that pf(4) doesn't have rule numbers and we thus can't specify a rule in pfctl. So this change introduces "rule labels" instead of rule numbers. Btw, why "schedule" and not just "label"? This all makes me think that in future you might want more functionality that would require pointing at particular rules from pfctl or other tool. So, may be this rule specifying functionality needs to be developed in a more generic way, and then provide a control to kill states by rule.

Don't count my opinion as a blocker. If this change needs to be done to faster accomplish some big project, e.g. D29468, please go for it.

[kp]

In D29669#666178, @glebius wrote:

AFAIU, the goal is to distinguish states created by certain rule(s). And in this particular case to kill those states. However, states already reference rules, so internally we can always attribute them. The problem is that pf(4) doesn't have rule numbers and we thus can't specify a rule in pfctl.

pf does have rule numbers (not shown by default by pfctl, but they are there), but they're not actually helpful here.
We want to remove the rule first, and the associated states later (to do it the other way around invites race conditions), and once the rule is removed we can't use its number any more.

So this change introduces "rule labels" instead of rule numbers. Btw, why "schedule" and not just "label"? This all makes me think that in future you might want more functionality that would require pointing at particular rules from pfctl or other tool. So, may be this rule specifying functionality needs to be developed in a more generic way, and then provide a control to kill states by rule.

pfctl already has a 'label' keyword. That label could be used for this feature as well, but we'd end up with a label that's used for two purposes. (And indeed, in pfsense where this change originates the label is used for other purposes than the 'schedule'.)

Don't count my opinion as a blocker. If this change needs to be done to faster accomplish some big project, e.g. D29468, please go for it.

It's not blocking for D29468, no. It's mostly a diff-reduction exercise with pfSense, but this is a standalone useful feature for FreeBSD as well. This is how it's done in pfsense, so my life is easiest to include it unmodified (well, mostly, the nvlist based ioctl is new, but that's not going to make pfsense's life harder), but I'm happy to modify it if we can improve on this.

One of my first thoughts was to allow multiple labels (rather than having a separate 'schedule' and 'label'), but allowing a dynamic number of labels on a rule also complicates things a lot more than simply adding the 'schedule'. (And now that we have nvlist based add/get rule calls adding extra fields is a lot less painful than it used to be.)

[glebius]

What are the downsides of using label for that purpose? Does it have any extra functionality except giving a human readable label? If so, killing states by this label is doable. Trying to imagine a situation where we require first label for just human readable label and second label (schedule) for deletion. Why can't the former be used for deletion as well?

P.S. Again, I'm not blocking the change rather trying to understand.

[kp]

In D29669#667198, @glebius wrote:

What are the downsides of using label for that purpose? Does it have any extra functionality except giving a human readable label? If so, killing states by this label is doable. Trying to imagine a situation where we require first label for just human readable label and second label (schedule) for deletion. Why can't the former be used for deletion as well?

P.S. Again, I'm not blocking the change rather trying to understand.

It's certainly possible to use the label for this, yes, but pfSense has three distinct features for which it wants label-like functionality: it comments the rules with the label, uses the 'schedule' keyword to terminate states corresponding to schedule-restricted hosts and finally has a 'tracker' to match rules across ruleset updates (and persist counters).

Each one individually could just use 'label', but combining all three in a single field isn't really ideal either.

[kp]

I'm starting to come around to the notion of just allowing multiple labels on a rule. If we set a static maximum (e.g. 5) it's not a lot of extra complexity, and we can support pretty much all of the use cases pfSense has.

One missing case is the preservation of counter values pfSense does. See https://github.com/pfsense/FreeBSD-src/commit/7a9c196af4e6d6e7408cac0949c854f2246fcc61.
That patch adds 'tracker', which the pfSense GUI uses to correlate rules across updates.
It also tries to keep counter values using this tracker number (although I dislike the implementation of that part). I'm hoping I can come up with an alternative approach to accomplish the same.

Finally, the 'tracker' is also passed along in pflog output. Doing that with string labels (as opposed to the current tracker number) would be less than ideal, but it appears that this isn't actually used, so perhaps we don't need to worry about it.

[kp]

In D29669#667418, @kp wrote:

Finally, the 'tracker' is also passed along in pflog output. Doing that with string labels (as opposed to the current tracker number) would be less than ideal, but it appears that this isn't actually used, so perhaps we don't need to worry about it.

Correction on this, I was wrong, the tracker value in pflog is actually used.

[eri]

You can add me to these reviews @kp !
I have implemented originally all of this without thinking of generic way of reusability.
The intention is to have some metadata tagged on states and rules to easy perfom operation and/or reporting.
The model can be improved by having a better tagging model then just stash new fields in the structure, but porting first the patches and later on collapsing them can be a path forward, as long as the ABI is not impacted too much.

[kp]

In D29669#667551, @eri wrote:

You can add me to these reviews @kp !

Excellent!

I have implemented originally all of this without thinking of generic way of reusability.

The constraints of pfSense are not those of FreeBSD, that's inevitable.

The intention is to have some metadata tagged on states and rules to easy perfom operation and/or reporting.

I'm (slowly!) starting to understand what sorts of things pfSense uses this for. Right now my main conceptual issue is that pfSense expects the 'tracker' value in pflog output. Otherwise I'd go for the approach to just support multiple labels on a rule. That seems to cover everything except for the pflog extension. (Sticking 5 strings of 64 bytes each in the pflog header seems like it'd be a bad idea.)

The model can be improved by having a better tagging model then just stash new fields in the structure, but porting first the patches and later on collapsing them can be a path forward, as long as the ABI is not impacted too much.

The ABI problem is mostly solved, now that we have nvlist variants of add/get rule. I would still prefer to not change pf.conf syntax more than required (i.e. if we add 'schedule' I'll want to keep it).

[kp]

I've separated out the 'preserve rule counters' feature from the 'tracker' patch, and posted it here: https://reviews.freebsd.org/D29780
It differs from the original in that it's done at commit time rather than when the rule is added, so it's less likely to miss entries, it doesn't rely on user-configured unique IDs and it's not automatically enabled.

That does still leave the open question of how to deal with the tracker id in the pflog output. I'm still trying to come up with a good plan there.

[kp]

Replaced by https://reviews.freebsd.org/D29936