Skip to content

auditbeat: Warn if auditd is running#6023

Merged
andrewkroh merged 5 commits intoelastic:masterfrom
adriansr:feature/ab/audit_pid
Jan 16, 2018
Merged

auditbeat: Warn if auditd is running#6023
andrewkroh merged 5 commits intoelastic:masterfrom
adriansr:feature/ab/audit_pid

Conversation

@adriansr
Copy link
Copy Markdown
Contributor

@adriansr adriansr commented Jan 9, 2018

Detect initialization failures for the auditd module:

  • When another process is already set as the audit process (auditd already running). This error is reported.
  • If rules are locked, prints a warning message and does not attempt to add its own rules (This implies auditd is running).

Closes #5845 and #6019

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Jan 9, 2018

@adriansr Seems to have a conflict.

@adriansr adriansr force-pushed the feature/ab/audit_pid branch from d13d0f0 to ceafea5 Compare January 9, 2018 05:29
@adriansr adriansr changed the title auditbeat: Warn if auditd is running (#5845) auditbeat: Warn if auditd is running Jan 9, 2018
@adriansr adriansr force-pushed the feature/ab/audit_pid branch 3 times, most recently from 774f290 to 1e8dee7 Compare January 9, 2018 12:04
@adriansr adriansr added the review label Jan 9, 2018
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you push the error to the reporter for this case (like we do at https://github.com/elastic/beats/pull/6023/files#diff-a3559204e3ac05aabc310b7b3dfd90e3R120). As an operator I'd like to see this type of information reported in Elasticsearch.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thought of one more thing. 😄
If the audit config is locked, will any of the above config changes result in a failure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently this is failing either in this line or the SetPID below.

I think we need to handle configuration locking differently as it will only work with a multicast socket. See this comment I left on the issue.

@adriansr adriansr added the in progress Pull request is currently in progress. label Jan 10, 2018
Detect failures When Auditbeat is installed as audit process by setting
the PID field in the AuditStatus structure. This usually means another
process is already set as the audit process.
The audit rules can be locked (enabled=2) so that further
changes are not possible. Skip rule configuration if this
is the case, displaying a warning message if rules are set
in the configuration.
@adriansr adriansr force-pushed the feature/ab/audit_pid branch from 1e8dee7 to 1e79add Compare January 14, 2018 15:10
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

@adriansr adriansr force-pushed the feature/ab/audit_pid branch from 1e79add to 5f9bc53 Compare January 16, 2018 07:13
Copy link
Copy Markdown
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. Is this one still "in progress" (per the label)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think we should log this message too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope. I wasn't sure which one did you want to be reported in your first comment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - Move the space to the previous line for consistency?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the logic change here. It's deterministic now if the user specifies a socket_type.

@adriansr adriansr force-pushed the feature/ab/audit_pid branch from 5f9bc53 to 0aec242 Compare January 16, 2018 11:34
@adriansr adriansr removed the in progress Pull request is currently in progress. label Jan 16, 2018
@andrewkroh andrewkroh merged commit 91fcb93 into elastic:master Jan 16, 2018
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