Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New postgres_ident_conf resource #1963

Merged
merged 23 commits into from
Jul 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fa3d145
Initial commit of pg_ident_conf resource
rx294 Jun 23, 2017
308d5c6
Initial commit of pg_ident_conf resource
rx294 Jun 23, 2017
b6d973f
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
rx294 Jun 23, 2017
a953332
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
rx294 Jun 23, 2017
4383801
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
rx294 Jun 23, 2017
45c06f6
Small updates to organization of code
aaronlippold Jun 23, 2017
7444a60
updated `conf_path` instance var to `conf_file` since we are returning
aaronlippold Jun 24, 2017
53e5c67
Updated few bugs on pg_ident_conf
rx294 Jun 25, 2017
7cfa475
Updated docs
rx294 Jun 25, 2017
6150438
Added mock folders
rx294 Jun 25, 2017
d05f9aa
Added mock folders
rx294 Jun 25, 2017
bbeb4a9
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
rx294 Jun 25, 2017
989b4da
Added mock folders
rx294 Jun 25, 2017
280a1bb
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
rx294 Jun 25, 2017
3fff3ff
Added OS check
rx294 Jun 26, 2017
ba6f278
Added mock file
rx294 Jun 26, 2017
14ed39f
Added mock folders
rx294 Jun 25, 2017
ae065ae
added windows mock file
aaronlippold Jun 26, 2017
00744ff
Changed resource name from pg_ident_conf to postgres_ident_conf
rx294 Jun 26, 2017
753b382
Completed corrections reccomended on PR
rx294 Jun 26, 2017
896519e
Merge branch 'al/pg_pg_ident' of https://github.com/aaronlippold/insp…
aaronlippold Jun 26, 2017
09d68a4
removed copyright information
aaronlippold Jul 3, 2017
e0e9304
Merge branch 'master' into al/pg_pg_ident
aaronlippold Jul 3, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions docs/resources/postgres_ident_conf.md.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
---
title: About the postgres_ident_conf Resource
---

# postgres_ident_conf

Use the `postgres_ident_conf` InSpec audit resource to test the client authentication data defined in the pg_hba.conf file.
## Syntax

An `postgres_ident_conf` InSpec audit resource block declares client authentication data that should be tested:

describe postgres_ident_conf.where { pg_username == 'filter_value' } do
its('attribute') { should eq ['value'] }
end

where

* `'attribute'` is a attribute in the pg ident configuration file
* `'filter_value'` is the value that is to be filtered for
* `'value'` is the value that is to be matched expected

## Matchers

This InSpec audit resource matches any service that is listed in the pg ident configuration file:

its('pg_username') { should_not eq ['peer'] }

or:

its('map_name') { should eq ['value'] }

For example:

describe postgres_ident_conf.where { pg_username == 'name' } do
its('system_username') { should eq ['value'] }
its('map_name') { should eq ['value'] }
end

### be

<%= partial "/shared/matcher_be" %>

### cmp

<%= partial "/shared/matcher_cmp" %>

### eq

<%= partial "/shared/matcher_eq" %>

### include

<%= partial "/shared/matcher_include" %>

### match

<%= partial "/shared/matcher_match" %>


## Supported Properties

'conf_file', 'map_name', 'params', 'pg_username', 'system_username'

## Property Examples and Return Types

### map_name([String])

`address` returns a an array of strings that matches the where condition of the filter table

describe pg_hba_conf.where { pg_username == 'name' } do
its('map_name') { should eq ['value'] }
end
### pg_username([String])

`pg_username` returns a an array of strings that matches the where condition of the filter table

describe pg_hba_conf.where { pg_username == 'name' } do
its('pg_username') { should eq ['value'] }
end

### system_username([String])

`system_username` returns a an array of strings that matches the where condition of the filter table

describe pg_hba_conf.where { pg_username == 'name' } do
its('system_username') { should eq ['value'] }
end
1 change: 1 addition & 0 deletions lib/inspec/resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ def self.validate_resource_dsl_version!(version)
require 'resources/packages'
require 'resources/parse_config'
require 'resources/passwd'
require 'resources/postgres_ident_conf'
require 'resources/pip'
require 'resources/port'
require 'resources/postgres'
Expand Down
79 changes: 79 additions & 0 deletions lib/resources/postgres_ident_conf.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# encoding: utf-8
# author: Rony Xavier, rx294@nyu.edu
# author: Aaron Lippold, lippold@gmail.com

