-
Notifications
You must be signed in to change notification settings - Fork 90
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
tests: add test for issue 7241/7242 for 7 #2035
Conversation
Add test that works with Suricata 7.
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.
This looks good to me.
How do you test the Suricata patch to use AppProtoEquals
?
I suppose it can really only be tested with profiling, where you'd check prefilter results or something. |
Why is CI green here ? 🤔 |
That is odd... did adding the 2nd rule to the test break the first? |
You can either merge the PRs, or go further down the rabbit hole ;-) |
Would this be useful? aa4883c |
Yes! Do you see it fail on |
Do we consider this test correct then? This is staged for merging but the conversation above indicates this test may be exposing a new issue and not showing the current one (which should have led to failure). |
The issue it exposes is not related to the PRs. |
match: | ||
alert.signature_id: 1 | ||
event_type: alert | ||
checks: |
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.
This test passes because this is wrong syntax for test.yaml
..
Here, we're checking for an event that matches a checks type dictionary in the yaml alongwith the attributes mentioned above. Since it does not exist, the count for such an event is 0..
Found when I was building upon this test and saw inconsistencies in non negation behavior.
- Non negation behavior is just fine. My analysis was incorrect because I was building upon an incorrect test.
- Negation behavior is buggy as shown by the ticket. This test does and should fail ideally on
main-7.0.x
.
Sending a PR fixing this..
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.
❤️ yaml
Had only tested for master x____x >__<" |
The engine-analysis tests don't fail for |
closing in favor of #2043 |
Add test that works with Suricata 7.
Replace #2034 to add a prefilter rule as well.