Skip to content
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.

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

net/firewall - priority matching doesn't match core's #7111

Closed
3 tasks done
AdSchellevis opened this issue Nov 15, 2023 · 4 comments
Closed
3 tasks done

net/firewall - priority matching doesn't match core's #7111

AdSchellevis opened this issue Nov 15, 2023 · 4 comments
Assignees
Labels
bug Production bug
Milestone

Comments

@AdSchellevis
Copy link
Member

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug

It looks like the priority matching doesn't match the one in core when matching on different types of rules.

https://github.com/opnsense/plugins/blob/master/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/FieldTypes/FilterRuleField.php#L104-L120

Noticed this while looking at #6991

The ordering in core originates from

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

To Reproduce

Add some rules and search for" "prio:" in /tmp/rules.debug

Expected behavior
Rule logic should likely match the one in core, but changing it might influence peoples setups.
A couple of alternatives are possible, leave as is and accept rules are sorted differently (amend text in code) as nobody reported an issue and the rule overview does show where they live.

Align logic to core, where manual rules are sorted like :

  • < 200000 --> internal
  • >= 200000 < 300000 --> floating
  • >= 300000 < 400000 --> interface groups
  • >= 400000 < 500000 --> manual rules
  • >= 500000 --> internal (after normal rules)
@AdSchellevis AdSchellevis added the bug Production bug label Nov 15, 2023
@AdSchellevis AdSchellevis self-assigned this Nov 15, 2023
@mimugmail
Copy link
Member

I didn't have a deeper look, but will this change the sorting of the rules when combined with net/firewall and local rules?
My understanding is/was that rule order is:

  • Floating
  • local FW groups
  • net/firewall rules
  • interface rules

Correct?

@AdSchellevis
Copy link
Member Author

@mimugmail that was more or less the idea, handled by priority, I just made the numbers to small 🙃

@mimugmail
Copy link
Member

Ah, should've following the thread more precisely, thx

@doktornotor
Copy link
Contributor

Tested, 5aaada6 works fine here for #6991

Thanks!

@AdSchellevis AdSchellevis transferred this issue from opnsense/plugins Jan 3, 2024
@AdSchellevis AdSchellevis added this to the 24.7 milestone Jan 3, 2024
fichtner pushed a commit that referenced this issue Feb 16, 2024
…uence. closes #7111

After giving this some thought, it looks like a good idea to fix this bug anyway. There is a very small chance people combine legacy and mvc rules which contradict eachother, but in the long run this will lead to more issues. Since getPriority() skipped group priority, we'll add the same calculation as being used in https://github.com/opnsense/core/blob/db4b90d218296826d37b7ce3fa368c274408f401/src/etc/inc/filter.lib.inc#L632-L638 too.

(cherry picked from commit 1c96851)
(cherry picked from commit aec9706)
fichtner pushed a commit that referenced this issue Mar 12, 2024
…uence. closes #7111

After giving this some thought, it looks like a good idea to fix this bug anyway. There is a very small chance people combine legacy and mvc rules which contradict eachother, but in the long run this will lead to more issues. Since getPriority() skipped group priority, we'll add the same calculation as being used in https://github.com/opnsense/core/blob/db4b90d218296826d37b7ce3fa368c274408f401/src/etc/inc/filter.lib.inc#L632-L638 too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Production bug
Development

No branches or pull requests

3 participants