Improve support for SSH_ASKPASS

[s1kx]

As mentioned in issue #18, if a user sets SSH_ASKPASS, it is silently ignored and the user is prompted for the passphrase in the terminal.

In order to fix this issue, I have implemented changd that ssh-add will be invoked with a < /dev/null redirect in order to force launching the SSH_ASKPASS prompt.

Additionally, ssh-agent needed to be spawned with certain environment variables to be compatible with ssh-add -c and SSH_ASKPASS.

[s1kx]

I would also suggest a refactoring of the static methods residing in AgentManager. There are some functions which should be moved out of the class and some functions which should be turned to class methods. I can work on this in the next days if you agree.

[s1kx]

After some tweaking, everything should be working now and ssh-ident should automatically detect if SSH_ASKPASS used and use ssh-agent and ssh-add accordingly.

[ccontavalli]

Patrick, thanks for the good work. A couple questions though, by skimming through the patches:

1) When run manually, I have never had to run ssh-agent in foreground mode, it ought to be possible to make it work correctly without using -D?

2) ssh-ident replaces sys.stdout and sys.stderr as soon as started with /dev/tty. Does this replace the stdout / stderr used by spawned processes? It is unclear from python docs? If that was the case, and "ssh" was invoked with stdout replaced as /dev/null, ssh-ident will instead start ssh and ssh-add with /dev/tty as stdin? Changing this to keep the original /dev/tty might solve the problem?

https://github.com/s1kx/ssh-ident/blob/3a3a73d1aefa6d782cd3c9b01ccef886fb708834/ssh-ident#L946

3) From a quick reading of the code (please, correct me if wrong! Maybe I just misread it), with SSH_ASKPASS_X11, the use of an agent will be mandated (or disabled) based on that variable only? I would personally prefer for ssh-ident to behave as closely and transparently as possible to the original ssh binary, eg, find a way to allow ssh-add to work just like it was invoked manually from the terminal? That was also the intent of propagating the environment unchanged.

[s1kx]

Hey Carlo,

Sorry I just pushed a big rebase that fixes all of the issues mentioned by you.

1) It does not need to be run in foreground mode. It temporarily fixed an issue I encountered, but it ended up not writing the SSH_AGENT_PID to the agent file. I changed it to run with /bin/sh (as initially in your code), which made it work as intended again. The most noteworthy change in GetAgentFile is that instead of being run with /usr/bin/env -i, it passes certain necessary environment variables with subprocess.Popen.

2) The documentation for subprocess states that unless explicitly overridden, the child process will inherit the file handles from the parent.

I believe that ssh-ident will currently start ssh/ssh-add with /dev/tty as stdout/stderr and inherit stdin from ssh-ident's parent (since both ssh-ident and it's subprocesses should inherit the file handles from their respective parents). I'm not sure though which problem exactly you are referring to

You may also note that I have implemented debug output through use of /bin/sh -xc ssh-agent -d & with setting stderr to sys.stderr to redirect it's output to that of ssh-ident.

I have changed the agent file back to being written via pipe in the shell command, since we don't want open file handles floating around (e.g. if run in debug mode and we have to fork manually).

3) The SSH_ASKPASS_X11 option did not mandate the use of an agent, but the use of SSH_ASKPASS. I have gotten rid of the SSH_ASKPASS_X11 option in favor of detecting if SSH_ASKPASS was set and using ssh-add </dev/null if that is the case. This is the behavior described in the ssh-add manpage to force using SSH_ASKPASS if DISPLAY is also set. (Maybe we should check for DISPLAY in os.environ also).

This is not directly transparent behavior, since the user may not want to use SSH_ASKPASS. If you would rather not enforce this behavior, I would strongly be in favor of adding an option again for toggling this behavior.

Note that right now, SSH_ASKPASS is only forced for ssh-add, not for ssh. e.g. if a user uses an identity file from outside the agent, the passphrase will still be read from the terminal instead of the askpass program.

I'm not sure how we could force ssh to also use SSH_ASKPASS, since at least on my end running ssh with </dev/null (and setsid ends) with no tty being available for the session. We might have to find a compromise here with only supporting SSH_ASKPASS for ssh-add, unless we can figure out some tty/pipe trickery for SSH.

Note that the change to ssh-agent is unrelated and required for SSH_ASKPASS to work.

[s1kx]

To clarify, currently the way that ssh-ident with the PR patches behaves is that if SSH_ASKPASS is set, it is used for all password entries and confirmations of the ssh-agent. This behavior is similar to that of other ssh agents such as gpg-agent.

As far as I know, using SSH_ASKPASS should offer the most universal way of providing passphrases in all use cases such as calling ssh-ident from a terminal, from a script or from git.

[s1kx]

Confirmed that it is working with scripts and git. There's currently no option though to disable ssh-ident log output (if e.g. running in a script) apart from setting SSH_BATCH_MODE=1 or VERBOSITY=0.

Have you considered writing log output to stderr instead of stdout? This is common practice with ssh etc. and would allow pipe redirection.

[maddes-b]

Hi s1kx,

had you tested the changes from kevinr. Would like to introduce them into the fork at https://github.com/ssh-ident/ssh-ident1 Any test results would be great.

Regards Maddes