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

dromazmj · 2017-08-15T11:35:45Z

Signed-off-by: dromazos [email protected]

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

adamleff

Hi @dromazmj I'm giving this a partial review. Given the feedback I've provided on the other 2 PRs, I'm going to stop here for now and allow you to apply the review feedback from those to all your pending PRs. Please ping me once complete and I'll take another look.

adamleff · 2017-08-17T18:52:26Z

docs/resources/etc_hosts_allow.md.erb

+## Supported Properties
+
+    'daemon_list', 'client_list', 'options'
+


Please apply comments from #2064 and #2065 to the documentation and examples of this PR as well.

It is under my impression that cmp only works with arrays of one value. So for this resource I think we should leave the matcher operator as eq because in most cases rules specify more than one daemon, client or option.

OK, I'm alright with that for the tests that will return an array of values, but let's change to cmp for number and single-element comparisons please.

adamleff · 2017-08-17T18:53:36Z

lib/resources/etc_hosts_allow_deny.rb

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


Remove this line, please.

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

test/unit/mock/files/hosts.allow

+sshd: ALL
+# Added for testing
+LOCAL : [fe80::]/10 : deny
+UNKNOWN, sshd : 127.0.1.154, [:fff:fAb0::] : deny : /etc/bin/


Are you sure about this example? UNKNOWN is a client keyword, not a daemon keyword. I was under the impression that you could only have a single daemon per line.

You're right that UNKNOWN is for clients but there can be more than one daemon specified for a rule. Ill change the UNKNOWN so it is correct.

Actually I looked more into the documentation and you can use ALL, LOCAL, UNKNOWN, etc in the daemon field. So to give more variety of tests Im going to leave this as is.

Firstly, can you link to that documentation? It doesn't match the docs I read yesterday.

If that's the case, I think this requires some additional treatment to duplicate the entries in the FilterTable results. Think of the user experience... a user will likely want to write a test like "make sure 1.2.3.4 can access sshd". They shouldn't have to ask "make sure 1.2.3.4 can access sshd and unknown".

https://www.centos.org/docs/rhel-rg-en-3/s1-tcpwrappers-access.html

And I see what you're saying about the user experience. maybe it would be better to use the include instead something like:
describe etc_hosts_allow.where (daemon_list include 'sshd') do
its ('client_list') { should include '192.168.0.1/24' }
end

Yeah, that's an option, but it requires the user to understand the resource's technical implementation. It doesn't really help them answer the question.

Think about what tcpwrappers is for... it controls access to daemons. A daemon is the primary key. A user shouldn't have to worry about a line protecting multiple daemons -- they want to ask a question about a daemon.

I think the right solution is, while parsing the lines, if there are multiple daemons listed on a line, add multiple entries to the hash used by the FilterTable.

Also, that doc says that the only keyword allowed in the daemon_list is "ALL", not "UNKNOWN"... so we should not use UNKNOWN as a daemon example.

So just to make sure if you have a line such as:
sshd, vsftpd : '192.168.0.1/24'

then you want

sshd : '192.168.0.1/24'
vsftpd : '192.168.0.1/24'
in the hash

I think that's the right thing to do, yes.

adamleff · 2017-08-17T18:58:59Z

lib/resources/etc_hosts_allow_deny.rb

+      end
+
+      # If there is a file and it contains content, continue
+      inspec.file(conf_path).content.lines


Re-reading file again - should probably be raw_conf.lines

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

adamleff

@dromazmj thanks for this PR. I've left some feedback in terms of the implementation of the line parsing. I'm concerned about the complexity and future debuggability of those methods. I provided a potential alternate solution. Let's chat more about this if you have any questions.

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

docs/resources/etc_hosts_allow.md.erb

+# etc_hosts_allow
+
+Use the `etc_hosts_allow` InSpec audit resource to test rules set to accept daemon and client traffic set in /etc/hosts.allow file.
+## Syntax


Newline needed between lines 7 and 8.

adamleff · 2017-08-25T16:09:40Z

docs/resources/etc_hosts_deny.md.erb

+# etc_hosts_deny
+
+Use the `etc_hosts_deny` InSpec audit resource to test rules set to reject daemon and client traffic set in /etc/hosts.deny.
+## Syntax


Newline needed between lines 7 and 8.

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

