[PUP-5915] exec resources fail unless cwd is readable Created: 2016/02/17  Updated: 2019/11/18  Resolved: 2019/10/21

Status: Resolved
Project: Puppet
Component/s: None
Affects Version/s: PUP 4.3.2
Fix Version/s: PUP 5.5.18, PUP 6.4.5, PUP 6.11.0

Type: Bug Priority: Normal
Reporter: James Ralston Assignee: Josh Cooper
Resolution: Fixed Votes: 4
Labels: resolved-issue-added
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
is duplicated by PUP-10080 Exec resources fail if the working di... Closed
Relates
relates to PUP-9997 Puppet must not call Dir.chdir() exce... Accepted
Template: PUP Bug Template
Epic Link: Execution API
Team: Coremunity
Sprint: Platform Core KANBAN
Release Notes: Bug Fix
Release Notes Summary: Previously if the `cwd` parameter was not specified, puppet would change its working directory to the current working directory, which was redundant and could fail if the current working directory was not accessible. Now wxec resources only change the current working directory if the `cwd` parameter is specified in a manifest.

 Description   

The exec() resource always attempts to perform a chdir() to the current working directory (cwd). This is true regardless of the value of the cwd parameter.

If the cwd isn't accessible by the user running Puppet, Puppet will fail to apply the exec() resource. This is true regardless of whether the command being executed actually requires the cwd to be readable.

An example:

$ cd /tmp
$ mkdir test
$ cd test
$ chmod 0000 .
 
$ puppet apply -e 'exec { "/bin/pwd": logoutput => true }'
Notice: Compiled catalog for myhost.example.org in environment production in 0.05 seconds
Error: Permission denied @ dir_chdir - /tmp/test
Error: /Stage[main]/Main/Exec[/bin/pwd]/returns: change from notrun to 0 failed: Permission denied @ dir_chdir - /tmp/test
Notice: Applied catalog in 0.04 seconds
 
$ puppet apply -e 'exec { "/bin/pwd": logoutput => true, cwd => "/" }'
Notice: Compiled catalog for myhost.example.org in environment production in 0.06 seconds
Error: Permission denied @ dir_chdir - /tmp/test
Error: /Stage[main]/Main/Exec[/bin/pwd]/returns: change from notrun to 0 failed: Permission denied @ dir_chdir - /tmp/test
Notice: Applied catalog in 0.05 seconds

If the cwd is accessible, the exec will succeed:

$ chmod 0755 .
 
$ puppet apply -e 'exec { "/bin/pwd": logoutput => true }'
Notice: Compiled catalog for myhost.example.org in environment production in 0.05 seconds
Notice: /Stage[main]/Main/Exec[/bin/pwd]/returns: /tmp/test
Notice: /Stage[main]/Main/Exec[/bin/pwd]/returns: executed successfully
Notice: Applied catalog in 0.05 seconds

You may be tempted to respond: "Why does this matter? It's almost never useful to run Puppet as a non-root user, and the root user has implicit access to all files and directories."

The problem is that this is not true.

At our site, we are deploying NFSv4 user home directories, where home directories are mounted from an NFSv4 server with sec=krb5p (that is, with Kerberos security). In such a scenario, the root user always and implicit possesses host/hostname@DOMAIN credentials, and nothing more.

User home directories are private (mode 0700) by default. Effectively, this means that the root user is unable to chdir() to a user's home directory, or any subdirectory thereof.

As we began to deploy NFSv4 user home directories, developers who run sudo puppet agent --test from the command line began reporting Puppet failures. Investigating the problems let us to discover the exec resource's behavior of always attempting to chdir() to the cwd, regardless of what the cwd parameter is set to (or whether it's defined at all).

Our developers get hit by this because they are typically sitting in their (NFSv4-based) home directory when they attempt to run sudo puppet agent --test.

I would argue that the exec resource should behave this way:

  1. If the cwd parameter is defined, the exec resource attempts to chdir() to that directory, and only that directory.
  2. If the cwd parameter is undefined, the exec resource should not call chdir() at all.

The current behavior of the exec resource is at best extremely non-intuitive and at worst outright incorrect, even if it only causes problems when specific (and uncommon) conditions are met (i.e., root not having implicit access to the cwd).

Thoughts?



 Comments   
Comment by James Ralston [ 2016/02/17 ]

Also, unless I'm missing something, the fix is pretty simple. Here's a quick diff (ignoring whitespace changes, so that it's easier to read):

--- /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/exec.rb.ORIG   2016-02-02 14:10:21.000000000 -0500
+++ /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/provider/exec.rb 2016-02-17 14:05:12.750781461 -0500
@@ -12,7 +12,9 @@
     checkexe(command)
 
     if dir = resource[:cwd]
-      unless File.directory?(dir)
+      if File.directory?(dir)
+        Dir.chdir(dir)
+      else
         if check
           dir = nil
         else
@@ -21,12 +23,8 @@
       end
     end
 
-    dir ||= Dir.pwd
-
     debug "Executing#{check ? " check": ""} '#{command}'"
     begin
-      # Do our chdir
-      Dir.chdir(dir) do
         environment = {}
 
         environment[:PATH] = resource[:path].join(File::PATH_SEPARATOR) if resource[:path]
@@ -66,7 +64,6 @@
           raise ArgumentError, output
         end
 
-      end
     rescue Errno::ENOENT => detail
       self.fail Puppet::Error, detail.to_s, detail
     end

I've tested this locally, and it seems to work.

This does move the chdir() call outside of the begin/rescue/end block, though. Does the chdir() call by itself warrant its own begin/rescue/end block?

Anyway, if this looks good, I'll be happy to submit a merge request.

Comment by Henrik Lindberg [ 2017/05/15 ]

James Ralston did you get around to making a PR?

Comment by Doug Penner [ 2018/01/29 ]

I can confirm that the problem also exists in puppet 5.3.3.

If we are in our home folders on a system with root-squash, we get the same error as James whether we set a cwd or not.

Comment by James Ralston [ 2018/03/02 ]

My first attempt at solving the problem was incorrect, which is why I never submitted a PR. But this is still biting us, so I'll take another crack at it.

Comment by Adam Winberg [ 2018/08/14 ]

This is in effect the same as PUP-5808.

Comment by James Ralston [ 2019/09/04 ]

The issue with the exec resource failing due to receiving EACCES when attempting to call chdir() to the cwd was resolved PUP-6919 (specifically, commit ebb57c3e), which rewrote the exec resource to have the child process call Dir.chdir().

But the root cause of this issue—that it is never safe to call chdir() except in a child process—is not completely resolved. See PUP-9997 for details.

(Puppet 5.5.7 was the first version to contain the fix.)

Comment by James Ralston [ 2019/10/08 ]

I was incorrect. This is not actually fixed.

Moving the chdir() call in Puppet::Util::Execution.execute() into the child solves the case where Puppet is running in a directory that is inaccessible, but applies an exec resource that specifies a different directory for the cwd parameter. Before, Puppet would chdir() in the parent process, and would therefore fail to chdir() back to the old cwd after applying the exec resource.

But the exec resource itself (still) implicitly defaults the cwd parameter to the cwd if the cwd parameter was not provided for the exec resource in question. And in the case where Puppet's cwd is inaccessible, this will cause all Puppet exec() resources to fail.

So: I assert that defaulting the cwd parameter to the cwd of the Puppet process is wrong. If the user does not supply a cwd parameter to the exec resource in question, Puppet must assume that the user does not want Puppet to attempt to chdir() to any directory. Assuming that the user wants Puppet to chdir() to the cwd is an erroneous assumption, as 1) this is not what the user specified, and 2) it's nonsensical, as even if the chdir() call succeeds, Puppet has not actually changed the cwd.

To fully fix this bug, I agree with John Bollinger in PUP-10080: the "implicitly default the cwd parameter to Puppet's cwd if it was not specified" behavior is wrong and should be removed.

I think the only thing required is removing this line from lib/puppet/provider/exec.rb:

      cwd ||= Dir.pwd

This is because, from my read, Puppet::Util::Execution.execute() will do the correct thing if the cwd parameter is unspecified:

    cwd = options[:cwd]
    if cwd && ! Puppet::FileSystem.directory?(cwd)
      raise ArgumentError, _("Working directory %{cwd} does not exist!") % { cwd: cwd }
    end

And:

        cwd = options[:cwd]
        Dir.chdir(cwd) if cwd

Comment by Josh Cooper [ 2019/10/15 ]

Merged to 5.5.x in https://github.com/puppetlabs/puppet/commit/a63ae7bb5517ec8699d7ec5d77280961257f399d

Comment by Josh Cooper [ 2019/10/21 ]

Passed CI in dc81e4e117

Generated at Fri Dec 13 09:24:27 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.