require 'resources/postgres'

module Inspec::Resources
class PostgresIdentConf < Inspec.resource(1)
name 'postgres_ident_conf'
desc 'Use the postgres_ident_conf InSpec audit resource to test the client
authentication data is controlled by a pg_ident.conf file.'
example "
describe postgres_ident_conf.where { pg_username == 'acme_user' } do
its('map_name') { should eq ['ssl-test'] }
end
"

attr_reader :params, :conf_file

def initialize(ident_conf_path = nil)
return skip_resource 'The `postgres_ident_conf` resource is not supported on your OS.' unless inspec.os.linux?
@conf_file = ident_conf_path || File.expand_path('pg_ident.conf', inspec.postgres.conf_dir)
@content = nil
@params = nil
read_content
return skip_resource '`pg_ident_conf` is not yet supported on your OS' if inspec.os.windows?
end

filter = FilterTable.create
filter.add_accessor(:where)
.add_accessor(:entries)
.add(:map_name, field: 'map_name')
.add(:system_username, field: 'system_username')
.add(:pg_username, field: 'pg_username')

filter.connect(self, :params)

def to_s
"PostgreSQL Ident Config #{@conf_file}"
end

private

def filter_comments(data)
content = []
data.each do |line|
line.chomp!
content << line unless line.match(/^\s*#/) || line.empty?
end
content
end

def read_content
@content = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line, and line 56, are unnecessary. You already assign these to nil in the initialize method, and it's our mostly-standard practice to keep empty variables set to nil whenever possible.

@params = {}
@content = filter_comments(read_file(@conf_file))
@params = parse_conf(@content)
end

def parse_conf(content)
content.map do |line|
parse_line(line)
end.compact
end

def parse_line(line)
x = line.split(/\s+/)
{
'map_name' => x[0],
'system_username' => x[1],
'pg_username' => x[2],
}
end

def read_file(conf_file = @conf_file)
inspec.file(conf_file).content.lines
end
end
end
7 changes: 7 additions & 0 deletions test/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ def md.directory?
# Test DH parameters, 2048 bit long safe prime, generator 2 for dh_params in PEM format
'dh_params.dh_pem' => mockfile.call('dh_params.dh_pem'),
'default.toml' => mockfile.call('default.toml'),
'/etc/postgresql/9.5/main/pg_ident.conf' => mockfile.call('pg_ident.conf'),
'C:/etc/postgresql/9.5/main/pg_ident.conf' => mockfile.call('pg_ident.conf'),
'/etc/postgresql/9.5/main' => mockfile.call('9.5.main'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line, and 155, look like an error to me. Why would InSpec be trying to read in this directory? We shouldn't have to mock this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since I can't add a review comment on an empty file, the mock files:

  • test/unit/mock/files/var.9.5.main/.gitkeep
  • test/unit/mock/files/9.5.main/.gitkeep

... shouldn't be necessary either. Let's fix whatever is causing you to add these to this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to cover an issue with appvoior given that it is trying to run the unit tests on Windows and failing because we didn't mock for the windows side given that the skip resource was not being respected with FilterTable. This was a work around until we address the open issue #1965 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely because of your direct call to inspec.postgres.conf_dir in the initialize method. Unit tests should not have to rely on OS or filesystem status as it's about testing the code/messages rather than a functional test.

You'll need to add a mock in your unit tests to cover this as adding these directory mockloader calls isn't appropriate.

It will look something like this (writing this off-the-cuff, haven't tested this):

describe 'blah' do
  let(:resource) { load_resource... }
  let(:inspec) { mock }
  let(:postgres) { mock }

  before do
    resource.stubs(:inspec).returns(inspec)
    inspec.stubs(:postgres).returns(postgres)
    postgres.stubs(:conf_dir).returns('/test/path/to/postgres')
  end

  it 'does something' do
    entries = resource.where ...
  end
end

This should stub/mock the call to inspec.postgres.conf_dir so that it always returns the string /test/path/to/postgres so it works regardless of operating system and whether or not PostgreSQL is installed on the machine performing the unit tests. And then I'd change the helper config to mock out /test/path/to/postgres/pg_ident.conf instead of the one in /etc/postgres... so that you're 100% certain your testing just your code, not some integration point on the system itself.

Search the tests in test/unit for additional examples of how to use mock and stubs. Let me know if you get hung up on this, but we have to square this away before this can be merged.

'/var/lib/postgresql/9.5/main' => mockfile.call('var.9.5.main'),
'/var/lib/fake_rpmdb' => mockdir.call(true),
'/var/lib/rpmdb_does_not_exist' => mockdir.call(false),
}
Expand Down Expand Up @@ -341,6 +345,9 @@ def md.directory?
'nc -vz -G 1 example.com 1234' => cmd.call('nc-example-com'),
# host resource: test-netconnection for reachability check on windows
'Test-NetConnection -ComputerName microsoft.com -WarningAction SilentlyContinue -RemotePort 1234| Select-Object -Property ComputerName, TcpTestSucceeded, PingSucceeded | ConvertTo-Json' => cmd.call('Test-NetConnection'),
# postgres tests
%q(bash -c 'type "psql"') => cmd.call('bash -c type psql'),
%q(psql --version | awk '{ print $NF }' | awk -F. '{ print $1"."$2 }') => cmd.call('psql-version'),
# mssql tests
"bash -c 'type \"sqlcmd\"'" => cmd.call('mssql-sqlcmd'),
"cf33896c4bb500abc23dda5b5eddb03cd35a9c46a7358a2c0a0abe41e08a73ae" => cmd.call('mssql-getdate'),
Expand Down
1 change: 1 addition & 0 deletions test/unit/mock/cmd/bash -c type psql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
psql is /usr/bin/psql
1 change: 1 addition & 0 deletions test/unit/mock/cmd/psql-version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
9.5
Empty file.
7 changes: 7 additions & 0 deletions test/unit/mock/files/pg_ident.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# BEGIN ANSIBLE MANAGED BLOCK
# MAPNAME SYSTEM-USERNAME PG-USERNAME
omicron bryanh bryanh
ssl-test ann ann
# bob has user name robert on these machines
pki-users robert bob
# END ANSIBLE MANAGED BLOCK
Empty file.
27 changes: 27 additions & 0 deletions test/unit/resources/postgres_ident_conf_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# encoding: utf-8
# author: Aaron Lippold, lippold@gmail.com
# author: Rony Xavier, rx294@nyu.edu

require 'helper'
require 'inspec/resource'

describe 'Inspec::Resources::PGIdentConf' do
describe 'PGIdentConf Paramaters' do
resource = load_resource('postgres_ident_conf')
it 'Verify postgres_ident_conf filtering by `system_username`' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these are fine tests, this isn't exactly what I was trying to convey with my earlier review. 🙂 Your tests are testing more the .where functionality of FilterTable than you are your parsing engine.

A better more-slimmed-down test might look like this:

it 'parses the pg_ident.conf file correctly' do
  resource.map_name.must_equal ['omicron', 'ssl-test', 'pki-users']
  resource.system_username.must_equal ['bryanh', 'ann', 'robert']
  resource.pg_username.must_equal ['bryanh', 'ann', 'bob']
end

This eliminates any unnecessary calls to FilterTable and focuses solely on this resource's ability to parse your file and populate the FilterTable itself.

You also don't need the two describe statements here... just whittle it down to one and make it match the actual class name of the resource without quotes:

describe Inspec::Resources::PostgresIdentConf do
  it 'parses the pg_ident.conf file correctly' do
    # stuff here
  end
end

Additional describe statements can always be added later to cover individual helper methods, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the corrections and added the tests.Please review them when you get a chance. Thank you again @adamleff !

entries = resource.where { system_username == 'bryanh' }
_(entries.map_name).must_equal ['omicron']
_(entries.pg_username).must_equal ['bryanh']
end
it 'Verify postgres_ident_conf filtering by `map_name`' do
entries = resource.where { map_name == 'ssl-test' }
_(entries.system_username).must_equal ['ann']
_(entries.pg_username).must_equal ['ann']
end
it 'Verify postgres_ident_conf filtering by `pg_username`' do
entries = resource.where { pg_username == 'bob' }
_(entries.map_name).must_equal ['pki-users']
_(entries.system_username).must_equal ['robert']
end
end
end