-
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
add_process_metadata: enrich process info with process owner (#21068) #21111
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Pinging @elastic/siem (Team:SIEM) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
jenkins run the tests please |
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 your contribution!
In addition to the comments I left, I think it's necessary to change the title/description of this PR. This isn't actually changing anything in Packetbeat, but updating the add_process_metadata
processor to add an username field.
Also, it needs an entry in the CHANGELOG.next.asciidoc file, under Added > Affecting all Beats, so that users know about the new field.
libbeat/processors/add_process_metadata/add_process_metadata.go
Outdated
Show resolved
Hide resolved
Hi. I do not understand, what's next requirements? I've resolved all issues. |
@adriansr Hi, could you review again please? |
jenkins run tests |
💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
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.
Apart from the CHANGELOG issue, we're discussing a possible rename of the field process.username
to align with the Elastic Common Schema.
CHANGELOG.next.asciidoc
Outdated
@@ -21,6 +21,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d | |||
- Remove the deprecated `xpack.monitoring.*` settings. Going forward only `monitoring.*` settings may be used. {issue}9424[9424] {pull}18608[18608] | |||
- Added `certificate` TLS verification mode to ignore server name mismatch. {issue}12283[12283] {pull}20293[20293] | |||
- Autodiscover doesn't generate any configuration when a variable is missing. Previously it generated an incomplete configuration. {pull}20898[20898] | |||
- `AddProcessMetadata` processor enrich process information with owner name. {issue}21068[21068] {pull}21111[21111] |
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 is in the wrong section, Breaking changes
. It should be under Added > Affecting All Beats
.
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.
@adriansr I've placed this line to proper section.
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 do you think is the proper field name for process owner? The options as I can see are:
process.username
process.owner
process.owner_name
Or process.owner
should be an object with it's own fields?
process.owner.uid
process.owner.name
...
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.
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.
Hi @dddpaul, we're still trying to figure out what makes more sense for this use case.
At some point, process.user.*
made it into ECS, but then it was reverted. See this comment.
So to align with ECS, we could do two things:
- Add user information to the top-level
user
object. - Add a custom object under
process
. For exampleowner
.
The first makes more sense, but at the same time is more risky for a processor, as this will cause the processor execution to fail if the input document already contains a user.name
or user.id
. If overwrite_keys
is set, then it will silently overwrite those keys, potentially breaking existing pipelines.
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.
As a general purpose processor I think it lacks the context to know if setting a ^user
is the correct thing to do. So setting a key(s) under process
is the safest thing to do and what I would most expect as a user.
Another alternative would be to offer an addition target_owner
config option that indicates where (or if) process owner info is added to the event. The registered_domain and translate_sid processors take an approach like this.
Hi, @adriansr what's next steps? |
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
Hi @dddpaul , I know this has been open for a long time, sorry about that. We are reviewing it. |
This pull request is now in conflicts. Could you fix it? 🙏
|
…mpty owner name. Update changelog. (elastic#21068)
fb82887
to
06aa7b2
Compare
/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
* master: Forward port 7.13.4 to master (elastic#26971) Use MustAddMetricSet in all metricsets (elastic#26907) add_process_metadata: enrich process info with process owner (elastic#21068) (elastic#21111) Use aws sdk paginator for FilterLogEvents and GetMetricData (elastic#26852) [Filebeat] Allow - for source IP for AWS S3 Access pipeline (elastic#26940) Increase timeout to 30secs (elastic#26841) Add Cluster filter on Kubernetes Overview ECS dashboard (elastic#26919)
Type: Enhancement
What does this PR do?
Enrich process metadata with process owner info.
Why is it important?
It's quite useful to know process owner, to control traffic, for example.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Exec some
curl http://example.com
from commandlineFlow type event will be enriched with
process.username
Related issues
Use cases
Screenshots
Logs