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

Rule deletion #128

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Rule deletion #128

wants to merge 4 commits into from

Conversation

jeffmahoney
Copy link

This PR adds the ability to delete rules from the running system.

@andrewkroh andrewkroh added the Team:Security-External Integrations Label for the Security External Integrations team label Apr 10, 2023
@andrewkroh andrewkroh requested a review from a team April 10, 2023 23:21
@jeffmahoney jeffmahoney force-pushed the rule-deletion branch 3 times, most recently from 5d08e23 to feed65b Compare April 10, 2023 23:32
audit.go Outdated Show resolved Hide resolved
audit.go Outdated Show resolved Hide resolved
rule/rule.go Outdated Show resolved Hide resolved
audit.go Outdated Show resolved Hide resolved
audit_test.go Outdated Show resolved Hide resolved
audit_test.go Outdated Show resolved Hide resolved
rule/flags/flags.go Outdated Show resolved Hide resolved
audit_test.go Outdated Show resolved Hide resolved
Comment on lines +156 to +162
deleteAll uint8
fileWatchFlags uint8
addFileWatch uint8
deleteFileWatch uint8
syscallFlags uint8
addSyscall uint8
deleteSyscall uint8
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this should be a bitset at this point. Maybe later.

@jeffmahoney jeffmahoney force-pushed the rule-deletion branch 2 times, most recently from 8aac899 to 64b0760 Compare September 8, 2023 23:03
Although the implementation for AuditClient.Delete is only used to back
AuditClient.DeleteAll, we'd like to be able to delete individual rules.
This commit adds checking of the netlink error field and reports when
the deletion has failed.  When DeleteAll is called, we ignore the ENOENT
return since it could've raced somewhere and we don't actually care
since we're deleting all of the rules.
Rule.Build assumes that if no syscalls are specified they all are set.
This is really only the case when the exit list is used since the
syscall numbers aren't available in the other lists.  When we assume
that all of the syscalls are enabled, we end up generating wireformat
rules for e.g. 'task,never' that have all of the syscall bits set.  That
doesn't match what is already used when 'auditctl -a task,never' is
used.  It may be ignored by the kernel when such a rule is added, but
it would cause problems when that rule is deleted.
@jeffmahoney
Copy link
Author

Thanks for the review. I've implemented your suggestions and have rebased on the current HEAD. It should be good to go.

We currently don't handle the '-d' or '-W' options that would remove
list rules or file watches.  This commit adds support to handle those
properly.  rule.ToCommandLine still returns the expected result, but
I've added a rule.ToCommandLineAddRemove that takes a bool indicating
whether the rule would be added or removed.  This was required to do
testing of deletion rules.
@efd6
Copy link
Contributor

efd6 commented Sep 10, 2023

@andrewkroh This doesn't appear to be triggering the buildkite build. How do we bump it?

@andrewkroh
Copy link
Member

/test

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 12, 2023

💔 Build Failed

Failed CI Steps

History

@elastic elastic deleted a comment from elasticmachine Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Security-External Integrations Label for the Security External Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants