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] Login dataset: Add event category and type #11339

Merged
merged 4 commits into from
Mar 22, 2019

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Mar 20, 2019

Adds event.category: authentication and event.type: authentication_success (or authentication_failure).

I also took the opportunity to add another unit test for a successful login.

@cwurm cwurm added review needs_backport PR is waiting to be backported to other branches. Auditbeat SecOps labels Mar 20, 2019
@cwurm cwurm requested a review from a team as a code owner March 20, 2019 14:19
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

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.

LGTM

@@ -58,6 +98,7 @@ func TestFailedLogins(t *testing.T) {
config["login.wtmp_file_pattern"] = ""
config["login.btmp_file_pattern"] = "../../../tests/files/btmp_ubuntu1804"
f := mbtest.NewReportingMetricSetV2(t, config)
defer f.(*MetricSet).utmpReader.bucket.DeleteBucket()
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added? IIUC the data directory is removed in between tests so the whole DB gets deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but for some reason that does not cause it to be re-read. Copy pasting one of the test functions causes the second one to fail, every time. I turned on debug logging and it still has the data written in the first function.

I didn't find out exactly why this happens, but I suspect it's because we use a sync.Once here, and the data path will never get re-resolved:

// OpenBucket returns a new Bucket that stores data in {path.data}/beat.db.
// The returned Bucket must be closed when finished to ensure all resources
// are released.
func OpenBucket(name string) (Bucket, error) {
initDatastoreOnce.Do(func() {
ds = &boltDatastore{
path: paths.Resolve(paths.Data, "beat.db"),
mode: 0600,
}
})
return ds.OpenBucket(name)
}

This only really matters in tests as far as I can see, so I didn't dig deeper into it but just added the DeleteBucket() call.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, that's what it is. OpenBucket uses the singleton instance and the DB path is only ever initialized once. The singleton instance is closed when all the users (metricsets) close their buckets. But when it gets reused again it still will have the original path so our cleanup has no effect.

One option would be to have a package global function that the metricsets use to open a bucket. That fn handle could be swapped for testing purposes. We use this pattern in a few places. For example

var openBucket = datastore.OpenBucket

Then for testing swap in a different func.

openBucket = func(name string) (Bucket, error) {
    return datastore.New(paths.Join(testDir, "beat.db"), 0600).OpenBucket(name)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could do that separately. Merging this for now.

@cwurm cwurm force-pushed the login_event_type branch from f3a5921 to ce585c4 Compare March 21, 2019 23:58
@cwurm cwurm merged commit 09a8fa8 into elastic:master Mar 22, 2019
@cwurm cwurm deleted the login_event_type branch March 22, 2019 11:24
cwurm pushed a commit to cwurm/beats that referenced this pull request Mar 22, 2019
Adds `event.category: authentication` and `event.type: authentication_success` (or `authentication_failure`).

(cherry picked from commit 09a8fa8)
@cwurm cwurm added v7.0.0 and removed needs_backport PR is waiting to be backported to other branches. labels Mar 22, 2019
cwurm pushed a commit that referenced this pull request Mar 25, 2019
…ory and type (#11387)

Cherry-pick of PR #11339 to 7.0 branch. Original message: 

Adds `event.category: authentication` and `event.type: authentication_success` (or `authentication_failure`).

I also took the opportunity to add another unit test for a successful login.
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.

4 participants