aide_conf resource: test configuration of the AIDE file integrity tool #2063
Conversation
Signed-off-by: Jennifer Burns <[email protected]>
Signed-off-by: Jennifer Burns <[email protected]>
adamleff
changed the title
docs/resources/aide_conf.md.erb
Outdated
|
|
||
| An `aide_conf` resource block can be used to determine if the selection lines contain one (or more) directories whose files should be added to the aide database: | ||
|
|
||
| describe aide_conf('path')do |
There was a problem hiding this comment.
missing space between ) and do
docs/resources/aide_conf.md.erb
Outdated
| Use the where clause to match a selection_line to one or more rules found in the aide.conf file: | ||
|
|
||
| describe aide_conf.where { selection_line == '/bin' } do | ||
| its('rules.flatten') { should include 'r' } |
There was a problem hiding this comment.
Is there any case where the rules shouldn't be flattened? Should the resource always flatten the rules value?
There was a problem hiding this comment.
There is the case when you know the entire set of rules you want to match upon and the order of those rules. Hopefully lines 29-32 and 79-81 clear that up.
| ### Test whether selection line for /bin contains a particular rule | ||
|
|
||
| describe aide_conf.where { selection_line == '/bin' } do | ||
| its('rules.flatten') { should include 'r' } |
There was a problem hiding this comment.
Same question re: flatten as above. 🙂
lib/resources/aide_conf.rb
Outdated
| desc 'Use the aide_conf InSpec audit resource to test the rules established for | ||
| the file integrity tool AIDE. Controlled by the aide.conf file typically at /etc/aide.conf.' | ||
| example " | ||
| describe aide_conf do |
There was a problem hiding this comment.
Can you please indent lines 14-24 so it's clear it's part of the example method?
lib/resources/aide_conf.rb
Outdated
|
|
||
| def all_have_rule(rule) | ||
| # Case when file didn't exist or perms didn't allow an open | ||
| if @content.instance_of?(String) |
There was a problem hiding this comment.
In Ruby, testing an object for a given Class is generally seen as a stop-gap since a user/program could supply an object that looks/acts like a String, for example. It's generally better to see if it responds to a method with respond_to?
However, in this case, I think you just care if @content is nil or not, right? I'd probably just change 41-43 to be:
return false if @content.nil?or...
return false unless defined?(@content)
lib/resources/aide_conf.rb
Outdated
| def parse_line(line) | ||
| line_and_rules = {} | ||
| # Case when line is a rule line | ||
| if line.include? ' = ' |
There was a problem hiding this comment.
We should be consistent with our argument style and wrap it in parentheses:
if line.include?(' = ')
lib/resources/aide_conf.rb
Outdated
| # Case when line is a rule line | ||
| if line.include? ' = ' | ||
| parse_rule_line(line) | ||
| end |
There was a problem hiding this comment.
Should 102-104 be an elsif? Is it possible for a line to be a "rule" line and a "selection" line at the same time?
lib/resources/aide_conf.rb
Outdated
| @rules[rule_name] = rules_list.flatten | ||
| end | ||
|
|
||
| def normalize_dir(dir) |
There was a problem hiding this comment.
This method appears to only be used in one place and it does one small thing. Should we perhaps just add this logic to #parse_selection_line directly?
| it 'Verify aide_conf finds selection_line dirs' do | ||
| _(resource.selection_lines).must_include '/bin' | ||
| end | ||
| end |
There was a problem hiding this comment.
There's enough logic happening in some of the private methods (parse_rule_line, parse_selection_line, and check_multi_rule) that I'd like to see some unit test coverage for those.
lib/resources/aide_conf.rb
Outdated
| if @rules.key?(rules_list[i]) | ||
| rules_list[i] = @rules[rules_list[i]] | ||
| end | ||
| check_multi_rule(rules_list, i) |
There was a problem hiding this comment.
I'm not a huge fan of passing an object to a method for it to be modified within that method. In this case, check_multi_rule actually modifies a value for a key in rules_list
Can we modify this to look like this:
rules_list[i] = handle_multi_rule(rules_list, i)... and then handle_multi_rule returns whatever should be be in that hash value, which, in some cases, may simply be what's already there?
Signed-off-by: Jennifer Burns <[email protected]>
No description provided.