Merge pull request #1240 from DavidS/fix-command-escaping

Fix command shell escaping

Author: sheenaajay

lib/puppet/provider/postgresql_psql/ruby.rb

@@ -16,7 +16,7 @@ def run_sql_command(sql)
     command = [resource[:psql_path]]
     command.push('-d', resource[:db]) if resource[:db]
     command.push('-p', resource[:port]) if resource[:port]
-    command.push('-t', '-X', '-c', '"' + sql.gsub('"', '\"') + '"')
+    command.push('-t', '-X', '-c', sql)
 
     environment = fetch_environment
 
@@ -57,13 +57,13 @@ def fetch_environment
   end
 
   def run_command(command, user, group, environment)
-    command = command.join ' '
     output = Puppet::Util::Execution.execute(command, uid: user,
                                                       gid: group,
                                                       failonfail: false,
                                                       combine: true,
                                                       override_locale: true,
-                                                      custom_environment: environment)
+                                                      custom_environment: environment,
+                                                      sensitive: resource[:sensitive] == :true)
     [output, $CHILD_STATUS.dup]
   end
 end

lib/puppet/type/postgresql_psql.rb

@@ -124,6 +124,13 @@ def matches(value)
     newvalues(:true, :false)
   end
 
+  newparam(:sensitive, boolean: true) do
+    desc "If 'true', then the executed command will not be echoed into the log. Use this to protect sensitive information passing through."
+
+    defaultto(:false)
+    newvalues(:true, :false)
+  end
+
   autorequire(:class) { ['Postgresql::Server::Service'] }
 
   def should_run_sql(refreshing = false)

lib/puppet/util/postgresql_validator.rb

@@ -10,40 +10,41 @@ def initialize(resource)
     end
 
     def build_psql_cmd
-      final_cmd = []
-
-      cmd_init = "#{@resource[:psql_path]} --tuples-only --quiet --no-psqlrc"
-
-      final_cmd.push cmd_init
-
-      cmd_parts = {
-        host: "--host #{@resource[:host]}",
-        port: "--port #{@resource[:port]}",
-        db_username: "--username #{@resource[:db_username]}",
-        db_name: "--dbname #{@resource[:db_name]}",
-        command: "--command '#{@resource[:command]}'",
+      cmd = [@resource[:psql_path], '--tuples-only', '--quiet', '--no-psqlrc']
+
+      args = {
+        host: '--host',
+        port: '--port',
+        db_username: '--username',
+        db_name: '--dbname',
+        command: '--command',
       }
 
-      cmd_parts.each do |k, v|
-        final_cmd.push v if @resource[k]
+      args.each do |k, v|
+        if @resource[k]
+          cmd.push v
+          cmd.push @resource[k]
+        end
       end
 
-      final_cmd.join ' '
+      cmd
     end
 
-    def parse_connect_settings
-      c_settings = @resource[:connect_settings] || {}
-      c_settings['PGPASSWORD'] = @resource[:db_password] if @resource[:db_password]
-      c_settings.map { |k, v| "#{k}=#{v}" }
+    def connect_settings
+      result = @resource[:connect_settings] || {}
+      result['PGPASSWORD'] = @resource[:db_password] if @resource[:db_password]
+      result
     end
 
     def attempt_connection(sleep_length, tries)
       (0..tries - 1).each do |_try|
         Puppet.debug "PostgresqlValidator.attempt_connection: Attempting connection to #{@resource[:db_name]}"
-        Puppet.debug "PostgresqlValidator.attempt_connection: #{build_validate_cmd}"
-        result = execute_command
+        cmd = build_psql_cmd
+        Puppet.debug "PostgresqlValidator.attempt_connection: #{cmd.inspect}"
+        result = Execution.execute(cmd, custom_environment: connect_settings, uid: @resource[:run_as])
+
         if result && !result.empty?
-          Puppet.debug "PostgresqlValidator.attempt_connection: Connection to #{@resource[:db_name] || parse_connect_settings.select { |elem| elem.include?('PGDATABASE') }} successful!"
+          Puppet.debug "PostgresqlValidator.attempt_connection: Connection to #{@resource[:db_name] || connect_settings.select { |elem| elem.include?('PGDATABASE') }} successful!"
           return true
         else
           Puppet.warning "PostgresqlValidator.attempt_connection: Sleeping for #{sleep_length} seconds"
@@ -52,15 +53,5 @@ def attempt_connection(sleep_length, tries)
       end
       false
     end
-
-    private
-
-    def execute_command
-      Execution.execute(build_validate_cmd, uid: @resource[:run_as])
-    end
-
-    def build_validate_cmd
-      "#{parse_connect_settings.join(' ')} #{build_psql_cmd} "
-    end
   end
 end

manifests/server/role.pp

@@ -3,8 +3,8 @@
 # @param update_password If set to true, updates the password on changes. Set this to false to not modify the role's password after creation.
 # @param password_hash Sets the hash to use during password creation.
 # @param createdb Specifies whether to grant the ability to create new databases with this role.
-# @param createrole Specifies whether to grant the ability to create new roles with this role. 
-# @param db Database used to connect to. 
+# @param createrole Specifies whether to grant the ability to create new roles with this role.
+# @param db Database used to connect to.
 # @param port Port to use when connecting.
 # @param login Specifies whether to grant login capability for the new role.
 # @param inherit Specifies whether to grant inherit capability for the new role.
@@ -76,18 +76,16 @@
     $superuser_sql   = $superuser   ? { true => 'SUPERUSER',   default => 'NOSUPERUSER' }
     $replication_sql = $replication ? { true => 'REPLICATION', default => '' }
     if ($password_hash != false) {
-      $environment  = "NEWPGPASSWD=${password_hash}"
-      $password_sql = "ENCRYPTED PASSWORD '\$NEWPGPASSWD'"
+      $password_sql = "ENCRYPTED PASSWORD '${password_hash}'"
     } else {
       $password_sql = ''
-      $environment  = []
     }
 
     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}",
+      command     => Sensitive("CREATE ROLE \"${username}\" ${password_sql} ${login_sql} ${createrole_sql} ${createdb_sql} ${superuser_sql} ${replication_sql} CONNECTION LIMIT ${connection_limit}"),
       unless      => "SELECT 1 FROM pg_roles WHERE rolname = '${username}'",
-      environment => $environment,
       require     => undef,
+      sensitive   => true,
     }
 
     postgresql_psql { "ALTER ROLE \"${username}\" ${superuser_sql}":
@@ -134,9 +132,9 @@
         $pwd_hash_sql = "md5${pwd_md5}"
       }
       postgresql_psql { "ALTER ROLE ${username} ENCRYPTED PASSWORD ****":
-        command     => "ALTER ROLE \"${username}\" ${password_sql}",
-        unless      => "SELECT 1 FROM pg_shadow WHERE usename = '${username}' AND passwd = '${pwd_hash_sql}'",
-        environment => $environment,
+        command     => Sensitive("ALTER ROLE \"${username}\" ${password_sql}"),
+        unless      => Sensitive("SELECT 1 FROM pg_shadow WHERE usename = '${username}' AND passwd = '${pwd_hash_sql}'"),
+        sensitive   => true,
       }
     }
   } else {

spec/acceptance/postgresql_psql_spec.rb

@@ -134,39 +134,25 @@ class { 'postgresql::server': } ->
     apply_manifest(pp_nine, expect_changes: true)
   end
 
-  context 'with secure password passing by environment' do
-    it 'runs SQL that contanins password passed by environment' do
-      select = "select \\'$PASS_TO_EMBED\\'"
-      pp = <<-MANIFEST.unindent
+  context 'when setting sensitive => true' do
+    it 'runs queries without leaking to the log' do
+      select = "select \\'pa$swD\\'"
+      pp = <<~MANIFEST
         class { 'postgresql::server': } ->
-        postgresql_psql { 'password embedded by environment: #{select}':
+        postgresql_psql { 'password protected by sensitive: #{select}':
           db          => 'postgres',
           psql_user   => 'postgres',
+          sensitive   => true,
           command     => '#{select}',
-          environment => [
-            'PASS_TO_EMBED=pa$swD',
-          ],
-        }
-      MANIFEST
-      apply_manifest(pp, catch_failures: true)
-      apply_manifest(pp, expect_changes: false)
-    end
-    it 'runs SQL that contanins password passed by environment in check' do
-      select = "select 1 where \\'$PASS_TO_EMBED\\'=\\'passwD\\'"
-      pp = <<-MANIFEST.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',
-          ],
         }
       MANIFEST
+      result = apply_manifest(pp, catch_failures: true, debug: true)
+      expect(result.stdout).not_to contain('pa$swD')
+      expect(result.stderr).not_to contain('pa$swD')
 
-      idempotent_apply(pp)
+      result = apply_manifest(pp, expect_changes: false, debug: true)
+      expect(result.stdout).not_to contain('pa$swD')
+      expect(result.stderr).not_to contain('pa$swD')
     end
   end
 end

spec/spec_helper_local.rb

@@ -33,7 +33,7 @@
     # this could definitely be optimized
     add_filter do |f|
       # system returns true if exit status is 0, which with git-check-ignore means file is ignored
-      system("git check-ignore --quiet #{f.filename}")
+      system('git', 'check-ignore', '--quiet', f.filename)
     end
   end
 end

spec/unit/defines/server/role_spec.rb

@@ -33,16 +33,16 @@
   it { is_expected.to contain_postgresql__server__role('test') }
   it 'has 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',
+      .with('command'     => 'Sensitive [value redacted]',
+            'sensitive'   => 'true',
             'unless'      => "SELECT 1 FROM pg_roles WHERE rolname = 'test'",
             'port'        => '5432')
   end
   it 'has 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 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'md5b6f7fcbbabb4befde4588a26c1cfd2fa'",
+      .with('command'     => 'Sensitive [value redacted]',
+            'sensitive'   => 'true',
+            'unless'      => 'Sensitive [value redacted]',
             'port'        => '5432')
   end
 
@@ -64,17 +64,17 @@
     it { is_expected.to contain_postgresql__server__role('test') }
     it 'has 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")
-        .with_environment('NEWPGPASSWD=new-pa$s')
+        .with_command('Sensitive [value redacted]')
+        .with_sensitive('true')
         .with_unless("SELECT 1 FROM pg_roles WHERE rolname = 'test'")
         .with_port(5432)
         .with_connect_settings('PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1', 'PGUSER' => 'login-user', 'PGPASSWORD' => 'login-pass')
         .that_requires('Class[postgresql::server::service]')
     end
     it 'has 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 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'md5b6f7fcbbabb4befde4588a26c1cfd2fa'", 'port' => '5432',
+        .with('command' => 'Sensitive [value redacted]', 'sensitive' => 'true',
+              'unless'  => 'Sensitive [value redacted]', 'port' => '5432',
               'connect_settings' => { 'PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1',
                                       'PGUSER' => 'login-user', 'PGPASSWORD' => 'login-pass' })
     end
@@ -99,15 +99,15 @@
     it { is_expected.to contain_postgresql__server__role('test') }
     it 'has 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 1 FROM pg_roles WHERE rolname = 'test'",
-              'connect_settings' => { 'PGHOST'     => 'postgres-db-server', 'DBVERSION' => '9.1',
-                                      'PGPORT'     => '1234', 'PGUSER' => 'login-user', 'PGPASSWORD' => 'login-pass' })
+        .with('command'   => 'Sensitive [value redacted]',
+              'sensitive' => 'true', 'unless' => "SELECT 1 FROM pg_roles WHERE rolname = 'test'",
+              'connect_settings' => { 'PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1',
+                                      'PGPORT' => '1234', 'PGUSER' => 'login-user', 'PGPASSWORD' => 'login-pass' })
     end
     it 'has 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 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'md5b6f7fcbbabb4befde4588a26c1cfd2fa'",
+        .with('command' => 'Sensitive [value redacted]', 'sensitive' => 'true',
+              'unless'  => 'Sensitive [value redacted]',
               'connect_settings' => { 'PGHOST' => 'postgres-db-server', 'DBVERSION' => '9.1',
                                       'PGPORT' => '1234', 'PGUSER' => 'login-user', 'PGPASSWORD' => 'login-pass' })
     end

spec/unit/puppet/provider/postgresql_conn_validator/ruby_spec.rb

@@ -30,37 +30,33 @@
 
   describe '#build_psql_cmd' do
     it 'contains expected commandline options' do
-      expect(provider.validator.build_psql_cmd).to match %r{/usr/bin/psql.*--host.*--port.*--username.*}
+      expect(provider.validator.build_psql_cmd).to eq(['/usr/bin/psql', '--tuples-only', '--quiet', '--no-psqlrc', '--host', 'db.test.com', '--port', 4444, '--username', 'testuser', '--command',
+                                                       'SELECT 1'])
     end
   end
 
-  describe '#parse_connect_settings' do
+  describe 'connect_settings' do
     it 'returns array if password is present' do
-      expect(provider.validator.parse_connect_settings).to eq(['PGPASSWORD=testpass'])
+      expect(provider.validator.connect_settings).to eq({ 'PGPASSWORD' => 'testpass' })
     end
 
     it 'returns an empty array if password is nil' do
       attributes.delete(:db_password)
-      expect(provider.validator.parse_connect_settings).to eq([])
+      expect(provider.validator.connect_settings).to eq({})
     end
 
     it 'returns an array of settings' do
       attributes.delete(:db_password)
       attributes.merge! connect_settings
-      expect(provider.validator.parse_connect_settings).to eq(['PGPASSWORD=testpass', 'PGHOST=db.test.com', 'PGPORT=1234'])
+      expect(provider.validator.connect_settings).to eq({ PGHOST: 'db.test.com', PGPASSWORD: 'testpass', PGPORT: '1234' })
     end
   end
 
   describe '#attempt_connection' do
     let(:sleep_length) { 1 }
     let(:tries) { 3 }
-    let(:exec) do
-      provider.validator.stub(:execute_command).and_return(true)
-    end
 
     it 'tries the correct number of times' do
-      expect(provider.validator).to receive(:execute_command).exactly(3).times
-
       provider.validator.attempt_connection(sleep_length, tries)
     end
   end

spec/unit/puppet/provider/postgresql_psql/ruby_spec.rb

@@ -15,7 +15,7 @@
 
       it 'executes with the given psql_path on the given DB' do
         expect(provider).to receive(:run_command).with(['psql', '-d',
-                                                        attributes[:db], '-t', '-X', '-c', '"SELECT \'something\' as \"Custom column\""'], 'postgres',
+                                                        attributes[:db], '-t', '-X', '-c', 'SELECT \'something\' as "Custom column"'], 'postgres',
                                                        'postgres', {})
 
         provider.run_sql_command('SELECT \'something\' as "Custom column"')
@@ -35,7 +35,7 @@
       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', '-X', '-c', '"SELECT \'something\' as \"Custom column\""'],
+                                                        '-d', attributes[:db], '-t', '-X', '-c', 'SELECT \'something\' as "Custom column"'],
                                                        attributes[:psql_user], attributes[:psql_group], {})
 
         provider.run_sql_command('SELECT \'something\' as "Custom column"')
@@ -50,7 +50,7 @@
 
       it 'executes with the given search_path' do
         expect(provider).to receive(:run_command).with(['psql', '-t', '-X', '-c',
-                                                        '"set search_path to schema1; SELECT \'something\' as \"Custom column\""'],
+                                                        'set search_path to schema1; SELECT \'something\' as "Custom column"'],
                                                        'postgres', 'postgres', {})
 
         provider.run_sql_command('SELECT \'something\' as "Custom column"')
@@ -65,7 +65,7 @@
 
       it 'executes with the given search_path' do
         expect(provider).to receive(:run_command).with(['psql', '-t', '-X', '-c',
-                                                        '"set search_path to schema1,schema2; SELECT \'something\' as \"Custom column\""'],
+                                                        'set search_path to schema1,schema2; SELECT \'something\' as "Custom column"'],
                                                        'postgres', 'postgres',
                                                        {})
 
@@ -79,7 +79,7 @@
     it 'executes with the given port' do
       expect(provider).to receive(:run_command).with(['psql',
                                                       '-p', '5555',
-                                                      '-t', '-X', '-c', '"SELECT something"'],
+                                                      '-t', '-X', '-c', 'SELECT something'],
                                                      'postgres', 'postgres', {})
 
       provider.run_sql_command('SELECT something')
@@ -91,7 +91,7 @@
     it 'executes with the given host' do
       expect(provider).to receive(:run_command).with(['psql',
                                                       '-t', '-X', '-c',
-                                                      '"SELECT something"'],
+                                                      'SELECT something'],
                                                      'postgres', 'postgres', 'PGHOST' => '127.0.0.1')
 
       provider.run_sql_command('SELECT something')