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

Support for attributes within audit cookbook #262

Merged
merged 8 commits into from Aug 10, 2017

Conversation

Projects
None yet
5 participants
@mhedgpeth
Contributor

mhedgpeth commented Jul 12, 2017

Fixes #261

Allows for node['audit']['attributes'] to define which attributes are passed into the InSpec run
Added documentation in README.md
Tested in kitchen, which relies on the attributes-file-exists-profile for testing that a file exists, driven off of an attribute.

@jeremymv2 this is what we were talking about as a better way to manage the "profile that rules them all" pattern.

Michael Hedgpeth and others added some commits Jul 12, 2017

Fixed rubocop error
Signed-off-by: Michael Hedgpeth <mhedgpeth@gmail.com>
Updated unit tests to properly test attribute existence
Signed-off-by: Michael Hedgpeth <mhedgpeth@gmail.com>
@seththoenen

This comment has been minimized.

seththoenen commented Jul 21, 2017

+1

@chris-rock

@mhedgpeth I apologize for the late response. This is an great addition. I added some minor comments, once fixed LGTM

Gemfile Outdated
@@ -21,6 +21,8 @@ group :test do
gem 'rb-readline'
gem 'webmock'
gem 'fauxhai', '~> 5.2'
gem 'pry-remote'

This comment has been minimized.

@chris-rock

chris-rock Jul 24, 2017

Collaborator

Do we need that for attributes support?

