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 handling for missing event data types in the experimental API #40684

Merged
merged 5 commits into from
Sep 20, 2024
Merged

[winlogbeat] Add handling for missing event data types in the experimental API #40684

merged 5 commits into from
Sep 20, 2024

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Sep 3, 2024

Proposed commit message

Adds handling for missing event data types in the experimental API

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

  • Add sample evtx files

Related issues

@marc-gr marc-gr added enhancement Winlogbeat Team:Security-Windows Platform Windows Platform Team in Security Solution labels Sep 3, 2024
@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 Sep 3, 2024
@mergify mergify bot assigned marc-gr Sep 3, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 3, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @marc-gr? 🙏.
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

@marc-gr marc-gr added the backport-skip Skip notification from the automated backport with mergify label Sep 3, 2024
@marc-gr marc-gr marked this pull request as ready for review September 3, 2024 11:35
@marc-gr marc-gr requested a review from a team as a code owner September 3, 2024 11:35
@elasticmachine
Copy link
Collaborator

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

marc-gr
marc-gr commented Sep 3, 2024
View reviewed changes
winlogbeat/sys/strings_windows.go
var ansiDecoder *encoding.Decoder

func init() {
ansiCP := windows.GetACP()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% about this one. Not sure if would be better to just add a config option that defaults either to ACP or Windows1250 and that can be user defined. I am not super confident the active page will always be the one we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

or rather option to use specific ANSI code page or let us discover it if not set
However I think the GetACP will return the ANSI code page we want, because if people are using non-English variants of OS the winlog messages are also non-English

@marc-gr marc-gr added backport-8.x Automated backport to the 8.x branch with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Sep 10, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 10, 2024

backport-v8.x has been added to help with the transition to the new branch 8.x.

@v1v v1v removed the backport-v8.x label Sep 11, 2024
andrewkroh
andrewkroh reviewed Sep 13, 2024
View reviewed changes
winlogbeat/sys/strings.go Show resolved Hide resolved
winlogbeat/sys/strings.go Outdated Show resolved Hide resolved
marc-gr and others added 2 commits September 17, 2024 12:52
@marc-gr @andrewkroh
Update winlogbeat/sys/strings.go
2daaf08 
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
@marc-gr
Add godoc
7c7e56f
@norrietaylor norrietaylor requested a review from a team September 18, 2024 14:11
@marc-gr marc-gr merged commit 9c0ff04 into elastic:main Sep 20, 2024
24 of 26 checks passed
mergify bot pushed a commit that referenced this pull request Sep 20, 2024
@marc-gr @mergify
[winlogbeat] Add handling for missing event data types in the experim…
bc672db 
…ental API (#40684)

(cherry picked from commit 9c0ff04)
@mergify mergify bot mentioned this pull request Sep 20, 2024
Merged
7 tasks
marc-gr pushed a commit that referenced this pull request Sep 20, 2024
@mergify
[winlogbeat] Add handling for missing event data types in the experim…
6f9b1ef 
…ental API (#40684) (#40913)
@ycombinator ycombinator mentioned this pull request Sep 26, 2024
Merged
ycombinator added a commit that referenced this pull request Sep 26, 2024
@ycombinator
Reverts #40684
6055af7 
We're reverting because Elastic Agent CI has been failing and we've narrowed it down to the type assertion failing here and not checking `ok` right after: https://github.com/elastic/beats/blob/138e43cad7eda93c1414641682056b6c88efcf1d/winlogbeat/sys/strings_windows.go#L31-L32

Specifically, when integration tests for Elastic Agent run on its CI Windows hosts, we are seeing this failure in the log:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x284f4bf]

goroutine 1 [running]:
golang.org/x/text/encoding/charmap.(*Charmap).ID(0x0)
        /go/pkg/mod/golang.org/x/text@v0.18.0/encoding/charmap/charmap.go:111 +0x1f
github.com/elastic/beats/v7/winlogbeat/sys.init.0()
        /go/src/github.com/elastic/beats/winlogbeat/sys/strings_windows.go:32 +0x10c
```
ycombinator added a commit that referenced this pull request Sep 26, 2024
@ycombinator
Reverts #40684 (#41009)
307e95c 
We're reverting because Elastic Agent CI has been failing and we've narrowed it down to the type assertion failing here and not checking `ok` right after: https://github.com/elastic/beats/blob/138e43cad7eda93c1414641682056b6c88efcf1d/winlogbeat/sys/strings_windows.go#L31-L32

Specifically, when integration tests for Elastic Agent run on its CI Windows hosts, we are seeing this failure in the log:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x284f4bf]

goroutine 1 [running]:
golang.org/x/text/encoding/charmap.(*Charmap).ID(0x0)
        /go/pkg/mod/golang.org/x/text@v0.18.0/encoding/charmap/charmap.go:111 +0x1f
github.com/elastic/beats/v7/winlogbeat/sys.init.0()
        /go/src/github.com/elastic/beats/winlogbeat/sys/strings_windows.go:32 +0x10c
```
mergify bot pushed a commit that referenced this pull request Sep 26, 2024
@ycombinator @mergify
Reverts #40684 (#41009)
d55de1c 
We're reverting because Elastic Agent CI has been failing and we've narrowed it down to the type assertion failing here and not checking `ok` right after: https://github.com/elastic/beats/blob/138e43cad7eda93c1414641682056b6c88efcf1d/winlogbeat/sys/strings_windows.go#L31-L32

Specifically, when integration tests for Elastic Agent run on its CI Windows hosts, we are seeing this failure in the log:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x284f4bf]

goroutine 1 [running]:
golang.org/x/text/encoding/charmap.(*Charmap).ID(0x0)
        /go/pkg/mod/golang.org/x/text@v0.18.0/encoding/charmap/charmap.go:111 +0x1f
github.com/elastic/beats/v7/winlogbeat/sys.init.0()
        /go/src/github.com/elastic/beats/winlogbeat/sys/strings_windows.go:32 +0x10c
```

(cherry picked from commit 307e95c)
@mergify mergify bot mentioned this pull request Sep 26, 2024
Merged
ycombinator added a commit that referenced this pull request Sep 26, 2024
@mergify @ycombinator
Reverts #40684 (#41009) (#41013)
e268982 
We're reverting because Elastic Agent CI has been failing and we've narrowed it down to the type assertion failing here and not checking `ok` right after: https://github.com/elastic/beats/blob/138e43cad7eda93c1414641682056b6c88efcf1d/winlogbeat/sys/strings_windows.go#L31-L32

Specifically, when integration tests for Elastic Agent run on its CI Windows hosts, we are seeing this failure in the log:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x284f4bf]

goroutine 1 [running]:
golang.org/x/text/encoding/charmap.(*Charmap).ID(0x0)
        /go/pkg/mod/golang.org/x/text@v0.18.0/encoding/charmap/charmap.go:111 +0x1f
github.com/elastic/beats/v7/winlogbeat/sys.init.0()
        /go/src/github.com/elastic/beats/winlogbeat/sys/strings_windows.go:32 +0x10c
```

(cherry picked from commit 307e95c)

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intxgo intxgo intxgo approved these changes

@andrewkroh andrewkroh Awaiting requested review from andrewkroh

Assignees

@marc-gr marc-gr

Labels
backport-8.x Automated backport to the 8.x branch with mergify enhancement Team:Security-Windows Platform Windows Platform Team in Security Solution Winlogbeat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Winlogbeat: Experimental API support missing event data types
5 participants
@marc-gr @elasticmachine @andrewkroh @intxgo @v1v