Skip to content

[windows] Windows Defender Data stream overhaul to GA #11249

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

Merged
merged 9 commits into from
Oct 21, 2024

Conversation

nicpenning
Copy link
Contributor

@nicpenning nicpenning commented Sep 25, 2024

  • Enhancement

Proposed commit message

Overhaul Windows Defender data stream in the Windows integration to make it GA.

Added many ECS fields and removed un-needed fields/processors

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Screenshots

image

@andrewkroh andrewkroh added bugfix Pull request that fixes a bug issue Integration:windows Windows labels Sep 25, 2024
@nicpenning nicpenning marked this pull request as ready for review September 25, 2024 22:25
@nicpenning nicpenning requested review from a team as code owners September 25, 2024 22:25
@nicpenning
Copy link
Contributor Author

Ready for review and tests.

@andrewkroh andrewkroh added the Team:Security-Windows Platform Security Windows Platform team [elastic/sec-windows-platform] label Sep 25, 2024
@elasticmachine
Copy link

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Agent Data Plane team [elastic/elastic-agent-data-plane] label Sep 26, 2024
@elasticmachine
Copy link

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@nicpenning
Copy link
Contributor Author

Made a few corrections. Please review and test now.

Copy link
Contributor

@intxgo intxgo left a comment

Choose a reason for hiding this comment

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

changes LGTM, but I guess this will fully process only a subset of Windows Defender events. Are the other IDs unimportant or there's just no documentation for them?

@nicpenning
Copy link
Contributor Author

nicpenning commented Oct 3, 2024

changes LGTM, but I guess this will fully process only a subset of Windows Defender events. Are the other IDs unimportant or there's just no documentation for them?

Great question! I targeted all the IDs I observed in my test environment. My hope is that after this beta version of the data stream is out there I can use it at a larger scale and narrow in on more IDs.

The problem is that I can't tell what fields and what type of data is expected in the data from the MS docs.

Also, this hits the primary ones of interest for sure (malware detected/prevented).

It will process most but there will be some casualties, I am sure. Without a full list of Event IDs and their actual field names to expect, it is hard to get this 100% :/

Copy link
Contributor

@taylor-swanson taylor-swanson left a comment

Choose a reason for hiding this comment

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

Couple of minor comments, otherwise seems good to me

Copy link
Contributor

@fearful-symmetry fearful-symmetry left a comment

Choose a reason for hiding this comment

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

Logic looks fine, can't comment on the windows-specific fields though.

@rdner
Copy link
Member

rdner commented Oct 14, 2024

/test

@nicpenning
Copy link
Contributor Author

Looks like pipeline tests failed. I will test locally to see what is going on.

@nicpenning
Copy link
Contributor Author

nicpenning commented Oct 15, 2024

Doesn't appear that the generated test files were pushed. I will get those pushed as soon as I can.

@nicpenning
Copy link
Contributor Author

Ready for tests again!

@rdner
Copy link
Member

rdner commented Oct 16, 2024

/test

@rdner
Copy link
Member

rdner commented Oct 16, 2024

@taylor-swanson could you validate the changes in 3de2f48 please?

@rdner rdner requested a review from taylor-swanson October 16, 2024 17:42
@elastic-vault-github-plugin-prod

🚀 Benchmarks report

Package windows 👍(5) 💚(3) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
windows_defender 30303.03 16949.15 -13353.88 (-44.07%) 💔

To see the full report comment with /test benchmark fullreport

Copy link

@elasticmachine
Copy link

💚 Build Succeeded

History

@taylor-swanson
Copy link
Contributor

@taylor-swanson could you validate the changes in 3de2f48 please?

I think the test results are good.

  • At first I thought an event was removed, but it was the git diff that tripped me up. It looks like one of the test cases (event ID 1116) was moved to the top and that can cause some misleading git diffs as a result. This includes extra lines showing up in the diff even though the results didn't really change. This is why I try to avoid reordering test cases, since the resulting git diffs tend to be misleading. Everything seems to check out, but feel free to point out something that's suspicious if I missed something.
  • It looks like the user_data field was removed and was replaced with an enriched event_data field. The PR doesn't mention why this is the case, but I'll assume we have better sample data now and are including it in the tests.
  • The rest of the changes to the expected test file are either from fields being sorted or updates from the changes in the pipeline.

@rdner rdner merged commit baf51ab into elastic:main Oct 21, 2024
5 checks passed
@elastic-vault-github-plugin-prod

Package windows - 2.2.0 containing this change is available at https://epr.elastic.co/search?package=windows

@nicpenning nicpenning deleted the defender_ecs_map_and_ga branch October 22, 2024 04:03
@nicpenning
Copy link
Contributor Author

@taylor-swanson could you validate the changes in 3de2f48 please?

I think the test results are good.

  • At first I thought an event was removed, but it was the git diff that tripped me up. It looks like one of the test cases (event ID 1116) was moved to the top and that can cause some misleading git diffs as a result. This includes extra lines showing up in the diff even though the results didn't really change. This is why I try to avoid reordering test cases, since the resulting git diffs tend to be misleading. Everything seems to check out, but feel free to point out something that's suspicious if I missed something.
  • It looks like the user_data field was removed and was replaced with an enriched event_data field. The PR doesn't mention why this is the case, but I'll assume we have better sample data now and are including it in the tests.
  • The rest of the changes to the expected test file are either from fields being sorted or updates from the changes in the pipeline.

Good observations.

I did reorder because I wanted the system test to use this more practical event in the docs. Also when I generated improved json that maps closer to the real world, I used it as is and didn't try to put them back in the right order.

As for the user fields, those were likely residual fields from other windows integrations that this one didn't really use so I capitalized on what actual event fields are being used in the event IDs I was able to generate for my tests.

I will be cautious next time keeping these event IDs in the proper order. Thanks for the review!

harnish-crest-data pushed a commit to chavdaharnish/integrations that referenced this pull request Feb 4, 2025
* Defender Data stream overhaul to GA
* Adjust pipeline to ensure event type is applied
* Update Readme
* Improve test data with event_data blocks, switch to GSUB and SET for file path extractions.
* Generated new JSON test files
harnish-crest-data pushed a commit to chavdaharnish/integrations that referenced this pull request Feb 5, 2025
* Defender Data stream overhaul to GA
* Adjust pipeline to ensure event type is applied
* Update Readme
* Improve test data with event_data blocks, switch to GSUB and SET for file path extractions.
* Generated new JSON test files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug issue Integration:windows Windows Team:Elastic-Agent-Data-Plane Agent Data Plane team [elastic/elastic-agent-data-plane] Team:Security-Windows Platform Security Windows Platform team [elastic/sec-windows-platform]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants