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

MikaelSmith · 2017-05-15T21:26:58Z

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 on POSIX systems. Poorly behaved scripts need special
handling, as a plain foo & will inherit stdout and not close it.
Read from the pipe without requiring EOF.

On Windows, we retain the old file-based IO to avoid breaking poorly
written scripts - start /b ping 127.0.0.1 captures stdout - and to
retain the current behavior when Puppet is terminated - a new Puppet run
can start even if a subprocess was left running by the prior process.

MikaelSmith · 2017-05-15T21:27:20Z

Re-opened after reverting the accidental merge.

MikaelSmith · 2017-05-15T21:55:11Z

Need to look into https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_intn-van-sys_suite-ad-hoc-vmpooler-pxp-agent-ad-hoc/39/.

puppetcla · 2017-05-16T05:00:23Z

CLA signed by all contributors.

MikaelSmith · 2017-05-16T19:19:14Z

lib/puppet/util/execution.rb

+              # case where stdout is never closed.
+              ready = IO.select([reader], [], [], 0.1)
+              begin
+                output << reader.read_nonblock(4096) if ready


@joshcooper @Iristyle this can result in returning a string that is BINARY/ASCII-8BIT encoded. The read is binary, and if the result of that read includes non 7-bit ASCII characters it will not implicitly convert to the internal encoding (by assigning to output).

Previously we would read from a file, which would use whatever default encoding File.open uses.

My intuition is that we should force_encoding to external_encoding after finishing all reads, then encode to internal_encoding. Alternatively, we could use the Puppet::Util::CharacterEncoding::convert_to_utf_8 helper to convert to UTF-8. Have any suggestions?

Pushed up a PR doing this. Fixes the pxp-agent issue. It may need some error handling.

Tests at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_intn-van-sys_suite-ad-hoc-vmpooler-puppet-ad-hoc/29/

I would use the new helper, which is designed around coercing byte streams into the correct encoding.

mruzicka · 2017-05-17T17:13:48Z

lib/puppet/util/execution.rb


+      # We close stdin/stdout/stderr immediately after execution as there no longer needed.


typo: there -> they're

puppetcla · 2017-05-17T21:00:24Z

CLA signed by all contributors.

Iristyle · 2017-05-18T18:35:57Z

lib/puppet/util/execution.rb

+            end
+
+            output.force_encoding(Encoding.default_external)
+            output.encode!


Not sure what this .encode! is for...

If you want to re-encode to UTF-8, I would try and be explicit about that.

output.encode!(Encoding::UTF_8)

It converts to Encoding.default_internal. I thought that was more generic than UTF-8, but if we assert that the default internal will always be UTF-8 then using Puppet::Util::CharacterEncoding::convert_to_utf_8 definitely makes sense.

Yeah, we always want UTF-8 within our system - I don't see that changing as our encoding lingua franca.

MikaelSmith · 2017-05-18T21:15:12Z

Build available with SHA=ac16ae4b0a2905f6ef1d0d52a9ed06ca9f0c775e, SUITE_VERSION=1.8.2.1240.gac16ae4, ad-hoc pipeline's acting up so plan to manually validate some of the platforms.

MikaelSmith · 2017-05-18T23:11:50Z

Now a Solaris failure in the pxp-agent test...

Update: on Solaris, when run as a service, the default_external encoding is ASCII-8BIT. So this code still fails because what we get over the pipe is UTF-8. I'm tempted to just force encoding to UTF-8.

MikaelSmith · 2017-05-19T20:48:15Z

lib/puppet/util/execution.rb

+            # Force to external encoding to preserve prior behavior when reading a file.
+            # Wait until after reading all data so we don't encounter corruption when
+            # reading part of a multi-byte unicode character if default_external is UTF-8.
+            output.force_encoding(Encoding.default_external)


Switched to forcing the encoding to Encoding.default_external to preserve the prior behavior with File.open (see wait_for_output).

To fully mimic the File.open behavior, this should then encode to default_internal (which is normally unset, resulting in a no-op). That could often cause problems, so I'm not sure it's a behavior worth emulating.

MikaelSmith · 2017-05-19T22:26:42Z

Build available at SHA=5a32508a947d649d9bb25bd0fb245fa3a6c0216a SUITE_VERSION=1.8.2.1240.g5a32508, doing acceptance runs on redhat7-64a-windows2012r2-64a-ubuntu1604-64a-redhat5-64a-solaris11-64a now.

Passed mcollective and pxp-agent tests. I believe puppet tests will as well, but ran out of time running them all.

mruzicka · 2017-06-02T16:13:52Z

lib/puppet/util/execution.rb

+      # `start /b ping 127.0.0.1`, we couldn't handle it with pipes as there's no non-blocking
+      # read available.
+      if options[:squelch]
+        stdout = Puppet::FileSystem.open(null_file, nil, 'w')


Would it make sense to reimplement https://stackoverflow.com/questions/34504970/non-blocking-read-on-os-pipe-on-windows to get non-blocking IO on pipes even on windows and unify the implementations?

Possibly. It's currently not causing any issues to use a file on Windows.

My feeling is this area might get a rewrite for Puppet 6, and changes to Windows aren't necessary. So I'd rather schedule that as separate work.

MikaelSmith · 2017-06-03T00:04:37Z

Re-running acceptance at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_intn-van-sys_suite-daily-puppet-ad-hoc/2/.

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 on POSIX systems. Poorly behaved scripts need special handling, as a plain foo & will inherit stdout and not close it. Read from the pipe without requiring EOF. On Windows, we retain the old file-based IO to avoid breaking poorly written scripts - start /b ping 127.0.0.1 captures stdout - and to retain the current behavior when Puppet is terminated - a new Puppet run can start even if a subprocess was left running by the prior process. When reading from the pipe in blocks, we allow binary format. This is primarily to avoid issues if we read only some bytes of a multi-byte UTF-8 character. At the end, we assign the default external encoding and convert to UTF-8, our standardized internal encoding.

MikaelSmith · 2017-06-05T18:47:56Z

acceptance/tests/resource/exec/should_run_bad_command.rb

+end
+
+teardown do
+  # On Windows, Puppet waits until the sleep process exits before exiting


Apparently Windows didn't behave the way I expected. This behavior isn't changed by my PR, but this test is different on Windows to take it into account.

MikaelSmith · 2017-06-05T23:28:05Z

Passed Puppet acceptance: https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_intn-van-sys_suite-daily-puppet-ad-hoc/5/
and pxp-agent acceptance with the new test from puppetlabs/pxp-agent#583: https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/view/vmpooler/job/platform_puppet-agent_intn-van-sys_suite-daily-pxp-agent-ad-hoc/2/

mruzicka

👍

MosesMendoza · 2017-06-09T15:09:18Z

@MikaelSmith sorry I didn't intend to close this - I deleted the revert-5871-revert-5870-PUP-6675 branch from puppetlabs/puppet believing it was one of the left-over automatically created branches from a github web-based revert. Apologies - can you re-open?

MikaelSmith · 2017-06-09T18:33:23Z

Reopened as #5967.

MikaelSmith requested a review from joshcooper May 15, 2017 21:27

MikaelSmith commented May 16, 2017

View reviewed changes

MikaelSmith mentioned this pull request May 17, 2017

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

Merged

mruzicka reviewed May 17, 2017

View reviewed changes

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch from 412dd6b to fe9758e Compare May 17, 2017 17:17

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch from fe9758e to 1b4262a Compare May 17, 2017 22:38

Iristyle reviewed May 18, 2017

View reviewed changes

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch 2 times, most recently from a6fc753 to 3914cff Compare May 18, 2017 18:53

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch 3 times, most recently from 361e8a2 to a7ed054 Compare May 19, 2017 20:47

MikaelSmith commented May 19, 2017

View reviewed changes

mruzicka reviewed Jun 2, 2017

View reviewed changes

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch from a7ed054 to a6c69dd Compare June 2, 2017 22:05

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch from a6c69dd to 3a211ba Compare June 5, 2017 15:48

MikaelSmith force-pushed the revert-5871-revert-5870-PUP-6675 branch from 3a211ba to 36daa64 Compare June 5, 2017 18:46

MikaelSmith commented Jun 5, 2017

View reviewed changes

mruzicka approved these changes Jun 6, 2017

View reviewed changes

MosesMendoza closed this Jun 9, 2017

MosesMendoza deleted the revert-5871-revert-5870-PUP-6675 branch June 9, 2017 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MikaelSmith commented May 15, 2017

MikaelSmith commented May 15, 2017

MikaelSmith commented May 15, 2017

puppetcla commented May 16, 2017

MikaelSmith May 16, 2017 •

edited

Loading

MikaelSmith May 16, 2017 •

edited

Loading

Iristyle May 18, 2017

mruzicka May 17, 2017

puppetcla commented May 17, 2017

Iristyle May 18, 2017 •

edited

Loading

MikaelSmith May 18, 2017

Iristyle May 18, 2017

MikaelSmith commented May 18, 2017 •

edited

Loading

MikaelSmith commented May 18, 2017 •

edited

Loading

MikaelSmith May 19, 2017 •

edited

Loading

MikaelSmith Jun 2, 2017

MikaelSmith commented May 19, 2017 •

edited

Loading

mruzicka Jun 2, 2017

MikaelSmith Jun 2, 2017 •

edited

Loading

mruzicka Jun 6, 2017

MikaelSmith commented Jun 3, 2017

MikaelSmith Jun 5, 2017

MikaelSmith commented Jun 5, 2017 •

edited

Loading

mruzicka left a comment

MosesMendoza commented Jun 9, 2017

MikaelSmith commented Jun 9, 2017


		# We close stdin/stdout/stderr immediately after execution as there no longer needed.

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

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

Conversation

MikaelSmith commented May 15, 2017

MikaelSmith commented May 15, 2017

MikaelSmith commented May 15, 2017

puppetcla commented May 16, 2017

MikaelSmith May 16, 2017 • edited Loading

Choose a reason for hiding this comment

MikaelSmith May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puppetcla commented May 17, 2017

Iristyle May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith commented May 18, 2017 • edited Loading

MikaelSmith commented May 18, 2017 • edited Loading

MikaelSmith May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith commented May 19, 2017 • edited Loading

Choose a reason for hiding this comment

MikaelSmith Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikaelSmith commented Jun 3, 2017

Choose a reason for hiding this comment

MikaelSmith commented Jun 5, 2017 • edited Loading

mruzicka left a comment

Choose a reason for hiding this comment

MosesMendoza commented Jun 9, 2017

MikaelSmith commented Jun 9, 2017

MikaelSmith May 16, 2017 •

edited

Loading

MikaelSmith May 16, 2017 •

edited

Loading

Iristyle May 18, 2017 •

edited

Loading

MikaelSmith commented May 18, 2017 •

edited

Loading

MikaelSmith commented May 18, 2017 •

edited

Loading

MikaelSmith May 19, 2017 •

edited

Loading

MikaelSmith commented May 19, 2017 •

edited

Loading

MikaelSmith Jun 2, 2017 •

edited

Loading

MikaelSmith commented Jun 5, 2017 •

edited

Loading