Make `put` sudo-able #2

Closed
bitprophet opened this Issue Aug 19, 2011 · 9 comments

Projects

None yet

1 participant

@bitprophet
Fabric member

Description

Right now, put can only write remote files as the logging-in user, which is problematic when one needs to write to locations owned by root or another user.

Paramiko's SFTP API doesn't support any sort of sudo functionality (though I rather doubt the SFTP protocol in general supports this anyways) so this will need to be implemented as a call to the current put behavior, plus a sudo('mv').


Originally submitted by Jeff Forcier (bitprophet) on 2009-07-20 at 04:56pm EDT

Relations

  • Related to #140: Recursive put() and get()
  • Related to #257: Add a convenience chown-ing kwarg to put()?

Closed as Done on 2010-11-14 at 09:02pm EST

@bitprophet bitprophet was assigned Aug 19, 2011
@bitprophet
Fabric member

Erich Heine (sophacles) posted:


This is trivial to build on top of the stuff I did for #140, and I am wiling to do so if that other stuff is accepted.


on 2010-03-31 at 01:19pm EDT

@bitprophet
Fabric member

Jeff Forcier (bitprophet) posted:


npmap has an implementation of this here.

Curious whether it'd be nice to do a bit of refactoring (Esp. with the #140 stuff in) so that it is still possible to access the "simple put" functionality in a standalone function. This would simplify the implementation of put itself. Will have to see -- not crucial.


on 2010-08-19 at 11:14am EDT

@bitprophet
Fabric member

Erich Heine (sophacles) posted:


The simple put is sort of already built into #140 -- depending on how simple you mean, operations.put/get cover most file transfers, and only file transfers (but w/ globs etc). sftp.put,get,put_dir,get_dir cover non-globbed simple put and get...

As for the ticket proper-- I was working on a put_sudo function and came across a semantic issue with the idea of "sudoable put". Basically this label could apply as:

  1. put files to places where escalated privilege is required. This method preserves the owner/group attributes of the original submitter. This is much like npmap's patch:

    put files /tmp/uniq/
    sudo mv /tmp/uniq/ orig_target
    
  2. put the files in places as an escalated privilege user. This would be the equivalent of using a host_string of root@remote, and is useful for systems that disable remote login for root or web users:

    put files /tmp/uniq/
    sudo chown -R root:root /tmp/uniq/
    mv /tmp/uniq/ original_target
    
  3. (unlikely) put the files as a locally escalated user. Basically, need to be root to access the relevant local file to put it...

Even assuming only 1 and 2 are every desired by users, we have to account for both, which starts to get pretty tricky in terms of number of options/kwargs to track. Further, it begins to require certain utilities and semantics on the remote system, things which are out of the scope of "only needs an sshd" fabric ideal.

A final thought: the tempfile stuff from npmap's code is pretty nice, perhaps a put_temporary building block could be made, then the sudo/chown stuff could be put into contrib modules, covering many types of semantics and various remote oses.


on 2010-08-25 at 12:29am EDT

@bitprophet
Fabric member

Erich Heine (sophacles) posted:


oops, pushed submit x2 :(


on 2010-08-25 at 12:29am EDT

@bitprophet
Fabric member

Jeff Forcier (bitprophet) posted:


Starting to try and read all the material related to put/get now in prep for, you know, doing something, and replying to Erich above:

  • I don't get your number 2 above :(
    • For one thing, it seems to contradict itself, "a host string of root@remote" + "systems that disable remote login for root users". Typo? or...?
    • For another, I don't get how the put/sudo chown/mv thing would work -- surely you would need to have root access to mv a root-owned file? And I don't get how it's substantially different from option 1, so maybe I'm missing something.

Re: npmap's code, just noting in here for readers that its benefit over the naive implementation, is that it provides filename collision protection -- i.e. it helps in the scenario that you're uploading a filename that may already exist in the remote temp directory.

I believe the linked implementation is also specifically designed to avoid remote use of tools like tmpdir which aren't necessarily portable/always there.


on 2010-09-10 at 08:40pm EDT

@bitprophet
Fabric member

Erich Heine (sophacles) posted:


Clarifications to my point 2:

-- I did mean it should be put/sudo chown/sudo mv.

-- doing the steps in point 2 is effectively the same as putting to a host as root, however some systems disable a root login, so these steps are required instead.

The differences between method 1 and method 2 are strictly about the semantics of what does a "sudo put" mean. Should the end owner of a sudo put be the root user or the login user. There are arguments for both cases, but it should be explicitly defined.


on 2010-09-17 at 02:21pm EDT

@bitprophet
Fabric member

Jeff Forcier (bitprophet) posted:


OK, I understand now. I don't think it's a reasonable default interpretation of "sudo put" that the file should end up owned by root instead of the user you're using to login. If somebody has a strict requirement that their file needs to be root-owned, and is not logging in as root, they will know to follow up their put call with a sudo(chown).

That said, this does hook into something else that I thought there'd been a ticket or discussion for but don't see now: the overall chown feature (i.e. "I want to upload a file while connecting as jforcier but the file should end up owned by eheine"). Having this in might be reasonable given we're already packaging a best-practice one-two punch, into the API.

In such a case, your number-2 interpretation would just turn into a user calling put(src, dst, use_sudo=True, user='root') (or put(src, dst, user='root') if one assumes the as kwarg automatically implies use_sudo). (N.B. I think as= would be a nicer kwarg, but either way it should be the same name as for sudo itself, which is currently user=)


on 2010-09-17 at 02:51pm EDT

@bitprophet
Fabric member

Jeff Forcier (bitprophet) posted:


Applied in changeset commit:44e131d00da227ac6fd24943a05ce6bee70da8b6.


on 2010-11-14 at 08:38pm EST

@bitprophet
Fabric member

Jeff Forcier (bitprophet) posted:


Implemented this pretty much as outlined in npmap's patch, but adapted heavily for the layout of code post-#140.

I'm making a new ticket for the "auto-chown" feature mentioned in the last few comments as while it is related, it's not a requirement of this chunk of code and shouldn't be worked on in this ticket.


on 2010-11-14 at 09:02pm EST

@bitprophet bitprophet closed this Aug 19, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment