diff --git a/lib/inspec/profile.rb b/lib/inspec/profile.rb index 76a238a8a7..5d19159c7e 100644 --- a/lib/inspec/profile.rb +++ b/lib/inspec/profile.rb @@ -325,10 +325,9 @@ def info(res = params.dup) # rubocop:disable Metrics/CyclomaticComplexity, Metri res end - # Check if the profile is internally well-structured. The logger will be - # used to print information on errors and warnings which are found. + # Sanity check the profile. This is the core of `inspec check` # - # @return [Boolean] true if no errors were found, false otherwise + # @return [Hash] check results. See below for structure. def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength # initial values for response object result = { @@ -411,8 +410,8 @@ def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metri @logger.info("Found #{count} controls.") end - # iterate over hash of groups - params[:controls].each { |id, control| + # iterate over hash of controls + params[:controls].each do |id, control| sfile = control[:source_location][:ref] sline = control[:source_location][:line] error.call(sfile, sline, nil, id, 'Avoid controls with empty IDs') if id.nil? or id.empty? @@ -422,7 +421,24 @@ def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metri warn.call(sfile, sline, nil, id, "Control #{id} has impact > 1.0") if control[:impact].to_f > 1.0 warn.call(sfile, sline, nil, id, "Control #{id} has impact < 0.0") if control[:impact].to_f < 0.0 warn.call(sfile, sline, nil, id, "Control #{id} has no tests defined") if control[:checks].nil? or control[:checks].empty? - } + + # Check for duplicate IDs within the same profile + rule = control[:rule_obj] + next unless Inspec::Rule.merge_count(rule) > 0 + profile_id = Inspec::Rule.profile_id(rule) + merges = Inspec::Rule.merge_changes(rule) + merges = merges.select do |merge_location| + # Look for rules that had a merge within the same profile... + merge_location[:profile_id] == profile_id && + !merge_location[:in_include] # and the merge did not happen in an include/require_controls + end + merges.each do |location| + cfile = location[:ref] + cline = location[:line] + msg = "Control #{id} is duplicated in profile #{profile_id} - clobbered by #{cfile}:#{cline}" + error.call(sfile, sline, nil, id, msg) + end + end # profile is valid if we could not find any error result[:summary][:valid] = result[:errors].empty? @@ -607,6 +623,7 @@ def load_rule_filepath(prefix, rule) def load_rule(rule, file, controls, groups) id = Inspec::Rule.rule_id(rule) location = rule.instance_variable_get(:@__source_location) + # TODO: why are we constructiong this thing, when the rule object already has this info? controls[id] = { title: rule.title, desc: rule.desc, @@ -617,6 +634,7 @@ def load_rule(rule, file, controls, groups) checks: Inspec::Rule.checks(rule), code: Inspec::MethodSource.code_at(location, source_reader), source_location: location, + rule_obj: rule, # We need the actual object to interrogate merge history } # try and grab code text from merge locations diff --git a/lib/inspec/rule.rb b/lib/inspec/rule.rb index 8b66c96ea4..745d724536 100644 --- a/lib/inspec/rule.rb +++ b/lib/inspec/rule.rb @@ -39,7 +39,7 @@ def initialize(id, profile_id, opts, &block) # not changeable by the user: @__code = nil @__block = block - @__source_location = __get_block_source_location(&block) + @__source_location = __get_block_source_location(&block).merge(profile_id: profile_id, in_include: __check_for_include) @__rule_id = id @__profile_id = profile_id @__checks = [] @@ -178,6 +178,10 @@ def self.set_rule_id(rule, value) rule.instance_variable_set(:@__rule_id, value) end + def self.source_location(rule) + rule.instance_variable_get(:@__source_location) + end + def self.profile_id(rule) rule.instance_variable_get(:@__profile_id) end @@ -312,5 +316,18 @@ def __get_block_source_location(&block) rescue MethodSource::SourceNotFoundError {} end + + # For duplicate control detection (github 822), + # we must only warn on a control being duplicated when + # it is *not* being overridden. Overrides happen within + # include_control / require control blocks. + # HACK: detect that using the call stack. + def __check_for_include + # A require_contols / include_controls call will be at least 8 frames back; + # the 6 is just a guess for an upper limit. We don't want to look too far + # back - each level of dependency adds about 12 frames. + frames = caller_locations(8, 6).map(&:base_label) + frames.include?('include_controls') || frames.include?('require_controls') + end end end diff --git a/lib/inspec/runner.rb b/lib/inspec/runner.rb index 36e042f0d6..7d86afb4ba 100644 --- a/lib/inspec/runner.rb +++ b/lib/inspec/runner.rb @@ -75,7 +75,7 @@ def reset @rules = [] end - def load + def load # rubocop: disable Metrics/AbcSize all_controls = [] @target_profiles.each do |profile| @@ -102,6 +102,25 @@ def load all_controls.each do |rule| register_rule(rule) unless rule.nil? + # Check for duplicate controls and warn + next if Inspec::Rule.merge_count(rule).nil? + next unless Inspec::Rule.merge_count(rule) > 0 + profile_id = Inspec::Rule.profile_id(rule) + merges = Inspec::Rule.merge_changes(rule) + merges = merges.select do |merge_location| + # Look for rules that had a merge within the same profile... + merge_location[:profile_id] == profile_id && + !merge_location[:in_include] # and the merge did not happen in an include/require_controls + end + merges.each do |location| + sfile = Inspec::Rule.source_location(rule)[:ref] + sline = Inspec::Rule.source_location(rule)[:line] + cfile = location[:ref] + cline = location[:line] + id = Inspec::Rule.rule_id(rule) + msg = "Control #{id} at #{sfile}:#{sline} is duplicated within the same profile '#{profile_id}' - clobbered by #{cfile}:#{cline}" + Inspec::Log.warn(msg) + end end end diff --git a/test/functional/helper.rb b/test/functional/helper.rb index 2c3c3a9c6e..3a571e2253 100644 --- a/test/functional/helper.rb +++ b/test/functional/helper.rb @@ -4,6 +4,7 @@ require 'helper' require 'rbconfig' +require 'byebug' require 'minitest/hell' class Minitest::Test diff --git a/test/functional/inspec_check_test.rb b/test/functional/inspec_check_test.rb index c219d54bd7..131b596220 100644 --- a/test/functional/inspec_check_test.rb +++ b/test/functional/inspec_check_test.rb @@ -88,6 +88,50 @@ out.exit_status.must_equal 1 out.stdout.must_include 'inspec.yml and inspec.lock are out-of-sync. Please re-vendor with `inspec vendor`.' out.stdout.must_include 'Cannot find linux-baseline in lockfile. Please re-vendor with `inspec vendor`.' - end + end + end + + describe 'inspec check should catch a duplicate control' do + describe 'when a simple profile has duplicates' do + it 'can detect a duplicate control' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'simple') + run_result = inspec('check ' + dupe_profile + ' --format json') + run_result.exit_status.must_equal 1 + json_result = JSON.parse(run_result.stdout) + + # Must detect exactly one error + json_result['errors'].count.must_equal 1 + + json_result['errors'][0]['control_id'].must_equal 'dupe-01' # Find the right control + json_result['errors'][0]['msg'].must_include 'is duplicated in profile dupe-controls' # The kernel of the error message + json_result['errors'][0]['msg'].must_include 'clobbered by' # And we tell you that is was overwritten + json_result['errors'][0]['msg'].must_include 'duplicates.rb:14' # And we tell you what overwrote it + end + end + + describe 'when a profile uses include_controls without a block' do + it 'should not detect a duplicate control' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-simple-include') + run_result = inspec('check ' + dupe_profile + ' --format json') + run_result.exit_status.must_equal 0 + run_result.stderr.must_be_empty + json_result = JSON.parse(run_result.stdout) + + # Must detect no errors + json_result['errors'].count.must_equal 0 + end + end + + describe 'when a profile uses include_controls with a block to modify a control' do + it 'should not detect a duplicate control' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-block-include') + run_result = inspec('check ' + dupe_profile + ' --format json') + json_result = JSON.parse(run_result.stdout) + run_result.exit_status.must_equal 0 + + # Must detect no errors + json_result['errors'].count.must_equal 0 + end + end end end diff --git a/test/functional/inspec_exec_test.rb b/test/functional/inspec_exec_test.rb index 8818397213..5a8e3ae1f0 100644 --- a/test/functional/inspec_exec_test.rb +++ b/test/functional/inspec_exec_test.rb @@ -524,4 +524,43 @@ run_result.stdout.wont_include('1 deprecation warning total') end end + + describe 'when using a profile that contains duplicate controls' do + describe 'when it is a simple profile' do + it 'merges the controls and emits a warning' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'simple') + run_result = inspec('exec ' + dupe_profile) + run_result.exit_status.must_equal 0 + + warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first + warning.wont_be_nil + warning.must_include 'dupe-01' # Which control + warning.must_include "is duplicated within the same profile 'dupe-controls'" + warning.must_include 'duplicates.rb:1' # the original location + warning.must_include 'duplicates.rb:12' # the duplicate location + end + end + + describe 'when it is a wrapper profile with a simple include' do + it 'no duplicate is detected' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-simple-include') + run_result = inspec('exec ' + dupe_profile) + run_result.exit_status.must_equal 0 + + warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first + warning.must_be_nil + end + end + + describe 'when it is a wrapper profile with a block include modifying an inherited control' do + it 'no duplicate is detected' do + dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-block-include') + run_result = inspec('exec ' + dupe_profile) + run_result.exit_status.must_equal 0 + + warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first + warning.must_be_nil + end + end + end end diff --git a/test/unit/mock/profiles/dupe-controls/child/controls/child.rb b/test/unit/mock/profiles/dupe-controls/child/controls/child.rb new file mode 100644 index 0000000000..f9667f2157 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/child/controls/child.rb @@ -0,0 +1,7 @@ +control 'control-01' do + title 'the title set in child' + description 'something' + describe 'describe-01' do + it { should include '01' } + end +end \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/child/inspec.yml b/test/unit/mock/profiles/dupe-controls/child/inspec.yml new file mode 100644 index 0000000000..89f938e874 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/child/inspec.yml @@ -0,0 +1,10 @@ +name: child +title: A simple profile to test inheritance with duplicate control ids +maintainer: The Authors +copyright: The Authors +copyright_email: you@example.com +license: Apache-2.0 +summary: An InSpec Compliance Profile +version: 0.1.0 +supports: + platform: os \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/simple/controls/duplicates.rb b/test/unit/mock/profiles/dupe-controls/simple/controls/duplicates.rb new file mode 100644 index 0000000000..d4ae64933f --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/simple/controls/duplicates.rb @@ -0,0 +1,25 @@ +control 'dupe-01' do + impact 1 + title 'dupe-01-title-one' + description 'something' + + desc 'collision', 'one' + desc 'uniq-one', 'one' + + describe true do + it { should eq false } + end +end + +control 'dupe-01' do + impact 0 + title 'dupe-01-title-two' + description 'something' + + desc 'collision', 'two' + desc 'uniq-two', 'two' + + describe true do + it { should eq true } + end +end \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/simple/inspec.yml b/test/unit/mock/profiles/dupe-controls/simple/inspec.yml new file mode 100644 index 0000000000..39d35ef114 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/simple/inspec.yml @@ -0,0 +1,10 @@ +name: dupe-controls +title: A profile containing duplicated controls, refs github issue 822 +maintainer: The Authors +copyright: The Authors +copyright_email: you@example.com +license: Apache-2.0 +summary: An InSpec Compliance Profile +version: 0.1.0 +supports: + platform: os \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/wrapper-block-include/controls/include.rb b/test/unit/mock/profiles/dupe-controls/wrapper-block-include/controls/include.rb new file mode 100644 index 0000000000..6771aea3df --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/wrapper-block-include/controls/include.rb @@ -0,0 +1,14 @@ +include_controls 'child' do + control 'control-01' do + title 'title set in wrapper-block-include, inside the block' + description 'something' + end +end + +control 'control-01' do + title 'the title set in wrapper-block-include, outside the block' + description 'something' + describe 'describe-01' do + it { should include '01' } + end +end \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/wrapper-block-include/inspec.yml b/test/unit/mock/profiles/dupe-controls/wrapper-block-include/inspec.yml new file mode 100644 index 0000000000..1a752836c7 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/wrapper-block-include/inspec.yml @@ -0,0 +1,13 @@ +name: wrapper-simple-include +title: A simple InSpec Profile to test inheritance with duplicate control IDs with an include block +maintainer: The Authors +copyright: The Authors +copyright_email: you@example.com +license: Apache-2.0 +summary: An InSpec Compliance Profile +version: 0.1.0 +supports: + platform: os +depends: + - name: child + path: ../child \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/controls/include.rb b/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/controls/include.rb new file mode 100644 index 0000000000..3a70c34095 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/controls/include.rb @@ -0,0 +1,9 @@ +include_controls 'child' + +control 'control-01' do + title 'the title set in wrapper-simple-include' + description 'something' + describe 'describe-01' do + it { should include '01' } + end +end \ No newline at end of file diff --git a/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/inspec.yml b/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/inspec.yml new file mode 100644 index 0000000000..eb78604fa8 --- /dev/null +++ b/test/unit/mock/profiles/dupe-controls/wrapper-simple-include/inspec.yml @@ -0,0 +1,13 @@ +name: wrapper-simple-include +title: A simple InSpec Profile to test inheritance with duplicate control IDs with a bare include +maintainer: The Authors +copyright: The Authors +copyright_email: you@example.com +license: Apache-2.0 +summary: An InSpec Compliance Profile +version: 0.1.0 +supports: + platform: os +depends: + - name: child + path: ../child \ No newline at end of file diff --git a/test/unit/profiles/profile_test.rb b/test/unit/profiles/profile_test.rb index b1f42b6a2e..47dd70a85e 100644 --- a/test/unit/profiles/profile_test.rb +++ b/test/unit/profiles/profile_test.rb @@ -49,26 +49,26 @@ end describe 'code info' do - let(:profile_id) { 'complete-profile' } + let(:profile_path) { 'complete-profile' } # Id is 'complete' let(:code) { "control 'test01' do\n impact 0.5\n title 'Catchy title'\n desc '\n example.com should always exist.\n '\n describe host('example.com') do\n it { should be_resolvable }\n end\nend\n" } - let(:loc) { {:ref=>"controls/host_spec.rb", :line=>6} } + let(:loc) { {:ref=>"controls/host_spec.rb", :line=>6, :profile_id => 'complete', :in_include => false } } it 'gets code from an uncompressed profile' do - info = MockLoader.load_profile(profile_id).info + info = MockLoader.load_profile(profile_path).info info[:controls][0][:code].must_equal code - loc[:ref] = File.join(MockLoader.profile_path(profile_id), loc[:ref]) + loc[:ref] = File.join(MockLoader.profile_path(profile_path), loc[:ref]) info[:controls][0][:source_location].must_equal loc end it 'gets code on zip profiles' do - path = MockLoader.profile_zip(profile_id) + path = MockLoader.profile_zip(profile_path) info = MockLoader.load_profile(path).info info[:controls][0][:code].must_equal code info[:controls][0][:source_location].must_equal loc end it 'gets code on tgz profiles' do - path = MockLoader.profile_tgz(profile_id) + path = MockLoader.profile_tgz(profile_path) info = MockLoader.load_profile(path).info info[:controls][0][:code].must_equal code info[:controls][0][:source_location].must_equal loc