Merge branch 'wavesoftware-feature/using-env-to-hide-dbpassword-in-output'

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
+      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

lib/puppet/type/postgresql_psql.rb

@@ -80,6 +80,20 @@ 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|
+      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."
 

manifests/server/passwd.pp

@@ -12,21 +12,24 @@ 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}'",
+      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",
+      unless      => "${psql_path} -h localhost -p ${port} -c 'select 1' > /dev/null",
       path        => '/usr/bin:/usr/local/bin:/bin',
     }
   }

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}":
+  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}":
+    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,
     }
   }
 }

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

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'
+if fact('osfamily') == 'RedHat' and fact('selinux') == true
-  shell("setenforce 0")
+  shell 'setenforce 0'
 end
 
 describe 'postgres::server', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do

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

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

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