Please review pull request #418: (#5224) Unset USER-related env vars during execs opened by (cprice-puppet)
Description:
This bug report was related to the fact that you may end up with very different values for environment variables such as "HOME", "USER", "LOGNAME" depending on how you start puppet (at boot time, via "service", via /etc/init.d, etc.). These variations can have an effect on the behavior of programs / services that are launched by puppet, particularly those that are triggered via Exec resources. Change the behavior such that these environment variables are simply unset by puppet prior to executing commands.
- Opened: Fri Jan 27 22:50:14 UTC 2012
- Based on: puppetlabs:master (7630671ec38d754464898ba3ad68caab144dc89b)
- Requested merge: cprice-puppet:bug/master/5224-user-env-vars-during-Exec (e44a8abd7dc7b2cac4c05eb7a50b53bdd71a1cbc)
Diff follows:
diff --git a/acceptance/pending/ticket_11860_exec_should_not_override_locale.rb b/acceptance/pending/ticket_11860_exec_should_not_override_locale.rb
index 69f31fd..1ff1a30 100644
--- a/acceptance/pending/ticket_11860_exec_should_not_override_locale.rb
+++ b/acceptance/pending/ticket_11860_exec_should_not_override_locale.rb
@@ -16,12 +16,6 @@
#
#######################################################################################
-# utility method that adds an extra backslash to the double quotes in a string; this
-# just lets us keep our manifest strings looking normal
-def escape_quotes(str)
- str.gsub("\"", '\"')
-end
-
temp_file_name = "/tmp/11860_exec_should_not_override_locale.txt"
locale_string = "es_ES.UTF-8"
diff --git a/acceptance/pending/ticket_5224_exec_should_unset_user_env_vars.rb b/acceptance/pending/ticket_5224_exec_should_unset_user_env_vars.rb
new file mode 100644
index 0000000..8f7d821
--- /dev/null
+++ b/acceptance/pending/ticket_5224_exec_should_unset_user_env_vars.rb
@@ -0,0 +1,109 @@
+test_name "#5224: exec resources should unset user-related environment variables"
+
+#######################################################################################
+# NOTE
+#######################################################################################
+#
+# This test depends on the following pull requests:
+#
+# https://github.com/puppetlabs/puppet-acceptance/pull/123
+#
+# because it needs to be able to set some environment variables for the duration of
+# the puppet commands. Shouldn't be moved out of 'pending' until after that has been
+# merged.
+#
+#######################################################################################
+
+
+temp_file_name = "/tmp/5224_exec_should_unset_user_env_vars.txt"
+sentinel_string = "Abracadabra"
+
+
+# these should match up with the value of Puppet::Util::POSIX_USER_ENV_VARS,
+# but I don't have access to that from here, so this is unfortunately hard-coded
+# (cprice 2012-01-27)
+POSIX_USER_ENV_VARS = ['HOME', 'USER', 'LOGNAME']
+
+
+
+step "Check value of user-related environment variables"
+
+# in this step we are going to run some "exec" blocks that writes the value of the
+# user-related environment variables to a file. We need to verify that exec's are
+# unsetting these vars.
+
+
+test_printenv_manifest = <<HERE
+exec {"print %s environment variable":
+ command => "/usr/bin/printenv %s > #{temp_file_name}",
+}
+HERE
+
+# loop over the vars that we care about; these should match up with the value of Puppet::Util::POSIX_USER_ENV_VARS,
+# but I don't have access to that from here, so this is unfortunately hard-coded (cprice 2012-01-27)
+POSIX_USER_ENV_VARS.each do |var|
+
+ # apply the manifest.
+ #
+ # note that we are passing in an extra :environment argument, which will cause the
+ # framework to temporarily set this variable before executing the puppet command.
+ # this lets us know what value we should be looking for as the output of the exec.
+
+ apply_manifest_on agents, test_printenv_manifest % [var, var], :environment => {var => sentinel_string}
+
+ # cat the temp file and make sure it contained the correct value.
+ on(agents, "cat #{temp_file_name}").each do |result|
+ assert_equal("", "#{result.stdout.chomp}", "Unexpected result for host '#{result.host}', environment var '#{var}'")
+ end
+end
+
+
+
+
+step "Check value of user-related environment variables when they are provided as part of the exec resource"
+
+# in this step we are going to run some "exec" blocks that write the value of the
+# user-related environment variables to a file. However, this time, the manifest
+# explicitly overrides these variables in the "environment" section, so we need to
+# be sure that we are respecting these overrides.
+
+test_printenv_with_env_overrides_manifest = <<HERE
+exec {"print %s environment variable":
+ command => "/usr/bin/printenv %s > #{temp_file_name}",
+ environment => ["%s=#{sentinel_string}", "FOO=bar"]
+}
+HERE
+
+# loop over the vars that we care about;
+POSIX_USER_ENV_VARS.each do |var|
+
+ # apply the manifest.
+ #
+ # note that we are passing in an extra :environment argument, which will cause the
+ # framework to temporarily set this variable before executing the puppet command.
+ # this lets us know what value we should be looking for as the output of the exec.
+
+ apply_manifest_on agents, test_printenv_with_env_overrides_manifest % [var, var, var],
+ :environment => {var => sentinel_string}
+
+ # cat the temp file and make sure it contained the correct value.
+ on(agents, "cat #{temp_file_name}").each do |result|
+ assert_equal(sentinel_string, "#{result.stdout.chomp}",
+ "Unexpected result for host '#{result.host}', environment var '#{var}'")
+ end
+end
+
+
+
+
+
+
+
+
+step "cleanup"
+
+# remove the temp file
+on agents, "rm -f #{temp_file_name}"
+
+
+
diff --git a/lib/puppet/provider/exec.rb b/lib/puppet/provider/exec.rb
index 1538a40..041016d 100644
--- a/lib/puppet/provider/exec.rb
+++ b/lib/puppet/provider/exec.rb
@@ -44,19 +44,21 @@ def run(command, check = false)
end
end
- withenv environment do
- Timeout::timeout(resource[:timeout]) do
- # note that we are passing "false" for the "override_locale" parameter, which ensures that the user's
- # default/system locale will be respected. Callers may override this behavior by setting locale-related
- # environment variables (LANG, LC_ALL, etc.) in their 'environment' configuration.
- output, status = Puppet::Util::SUIDManager.
- run_and_capture(command, resource[:user], resource[:group], :override_locale => false)
- end
- # The shell returns 127 if the command is missing.
- if status.exitstatus == 127
- raise ArgumentError, output
- end
+
+ Timeout::timeout(resource[:timeout]) do
+ # note that we are passing "false" for the "override_locale" parameter, which ensures that the user's
+ # default/system locale will be respected. Callers may override this behavior by setting locale-related
+ # environment variables (LANG, LC_ALL, etc.) in their 'environment' configuration.
+ output, status = Puppet::Util::SUIDManager.
+ run_and_capture(command, resource[:user], resource[:group],
+ :override_locale => false,
+ :custom_environment => environment)
end
+ # The shell returns 127 if the command is missing.
+ if status.exitstatus == 127
+ raise ArgumentError, output
+ end
+
end
rescue Errno::ENOENT => detail
self.fail detail.to_s
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 2473723..dece838 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -28,6 +28,10 @@ module Util
POSIX_LOCALE_ENV_VARS = ['LANG', 'LC_ALL', 'LC_MESSAGES', 'LANGUAGE',
'LC_COLLATE', 'LC_CTYPE', 'LC_MONETARY', 'LC_NUMERIC', 'LC_TIME']
+ # This is a list of user-related environment variables that we will unset when we want to provide a pristine
+ # environment for "exec" runs
+ POSIX_USER_ENV_VARS = ['HOME', 'USER', 'LOGNAME']
+
def self.activerecord_version
if (defined?(::ActiveRecord) and defined?(::ActiveRecord::VERSION) and defined?(::ActiveRecord::VERSION::MAJOR) and defined?(::ActiveRecord::VERSION::MINOR))
([::ActiveRecord::VERSION::MAJOR, ::ActiveRecord::VERSION::MINOR].join('.').to_f)
@@ -308,10 +312,23 @@ def execute_posix(command, arguments, stdin, stdout, stderr)
# loop over them and clear them
POSIX_LOCALE_ENV_VARS.each { |name| ENV.delete(name) }
# set LANG and LC_ALL to 'C' so that the command will have consistent, predictable output
+ # it's OK to manipulate these directly rather than, e.g., via "withenv", because we are in
+ # a forked process.
ENV['LANG'] = 'C'
ENV['LC_ALL'] = 'C'
end
- Kernel.exec(*command)
+
+ # unset all of the user-related environment variables so that different methods of starting puppet
+ # (automatic start during boot, via 'service', via /etc/init.d, etc.) won't have unexpected side
+ # effects relating to user / home dir environment vars.
+ # it's OK to manipulate these directly rather than, e.g., via "withenv", because we are in
+ # a forked process.
+ POSIX_USER_ENV_VARS.each { |name| ENV.delete(name) }
+
+ arguments[:custom_environment] ||= {}
+ Puppet::Util::Execution.withenv(arguments[:custom_environment]) do
+ Kernel.exec(*command)
+ end
rescue => detail
puts detail.message
puts detail.backtrace if Puppet[:trace]
@@ -328,7 +345,11 @@ def execute_windows(command, arguments, stdin, stdout, stderr)
part.include?(' ') ? %Q["#{part.gsub(/"/, '\"')}"] : part
end.join(" ") if command.is_a?(Array)
- process_info = Process.create( :command_line => command, :startup_info => {:stdin => stdin, :stdout => stdout, :stderr => stderr} )
+ arguments[:custom_environment] ||= {}
+ process_info =
+ Puppet::Util::Execution.withenv(arguments[:custom_environment]) do
+ Process.create( :command_line => command, :startup_info => {:stdin => stdin, :stdout => stdout, :stderr => stderr} )
+ end
process_info.process_id
end
module_function :execute_windows
@@ -348,6 +369,8 @@ def execute_windows(command, arguments, stdin, stdout, stderr)
# the user/system locale to "C" (via environment variables LANG and LC_*) while we are executing the command.
# This ensures that the output of the command will be formatted consistently, making it predictable for parsing.
# Passing in a value of false for this option will allow the command to be executed using the user/system locale.
+ # :custom_environment (default {}) -- a hash of key/value pairs to set as environment variables for the duration
+ # of the command
def execute(command, arguments = {})
# specifying these here rather than in the method signature to allow callers to pass in a partial
@@ -360,6 +383,7 @@ def execute(command, arguments = {})
:stdinfile => nil,
:squelch => false,
:override_locale => true,
+ :custom_environment => {},
}
arguments = default_arguments.merge(arguments)
diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb
index 4864859..dd21f85 100644
--- a/lib/puppet/util/suidmanager.rb
+++ b/lib/puppet/util/suidmanager.rb
@@ -157,22 +157,26 @@ def initgroups(user)
# [new_gid] (optional) a groupid to run the command as
# [options] (optional, defaults to {}) a hash of option key/value pairs; currently supported:
# :override_locale (defaults to true) a flag indicating whether or puppet should temporarily override the
- # system locale for the duration of the command. If true, the locale will be set to 'C' to ensure consistent
- # output / formatting from the command, which makes it much easier to parse the output. If false, the system
- # locale will be respected.
+ # system locale for the duration of the command. If true, the locale will be set to 'C' to ensure consistent
+ # output / formatting from the command, which makes it much easier to parse the output. If false, the system
+ # locale will be respected.
+ # :custom_environment (default {}) -- a hash of key/value pairs to set as environment variables for the duration
+ # of the command
def run_and_capture(command, new_uid=nil, new_gid=nil, options = {})
# specifying these here rather than in the method signature to allow callers to pass in a partial
# set of overrides without affecting the default values for options that they don't pass in
default_options = {
:override_locale => true,
+ :custom_environment => {},
}
options = default_options.merge(options)
output = Puppet::Util.execute(command, :failonfail => false, :combine => true,
:uid => new_uid, :gid => new_gid,
- :override_locale => options[:override_locale])
+ :override_locale => options[:override_locale],
+ :custom_environment => options[:custom_environment])
[output, $CHILD_STATUS.dup]
end
module_function :run_and_capture
diff --git a/spec/unit/provider/exec/posix_spec.rb b/spec/unit/provider/exec/posix_spec.rb
index 76158a9..bf0036d 100755
--- a/spec/unit/provider/exec/posix_spec.rb
+++ b/spec/unit/provider/exec/posix_spec.rb
@@ -113,12 +113,13 @@ def make_exe
@logs.map {|l| "#{l.level}: #{l.message}" }.should == ["warning: Overriding environment setting 'WHATEVER' with '/foo'"]
end
+
describe "posix locale settings", :unless => Puppet.features.microsoft_windows? do
# a sentinel value that we can use to emulate what locale environment variables might be set to on an international
# system.
lang_sentinel_value = "es_ES.UTF-8"
# a temporary hash that contains sentinel values for each of the locale environment variables that we override in
- # "execute"
+ # "exec"
locale_sentinel_env = {}
Puppet::Util::POSIX_LOCALE_ENV_VARS.each { |var| locale_sentinel_env[var] = lang_sentinel_value }
@@ -151,5 +152,53 @@ def make_exe
output.strip.should == 'bar'
end
end
+
+ describe "posix user-related environment vars", :unless => Puppet.features.microsoft_windows? do
+ # a temporary hash that contains sentinel values for each of the user-related environment variables that we
+ # are expected to unset during an "exec"
+ user_sentinel_env = {}
+ Puppet::Util::POSIX_USER_ENV_VARS.each { |var| user_sentinel_env[var] = "Abracadabra" }
+
+ command = "/bin/echo $%s"
+
+ it "should unset user-related environment vars during execution" do
+ # first we set up a temporary execution environment with sentinel values for the user-related environment vars
+ # that we care about.
+ Puppet::Util::Execution.withenv(user_sentinel_env) do
+ # with this environment, we loop over the vars in question
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ # ensure that our temporary environment is set up as we expect
+ ENV[var].should == user_sentinel_env[var]
+
+ # run an "exec" via the provider and ensure that it unsets the vars
+ output, status = provider.run(command % var)
+ output.strip.should == ""
+
+ # ensure that after the exec, our temporary env is still intact
+ ENV[var].should == user_sentinel_env[var]
+ end
+
+ end
+ end
+
+ it "should respect overrides to user-related environment vars in caller's 'environment' configuration" do
+ sentinel_value = "Abracadabra"
+ # set the "environment" property of the resource, populating it with a hash containing sentinel values for
+ # each of the user-related posix environment variables
+ provider.resource[:environment] = Puppet::Util::POSIX_USER_ENV_VARS.collect { |var| "#{var}=#{sentinel_value}"}
+
+ # loop over the posix user-related environment variables
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ # run an 'exec' to get the value of each variable
+ output, status = provider.run(command % var)
+ # ensure that it matches our expected sentinel value
+ output.strip.should == sentinel_value
+ end
+ end
+
+
+ end
+
+
end
end
diff --git a/spec/unit/type/exec_spec.rb b/spec/unit/type/exec_spec.rb
index 01bcb61..1effbb4 100755
--- a/spec/unit/type/exec_spec.rb
+++ b/spec/unit/type/exec_spec.rb
@@ -22,7 +22,13 @@ def exec_tester(command, exitstatus = 0, rest = {})
status = stub "process", :exitstatus => exitstatus
Puppet::Util::SUIDManager.expects(:run_and_capture).times(tries).
- with(command, nil, nil, :override_locale => false).returns([output, status])
+ with() { |*args|
+ args[0] == command &&
+ args[1] == nil &&
+ args[2] == nil &&
+ args[3][:override_locale] == false &&
+ args[3].has_key?(:custom_environment)
+ } .returns([output, status])
return exec
end
diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb
index 28f1de8..2953a2c 100755
--- a/spec/unit/util/suidmanager_spec.rb
+++ b/spec/unit/util/suidmanager_spec.rb
@@ -218,8 +218,15 @@
it "should capture the output and return process status" do
Puppet::Util.
expects(:execute).
- with('yay', :combine => true, :failonfail => false, :uid => user[:uid], :gid => user[:gid],
- :override_locale => true).
+ with() { |*args|
+ args[0] == 'yay' &&
+ args[1][:combine] == true &&
+ args[1][:failonfail] == false &&
+ args[1][:uid] == user[:uid] &&
+ args[1][:gid] == user[:gid] &&
+ args[1][:override_locale] == true &&
+ args[1].has_key?(:custom_environment)
+ } .
returns('output')
output = Puppet::Util::SUIDManager.run_and_capture 'yay', user[:uid], user[:gid]
diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb
index 11990d9..c5264a7 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -436,11 +436,68 @@ def process_status(exitstatus)
end
end
+ end
+
+ describe "#execute (posix user env vars)", :unless => Puppet.features.microsoft_windows? do
+ # build up a printf-style string that contains a command to get the value of an environment variable
+ # from the operating system. We can substitute into this with the names of the desired environment variables later.
+ get_env_var_cmd = 'echo $%s'
+ # a sentinel value that we can use to emulate what locale environment variables might be set to on an international
+ # system.
+ user_sentinel_value = "Abracadabra"
+ # a temporary hash that contains sentinel values for each of the locale environment variables that we override in
+ # "execute"
+ user_sentinel_env = {}
+ Puppet::Util::POSIX_USER_ENV_VARS.each { |var| user_sentinel_env[var] = user_sentinel_value }
+
+ it "should unset user-related environment vars during execution" do
+ # first we set up a temporary execution environment with sentinel values for the user-related environment vars
+ # that we care about.
+ Puppet::Util::Execution.withenv(user_sentinel_env) do
+ # with this environment, we loop over the vars in question
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ # ensure that our temporary environment is set up as we expect
+ ENV[var].should == user_sentinel_env[var]
+
+ # run an "exec" via the provider and ensure that it unsets the vars
+ Puppet::Util::execute(get_env_var_cmd % var).strip.should == ""
+
+ # ensure that after the exec, our temporary env is still intact
+ ENV[var].should == user_sentinel_env[var]
+ end
+
+ end
+ end
+
+ it "should have restored the user-related environment variables after execution" do
+ # we'll do this once without any sentinel values, to give us a little more test coverage
+ orig_env_vals = {}
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ orig_env_vals[var] = ENV[var]
+ end
+ # now we can really execute any command--doesn't matter what it is...
+ Puppet::Util::execute(get_env_var_cmd % 'anything')
+ # now we check and make sure the original environment was restored
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ ENV[var].should == orig_env_vals[var]
+ end
+ # now, once more... but with our sentinel values
+ Puppet::Util::Execution.withenv(user_sentinel_env) do
+ # now we can really execute any command--doesn't matter what it is...
+ Puppet::Util::execute(get_env_var_cmd % 'anything')
+ # now we check and make sure the original environment was restored
+ Puppet::Util::POSIX_USER_ENV_VARS.each do |var|
+ ENV[var].should == user_sentinel_env[var]
+ end
+ end
+ end
end
+
+
describe "after execution" do
let(:executor) { Puppet.features.microsoft_windows? ? 'execute_windows' : 'execute_posix' }
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
