-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
detect-flowbits: add details for flowbits v8 #10026
Conversation
DETECT_FLOWBITS_CMD_NOALERT is misleading as it gives an impression that noalert is a flowbit specific command that'll be used and dealt with at some point but as soon as noalert is found in the rule lang, signature flag for noalert is set and control is returned. It never gets added to cmd of the flowbits object.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #10026 +/- ##
==========================================
- Coverage 82.47% 82.42% -0.06%
==========================================
Files 970 970
Lines 271372 271403 +31
==========================================
- Hits 223821 223702 -119
- Misses 47551 47701 +150
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me, but the intermediary commit that still contains the NOALERT
cmd type isn't necessary - as leads to the CI check. Could you please squash the commits?
I'll leave to Shivani the part about how to handle the and
cases, as she'll probably be able to give insight on that much faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Hadiqa! :)
Some minor comments inline. looking good from first sight.
- Please squash commits as asked by Juliana.
- Leave
and
for now. It is ok. It's not a separate command likeor
anyway. - Handle nits.
Make sure to submit clean PRs for both s-v and suricata. It'll make easier for us to review. Thanks!
|
||
jb_open_object(js, "flowbits"); | ||
switch (cd->cmd) { | ||
/* noalert has been removed and never gets to DumpMatches */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we haven't like removed anything. It just wasn't supposed to be here so we can remove this comment, I think.
} | ||
} | ||
jb_close(js); // array | ||
if (is_or == true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could just be if (is_or)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally didn't think of that D:
New PR: #10044 |
Task #6309
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6309
Previous PR: #10018
Describe changes:
SV_BRANCH=OISF/suricata-verify#1532