Added support for additional OpenSSH compatibility flags.

[russjones]

Added support for OpenSSH compatibility flags for interactivity (-t and -T) and version information (-V). This is for customers that alias ssh to tsh ssh.

changelog: Added support for -t, -T, and -V flags to tsh for OpenSSH compatibility.

[russjones]

/excludeflake *

[zmb3]

I don't follow why we had to make the argument optional in kingpin and validate it here. Can we leave a comment explaining why this was necessary?

[russjones]

It's to support tsh ssh -V which can run without any additional arguments. I'll update the comment to explain why I had to remove Required() from that Arg.

[espadolini]

Does this affect the usage docs badly? I reckon that everyone would likely figure out that sshing into an unspecified destination is never going to work, but still...

[russjones]

It does.

Adding Required() removes the square brackets from --help output.

Not ideal, but I can update the help text to indicate this is a required argument.

Output of tsh ssh --help before:

$ tsh ssh --help
usage: tsh ssh [<flags>] <[user@]host> [<command>...]

Run shell or execute a command on a remote SSH node.

Flags:
[...]

Args:
  <[user@]host>  Remote hostname and the login to use
  [<command>]    Command to execute on a remote host

Aliases:

Output of tsh ssh --help after this PR:

$ tsh ssh --help
usage: tsh ssh [<flags>] [<[user@]host>] [<command>...]

Run shell or execute a command on a remote SSH node.

Flags:
[...]

Args:
  [<[user@]host>]  Remote hostname and the login to use, this argument is required
  [<command>]      Command to execute on a remote host

Aliases:

What do you guys think?

[espadolini]

Does this affect the usage docs badly? I reckon that everyone would likely figure out that sshing into an unspecified destination is never going to work, but still...

[zmb3]

/excludeflake *

[rosstimothy]

We already use a fork of kingpin, would it be worth investigating adding support in kingpin instead of all of the various workarounds that we've had to introduce in this PR to overcome its shortcomings?

[russjones]

@rosstimothy That's what I originally did and even had a working implementation.

I don't remember all the details, but during testing I found that the way we hook kingpin for terminate (which I would have to use) is broken and the fact that tsh does not panic often is incidental. Fixing this would have been a fairly big undertaking because we hook termination from tsh.

This is what I have in my notes:

https://github.com/gravitational/kingpin/blob/master/app.go#L617

None of those `a.terminate(1)` will actually exit due to how we hook the terminate function
in Teleport, which means things will not be set downstream and panic when it flows through
the rest of kingpin code.

This FatalIfError is called all over the place in kingpin.

https://github.com/gravitational/kingpin/blob/master/app.go#L642C23-L642C35