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.

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

[Auditbeat] Add message field to system module #9483

Merged

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Dec 11, 2018

This adds a top-level message field to the host, process, socket, and user metricsets.

Some example messages:

  • host: Ubuntu host ubuntu-bionic (IP: 10.0.2.15) is up for 0 days, 5 hours, 11 minutes
  • process: Process zsh (PID: 2363) is RUNNING
  • socket: Inbound socket (10.0.2.2:52002 -> 10.0.2.15:22) OPEN by process sshd (PID: 2293) and user root (UID: 0)
  • user: Existing user elastic (UID: 1002, Groups: elastic,docker)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@cwurm cwurm force-pushed the message_everywhere_squashed branch from f8b2403 to df08708 Compare December 11, 2018 17:59
var actionString string
switch action {
case eventActionExistingUser:
actionString = "Existing"
Copy link
Contributor

Choose a reason for hiding this comment

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

These could have been a method on the eventAction type, like Render() or something like that. Just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in general. In this case I prefer this since the value is only ever used right after being filled. A method would separate this flow, and probably to different parts of the file.

return message
}

func fmtDuration(d time.Duration) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Functions like this should get some unit tests. Fine with adding them later in this case, but normally we'd expect them with the PR.

@tsg
Copy link
Contributor

tsg commented Dec 11, 2018

Some unit tests should be added at least for:

  • hostMessage
  • fmtDuration
  • processMessage
  • socketMessage
  • userMessage

Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

Good from my side.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM overall.

Two things I'd like to bring up:

  • Can you use "LISTENING" instead of "OPEN", for socket message? Seems more in line with the system terminology
  • Is it possible to report previous and new host ID, when it changes?

This makes me realize that integration tests will be a challenge. The current state of having many values change around because it's a different run of the VM will make it hard to spot legit differences that are expected or that should be investigated.

Perhaps we could separate the tests in two. Test how the data/events are acquired and do high level checks on them (field names, regexes on values), then have unit tests that start from static states, and ensure they are processed correctly, with very precise diffs like we have right now. This kind of assumes we can split these tests via an intermediate state, however...

But that's for later. Only my two bullet points above need discussion. I'm good if we move forward despite my points, however.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@cwurm cwurm force-pushed the message_everywhere_squashed branch from 31aaa3d to 48d5ac7 Compare December 12, 2018 21:44
@cwurm cwurm merged commit b28a9fb into elastic:feature-auditbeat-host Dec 12, 2018
cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 16, 2018
This adds a top-level `message` field to the `host`, `process`, `socket`, and `user` metricsets.
@cwurm cwurm mentioned this pull request Dec 18, 2018
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants