auditd resource: test active auditd configuration against the audit daemon #2133
Conversation
Signed-off-by: Jennifer Burns <[email protected]>
… match new entries in auditctl Signed-off-by: Jennifer Burns <[email protected]>
Signed-off-by: Jennifer Burns <[email protected]>
|
@jburns12 and I just chatted and she's going to remove some of the legacy stuff and ping us here when she's ready for a new review. At quick glance, this looks like a great improvement over the existing resource. Thanks, Jen! |
…sistency. Signed-off-by: Jennifer Burns <[email protected]>
| ] | ||
| end | ||
|
|
||
| it 'auditd_rules syscall interface' do | ||
| resource = MockLoader.new(:centos7).load_resource('auditd_rules') | ||
| _(resource.send('syscall', 'open').send('rules')).must_equal [ | ||
| {:syscall=>"open", :list=>"exit", :action=>"always", :fields=>["arch=b64", "exit=-EACCES", "key=access"], :arch=>"b64", :exit=>"-EACCES", :key=>"access"} |
There was a problem hiding this comment.
Unfortunately I had to update the test file of the auditd_rules resource since the new and old both use the same command auditctl -l and therefore the same mock command.
|
Thanks for the chat, @adamleff ! I yanked all of the legacy code and added updates to the documentation to explain that support is for audit >= 2.3. As a reminder, the reason I had to change the auditd_rules_test file was due to the new and old resource both needing to use the same mock cmd based on |
adamleff
changed the title
There was a problem hiding this comment.
@jburns12 this looks really good. Thank you! Just a few things to tidy up.
Also, many Ruby programmers will tell you that prefixing methods with get_ or set_ is not great practice. Setters will usually end with an = and getters are usually self-defining. I'd encourage you to rethink some of your method names... for example I'd rename get_syscall_rules(line) to syscall_rules_for(line). It's not a blocking issue but it's something I would recommend addressing.
Lemme know when you've made another pass and I think we're pretty close to a final review on this one. Thank you for your contribution!
lib/resources/auditd.rb
Outdated
| end | ||
|
|
||
| def get_file_syscall_syntax_rules(line) | ||
| file = get_file_syscall_syntax line |
There was a problem hiding this comment.
Sorry @jburns12 if I wasn't clear. The standard is to use parens rather than remove them, so can you update these lines as such?
file = get_file_syscall_syntax(line)
action, list = get_action_list(line)
... etc ...
... and throughout the resource, please? Thank you! 🙂
lib/resources/auditd.rb
Outdated
| end | ||
|
|
||
| def to_s | ||
| 'Audit Daemon Rules' |
There was a problem hiding this comment.
Because of our silly CLI parser, I'd recommend we keep this to two space-separate words. Otherwise, Rules will get pushed down to the next line. It's a long-standing bug and we'll fix it eventually :)
I'd recommend auditd rules for now.
lib/resources/auditd.rb
Outdated
| @@ -0,0 +1,236 @@ | |||
| # encoding: utf-8 | |||
| # copyright: 2015, Vulcano Security GmbH | |||
There was a problem hiding this comment.
We can remove this line - the copyright and license are covered at a project-level.
lib/resources/auditd.rb
Outdated
|
|
||
| if @content =~ /^LIST_RULES:/ | ||
| return skip_resource 'The version of audit is outdated. The `auditd` resource supports versions of audit >= 2.3.' | ||
| else |
There was a problem hiding this comment.
Since there's a return on line 41, you can get rid of the else on 42 and end on 45, and outdent 43 and 44.
lib/resources/auditd.rb
Outdated
| return skip_resource 'The version of audit is outdated. The `auditd` resource supports versions of audit >= 2.3.' | ||
| else | ||
| parse_content | ||
| @legacy = nil |
lib/resources/auditd.rb
Outdated
| filter.connect(self, :params) | ||
|
|
||
| def status(name = nil) | ||
| return @legacy.status(name) if @legacy |
There was a problem hiding this comment.
This can be removed now, right?
Signed-off-by: Jennifer Burns <[email protected]>
|
Thanks so much for the info and feedback, @adamleff ! I just made a commit/push that addresses the issues, including the naming of "get" methods. Hopefully the name changes are in the vein of what you expressed, but if not let me know. :) |
This is a new version of the auditd_rules resource, renamed auditd, that implements Filter Tables as a means of creating params based upon the lines that are returned upon executing the command 'auditctl -l'.
New features include the ability to filter rules based on path, match on all fields sans the key field, match on single permissions values individually, and match on file watch rules written in a syscall syntax by means of the file name or the syscall field.