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

Add ECS 1.8 session and map user and groups #86

Merged
merged 15 commits into from Feb 3, 2021

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Jan 20, 2021

Add ECS user and group fields to aucoalesce.Event:

  • Update normalizations to enable population of ECS user.* and group.*
    fields from event data (entity., object., uids and raw data).
  • Refactors the user/group ID lookup as sometimes it's necessary to
    lookup a user/group ID from a name.

Adds the new ECS session categorization to USER_START and USER_END messages.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 20, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #86 updated

    • Start Time: 2021-02-02T15:20:51.129+0000
  • Duration: 2 min 2 sec

  • Commit: 9113b6a

Test stats 🧪

Test Results
Failed 0
Passed 365
Skipped 40
Total 405

Add ECS user and group fields to aucoalesce.Event.

Update normalizations to enable population of ECS user.* and group.*
fields from event data (entity.*, object.*, uids and raw data).

Also refactors the user/group ID lookup as sometimes it's necessary to
lookup a user/group ID from a name.
Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

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

I'll work through a bit more of a detailed review in a bit, but general approach looks good to me, I did have a question about other effective/target users though, a quick pass through the normalization file and I'm wondering if the following should also be handled?

  • AUDIT_LOGIN
  • AUDIT_CHGRP_ID
  • AUDIT_CHUSER_ID
  • syscalls: chown, fchown, fchownat, lchown, setuid, seteuid, setfsuid, setreuid, setgid, setegid, setfsgid, setregid, setresuid, setresgid

WDYT?

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Nice progress! A few things to cover before closing this out:

  • Update examples in README with the new fields
  • Update the ECS version in the README
  • Add a changelog

Nit: Fixes an implicit rune to string conversion warning emitted by vet.

There was no bug, but the implicit conversion is deprecated starting in
Go 1.15.

> rule/rule.go:367:29: conversion from untyped int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)
@adriansr adriansr marked this pull request as ready for review January 25, 2021 14:03
@adriansr adriansr changed the title aucoalesce: ECS user and group mapping Add ECS session and user, group mapping Jan 25, 2021
@adriansr adriansr changed the title Add ECS session and user, group mapping Add ECS 1.8 session and map user and groups Jan 25, 2021
@adriansr
Copy link
Contributor Author

Been discussing @andrewstucki requests via Slack. The current status of this PR is that everything that we can support is supported:

* `AUDIT_LOGIN`

This one was (erroneously) ignored by Auditbeat. I've added support for it and will also update Auditbeat in elastic/beats#23594.

* `AUDIT_CHGRP_ID`
* `AUDIT_CHUSER_ID`

This ones don't contain any interesting information for ECS multiuser enrichment, only generic *id fields which were already supported.

* syscalls: `chown`, `fchown`, `fchownat`, `lchown`

We decided to leave file ownership syscalls for now, as it's not clear how/if they fit into ECS 1.8 multiuser at all.

setuid, seteuid, setfsuid, setreuid, setgid, setegid, setfsgid, setregid, setresuid, setresgid

The set*id syscalls are already supported through the generic *id fields.

@@ -1001,7 +1072,9 @@ normalizations:
- <<: *macro-user-session
record_types: USER_END
action: ended-session
ecs: *ecs-auth
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes USER_START/USER_END messages from event.category: authentication to session.

I wonder if I should keep the former, event.category: [authentication, session]

WDYT @andrewstucki ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm leaning toward only session since there are separate events for authentication. If we had session earlier I assume that's the way we would have done it. It's a minor breaking change though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with that change. Also, just in the interest of spreading the knowledge around, main place I could find a reference to AUDIT_USER_LOGIN is when login from shadow-utils returns successfully.

https://github.com/shadow-maint/shadow/blob/ae169c40468bc9d016016880613c01aca6050b60/src/login.c#L837

AUDIT_USER_START is predominantly an artifact of PAM and is called in pam_open_session:

https://github.com/linux-pam/linux-pam/blob/491e5500b6b3913f531574208274358a2df88659/libpam/pam_audit.c#L148-L151

Both work hand-in-hand, as you can see the invocation of pam_open_session here:

https://github.com/shadow-maint/shadow/blob/ae169c40468bc9d016016880613c01aca6050b60/src/login.c#L885

Seems to mirror exactly what's described in the audit framework docs

The next event can be AUDIT_USER_ROLE_CHANGE depending on whether a MAC framework that has the notion of roles is in use. It records what role has been assigned to the login session. This event is followed by AUDIT_USER_LOGIN to signify that the session that is being created is interactive. This is followed by AUDIT_USER_START to denote that the session has started.

README.md Outdated
target: {}
changes: {}
group:
effective: {}
Copy link

Choose a reason for hiding this comment

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

I think the only one having effective etc is the user field https://www.elastic.co/guide/en/ecs/master/ecs-user-usage.html.

For the effective group would be something like user.effective.group.*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. group.effective was unused, but group.target is used for group creation/deletion/changes.

I think the best for now is to set group.id in those cases? Because it's mapping actions like User A creates Group B. Another option is user.target.group? But that doesn't make a lot of sense without filling the user fields in user.target.

/cc @andrewstucki

Copy link

Choose a reason for hiding this comment

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

I think as far as user is modifying/creating a group does does not belong to a different user, is probably fine having both user and group at the root. Maybe @ebeahan can bring some guideline

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree with @marc-gr that using the top-level user.* and group.* is most appropriate in this case.

group.target is not defined by ECS.
"user": {
"id": "unset",
"effective": {
"name": "(invalid user)"
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to actually enrich these if they're invalid/unset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a failed ssh login attempt using an invalid user, I think it's worth to report that back to the user instead of an empty user.

@adriansr adriansr merged commit dde11ff into elastic:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants