Support for 'as' method

[robd]

I was wondering if it would be possible to support calls to the as() method?

For some reason I couldn't work out, when execute is called, it does not call the patched _execute! method, it calls the original one, which hangs waiting for input.

I couldn't understand the class hierarchy, but is something that could be fixed?

[kentaroi]

Would you please tell me the situation that you have to use as method?

Because I was not sure how people use SSHKit or Capistrano, I defined execute! method instead of overloading execute method.

Current implementation of _execute! method always request pty even if a user set :pty, false so that sshkit-sudo gem does not affect the behavior of the other tasks a user defined.

The other way is that _execute! method sees fetch(:pty) value and overloads execute method (users must be set :pty, true when using sshkit-sudo).

I am considering changing it into the latter.

[robd]

Hi

My setup is like this:

I have an admin user for deployments etc, app_admin with full sudo privileges (requires password). I have a runtime user, app_run, with no sudo privileges which I use to to run the rails app server processes.

I am using the as method in order to run a backup as the app_run user when logged in as app_admin.

In the end, I set up passwordless sudo for the app_admin to run anything as app_run, but I'm not quite sure if this is desirable or not.

Re pty, I agree this is a hard decision - I don't really know what the best thing is to do here.

In my case I'd prefer to use pty only for sudo commands, and not for 'normal' commands which don't need it. This is to minimise any unexpected side effects of pty. This could be achieved by enabling it for the :sudo command or a user has set :pty, true.

[kentaroi]

Hi @robd !

Thank you very much. All your comments are helpful for me.

In my case I'd prefer to use pty only for sudo commands, and not for 'normal' commands which don't need it. This is to minimise any unexpected side effects of pty. This could be achieved by enabling it for the :sudo command or a user has set :pty, true.

So, your point is as method should use pty and call _execute! because as method executes sudo here.

Do I understand correctly?

command	requre pty
execute/capture/test	fetch(:pty)
sudo	YES
execute!	YES
as	YES

Thank you,

[robd]

Yes! Your explanation is much better than mine was!

Looking at the code it might be hard to support that behaviour, because as calls execute directly so it's not simple to know whether to open pty etc or not. Also in order to make this work, you would also have to intercept the user method in command.rb.

So there's not an easy way of intercepting these sudo calls without requiring pty for all execute calls.

One, hacky, way would be to simply string match 'sudo' in the execute method and require a pty in these cases. That would have the advantage of not needing the sudo or execute! methods. The users could just call execute('sudo whatever') or execute(:sudo, 'whatever') and a tty would be opened for these connections, and the password prompts would be correctly intercepted.

What do you think?

[kentaroi]

Hi @robd !

I don't like 'sudo' matching in execute method very much, but it might be the best choice.

I am considering to choose the hacky way you showed and add config to allow users to customize behaviors.

Thank you,

[robd]

Yes, I agree string matching sudo isn't nice at all. I was wondering about being able to configure different rules depending on the contents of the command sent and the response received. One way to look at this is that there two separate things going on:

1. allowing customising the channel based on the execute args here:

ssh.open_channel do |chan|
  chan.request_pty

2. supporting performing some action based on matching the response data here:

if data =~ /Sorry.*\stry\sagain/
  SSHKit::Sudo.password_cache[password_cache_key(cmd.host)] = nil
end
if data =~ /password.*:/
  key = password_cache_key(cmd.host)
  pass = SSHKit::Sudo.password_cache[key]
  unless pass
    pass = $stdin.noecho(&:gets)
    SSHKit::Sudo.password_cache[key] = pass
  end
  ch.send_data(pass)
end

---

So maybe this should be configured via a strategy object with two commands configure_channel(chan, cmd) and handle_data(data, cmd) or two procs. For configuring the channel, we could then have different implementations depending on how people want it to behave eg

# Current sshkit-sudo implementation
ALWAYS_PTY = lambda { |chan, cmd| chan.request_pty } 

# For people who only want interactive sudo if they explicitly call execute(:sudo ...)
PTY_ON_SUDO = lambda { |chan, cmd| chan.request_pty if cmd.command == :sudo }

# For people who want interactive sudo if they explicitly call execute(:sudo ...) or as(:whoever)
PTY_ON_SUDO_OR_AS = lambda do |chan, cmd| 
  if cmd.command == :sudo || cmd.options.key?(:user) || cmd.command =~ /sudo -u .* whoami/
    chan.request_pty
  end
end

The user could then configure it with one of these options or write their own:

SSHKit::Sudo.configure_channel_proc = SSHKit::Sudo::PTY_ON_SUDO_OR_AS

Implementation calls out to the proc when the channel is opened:

ssh.open_channel do |chan|
  SSHKit::Sudo.configure_channel_proc.call(chan, cmd)

An alternative less versatile approach (but probably better since we only have one use-case for configuring the channel) would be to just add support for a proc for Netssh.config.pty. This might even be accepted as a PR in the main sshkit repo.

ALWAYS_PTY = lambda { |cmd| true } 
PTY_ON_SUDO = lambda { |cmd| cmd.command == :sudo }
PTY_ON_SUDO_OR_AS = lambda do |cmd| 
  cmd.command == :sudo || cmd.options.key?(:user) || cmd.command =~ /sudo -u .* whoami/
end

ssh.open_channel do |chan|
  chan.request_pty if Netssh.config.pty.call(cmd)

Another approach I considered would be to introduce a pty attribute on command, and set this up somewhere in the flow, but I couldn't really get my head around whether this would be better or not. Calling out to a global config variable Netssh.config.pty feels like a bit of a code smell, but maybe, since pty is specific to the Netssh backend, this is the right place for this to live.

