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: Warn if auditd is running #6023

Merged
merged 5 commits into from
Jan 16, 2018

Conversation

adriansr
Copy link
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
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
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.

return errors.Wrap(err, "failed to get audit status before adding rules")
}
if status.Enabled == auditLocked {
ms.log.Warn("Skipping rule configuration: Audit rules are locked")
Copy link
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.


if err := ms.client.SetPID(libaudit.NoWait); err != nil {
return errors.Wrap(err, "failed to set audit PID")
if err := ms.client.WaitForPendingACKs(); err != nil {
Copy link
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
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
return multicast, nil
}
return unicast, nil
} else {

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
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)?

return err
}
if status.Enabled == auditLocked {
reporter.Error(errors.New("Skipping rule configuration: Audit rules are locked"))
Copy link
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
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.

}
if isLocked {
log.Errorf("Cannot continue: audit configuration is locked" +
" in the kernel (enabled=2) which prevents using unicast " +
Copy link
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?

hasMulticast := hasMulticastSupport()
hasRules := len(rules) > 0

const useAutodetect = "Remove the socket_type option to have auditbeat " +
Copy link
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