Skip to content

Commit

Permalink
(PUP-6675) Use pipes instead of temporary files for Puppet exec
Browse files Browse the repository at this point in the history 
Under selinux, when Puppet is invoked by another process with reduced
privileges, any sub-programs invoked by Puppet will not inherit Puppet's
selinux priveleges. This specifically causes silent failures when
invoking applications that don't normally have the ability to write
files - such as iptables-save or hostname - because Puppet redirects
their output to a temporary file.

Use pipes instead of a temporary file to capture the output of
subprocesses.
  • Loading branch information
@MikaelSmith
MikaelSmith committed May 5, 2017
1 parent 6289eee commit 3c1cf0f
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 94 deletions.
38 changes: 4 additions & 34 deletions lib/puppet/util/execution.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ def self.execute(command, options = NoOptionsSpecified)

begin
stdin = Puppet::FileSystem.open(options[:stdinfile] || null_file, nil, 'r')
stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : Puppet::FileSystem::Uniquefile.new('puppet')
reader, writer = IO.pipe
stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : writer
stderr = options[:combine] ? stdout : Puppet::FileSystem.open(null_file, nil, 'w')

exec_args = [command, options, stdin, stdout, stderr]
Expand Down Expand Up @@ -227,7 +228,7 @@ def self.execute(command, options = NoOptionsSpecified)

# read output in if required
unless options[:squelch]
output = wait_for_output(stdout)
output = reader.read
Puppet.warning "Could not get output" unless output
end

Expand All @@ -237,7 +238,7 @@ def self.execute(command, options = NoOptionsSpecified)
ensure
if !options[:squelch] && stdout
# if we opened a temp file for stdout, we need to clean it up.
stdout.close!
reader.close
end
end

Expand Down Expand Up @@ -325,35 +326,4 @@ def self.execute_windows(command, options, stdin, stdout, stderr)
end
end
private_class_method :execute_windows


# This is private method.
# @comment see call to private_class_method after method definition
# @api private
#
def self.wait_for_output(stdout)
# Make sure the file's actually been written. This is basically a race
# condition, and is probably a horrible way to handle it, but, well, oh
# well.
# (If this method were treated as private / inaccessible from outside of this file, we shouldn't have to worry
# about a race condition because all of the places that we call this from are preceded by a call to "waitpid2",
# meaning that the processes responsible for writing the file have completed before we get here.)
2.times do |try|
if Puppet::FileSystem.exist?(stdout.path)
stdout.open
begin
return stdout.read
ensure
stdout.close
stdout.unlink
end
else
time_to_sleep = try / 2.0
Puppet.warning "Waiting for output; will sleep #{time_to_sleep} seconds"
sleep(time_to_sleep)
end
end
nil
end
private_class_method :wait_for_output
end
73 changes: 13 additions & 60 deletions spec/unit/util/execution_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,78 +216,49 @@ def stub_process_wait(exitstatus)
end

describe "when squelch is not set" do
it "should set stdout to a temporary output file" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

it "should set stdout to a pipe" do
Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,_|
stdout.path == outfile.path
stdout.class == IO
end.returns(rval)

Puppet::Util::Execution.execute('test command', :squelch => false)
end

it "should set stderr to the same file as stdout if combine is true" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == outfile.path
stdout == stderr
end.returns(rval)

Puppet::Util::Execution.execute('test command', :squelch => false, :combine => true)
end

it "should set stderr to the null device if combine is false" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == null_file
stdout.class == IO and stderr.path == null_file
end.returns(rval)

Puppet::Util::Execution.execute('test command', :squelch => false, :combine => false)
end

it "should combine stdout and stderr if combine is true" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == outfile.path
end.returns(rval)

Puppet::Util::Execution.execute('test command', :combine => true)
end

it "should default combine to true when no options are specified" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == outfile.path
stdout == stderr
end.returns(rval)

Puppet::Util::Execution.execute('test command')
end

it "should default combine to false when options are specified, but combine is not" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == null_file
stdout.class == IO and stderr.path == null_file
end.returns(rval)

Puppet::Util::Execution.execute('test command', :failonfail => false)
end

it "should default combine to false when an empty hash of options is specified" do
outfile = Puppet::FileSystem::Uniquefile.new('stdout')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)

Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == null_file
stdout.class == IO and stderr.path == null_file
end.returns(rval)

Puppet::Util::Execution.execute('test command', {})
Expand Down Expand Up @@ -550,39 +521,21 @@ def stub_process_wait(exitstatus)
end

it "should read and return the output if squelch is false" do
stdout = Puppet::FileSystem::Uniquefile.new('test')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
stdout.write("My expected command output")
r, w = IO.pipe
IO.expects(:pipe).returns([r, w])
w.write("My expected command output")

expect(Puppet::Util::Execution.execute('test command')).to eq("My expected command output")
end

it "should not read the output if squelch is true" do
stdout = Puppet::FileSystem::Uniquefile.new('test')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
stdout.write("My expected command output")
r, w = IO.pipe
IO.expects(:pipe).returns([r, w])
w.write("My expected command output")

expect(Puppet::Util::Execution.execute('test command', :squelch => true)).to eq('')
end

it "should delete the file used for output if squelch is false" do
stdout = Puppet::FileSystem::Uniquefile.new('test')
path = stdout.path
Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)

Puppet::Util::Execution.execute('test command')

expect(Puppet::FileSystem.exist?(path)).to be_falsey
end

it "should not raise an error if the file is open" do
stdout = Puppet::FileSystem::Uniquefile.new('test')
Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
file = File.new(stdout.path, 'r')

Puppet::Util::Execution.execute('test command')
end

it "should raise an error if failonfail is true and the child failed" do
stub_process_wait(1)

Expand Down

0 comments on commit 3c1cf0f

Please sign in to comment.