Auditbeat: `initial_scan` action for new paths #7954

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

@@ -177,6 +179,32 @@ func (ms *MetricSet) init(reporter mb.PushReporterV2) bool {
 	return true
 }

+// Determine which - if any - paths  have been newly added to the config


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

 }

 // NewFileSystemScanner creates a new EventProducer instance that scans the
 // configured file paths.
-func NewFileSystemScanner(c Config) (EventProducer, error) {
+func NewFileSystemScanner(c Config, newP map[string]bool) (EventProducer, error) {


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

@@ -177,6 +179,32 @@ func (ms *MetricSet) init(reporter mb.PushReporterV2) bool {
 	return true
 }

+// findNewPaths determines which - if any - paths  have been newly added to the config


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

-	config Config
+	log      *logp.Logger
+	config   Config
+	newPaths map[string]bool


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

+			continue
+		}
+
+		lastEvent, err := load(ms.bucket, evalPath)


This seems like nice clear solution to the problem. 👍

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

auditbeat/module/file_integrity/action.go

@@ -47,6 +48,7 @@ var actionNames = map[Action]string{
 	Updated:            "updated",
 	Moved:              "moved",
 	ConfigChange:       "config_change",
+	Found:              "found",


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 andrewkroh and adriansr 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auditbeat: `initial_scan` action for new paths #7954

Auditbeat: `initial_scan` action for new paths #7954

cwurm commented Aug 13, 2018

cwurm commented Aug 13, 2018

andrewkroh left a comment

andrewkroh Aug 13, 2018

cwurm Aug 14, 2018

adriansr left a comment

adriansr Aug 13, 2018

cwurm Aug 14, 2018

adriansr commented Aug 14, 2018

cwurm commented Aug 14, 2018

cwurm commented Aug 14, 2018

andrewkroh left a comment •

edited

Loading

andrewkroh Aug 15, 2018

cwurm Aug 16, 2018

andrewkroh Aug 15, 2018

cwurm Aug 16, 2018

andrewkroh Aug 15, 2018

andrewkroh Aug 15, 2018

cwurm Aug 16, 2018

cwurm commented Aug 16, 2018

andrewkroh left a comment

andrewkroh commented Aug 16, 2018

cwurm commented Aug 16, 2018

Auditbeat: initial_scan action for new paths #7954

Auditbeat: initial_scan action for new paths #7954

Conversation

cwurm commented Aug 13, 2018

cwurm commented Aug 13, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansr commented Aug 14, 2018

cwurm commented Aug 14, 2018

cwurm commented Aug 14, 2018

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwurm commented Aug 16, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh commented Aug 16, 2018

cwurm commented Aug 16, 2018

Auditbeat: `initial_scan` action for new paths #7954

Auditbeat: `initial_scan` action for new paths #7954

andrewkroh left a comment •

edited

Loading