Merge pull request #11377 from chef/mp/compliance-mode-preflight-validations

Move most compliance validation to pre-run

14 files changed, 166 insertions, 67 deletions

diff --git a/cspell.json b/cspell.json
index a54071a0db..0ee63efc67 100644
--- a/cspell.json
+++ b/cspell.json
@@ -235,6 +235,7 @@
     "cmds",
     "CMDS",
     "Cmnd",
+    "CMPL",
     "cname",
     "codepage",
     "coderanger",
diff --git a/lib/chef/compliance/default_attributes.rb b/lib/chef/compliance/default_attributes.rb
index a1ffff440f..90ddbbc83a 100644
--- a/lib/chef/compliance/default_attributes.rb
+++ b/lib/chef/compliance/default_attributes.rb
@@ -38,11 +38,12 @@ class Chef
       # Allow for connections to HTTPS endpoints using self-signed ssl certificates.
       "insecure" => nil,
 
-      # Controls verbosity of Chef InSpec runner.
+      # Controls verbosity of Chef InSpec runner. See less output when true.
       "quiet" => true,
 
       # Chef Inspec Compliance profiles to be used for scan of node.
-      # See README.md for details
+      # See Compliance Phase documentation for further details:
+      # https://docs.chef.io/chef_compliance_phase/#compliance-phase-configuration
       "profiles" => {},
 
       # Extra inputs passed to Chef InSpec to allow finer-grained control over behavior.
diff --git a/lib/chef/compliance/fetcher/automate.rb b/lib/chef/compliance/fetcher/automate.rb
index 64aff6833a..47dc7f2e6a 100644
--- a/lib/chef/compliance/fetcher/automate.rb
+++ b/lib/chef/compliance/fetcher/automate.rb
@@ -46,13 +46,6 @@ class Chef
 
             config["token"] = Chef::Config[:data_collector][:token]
 
-            if config["token"].nil?
-              raise Inspec::FetcherFailure,
-                "No data-collector token set, which is required by the chef-automate fetcher. " \
-                "Set the `data_collector.token` configuration parameter in your client.rb " \
-                'or use the "chef-server-automate" reporter which does not require any ' \
-                "data-collector settings and uses #{ChefUtils::Dist::Server::PRODUCT} to fetch profiles."
-            end
           end
 
           new(profile_fetch_url, config)
diff --git a/lib/chef/compliance/reporter/automate.rb b/lib/chef/compliance/reporter/automate.rb
index cae0256085..23514c857d 100644
--- a/lib/chef/compliance/reporter/automate.rb
+++ b/lib/chef/compliance/reporter/automate.rb
@@ -28,18 +28,28 @@ class Chef
           @token = Chef::Config[:data_collector][:token]
         end
 
-        # Method used in order to send the inspec report to the data_collector server
-        def send_report(report)
-          unless @entity_uuid && @run_id
-            Chef::Log.error "entity_uuid(#{@entity_uuid}) or run_id(#{@run_id}) can't be nil, not sending report to #{ChefUtils::Dist::Automate::PRODUCT}"
-            return false
+        def validate_config!
+          unless @entity_uuid
+            # TODO - this is a weird leakage of naming from the parent class
+            # but entity_uuid is never an attribute that the user can see;
+            # it is sourced from chef_guid, which we don't technically know about in this class -
+            # but telling the operator about a missing chef_guid is more helpful than telling
+            # them about a missing field they've never heard of. Better would be a dock link
+            # that described how to fix this situation.
+            raise "CMPL004: automate_reporter: chef_guid is not available and must be provided. Aborting because we cannot report the scan."
+          end
+
+          unless @run_id
+            raise "CMPL005: automate_reporter: run_id is not available, aborting because we cannot report the scan."
           end
 
           unless @url && @token
-            Chef::Log.warn "data_collector.token and data_collector.server_url must be defined in client.rb! Further information: https://docs.chef.io/chef_compliance_phase/#direct-reporting-to-chef-automate"
-            return false
+            raise "CMPL006: data_collector.token and data_collector.server_url must be configured in client.rb! Further information: https://docs.chef.io/chef_compliance_phase/#direct-reporting-to-chef-automate"
           end
+        end
 
