-
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 v7 #10018
Conversation
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.
Kudos for the progress! :)
I'm still considering what should be the approach for the noalert
case - might be we're missing documentation, maybe it's a case where we need a warning - like we have for the flowbits that haven't been set...
I think this has to be discussed with more mentors, for us to understand what approach to follow, here.
jb_set_string(js, "cmd", "toggle"); | ||
break; | ||
} | ||
int flag = 0; |
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.
As it's being used as a bool, let's declare it as such ;)
We can also use a more descriptive name, maybe is_or
?
Could we move it into the if noalert
context?
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.
+1 to everything
break; | ||
} | ||
int flag = 0; | ||
if (cd->cmd != DETECT_FLOWBITS_CMD_NOALERT) { |
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.
And now wondering if we need this noalert
if
, here.
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 don't think we do. I think we should also remove this constant though bc it gives me an impression that this is a valid flowbit specific command and while it is used w flowbits, it is never really used to set cmd
.
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.
wrt I have made #10021
If it gets accepted, please rebase your code on that PR and remove all usages of DETECT_FLOWBITS_CMD_NOALERT
.
If it gets rejected, maybe just add some notes like Juliana mentioned about noalert control never getting to this part.
case DETECT_FLOWBITS_CMD_NOALERT: | ||
jb_set_string(js, "action", "noalert"); | ||
break; |
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.
Now, we know from our discussions elsewhere that we can't ever have this one here, so we can probably skip the jb call, maybe just add a comment that noalert
doesn't lead to calling DumpMatches
? 🤔
if (flag == 1) { | ||
jb_set_string(js, "operator", "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 saw that this is working! :) Do you think we can move on to thinking about the and
case implementation now, @inashivb ?
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.
Indeed. We don't define it as a specific operator anyway like we do for or. It's only fair. 😛
I think it is fine as-is. We do mention in our docs that it is identical to standalone |
New PR: #10026 |
Task #6309
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6309
Previous PR: #10008
Describe changes:
SV_BRANCH=OISF/suricata-verify#1529