-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Enhance Suricata pipeline to handle destination.domain being set #10861
Enhance Suricata pipeline to handle destination.domain being set #10861
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.
LGTM
{ | ||
"script": { | ||
"source": "def domain = ctx.destination?.domain; if (domain instanceof Collection) { if (domain.length == 1) { ctx.destination.domain = domain[0]; } else { ctx.destination.domain = ctx.destination.domain.stream().distinct().collect(Collectors.toList()); } }", | ||
"ignore_failure": 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.
I wonder if domain containing an array will be a problem.
Suggestion: dedupe before if (domain.length == 1)
.
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've been sending in arrays for a few weeks and have made the team aware that this can be an array.
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 I implemented your suggestion. Good idea.
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.
Yeah we'll need to start looking into defining this more clearly in ECS.
But I agree that we have to support arrays here, because of the reverse DNS possibility.
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.
Ah wait. You're only keeping the first of the array? 🤔How do you know it's the interesting entry?
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. This is the code. It doesn't drop any data.
def domain = ctx.destination?.domain;
if (domain instanceof Collection) {
// Deduplicate
ctx.destination.domain = ctx.destination.domain.stream().distinct().collect(Collectors.toList());
// Make it a plain old string if there's only one item.
if (domain.length == 1) {
ctx.destination.domain = domain[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.
Can the reverse DNS processor return more than one DNS entry for a given IP?
If so, this only keeps the first one, right?
Or does the processor always return an array of 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.
Oh, I see the problem. When I refactored it from the initial version I broke it. The fixed version is:
def domain = ctx.destination?.domain;
if (domain instanceof Collection) {
// Deduplicate
domain = domain.stream().distinct().collect(Collectors.toList());
// Make it a plain old string if there's only one item.
if (domain.length == 1) {
domain = domain[0];
}
// Set the value
ctx.destination.domain = domain;
}
Does this address your concern?
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.
Ah, happy to see we weren't just going around in circles :-)
Yes, this makes sense now :-)
4e9e41b
to
06ab068
Compare
This replaces the usage of a `rename` processor with an `append` + `remove` processor. Then a script processor is used to deduplicate the domains. Fixes elastic#10510
06ab068
to
a95e713
Compare
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.
LGTM
This replaces the usage of a
rename
processor with anappend
+remove
processor.Then a script processor is used to deduplicate the domains.
This makes the pipeline compatible with the reverse dns processor being used on the Beat side.
Fixes #10510