Auditbeat: `initial_scan` action for new paths by cwurm · Pull Request #7954 · elastic/beats

cwurm · 2018-08-13T12:42:38Z

Introduces a new found action that is used by the scanner on startup (when scan_at_start: true) to mark files found in configured paths that it has not seen before.

Addresses #7821

cwurm · 2018-08-13T12:43:15Z

A bit more detail: The new logic follows Option 2 laid out in a comment in the original issue. Before the scanner is called, the Metricset checks which paths from the config are not contained in the db and are therefore assumed to be new. It passes this list to the scanner which allows it to correctly mark new dirs and files it sees as either found (when they are in a new path) or created (when they are in a previously known path).

andrewkroh

I'll try to take a deeper look at this today. Thanks for opening a PR!

andrewkroh · 2018-08-13T16:52:02Z

auditbeat/module/file_integrity/metricset.go

The comments are great. I just want to encourage you to use the godoc format (even for private methods) which is to start the sentence with the name of the method (e.g. findNewPaths finds the new paths...).

Thanks for the pointer, I fixed it here and will try to remember going forward.

adriansr

Just one minor comment, otherwise LGTM.

adriansr · 2018-08-13T16:34:25Z

auditbeat/module/file_integrity/scanner.go

Can you update the function's comment to reflect the meaning of this parameter and maybe give it a more descriptive name?

adriansr · 2018-08-14T10:45:57Z

There's an issue with the CLA related to the e-mail address you used for this commits. I guess it can be fixed by associating this e-mail address with your Github account

cwurm · 2018-08-14T11:48:47Z

There's an issue with the CLA related to the e-mail address you used for this commits. I guess it can be fixed by associating this e-mail address with your Github account

Thanks, fixed it.

Test failed in CI that didn't fail locally, I'm not sure why. Looking into it.

cwurm · 2018-08-14T14:34:32Z

Test failed in CI that didn't fail locally, I'm not sure why. Looking into it.

Found it: When the test runs, there's a race about whether the changes are found by the scanner or by filesystem notification (both are started at the same time). The scanner usually wins, and the actions it sets is what the test was originally written for. The filesystem reports slightly different actions (e.g. created|updated instead of created|attributes_modified). I've changed the test to only test for the common actions (e.g. created).

It's a bit of a shame though that the reported actions are not deterministic, and maybe this is something that we can normalize in the future.

I've pushed a commit, let's hope the CI is good with it. It's a non-deterministic failure, so it's a bit tricky. I tested locally with -run 1000 to be sure.

andrewkroh

I like the solution you reached here. I left a few comments.

andrewkroh · 2018-08-15T15:41:32Z

auditbeat/module/file_integrity/metricset.go

There's a double space between paths have. And it should have a period to end the sentence.

Thanks, fixed.

andrewkroh · 2018-08-15T15:57:18Z

auditbeat/module/file_integrity/scanner.go

When using a map as a set data structure you can use map[string]struct{}. To me this is more clear because it's saying that you don't care about the value and that when you use map you will only be checking whether the key exists (rather than that the key exists and its value is true). Also it may save a few bytes.

m := map[string]struct{}{} // Init. m[name] = struct{} // Add. if _, exists := m[name]; exists { // Contains. // Exists! }

Oh nice, I like that it takes 0 bytes - done.

andrewkroh · 2018-08-15T15:58:36Z

auditbeat/module/file_integrity/metricset.go

This seems like nice clear solution to the problem. 👍

andrewkroh · 2018-08-15T16:11:41Z

auditbeat/module/file_integrity/action.go

So this flag indicates that the event was generated due to the initial scan? If so how about we call it initial_scan rather than found?

Makes sense - changed it.

cwurm · 2018-08-16T15:34:44Z

Thanks @andrewkroh - all implemented. Travis is unhappy, but it doesn't look related.

andrewkroh

LGTM.

andrewkroh · 2018-08-16T16:39:51Z

@cwurm A few notes on your commit message:

The first line should be a short summary.
Follow that by a blank line.
Then do your detailed description.
If you want the related issue to auto-close on merge (your choice) then use one of the keywords recognized by GH.

When `scan_at_start: true` the filesystem scanner will look through all paths and record any new or modified files and directories it finds. This commit introduces a new action `initial_scan` that the scanner uses when it scans a path for the first time. Previously, the scanner would report everything under a new path as `created`, making it impossible to distinguish between files and directories that are truly new (e.g. have been added during a restart of Auditbeat) and those that are merely new because they are under a newly added path in the configuration. Resolves elastic#7821

cwurm · 2018-08-16T16:58:30Z

Thanks @andrewkroh - I've changed it to follow the format and made it more descriptive. Let me know if anything is still missing.

cwurm added review Auditbeat labels Aug 13, 2018

cwurm requested review from adriansr and andrewkroh August 13, 2018 12:42

cwurm mentioned this pull request Aug 13, 2018

Add the ability to prevent auditbeat's file_integrity module from sending all monitoring files in an initial "created" status #7821

Closed

ph mentioned this pull request Aug 13, 2018

X-Pack binaries #7783

Merged

andrewkroh reviewed Aug 13, 2018

View reviewed changes

adriansr reviewed Aug 13, 2018

View reviewed changes

andrewkroh reviewed Aug 15, 2018

View reviewed changes

cwurm force-pushed the auditbeat_found_status branch from 9db5594 to 2cb655c Compare August 16, 2018 14:07

cwurm changed the title ~~Auditbeat: found action for new paths~~ Auditbeat: initial_scan action for new paths Aug 16, 2018

andrewkroh approved these changes Aug 16, 2018

View reviewed changes

cwurm force-pushed the auditbeat_found_status branch from 9a647ef to a1b5e8a Compare August 16, 2018 16:10

cwurm force-pushed the auditbeat_found_status branch from a1b5e8a to de677c8 Compare August 16, 2018 16:56

andrewkroh merged commit 95e53bc into elastic:master Aug 16, 2018

cwurm added the v7.0.0-alpha1 label Sep 17, 2018

Conversation

cwurm commented Aug 13, 2018

Uh oh!

cwurm commented Aug 13, 2018

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriansr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriansr commented Aug 14, 2018

Uh oh!

cwurm commented Aug 14, 2018

Uh oh!

cwurm commented Aug 14, 2018

Uh oh!

andrewkroh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwurm commented Aug 16, 2018

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkroh commented Aug 16, 2018

Uh oh!

cwurm commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrewkroh left a comment •

edited

Loading