etc_hosts resource: test the contents of the /etc/hosts file #2065

dromazmj · 2017-08-14T12:50:16Z

Signed-off-by: dromazos [email protected]

Signed-off-by: dromazos <[email protected]>

adamleff

Thanks for your contribution @dromazmj - I've left some feedback for this PR.

adamleff · 2017-08-17T18:35:19Z

docs/resources/etc_hosts.md.erb

+# etc_hosts
+
+Use the `etc_hosts` InSpec audit resource to test rules set to match IP address's with hostnames.
+## Syntax


Need extra newline here.

adamleff · 2017-08-17T18:35:29Z

docs/resources/etc_hosts.md.erb

+
+# etc_hosts
+
+Use the `etc_hosts` InSpec audit resource to test rules set to match IP address's with hostnames.


IP addresses

adamleff · 2017-08-17T18:35:41Z

docs/resources/etc_hosts.md.erb

+Use the `etc_hosts` InSpec audit resource to test rules set to match IP address's with hostnames.
+## Syntax
+
+An etc/hosts rule specifies an IP address and what it's hostname is along with an aliase names it can have.


Also not sure what aliase should be... alias or aliases? Probably alias?

adamleff · 2017-08-17T18:37:10Z

docs/resources/etc_hosts.md.erb

+
+An etc/hosts rule specifies an IP address and what it's hostname is along with an aliase names it can have.
+
+## Syntax


Please see the #2064 PR and apply the same points to the documentation here (remove extra spaces in parens, use the cmp matcher to avoid the double-array stuff, removing the return object type from the headers in each of the supported methods/matchers, etc.)

adamleff · 2017-08-17T18:37:46Z

docs/resources/etc_hosts.md.erb

+      its ( 'all_host_names' ) { should eq [['list']] }
+    end
+
+Use the optional constructor parameter to give an alternative path to hosts file


The example here doesn't use the optional parameter you speak of. 🙂

adamleff · 2017-08-17T18:41:15Z

lib/resources/etc_hosts.rb

+
+  def initialize(hosts_path = nil)
+    return skip_resource 'The `etc_hosts` resource is not supported on your OS.' unless inspec.os.linux? || inspec.os.windows?
+    @conf_path      = get_hosts_path_by_os(hosts_path)


It's more common to handle the override here, rather than in the get_hosts_path_by_os method...

@conf_path = hosts_path || get_hosts_path_by_os

I'd also recommend naming the method default_hosts_file_path so it's clear why the method exists and its purpose.

adamleff · 2017-08-17T18:42:34Z

lib/resources/etc_hosts.rb

+
+  private
+
+  def get_hosts_path_by_os(hosts_path)


I'd rewrite this a bit based on the above comment.

if inspec.os.linux? '/etc/hosts' elsif inspec.os.windows? 'c:\windows\system32\drivers\etc\hosts' end

This produces an error with lint, it tells you to use the return => unless.

I am unable to reproduce that, and likely had to do with the nil checking, not the structure of the conditional. We use this pattern in other resources already. You can see it at the top of the file resource.

adamleff · 2017-08-17T18:43:27Z

lib/resources/etc_hosts.rb

+    {
+      'ip_address'         => line_parts[0],
+      'canonical_hostname' => line_parts[1],
+      'all_host_names'     => line_parts[1..-1] || '',


Shouldn't the default return if there are no elements 1+ be an empty array [] instead of an empty string?

Also, to avoid that condition completely, maybe this should skip any line that has less than 2 line_parts...

line_parts = line.split return unless line_parts.length >= 2 ... etc ...

Then you shouldn't even need to worry about the default value for all_host_names

adamleff · 2017-08-17T18:45:01Z

lib/resources/etc_hosts.rb

+
+    raw_conf = file.content
+    if raw_conf.empty? && !file.empty?
+      return skip_resource("File is empty.\"#{@conf_path}\"")


File isn't empty. Better error message: "Could not read file contents #{@conf_path}"

adamleff · 2017-08-17T18:45:39Z

lib/resources/etc_hosts.rb

+    if raw_conf.empty? && !file.empty?
+      return skip_resource("File is empty.\"#{@conf_path}\"")
+    end
+    inspec.file(conf_path).content.lines


Why are you reading the file again? You've already read it in and stored it in raw_conf. This should probably be raw_conf.lines

Signed-off-by: dromazos <[email protected]>

adamleff

Some final cleanup and we should be good to go, @dromazmj!

adamleff · 2017-08-25T16:04:08Z

lib/resources/etc_hosts.rb

+# author: Matthew Dromazos
+
+require 'utils/parser'
+require 'pry'


Still needs to be removed, please!

adamleff · 2017-08-25T16:04:25Z

lib/resources/etc_hosts.rb

+
+  def initialize(hosts_path = nil)
+    return skip_resource 'The `etc_hosts` resource is not supported on your OS.' unless inspec.os.linux? || inspec.os.windows?
+    @conf_path      = hosts_path || default_hosts_file_path(hosts_path)


You're doing an || here (which is great), which means default_hosts_file_path doesn't need to receive it as an argument. It will simplify the default_hosts_file_path method as well.

adamleff · 2017-08-25T16:06:11Z

lib/resources/etc_hosts.rb

+
+  private
+
+  def default_hosts_file_path(hosts_path)


As per above, hosts_path should be needed here; this method should just return the appropriate default and let the initialize method either call it or not call it depending on whether the user supplied a path. I'd really simplify down to one line:

inspec.os.windows? ? 'C:\windows\system32\drivers\etc\hosts' : '/etc/hosts'

adamleff · 2017-08-25T16:07:24Z

lib/resources/etc_hosts.rb

+    line_parts = line.split
+    return nil unless line_parts.length >= 2
+    {
+      'ip_address'         => line_parts[0],


Can you please align these or remove the extraneous spaces? Lines 64-66. Either way is fine as long as it's consistent. Thanks!

adamleff · 2017-08-25T16:07:55Z

lib/resources/etc_hosts.rb

+
+    raw_conf = file.content
+    if raw_conf.empty? && !file.empty?
+      return skip_resource("Could not read file contents\"#{@conf_path}\"")


Missing a space between contents and \"

Signed-off-by: dromazmj <[email protected]>

adamleff

This looks good to me. We'll need to fix the conflict in test/helper.rb but I'd hold off on doing that until right before we merge after getting a 2nd approval or we're just going to have to come back and fix it again due to the other pending PRs that may get merged before this.

Thanks, @dromazmj.

arlimus

Amazing addition @dromazmj thank you so much!! This is great 😁

Thank you for your guidance in reviewing @adamleff

dromazmj added 5 commits August 11, 2017 08:55

Push request for etc_hosts

958618b

Signed-off-by: dromazos <[email protected]>

New Resource - etc_hosts.

4c9d3cd

Signed-off-by: dromazos <[email protected]>

New Resource - etc_hosts

74504a9

Signed-off-by: dromazos <[email protected]>

New Resource - etc_hosts

d6cf548

Signed-off-by: dromazos <[email protected]>

New Resource - etc_hosts

244dcf7

Signed-off-by: dromazos <[email protected]>

dromazmj closed this Aug 15, 2017

dromazmj reopened this Aug 15, 2017

dromazmj closed this Aug 15, 2017

dromazmj reopened this Aug 15, 2017

adamleff changed the title ~~New Resource - etc_hosts~~ etc_hosts resource: test the contents of the /etc/hosts file Aug 17, 2017

adamleff suggested changes Aug 17, 2017

View reviewed changes

adamleff added the Type: New Resource label Aug 17, 2017

adamleff mentioned this pull request Aug 17, 2017

etc_hosts_allow and etc_hosts_deny resources: test the content of the tcpwrappers configuration files #2073

Merged

Modifications to new resource - etc_hosts

d32019b

Signed-off-by: dromazos <[email protected]>

dromazmj requested a review from a team as a code owner August 18, 2017 14:44

adamleff suggested changes Aug 25, 2017

View reviewed changes

Modifications to new resource etc_hosts

e09bb0f

Signed-off-by: dromazmj <[email protected]>

adamleff approved these changes Aug 30, 2017

View reviewed changes

adamleff requested a review from a team August 30, 2017 17:31

arlimus approved these changes Aug 30, 2017

View reviewed changes

Merge branch 'master' into al/etc_hosts

a09f0be

adamleff merged commit cb5b475 into inspec:master Aug 31, 2017

adamleff deleted the al/etc_hosts branch August 31, 2017 13:51

adamleff restored the al/etc_hosts branch August 31, 2017 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etc_hosts resource: test the contents of the /etc/hosts file #2065

etc_hosts resource: test the contents of the /etc/hosts file #2065

dromazmj commented Aug 14, 2017

adamleff left a comment

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

dromazmj Aug 18, 2017

adamleff Aug 18, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff left a comment

adamleff Aug 25, 2017

adamleff Aug 25, 2017

adamleff Aug 25, 2017

adamleff Aug 25, 2017

adamleff Aug 25, 2017

adamleff left a comment

arlimus left a comment


		# etc_hosts

		Use the `etc_hosts` InSpec audit resource to test rules set to match IP address's with hostnames.


		An etc/hosts rule specifies an IP address and what it's hostname is along with an aliase names it can have.

		## Syntax

etc_hosts resource: test the contents of the /etc/hosts file #2065

etc_hosts resource: test the contents of the /etc/hosts file #2065

Conversation

dromazmj commented Aug 14, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment