Skip to content
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

[Auditbeat] fim(kprobes): enrich file events by coupling add_process_metadata processor #38776

Conversation

pkoutsovasilis
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis commented Apr 8, 2024

Proposed commit message

This PR adds reporting of process.group.id, process.group.name and process.entity_id in add_process_metadata processor. Also it changes the factory of MetricSets to allow the latter to specify Processors after successful instantiation; this is required as FIM has 3 different available backends, namely fsnotify, kprobes, ebpf and only the kprobes one requires to have add_process_metadata processor. Utilising the former, kprobes backend always adds a properly configured add_process_metadata processor. As a result, enriching kprobes file events with process-related data exhibits the same robustness levels of the current add_process_metadata processor. However, the current design is aligned with @nick-alayil and the sec-linux-platform which plan to increase the robustness of add_process_metadata processor in a separate effort.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

Related issues

Screenshots

image

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 8, 2024
@pkoutsovasilis pkoutsovasilis changed the title Pkoutsovasilis/fim kprobes process metadata [Auditbeat] fim(kprobes): enrich file events by coupling add_process_metadata processor Apr 8, 2024
Copy link
Contributor

mergify bot commented Apr 8, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @pkoutsovasilis? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@pkoutsovasilis pkoutsovasilis added enhancement Team:Security-Linux Platform Linux Platform Team in Security Solution backport-v8.13.0 Automated backport with mergify labels Apr 8, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 8, 2024
@pkoutsovasilis pkoutsovasilis marked this pull request as ready for review April 8, 2024 18:01
@pkoutsovasilis pkoutsovasilis requested review from a team as code owners April 8, 2024 18:01
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 8, 2024

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-04-13T00:10:14.568+0000

  • Duration: 150 min 34 sec

Test stats 🧪

Test Results
Failed 0
Passed 29384
Skipped 2061
Total 31445

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Apr 9, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty clean approach you took to add processors to MetricSets. I think we just need a few more details in the interface godocs as mentioned in my comment.


I tend to view Beats as being the building block to getting data ingested how you want it. So I am not in favor of turning extra fields on by default. For example, I would rather see events starting as empty documents and the inputs contributing the raw data. Anything else is opt-in by the user through processors whether that be agent metadata, host metadata, process metadata, etc (but I'm probably not the typical user).

Today's situation is that we have a lot of things that are on by default (or not even possible to disable like host.name or agent.* or even docker metadata when run under Elastic Agent) so if you want to opt-out your only solution is to delete the fields with a processor (which is wasteful).

But I understand the desire to have everything just work out of the box. It's hard to serve the two personas that want it to just work and want to be a composable ingest tool. So allow me to mention an alternative.

  1. Document within Auditbeat's FIM guide how to add the enrichment processor yourself. This gives the user full control over what fields are they wish to add. e.g.
processors:
  - if.and:
      - equals.event.module: file_integrity
      - not.has_fields: [process.name]
      - has_fields: [process.pid]
    then:
      add_process_metadata:
        match_pids:
        - process.pid
  1. Add the processor to the Fleet FIM integration configuration. For a Fleet integration, the batteries should be included.

auditbeat/module/file_integrity/metricset.go Outdated Show resolved Hide resolved
auditbeat/module/file_integrity/eventreader_kprobes.go Outdated Show resolved Hide resolved
metricbeat/mb/module/factory.go Show resolved Hide resolved
auditbeat/module/file_integrity/eventreader_kprobes.go Outdated Show resolved Hide resolved
auditbeat/module/file_integrity/metricset.go Show resolved Hide resolved
…silis/fim-kprobes-process-metadata

# Conflicts:
#	libbeat/processors/add_process_metadata/add_process_metadata.go
#	libbeat/processors/add_process_metadata/config.go
#	libbeat/processors/add_process_metadata/gosysinfo_provider.go
#	metricbeat/mb/module/configuration.go
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pkoutsovasilis pkoutsovasilis requested a review from a team as a code owner April 13, 2024 00:09
@pkoutsovasilis pkoutsovasilis enabled auto-merge (squash) April 13, 2024 00:26
@pkoutsovasilis pkoutsovasilis merged commit ca4adce into elastic:main Apr 13, 2024
198 of 207 checks passed
mergify bot pushed a commit that referenced this pull request Apr 13, 2024
…metadata processor (#38776)

* feat(processors/process_metadata): support reporting group id and name

* feat(processors/process_metadata): support reporting process entity_id

* feat(fim/kprobes): allow metricsSets to expose beat processors after initialisation

* doc: update CHANGELOG.next.asciidoc

* fix(linter): SA1015 prevent leaking the ticker

* fix(linter): SA1019 mark metricbeat/mb deprecation warnings that are not removed yet

* fix(linter): check for return err

* fix(linter): prealloc slices

* fix(linter): remove unused field

* fix(linter): G601 prevent implicit memory aliasing in for loop

* doc: update CHANGELOG.next.asciidoc

* fix: update filebaet fields.asciidoc (unrelated to this work)

* doc: remove irrelevant changes from CHANGELOG.next.asciidoc

* feat(processor/metadata): introduce new type based allocation func

* feat(fim/kprobe): instantiate new processor alongside a new kprobes event reader

* fix(fim): remove redundant whitespace

* doc(metricbeat): enrich documentation about Processors attached to a Metricbeat

* fix(fim): gofumpt eventreader_kprobes.go

* fix(add_process_metadata): gofmt add_process_metadata.go gosysinfo_provider.go

* fix(lint): goimports eventreader_kprobes.go

* fix(winlogbeat): generate include list [unrelated to this PR]

(cherry picked from commit ca4adce)

# Conflicts:
#	libbeat/processors/add_process_metadata/add_process_metadata.go
#	libbeat/processors/add_process_metadata/config.go
#	libbeat/processors/add_process_metadata/gosysinfo_provider.go
#	metricbeat/mb/module/configuration.go
@pkoutsovasilis pkoutsovasilis removed the backport-v8.13.0 Automated backport with mergify label Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Elastic-Agent Label for the Agent team Team:Security-Linux Platform Linux Platform Team in Security Solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants