Skip to content

fw: validate scope inheritance in policies and rules + pass&config action updates v3 - #16138

Closed
lukashino wants to merge 4 commits into
OISF:mainfrom
lukashino:bug/8954-action-scope-verify-v3
Closed

fw: validate scope inheritance in policies and rules + pass&config action updates v3#16138
lukashino wants to merge 4 commits into
OISF:mainfrom
lukashino:bug/8954-action-scope-verify-v3

Conversation

@lukashino

@lukashino lukashino commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Follow-up of #16132

Link to ticket: https://redmine.openinfosecfoundation.org/issues/8954

Describe changes:
v3:

  • config action disallowed to be used in policies
  • document config action in the FW overview action enumeration

v2:

  • validate pass only is used next to accept primary action

v1:

  • validate pass only inherits packet and flow scopes

SV_BRANCH=OISF/suricata-verify#3331

Lukas Sismis added 4 commits August 28, 2026 11:51
A secondary action given without an explicit scope inherits the scope of
the primary action, but was never validated against the scopes it
supports itself. `pass` only supports packet and flow scope but
the inheritance was never verified.

Ticket: 8954
Previously, hook sequences in both rules and default policies
allowed `pass` to be combined with incompatible hooks (e.g.
reject or drop).

This commit clamps pass to accept only.

Ticket: 8954

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens firewall action validation in detect-parse.c to enforce correct scope/action combinations (notably around pass scope inheritance and config usage), and updates the user guide to document the config firewall action.

Changes:

  • Enforce that pass can only be used as a secondary action when the primary firewall action is accept, and restrict pass scope inheritance to packet/flow.
  • Disallow config as a default policy action in firewall.policies.
  • Document the config firewall action in the firewall design user guide.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/detect-parse.c Adds parser validations for pass usage/scope inheritance and rejects config in default policies.
doc/userguide/firewall/firewall-design.rst Adds documentation for the config firewall action in the action overview.
Suppressed comments (1)

src/detect-parse.c:4220

  • This introduces a new policy-level validation (config is rejected as a default policy action). Please add unit tests that cover parsing firewall.policies with config:packet (expected failure) and a representative valid policy action list (expected success), so this behavior stays enforced across future refactors.
    if (action & ACTION_CONFIG) {
        SCLogError("%s: 'config' is not a valid default policy action", policy_name);
        return -1;
    }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/detect-parse.c
Comment thread doc/userguide/firewall/firewall-design.rst
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.05%. Comparing base (389700e) to head (2f80978).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16138   +/-   ##
=======================================
  Coverage   83.05%   83.05%           
=======================================
  Files        1004     1004           
  Lines      277407   277418   +11     
=======================================
+ Hits       230410   230420   +10     
- Misses      46997    46998    +1     
Flag Coverage Δ
fuzzcorpus 61.49% <9.09%> (-0.01%) ⬇️
livemode 18.49% <9.09%> (+0.07%) ⬆️
netns 22.88% <18.18%> (-0.03%) ⬇️
pcap 45.40% <9.09%> (+0.01%) ⬆️
suricata-verify 67.24% <100.00%> (-0.01%) ⬇️
unittests 58.52% <9.09%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 33427

@lukashino
lukashino marked this pull request as draft August 28, 2026 17:58
@lukashino
lukashino marked this pull request as ready for review August 29, 2026 15:27
@victorjulien victorjulien added this to the 9.0 milestone Sep 4, 2026
@victorjulien

Copy link
Copy Markdown
Member

Merged in #16185, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants