Skip to content

Firewall: Rules [new]: Fix handling of interfacenot, evaluate as floating rules in correct prio_group #9426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Monviech wants to merge 9 commits into
base: master
Choose a base branch
from
+20 −4
Open

Firewall: Rules [new]: Fix handling of interfacenot, evaluate as floating rules in correct prio_group #9426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b987638
Firewall: Rules [new]: Fix handling of interfacenot, evaluate as floa…
Monviech Nov 19, 2025
df0c802
Correct the floating logic in the FilterRuleField.php
Monviech Nov 21, 2025
27dd202
Fix comment
Monviech Nov 21, 2025
c564d45
This is redundant
Monviech Nov 21, 2025
e6c13f3
Update src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/FilterC…
Monviech Dec 2, 2025
44cea6b
Update src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/FilterC…
Monviech Dec 2, 2025
d4c4a65
Update src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/FilterC…
Monviech Dec 2, 2025
9951b13
Update src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/FilterC…
Monviech Dec 3, 2025
8f152d9
Merge branch 'master' into firewall-interfacenot-floating
Monviech Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/FilterController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ public function searchRuleAction()
$is_cat = empty($categories) || array_intersect(explode(',', $record->categories), $categories);
$rule_interfaces = $record->interface->getValues();

if ((string)$record->interfacenot === "1") {
/*
* XXX Block the search from matching the interface in this
* case. It's not entirely correct, but at the moment we are
* not able to search for !interface either.
*/
$rule_interfaces = [];
}

if (empty($interfaces)) {
$is_if = count($rule_interfaces) != 1;
} elseif ($show_all) {
Expand Down Expand Up @@ -141,6 +150,9 @@ public function searchRuleAction()

if (empty($interfaces)) {
$is_if = empty($record['interface']) || count(explode(',', $record['interface'])) > 1;
} elseif ((string)$record['interfacenot'] === "1") {
/* XXX given that $interfaces is deduplicated multiple interfaces unmute the match */
$is_if = empty($record['interface']) || count($interfaces) > 1 || $record['interface'] != $interfaces[0];
} else {
$is_if = array_intersect(explode(',', $record['interface'] ?? ''), $interfaces);
$is_if = $is_if || empty($record['interface']);
Expand Down Expand Up @@ -403,8 +415,8 @@ public function getInterfaceListAction()
foreach ((new \OPNsense\Firewall\Filter())->rules->rule->iterateItems() as $rule) {
$interfaces = $rule->interface->getValues();

if (count($interfaces) !== 1) {
// floating: empty or multiple interfaces
if ($rule->interfacenot->isEqual(1) || count($interfaces) !== 1) {
// floating: empty, multiple, or inverted interface
$ruleCounts['floating'] = ($ruleCounts['floating'] ?? 0) + 1;
} else {
// single interface
Expand Down
8 changes: 6 additions & 2 deletions src/opnsense/mvc/app/models/OPNsense/Firewall/FieldTypes/FilterRuleField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,12 @@ public function getPriority()
{
$configObj = Config::getInstance()->object();
$interface = $this->interface->getValue();
if (strpos($interface, ',') !== false || empty($interface)) {
// floating (multiple interfaces involved)
if (
// floating: empty, multiple, or inverted interface
(string)$this->interfacenot === "1" ||
(strpos($interface, ",")) !== false ||
empty($interface)
Copy link
Member

@AdSchellevis AdSchellevis Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem to match our actual sorting

core/src/etc/inc/filter.lib.inc

Lines 675 to 681 in c6eaefc

if (isset($rule['floating'])) {
$prio = 200000;
} elseif (isset($ifgroups[$rule['interface']])) {
$prio = 300000 + $ifgroups[$rule['interface']];
} else {
$prio = 400000;
}

Copy link
Member

@fichtner fichtner Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be0b18930f36f9 says it's a floating rule pinned to one interface so this seems correct?

Copy link
Member

@fichtner fichtner Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the file a bit in 37e5056 5100521

For now we should limit floating to this condition and keep sorting single-floating rules to their interface-specific ordering location

The whole floating concept needs to die and adapting it further will just play ping pong with rules that work how they work given their current ordering (if it's correct or not is debatable)

Copy link
Member Author

@Monviech Monviech Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no single interface floating rules possible here. The condition right now should be correct.

Copy link
Member

@fichtner fichtner Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(string)$this->interfacenot === "1" || but this adds single interface ones ;)

Copy link
Member Author

@Monviech Monviech Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda, but only inverted single interface ones, which counts as multiple interfaces implicitely?

I guess inverting "LAN" makes it an infinite set of interfaces that exludes LAN, which counts as multiple?

Copy link
Member

@fichtner fichtner Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. given that the situation is what it is right now and we don't have any reports -- fixing a spot and missing several others is going to make it worse likely.

) {
return 200000;
} elseif (
!empty($configObj->interfaces) &&
Expand Down