Auditbeat file module improvements #4636

andrewkroh · 2017-07-10T04:54:27Z

Unify the behavior across operating systems.

Add tests.
Resolve UID/GID/SID into owner/group.
Log warning if a file watch cannot be added. Previously it would fail at startup.
Remove usage of fsevents on macOS and use kqueues instead. The data provided was inconsistent with the fsnotify. It was valuable data but the signal needs some refinement to make it consistent. The biggest issue is that multiple events are coalesced into one. Another issue is that the watches are recursive (this is great, but it's inconsistent) and we will need to allow a glob to be specified then match the incoming events against the glob (future enhancement).
Document hash fields
Initialize watches on Start() rather than at construction time.
Update data.json to include file owner and group names.
Add file type and symlink target path to the events.
Filter fsnotify events with empty paths (saw this once on Windows).

And add a simple dashboard:

Unify the behavior across operating systems. - Add tests. - Resolve UID/GID/SID into owner/group. - Log warning if a file watch cannot be added. Previously it would fail at startup. - Remove usage of fsevents on macOS and use kqueues instead. The data provided was inconsistent with the fsnotify. It was valuable data but the signal needs some refinement to make it consistent. The biggest issue is that multiple events are coalesced into one. Another issue is that the watches are recursive (this is great, but it's inconsistent) and we will need to allow a glob to be specified then match the incoming events against the glob (future enhancement). - Document hash fields - Initialize watches on Start() rather than at construction time. - Update data.json to include file owner and group names. - Add file type and symlink target path to the events. - Filter fsnotify events with empty paths (saw this once on Windows).

ruflin

LGTM. Most comments are not directly related to this PR and things that popped up in my head when reading it.

ruflin · 2017-07-11T19:46:38Z

auditbeat/module/audit/file/event.go

+		} else {
+			m["uid"] = info.UID
+			m["gid"] = info.GID
+			m["mode"] = fmt.Sprintf("%#o", uint32(info.Mode))


Why do you use # here?

This will "add leading 0 for octal (%#o)". https://golang.org/pkg/fmt/

I was wanting something like 0644, but maybe a different format string would be more appropriate because what I really want is a minimum of 4 octal digits.

ruflin · 2017-07-11T19:49:03Z

auditbeat/module/audit/file/eventreader_fsnotify.go

+	}
+
+	for _, p := range paths {
+		if err := r.watcher.Add(p); err != nil {


Every time I see this I'm thinking if we should add a new prospector type in filebeat based on inotify which only works with "most" OS. Could make to code a lot simpler.

Yeah, this fsnotify package seems to work pretty well. The only downside is that you can't do recursive monitoring. Recursive monitoring is supported by Windows but this isn't exposed by fsnotify (the other OSes don't support it).

ruflin · 2017-07-11T19:50:21Z

auditbeat/module/audit/file/eventreader_fsnotify.go

+
+func convertToFileEvent(e fsnotify.Event, maxFileSize int64) Event {
+	event := Event{
+		Timestamp: time.Now().UTC(),


We should introduce a helper in common to get time.Now() that always includes UTC. I think there are several places where it's missing.

That reminds me, I think I will open a PR to automatically convert all time.Time objects to UTC during event normalization. Then we won't need a helper because it won't matter.

I opened #4658.

ruflin · 2017-07-11T19:52:35Z

auditbeat/module/audit/file/fileinfo_bsd.go

 	if err != nil {
-		return errors.Wrap(err, "failed to stat file")
+		return nil, errors.Wrap(err, "failed to stat path")


Would be nice to include the "failed" path in the error

Stat/Lstat return a *os.PathError that includes the path already so it would be redundant.

ruflin · 2017-07-11T19:53:01Z

auditbeat/module/audit/file/fileinfo_bsd.go

 	}

 	stat, ok := info.Sys().(*syscall.Stat_t)
 	if !ok {
-		return errors.Errorf("unexpected fileinfo sys type %T", info.Sys())
+		return nil, errors.Errorf("unexpected fileinfo sys type %T", info.Sys())


perhaps also include the path?

ruflin · 2017-07-11T19:54:13Z

auditbeat/module/audit/file/fileinfo_bsd.go

+		GID:   stat.Gid,
+		Mode:  info.Mode(),
+		Size:  info.Size(),
+		ATime: time.Unix(0, stat.Atimespec.Nano()).UTC(),


I know it was in before, but I would assume Micro should be enough.

Not sure I follow... the time.Unix() function accepts nanoseconds which is why I'm using Nano().

ruflin · 2017-07-11T19:55:01Z

auditbeat/module/audit/file/fileinfo_linux.go

 	if err != nil {
-		return errors.Wrap(err, "failed to stat file")
+		return nil, errors.Wrap(err, "failed to stat path")


See comments above for logging, also line 22.

ruflin · 2017-07-11T19:56:02Z

auditbeat/module/audit/file/fileinfo_linux.go

 	}

-	return nil
+	switch {


Looks like at least some of the code could be shared between linux and bsd (did not do the detailed comparison, devil is probably in the detail.

It really could be, and I was debating this. The only difference between the BSD and Linux variants is that the stat_t structs use different member names for the time values. I could create a fileinfo_posix.go that's common to BSD + Linux. Then have a fileinfo_linux.go and fileinfo_bsd.go that provide a helper method for reading the time values from the stat_t objects. I'll try it out, but I'm kind of thinking that the complexity out weighs the value.

I changed this to reduce duplication.

ruflin · 2017-07-11T19:59:12Z

auditbeat/module/audit/file/metricset_test.go

+		return
+	}
+
+	info1, err := os.Stat(f1)


Should you use LStat here? if one of the params is a symlink to the file, do you expect this check to return true or false?

Yes, will fix.

- Change mode format to %#04o to always get a leading 0 and a minimum of 4 digits. - Change Stat of Lstat in assertSameFile - Reduce code duplication between linux and bsd platforms.

andrewkroh · 2017-07-12T18:32:03Z

@ruflin I addressed the review feedback.

andrewkroh added Auditbeat review labels Jul 10, 2017

andrewkroh added 2 commits July 10, 2017 14:13

Add auditbeat file dashboard

bb7e1dc

andrewkroh force-pushed the feature/ab/audit-file-tests branch from 9c6d0d2 to bb7e1dc Compare July 10, 2017 18:14

andrewkroh added the v6.0.0-beta1 label Jul 10, 2017

ruflin approved these changes Jul 11, 2017

View reviewed changes

Address review feedback

9d992af

- Change mode format to %#04o to always get a leading 0 and a minimum of 4 digits. - Change Stat of Lstat in assertSameFile - Reduce code duplication between linux and bsd platforms.

ruflin merged commit 211d252 into elastic:master Jul 13, 2017

andrewkroh deleted the feature/ab/audit-file-tests branch April 20, 2018 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auditbeat file module improvements #4636

Auditbeat file module improvements #4636

andrewkroh commented Jul 10, 2017 •

edited

Loading

ruflin left a comment

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

andrewkroh Jul 12, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

ruflin Jul 11, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

ruflin Jul 11, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

andrewkroh Jul 12, 2017

ruflin Jul 11, 2017

andrewkroh Jul 11, 2017

andrewkroh commented Jul 12, 2017

Auditbeat file module improvements #4636

Auditbeat file module improvements #4636

Conversation

andrewkroh commented Jul 10, 2017 • edited Loading

ruflin left a 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

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

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

andrewkroh commented Jul 12, 2017

andrewkroh commented Jul 10, 2017 •

edited

Loading