Add nil check for sshd config file #2217
Conversation
jquick
force-pushed
the
jq/add_nil_check_sshd_config
branch
from
ef0a84d to
3a284e4
Compare
jquick
changed the title
jquick
force-pushed
the
jq/add_nil_check_sshd_config
branch
2 times, most recently
from
7847e82 to
3f13e43
Compare
jquick
changed the title
There was a problem hiding this comment.
@jquick this is a great first pass at this. We need to make sure we're always using InSpec resource, and not the ruby stdlib, when running commands or reading files... otherwise, we'll break the experience for remote scanned targets.
lib/resources/ssh_conf.rb
Outdated
| @@ -63,7 +63,7 @@ def read_content | |||
| end | |||
|
|
|||
| @content = file.content | |||
| if @content.empty? && !file.empty? | |||
| if @content.nil? || (@content.empty? && File.size?(@conf_path)) | |||
There was a problem hiding this comment.
We can't use the Ruby File class because this will always operate locally on the machine executing InSpec. If the user is scanning a remote target with --target, this call will still happen on the local machine. That's why the call to file was being done (lowercase file referring to the variable storing the inspec.file resource that will work both remotely and locally).
There was a problem hiding this comment.
There is no inspec.file.empty? currently. Should we create one that does a size check accordingly?
There was a problem hiding this comment.
Since we basically just hand over logic on files to Train, it should probably be implemented there. Can we do file.size == 0 for now and address that in a future PR?
| @@ -109,6 +109,8 @@ def md.directory? | |||
| '/proc/net/bonding/bond0' => mockfile.call('bond0'), | |||
| '/etc/ssh/ssh_config' => mockfile.call('ssh_config'), | |||
| '/etc/ssh/sshd_config' => mockfile.call('sshd_config'), | |||
| '/etc/ssh/sshd_config_does_not_exist' => mockfile.call('sshd_config_does_not_exist'), | |||
There was a problem hiding this comment.
I wonder if we should create a fileempty object, much like the empty object you see in the command mocks, so we don't have to create empty mock files. Like this, but for files: https://github.com/chef/inspec/blob/master/test/helper.rb#L339
There was a problem hiding this comment.
Created the emptyfile object for the empty file mock. For this one I want it to point to file it cannot find.
test/unit/resources/ssh_conf_test.rb
Outdated
| it 'check bad path' do | ||
| resource = load_resource('sshd_config', '/etc/ssh/sshd_config_does_not_exist') | ||
| _(resource.send(:read_content)).must_equal "Can't find file \"/etc/ssh/sshd_config_does_not_exist\"" | ||
| assert_nil(resource.Protocol) |
There was a problem hiding this comment.
We have not used assert_nil anywhere in the codebase yet and instead use must_be_nil. We should probably be consistent.
This fixes #1778. There was a issue where if the user did not have read permissions on /etc/ssh/sshd_config it would error out on the empty? check. The fix here is to also look for nil on the file content. Along with this I refactored the inspec file empty? check as it does not exist and was also erroring during my testing. Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/add_nil_check_sshd_config
branch
from
3f13e43 to
46bb47f
Compare
jquick
force-pushed
the
jq/add_nil_check_sshd_config
branch
2 times, most recently
from
1c5c55e to
822ceae
Compare
Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/add_nil_check_sshd_config
branch
from
822ceae to
1e86bbb
Compare
There was a problem hiding this comment.
Thank you @jquick for your contribution! This is a great improvement.
This fixes #1778. There was a issue where if the user did not have read permissions on /etc/ssh/sshd_config it would error out on the empty? check. The fix here is to also look for nil on the file content. Along with this I refactored the inspec file empty? check as it does not exist and was also erroring during my testing. Output with the changes:
Signed-off-by: Jared Quick [email protected]