README.md Outdated
@@ -163,6 +163,17 @@ default["audit"] = {
}
```
#### Attributes
You can also pass in [InSpec Attributes](http://www.anniehedgie.com/inspec-basics-10) to your audit run. You do this by defining the attributes here:

This comment has been minimized.

@chris-rock

chris-rock Jul 24, 2017

Collaborator

I would prefer if we could reference to the offical docs https://www.inspec.io/docs/reference/profiles/

@@ -0,0 +1,5 @@
# ensures that the file defined by attributes exists, so its associated profile will pass

This comment has been minimized.

@chris-rock

chris-rock Jul 24, 2017

Collaborator

Is this profile missing?

This comment has been minimized.

@adamleff

adamleff Aug 8, 2017

Collaborator

No, it's in the .kitchen.yml.

@@ -80,6 +80,18 @@ suites:
- name: ssh
supermarket: hardening/ssh-hardening
# default setting does not fail for missing profile
- name: attributes-work

This comment has been minimized.

@chris-rock

chris-rock Jul 24, 2017

Collaborator

Can we add some integration tests?

output = quiet ? ::File::NULL : $stdout
Chef::Log.warn "Format is #{format}"
opts = {
'report' => true,
'format' => format,
'output' => output,
'logger' => Chef::Log, # Use chef-client log level for inspec run
'logger' => Chef::Log, # Use chef-client log level for inspec run,
'attributes' => attributes,

This comment has been minimized.

@jeremymv2

jeremymv2 Jul 24, 2017

Contributor

The attributes key needs to be a symbol, otherwise it's not working; ie. attributes: attributes,

As-is, you can see that there's another symbol key that is not assigned the value I'm assigning:

From: /opt/chef/embedded/lib/ruby/gems/2.4.0/gems/inspec-1.30.0/lib/inspec/runner.rb @ line 124 Inspec::Runner#load_attributes:

    120: def load_attributes(options)
    121:   options[:attributes] ||= {}
    122:   require 'pry';binding.pry
    123:
 => 124:   secrets_targets = options[:attrs]
    125:   return options[:attributes] if secrets_targets.nil?
    126:
    127:   secrets_targets.each do |target|
    128:     secrets = Inspec::SecretsBackend.resolve(target)
    129:     if secrets.nil?
    130:       raise Inspec::Exceptions::SecretsBackendNotFound,
    131:             "Unable to find a parser for attributes file #{target}. " \
    132:             'Check to make sure the file exists and has the appropriate extension.'
    133:     end
    134:
    135:     next if secrets.attributes.nil?
    136:     options[:attributes].merge!(secrets.attributes)
    137:   end
    138:
    139:   options[:attributes]
    140: end

[1] pry(#<Inspec::Runner>)> options
=> {"report"=>true,
 "format"=>"json",
 "output"=>"/dev/null",
 "logger"=>Chef::Log,
 "attributes"=>{"password"=>"bob"},
 :logger=>#<Logger:0x00000004f37010 @default_formatter=#<Logger::Formatter:0x00000004f36fc0 @datetime_format=nil>, @formatter=nil, @level=0, @logdev=nil, @progname=nil>,
 :attributes=>{}}
[2] pry(#<Inspec::Runner>)>

When line 124 of audit_report.rb is changed to attributes: attributes,:

From: /opt/chef/embedded/lib/ruby/gems/2.4.0/gems/inspec-1.30.0/lib/inspec/runner.rb @ line 124 Inspec::Runner#load_attributes:

    120: def load_attributes(options)
    121:   options[:attributes] ||= {}
    122:   require 'pry';binding.pry
    123:
 => 124:   secrets_targets = options[:attrs]
    125:   return options[:attributes] if secrets_targets.nil?
    126:
    127:   secrets_targets.each do |target|
    128:     secrets = Inspec::SecretsBackend.resolve(target)
    129:     if secrets.nil?
    130:       raise Inspec::Exceptions::SecretsBackendNotFound,
    131:             "Unable to find a parser for attributes file #{target}. " \
    132:             'Check to make sure the file exists and has the appropriate extension.'
    133:     end
    134:
    135:     next if secrets.attributes.nil?
    136:     options[:attributes].merge!(secrets.attributes)
    137:   end
    138:
    139:   options[:attributes]
    140: end

[1] pry(#<Inspec::Runner>)> options
=> {"report"=>true,
 "format"=>"json",
 "output"=>"/dev/null",
 "logger"=>Chef::Log,
 :attributes=>{"password"=>"bob"},
 :logger=>#<Logger:0x00000003d4acd0 @default_formatter=#<Logger::Formatter:0x00000003d4ac80 @datetime_format=nil>, @formatter=nil, @level=0, @logdev=nil, @progname=nil>}
[2] pry(#<Inspec::Runner>)>

With the change, my inspec control test works as expected, without, I get nil for the attribute value.

As I mentioned, this is because of the key being symbol: https://github.com/chef/inspec/blob/master/lib/inspec/runner.rb#L121

Integration tests will help expose this 😃

This comment has been minimized.

@jeremymv2

jeremymv2 Jul 24, 2017

Contributor

Thanks @mhedgpeth for getting this support for attributes!!

Aside from my above comment and Chris' comments, lgtm.

@chris-rock

This comment has been minimized.

Collaborator

chris-rock commented Jul 27, 2017

@mhedgpeth anything I can help with?

@adamleff

@mhedgpeth this is a super-great addition. Thank you for your PR. 🙂

I left a few comments that I hope we can address before merging. But otherwise, this looks great.

# default setting does not fail for missing profile
- name: attributes-work

This comment has been minimized.

@adamleff

adamleff Aug 8, 2017

Collaborator

Can I suggest a suite name of pass-inspec-attributes so it's clear what we're testing, and it doesn't read as if it might be a work in progress?

@@ -0,0 +1,5 @@
# ensures that the file defined by attributes exists, so its associated profile will pass

This comment has been minimized.

@adamleff

adamleff Aug 8, 2017

Collaborator

No, it's in the .kitchen.yml.

@@ -0,0 +1 @@
# will put a test that makes sure a file is there

This comment has been minimized.

@adamleff

adamleff Aug 8, 2017

Collaborator

Because the profile that actually consumes and uses the attribute is elsewhere (and run by the audit cookbook, not TK), and this is empty, TK won't actually be doing any real testing of this new test suite. To make matters worse, Chef will still exit with a zero exit code even if the audit cookbook failed to scan properly or if there are compliance failures. 🙂

Can I recommend we add a test here that reads in the JSON outputted by the audit cookbook, and ensures that our test succeeds?

@adamleff

This comment has been minimized.

Collaborator

adamleff commented Aug 10, 2017

Merging now, will submit a PR to address the issues I raised separately.

@adamleff adamleff merged commit 57a0384 into chef-cookbooks:master Aug 10, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
DCO This commit has a DCO Signed-off-by
Details

adamleff added a commit that referenced this pull request Aug 10, 2017

Add test for InSpec Attributes functionality
The work introduced by #262 was lacking an integration test for Test Kitchen. I
also renamed the suite to be clearer about what it does and to avoid any
confusion that the work may be a work-in-progress.

Signed-off-by: Adam Leff <adam@leff.co>
@chris-rock

This comment has been minimized.

Collaborator

chris-rock commented Aug 10, 2017

Thank you @mhedgpeth and @adamleff

adamleff added a commit that referenced this pull request Aug 10, 2017

Add test for InSpec Attributes functionality (#266)
The work introduced by #262 was lacking an integration test for Test Kitchen. I
also renamed the suite to be clearer about what it does and to avoid any
confusion that the work may be a work-in-progress.

Signed-off-by: Adam Leff <adam@leff.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment