Skip to content

Fw updates/v15#15239

Closed
victorjulien wants to merge 13 commits intoOISF:mainfrom
victorjulien:fw-updates/v15
Closed

Fw updates/v15#15239
victorjulien wants to merge 13 commits intoOISF:mainfrom
victorjulien:fw-updates/v15

Conversation

@victorjulien
Copy link
Copy Markdown
Member

@victorjulien victorjulien commented Apr 21, 2026

Various firewall related improvements, including fix for https://redmine.openinfosecfoundation.org/issues/8495

SV_BRANCH=OISF/suricata-verify#3039

Includes #15223

jufajardini and others added 13 commits April 17, 2026 10:22
- tls.cert_chain_len
- datarep
- dataset
- dns.opcode

Part of
Ticket OISF#8387
The firewall enabling flag for tcp.flags was being overwritten by
another line of code.

Related to
Ticket OISF#8387
Clean up host mode tracking, which is used by reject to control how
rejects are sent. Before this patch there were 2 modes: sniffer only
and router. This patch introduces a bridge mode that is automatically
set by the bridge modes. In bridge mode the `Packet::livedev` is used.

Ticket: OISF#8390.
So a value of 0 means no device.
Most code uses an opague type for LiveDevice, so add an id getter.
In prep for storing both directions for IPS.
Update ctx caching to take direction into account.
Use an enum for the firewall related flow control, to improve
readability of the firewall inspection logic.
If a ruleset would use `dns:request_complete` but not have a rule for
`dns:request_started`, the `request_started` hook default policy would
not get invoked.

Add a check to make sure it is invoked.

Ticket: OISF#8495.
@victorjulien victorjulien requested review from a team and jasonish as code owners April 21, 2026 10:03
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 64.67391% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.69%. Comparing base (214e47b) to head (6ef3779).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15239   +/-   ##
=======================================
  Coverage   82.68%   82.69%           
=======================================
  Files         993      993           
  Lines      271880   271956   +76     
=======================================
+ Hits       224807   224892   +85     
+ Misses      47073    47064    -9     
Flag Coverage Δ
fuzzcorpus 61.04% <25.14%> (+0.04%) ⬆️
livemode 18.39% <33.14%> (+0.02%) ⬆️
netns 22.62% <43.50%> (-0.01%) ⬇️
pcap 45.31% <26.47%> (+0.01%) ⬆️
suricata-verify 66.29% <52.35%> (-0.04%) ⬇️
unittests 58.83% <19.55%> (-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 = 31017

@victorjulien victorjulien mentioned this pull request Apr 21, 2026
Copy link
Copy Markdown
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

To the extent that I can understand things, this is looking good.

Comment thread src/detect.c
Comment on lines +1577 to +1578
const bool accept_tx_applies_to_packet = last_tx;
if (accept_tx_applies_to_packet) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitty, but, do we need accept_tx_applies_to_packet now, if we'll just set it to last_tx and use it in this place?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For no I will leave it as I think the focus is on being able to follow the logic. I think the whole code needs to be split and cleaned later.

Comment thread src/detect.c
* - check for missing accept hooks
*
* \retval DETECT_TX_FW_FC_OK no action needed
* \retval DETECT_TX_FW_FC_BREAK rest of rules shouldn't inspected
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* \retval DETECT_TX_FW_FC_BREAK rest of rules shouldn't inspected
* \retval DETECT_TX_FW_FC_BREAK rest of rules shouldn't be inspected

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed locally, will be part of the next PR.

@catenacyber
Copy link
Copy Markdown
Contributor

Replaced by #15253 ?

Copy link
Copy Markdown
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Victor said he still wants to adress a comment here in a newer version of the PR

@victorjulien
Copy link
Copy Markdown
Member Author

Replaced by #15253

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