Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sudo() doesn't play nice with shell syntax (&&, ;, etc) such as what cd() does #459

Open
jrmi opened this issue Jul 25, 2017 · 15 comments
Open

Comments

@jrmi
Copy link

jrmi commented Jul 25, 2017

As cd is a shell builtin, it can't be executed as a sudo command. When I do that:

@task
def test(ctx):
    """ test command """
    with ctx.cd('my/path'):
        ctx.run("pwd") # Show correct path
        ctx.sudo("whoami") # fails with: sudo: cd: command not found

The last command fail.

After a quick look in the source, sudo command appears to be:

cmd_str = "sudo -S -p '{0}' {1}{2}".format(prompt, user_flags, command)

with the cd prefix the executed command is

$ sudo -S -p cd my/path && whoami

where cd is unknow inside sudo.

To fix the issue, new sudo command could be:

cmd_str = "sudo -S -p '{0}' {1} sh -c \"{2}\"".format(prompt, user_flags, command)

but not sure of all the consequences of this behaviour.

@bitprophet bitprophet changed the title ctx.cd and ctx.sudo doesn't works together Context.sudo() doesn't play nice with nontrivial shell syntax Jul 25, 2017
@bitprophet
Copy link
Member

bitprophet commented Jul 25, 2017

Thought there was an issue for this already, but not seeing it. Thanks for the report!

The primary issue here is that by default, commands are transparently sent to a real shell for execution (without a PTY, via execve explicitly calling the shell with -c; with a PTY, implicitly requesting that same shell via Popen(shell=True, executable=<shell>, ...)), which is why the && ends up 'splitting' the command in two with sudo only getting what comes beforehand.

As you've noted, the first obvious solution (listed in man sudo, even) is to use a subshell so that sudo is still given a single "command" to run. Unfortunately that has a raft of baggage which has been explored by its use in the predecessor package, Fabric 1.x. For example, the need to escape command so it doesn't accidentally break out of its enclosing quotes is fraught with peril.

I haven't sat down and given the alternatives a hard think yet, so suggestions are welcome. (Note that wanting to explore alternatives doesn't mean a hard ruling-out of the subshell option, just that if we go there I want to be sure it's the least crummy path to take.)


The only one that immediately springs to mind is the old chestnut of "drop command into a temporary shell script and run that". Avoids any and all escaping problems, but has its own:

  • Incurs disk IO.
    • Presumably only a problem if one is running very many very fast commands with a very slow disk, which seems rather unlikely. 99.9% of the time any individual command one deigns to run will have a runtime dwarfing the time it takes to write a trivial file to disk.
  • Potential for leaving droppings on the filesystem.
    • Should be easily solved by using context managers; if one's Python process is crashing hard enough to leave the files around anyway, you have bigger problems...
    • Should be trivial to use tempfile methods so the files are out of sight, out of mind either way.
  • Debugging becomes less obvious since there's now an intermediate log line & process table entry as well as the final one.
  • Becomes less trivial in client libraries like Fabric 2 where the execution context is not local, but remote (e.g. over SSH).
    • Disk IO on a remote target may be more contentious than on the invoking system (meh.)
    • File transfer is not always allowed on all targets or via all mediums (e.g. SFTP disabled.)
    • File cleanup requires an additional, final remote command to execute cleanly and successfully, which is more vulnerable to being disrupted than a local contextmanager+unlink.
      • Especially if for whatever reason, the final removal is itself inadvertently leveraging the same "run as a script" functionality (ideally preventable, but.)

Due to all these problems, it'd be ideal if this was purely opt-in and not the default method of using sudo - but implementing that can itself be problematic:

  • Do we force users to explicitly request this behavior? That always results in users forgetting to enable it until they've failed at least once, often with an obtuse error ("oh right, random error due to 2nd command running w/o sudo means I have to add via_script=true...sigh").
  • Do we attempt to automatically detect when it's necessary, e.g. searching for any shell constructs within the command string? That's fragile and will have a lot of not-fun edge cases (e.g. it will attempt to script-wrap if the user is constructing their own subshell!)

@jrmi
Copy link
Author

jrmi commented Jul 26, 2017

I knew it was not so easy....

To avoid file creation and associated issues which your solution, can we send command via stdin to avoid escaping problems ?

Like this example:

import subprocess

cmd = "cd /home && pwd && whoami"
shell_cmd = ['sudo', '-i']

p = subprocess.Popen(shell_cmd, stdin=subprocess.PIPE)
p.communicate(cmd)

shell_cmd = ['/bin/sh']

p = subprocess.Popen(shell_cmd, stdin=subprocess.PIPE)
p.communicate(cmd)

May also help for other use cases like sending ssh commands ?

By reading the code I see some potential issues:

  • can we do the same with execve ?
  • sdin may already be used anywhere ?
  • the patch isn't too big ?

@bitprophet
Copy link
Member

bitprophet commented Jul 26, 2017

That's actually a great idea, I'll have to take a poke and see how feasible it is. Off the top of my head, though, our use of sudo -s + submission of the password via stdin, may clash with that approach, but I suspect not. (The latter is done via a 'watcher' that man-in-the-middle's the streams, it doesn't actually override normal stdin.)

Certainly it appears to work by hand, I guess since sudo is just hooking its pipes up direct to the shell (for some reason, probably because I just woke up, I was trying to figure out if sudo itself could accept the command via stdin...no, no):

» sudo -s whoami && whoami                                                                                                   
Password:
root
jforcier
» sudo -K
» echo "whoami && whoami" | sudo -s bash
Password:
root
root

(We'd use the same shell setting as the rest of the command running uses, here; I just explicitly chose bash since that is what sh maps to on my system.)

@bitprophet bitprophet added this to the 0.20 milestone Jul 26, 2017
@bitprophet
Copy link
Member

bitprophet commented Apr 25, 2018

Another later idea as I run across this during the run up to Fabric 2...can we trivially solve this by using the list form of argv in our exec calls? (aka #2) I never really use that mode of subprocess munging but IIRC it often incidentally gives you greater control over tokenization, so if the average sudo implementation doesn't try to re-tokenize what we give it, could potentially work.

Of course, this assumes sudo is handing it off to a shell internally or something, otherwise we're back to square one. But it's worth looking into before trying the file redirection or stdin ideas above.

Also, if we go that route, it may need to be in tandem with sudo -i given its ties to an internal login shell invocation. (I actually note that jrmi's comment above does this as well.)

@bitprophet bitprophet changed the title Context.sudo() doesn't play nice with nontrivial shell syntax sudo() doesn't play nice with nontrivial shell syntax Apr 25, 2018
@mogoh
Copy link

mogoh commented Jun 15, 2018

Can someone suggest a workaround for now? I have something like:

with context.cd('/somewhere'):
    context.sudo('git pull', user='mygituser')

Edit: I am now doing this but it is not that elegant:

    context.sudo('bash -c cd "/somewhere && git pull"', user='mygituser')

@Motor-taxi-master-rider
Copy link

Can someone suggest a workaround for now? I have something like:

with context.cd('/somewhere'):
    context.sudo('git pull', user='mygituser')

Edit: I am now doing this but it is not that elegant:

    context.sudo('bash -c cd "/somewhere && git pull"', user='mygituser')

I've tried your workaround and find it works if I modify your example like this:

    context.sudo('bash -c "cd /somewhere && git pull"')

Just move the double quotation marks in front of the cd .

Anyway, thanks a lot for your workaround, it do solve my problem. Hope this bug could be fix in the near future.

@maparent
Copy link

I just ran in this issue. I have a suggestion that can serve as a temporary workaround.

Instead of command_prefixes, have command_wrappers. Each wrapper would have a %s where the next command gets inserted. (If no %s is present, default to prefixing to ease transition.)
Then, it becomes easier to wrap a command inside a 'bash -c "%s"' wrapper, (at your own quoting risks) if so desired.

Another point: Many of my sudo commands would be simpler if I could add the -i flag... Can this be added to the sudo.* environment?

@Satchitananda
Copy link

Have exactly the same problem since fabric1, today have ported my code from fabric 1 to fabric 2 and found out that is still not fixed

@hardfalcon
Copy link

A hint: At least on most real-life Linux systems, when wrapping the commands into a temporary shell script, you can avoid the disk I/O overhead if you write the shell script to /dev/shm if that directory exists. On systems where /dev/shm does not exist, you can still fall back to /tmp.

@powderflask
Copy link

Same issue -- converting a fabric1 script to fabric2, all good so far except this issue.
In my case, I need to:

with c.prefix('source path/to/bin/activate'):
    c.sudo('pip install xyz')

Since source is also built-in, I have the same issue. My use case actually sources the activate script in a contextmanager, so a little more indirection, can't use suggested workarounds. This all worked in fabric1, now at least I understand why it broke.
Thanks for the great package and detailed explanation of this issue -- lovin the new architecture otherwise.

marceloslacerda pushed a commit to marceloslacerda/invoke that referenced this issue Oct 21, 2019
@bitprophet bitprophet changed the title sudo() doesn't play nice with nontrivial shell syntax sudo() doesn't play nice with '&&' (cd(), etc) Dec 10, 2019
@bitprophet bitprophet modified the milestones: Next bugfix, Priority Dec 10, 2019
@bitprophet
Copy link
Member

Belatedly sticking this in my priority queue as I get back into things here. I'm honestly not sure why I had it marked as bugfix since most solutions outlined are "feature sized". Not like it matters when these tickets stick around for so long 😩

Jyrno42 added a commit to thorgate/tg-hammer that referenced this issue Jan 13, 2020
Currently fabric2 has a problem with `cd` context manager and `sudo` command. To work around the issue
I implemented a customcontext manager that keeps the cd paths as an array (stack). If stack contains items
then the top path is fed into fabric.cd when running commands with use_sudo=False. If use_sudo is True then
we modify the command executed and wrap it into `bash -c "cd {path} && {command}"`. This should be reverted
to rely on builtin `cd` after the issue gets resolved in upstream.

Also:

- Implement hide as context manager on fabric2
@pkrefta
Copy link

pkrefta commented Feb 24, 2020

Are there any chances for workaround to get merged ? 😄

@char101
Copy link

char101 commented Aug 3, 2020

If the extra feature of the sudo command is not needed just use run

with c.cd(dir):
   c.run(f'sudo -u {user} command')

@bitprophet bitprophet changed the title sudo() doesn't play nice with '&&' (cd(), etc) sudo() doesn't play nice with shell syntax (&&, ;, etc) such as what cd() does Dec 31, 2020
@mpetronic
Copy link

mpetronic commented Apr 25, 2021

Why is sudo not including the -i flag? In fabric3, I was doing this to run a gcloud command and the gcloud command was getting picked up in the remote user's PATH just fine:

sudo(f'gcloud auth activate-service-account --key-file={credentials_file}', user=user)

But in modern fabric, that command fails because sudo is not causing the user's PATH to be set. I have to do this to make the same command work now:

c.sudo(f'bash -l -c "gcloud auth activate-service-account --key-file={credentials_file}"', user=user)

By hacking the library and adding -i here in context.py:

if user is not None:
    user_flags = "-i -H -u {} ".format(user)

the original command line from my fabric3 implementation just works with no changes.

@maltevesper
Copy link

Are there any chances for workaround to get merged ? 😄

This issue is a bit of a nuisance, what needs to happen that we can get a PR that will be merged? (What criteria does the pull request need to fullfill? I am willing to look into it, and propose alternative solution if there is a chance that a fix is actually adopted into an release, otherwise we would just have to work around this with another dirty hack 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests