xinetd_conf resource: fix false positives when config file or directory doesn't exist #2302

eramoto · 2017-11-10T12:21:17Z

Not return 'nil' when xinetd_conf resource checks an empty file or a
non-existent file. So the following test succeeds, where /empty_file is
empty file.

describe xinetd_conf("/empty_file") do
it { should be_disabled }
end

Signed-off-by: ERAMOTO Masaya [email protected]

adamleff · 2017-11-13T23:16:02Z

Thanks for the PR, @eramoto! There's actually a more graceful way for us to handle this now with raising exceptions, but I discovered a bug while building my reply to you. Let me get this bug fixed first (See #2307) and then I'll describe the change I believe will address this in a better way and in a manner in which we plan to write resources going forward.

adamleff

Hi @eramoto! I've provided some feedback on a way to handle this a bit more cleanly and what will become a more standard way of us handling resource failures in the future using Ruby exceptions.

adamleff · 2017-11-14T20:12:05Z

lib/resources/xinetd.rb

@@ -53,12 +53,14 @@ def read_content(path = @conf_path)
      return @contents[path] if @contents.key?(path)
      file = inspec.file(path)
      if !file.file?
-        return skip_resource "Can't find file \"#{path}\""
+        skip_resource "Can't find file \"#{path}\""


With #2307 now merged, a cleaner way to handle this would be to raise an actual Ruby exception. We have two to primary ones to choose from:

Inspec::Exceptions::ResourceSkipped: will skip the resource in the output

Inspec::Exceptions::ResourceFailed: will fail the resource in the output

One could make the argument that a failure is more appropriate here than a skip, but in this case, we should aim to keep consistency since that could be considered a breaking change. So, we can change this line to:

raise Inspec::Exceptions::ResourceSkipped, "Can't find file: #{path}"

... and then you can remove the return nil below it since the raise will stop the resource from continuing on.

adamleff · 2017-11-14T20:12:28Z

lib/resources/xinetd.rb

      end

      @contents[path] = file.content
      if @contents[path].nil? || @contents[path].empty?
-        return skip_resource "Can't read file \"#{path}\""
+        skip_resource "Can't read file \"#{path}\""


Same as above - change to a raised exception, and then we can remove the return nil

Not return 'nil' when xinetd_conf resource checks an empty file or a non-existent file/directory. So the following test succeeds, where /empty_file is a empty file. describe xinetd_conf("/empty_file") do it { should be_disabled } end Signed-off-by: ERAMOTO Masaya <[email protected]>

When the following double check is done for a unreadable file, the second check wrong succeeds. This fixes to register the file which passes all validation tests. describe xinetd_conf("/unreadable_file") do it { should_not be_disabled } it { should be_enabled } end Signed-off-by: ERAMOTO Masaya <[email protected]>

eramoto · 2017-11-15T07:57:26Z

Hi @adamleff ,
Thank you for your good opinion. I reflected this, also added this exception to the check against xinetd directory, and fixed a new bug due to using of an unreadable file.

adamleff

Nicely done, @eramoto!

chris-rock

Thank you for discovering and fixing that bug @eramoto

There are the similar issues as PR inspec#2302. Almost resources return false positives when a file does not exist or is not read. Signed-off-by: ERAMOTO Masaya <[email protected]>

* Create file-check functionality into utility file There are the similar issues as PR #2302. Almost resources return false positives when a file does not exist or is not read. * Replace to file-check functionality * Fix dh_params and x509_certificate resources If a file is empty, OpenSSL::PKey::DH and OpenSSL::X509::Certificate have raised an exception and have skipped the inspection. Thus x509_certificate and dh_params resources are not allowed to read a empty file. * to_s of shadow expects filters is not nil * Remove workaround of sshd_config Removes the workaround of sshd_config since Travis CI fails due to a bug of dev-sec/ssh-baseline and the PR #100 will fix it. * Use init block variable in methods Signed-off-by: ERAMOTO Masaya <[email protected]>

eramoto requested a review from a team as a code owner November 10, 2017 12:21

adamleff suggested changes Nov 14, 2017

View reviewed changes

eramoto added 2 commits November 15, 2017 16:19

eramoto force-pushed the Fix_fake_success_of_xinetd branch from a4130af to b0155f6 Compare November 15, 2017 07:31

adamleff changed the title ~~Fix fake success with xinetd_conf resource~~ xinetd_conf resource: fix false positives with config file or directory doesn't exist Nov 15, 2017

adamleff added the Type: Bug Feature not working as expected label Nov 15, 2017

adamleff approved these changes Nov 15, 2017

View reviewed changes

chris-rock approved these changes Nov 15, 2017

View reviewed changes

adamleff changed the title ~~xinetd_conf resource: fix false positives with config file or directory doesn't exist~~ xinetd_conf resource: fix false positives when config file or directory doesn't exist Nov 15, 2017

adamleff merged commit 986c881 into inspec:master Nov 15, 2017

eramoto mentioned this pull request Dec 1, 2017

Unify method in which file content is read across all resources #2359

Merged

eramoto deleted the Fix_fake_success_of_xinetd branch March 7, 2018 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xinetd_conf resource: fix false positives when config file or directory doesn't exist #2302

xinetd_conf resource: fix false positives when config file or directory doesn't exist #2302

eramoto commented Nov 10, 2017

adamleff commented Nov 13, 2017

adamleff left a comment

adamleff Nov 14, 2017

adamleff Nov 14, 2017

eramoto commented Nov 15, 2017

adamleff left a comment

chris-rock left a comment

xinetd_conf resource: fix false positives when config file or directory doesn't exist #2302

xinetd_conf resource: fix false positives when config file or directory doesn't exist #2302

Conversation

eramoto commented Nov 10, 2017

adamleff commented Nov 13, 2017

adamleff left a comment

Choose a reason for hiding this comment

adamleff Nov 14, 2017

Choose a reason for hiding this comment

adamleff Nov 14, 2017

Choose a reason for hiding this comment

eramoto commented Nov 15, 2017

adamleff left a comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment