[Auditbeat] User dataset: Numerous fixes to error handling #10942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cwurm
added
bug
review
needs_backport
|
Pinging @elastic/secops |
andrewkroh
reviewed
Feb 28, 2019
| } | ||
| for _, user := range users { | ||
| event := ms.userEvent(user, eventTypeState, eventActionExistingUser) | ||
| event.RootFields.Put("event.id", stateID.String()) |
There was a problem hiding this comment.
stateID will be an all zero uuid if uuid.NewV4() failed. What's the impact of this on the metricset and the consumers of this data?
There was a problem hiding this comment.
When sending a list of all users, the event.id is what they have in common, to make it easy for consumers to correlate them. If generating it failed, it would no longer be possible to use it for that purpose - lists of users sent from the same system at different times would have the same (zero) event.id. Instead, they would have to fall back on the less user-friendly timestamp.
It's unlikely to happen, but if it did the dataset would still send all the users, and an error as well.
andrewkroh
approved these changes
Mar 6, 2019
Good point, I've added an entry. |
added 7 commits
March 6, 2019 13:36
andrewkroh
approved these changes
Mar 6, 2019
cwurm
removed
the
needs_backport
cwurm
pushed a commit
to cwurm/beats
that referenced
this pull request
Mar 11, 2019
…0942) Fixes a number of issues in the `user` dataset discovered while testing on Fedora 29: * When calling C functions via cgo, don't rely on errno as an indicator of success - check the return value first. * Set errno to 0 before calling getpwent(3) or getspent(3). * Make C function calls thread-safe. * Address a systemd bug where it can return ENOENT even when there is no error. * Don't fail on every error. * Fail system test on WARN/ERROR messages in the log. (cherry picked from commit 8ce5440)
cwurm
pushed a commit
to cwurm/beats
that referenced
this pull request
Mar 11, 2019
…0942) Fixes a number of issues in the `user` dataset discovered while testing on Fedora 29: * When calling C functions via cgo, don't rely on errno as an indicator of success - check the return value first. * Set errno to 0 before calling getpwent(3) or getspent(3). * Make C function calls thread-safe. * Address a systemd bug where it can return ENOENT even when there is no error. * Don't fail on every error. * Fail system test on WARN/ERROR messages in the log. (cherry picked from commit 8ce5440)
cwurm
pushed a commit
to cwurm/beats
that referenced
this pull request
Mar 11, 2019
…0942) Fixes a number of issues in the `user` dataset discovered while testing on Fedora 29: * When calling C functions via cgo, don't rely on errno as an indicator of success - check the return value first. * Set errno to 0 before calling getpwent(3) or getspent(3). * Make C function calls thread-safe. * Address a systemd bug where it can return ENOENT even when there is no error. * Don't fail on every error. * Fail system test on WARN/ERROR messages in the log. (cherry picked from commit 8ce5440)
cwurm
pushed a commit
that referenced
this pull request
Mar 12, 2019
…11190) Fixes a number of issues in the `user` dataset discovered while testing on Fedora 29: * When calling C functions via cgo, don't rely on errno as an indicator of success - check the return value first. * Set errno to 0 before calling getpwent(3) or getspent(3). * Make C function calls thread-safe. * Address a systemd bug where it can return ENOENT even when there is no error. * Don't fail on every error. * Fail system test on WARN/ERROR messages in the log. (cherry picked from commit 8ce5440)
cwurm
pushed a commit
that referenced
this pull request
Mar 12, 2019
…11189) Fixes a number of issues in the `user` dataset discovered while testing on Fedora 29: * When calling C functions via cgo, don't rely on errno as an indicator of success - check the return value first. * Set errno to 0 before calling getpwent(3) or getspent(3). * Make C function calls thread-safe. * Address a systemd bug where it can return ENOENT even when there is no error. * Don't fail on every error. * Fail system test on WARN/ERROR messages in the log. (cherry picked from commit 8ce5440)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing the
userdataset on Fedora 29, I noticed it's not working. Digging into it, there were a few issues, all of which this PR fixes:errnois going to be zero on success (see here for some detail). Instead, we should check the return value first, and if that indicates that there might have been an error (usually when it'sNULL/nil) we should check the error return.If one wants to check errno after the call, it should be set to zero before the call.errnocannot be accessed directly in Go code, so we introduce a helper functionsetErrnofor that.getspent(3)does not have the same warning, but given that at least glibc uses the same code path for both, it's reasonable to assume the same applies. So we seterrnoto0before calling both functions.errnois thread-local, and the function familiessetpwent/endpwent/getpwentandsetspent/endspent/getspentare not thread-safe, so we introduceruntime.LockOSThread()/UnlockOSThread()to make sure all C functions are run on the same OS thread.getpwent()should returnNULLanderrnoshould be zero when all entries have been returned. However, there is a bug in systemd that causes it to seterrnotoENOENTeven when there is no error (nss-systemd's getpwent sets errno to ENOENT systemd/systemd#9585). This bug affects at least Fedora 29. Following this changeENOENTis treated as if there is no error. It's not supposed to be a valid error value forgetpwent()anyway, so should not happen anytime outside this bug.userdataset to return no users, even though all users had been read successfully. This PR changes to gathering errors in multierrors and returning them alongside whatever data could be read. This is in line with wanting to make the System module more resilient to non-fatal failures during data collection (better to return some things alongside an error than no things at all).WARN/ERRORmessages in the log. This change can probably also be made for the other datasets.Writing up this PR description I'm realizing this combines quite a few issues. I could split it into multiple PRs if anybody wants, though most would have only a few lines of changed code. Because some indentation changed it's best to check Github's
No Whitespacebutton.