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.

Sign up for GitHub

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

Jump to bottom

[Winlogbeat] Add provider name to Security routing pipeline check #29781

Merged
merged 2 commits into from
Jan 12, 2022
Merged

[Winlogbeat] Add provider name to Security routing pipeline check #29781

merged 2 commits into from
Jan 12, 2022

Conversation

taylor-swanson
Copy link
Contributor

@taylor-swanson taylor-swanson commented Jan 10, 2022 •
edited
Loading

What does this PR do?

  • Added the two provider names currently supported by the Security pipeline
    to the conditional check in the routing pipeline. These two providers are
    Microsoft-Windows-Eventlog and Microsoft-Windows-Security-Auditing.
  • This will prevent unsupported providers such as "AD FS" from being
    enriched with incorrect information.

Why is it important?

  • Different log providers may use the same event ID. When a pipeline handles an event and blindly acts on the event ID without considering the provider, the event may be enriched with the wrong metadata.

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.

How to test this PR locally

Best way to test this would be to install winlogbeat and point it to the Security channel. I did install AD FS on my system, but I wasn't sure how to generate the necessary events to have them show up in the Security channel.

Related issues

Use cases

  • This in particular affects environments were AD FS is running, as it can log to the security channel, and it can log events that collide with the two existing providers.

@taylor-swanson
[Winlogbeat] Add provider name to Security routing pipeline check
760f0e7 
- Added the two provider names currently supported by the Security pipeline
to the conditional check in the routing pipeline. These two providers are
"Microsoft-Windows-Eventlog" and "Microsoft-Windows-Security-Auditing".
- This will prevent unsupported providers such as "AD FS" from being
enriched with incorrect information.
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jan 10, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 10, 2022

This pull request does not have a backport label. Could you fix it @taylor-swanson? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 10, 2022
@taylor-swanson taylor-swanson added backport-v8.1.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Jan 10, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 10, 2022 •
edited by jenkins-beats-ci bot
Loading

💚 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: 2022-01-10T20:44:31.832+0000

  • Duration: 59 min 33 sec

  • Commit: ab5ec38

Test stats 🧪

Test Results
Failed 0
Passed 27
Skipped 0
Total 27

💚 Flaky test report

Tests succeeded.

🤖 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!)

@taylor-swanson taylor-swanson marked this pull request as ready for review January 11, 2022 15:05
@taylor-swanson taylor-swanson requested a review from a team as a code owner January 11, 2022 15:05
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

andrewkroh
andrewkroh approved these changes Jan 11, 2022
View reviewed changes
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. Has this change been applied to elastic/integrations (system/windows)?

@taylor-swanson
Copy link
Contributor Author

LGTM. Has this change been applied to elastic/integrations (system/windows)?

Working on that one now. I'll get a PR up soon.

@taylor-swanson
Copy link
Contributor Author

PR for elastic/integrations change: elastic/integrations#2523

@taylor-swanson taylor-swanson merged commit 56423a0 into elastic:master Jan 12, 2022
@taylor-swanson taylor-swanson deleted the win-eventid-reuse branch January 12, 2022 15:06
v1v added a commit to v1v/beats that referenced this pull request Jan 12, 2022
@v1v
Merge remote-tracking branch 'upstream/master' into feature/add-githu…
0bab1d5 
…b-for-macos

* upstream/master: (172 commits)
  [Elastic Agent] Fix issue with ensureServiceToken. (elastic#29800)
  [Winlogbeat] Add provider name to Security routing pipeline check (elastic#29781)
  Add summary to journeys which don't emit journey:end (early node subprocess exits) (elastic#29606)
  Prepare 8.0.0-rc1 changelog (elastic#29795) (elastic#29806)
  Change docker image from CentOS 7 to Ubuntu 20.04 (elastic#29681)
  libbeat/processors/add_process_metadata: implement a process cache eviction policy (elastic#29717)
  [Automation] Update elastic stack version to 8.1.0-7004acda for testing (elastic#29783)
  Missing changelog entry for elastic#29773 (elastic#29791)
  Add a readme for k8s autodiscover provider (elastic#28213)
  Remove overriding of index pattern on the Kubernetes overview dashboard (elastic#29676)
  jjbb: remove obsoleted branches (<7.16) (elastic#29707)
  Add k8s metadata in state_cronjob metricset (elastic#29572)
  ibmmq: Fix timestamp parsing (elastic#29773)
  Do not add date to index if `@meta.index` is set (elastic#29775)
  ci: uses aliases for the branches (elastic#29706)
  Filebeat tests: Restore `@timestamp` field validation (elastic#29772)
  Forward port 7.16.3 changelog to master (elastic#29777)
  auditd: Store program arguments in process.args array (elastic#29601)
  System/socket: Support kernel_clone() replacement for _do_fork() (elastic#29744)
  Do not mention removal if version is not specified in `cfgwarn` messages (elastic#29727)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees
No one assigned
Labels
backport-v8.1.0 Automated backport with mergify bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Winlogbeat] Event ID reuse in the Security channel causes wrong enrichment
3 participants
@taylor-swanson @elasticmachine @andrewkroh