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 system module #9546

Merged
merged 30 commits into from
Dec 17, 2018
Merged

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Dec 14, 2018

Merges the feature-auditbeat-host branch into master.

Note: This is going off cwurm:feature-auditbeat-host simply so that I don't have to push any further commits directly into upstream. cwurm:feature-auditbeat-host and elastic:feature-auditbeat-host are exactly the same at this point in time.

andrewkroh and others added 22 commits September 18, 2018 08:47
This adds an skeleton x-pack module to Auditbeat. The module is only included in the Elastic licensed Auditbeat binary.

The config and fields.yml data are not yet included in the packaging. Additional updates are required.
Adds host, packages, and processes metricsets to Auditbeat.

Host collects general host information, e.g. boottime, timezone, OS, network interfaces.

Packages collects information about installed packages. For now, it supports debian and homebrew on darwin.

Processes collects information about currently running, started, and stopped processes.
Changes necessary to make `mage fields` and `make system-tests`
work again after merging master. (And deleting a misplaced parenthesis.)
Collects (via C functions) user information from /etc/passwd, /etc/shadow,
and /etc/group on Linux.

Detects new users, deleted users, changes to users (e.g. groups), and - 
as a special distinct category - password changes.

Sends periodic state information about all users (frequency can be controlled).
Otherwise, periodically checks the ctime of the above files, reads them if
the ctime has changed, detects changes compared to its internal cache,
and reports any changes.

The cache is persisted to disk in a `beat.db` file (already used by
the `file_integrity` module) after every `Fetch` and on `Close`. It contains a copy
of all current user information incl. a SHA-512 hash of the password hash from
/etc/shadow (to detect password changes between Auditbeat restarts - this hash
is not sent to any output).
Collects information about open sockets (Linux only).

Uses netlink to query for all currently open sockets. Sends information about all sockets on start, and periodically as determined by `state.period`. Otherwise, sends only newly opened or closed sockets. The sockets are enriched with process and user information.
Adds additional build flags to the new user metricset to prevent build failures on non-Linux systems. If it is configured, it will now throw an error and abort the launch.
Updates the `process` metricset to follow newest conventions:

- Rename from `processes` to `process`
- Change to single documents instead of arrays
- Scheduled state reporting
- Use top-level ECS fields
* Refactor Auditbeat build logic

Update auditbeat and x-pack/auditbeat to share logic for generating config and packages.
This makes auditbeat and x-pack/auditbeat have independent `package` targets where auditbeat
generates only OSS packages and x-pack/auditbeat generates Elastic licensed packages.

And x-pack/auditbeat will now be tested on Travis CI.

* Skip failing Auditbeat system module test
* Skip failing system/process test
* Add temporary target alias for Windows CI
* Fix file permission issues caused by Docker usage
* Optimize chown by checking if UID/GID need changed
Updates the `host` metricset to be in line with the other metricsets in the `system` module:

1. Adds regular state reporting based on `state.period`/`host.state.period`
2. Persists state between restarts in `beat.db`
3. Detects changes in host information
4. Changes to using `system.host.ip`/`system.host.mac` instead of `system.host.network.interfaces`
Introduces a `user.detect_password_changes` config parameter that defaults to true in the config, but false in the code. Only if it is set to true will the code read the password field in /etc/passwd and /etc/shadow to detect password changes.

The read password field values are put through 10 round of SHA-512 hashing before being locally stored.
To be compatible with ECS, changes the `event.type` field to `event.kind` throughout the system module.
The `packages` metricset is not yet ready. This disables the metricset's code, tests, fields, and docs until we have time to finish the work.
Namespaces all Auditbeat system module metricsets to `system.audit` to avoid any potential field clashes with Metricbeat.
…stic#9500)

Setting `auditbeat.max_start_delay: 0` for system tests greatly reduces their execution time.

(cherry picked from commit 7a2cf0f)
This adds a top-level `message` field to the `host`, `process`, `socket`, and `user` metricsets.
Adds config and asciidoc documentation for the four metricsets of the system module that are ready today: host, process, socket, user.

Also adjusts the doc generation to include files from x-pack/auditbeat.
Allow the `process` metricset to run as any user by catching permission errors when trying to read other user's private process information.
@cwurm cwurm requested a review from a team as a code owner December 14, 2018 11:37
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@cwurm
Copy link
Contributor Author

cwurm commented Dec 14, 2018

Ok, so some things that need fixing:

  1. auditbeat.yml and auditbeat.reference.yml have lost some newlines, I'll find a way to re-add them.
  2. We'll need to disable docs references to x-pack/auditbeat for now, until that directory is added to the docs build (similar to Beats: add x-pack/filebeat/docs to contents docs#507).

Plus whatever is making the build fail.

@cwurm cwurm added the review label Dec 14, 2018
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.

Were some of the files more problematic when rebasing on top of master?

I think this looks good overall. I'm in favor of merging sooner than later, then following up with smaller PRs where needed.

  • I noticed two unexpected files with extension .disabled (see the review). These two files are the only thing I'd like to get an answer about, to understand if it's a problem with this PR.
  • Left a few comments about ECS & field naming. I think we should address those in a separate PR.
  • I didn't remember you had modified Metricbeat in this branch, but I realized that this was from [Auditbeat] Socket metricset #8834. Makes sense.

LGTM

auditbeat/scripts/docs_collector.py Show resolved Hide resolved
},
"system": {
"audit": {
"host": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocker for this PR, I think we should merge it sooner than later.

But we'll need to revisit this wrt ECS. Almost all keys under system.audit.host map perfectly to ECS at the top level. So they should be moved there.

The exceptions I see are system.audit.host.uptime and system.audit.host.timezone.offset.sec. And lastly, the system.audit.host.timezone.name should become event.timezone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The top level host object is populated by the add_host_metadata processor. That processor is using a cache and so will not report the most up-to-date information. What would happen if we didn't include our own data here is that a change event would not actually have any changed data - it would still report the old state because of the cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very interesting edge case. It's way outside of the scope of this PR to fix, of course.

But the end goal with these field names is to have this data in the same fields, because the we want to allow for correlation across indices, and for things to be named as expected, and so on.

The fact that at this stage, the processor would override this is a very real problem that we'll need to work around, however. Not sure what the best course of action would be here. Modify Auditbeat to prevent this from happening? Or modify the processor to consider Auditbeat differently and never override the fields?

cc @ruflin

"working_directory": "/home/elastic"
},
"service": {
"type": "system"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also add this to a follow-up PR for mapping to ECS. But service.type is meant to describe the name of a concrete application being monitored by the agent. The fact that this is a system monitoring event is already captured by event.module:system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this comes from the Metricbeat framework we're using in Auditbeat. I think it's set for all of Metricbeat and Auditbeat.

I think we should address it separately and more broadly - I assume all other metricsets have an event.module as well (e.g. event.module: mysql) that already says what it's monitoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right about event.module: #8879

"@timestamp": "2017-10-12T08:05:34.853Z",
"agent": {
"hostname": "host.example.com",
"name": "host.example.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

agent.type:auditbeat, with agent.name defaulting to the same value, or absent. I'd go for absent in the case of Auditbeat. This pattern around .name needs to be better documented (see elastic/ecs#269).

agent.hostname should instead be host.hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are standard values that the testing framework puts in. The actual data has:

"agent": {
    "hostname": "5efeb8ba7b09",
    "version": "7.0.0",
    "type": "auditbeat"
  },

We should probably adjust the testing framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha.

This snippet you pasted is what live Auditbeat generates, though? If that's the case, agent.hostname still needs to move to host.hostname.

The definition of agent is that it's a monitor that runs on the host, so there's no need for hostname there. This is in contrast to observer, which is defined as a separate host or device that monitors a system from the outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. This is in master today (introduced last month with #8873) and affects all Beats - we should address it separately.

@andrewkroh
Copy link
Member

I started a packaging test at
https://beats-ci.elastic.co/job/elastic+beats+pull-request+package/ (job 12).

If that’s taking too long to get a macos worker then you could also test it locally with

cd elastic/beats
PLATFORMS=+linux/armv7 +linux/ppc64le +linux/s390x +linux/mips64 make snapshot

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I wouldn’t worry about the removed newline in auditbeat.yml. That’s not going to cause any problems.

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.

Reiterating my LGTM after recent discussion.

return filepath.Walk(pkgFile.Source, func(path string, info os.FileInfo, err error) error {
// filepath.Walk() does not resolve symlinks, but pkgFile.Source might be one,
// see mage.KibanaDashboards().
resolvedSource, err := filepath.EvalSymlinks(pkgFile.Source)
Copy link
Member

Choose a reason for hiding this comment

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

This will prevent the usage of symlinks in packages. I’d rather have packages declare exactly want they include. There is a helper method to update the package manifest for build/kibana. Eventually we won’t need that at all because all beats will be the same and use build/kibana.

https://github.com/elastic/beats/blob/feature-auditbeat-host/dev-tools/mage/kibana.go#L95

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will prevent the usage of symlinks in packages.

Sorry, I don't follow. This was meant to allow PackageFile.Source to be a symlink to a directory. This is never the case at the moment - when I changed it to create a symlink from _meta/kibana.generated to build/kibana the build failed and I had to make the change here to allow it. So I'm not sure how it prevents symlinks?

The problem is that make snapshot depends on make beats-dashboards (via make release) which is implemented as mage packageBeatDashboards which assumes all Beats have a _meta/kibana.generated directory:

https://github.com/elastic/beats/blob/feature-auditbeat-host/magefile.go#L64

for _, beat := range Beats {
		spec.Files[beat] = mage.PackageFile{
			Source: filepath.Join(beat, "_meta/kibana.generated"),
		}
	}

	return mage.PackageZip(spec.Evaluate())

This would have to be changed to take Beats specifics into account. E.g. Auditbeat produces a build/kibana directory, while Filebeat still has _meta/kibana.generated. I believe PackageKibanaDashboardsFromBuildDir() just changes everything, regardless of what each Beat needs. So I'm not sure if we can currently do this without a bigger effort? Symlinks seemed like the simpler way.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn’t realize this was solving for the beats dashboards zip case. (one the challenges of reviewing from a phone)

No one is adding symlinks via packages afaik (e.g. like installing a symlink on the system that is owned by an rpm/deb package) and I’m not sure that works anyways with our cade. So don’t consider this a problem. And once all Beats are writing dashboards to build/kibana this won’t be necessary anyways.

However this does make me wonder if dashboards from x-pack/auditbeat (or x-pack/filebeat) are being bundled in the beat dashboard zip. I suspect not, but that’s also not a problem for this PR to solve.

@cwurm
Copy link
Contributor Author

cwurm commented Dec 16, 2018

I started a packaging test at
beats-ci.elastic.co/job/elastic+beats+pull-request+package (job 12).

@andrewkroh I've started another job, but it ran out of disk space (/usr/lib/ruby/2.3.0/fileutils.rb:1394:in `copy_stream': No space left on device - sendfile (Errno::ENOSPC)). Can we increase the disk size?

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

There's one merge conflict (probably due to a Go version change).

@cwurm cwurm merged commit aebcf9c into elastic:master Dec 17, 2018
@cwurm cwurm mentioned this pull request Dec 18, 2018
21 tasks
cwurm pushed a commit that referenced this pull request Dec 18, 2018
Adds the system module to Auditbeat, with four metricsets: host, process, socket, and user.

Host collects general host information, e.g. boottime, timezone, OS, network interfaces. Processes collects information about currently running, started, and stopped processes. Socket collects information about open sockets. User detects new users, deleted users, changes to users (e.g. groups), and - as a special distinct category - password changes.
@cwurm cwurm added the v6.6.0 label Dec 18, 2018
cwurm pushed a commit that referenced this pull request Jan 7, 2019
Fixes a bug introduced with #9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes #9785.
kvch pushed a commit to kvch/beats that referenced this pull request Jan 7, 2019
Fixes a bug introduced with elastic#9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes elastic#9785.
kvch pushed a commit to kvch/beats that referenced this pull request Jan 7, 2019
Fixes a bug introduced with elastic#9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes elastic#9785.
(cherry picked from commit 3a51720)
kvch added a commit that referenced this pull request Jan 7, 2019
Fixes a bug introduced with #9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes #9785.
kvch added a commit that referenced this pull request Jan 7, 2019
Fixes a bug introduced with #9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes #9785.
(cherry picked from commit 3a51720)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Fixes a bug introduced with elastic#9546 where a symlink from `_meta/kibana.generated` to `build/kibana` would cause objects to be included in the dashboard ZIP file with the wrong path. This removes the symlink in favor of a conditional.

Fixes elastic#9785.
(cherry picked from commit 23f1ea2)
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.

5 participants