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] Add user information to processes #9963

Merged
merged 14 commits into from Jan 29, 2019
Merged

Conversation

@cwurm
Copy link
Member

cwurm commented Jan 9, 2019

Since go-sysinfo can now report the UIDs and GIDs of a process, this adds this information to the process metricset.

The added fields are:

  • user.id (UID or SID)
  • user.name
  • user.group.id (GID or SID of primary group)
  • user.group.name
  • user.effective.id (EUID)
  • user.effective.group.id (EGID)
  • user.saved.id (SUID)
  • user.saved.group.id (SGID)

Also adds some unit tests and tightens the system test.

@cwurm cwurm requested review from elastic/beats as code owners Jan 9, 2019
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Jan 9, 2019

Pinging @elastic/secops

@urso urso removed the request for review from elastic/beats Jan 9, 2019
@webmat
webmat approved these changes Jan 9, 2019
Copy link
Contributor

webmat left a comment

One minor nit that I'm ok if we fix subsequently (you decide).

Otherwise LGTM.

Thanks for the ping, btw, I created elastic/ecs#299

},
"user": {
"id": "1002",
"name": "elastic"

This comment has been minimized.

Copy link
@webmat

webmat Jan 9, 2019

Contributor

Note that ECS now defines the group field set as well. So you should also copy system.audit.process.gid to group.id.

Other than that, this event looks good

This comment has been minimized.

Copy link
@cwurm

cwurm Jan 10, 2019

Author Member

Great, I've added group.id and group.name. I'm less comfortable having two duplicated fields (probably shouldn't even had one in the first place since we don't have those in other places either), so I've removed system.audit.process.uid and system.audit.process.gid.

@webmat
webmat approved these changes Jan 10, 2019
Copy link
Contributor

webmat left a comment

Yes, this works for me. LGTM

@cwurm cwurm requested a review from elastic/siem Jan 11, 2019
@andrewkroh

This comment has been minimized.

Copy link
Member

andrewkroh commented Jan 14, 2019

Auditbeat's auditd module already defines fields like user.sgid. It would be better to reuse the existing fields this way users can query and get all data that has the given user ID value. You'll need move fields from auditd up to the field.common.yml.

@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 16, 2019

@andrewkroh You mean extend the top-level user field, right? I don't know about that - to me it feels that euid/suid/egid/sgid are more properties of the process entity, not the user (technically, they're also different users/accounts).

I took a look at the auditd module's sample data - it does not look ECS compliant to me? Fields like user.name_map and user.uid (instead of user.id) are not in ECS, and are not correctly documented in the fields.yml either (they're documented under the auditd.user namespace, but are actually in the top-level user object). That should not be I think? In Metricbeat and the Auditbeat system module, the system tests call Libbeat's assert_fields_are_documented(evt) to make sure that does not happen. Maybe this is something we should do for auditd as well?

@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 16, 2019

I don't know about that - to me it feels that euid/suid/egid/sgid are more properties of the process entity, not the user (technically, they're also different users/accounts).

I could see how they would be part of the user object if that one is treated more like "any user information" rather than "the user entity". I wonder what it should look like then? The challenge is that at the moment ECS assumes the user fields to represent a single user, with one id, one name, one email, etc. And now we have multiple values.

If we follow Auditd, it would probably be:

"user": {
  "id": "1000",
  "name": "elastic",
  "euid": "1000",
  "suid": "1000",
  "name_map": {
    "euid": "elastic",
    "suid": "elastic"
  }

But I wonder, what if we ever wanted add more information than the name? Would we need more maps?

Maybe it could allow nesting, e.g.:

"user": {
  "id": "1000",
  "name": "elastic",
  "email": "elastic@localhost",    // example additional field
  "created": "2019-01-01 19:37:56" // example additional field
  "effective": { // Nested user object
    "id": "1000",
    "name": "elastic",
    "email": "elastic@localhost",    // example additional field
    "created": "2019-01-01 19:37:56" // example additional field
  },
  "saved": { // Nested user object
    "id": "1000",
    "name": "elastic",
    "email": "elastic@localhost",    // example additional field
    "created": "2019-01-01 19:37:56" // example additional field
  }
}

The group object is also affected, but would probably just follow whatever we do for user.

Anyway, if this is a change to the ECS user object, maybe this discussion has to happen in the ECS repo?

@webmat

This comment has been minimized.

Copy link
Contributor

webmat commented Jan 16, 2019

Yeah I addressed these fields under user in my review of Auditbeat/auditd here: #10111.

Waiting on input from Nic on it before bugging you guys. But my conclusion over in that issue is that despite not being in ECS (yet?), I don't think it's a problem. Read up over there for my thinking on this.

@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 17, 2019

@webmat I see that in #10111 we have the group IDs under user as well: user.gid, user.egid, etc.

Whereas here we concluded to have them under group.

I'm confused now - I think we should have one or the other, and ECS should specify which.

Personally, I like the idea of having all user information under user and not have a top-level group object at all.

@webmat

This comment has been minimized.

Copy link
Contributor

webmat commented Jan 17, 2019

To follow up on the discussion we've had elsewhere, I would also recommend populating group.id with user.egid. This will help correlation with other places that are using group.id this way.

I agree that ECS will need to:

  • define how to represent user privilege escalation
  • clarify or review whether a user's group ID, when seen on an event belongs at group.id or nested under the user. This work will be related to the privilege escalation work. Just like with user IDs, multiple group IDs may have to be considered before determining the effective group ID

@cwurm I also think you mentioned that the sets of IDs represented here for privilege escalation is different than what has been happening in the new system module. I agree we would benefit from harmonizing this. However since this isn't in ECS yet and we can't tackle this before FF, I would say it's a decision that is up to you and Andrew on whether to move the fields around on one of the modules to align with the other.

Although you made the point that these details may make more sense under process. than under user., which also makes sense. Perhaps we touch neither of them for now?

@webmat

This comment has been minimized.

Copy link
Contributor

webmat commented Jan 17, 2019

Oops that first paragraph was meant as a recommendation for #10111. You're already doing this here :-) I have too many PRs to review LOL

@webmat

This comment has been minimized.

Copy link
Contributor

webmat commented Jan 17, 2019

Actually this whole comment was written for #10111 :-)

@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 21, 2019

I've updated to the nested user format proposed in #10111 (comment) and rebased on master.

It would be nice to have resolved user and group names for effective and saved UIDs and GIDs (user.effective.name, user.effective.group.name, user.saved.name, user.saved.group.name), but I didn't want to do it here without a cache in place for those lookups. Something for a follow-up though.

@webmat
webmat approved these changes Jan 21, 2019
Copy link
Contributor

webmat left a comment

Loving this representation of all the considered users, in determining the effective rights.

Let's wait for people's input tomorrow, when the US isn't on holiday. But I think this is good to go :-)

LGTM

"id": "1000"
},
"id": "1000"
}

This comment has been minimized.

Copy link
@webmat

webmat Jan 21, 2019

Contributor

❤️

Copy link
Member

andrewkroh left a comment

LGTM. Can you please update the PR description to reflect the final format used in the PR.

@webmat

This comment has been minimized.

Copy link
Contributor

webmat commented Jan 23, 2019

Agree with @andrewkroh.

However this PR depends on #10275

@cwurm cwurm force-pushed the cwurm:process_user_info branch from b5319d7 to f92d87e Jan 25, 2019
@webmat
webmat approved these changes Jan 25, 2019
Copy link
Contributor

webmat left a comment

LGTM, but a few points:

  • A rebase will get you green again
  • Please adjust the PR description with the final outcome
  • I think we're fine like this, wrt field definitions. ECS change was only adding user.group.id and user.group.name. You're defining everything you need here already. I'm defining them locally to Filebeat for the Fb auditd module.
cwurm added 12 commits Jan 8, 2019
This reverts commit eb8f7bf.
@cwurm cwurm force-pushed the cwurm:process_user_info branch from 0397c47 to 1c0d8f1 Jan 28, 2019
@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 28, 2019

jenkins, test this

1 similar comment
@cwurm

This comment has been minimized.

Copy link
Member Author

cwurm commented Jan 28, 2019

jenkins, test this

@cwurm cwurm merged commit fa40a54 into elastic:master Jan 29, 2019
5 checks passed
5 checks passed
CLA Commit author is a member of Elasticsearch
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cwurm cwurm deleted the cwurm:process_user_info branch Jan 29, 2019
cwurm added a commit to cwurm/beats that referenced this pull request Jan 29, 2019
Adds real, effective, and saved UID and GID information to the process dataset.

(cherry picked from commit fa40a54)
@cwurm cwurm added v6.7.0 and removed needs_backport labels Jan 29, 2019
cwurm added a commit that referenced this pull request Jan 31, 2019
…es (#10395)

Adds real, effective, and saved UID and GID information to the process dataset.

(cherry picked from commit fa40a54, then adjusted)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.