A similar thing could be done with a Proc for matching and handling the response data, although again we only have one use-case for this at the moment. But it might be possible to get a PR accepted into sshkit if this was more general.

Let me know your thoughts.

[kentaroi]

Hi @robd !

So maybe this should be configured via a strategy object with two commands configure_channel(chan, cmd) and handle_data(data, cmd) or two procs.

I agree with you.

I don't have a strong opinion on which module, SSHKit::Sudo or Netssh, should have pty configuration.

A similar thing could be done with a Proc for matching and handling the response data, although again we only have one use-case for this at the moment. But it might be possible to get a PR accepted into sshkit if this was more general.

About doing a PR to sshkit, execute! method won't be merged. But, I do want to have per-command settings as well as global settings like this:

def _execute(*args, pty: nil, &data_handler)

Because in some cases, we need to use pty even if the command does not start with 'sudo'. For example, when using pipe and sudo like this: echo 'appending line' | sudo tee -a somewhere.

And, as a gem creator, sometimes you want to force command to use (or not to use) pty ignoring global pty setting. (Current capistrano-sidekiq gem requires a user to set :pty, false globally. https://github.com/seuros/capistrano-sidekiq/issues/23)

By the way, because I don’t have enough time to work on this gem or make a PR to sshkit right now, if you do a PR to sshkit, I am very happy.

Thank you,

[robd]

Hey @kentaroi

I certainly understand that getting a PR to sshkit would be a lot more work than maintaining the separate sshkit-sudo gem, and from what I've seen over there, the sshkit team are very against sudo with passwords. They just close any issue mentioning password based sudo as 'not a bug' so I probably don't have time to try and get this added.

Understand about the per-command config. The one thing I am not sure about is whether the pty: option should be on the command api, because it only applies to the NetSSH backend, not the local backend.

echo 'appending line' | sudo tee -a somewhere

This could be achieved by writing a more general or a specific custom Netssh.config.pty proc:

PTY_ON_CONTAINS_SUDO = lambda do |cmd| 
  cmd.command.to_s =~ /sudo/ # General
end

# Or

PTY_FOR_TEE_ONLY = lambda do |cmd| 
  cmd.command == "echo 'appending line' | sudo tee -a somewhere" # Brittle, but highly targetted
end

Configuring it like this has a big advantage that any user can target any commands in any gem by writing a custom matching rule without having to monkey patch the gem to add the :pty option.

Another approach for your example might be to use as(:root) (not sure if this would work in all cases though):

as(:root)  do
  execute("echo 'appending line' | tee -a somewhere")
end

Cheers, Rob

[kentaroi]

Hi Rob!

I certainly understand that getting a PR to sshkit would be a lot more work than maintaining the separate sshkit-sudo gem, and from what I've seen over there, the sshkit team are very against sudo with passwords. They just close any issue mentioning password based sudo as 'not a bug' so I probably don't have time to try and get this added.

I see.

Understand about the per-command config. The one thing I am not sure about is whether the pty: option should be on the command api, because it only applies to the NetSSH backend, not the local backend.

I see the point, but, in my understanding, the local backend is just a toy. The sshkit team has not been thinking about the local backend seriously. (ex. leehambley wrote "I think run_locally in SSHKit is a terrible idea anyway!" (https://github.com/capistrano/sshkit/pull/24). The local backend did not have capture and test methods for half a year after the public release of Capistrano 3 (https://github.com/capistrano/sshkit/pull/50). The local backend cannot do —dry-run for a long time (https://github.com/capistrano/sshkit/issues/39). ) Therefore, I also don’t think about the local backend seriously.

From a practical perspective, each command opens a channel so it should be configurable on each command. Just having a global configuration is unacceptable, even if it has a flexible API.

In the case of capistrano-sidekiq, I am not sure the cause of the bug, but if there were a pty option for command, the maintainer could avoid the bug by setting pty to false for the very command without any effect on global pty configuration.

Of course, this is a sshkit-sudo issue and even if pty is configurable on command by sshkit-sudo gem, the above capistrano-sidekiq issue is not solved, but finer configuration feels good for me.

Another approach for your example might be to use as(:root) (not sure if this would work in all cases though):

as(:root)  do
  execute("echo 'appending line' | tee -a somewhere")
end

Even if it works, I am against forcing users to change their one liners.

Thank you, Kentaro

[robd]

Hey

Thanks for the info on the local backend - very interesting . I hadn't realised that the local backend wasn't taken that seriously, I now see why the pty: option per command makes sense.

Having said that, I would be in favour of having some sort of support for global matching commands as well. i.e. what I was talking about before:

ssh.open_channel do |chan|
  SSHKit::Sudo.configure_channel_proc.call(chan, cmd)

This would cover the cases where people are using a gem and the gem author isn't using sshkit-sudo so hasn't specified pty: nil on their execute lines, for example the as() case above.

Do you think it would be OK to have both approaches? I will try to find time to make a PR for sshkit-sudo if you like.

Thanks, Rob

[kentaroi]

Hi!

Do you think it would be OK to have both approaches? I will try to find time to make a PR for sshkit-sudo if you like.

Yes, of course! If you do a PR, I will merge it in.

Thank you, Kentaro

[robd]

Hey. See what you think of this: https://github.com/kentaroi/sshkit-sudo/pull/3

[robd]

Continuing this in https://github.com/capistrano/sshkit/pull/234