hashicorp / vagrant

Vagrant is a tool for building and distributing development environments.
https://www.vagrantup.com
Other
26.24k stars 4.44k forks source link

--no-tty option not being propagated to SSH command #11882

Open 0x09 opened 4 years ago

0x09 commented 4 years ago

Hi,

In https://github.com/hashicorp/vagrant/commit/0296e59baff5f74df30c3a658492d1d916326911 --no-tty was added as a global option to the vagrant CLI. This option is also defined by the SSH command in order to enable/disable forced pseudo-terminal allocation. However, this addition results in the option being consumed before it can reach the SSH command and as a result ssh is always invoked with -t:

$ vagrant --debug ssh vm -c ''
...
INFO ssh: Executing SSH in subprocess: /usr/bin/ssh ["vagrant@127.0.0.1", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "Compression=yes", "-o", "DSAAuthentication=yes", "-o", "IdentitiesOnly=yes", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-i", "<path>/private_key", "-t", "bash -l -c ''"]

$ vagrant --debug ssh vm --no-tty -c ''
...
INFO ssh: Executing SSH in subprocess: /usr/bin/ssh ["vagrant@127.0.0.1", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "Compression=yes", "-o", "DSAAuthentication=yes", "-o", "IdentitiesOnly=yes", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-i", "<path>/private_key", "-t", "bash -l -c ''"]

Previously the -t option to ssh would be omitted in the second command.

I attached a simple fix for this as a gist patch, which just keeps this option in ARGV and gives the SSH command's opt proc precedence for parsing it. This produces the expected result when run as above and passes the unit test suite, but I decided to open an issue instead of preemptively creating a PR as I'm not sure if there are unwanted implications to allowing --no-tty through to other plugins that may not be expecting it (in which case a different approach for propagating this to CommandSSH would be needed.)

Let me know if I can provide any other info that would help, thanks!

Patch against current HEAD: https://gist.github.com/0x09/f2d8ddf965bf03115225949bb6e3faac

gkpacker commented 4 years ago

I'd like to implement that + I'll try to investigate if removing that line would have any side effect

gkpacker commented 3 years ago

Not sure, but it looks like you have to pass -- --no-tty to be sent correctly:

bundle exec vagrant --debug ssh vm --no-tty -c ''"

# ...
DEBUG cli: Invoking command class: VagrantPlugins::CommandSSH::Command ["vm", "-c", ""]

vs.

bundle exec vagrant --debug ssh vm -c '' -- --no-tty
DEBUG cli: Invoking command class: VagrantPlugins::CommandSSH::Command ["vm", "-c", "", "--", "--no-tty"]

It looks like removing argv.delete("--no-tty") will have some undesirable side effects

@0x09, can you test it?

0x09 commented 3 years ago

Hi @gkpacker, arguments following -- are passed directly to the ssh command rather than being interpreted by the plugin, so that results in

INFO ssh: Executing SSH in subprocess: /usr/bin/ssh ["vagrant@127.0.0.1", "-p", "2222", "-o", "LogLevel=FATAL", "-o", "Compression=yes", "-o", "DSAAuthentication=yes", "-o", "IdentitiesOnly=yes", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-i", "<path>/private_key", "--no-tty", "-t", "bash -l -c ''"]

Which errors out since --no-tty isn't a recognized option for ssh.

n.b. This same functionality can be used to workaround the issue of -t always being passed by vagrant by using vagrant ssh ... -- -T, as the -T option is used by ssh to do the opposite of -t i.e. force a pseudo-terminal not to be allocated.