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

Renamed auditd module fields #6080

Merged
merged 2 commits into from
Jan 16, 2018

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Jan 16, 2018

Updates to the auditd module

  • Update to latest go-libaudit.
  • Fixed an issue where the proctitle value was being truncated.
  • Fixed an issue where process args were incorrectly interpretted as hex data.
  • Fixed parsing of the key value when multiple keys are present.
  • Added a warning to the examples rules about the cost of auditing all network traffic.
  • Changed the event format to allow for some common fields between modules.
  • Updated fields.yml.

The dashboards need to be updated due to the field changes.

Closes #5423

@andrewkroh andrewkroh added in progress Pull request is currently in progress. review Auditbeat labels Jan 16, 2018
"action": "accepted-connection-from",
"category": "audit-rule",
"module": "auditd",
"type": "syscall"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice usage of type, module and category.

"user": {
"auid": "unset",
"egid": "0",
"euid": "0",
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the the ids which would also end up in user.id: [...] I assume?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Do you think I should add that now? I could add it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add it now as it's just an addition / feature.

@andrewkroh andrewkroh force-pushed the feature/ab/auditd-event-format branch from 96f5459 to cdc991e Compare January 16, 2018 08:07
@andrewkroh
Copy link
Member Author

I updated this with fields.yml changes, squashed, and give it a useful commit message.

Please use the Rebase and Merge to keep the vendor changes separate from code changes.

@andrewkroh andrewkroh added v6.2.0 and removed in progress Pull request is currently in progress. labels Jan 16, 2018
description: >
The name of the module that generated the event.

- name: event.action
Copy link
Contributor

Choose a reason for hiding this comment

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

@MikePaquette That is an interesting field. I wonder if we can reuse this.

type: text
description: The path to the file.
multi_fields:
- name: raw
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewkroh Potentially something we should support in ECS by default.

fields:
- name: action
- name: user
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind strange naming as it's user.selinux.user. Should this be account?

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. Left some minor comments which we should discuss but are not blocking for this PR, so I'm merging it.

type: keyword
description: This is the path associated with a unix socket.

- name: network.direction
Copy link
Contributor

Choose a reason for hiding this comment

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

Fingers cross that this is not changing ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need to standardize on the enum values.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on having a recommend standard.

- name: how
type: keyword
description: >
This describes how the action was performed. Usually this is the exe
or command that was being executed that triggered the event.
- name: key

- name: paths
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be singular?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is usually a list of raw path objects so I named with a plural. Is that the correct way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

My argument is normally to make it singular as long as it's not an array, as the user that is accessing it only looks for a single item and the "key" for this entry reads correct. Here for example paths.inode, inode is only one path. But I have this conversation often with @exekias and we probably agreed to disagree 😆

- name: saddr
type: keyword
description: The raw socket address structure.
- name: addr
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not to use abbreviation for address.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted for addr because that's the exact name coming from the kernel. But I can rename this is go-libaudit.

@ruflin
Copy link
Contributor

ruflin commented Jan 16, 2018

@andrewkroh This PR needs a make update.

Updating to bc29b128d4099fb834634afb535241f1608fb2f0.
- Update to latest go-libaudit.
- Fixed an issue where the proctitle value was being truncated.
- Fixed an issue where process args were incorrectly interpretted as hex data.
- Fixed parsing of the `key` value when multiple keys are present.
- Added a warning to the examples rules about the cost of auditing all network traffic.
- Changed the event format to allow for some common fields between modules.
- Updated fields.yml.

The dashboards need to be updated due to the field changes.
@andrewkroh andrewkroh force-pushed the feature/ab/auditd-event-format branch from 4d135cc to e9870c3 Compare January 16, 2018 21:56
@andrewkroh
Copy link
Member Author

I rebased it to fix some merge conflicts caused by me merging another PR.

@ruflin ruflin merged commit d3769b5 into elastic:master Jan 16, 2018
@ruflin
Copy link
Contributor

ruflin commented Jan 16, 2018

@andrewkroh I merged this to get the major change in. We can still continue to discuss the minor details.

@andrewkroh andrewkroh deleted the feature/ab/auditd-event-format branch April 20, 2018 00:04
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.

2 participants