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

[Auditbeat] Use a separate netlink socket for control to avoid data congestion. #41207

Merged
merged 11 commits into from
Oct 16, 2024
Merged

[Auditbeat] Use a separate netlink socket for control to avoid data congestion. #41207

merged 11 commits into from
Oct 16, 2024

Conversation

nicholasberlin
Copy link
Contributor

@nicholasberlin nicholasberlin commented Oct 11, 2024 •
edited
Loading

Proposed commit message

Use a separate socket for GetStatus.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Run this program while starting Auditbeat. In a separate terminal run while true; do ss -f netlink | grep auditbeat; done

You will see the auditbeat netlink sockets disappear but Auditbeat will continue to run.
Data will stop flowing to Elasticsearch, and an error message will be pushed Elasticsearch, which will be similar to this:
image

With this PR's patch, the netlink sockets will remain, data will flow to Elasticsearch, and no error message will be pushed.

@nicholasberlin nicholasberlin requested a review from a team as a code owner October 11, 2024 16:27
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 11, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 11, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @nicholasberlin? 🙏.
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-8./d is the label to automatically backport to the 8./d branch. /d is the digit

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 11, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 11, 2024
fearful-symmetry
fearful-symmetry reviewed Oct 11, 2024
View reviewed changes
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.

just some small nits

auditbeat/module/auditd/audit_linux.go Outdated
client, err := libaudit.NewAuditClient(nil)
defer func() {
if client != nil {
client.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that if we move the defer statement to after the error check, we don't need the nil check, and we can just do defer client.close()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, makes sense. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a6b5df3

auditbeat/module/auditd/audit_linux.go Outdated
}()

if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we get a fmt.Errorf() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a6b5df3

@andrewkroh andrewkroh added Auditbeat Team:Security-Linux Platform Linux Platform Team in Security Solution labels Oct 14, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 14, 2024
@nicholasberlin
Copy link
Contributor Author

@fearful-symmetry I re-worked the whole thing, please have another look. Thanks.

@fearful-symmetry
Copy link
Contributor

@nicholasberlin looks like mostly linter errors from old code, we need to use value, ok := interface.(type) when you're typecasting.

@nicholasberlin
Copy link
Contributor Author

we need to use value, ok := interface.(type)

Thanks! Will get into it tomorrow.

@nicholasberlin
Copy link
Contributor Author

@fearful-symmetry ready for review now. Thanks again.

@nicholasberlin nicholasberlin merged commit ac81fbd into main Oct 16, 2024
30 checks passed
@nicholasberlin nicholasberlin deleted the nberlin/use_separate_socket_for_control branch October 16, 2024 18:17
mergify bot pushed a commit that referenced this pull request Oct 16, 2024
@nicholasberlin @mergify
[Auditbeat] Use a separate netlink socket for control to avoid data c…
872a91b 
…ongestion. (#41207)

(cherry picked from commit ac81fbd)
@mergify mergify bot mentioned this pull request Oct 16, 2024
Merged
3 tasks
nicholasberlin added a commit that referenced this pull request Oct 16, 2024
@mergify @nicholasberlin
[Auditbeat] Use a separate netlink socket for control to avoid data c…
27a6078 
…ongestion. (#41207) (#41262)

(cherry picked from commit ac81fbd)

Co-authored-by: Nicholas Berlin <56366649+nicholasberlin@users.noreply.github.com>
belimawr pushed a commit to belimawr/beats that referenced this pull request Oct 18, 2024
@nicholasberlin @belimawr
[Auditbeat] Use a separate netlink socket for control to avoid data c…
e6e3f14 
…ongestion. (elastic#41207)
@nicholasberlin nicholasberlin added the backport-8.16 Automated backport with mergify label Oct 24, 2024
@hakong
Copy link

hakong commented Oct 31, 2024

FYI: Searching for this error 'failed to get audit status before adding rules: failed to get audit status ack: error receiving audit reply: no buffer space available' didn't give me any results on Google or Github. The reason being the error message is in a screenshot, not in plaintext.

image
image

I ended up creating a ticket with Elastic and after a few days of messages I was finally told it would be fixed in 8.16. I then had to dig through the MR's for the 8.16 branch to find anything auditd related. That led me to this PR.

Would be good to put the error message in plaintext in the issue/PR/MR so people can actually search for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fearful-symmetry fearful-symmetry fearful-symmetry approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees

@nicholasberlin nicholasberlin

Labels
Auditbeat backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify bugfix Team:Security-Linux Platform Linux Platform Team in Security Solution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nicholasberlin @elasticmachine @fearful-symmetry @hakong @andrewkroh