-
Notifications
You must be signed in to change notification settings - Fork 441
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
[santa] update ingest pipeline to match new log format #3347
Conversation
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
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 we leave the original tests in place while adding the new ones.
/test |
Is there any use case for preserving those old log samples? If I leave the original ones, the tests will fail. |
Do we know that these are no users still on the old version? If we are going to drop support for the older log format then this should really be a breaking change and marked as such. The preferred option though would be to retain support for the old format while adding the new. |
In my opinion, it doesn't make sense to support a pre-release output format for Santa. All the GA versions are outputting logs in the new format. I'll mark this release as a breaking change. |
/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.
Thanks for the fix!
🌐 Coverage report
|
"pid": 72012, | ||
"pidversion": 1097765, |
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.
process.pidversion
is not an allowed ECS field, so it should be placed in santa
, but we could construct a process.entity_id
with it and process.pid
, something like "{{{process.pid}}}-{{[process.pidversion}}}". @andrewkroh WDYT? We don't have any examples of it being used anywhere else AFAICS.
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.
That sounds like a good idea. I am looking for some documentation on what pidversion
in the Santa logs means just to confirm that it makes sense to use it as part of process.entity_id
.
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.
From what I could find it's just another pid, but from the kernel to prevent pid collision on pid rollover, e.g. https://www.exploit-db.com/exploits/46648. The Apple documentation is also not wildly helpful https://developer.apple.com/documentation/kernel/proc_persona_info/1560796-pidversion, the source helps https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/bsd/kern/kern_exec.c#L6215-L6216.
This in the context of this gives the best answer I could find.
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 have changed the field name to santa.pidversion
while the ECS is not ready yet to support it.
/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.
LGTM. We could add a process.entity_id
independently.
I was curious how Elastic Endpoint generates its process.entity_id
to see if we could replicate that here. The code is
(void)StringLib::FormatDuplicate(str, "%s-%llu-%llu.%llu", Cache::GetEndpointID().c_str(),
(unsigned long long)pid, (unsigned long long)ts.tv_sec, (unsigned long long)ts.tv_nsec);
(void)Titan_Encoding_toBase64(str.c_str(), str.size(), (void **)&(pBase64), &(base64Size));
But given that the start time we have is coming from the logger timestamp and lacks nanosecond precision, I don't think we can. So the only thing I would say is that if we do add an entity_id we should consider incorporating the agent ID.
SGTM |
What does this PR do?
The log output for Santa changed on versions 0.9.19, v0.9.20, and 0.9.21, making this integration fundamentally broken since then. This PR addresses the new log format parsing.
Checklist
changelog.yml
file.Author's Checklist
pidversion
How to test this PR locally
Related issues
I didn't find any related issue, although it seems to be broken since 2017.