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

Fix related.ip field #24892

Merged
merged 5 commits into from
May 5, 2021
Merged

Fix related.ip field #24892

merged 5 commits into from
May 5, 2021

Conversation

janniten
Copy link
Contributor

@janniten janniten commented Apr 1, 2021

What does this PR do?

Fix a bug found when populating related.ip field information in the renameCommonAuthFields
The wrong field winlog.event_data.ClientAddress was copied into related.ip instead of winlog.event_data.IpAddress which is the proper one.

Why is it important?

The related.ip information was not populated correctly in some events. For example: 4624,4648,4625

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 an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@janniten janniten requested a review from a team as a code owner April 1, 2021 09:52
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 1, 2021
@janniten janniten mentioned this pull request Apr 1, 2021
Closed
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 1, 2021 •
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

Expand to view the summary

Build stats

  • Build Cause: Pull request #24892 updated

  • Start Time: 2021-05-04T20:20:49.999+0000

  • Duration: 71 min 54 sec

  • Commit: c4de305

Test stats 🧪

Test Results
Failed 0
Passed 880
Skipped 0
Total 880

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 880
Skipped 0
Total 880

@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 1, 2021
@jsoriano jsoriano added the review label Apr 1, 2021
andrewkroh
andrewkroh reviewed Apr 8, 2021
View reviewed changes
x-pack/winlogbeat/module/security/config/winlogbeat-security.js Outdated
@@ -2005,7 +2005,7 @@ var security = (function () {
{from: "winlog.event_data.ProcessId", to: "process.pid", type: "long"},
{from: "winlog.event_data.ProcessName", to: "process.executable"},
{from: "winlog.event_data.IpAddress", to: "source.ip", type: "ip"},
{from: "winlog.event_data.ClientAddress", to: "related.ip", type: "ip"},
{from: "winlog.event_data.IpAddress", to: "related.ip", type: "ip"},
Copy link
Member

Choose a reason for hiding this comment

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

Does the earlier line that renames winlog.event_data.IpAddress to source.ip cause a problem for this? I wonder if a more generic copy from source.ip/destination.ip to related.ip that gets applied to all events would be better (not sure if there's already something like this in the pipeline).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @andrewkroh, I agree with you.
I'll populate the related.ip from the generic fields, I'll test it and commit the modifications

@andrewkroh andrewkroh added the needs_integration_sync Changes in this PR need synced to elastic/integrations. label Apr 8, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 21, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b related.ip_fix upstream/related.ip_fix
git merge upstream/master
git push upstream related.ip_fix

@leehinman leehinman added the backport-v7.14.0 Automated backport with mergify label May 4, 2021
leehinman
leehinman approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Looks good. I made 2 changes

  • updated golden files
  • replaced () with {} in asciidoc

@leehinman
Copy link
Contributor

removing "needs_integration_sync" label because the switch to ingest node pipeline already has this fix.

@leehinman leehinman removed the needs_integration_sync Changes in this PR need synced to elastic/integrations. label May 4, 2021
@leehinman leehinman merged commit d0887fd into elastic:master May 5, 2021
mergify bot pushed a commit that referenced this pull request May 5, 2021
@janniten
Fix related.ip field (#24892)
09367ac 
- [Winlogbeat] fix related.ip field in renameCommonAuthFields function

Co-authored-by: Lee E. Hinman <[email protected]>
(cherry picked from commit d0887fd)
@mergify mergify bot mentioned this pull request May 5, 2021
Merged
leehinman pushed a commit that referenced this pull request May 12, 2021
@mergify @janniten
Fix related.ip field (#24892) (#25534)
43a09c6 
- [Winlogbeat] fix related.ip field in renameCommonAuthFields function

Co-authored-by: Lee E. Hinman <[email protected]>
(cherry picked from commit d0887fd)

Co-authored-by: Anabella Cristaldi <[email protected]>
@janniten janniten deleted the related.ip_fix branch May 17, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh left review comments

@leehinman leehinman leehinman approved these changes

Assignees

@leehinman leehinman

Labels
backport-v7.14.0 Automated backport with mergify review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

related.ip is not completed correctly for some events
5 participants
@janniten @elasticmachine @leehinman @andrewkroh @jsoriano