move temp sudo put files to /tmp #694

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@akvadrako

When creating a temporary file on the remote host, it should go into a temporary directory. I ran into this problem because I didn't have write access to whatever directory it was trying to access.

@bitprophet
Member

Unfortunately this isn't cross-platform-friendly; I think we used to use /tmp and had to remove it in favor of using a randomly named file in $CWD as that generally worked better, and users typically had write permissions to that directory (which, absent any cd() is typically the login user's homedir, and with cd() is typically the intended target.)

I'm curious how you ended up not having write access to the remote CWD, as per above usually that's not a problem.

@bitprophet
Member

Actually, I just ran into a real-world scenario where having an absolute path would be helpful -- a user has an experimental setup where they modified sudo_prefix to include -i, which means sudo() calls have a different implicit CWD than that of the login user.

Thus, their put(xxx, yyy, use_sudo=True) calls fail because the sftp.put drops it into the connecting user's homedir, but the sudo mv call done internally is running in the context of eg root's homedir -- so the temp file isn't available.


Unfortunately this doesn't change the fact that hardcoding /tmp is not portable -- we'd need to come up with something else, e.g. ~<loginuser>, though that's still not very portable as it assumes ~ works (we've had issues with that in the past IIRC.)

Basically, put(use_sudo=True) is a horrible idea with a million possible failure scenarios, and I kind of wish we had never bothered adding it :D

@Dieterbe
Dieterbe commented Mar 4, 2013

other scenario: home dir on an nfs mount. root (sudo) has no access to it.
how about just providing an option like temp_dir=<foo> then create a random file in there using something like mktemp.

@jessemyers

I think I might be echoing Dieterbe: the "sudo mv" operation can fail if the user's home directory is NFS mounted and root is squashed. That is: put() creates the temporary file in the user's home directory without any problem, but, by policy, the "sudo mv" operation is rejected because the system administrator does not want users running sudo to have access to any other user's home directory.

An optional temp_dir argument would suffice; we know we're running on LINUX/UNIX and have a configuration system that would let us customize the value if necessary.

@bitprophet
Member

Yea, making this configurable sounds like the best way out for now. Will poke.

@bitprophet bitprophet added a commit that referenced this pull request May 27, 2013
@bitprophet bitprophet Sanity + failing tests re #694 3558209
@bitprophet bitprophet added a commit that referenced this pull request May 27, 2013
@bitprophet bitprophet Implement #694 e7e9649
@bitprophet
Member

Have this (put(..., temp_dir=xxx)) working for my handful of integration tests now. Just need to update changelog and I'll merge it. Not sure exactly when it will release (trying to blast thru a lot of tickets this weekend) but if @jessemyers @Dieterbe or @akvadrako get this before 1.7.0 comes out, please do test & provide feedback. Thanks!

@bitprophet bitprophet added a commit that referenced this pull request May 27, 2013
@bitprophet bitprophet Changelog re #694; fixes #694 ab98e00
@bitprophet bitprophet added a commit that closed this pull request May 27, 2013
@bitprophet bitprophet Changelog re #694; fixes #694 ab98e00
@bitprophet
Member

Merged this into master - again, feedback appreciated.

@jessemyers

I verified that this works around the NFS squash root problem. Thanks.

@nburlett nburlett added a commit to nburlett/fabric that referenced this pull request Jul 19, 2013
@nburlett nburlett Implement #694
Merge back e7e9649

Conflicts:

	fabric/operations.py

Conflict resolved by removing "use_glob=True,", which depended on
an earlier change.
63f3fc9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment