Skip to content

[Auditbeat] System module documentation#9512

Merged
cwurm merged 19 commits intoelastic:feature-auditbeat-hostfrom
cwurm:system_document_config
Dec 14, 2018
Merged

[Auditbeat] System module documentation#9512
cwurm merged 19 commits intoelastic:feature-auditbeat-hostfrom
cwurm:system_document_config

Conversation

@cwurm
Copy link
Copy Markdown
Contributor

@cwurm cwurm commented Dec 12, 2018

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 work with files from x-pack/auditbeat. The result is still written to auditbeat/docs, but depending on whether mage docs or mage update is run in auditbeat or x-pack/auditbeat the modules documentation will contain the system module (or any other modules in the future) or not.

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/secops

@cwurm cwurm force-pushed the system_document_config branch 3 times, most recently from 18a294b to d2f064d Compare December 12, 2018 22:17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cwurm Could we use the same script also for metricbeat?

@kaiyan-sheng @sayden In case one of you is already working on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ruflin They look rather similar for sure, though there are also some differences.

@andrewkroh might know more about why they are separate?

@tsg
Copy link
Copy Markdown
Contributor

tsg commented Dec 13, 2018

@dedemorton @karenzone FYI, if one of you wants to review the text here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add here a warning box to explicitly call out the danger of dictionary attacks on beat.db?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I wouldn't. The scenario is very remote, where an attacker would have to reverse with exact collisions 10 successive rounds of SHA-512 hashes, and then would still have to break the original salt and hash combination.

It seems more likely to needlessly scare users that are not familiar with how passwords and hashes work.

But I'm open to other opinions here. @joshbressers maybe?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If someone has root on the system, they have access to the hash directly from passwd, and they have access to a hashed version of the Linux hash in beats.db. Leveraging what's in beats.db is more difficult than leveraging the standard passwd/shadow files. So brute force is not the risk I would warn about.

Perhaps instead we should insist on the fact that this file now contains a copy of the hashes. So users should ensure it's as well scrutinized (e.g. FIM) as /etc/passwd and /etc/shadow, and that the file is not copied around lightly (e.g. for diagnostics, misconfigured backups).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about adding this sentence:

The beat.db file should be readable only by the root user and be treated similar to the shadow file itself.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this works.

Copy link
Copy Markdown
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.

Docs look reasonable to me. Thanks!

Copy link
Copy Markdown
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.

A few minor issues. I'm only commenting on the documentation itself. I'm not the best person to review the code here (although the changes looked ok to me).

  • Suggesting a few ways to clarify the doc
  • Suggestion for a different security/auditing warning
  • A few nits, feel free to ignore those

Overall I'm good with this PR.

I think in all cases, the script should generate the full doc including x-pack. It shouldn't depend on which directory it's being run from. This can be addressed as a separate PR, however.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can remove "for controlling its behavior".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I copied it from auditd which says "tuning its behavior". I agree the whole sentence doesn't add a lot of value - the title already says what this section is about. But anyway, I think I like this flow better than if it just said "This module has some configuration options. The
following example shows all configuration options ...". Seems abrupt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right LOL, that's true 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If someone has root on the system, they have access to the hash directly from passwd, and they have access to a hashed version of the Linux hash in beats.db. Leveraging what's in beats.db is more difficult than leveraging the standard passwd/shadow files. So brute force is not the risk I would warn about.

Perhaps instead we should insist on the fact that this file now contains a copy of the hashes. So users should ensure it's as well scrutinized (e.g. FIM) as /etc/passwd and /etc/shadow, and that the file is not copied around lightly (e.g. for diagnostics, misconfigured backups).

@cwurm
Copy link
Copy Markdown
Contributor Author

cwurm commented Dec 13, 2018

Thanks @webmat I've incorporated most feedback.

The mage docs command now generates combined documentation for both OSS and X-Pack parts in auditbeat/docs, with links to x-pack/auditbeat/docs as required.

Copy link
Copy Markdown
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

@karenzone
Copy link
Copy Markdown
Contributor

@dedemorton I'm experiencing build errors, and wonder if they could be related to generated IDs:
IDREF attribute linkend references an unknown ID "exported-fields-system". I'd like to get your expertise. In the meantime, I'll focus on content. Thanks!

@cwurm
Copy link
Copy Markdown
Contributor Author

cwurm commented Dec 13, 2018

@karenzone Sorry, the system module fields were missing from fields.asciidoc. Should be fixed now, at least it's building for me locally.

@dedemorton
Copy link
Copy Markdown
Contributor

Did you remember to add fields.yml? make update fails for me with IOError: [Errno 2] No such file or directory: '/Users/dedemorton/go/src/github.com/elastic/beats/x-pack/auditbeat/fields.yml'

@cwurm
Copy link
Copy Markdown
Contributor Author

cwurm commented Dec 13, 2018

@dedemorton Ah, you're right. Now that mage update builds the combined documentation it also needs an up-to-date combined fields.yml. I've added a dependency.

@cwurm cwurm force-pushed the system_document_config branch from d972baa to 0bbeaed Compare December 14, 2018 10:14
@cwurm cwurm merged commit 4f0e3fa into elastic:feature-auditbeat-host Dec 14, 2018
@cwurm
Copy link
Copy Markdown
Contributor Author

cwurm commented Dec 14, 2018

I've merged this now so we can get the feature branch merged into master. I assume we will have to do more work on the documentation - I found one or two things myself when browsing through before the merge - but we can do these on master.

cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 16, 2018
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.
@cwurm cwurm mentioned this pull request Dec 18, 2018
21 tasks
@cwurm cwurm mentioned this pull request Jan 16, 2019
10 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.

7 participants