+        # Method used in order to send the inspec report to the data_collector server
+        def send_report(report)
           headers = {
             "Content-Type" => "application/json",
             "x-data-collector-auth" => "version=1.0",
diff --git a/lib/chef/compliance/reporter/chef_server_automate.rb b/lib/chef/compliance/reporter/chef_server_automate.rb
index 46ca7dc7ba..569fb22426 100644
--- a/lib/chef/compliance/reporter/chef_server_automate.rb
+++ b/lib/chef/compliance/reporter/chef_server_automate.rb
@@ -30,11 +30,6 @@ class Chef
         end
 
         def send_report(report)
-          unless @entity_uuid && @run_id
-            Chef::Log.error "entity_uuid(#{@entity_uuid}) or run_id(#{@run_id}) can't be nil, not sending report to #{ChefUtils::Dist::Automate::PRODUCT}"
-            return false
-          end
-
           automate_report = truncate_controls_results(enriched_report(report), @control_results_limit)
 
           report_size = Chef::JSONCompat.to_json(automate_report, validate_utf8: false).bytesize
@@ -51,6 +46,16 @@ class Chef
           false
         end
 
+        def validate_config!
+          unless @entity_uuid
+            raise "CMPL007: chef_server_automate reporter: chef_guid is not available and must be provided. Aborting because we cannot report the scan"
+          end
+
+          unless @run_id
+            raise "CMPL008: chef_server_automate reporter: run_id is not available, aborting because we cannot report the scan."
+          end
+        end
+
         def http_client
           config = if @insecure
                      Chef::Config.merge(ssl_verify_mode: :verify_none)
@@ -80,7 +85,7 @@ class Chef
           when /404/
             Chef::Log.error "Object does not exist on remote server."
           when /413/
-            Chef::Log.error "You most likely hit the erchef request size in #{ChefUtils::Dist::Server::PRODUCT} that defaults to ~2MB. To increase this limit see the Compliance Phase troubleshooting documentation (http://docs.chef.io/chef_compliance_phase/#troubleshooting) or the Chef Infra Server configuration documentation (https://docs.chef.io/server/config_rb_server/)"
+            Chef::Log.error "You most likely hit the request size limit in #{ChefUtils::Dist::Server::PRODUCT} that defaults to ~2MB. To increase this limit see the Compliance Phase troubleshooting documentation (http://docs.chef.io/chef_compliance_phase/#troubleshooting) or the Chef Infra Server configuration documentation (https://docs.chef.io/server/config_rb_server/)"
           when /429/
             Chef::Log.error "This error typically means the data sent was larger than #{ChefUtils::Dist::Automate::PRODUCT}'s limit (4 MB). Run InSpec locally to identify any controls producing large diffs."
           end
diff --git a/lib/chef/compliance/reporter/cli.rb b/lib/chef/compliance/reporter/cli.rb
index 4905c1f653..7061e5751c 100644
--- a/lib/chef/compliance/reporter/cli.rb
+++ b/lib/chef/compliance/reporter/cli.rb
@@ -22,6 +22,10 @@ class Chef
           puts output.join("\n")
         end
 
+        def validate_config!
+          true
+        end
+
         private
 
         # pastel.decorate is a lightweight replacement for highline.color
diff --git a/lib/chef/compliance/reporter/compliance_enforcer.rb b/lib/chef/compliance/reporter/compliance_enforcer.rb
index 1c63e43b28..47b3a4d2df 100644
--- a/lib/chef/compliance/reporter/compliance_enforcer.rb
+++ b/lib/chef/compliance/reporter/compliance_enforcer.rb
@@ -14,6 +14,10 @@ class Chef
           end
           true
         end
+
+        def validate_config!
+          true
+        end
       end
     end
   end
diff --git a/lib/chef/compliance/reporter/json_file.rb b/lib/chef/compliance/reporter/json_file.rb
index 471d9f64b1..4d074242ca 100644
--- a/lib/chef/compliance/reporter/json_file.rb
+++ b/lib/chef/compliance/reporter/json_file.rb
@@ -13,6 +13,12 @@ class Chef
 
           File.write(@path, Chef::JSONCompat.to_json(report))
         end
+
+        def validate_config!
+          if @path.nil? || @path.class != String || @path.empty?
+            raise "CMPL007: json_file reporter: node['audit']['json_file']['location'] must contain a file path"
+          end
+        end
       end
     end
   end
diff --git a/lib/chef/compliance/runner.rb b/lib/chef/compliance/runner.rb
index c083109875..16cdf780ed 100644
--- a/lib/chef/compliance/runner.rb
+++ b/lib/chef/compliance/runner.rb
@@ -1,17 +1,15 @@
 autoload :Inspec, "inspec"
 
 require_relative "default_attributes"
-require_relative "reporter/automate"
-require_relative "reporter/chef_server_automate"
-require_relative "reporter/compliance_enforcer"
-require_relative "reporter/cli"
-require_relative "reporter/json_file"
 
 class Chef
   module Compliance
     class Runner < EventDispatch::Base
       extend Forwardable
 
+      SUPPORTED_REPORTERS = %w{chef-automate chef-server-automate json-file audit-enforcer cli}.freeze
+      SUPPORTED_FETCHERS = %w{chef-automate chef-server}.freeze
+
       attr_accessor :run_id
       attr_reader :node
       def_delegators :node, :logger
@@ -47,6 +45,15 @@ class Chef
         self.run_id = run_status.run_id
       end
 
+      def converge_start(run_context)
+        # With all attributes - including cookbook - loaded, we now have enough data to validate
+        # configuration.  Because the converge is best coupled with the associated compliance run, these validations
+        # will raise (and abort the converge) if the compliance phase configuration is incorrect/will
+        # prevent compliance phase from completing and submitting its report to all configured reporters.
+        # can abort the converge if the compliance phase configuration (node attributes and client config)
+        load_and_validate!
+      end
+
       def run_completed(_node, _run_status)
         return unless enabled?
 
@@ -85,6 +92,8 @@ class Chef
       end
 
       def report(report = generate_report)
+        # This is invoked at report-time instead of with the normal validations at node loaded,
+        # because we want to ensure that it is visible in the output - and not lost in back-scroll.
         warn_for_deprecated_config_values!
 
         if report.empty?
@@ -92,8 +101,9 @@ class Chef
           return
         end
 
-        Array(node["audit"]["reporter"]).each do |reporter|
-          send_report(reporter, report)
+        Array(node["audit"]["reporter"]).each do |reporter_type|
+          logger.info "Reporting to #{reporter_type}"
+          @reporters[reporter_type].send_report(report)
         end
       end
 
@@ -119,10 +129,8 @@ class Chef
 
       def inspec_profiles
         profiles = node["audit"]["profiles"]
-
-        # TODO: Custom exception class here?
         unless profiles.respond_to?(:map) && profiles.all? { |_, p| p.respond_to?(:transform_keys) && p.respond_to?(:update) }
-          raise "#{Inspec::Dist::PRODUCT_NAME} profiles specified in an unrecognized format, expected a hash of hashes."
+          raise "CMPL010: #{Inspec::Dist::PRODUCT_NAME} profiles specified in an unrecognized format, expected a hash of hashes."
         end
 
         profiles.map do |name, profile|
@@ -138,8 +146,6 @@ class Chef
           require_relative "fetcher/chef_server"
         when nil
           # intentionally blank
-        else
-          raise "Invalid value specified for Compliance Phase's fetcher: '#{node["audit"]["fetcher"]}'. Valid values are 'chef-automate', 'chef-server', or nil."
         end
       end
 
@@ -212,17 +218,10 @@ class Chef
         }
       end
 
-      def send_report(reporter_type, report)
-        logger.info "Reporting to #{reporter_type}"
-
-        reporter = reporter(reporter_type)
-
-        reporter.send_report(report) if reporter
-      end
-
       def reporter(reporter_type)
-        case reporter_type
+        case reporter_type.downcase
         when "chef-automate"
+          require_relative "reporter/automate"
           opts = {
             control_results_limit: node["audit"]["control_results_limit"],
             entity_uuid: node["chef_guid"],
@@ -233,6 +232,7 @@ class Chef
           }
           Chef::Compliance::Reporter::Automate.new(opts)
         when "chef-server-automate"
+          require_relative "reporter/chef_server_automate"
           opts = {
             control_results_limit: node["audit"]["control_results_limit"],
             entity_uuid: node["chef_guid"],
@@ -244,15 +244,16 @@ class Chef
           }
           Chef::Compliance::Reporter::ChefServerAutomate.new(opts)
         when "json-file"
+          require_relative "reporter/json_file"
           path = node["audit"]["json_file"]["location"]
           logger.info "Writing compliance report to #{path}"
           Chef::Compliance::Reporter::JsonFile.new(file: path)
         when "audit-enforcer"
+          require_relative "reporter/compliance_enforcer"
           Chef::Compliance::Reporter::ComplianceEnforcer.new
         when "cli"
+          require_relative "reporter/cli"
           Chef::Compliance::Reporter::Cli.new
-        else
-          raise "'#{reporter_type}' is not a supported reporter for Compliance Phase."
         end
       end
 
@@ -269,6 +270,31 @@ class Chef
         url.path = File.join(url.path, "organizations/#{org}/data-collector")
         url
       end
+
+      # Load the resources required for this runner, and validate configuration
+      # is correct to proceed. Requires node state to be loaded.
+      # Will raise exception if fetcher is not valid, if a reporter is not valid,
+      # or the configuration required by a reporter is not provided.
+      def load_and_validate!
+        return unless enabled?
+
+        @reporters = {}
+        Array(node["audit"]["reporter"]).each do |reporter_type|
+          type = reporter_type.downcase
+          unless SUPPORTED_REPORTERS.include? type
+            raise "CMPL003: '#{reporter_type}' found in node['audit']['reporter'] is not a supported reporter for Compliance Phase. Supported reporters are: #{SUPPORTED_REPORTERS.join(",")}. For more information, see the documentation at https://docs.chef.io/chef_compliance_phase/chef_compliance_runners/#reporters"
+          end
+
+          @reporters[type] = reporter(type)
+          @reporters[type].validate_config!
+        end
+
+        unless (fetcher = node["audit"]["fetcher"]).nil?
+          unless SUPPORTED_FETCHERS.include? fetcher.downcase
+            raise "CMPL002: Unrecognized Compliance Phase fetcher (node['audit']['fetcher'] is #{fetcher}. Supported fetchers are: or #{SUPPORTED_FETCHERS.join(",")}, or nil. For more information, see the documentation at https://docs.chef.io/chef_compliance_phase/chef_compliance_runners/#fetchers"
+          end
+        end
+      end
     end
   end
 end
diff --git a/spec/unit/compliance/fetcher/automate_spec.rb b/spec/unit/compliance/fetcher/automate_spec.rb
index f3554b8b0f..a4cd0c76c3 100644
--- a/spec/unit/compliance/fetcher/automate_spec.rb
+++ b/spec/unit/compliance/fetcher/automate_spec.rb
@@ -29,14 +29,6 @@ describe Chef::Compliance::Fetcher::Automate do
         expect(res.target).to eq(expected)
       end
 
-      it "raises an exception with no data collector token" do
-        Chef::Config[:data_collector].delete(:token)
-
-        expect {
-          Chef::Compliance::Fetcher::Automate.resolve("compliance://namespace/profile_name")
-        }.to raise_error(/No data-collector token set/)
-      end
-
       it "includes the data collector token" do
         expect(Chef::Compliance::Fetcher::Automate).to receive(:new).with(
           "https://automate.test/compliance/profiles/namespace/profile_name/tar",
@@ -108,14 +100,6 @@ describe Chef::Compliance::Fetcher::Automate do
         expect(res.target).to eq(expected)
       end
 
-      it "raises an exception with no data collector token" do
-        Chef::Config[:data_collector].delete(:token)
-
-        expect {
-          Chef::Compliance::Fetcher::Automate.resolve(compliance: "namespace/profile_name")
-        }.to raise_error(Inspec::FetcherFailure, /No data-collector token set/)
-      end
-
       it "includes the data collector token" do
         expect(Chef::Compliance::Fetcher::Automate).to receive(:new).with(
           "https://automate.test/compliance/profiles/namespace/profile_name/tar",
diff --git a/spec/unit/compliance/reporter/automate_spec.rb b/spec/unit/compliance/reporter/automate_spec.rb
index e0a33892b0..60d630d32b 100644
--- a/spec/unit/compliance/reporter/automate_spec.rb
+++ b/spec/unit/compliance/reporter/automate_spec.rb
@@ -1,6 +1,7 @@
 require "spec_helper"
 require "json" # For .to_json
 
+require "chef/compliance/reporter/automate"
 describe Chef::Compliance::Reporter::Automate do
   let(:reporter) { Chef::Compliance::Reporter::Automate.new(opts) }
 
@@ -264,11 +265,34 @@ describe Chef::Compliance::Reporter::Automate do
       expect(metasearch_stub).to have_been_requested
       expect(report_stub).to have_been_requested
     end
+  end
 
-    it "does not send report when entity_uuid is missing" do
+  describe "#validate_config!" do
+    it "raises CMPL004 when entity_uuid is not present" do
       opts.delete(:entity_uuid)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL004/)
+    end
+
+    it "raises CMPL005 when run_id is not present" do
+      opts.delete(:run_id)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL005/)
+    end
+
+    it "raises CMPL006 when data collector URL is missing" do
+      Chef::Config[:data_collector] = { token: "not_nil", server_url: nil }
       reporter = Chef::Compliance::Reporter::Automate.new(opts)
-      expect(reporter.send_report(inspec_report)).to eq(false)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL006/)
+    end
+
+    it "raises CMPL006 when data collector token is missing" do
+      Chef::Config[:data_collector] = { token: nil, server_url: "not_nil" }
+      reporter = Chef::Compliance::Reporter::Automate.new(opts)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL006/)
+    end
+
+    it "otherwise passes" do
+      Chef::Config[:data_collector] = { token: "not_nil", server_url: "not_nil" }
+      reporter.validate_config!
     end
   end
 
diff --git a/spec/unit/compliance/reporter/chef_server_automate_spec.rb b/spec/unit/compliance/reporter/chef_server_automate_spec.rb
index e45a7157ee..33642dea31 100644
--- a/spec/unit/compliance/reporter/chef_server_automate_spec.rb
+++ b/spec/unit/compliance/reporter/chef_server_automate_spec.rb
@@ -1,7 +1,9 @@
 require "spec_helper"
+require "chef/compliance/reporter/chef_server_automate"
 
 describe Chef::Compliance::Reporter::ChefServerAutomate do
   before do
+    # Isn't this already done globally in
     WebMock.disable_net_connect!
 
     Chef::Config[:client_key] = File.expand_path("../../../data/ssl/private_key.pem", __dir__)
@@ -174,4 +176,22 @@ describe Chef::Compliance::Reporter::ChefServerAutomate do
 
     expect(report_stub).to have_been_requested
   end
+
+  describe "#validate_config!" do
+    it "raises CMPL007 when entity_uuid is not present" do
+      opts.delete(:entity_uuid)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL007/)
+    end
+
+    it "raises CMPL008 when run_id is not present" do
+      opts.delete(:run_id)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL008/)
+    end
+
+    it "otherwise passes" do
+      reporter.validate_config!
+    end
+
+  end
+
 end
diff --git a/spec/unit/compliance/reporter/compliance_enforcer_spec.rb b/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
index ae63cf0853..3f3ce6286b 100644
--- a/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
+++ b/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
@@ -1,4 +1,5 @@
 require "spec_helper"
+require "chef/compliance/reporter/compliance_enforcer"
 
 describe Chef::Compliance::Reporter::AuditEnforcer do
   let(:reporter) { Chef::Compliance::Reporter::AuditEnforcer.new }
diff --git a/spec/unit/compliance/runner_spec.rb b/spec/unit/compliance/runner_spec.rb
index c100029a2c..3948970137 100644
--- a/spec/unit/compliance/runner_spec.rb
+++ b/spec/unit/compliance/runner_spec.rb
@@ -130,7 +130,7 @@ describe Chef::Compliance::Runner do
       expect(runner.inspec_profiles).to eq(expected)
     end
 
-    it "raises an error when the profiles are in the old audit-cookbook format" do
+    it "raises a CMPL010 message when the profiles are in the old audit-cookbook format" do
       node.normal["audit"]["profiles"] = [
         {
           name: "Windows 2019 Baseline",
@@ -138,7 +138,7 @@ describe Chef::Compliance::Runner do
         },
       ]
 
-      expect { runner.inspec_profiles }.to raise_error(/profiles specified in an unrecognized format, expected a hash of hashes./)
+      expect { runner.inspec_profiles }.to raise_error(/CMPL010:/)
     end
   end
 
@@ -186,9 +186,29 @@ describe Chef::Compliance::Runner do
       end
     end
 
-    it "fails with unexpected reporter value" do
-      expect { runner.reporter("tacos") }.to raise_error(/'tacos' is not a supported reporter for Compliance Phase/)
+  end
+
+  describe "#load_and_validate! when compliance is enabled" do
+    before do
+      allow(runner).to receive(:enabled?).and_return(true)
+    end
+
+    it "raises CMPL003 when the reporter is not a supported reporter type" do
+      node.normal["audit"]["reporter"] = [ "invalid" ]
+      expect { runner.load_and_validate! }.to raise_error(/^CMPL003:/)
     end
+    it "raises CMPL002 if the configured fetcher is not supported" do
+      node.normal["audit"]["fetcher"] = "invalid"
+      expect { runner.load_and_validate! }.to raise_error(/^CMPL002:/)
+    end
+
+    it "validates configured reporters" do
+      node.normal["audit"]["reporter"] = [ "chef-automate" ]
+      reporter_double = double("reporter", validate_config!: nil)
+      expect(runner).to receive(:reporter).with("chef-automate").and_return(reporter_double)
+      runner.load_and_validate!
+    end
+
   end
 
   describe "#inspec_opts" do