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

7.0.x: backport treating unknown requirements as unsatisfied with opt-out flag - v2 #12226

Closed
wants to merge 2 commits into from

Conversation

jasonish
Copy link
Member

@jasonish jasonish commented Dec 4, 2024

For example, "requires: foo bar" is an unknown requirement, however
its not tracked, nor an error as it follows the syntax. Instead,
record these unknown keywords, and fail the requirements check if any
are present.

A future version of Suricata may have new requires keywords, for
example a check for keywords.

Ticket: OISF#7418
(cherry picked from commit 820a3e5)
The new behavior in 8, and backported is to treat unknown requirements
as unsatisfied requirements.

For 7.0.8, add a configuration option, "ignore-unknown-requirements"
to completely ignore unknown requirements, effectively treating them
as available.

Ticket: OISF#7434
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.17%. Comparing base (093397f) to head (7b63231).
Report is 11 commits behind head on main-7.0.x.

Additional details and impacted files
@@              Coverage Diff               @@
##           main-7.0.x   #12226      +/-   ##
==============================================
- Coverage       83.18%   83.17%   -0.01%     
==============================================
  Files             922      922              
  Lines          260858   260880      +22     
==============================================
  Hits           216982   216982              
- Misses          43876    43898      +22     
Flag Coverage Δ
fuzzcorpus 64.15% <100.00%> (+<0.01%) ⬆️
suricata-verify 63.35% <100.00%> (-0.02%) ⬇️
unittests 62.38% <84.90%> (+<0.01%) ⬆️

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

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 145 140 96.55%

Pipeline 23679

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Code looks good. Have a question on the new option inline.

as unsatisfied requirements, causing the rule to not be loaded. See
:ref:`keyword_requires`. To opt out of this change and to ignore
uknown requirements, effectively treating them as satified the
``ignore-unknown-requirements`` configuration option can be used.
Copy link
Member

Choose a reason for hiding this comment

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

are you thinking about making this a 7.0.x only option? If so, we should probably state that this is not behavior that will continue in 8

Copy link
Member

Choose a reason for hiding this comment

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

if we want to keep this in 8 and beyond, I'd like it to live under detect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we wouldn't have it at all. So will add a note that it won't be around in 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we want to keep this in 8 and beyond, I'd like it to live under detect.

Also note the reasoning for the location: #12226 (comment)

@@ -20,6 +20,10 @@
#include "detect-engine.h"
#include "rust.h"

/* Set to true if unknown requirements should be ingored. In Suricata
* 8, unknown requirements are treated as unsatisfied requirements. */
Copy link
Member

Choose a reason for hiding this comment

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

ah I think this answers my question. So do we need a warning/doc reference about this being 7 only behavior?

@jasonish
Copy link
Member Author

jasonish commented Dec 5, 2024

Continued at #12239.

@jasonish jasonish deleted the 7.0.x-7434-requires/v2 branch December 12, 2024 15:14
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.

3 participants