From cfbf2f930811dbcef278218b86cc31ee1e2a1123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Suszy=C5=84ski?= Date: Fri, 14 Nov 2014 15:41:46 +0100 Subject: [PATCH 1/4] Using enviromental varables to hide actual password in psql commands displayed in output and reports. --- lib/puppet/provider/postgresql_psql/ruby.rb | 31 +++++++++++++++++-- lib/puppet/type/postgresql_psql.rb | 15 +++++++++ manifests/server/passwd.pp | 19 +++++++----- manifests/server/role.pp | 23 +++++++++----- spec/unit/classes/server_spec.rb | 19 +++++++++++- spec/unit/defines/server/role_spec.rb | 16 +++++++++- .../provider/postgresql_psql/ruby_spec.rb | 16 +++++----- 7 files changed, 111 insertions(+), 28 deletions(-) diff --git a/lib/puppet/provider/postgresql_psql/ruby.rb b/lib/puppet/provider/postgresql_psql/ruby.rb index 7dd69ac..360116b 100644 --- a/lib/puppet/provider/postgresql_psql/ruby.rb +++ b/lib/puppet/provider/postgresql_psql/ruby.rb @@ -14,7 +14,7 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do command = [resource[:psql_path]] command.push("-d", resource[:db]) if resource[:db] command.push("-p", resource[:port]) if resource[:port] - command.push("-t", "-c", sql) + command.push("-t", "-c", '"' + sql.gsub('"', '\"') + '"') if resource[:cwd] Dir.chdir resource[:cwd] do @@ -27,9 +27,34 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do private + def get_environment + environment = {} + if envlist = resource[:environment] + envlist = [envlist] unless envlist.is_a? Array + envlist.each do |setting| + if setting =~ /^(\w+)=((.|\n)+)$/ + env_name = $1 + value = $2 + if environment.include?(env_name) || environment.include?(env_name.to_sym) + warning "Overriding environment setting '#{env_name}' with '#{value}'" + end + environment[env_name] = value + else + warning "Cannot understand environment setting #{setting.inspect}" + end + end + end + return environment + end + def run_command(command, user, group) + command = command.join ' ' + environment = get_environment if Puppet::PUPPETVERSION.to_f < 3.4 - Puppet::Util::SUIDManager.run_and_capture(command, user, group) + require 'puppet/util/execution' + Puppet::Util::Execution.withenv environment do + Puppet::Util::SUIDManager.run_and_capture(command, user, group) + end else output = Puppet::Util::Execution.execute(command, { :uid => user, @@ -37,7 +62,7 @@ Puppet::Type.type(:postgresql_psql).provide(:ruby) do :failonfail => false, :combine => true, :override_locale => true, - :custom_environment => {} + :custom_environment => environment }) [output, $CHILD_STATUS.dup] end diff --git a/lib/puppet/type/postgresql_psql.rb b/lib/puppet/type/postgresql_psql.rb index d724cf8..5017ee7 100644 --- a/lib/puppet/type/postgresql_psql.rb +++ b/lib/puppet/type/postgresql_psql.rb @@ -80,6 +80,21 @@ Puppet::Type.newtype(:postgresql_psql) do defaultto("/tmp") end + newparam(:environment) do + desc "Any additional environment variables you want to set for a + SQL command. Multiple environment variables should be + specified as an array." + + validate do |values| + values = [values] unless values.is_a? Array + values.each do |value| + unless value =~ /\w+=/ + raise ArgumentError, "Invalid environment setting '#{value}'" + end + end + end + end + newparam(:refreshonly, :boolean => true) do desc "If 'true', then the SQL will only be executed via a notify/subscribe event." diff --git a/manifests/server/passwd.pp b/manifests/server/passwd.pp index 637885d..2b489af 100644 --- a/manifests/server/passwd.pp +++ b/manifests/server/passwd.pp @@ -12,22 +12,25 @@ class postgresql::server::passwd { # without specifying a password ('ident' or 'trust' security). This is # the default for pg_hba.conf. $escaped = postgresql_escape($postgres_password) - $env = "env PGPASSWORD='${postgres_password}'" exec { 'set_postgres_postgrespw': # This command works w/no password because we run it as postgres system # user - command => "${psql_path} -c 'ALTER ROLE \"${user}\" PASSWORD ${escaped}'", - user => $user, - group => $group, - logoutput => true, - cwd => '/tmp', + command => "${psql_path} -c \"ALTER ROLE \\\"${user}\\\" PASSWORD \${NEWPASSWD_ESCAPED}\"", + user => $user, + group => $group, + logoutput => true, + cwd => '/tmp', + environment => [ + "PGPASSWORD='${postgres_password}'", + "NEWPASSWD_ESCAPED='${escaped}'", + ], # With this command we're passing -h to force TCP authentication, which # does require a password. We specify the password via the PGPASSWORD # environment variable. If the password is correct (current), this # command will exit with an exit code of 0, which will prevent the main # command from running. - unless => "${env} ${psql_path} -h localhost -p ${port} -c 'select 1' > /dev/null", - path => '/usr/bin:/usr/local/bin:/bin', + unless => "${psql_path} -h localhost -p ${port} -c 'select 1' > /dev/null", + path => '/usr/bin:/usr/local/bin:/bin', } } } diff --git a/manifests/server/role.pp b/manifests/server/role.pp index a117456..ec1b826 100644 --- a/manifests/server/role.pp +++ b/manifests/server/role.pp @@ -24,9 +24,11 @@ define postgresql::server::role( $superuser_sql = $superuser ? { true => 'SUPERUSER', default => 'NOSUPERUSER' } $replication_sql = $replication ? { true => 'REPLICATION', default => '' } if ($password_hash != false) { - $password_sql = "ENCRYPTED PASSWORD '${password_hash}'" + $environment = "NEWPGPASSWD=${password_hash}" + $password_sql = "ENCRYPTED PASSWORD '\$NEWPGPASSWD'" } else { $password_sql = '' + $environment = [] } Postgresql_psql { @@ -35,12 +37,17 @@ define postgresql::server::role( psql_user => $psql_user, psql_group => $psql_group, psql_path => $psql_path, - require => [ Postgresql_psql["CREATE ROLE \"${username}\" ${password_sql} ${login_sql} ${createrole_sql} ${createdb_sql} ${superuser_sql} ${replication_sql} CONNECTION LIMIT ${connection_limit}"], Class['postgresql::server'] ], + require => [ + Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], + Class['postgresql::server'], + ], } - postgresql_psql {"CREATE ROLE \"${username}\" ${password_sql} ${login_sql} ${createrole_sql} ${createdb_sql} ${superuser_sql} ${replication_sql} CONNECTION LIMIT ${connection_limit}": - unless => "SELECT rolname FROM pg_roles WHERE rolname='${username}'", - require => Class['Postgresql::Server'], + postgresql_psql { "CREATE ROLE ${username} ENCRYPTED PASSWORD ****": + command => "CREATE ROLE \"${username}\" ${password_sql} ${login_sql} ${createrole_sql} ${createdb_sql} ${superuser_sql} ${replication_sql} CONNECTION LIMIT ${connection_limit}", + unless => "SELECT rolname FROM pg_roles WHERE rolname='${username}'", + require => Class['Postgresql::Server'], + environment => $environment, } postgresql_psql {"ALTER ROLE \"${username}\" ${superuser_sql}": @@ -86,8 +93,10 @@ define postgresql::server::role( $pwd_md5 = md5("${password_hash}${username}") $pwd_hash_sql = "md5${pwd_md5}" } - postgresql_psql {"ALTER ROLE \"${username}\" ${password_sql}": - unless => "SELECT usename FROM pg_shadow WHERE usename='${username}' and passwd='${pwd_hash_sql}'", + postgresql_psql { "ALTER ROLE ${username} ENCRYPTED PASSWORD ****": + command => "ALTER ROLE \"${username}\" ${password_sql}", + unless => "SELECT usename FROM pg_shadow WHERE usename='${username}' and passwd='${pwd_hash_sql}'", + environment => $environment, } } } diff --git a/spec/unit/classes/server_spec.rb b/spec/unit/classes/server_spec.rb index 1bba7c5..679755f 100644 --- a/spec/unit/classes/server_spec.rb +++ b/spec/unit/classes/server_spec.rb @@ -26,12 +26,29 @@ describe 'postgresql::server', :type => :class do end describe 'service_ensure => running' do - let(:params) {{ :service_ensure => 'running' }} + let(:params) do + { + :service_ensure => 'running', + :postgres_password => 'new-p@s$word-to-set' + } + end it { is_expected.to contain_class("postgresql::params") } it { is_expected.to contain_class("postgresql::server") } + it { is_expected.to contain_class("postgresql::server::passwd") } it 'should validate connection' do is_expected.to contain_postgresql__validate_db_connection('validate_service_is_running') end + it 'should set postgres password' do + is_expected.to contain_exec('set_postgres_postgrespw').with({ + 'command' => '/usr/bin/psql -c "ALTER ROLE \"postgres\" PASSWORD ${NEWPASSWD_ESCAPED}"', + 'user' => 'postgres', + 'environment' => [ + "PGPASSWORD='new-p@s$word-to-set'", + "NEWPASSWD_ESCAPED='$$new-p@s$word-to-set$$'" + ], + 'unless' => "/usr/bin/psql -h localhost -c 'select 1' > /dev/null", + }) + end end describe 'service_ensure => stopped' do diff --git a/spec/unit/defines/server/role_spec.rb b/spec/unit/defines/server/role_spec.rb index f73b909..8f90855 100644 --- a/spec/unit/defines/server/role_spec.rb +++ b/spec/unit/defines/server/role_spec.rb @@ -19,7 +19,7 @@ describe 'postgresql::server::role', :type => :define do let :params do { - :password_hash => 'test', + :password_hash => 'new-pa$s', } end @@ -28,4 +28,18 @@ describe 'postgresql::server::role', :type => :define do end it { is_expected.to contain_postgresql__server__role('test') } + it 'should have create role for "test" user with password as ****' do + is_expected.to contain_postgresql_psql('CREATE ROLE test ENCRYPTED PASSWORD ****').with({ + 'command' => "CREATE ROLE \"test\" ENCRYPTED PASSWORD '$NEWPGPASSWD' LOGIN NOCREATEROLE NOCREATEDB NOSUPERUSER CONNECTION LIMIT -1", + 'environment' => "NEWPGPASSWD=new-pa$s", + 'unless' => "SELECT rolname FROM pg_roles WHERE rolname='test'", + }) + end + it 'should have alter role for "test" user with password as ****' do + is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****').with({ + 'command' => "ALTER ROLE \"test\" ENCRYPTED PASSWORD '$NEWPGPASSWD'", + 'environment' => "NEWPGPASSWD=new-pa$s", + 'unless' => "SELECT usename FROM pg_shadow WHERE usename='test' and passwd='md5b6f7fcbbabb4befde4588a26c1cfd2fa'", + }) + end end diff --git a/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb b/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb index 2282edf..752162f 100644 --- a/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb +++ b/spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb @@ -14,10 +14,10 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do it "executes with the given psql_path on the given DB" do expect(provider).to receive(:run_command).with(['psql', '-d', - attributes[:db], '-t', '-c', 'SELECT something'], 'postgres', + attributes[:db], '-t', '-c', '"SELECT \'something\' as \"Custom column\""'], 'postgres', 'postgres') - provider.run_sql_command("SELECT something") + provider.run_sql_command('SELECT \'something\' as "Custom column"') end end describe "with psql_path and db" do @@ -32,10 +32,10 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do it "executes with the given psql_path on the given DB" do expect(Dir).to receive(:chdir).with(attributes[:cwd]).and_yield expect(provider).to receive(:run_command).with([attributes[:psql_path], - '-d', attributes[:db], '-t', '-c', 'SELECT something'], + '-d', attributes[:db], '-t', '-c', '"SELECT \'something\' as \"Custom column\""'], attributes[:psql_user], attributes[:psql_group]) - provider.run_sql_command("SELECT something") + provider.run_sql_command('SELECT \'something\' as "Custom column"') end end describe "with search_path string" do @@ -45,10 +45,10 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do it "executes with the given search_path" do expect(provider).to receive(:run_command).with(['psql', '-t', '-c', - 'set search_path to schema1; SELECT something'], + '"set search_path to schema1; SELECT \'something\' as \"Custom column\""'], 'postgres', 'postgres') - provider.run_sql_command("SELECT something") + provider.run_sql_command('SELECT \'something\' as "Custom column"') end end describe "with search_path array" do @@ -58,12 +58,12 @@ describe Puppet::Type.type(:postgresql_psql).provider(:ruby) do it "executes with the given search_path" do expect(provider).to receive(:run_command).with(['psql', '-t', '-c', - 'set search_path to schema1,schema2; SELECT something'], + '"set search_path to schema1,schema2; SELECT \'something\' as \"Custom column\""'], 'postgres', 'postgres' ) - provider.run_sql_command("SELECT something") + provider.run_sql_command('SELECT \'something\' as "Custom column"') end end end From 5419384473805ffef9e87d8dcc3fff6edae36de2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Suszy=C5=84ski?= Date: Sat, 22 Nov 2014 01:28:26 +0100 Subject: [PATCH 2/4] Fix for not running acceptance tests on CentOS box This was caused by fact that SELinux is already disabled and `setenforce 0` returns 1 insted of 0 in that case. --- spec/acceptance/z_alternative_pgdata_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/acceptance/z_alternative_pgdata_spec.rb b/spec/acceptance/z_alternative_pgdata_spec.rb index bf1825a..4a113be 100644 --- a/spec/acceptance/z_alternative_pgdata_spec.rb +++ b/spec/acceptance/z_alternative_pgdata_spec.rb @@ -4,8 +4,8 @@ require 'spec_helper_acceptance' # location properly. # Allow postgresql to use /tmp/* as a datadir -if fact('osfamily') == 'RedHat' - shell("setenforce 0") +if fact('osfamily') == 'RedHat' and fact('selinux') == true + shell 'setenforce 0' end describe 'postgres::server', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do From a9b16b6ccf344d3377297aaad64552a47ea05149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Suszy=C5=84ski?= Date: Sat, 22 Nov 2014 01:29:43 +0100 Subject: [PATCH 3/4] I've added some acceptance tests to 100% check that my method is valid and passwords ane not placed in report --- .gitignore | 1 + lib/puppet/type/postgresql_psql.rb | 3 +- spec/acceptance/postgresql_psql_spec.rb | 37 +++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index b5db85e..b182e96 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ pkg/ Gemfile.lock vendor/ +.vendor/ spec/fixtures/ .vagrant/ .bundle/ diff --git a/lib/puppet/type/postgresql_psql.rb b/lib/puppet/type/postgresql_psql.rb index 5017ee7..6b4519e 100644 --- a/lib/puppet/type/postgresql_psql.rb +++ b/lib/puppet/type/postgresql_psql.rb @@ -86,8 +86,7 @@ Puppet::Type.newtype(:postgresql_psql) do specified as an array." validate do |values| - values = [values] unless values.is_a? Array - values.each do |value| + Array(values).each do |value| unless value =~ /\w+=/ raise ArgumentError, "Invalid environment setting '#{value}'" end diff --git a/spec/acceptance/postgresql_psql_spec.rb b/spec/acceptance/postgresql_psql_spec.rb index 1dd3ee4..2614ad8 100644 --- a/spec/acceptance/postgresql_psql_spec.rb +++ b/spec/acceptance/postgresql_psql_spec.rb @@ -113,4 +113,41 @@ describe 'postgresql_psql', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfa apply_manifest(pp, :expect_changes => true) end end + + context 'with secure password passing by environment' do + it 'should run SQL that contanins password passed by environment' do + select = "select \\'$PASS_TO_EMBED\\'" + pp = <<-EOS.unindent + class { 'postgresql::server': } -> + postgresql_psql { 'password embedded by environment: #{select}': + db => 'postgres', + psql_user => 'postgres', + command => '#{select}', + environment => [ + 'PASS_TO_EMBED=pa$swD', + ], + } + EOS + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :expect_changes => false) + end + it 'should run SQL that contanins password passed by environment in check' do + select = "select 1 where \\'$PASS_TO_EMBED\\'=\\'passwD\\'" + pp = <<-EOS.unindent + class { 'postgresql::server': } -> + postgresql_psql { 'password embedded by environment in check: #{select}': + db => 'postgres', + psql_user => 'postgres', + command => 'invalid sql query', + unless => '#{select}', + environment => [ + 'PASS_TO_EMBED=passwD', + ], + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :expect_changes => false) + end + end end From 6b8c3a952656e61434d161444bc9d9eaaf61b386 Mon Sep 17 00:00:00 2001 From: Hunter Haugen Date: Thu, 12 Mar 2015 10:57:59 -0700 Subject: [PATCH 4/4] Don't make change to .gitignore It will be stomped on by modulesync. And remove trailing whitespace --- .gitignore | 1 - spec/unit/classes/server_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index b182e96..b5db85e 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,6 @@ pkg/ Gemfile.lock vendor/ -.vendor/ spec/fixtures/ .vagrant/ .bundle/ diff --git a/spec/unit/classes/server_spec.rb b/spec/unit/classes/server_spec.rb index 679755f..c6be844 100644 --- a/spec/unit/classes/server_spec.rb +++ b/spec/unit/classes/server_spec.rb @@ -26,8 +26,8 @@ describe 'postgresql::server', :type => :class do end describe 'service_ensure => running' do - let(:params) do - { + let(:params) do + { :service_ensure => 'running', :postgres_password => 'new-p@s$word-to-set' }