-
Notifications
You must be signed in to change notification settings - Fork 1.6k
snmp: can not be set to detection-only #14045
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
base: main
Are you sure you want to change the base?
Conversation
Fix the snmp app-layer config problem, Normalize the dynamic alproto registration bug: OISF#8000
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.
Thanks for this work.
My main remark is :
Could we have a suricata-verify test for this ?
Also, CI is red because of nits like formatting... Could you fix it ?
For the commit title, I would phrase more like the solution than the problem, something like
snmp: can be set to detection-only
dir: SCOutputJsonLogDirection::LOG_DIR_PACKET as u8, | ||
LogTx: Some(snmp_log_json_response), | ||
}; | ||
SCOutputEvePreRegisterLogger(reg_data); |
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.
Does it mean that we should call SCOutputEvePreRegisterLogger
even if SCAppLayerProtoDetectConfProtoDetectionEnabled
returns 0 ?
I would not think so
{ | ||
SCEnter(); | ||
|
||
if (alp_ctx.ctxs_len <= alproto && alproto < g_alproto_max) { |
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 am not sure about the moving of this code to be called in AppProtoRegisterProtoString
Could you explain it in the commit message ?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14045 +/- ##
=======================================
Coverage 84.43% 84.44%
=======================================
Files 1011 1011
Lines 272251 272261 +10
=======================================
+ Hits 229876 229909 +33
+ Misses 42375 42352 -23
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Fix the snmp app-layer config problem, Normalize the dynamic alproto registration
bug: #8000
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
Describe changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCH
variable.SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=