-
Notifications
You must be signed in to change notification settings - Fork 91
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 log alert tx one 7199 v6 #2157
Detect log alert tx one 7199 v6 #2157
Conversation
More of a change in behavior than a bug, but important to be documented Related to Bug https://redmine.openinfosecfoundation.org/issues/7199
2f40758
to
cde493b
Compare
@@ -16,3 +16,6 @@ app-layer: | |||
protocols: | |||
pgsql: | |||
enabled: yes | |||
|
|||
detect: | |||
guess-applayer-tx: 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.
should there also be a test.yaml update with this test?
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.
No, why should there be ?
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.
to include that the tx was guessed? (I guess?)
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 do not think it is important... Is it ?
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.
what is the point of the change if there is no test update with it?
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.
TL;DR Otherwise this test pgsql-bug-6983-ips fails with the Suricata PR.
I think you are referring to Suricata change here : https://github.com/OISF/suricata/pull/12197/files#diff-ee484ae4b77e59eb8b6b50f628c84ab626bd6178a6d43e1219dbc9619be7e027L816
Over simplifying it : instead of using (alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH)
we now use de_ctx->force_applayer
pgsql-bug-6983-ips resorted to (alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH)
to have pgsql metadata in alerts.
With the Suricata PR, we now need de_ctx->force_applayer
meaning detect.guess-applayer-tx: true
in yaml
That comes from how I understood your comment OISF/suricata#12166 (comment)
We've decided it's worse to log the wrong one, than to log none.
Beyond pgsql-bug-6983-ips test, this SV PR has test bug-7199 to show the new benefits of the new behavior.
cde493b
to
18aba67
Compare
Follow up, as agreed I'd do: #2168 |
So closing in favor of #2168 |
Ticket
Redmine ticket:
https://redmine.openinfosecfoundation.org/issues/7199
https://redmine.openinfosecfoundation.org/issues/7350
#2146 next version
guess-applayer-tx