lib/resources/etc_hosts_allow_deny.rb

+      end.compact
+    end
+
+    def parse_line(line)


While this method, and the parse_attributes method, are creative ways of solving the parsing problem, I think they may be too complicated and there's definitely a high maintenance burden if we have to debug it in the future.

I think this can be a lot simpler. I whipped this method up for some local testing and I think something similar to this would do the trick:

def parse(line) daemons, clients_and_options = line.split(/\s+:\s+/, 2) daemon_list = daemons.split(',').map(&:strip) clients_and_options ||= '' clients, options = clients_and_options.split(' : ', 2) client_list = clients.split(',').map(&:strip) options ||= '' options_list = options.split(': ').map(&:strip) puts "Daemons: #{daemon_list.inspect}" puts "Clients: #{client_list.inspect}" puts "Options: #{options_list.inspect}" end

I'd also like to see some specific unit tests for the method that parses these lines given how complex they can be. The tests should focus on parsing a single line that you hardcode in your tests. The tests might look something like this:

describe 'Inspec::Resources::EtcHostsAllow' do describe '#parse_line' do it 'parses a line with multiple clients' do line = 'sshd : 1.2.3.4, 4.3.2.1' _(resource.parse_line(line)['daemons']).must_equal ['sshd'] _(resource.parse_line(line)['clients']).must_equal ['1.2.3.4', '4.3.2.1'] end it 'parses a line with multiple daemons' do # etc end it 'parses a line with one option' do end it 'parses a line with multiple options' do end end end

Let me know if you have questions about this or if you think my suggestion doesn't properly parse these lines.

I will note that hosts.allow / hosts.deny files can contain entries that span multiple lines and can even reference other files for the client list, for example. We don't have to solve that problem here, but it's something to be aware of and why I want the line parsing methods to be as simple as can be.

The only issue I see with this is when you have an ipv6 address in the client list, because then you cannot split by ':'

It still works because the tcpwrappers spec states that there must be a space before and after the : between the clients and any options.

[14] pry(main)> parse('vsftpd, sshd : 127.0.1.154, [:fff:fAb0::] : deny : /etc/bin/') Daemons: ["vsftpd", "sshd"] Clients: ["127.0.1.154", "[:fff:fAb0::]"] Options: ["deny", "/etc/bin/"]

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

lib/resources/etc_hosts_allow_deny.rb

+    end
+  end
+
+  class EtcHostsDeny < EtcHostsAllow


This is a great job doing subclassing. Thank you for helping keep our codebase only as large as it needs to be!

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

adamleff · 2017-09-05T12:31:06Z

lib/resources/etc_hosts_allow_deny.rb

+      options ||= ''
+      options_list = options.split(': ').map(&:strip)
+
+      puts "Daemons: #{daemon_list.inspect}"


These should be removed. I provided them to you in order to provide output to show how my method worked but they should not be included in your proposed pull request.

adamleff · 2017-09-05T12:32:12Z

lib/resources/etc_hosts_allow_deny.rb

      {
-        'daemon'      => daemon,
+        'daemon'      => daemons.strip,


Why is the strip needed?

adamleff · 2017-09-05T12:35:54Z

test/unit/resources/etc_hosts_allow_deny_test.rb

@@ -23,6 +23,32 @@
      _(entries.client_list).must_include ['127.0.1.154',  '[:fff:fAb0::]']
    end
  end
+
+  describe '#parse_line' do


This isn't quite what I was suggesting, though it's fairly effective.

You can test individual methods, even if they are private (and we should make sure parse_line is private, please!). For example:

it 'parses a line with multiple clients' do line = 'foo: client1, client2 : some_option' _(resource.send(:parse_line, line)).must_equal [ 'client1', 'client2 ' ] end

This allows you test lots and lots of iterations of that method accepting different values by just focusing on that method - you don't exercise FilterTable, the mocked data, etc. Your test remains laser-focused. Given the importance of this one method, we should isolate a number of tests just on that method to ensure it works properly and catch any future regressions.

Okay sounds good, Only test you suggested that wouldnt make sense is to test a line with multiple daemons because I split up lines with mulitiple daemons before that method is ever called.

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

adamleff · 2017-09-11T14:24:57Z

@dromazmj Please fix merge conflicts and then I think this will be ready for final review.

….rb, adding proper solution that tcp wrapper uses to parse the file. Signed-off-by: dromazmj <[email protected]>

adamleff

@dromazmj I think this is ready for final review but we need to rebase on master and cleanup resource.rb and test/helper.rb... your current commits have changes that are not part of the focus on this PR, likely from an attempt to resolve some merge conflicts.

adamleff · 2017-09-18T03:13:21Z

test/helper.rb

@@ -158,6 +158,14 @@ def md.directory?
      'C:/etc/postgresql/9.5/main/pg_ident.conf' => mockfile.call('pg_ident.conf'),
      '/etc/postgresql/9.5/main' => mockfile.call('9.5.main'),
      '/var/lib/postgresql/9.5/main' => mockfile.call('var.9.5.main'),
+      '/etc/hosts.allow' => mockfile.call('hosts.allow'),


There are a bunch of changes in test/helper.rb that have nothing to do with this change. I think you need to rebase onto master. Feel free to squash your commits at this point as well since we're pretty much at final review.

adamleff · 2017-09-18T03:14:03Z

lib/inspec/resource.rb

@@ -89,6 +89,8 @@ def self.validate_resource_dsl_version!(version)
 require 'resources/docker_image'
 require 'resources/docker_container'
 require 'resources/etc_group'
+require 'resources/etc_hosts_allow_deny'
+require 'resources/etc_hosts'


Line 93 has nothing to do with this PR. I think this is likely from a bad merge conflict resolution. Please rebase on master and ensure your changes are only related to your PR focus.

Sounds good, Im having a little trouble Ill need to talk to you or Aaron tomorrow if you have time?

I'm traveling for work today and tomorrow, but I'd be happy to pair with you on Wednesday.

Signed-off-by: dromazos <[email protected]> Modifications to new resource - etc_hosts_allow_deny Signed-off-by: dromazos <[email protected]> Modifications to new resource - etc_hosts_allow_deny Signed-off-by: dromazos <[email protected]> Modification to new resource - etc_hosts_allow_deny Signed-off-by: dromazos <[email protected]> Modification to new resource - etc_hosts_allow_deny Signed-off-by: dromazos <[email protected]> Modifications to new resource - etc_hosts_allow_deny Signed-off-by: dromazmj <[email protected]> Modifications to new resource etc_hosts_allow_deny Signed-off-by: dromazmj <[email protected]> Modification to new resource - etc_hosts_allow_deny Signed-off-by: dromazmj <[email protected]> Resolving merge conflicts with test/helper.rb and lib/inspec/resource.rb, adding proper solution that tcp wrapper uses to parse the file. Signed-off-by: dromazmj <[email protected]>

…pold/inspec into al/etc_hosts_allow_deny

…osts_allow_deny Fixing merge conflicts with inspec/resource.rb and test/helper.rb Signed-off-by: dromazmj <[email protected]>

…al/etc_hosts_allow_deny Signed-off-by: dromazmj <[email protected]>

* Added loading the etc_hosts_allow_deny in lib/resource.rb * Added loading mock files for test in test/unit/resources/helper.rb Signed-off-by: dromazmj <[email protected]>

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

dromazmj · 2017-09-25T14:09:25Z

@adamleff Changes are made and it should be good for you.
* Removed unneeded files that were duplicated after rebasing from master
* Removed DS_Store file

adamleff

This looks good, @dromazmj. You have an unused variable in your #initialize method that we should remove, but this is approved, otherwise. Please send up a commit removing that line. Thanks!

adamleff · 2017-09-25T14:45:26Z

lib/resources/etc_hosts_allow_deny.rb

+    def initialize(hosts_allow_path = nil)
+      return skip_resource 'The `etc_hosts_allow` resource is not supported on your OS.' unless inspec.os.linux?
+      @conf_path      = hosts_allow_path || '/etc/hosts.allow'
+      @files_contents = {}


Unused variable - let's remove.

arlimus

Sweet new resources, thank you @dromazmj !!
Kudos for the reviews and guidance @adamleff as well

adamleff · 2017-09-25T15:21:35Z

@dromazmj as soon as you remove the unused variable, @ me here to let me know and then i'll get this merged. Thanks!

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

dromazmj · 2017-09-25T17:33:04Z

@adamleff I removed the unused variable and pushed the commit

adamleff · 2017-09-25T17:45:28Z

Looks great! Going to merge this in now and it will be part of our next stable release. Thanks!

New Resource-combined etc_hosts_allow etc_hosts_deny

21832ab

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

adamleff changed the title ~~New Resource-combined etc_hosts_allow and etc_hosts_deny~~ etc_hosts_allow and etc_hosts_deny resources: test the configuration of the tcpwrappers configuration files Aug 17, 2017

adamleff changed the title ~~etc_hosts_allow and etc_hosts_deny resources: test the configuration of the tcpwrappers configuration files~~ etc_hosts_allow and etc_hosts_deny resources: test the content of the tcpwrappers configuration files Aug 17, 2017

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

adamleff suggested changes Aug 17, 2017

View reviewed changes

Modifications to new resource - etc_hosts_allow_deny

872fd7f

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

dromazmj requested a review from a team as a code owner August 18, 2017 15:01

dromazmj added 3 commits August 18, 2017 12:26

Modifications to new resource - etc_hosts_allow_deny

4400ea4

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

Modification to new resource - etc_hosts_allow_deny

a312c21

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

Modification to new resource - etc_hosts_allow_deny

5ecc31e

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

adamleff suggested changes Aug 25, 2017

View reviewed changes

Modifications to new resource - etc_hosts_allow_deny

ffd21d8

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

adamleff suggested changes Sep 5, 2017

View reviewed changes

dromazmj added 2 commits September 6, 2017 13:53

Modifications to new resource etc_hosts_allow_deny

d0deefc

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

Modification to new resource - etc_hosts_allow_deny

77b4c83

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

Resolving merge conflicts with test/helper.rb and lib/inspec/resource…

4f8d2db

….rb, adding proper solution that tcp wrapper uses to parse the file. Signed-off-by: dromazmj <[email protected]>

adamleff suggested changes Sep 18, 2017

View reviewed changes

dromazmj added 7 commits September 18, 2017 00:32

Merge branch 'al/etc_hosts_allow_deny' of https://github.com/aaronlip…

410f087

…pold/inspec into al/etc_hosts_allow_deny

Merge branch 'master' of https://github.com/chef/inspec into al/etc_h…

3d5004e

…osts_allow_deny Fixing merge conflicts with inspec/resource.rb and test/helper.rb Signed-off-by: dromazmj <[email protected]>

Merge branch 'master' of https://github.com/aaronlippold/inspec into …

571f956

…al/etc_hosts_allow_deny Signed-off-by: dromazmj <[email protected]>

Readded code that caused merge conflicts

87bf541

* Added loading the etc_hosts_allow_deny in lib/resource.rb * Added loading mock files for test in test/unit/resources/helper.rb Signed-off-by: dromazmj <[email protected]>

Removed duplicated files while retreiving baseline

40527e4

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

Removing duplicated files that got added while rebasing on master.

3f4bf3c

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

adamleff approved these changes Sep 25, 2017

View reviewed changes

arlimus approved these changes Sep 25, 2017

View reviewed changes

Removes unused variable in resource etc_hosts_allow_deny.rb

c93d93c

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

adamleff added the Version: Bump Minor label Sep 25, 2017

adamleff merged commit b23a58b into inspec:master Sep 25, 2017

dromazmj deleted the al/etc_hosts_allow_deny branch September 25, 2017 18:30

zenspider added Expeditor: Bump Minor Version and removed Version: Bump Minor labels Nov 15, 2019

		## Supported Properties

		'daemon_list', 'client_list', 'options'

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

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

Conversation

dromazmj commented Aug 15, 2017 • edited Loading

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

dromazmj Aug 18, 2017 • edited Loading

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

adamleff Aug 28, 2017 • edited Loading

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 commented Sep 11, 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

dromazmj commented Sep 25, 2017 • edited Loading

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

adamleff commented Sep 25, 2017

dromazmj commented Sep 25, 2017

adamleff commented Sep 25, 2017

dromazmj commented Aug 15, 2017 •

edited

Loading

dromazmj Aug 18, 2017 •

edited

Loading

adamleff Aug 28, 2017 •

edited

Loading

dromazmj commented Sep 25, 2017 •

edited

Loading