From 31e29c254ff64f47a3f41e21c891eadad817817f Mon Sep 17 00:00:00 2001 From: Steven Pritchard Date: Fri, 7 Oct 2022 16:18:42 -0500 Subject: [PATCH] Validate Hiera values * Use compliance_engine gem for data ingest * Check that all fact combinations return hiera data * Pass the logger object from the CLI to the Scelint::Lint class * Dedupe legacy fact warnings * Add `notes` for info-level log items * Bump version Fixes #14 --- Gemfile | 6 +- lib/scelint.rb | 164 ++++++++++++------ lib/scelint/cli.rb | 8 +- lib/scelint/version.rb | 2 +- scelint.gemspec | 6 +- .../SIMP/compliance_profiles/ces.yaml | 6 + .../SIMP/compliance_profiles/checks.yaml | 17 ++ spec/unit/scelint/lint_spec.rb | 31 +++- 8 files changed, 172 insertions(+), 68 deletions(-) create mode 100644 spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/ces.yaml create mode 100644 spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/checks.yaml diff --git a/Gemfile b/Gemfile index ed9a7e0..effe295 100644 --- a/Gemfile +++ b/Gemfile @@ -4,6 +4,7 @@ source 'https://rubygems.org' gemspec gem 'rake', '~> 13.2' +gem 'compliance_engine', git: 'https://github.com/simp/rubygem-simp-compliance_engine.git' group :tests do gem 'rspec', '~> 3.13' @@ -14,6 +15,7 @@ group :tests do end group :development do - gem 'pry' - gem 'pry-byebug' + gem 'pry', '~> 0.14.1' + gem 'pry-byebug', '~> 3.10' + gem 'rdoc', '~> 6.4' end diff --git a/lib/scelint.rb b/lib/scelint.rb index b8ba167..2e181a6 100644 --- a/lib/scelint.rb +++ b/lib/scelint.rb @@ -3,6 +3,8 @@ require 'yaml' require 'json' require 'deep_merge' +require 'logger' +require 'compliance_engine' require 'scelint/version' @@ -126,68 +128,25 @@ class Error < StandardError; end # @example Look for data in all modules in the current directory # lint = Scelint::Lint.new(Dir.glob('*')) class Lint - attr_accessor :data, :errors, :warnings + attr_accessor :data, :errors, :warnings, :notes, :log - def initialize(paths = ['.']) - @data = {} + def initialize(paths = ['.'], logger: Logger.new(STDOUT, level: Logger::INFO)) + @log = logger @errors = [] @warnings = [] + @notes = [] - merged_data = {} - - Array(paths).each do |path| - if File.directory?(path) - [ - 'SIMP/compliance_profiles', - 'simp/compliance_profiles', - ].each do |dir| - ['yaml', 'json'].each do |type| - Dir.glob("#{path}/#{dir}/**/*.#{type}") do |file| - @data[file] = parse(file) - merged_data = merged_data.deep_merge!(Marshal.load(Marshal.dump(@data[file]))) - end - end - end - elsif File.exist?(path) - @data[path] = parse(path) - else - raise "Can't find path '#{path}'" - end - end + @data = ComplianceEngine::Data.new(*Array(paths)) - return nil if @data.empty? - - @data['merged data'] = merged_data - - @data.each do |file, file_data| - lint(file, file_data) - end - end - - def parse(file) - return data[file] if data[file] - - type = case file - when %r{\.yaml$} - 'yaml' - when %r{\.json$} - 'json' - else - errors << "#{file}: Failed to determine file type" - nil - end - begin - return YAML.safe_load(File.read(file)) if type == 'yaml' - return JSON.parse(File.read(file)) if type == 'json' - rescue => e - errors << "#{file}: Failed to parse file: #{e.message}" + @data.files.each do |file| + lint(file, @data.get(file)) end - {} + validate end def files - data.keys - ['merged data'] + data.files end def check_version(file, file_data) @@ -265,7 +224,10 @@ def check_confine(file, file_data) file_data.each_key do |key| warnings << "#{file}: unexpected key '#{key}' in confine '#{file_data}'" if not_ok.include?(key) - warnings << "#{file}: legacy fact '#{key}' in confine '#{file_data}'" if Scelint::LEGACY_FACTS.any? { |legacy_fact| legacy_fact.is_a?(Regexp) ? legacy_fact.match?(key) : (legacy_fact == key) } + if Scelint::LEGACY_FACTS.any? { |legacy_fact| legacy_fact.is_a?(Regexp) ? legacy_fact.match?(key) : (legacy_fact == key) } + warning = "#{file}: legacy fact '#{key}' in confine '#{file_data}'" + warnings << warning unless warnings.include?(warning) + end end end @@ -508,6 +470,100 @@ def check_checks(file, file_data) end end + def normalize_confinement(confine) + normalized = [] + + # Step 1, sort the hash keys + sorted = confine.sort.to_h + + # Step 2, expand all possible combinations of Array values + index = 0 + max_count = 1 + sorted.each_value { |value| max_count *= Array(value).size } + + sorted.each do |key, value| + (index..(max_count - 1)).each do |i| + normalized[i] ||= {} + normalized[i][key] = Array(value)[i % Array(value).size] + end + end + + # Step 3, convert dotted fact names into a facts hash + normalized.map do |c| + c.each_with_object({}) do |(key, value), result| + current = result + parts = key.split('.') + parts.each_with_index do |part, i| + if i == parts.length - 1 + current[part] = value + else + current[part] ||= {} + current = current[part] + end + end + end + end + end + + def confines + return @confines unless @confines.nil? + + @confines = [] + + [:profiles, :ces, :checks, :controls].each do |type| + data.public_send(type).each_value do |value| + # FIXME: This is calling a private method + value.send(:fragments).each_value do |v| + next unless v.is_a?(Hash) + next unless v.key?('confine') + normalize_confinement(v['confine']).each do |confine| + @confines << confine unless @confines.include?(confine) + end + end + end + end + + @confines + end + + def validate + if data.profiles.keys.empty? + notes << 'No profiles found, unable to validate Hiera data' + return nil + end + + # Unconfined, verify that hiera data exists + data.profiles.each_key do |profile| + hiera = data.hiera([profile]) + if hiera.nil? + errors << "Profile '#{profile}': Invalid Hiera data (returned nil)" + next + end + if hiera.empty? + warnings << "Profile '#{profile}': No Hiera data found" + next + end + log.debug "Profile '#{profile}': Hiera data found (#{hiera.keys.count} keys)" + end + + # Again, this time confined + confines.each do |confine| + data.facts = confine + data.profiles.select { |_, value| value.ces&.count&.positive? || value.controls&.count&.positive? }.each_key do |profile| + hiera = data.hiera([profile]) + if hiera.nil? + errors << "Profile '#{profile}': Invalid Hiera data (returned nil) with facts #{confine}" + next + end + if hiera.empty? + warnings << "Profile '#{profile}': No Hiera data found with facts #{confine}" + next + end + log.debug "Profile '#{profile}': Hiera data found (#{hiera.keys.count} keys) with facts #{confine}" + end + end + end + def lint(file, file_data) check_version(file, file_data) check_keys(file, file_data) @@ -516,6 +572,8 @@ def lint(file, file_data) check_ce(file, file_data['ce']) if file_data['ce'] check_checks(file, file_data['checks']) if file_data['checks'] check_controls(file, file_data['controls']) if file_data['controls'] + rescue => e + errors << "#{file}: #{e.message} (not a hash?)" end end end diff --git a/lib/scelint/cli.rb b/lib/scelint/cli.rb index 549d282..40aaa58 100644 --- a/lib/scelint/cli.rb +++ b/lib/scelint/cli.rb @@ -14,7 +14,7 @@ class Scelint::CLI < Thor option :strict, type: :boolean, aliases: '-s', default: false def lint(*paths) paths = ['.'] if paths.nil? || paths.empty? - lint = Scelint::Lint.new(paths) + lint = Scelint::Lint.new(paths, logger: logger) count = lint.files.count @@ -31,6 +31,10 @@ def lint(*paths) logger.warn warning end + lint.notes.each do |note| + logger.info note + end + message = "Checked #{count} files." if lint.errors.count == 0 message += ' No errors.' @@ -45,6 +49,8 @@ def lint(*paths) exit_code = 1 if options[:strict] end + message += " #{lint.notes.count} notes." if lint.notes.count > 0 + logger.info message exit exit_code rescue => e diff --git a/lib/scelint/version.rb b/lib/scelint/version.rb index ea5ccbf..de2d874 100644 --- a/lib/scelint/version.rb +++ b/lib/scelint/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Scelint - VERSION = '0.2.0' + VERSION = '0.3.0' end diff --git a/scelint.gemspec b/scelint.gemspec index 121cf95..3b0dd46 100644 --- a/scelint.gemspec +++ b/scelint.gemspec @@ -24,6 +24,8 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ['lib'] - spec.add_runtime_dependency 'deep_merge', '~> 1.2' - spec.add_runtime_dependency 'thor', '~> 1.3' + spec.add_dependency 'deep_merge', '~> 1.2' + spec.add_dependency 'thor', '~> 1.3' + # FIXME: Add dependency on compliance_engine once it has been released + # spec.add_dependency 'compliance_engine', '~> 0.1.0' end diff --git a/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/ces.yaml b/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/ces.yaml new file mode 100644 index 0000000..0eeced0 --- /dev/null +++ b/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/ces.yaml @@ -0,0 +1,6 @@ +--- +version: 2.0.0 +ce: + 11_ce1: + controls: + 11_control1: true diff --git a/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/checks.yaml b/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/checks.yaml new file mode 100644 index 0000000..2a86d01 --- /dev/null +++ b/spec/fixtures/modules/test_module_11/SIMP/compliance_profiles/checks.yaml @@ -0,0 +1,17 @@ +--- +version: 2.0.0 +checks: + 11_check1: + type: puppet-class-parameter + settings: + parameter: test_module_11::test_param + value: a string + ces: + - 11_ce1 + 11_check2: + type: puppet-class-parameter + settings: + parameter: test_module_11::test_param2 + value: another string + ces: + - 11_ce1 diff --git a/spec/unit/scelint/lint_spec.rb b/spec/unit/scelint/lint_spec.rb index 9d5d0f3..39ac649 100644 --- a/spec/unit/scelint/lint_spec.rb +++ b/spec/unit/scelint/lint_spec.rb @@ -3,11 +3,12 @@ require 'spec_helper' RSpec.describe Scelint::Lint do - # Each test assumes 3 files, no errors, no warnings. + # Each test assumes 3 files, no errors, no warnings, no notes. # Exceptions are listed below. - let(:lint_files) { { '04' => 37 } } + let(:lint_files) { { '04' => 37, '11' => 2 } } let(:lint_errors) { {} } let(:lint_warnings) { { '04' => 17 } } + let(:lint_notes) { { '11' => 1 } } test_modules = Dir.glob(File.join(File.expand_path('../../fixtures', __dir__), 'modules', 'test_module_*')) test_modules.each do |test_module| @@ -26,13 +27,6 @@ expect(lint.files.count).to eq(lint_files[index] || 3) end - it 'has the expected data' do - lint.files.each do |file| - require 'yaml' - expect(lint.data[file]).to eq(YAML.safe_load(File.read(file))) - end - end - it 'has expected errors' do expect(lint.errors).to be_instance_of(Array) pp lint.errors if lint.errors.count != (lint_errors[index] || 0) @@ -44,6 +38,12 @@ pp lint.warnings if lint.warnings.count != (lint_warnings[index] || 0) expect(lint.warnings.count).to eq(lint_warnings[index] || 0) end + + it 'has expected notes' do + expect(lint.notes).to be_instance_of(Array) + pp lint.notes if lint.notes.count != (lint_notes[index] || 0) + expect(lint.notes.count).to eq(lint_notes[index] || 0) + end end end @@ -68,6 +68,12 @@ lint_warnings[index] || 0 end end + let(:total_notes) do + test_modules.sum do |test_module| + index = File.basename(test_module).delete_prefix('test_module_') + lint_notes[index] || 0 + end + end it 'initializes' do expect(lint).to be_instance_of(described_class) @@ -90,5 +96,12 @@ pp lint.warnings if lint.warnings.count != total_warnings expect(lint.warnings.count).to eq(total_warnings) end + + it 'has expected notes' do + pending "The number of notes isn't a simple addition." + expect(lint.notes).to be_instance_of(Array) + pp lint.notes if lint.notes.count != total_notes + expect(lint.notes.count).to eq(total_notes) + end end end