-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Backports/708/v3 #12267
Backports/708/v3 #12267
Conversation
Ticket: 7199 Uses a config parameter detect.guess-applayer-tx to enable this behavior (off by default) This feature is requested for use cases with signatures not using app-layer keywords but still targetting application layer transactions, such as pass/drop rule combination, or lua usage. This overrides the previous behavior of checking if the signature has a content match, by checking if there is only one live transaction, in addition to the config parameter being set. (cherry picked from commit f2c3776)
The `guess-applayer-tx` work also removed the stream match condition for adding app-layer metadata to alerts. This is a behavior change that is not desired at this point, so this commit reverts that part of the changes. We keep the exising logging of app-layer metadata if the match was in the stream.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-7.0.x #12267 +/- ##
==============================================
- Coverage 83.19% 83.18% -0.02%
==============================================
Files 922 922
Lines 260888 260898 +10
==============================================
- Hits 217048 217026 -22
- Misses 43840 43872 +32
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.
Original PR ok for me, plus added doc commit ok for me
picked up.** This is to reduce the chances of logging unrelated data, and may | ||
lead to alerts being logged without metadata, in some cases. | ||
The alert event will have ``tx_guessed: true`` to recognize | ||
such alerts. |
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.
Do you want to add there that we also log some transaction metadata for rule with stream matches ?
And that the first transaction is logged, like one TCP packet with 3 DNS requests to suricata.io oisf.net and suricon.net and rule alert tcp any any -> any any (content: "suricon"; sid: 1)
will log the request to suricata.io
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.
Seems all too much of a low level implementation detail to discuss in configuration docs. But these types of issues are making me think we need a know issues and shortcomings chapter.
# try to tie an app-layer transaction for rules without app-layer keywords | ||
# if there is only one live transaction for the flow | ||
# allows to log app-layer metadata in alert | ||
# but the transaction may not be the relevant one. |
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.
Tried to add a bit more here (from https://github.com/OISF/suricata/pull/12260/files):
# try to tie an app-layer transaction for rules without app-layer keywords | |
# if there is only one live transaction for the flow | |
# allows to log app-layer metadata in alert | |
# but the transaction may not be the relevant one. | |
# Try to guess an app-layer transaction for rules without app-layer keywords, | |
# ONLY IF there is just one live transaction for the flow. | |
# This allows logging app-layer metadata in alert - the transaction may not | |
# be the relevant one for the alert. |
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.
Tried to add a bit more here (from https://github.com/OISF/suricata/pull/12260/files):
On suggestion...
ONLY IF there is a single live transaction for the flow,
IMO, can be cleaned up in post-release.
Should pass with: SV_BRANCH=OISF/suricata-verify#2179 |
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.
Stage with #12264 and OISF/suricata-verify#2179
replaced by #12268 |
#12263, with 5d22aa4