(PUP-6675) Use pipes instead of temporary files for Puppet exec #5844

MikaelSmith · 2017-05-04T23:55:57Z

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.

MikaelSmith · 2017-05-04T23:56:23Z

I think this works, and it solves the problem I'm trying to fix. However, it's pretty fundamental and I'm still in the process of testing it.

puppetcla · 2017-05-05T07:00:22Z

CLA signed by all contributors.

MikaelSmith · 2017-05-05T18:19:32Z

Test build available at http://builds.puppetlabs.lan/puppet-agent/f4d9c43839580f89c3313601d316ab77ef89c733.

MikaelSmith · 2017-05-05T18:31:57Z

Added some testing via pxp-agent and mcollective: puppetlabs/pxp-agent#583, https://github.com/puppetlabs/marionette-collective/pull/433

nicklewis · 2017-05-05T18:38:33Z

lib/puppet/util/execution.rb

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


I believe the child needs to close the reader and the parent needs to close the writer.

Line 226/227 should be closing the writer.

64KB test fails, need to read output in a loop to clear the buffer before waiting on the process.

MikaelSmith · 2017-05-05T20:55:21Z

I believe this is ready. Kicked off a build at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_pkg-van-ship_ad-hoc-vmpooler-ad-hoc/54/, which includes acceptance tests from this PR, the pxp-agent PR, and the mco PR.

MikaelSmith · 2017-05-05T22:15:08Z

Manually ran mco tests on RedHat 7

      Test Suite: tests @ 2017-05-05 15:07:30 -0700

      - Host Configuration Summary -


              - Test Case Summary for suite 'tests' -
       Total Suite Time: 156.45 seconds
      Average Test Time: 26.08 seconds
              Attempted: 6
                 Passed: 5
                 Failed: 0
                Errored: 0
                Skipped: 1
                Pending: 0
                  Total: 6

      - Specific Test Case Status -

Failed Tests Cases:
Errored Tests Cases:
Skipped Tests Cases:
    Test Case tests/mco_puppet_powershell.rb
Pending Tests Cases:

MikaelSmith · 2017-05-05T22:53:34Z

pxp-agent and mcollective tests failed on a few platforms due to test setup. I've fixed them, but verifying they now where can be handled after merging this fix.

MikaelSmith · 2017-05-08T19:20:53Z

pxp-agent's run_puppet_killed_puppet test seems to be a real issue caused by this change. Looking into it.

Iristyle · 2017-05-08T20:42:16Z

Wow, looks like the original Windows / Posix split happened in cb53870

Though the original Tempfile usage stems from f8c1b08

# There are problems with read blocking with badly behaved children
# read.partialread doesn't seem to capture either stdout or stderr
# We hack around this using a temporary file

MikaelSmith · 2017-05-08T20:49:14Z

The original Tempfile usage is actually 30ebbc9.

Looks like they attempted to fix it, which led to https://projects.puppetlabs.com/issues/3025.

MikaelSmith · 2017-05-08T21:02:04Z

Using pipes on Windows seems to cause problems when we test the lockfile after Puppet is killed. Process.kill(0, pid) continues to return 1 until the subprocess exits.

Iristyle · 2017-05-08T21:04:25Z

acceptance/tests/resource/exec/should_accept_large_output.rb

+Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
+occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
+EOF
+  on(agent, "for i in {1..10}; do cat #{testfile} #{testfile} > #{testfile}.2 && mv #{testfile}.2 #{testfile}; done")


Why not just generate 64k+ content on the Ruby side? It will send more over the wire, but will remove the Bash-ism above. There will be a QA undertaking shortly to prune / rewrite tests where possible. The less Bash shows up in tests, the better I think.

Ok, I can rewrite that.

Iristyle · 2017-05-08T21:24:40Z

lib/puppet/util/execution.rb

      elsif Puppet.features.posix?
        child_pid = nil
        begin
          child_pid = execute_posix(*exec_args)
+          [stdin, stdout, stderr].each {|io| io.close rescue nil}
+          unless options[:squelch]
+            while !reader.eof?


I believe read is normally performed within a Ruby Thread since it's blocking.

