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] Auditd: Change user fields to ECS #10456

Merged
merged 4 commits into from Feb 5, 2019

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Jan 31, 2019

Changes the UID and GID fields of the auditd module to follow the new ECS format:

Specifically, these fields are remapped:

Old New
user.auid user.audit.id
user.uid user.id
user.euid user.effective.id
user.fsuid user.filesystem.id
user.suid user.saved.id
user.gid user.group.id
user.egid user.effective.group.id
user.sgid user.saved.group.id
user.fsgid user.filesystem.group.id
user.name_map.auid user.audit.name
user.name_map.uid user.name
user.name_map.euid user.effective.name
user.name_map.fsuid user.filesystem.name
user.name_map.suid user.saved.name
user.name_map.gid user.group.name
user.name_map.egid user.effective.group.name
user.name_map.sgid user.saved.group.name
user.name_map.fsgid user.filesystem.group.name

Follow-up to #10195.

Field names should be the same as in the Filebeat auditd module after #10192.

@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.

Two requests about the field definitions and we're good.

Love the default on the nesting: better to have a weird field name and not lose data.

case "auid":
user.Put("audit.id", value)
default:
user.Put(id+".id", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the ID field is unknown, it would be worth logging a warning, IMO.

I like that we're not losing the information, however. If for example ouid came up, stuff would be nested as user.ouid.id and user.ouid.name. Obviously not perfect, but a good fallback 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the ID field is unknown, it would be worth logging a warning, IMO.

There's no logging infrastructure at that point in the code at the moment. I personally wouldn't want to bring in that complexity for this one case. The current code does not log a warning either when this happens so we're not losing anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if you don't have logging infrastructure in here already, this is way out of scope indeed.

- name: group
type: group
description: Saved group information.
fields:
- name: id
type: keyword
description: Saved group ID.
- name: name
type: keyword
description: Saved group name.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it confusing to have the effective & saved fields defined here, and the audit & filesystem fields defined only in the auditd module. Especially since they're defined mixed in the ECS user field set.

I think it would make most sense to have them grouped together here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would assume Auditbeat's FIM has events with ouid and ogid (file owner). I think it would make sense to define them here as (and adjust FIM to use them ;-) ).

I've seen them pop up in the Filebeat auditd module, so you can pilfer the definitions from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify about FIM above: I would define the fields in this PR. Addressing whether FIM emits owner IDs in these fields can be addressed as a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it confusing to have the effective & saved fields defined here, and the audit & filesystem fields defined only in the auditd module. Especially since they're defined mixed in the ECS user field set.

I think it would make most sense to have them grouped together here.

+1 I'll make the change

I would assume Auditbeat's FIM has events with ouid and ogid (file owner). I think it would make sense to define them here as (and adjust FIM to use them ;-) ).

I think we have a few options. But regardless, I don't think we should address it here in this PR.

The FIM file metadata has a uid, gid, owner, and group. This is also what is currently in ECS.

Our options for moving the fields:

  1. Move them under user.*: user.id, user.name, user.group.id, user.group.name - no new definitions required, easy field names, but problematic if we ever have another user object (e.g. the user that modified the file)
  2. Move them under user.owner.*: user.owner.id, user.owner.name, user.owner.group.id, user.owner.group.name - more future proof, somewhat longer field names
  3. Move them under file.user.*: file.user.owner.id, file.user.owner.name, file.user.owner.group.id, file.user.owner.group.name
  4. Move them under file.owner.*: file.owner.id, file.owner.name, file.owner.group.id, file.owner.group.name

The benefit the last two options under file have is that it would allow us to keep the file object of the FIM module and the auditd.paths object (containing multiple files) of the Auditd module to share the same fields for users. They're a bit disparate at the moment, with FIM using uid/gid and Auditd using ouid/ogid for the same data.

Again, I think we should address this whole thing in another PR, including any field definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for moving the fields defs :-)

I agree the owner stuff is to be addressed in another PR, if we address it.

The discrepancy in representing users in file.* in ECS saddens me a bit, haha. This one slipped by. But I think we should leave it as is and concentrate on polishing everything that's already in flight, for FF. I don't think we should open this can of worms as well.

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

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.

LGTM

@cwurm cwurm merged commit 89af30d into elastic:master Feb 5, 2019
@cwurm cwurm deleted the auditd_user_ecs branch February 5, 2019 00:05
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.

None yet

4 participants