You can look at the capture2 source for an example - https://github.com/ruby/ruby/blob/trunk/lib/open3.rb#L305-L322

You can also take a look at the PowerShell module which uses pipes - https://github.com/puppetlabs/puppetlabs-powershell/blob/master/lib/puppet_x/puppetlabs/powershell/powershell_manager.rb#L240-L307

Since we're only using a single pipe, I think that's unnecessary.

Specifically, capture2 is using a thread for read while it also writes to stdin. We're not handling anything except reading from a single pipe, so only a single thread needed.

MikaelSmith · 2017-05-08T21:50:14Z

lib/puppet/util/execution.rb

@@ -187,18 +187,30 @@ def self.execute(command, options = NoOptionsSpecified)
    null_file = Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null'

    begin
+      reader, writer = IO.pipe unless options[:squelch]


Using a pipe on Windows causes an issue in https://github.com/puppetlabs/pxp-agent/blob/master/acceptance/tests/pxp-module-puppet/run_puppet_killed_puppet.rb.

When that test kills the Puppet process, the child process is intentionally left running. However, the mechanism Puppet uses to determine whether a previous Puppet lockfile is stale reads the Puppet run as active. This appears to be because it's terminated, but still has an open handle to the pipe with the child process (perhaps specifically that it's waiting to read from that pipe).

I'm not sure what to do about that. It's not ideal to be unable to run Puppet again if a previous run was killed but left a long-running or hung process behind, but I'm not sure when that would really come up.

Puppet uses Ruby's Process.kill to check whether the process is still running, and that states the process is running if OpenProcess succeeds. Which it appears to as long as the child process has an open handle to the shared pipe.

Full explanation of closing a process in https://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx.

I don't think this is behavior people depend on in Puppet, so I modified the pxp-agent test to not expect it in puppetlabs/pxp-agent#583.

@Iristyle have any concerns here?

MikaelSmith · 2017-05-08T23:05:05Z

Updated, test pipeline at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_pkg-van-ship_ad-hoc-vmpooler-ad-hoc/58/.

MikaelSmith · 2017-05-09T15:49:40Z

Solaris machine died during Puppet acceptance tests, but everything else passed.

joshcooper · 2017-05-09T20:30:18Z

acceptance/tests/resource/exec/should_accept_large_output.rb

+      "C:/#{@testfilename}"
+    else
+      "/tmp/#{@testfilename}"
+    end


I think you can use host.tmpfile which IIRC will return a platform-specific path that you can use later in the test. Might also add a teardown for deleting the file.

joshcooper · 2017-05-09T20:33:51Z

spec/unit/util/execution_spec.rb

-        Puppet::Util::Execution.execute('test command')
-
-        expect(Puppet::FileSystem.exist?(path)).to be_falsey
-      end


should we add a test to make sure both ends of the pipe are closed (during normal run and if an exception is raised)?

I'll try, that seems useful.

joshcooper · 2017-05-09T20:37:56Z

lib/puppet/util/execution.rb


      if execution_stub = Puppet::Util::ExecutionStub.current_value
-        return execution_stub.call(*exec_args)
+        child_pid = execution_stub.call(*exec_args)
+        [stdin, stdout, stderr].each {|io| io.close rescue nil}


Puppetserver installs an execution stub, e.g. when executing an autosigning policy. Will it work correctly if we close these descriptors? /cc @camlow325

We should only be closing them on the Puppet side, the forked process inherits the handles. I'm actually not sure we need to close them earlier than we did before, since nothing else about forking changed.

We already do close them in Puppet Server, although it seems like it would have better if we didn't have to. See this code.

If we add this here, we might get exceptions when the second set of close calls are made. That should be okay with the swallowing rescue, though, right? Not sure if anything would be written to the log when this happens?

rescue nil should handle that in both cases, and nothing should be logged. The execution doesn't change at all in this case, after execution_stub.call we still would have closed all the handles at old line 226.

joshcooper · 2017-05-09T20:45:32Z

lib/puppet/util/execution.rb

-      unless options[:squelch]
-        output = wait_for_output(stdout)
-        Puppet.warning "Could not get output" unless output
+      unless options[:squelch] || read_succeeded


Is read_succeeded necessary? If options[:squelch] is false, then we always call (for posix and windows):

while !reader.eof? output << reader.read end read_succeeded = true

which always sets read_succeeded to true or will raise. But if it's the latter, then we never reach this line.

There doesn't seem to be a rescue, so I agree I don't see any circumstance where we'd reach this line but reading had failed.

This might make more sense outside the last ensure block. Update: nevermind, still no rescue that would cause it to trigger.

joshcooper · 2017-05-09T20:49:38Z

lib/puppet/util/execution.rb

-        stdout.close!
+      if !options[:squelch] && reader
+        # if we opened a pipe, we need to clean it up.
+        reader.close


If an exception is raised between the time we open the pipe and when the execute_{posix,windows} call returns, then the writer is never closed.

I think you're misreading what block this ensure relates to. It starts at line 189.

If an exception is raised between lines 190 and 201 (posix) or 207 (windows), then I don't think we close the writer:

begin reader, writer = IO.pipe unless options[:squelch] ... process_info = execute_windows(*exec_args) # if this raises begin [stdin, stdout, stderr].each {|io| io.close rescue nil} # this line is never reached ... end ensure if !options[:squelch] && reader reader.close end end

A good test is to try to execute a non-existing program with squelch = false. On Windows, I know execute_windows will raise, and I think the posix impl behaves the same.

That does seem like a loose end. However, as far as I can tell if we never fork and then close one end, the other end also closes. So in that scenario the pipe would still be closed.

That still leaves dangling handles for the files. We could attempt to close all handles in the ensure block anyway.

MikaelSmith · 2017-05-09T21:56:23Z

Updated and kicked off a new build: https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_pkg-van-ship_ad-hoc-vmpooler-ad-hoc/60/

Iristyle · 2017-05-09T23:26:18Z

Do we want this on stable or master @MikaelSmith ?

MikaelSmith · 2017-05-09T23:27:59Z

I was hoping stable.

joshcooper · 2017-05-10T20:55:37Z

@MikaelSmith In talking this over with the team, it feels too risky of a change to make in a .z without some assurances it won't cause regressions on other platforms. Can we target this for master, and if there are no downstream issues, backport to stable?

MikaelSmith · 2017-05-10T21:11:50Z

Sure. I'll rebase and retarget.

This test passes lots of data over pipes when performing an exec, to attempt to catch any issues with buffered reads over pipes. On *nix the standand buffer size is 64KB.

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.

MikaelSmith · 2017-05-10T21:15:03Z

Done. I'll retarget the pxp-agent and mco acceptance tests to master as well.

MikaelSmith · 2017-05-10T22:41:56Z

Not sure why it pulled in extra commits for commit checking, but everything else was passing.

Magisus · 2017-05-10T22:45:19Z

Yeah looks like it didn't update the job parameter for some reason... weird.

joshcooper · 2017-05-10T22:45:20Z

I think the failure is because we're using double-dots in the commit check, so it's pulling in commits on the base branch that were made after you forked this branch, and it looks like the "Revert" commits are causing issues:

Checking commits 054989b458a3a48188551bf8efbf855cb767a201..6519360061e2a005b0b5913f2789ea15fe792842

I thought we had updated travis and appveyor to use triple dots?

Magisus · 2017-05-10T22:50:28Z

Weirdly, it kind of looks like we're doing the opposite: https://github.com/puppetlabs/puppet/blob/master/Rakefile#L86. However, I think in this case it was just Github's PR re-targeting being kind of screwy with commit hooks. We've seen stuff like that a lot.

joshcooper

Waiting on appveyor

Iristyle · 2017-05-14T19:08:05Z

Reverted in #5868

MikaelSmith requested review from Iristyle and joshcooper May 4, 2017 23:56

MikaelSmith force-pushed the PUP-6675 branch from ae9ff27 to 3c1cf0f Compare May 5, 2017 00:19

MikaelSmith added the blocked PRs blocked on work external to the PR itself label May 5, 2017

MikaelSmith changed the title ~~WIP (PUP-6675) Use pipes instead of temporary files for Puppet exec~~ (PUP-6675) Use pipes instead of temporary files for Puppet exec May 5, 2017

MikaelSmith force-pushed the PUP-6675 branch from 3db215b to 829c055 Compare May 5, 2017 18:21

nicklewis reviewed May 5, 2017

View reviewed changes

MikaelSmith force-pushed the PUP-6675 branch 4 times, most recently from fd242cb to cec28d7 Compare May 5, 2017 20:22

This was referenced May 5, 2017

(maint) Add testing around exec via Puppet puppetlabs/pxp-agent#583

Merged

(maint) Add testing around exec via Puppet choria-legacy/marionette-collective#433

Merged

MikaelSmith removed the blocked PRs blocked on work external to the PR itself label May 8, 2017

MikaelSmith force-pushed the PUP-6675 branch from cec28d7 to 0fef750 Compare May 8, 2017 19:39

Iristyle reviewed May 8, 2017

View reviewed changes

MikaelSmith commented May 8, 2017

View reviewed changes

MikaelSmith force-pushed the PUP-6675 branch from 0fef750 to 84d758d Compare May 8, 2017 23:01

joshcooper reviewed May 9, 2017

View reviewed changes

MikaelSmith force-pushed the PUP-6675 branch from 84d758d to f06fabc Compare May 9, 2017 21:55

MikaelSmith added 2 commits May 10, 2017 14:12

(PUP-6675) Add exec test using large output

8cec657

This test passes lots of data over pipes when performing an exec, to attempt to catch any issues with buffered reads over pipes. On *nix the standand buffer size is 64KB.

MikaelSmith force-pushed the PUP-6675 branch from f06fabc to 6519360 Compare May 10, 2017 21:14

MikaelSmith changed the base branch from stable to master May 10, 2017 21:14

MikaelSmith closed this May 10, 2017

MikaelSmith reopened this May 10, 2017

joshcooper approved these changes May 10, 2017

View reviewed changes

joshcooper merged commit 4b10d53 into puppetlabs:master May 10, 2017

MikaelSmith deleted the PUP-6675 branch May 10, 2017 23:32

MikaelSmith mentioned this pull request May 12, 2017

Revert "(PUP-6675) Use pipes instead of temporary files for Puppet exec" #5868

Merged

(PUP-6675) Use pipes instead of temporary files for Puppet exec #5844

(PUP-6675) Use pipes instead of temporary files for Puppet exec #5844

Conversation

MikaelSmith commented May 4, 2017

MikaelSmith commented May 4, 2017

puppetcla commented May 5, 2017

MikaelSmith commented May 5, 2017

MikaelSmith commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith commented May 5, 2017

MikaelSmith commented May 5, 2017

MikaelSmith commented May 5, 2017

MikaelSmith commented May 8, 2017

Iristyle commented May 8, 2017 • edited Loading

MikaelSmith commented May 8, 2017

MikaelSmith commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith commented May 8, 2017 • edited Loading

MikaelSmith commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith May 9, 2017 • edited Loading

Choose a reason for hiding this comment

MikaelSmith commented May 9, 2017 • edited Loading

Iristyle commented May 9, 2017

MikaelSmith commented May 9, 2017

joshcooper commented May 10, 2017

MikaelSmith commented May 10, 2017

MikaelSmith commented May 10, 2017

MikaelSmith commented May 10, 2017

Magisus commented May 10, 2017

joshcooper commented May 10, 2017

Magisus commented May 10, 2017 • edited Loading

joshcooper left a comment

Choose a reason for hiding this comment

Iristyle commented May 14, 2017

Iristyle commented May 8, 2017 •

edited

Loading

MikaelSmith commented May 8, 2017 •

edited

Loading

MikaelSmith May 8, 2017 •

edited

Loading

MikaelSmith commented May 8, 2017 •

edited

Loading

MikaelSmith May 9, 2017 •

edited

Loading

MikaelSmith May 9, 2017 •

edited

Loading

MikaelSmith commented May 9, 2017 •

edited

Loading

Magisus commented May 10, 2017 •

edited

Loading