Prompt for passphrase if private key is encrypted

[guludo]

Hi there.

By browsing the code and looking a some Github issues, it looks like that passing a passphrase as argument is required when the private key to be used is encrypted. While that makes a lot of sense, I think it would be nice if asyncssh provided a way to automatically prompt for the passphrase if the private key is encrypted. For example:

async with asyncssh.connect('localhost', client_keys=['my_ssh_key'], passphrase=prompt_passphrase) as conn:

Where prompt_passphrase is a callable that will interactively read the passphrase from the keyboard. Such a callable would be used only if my_ssh_key is encrypted.

[ronf]

This is an interesting idea! You wouldn't be able to invoke a regular callable directly if it was going to block waiting for user input, as that would prevent the asyncio event loop from making progress on other tasks. However, you could run something like that in an asyncio executor.

Here's a simple example of how you might do something like this yourself:

async def read_key(path, prompt_passphrase):
    try:
        return asyncssh.read_private_key(path)
    except asyncssh.KeyImportError:
        loop = asyncio.get_event_loop()
        passphrase = await loop.run_in_executor(None, prompt_passphrase)
        return asyncssh.read_private_key(path, passphrase)

In my test case, I used getpass to read the passphrase:

def prompt_passphrase():
    return getpass.getpass('Passphrase: ')

Then, to actually read the key, I used something like:

    k = await read_key('/tmp/k', prompt_passphrase)

If the key isn't encrypted, this reads and returns it immediately. If it is encrypted, though, the except block runs and requests the passphrase in an executor so that the event loop keeps running in the meantime. Once the passphrase is entered, it returns the decrypted key (or an exception if the passphrase was wrong).

With a tiny bit more effort, this could probably also be changed to support passing in an awaitable, which could be used without have to involve an executor while still allowing the event loop to keep running. I think something like the following would work:

async def read_key(path, passphrase):
    try:
        return asyncssh.read_private_key(path)
    except asyncssh.KeyImportError:
        if inspect.iscoroutine(passphrase):
            passphrase = await passphrase
        elif callable(passphrase):
            loop = asyncio.get_event_loop()
            passphrase = await loop.run_in_executor(None, passphrase)

        return asyncssh.read_private_key(path, passphrase)

This version accepts awaitables, callables, or a static value.

I see only one problem with integrating this into the existing AsyncSSH code. All of the functions that actually load the keys are non-async, expecting to always return immediately and with no guarantee that they are even being invoked from inside an asyncio event loop. So, at the point where the code knows if a passphrase is going to be required or not, there's no way to trigger this new behavior. It would have to be done up front in a async function (like in the example above).

[guludo]

Thanks for the explanation! I think I understand the problem you are pointing out.

I do not have much experience writing async code in python, but I wonder if we could check if we are running in an event loop and trigger the correct behavior based on that information. It looks like it is possible to check if we are inside an event loop:

import asyncio


def regular_function():
    try:
        asyncio.get_running_loop()
    except RuntimeError:
        print('Not in an event loop')
    else:
        print('In an event loop right now')


async def main():
    regular_function()


asyncio.run(main()) # Output: In an event loop right now
regular_function() # Output: Not in an event loop

[ronf]

Yes - it's possible to tell if a non-async function is called from within an event loop or not. However, a non-async function can't actually await anything. That's only possible from an async function, and the existing AsyncSSH API functions that take passphrases are almost all non-async functions right now. There are some exceptions, like the top-level connect() call, but all the functions that import or read private keys or key pairs are all non-async. A new API (using an async function) would be needed for them.

Given that a caller of AsyncSSH can do this sort of thing themselves in just a few lines of code, I'm not sure it's worth the disruption it would cause in the existing library code to try and add it there. I do like the concept, but if the caller knows they need this, it could be as simple as replacing:

    k = asyncssh.read_private_key(path, passphrase)

with:

    k = asyncssh.read_private_key(path, await prompt_passphrase())

...if prompt_passphrase was an awaitable. If it is a plain callable, it's one extra line, but still quite easy:

    loop = asyncio.get_event_loop()
    k = asyncssh.read_private_key(path, await loop.run_in_executor(None, prompt_passphrase))

In Python 3.9 and later, this gets even simpler for callables:

    k = asyncssh.read_private_key(path, await asyncio.to_thread(prompt_passphrase))

[ronf]

Closing due to inactivity. Feel free to re-open this or create a new issue if you're unable to get the above suggestions to work.

[goblin]

The approach you've suggested relies on knowing the path to the key, and knowing that a key file will be used at all. AsyncSSH already determines this, but doesn't seem to export functions that could ease this for the user (i.e. something like get_keyfilename_for_connecting_to(host)). Users of the library thus have no easy way of determining that they need to prompt for a passphrase (or a password, for that matter). They would need to parse the config files again.

I think it would be pretty nice if SSHClientConnectionOptions supported a new option to provide an awaitable to prompt the user for passphrase or password (passing the name of the keyfile being tried, or e.g. None if a password is needed).

[ronf]

Interesting idea. I'll take a closer look at this over the weekend.

My thinking is to simply allow the existing password and passphrase arguments to optionally be an awaitable. When this is the case and the value is needed, the awaitable would be executed and return the value. In the case of passphrases, the awaitable could be called with the key filename as an argument. This would mean probably not supporting the awaitable for import_private_key, but it's probably not all that necessary there since the caller is passing in specific key data and should already know the passphrase to use for it.

For passwords, there's already support for a callback by subclassing SSHClient and implementing password_auth_requested on that class. However, supporting an awaitable here would be less code for the caller, and I don't think it will be all that difficult to implement.

[goblin]

Yes, the solution you're proposing is even better. I also don't think import_private_key needs to support an awaitable.

Many thanks!

[ronf]

Ok - part 1 of this change is now in the "develop" branch as commit f4df7f4. It adds support for the password argument in SSHClientConnectionOptions to be a callable, coroutine function, or an awaitable which return either a string or None.

Note that you if you are trying to prompt for a password interactively through stdin/stdout, you need to be careful not to do blocking I/O there, since that will block the event loop from running.

If you choose to do async I/O on stdin/stdout, that may cause other problems if you later try to use blocking functions like print() or do logging to there.

Next up is a similar change for setting the passphrase to use when loading private keys from files.

[goblin]

@ronf please also check out #641 as it might be related.

[ronf]

As it turns out, supporting an awaitable when loading key pairs is a bit tricky, as the load function itself is not currently an async function. All of the functions which determine the config (including the key-loading functions) are synchronous right now. It would be easy to add support for a callable here, but it would be much trickier to add support for an awaitable. So, I think as an initial attempt I'll add support for only a callable in this case.

[ronf]

Ok - I've got something working here, but I'm not sure it will solve your use case, if you really need to make a blocking call after finding out which key file is being loaded. You'd basically have to do the key loading prior to starting the event loop, or be ok with the loop blocking all tasks until the passphrase was provided. Once the key is decrypted, you could then start up an event loop with the already-loaded set of keys.

[ronf]

Looking at the code more closely, the main AsyncSSH connect/listen functions already do the config evaluation in a separate thread (using an executor), so it turns out that running the synchronous callable won't block the asyncio event loop after all. It could if you created your own SSHClientConnectionOptions object, but if you wanted to do that you'd just need to make sure that you created that options object from within an executor as well, and the callable you provide will be run from within that.

A first cut of this code is now checked in as commit 56e533b if you'd like to give it a try.

[goblin]

I was hoping to prompt for the passphrase using Textual's ModalScreen dialog. Textual also uses asyncio for all its magic.

Do I understand correctly, you're saying that:

1. AsyncSSH's .connect() will run in a separate OS thread and will call my callable there, and
2. my main asyncio loop will continue running while that thread waits for the callable to finish?

If that's the case, then I believe I could use the thread-safe queue.Queue to communicate the passphrase from my main thread (running the asyncio loop) to the blocked .connect() thread, am I right?

[goblin]

Allright, it looks like it works :-)

I've created a stub app that:

1. shows a Textual UI with a connect button and a counter (to show main loop is ticking)
2. upon clicking that button, connects to the host passed as an argument (sys.argv[1])
3. execs ls / on that host, displaying the stdout result.

Here it is: https://gist.github.com/goblin/b0651fa0bc90c711b5a0d9bf74adbb08

I've decided to use janus as it's a two-faced sync/async queue, which looks like it was done exactly for this purpose.

Two caveats:

1. The passphrase is asked for even if the key is unencrypted (although anything could be entered as the passphrase),
2. There's no support for re-asking the passphrase if it was mistyped.

In case of 1., I believe AsyncSSH shouldn't call the callable on unencrypted keys.

In case of 2., we might argue that it's the app's job to re-ask for the passphrase upon seeing KeyEncryptionError, however, when the callable is passed in during .connect(), we have the slight issue that we'll have to reconnect again. That's not a biggie, but I believe OpenSSH actually keeps the connection open while asking the user 3 times to re-enter the passphrase, which is slightly better in terms of network calls (connecting/reconnecting). Therefore it might be slightly more convenient if AsyncSSH re-asked for the passphrase by calling the callable again (up to some configurable number of times, or let's just say 3x is reasonable enough).

Overall I'm very impressed with your lightning-fast response, thank you very much for adding this new feature so quickly!

[goblin]

Also, I just wanted to point out: if you'd like to also support awaitables instead of just callables (which could be very handy for my use case), you could perhaps use janus internally in a similar fashion.

I haven't considered all the edge cases, so my stub app might be prone to deadlocks or other races, but the general idea seems to work.

[ronf]

Do I understand correctly, you're saying that:

1. AsyncSSH's .connect() will run in a separate OS thread and will call my callable there, and
2. my main asyncio loop will continue running while that thread waits for the callable to finish?

If that's the case, then I believe I could use the thread-safe queue.Queue to communicate the passphrase from my main thread (running the asyncio loop) to the blocked .connect() thread, am I right?

Not quite. The actual connect() call still runs as a standard asyncio task, in the asyncio event loop that runs in the main thread. The only thing that runs in a separate (normally short-lived) thread is the evaluation of the SSHClientConnectionOptions, which would include the call it makes to load_keypairs() that could involve your callback if you provided one in the passphrase argument. This may be called multiple times if there's more than one key being loaded. At moment, all of this is being done prior to actually attempting to use any of these keys. In fact, it's generally done even before making an upstream SSH connection.

Because this is running in a separate thread, anything happening in the main thread's asyncio event loop will continue to run unaffected by whatever the thread might be doing. If you want to actually use something in the event loop to read the key, you should be able to use run_coroutine_threadsafe() from your callback to run something in the event loop and wait for a response. There's no need for janus (or any other form of queue) if you do that. Eventually, the value computed by the callback will be passed back to the task in the event loop which is doing the connect() via the return value to run_in_executor(), so that too is already thread safe without involving any queue.

I can see where you'd want the callback to not be invoked on unencrypted keys, but that turns out to be quite a bit more difficult and invasive. First, this would involve pushing this change done MUCH deeper than it is right now, and it would need to be customized for each of the different key formats, since each one has a different way to represent encrypted keys. Also, in at least one case, there is no way to identify in advance whether a key is encrypted or not. The code currently just tries to decrypt the key blindly if you provide a passphrase and if that fails then it falls back to seeing if the read is readable without decryption, but it actually has to try both PKCS#8 decoding and PKCS#1 decoding on that data, and there are actually multiple PKCS#1 encodings that need to be tried.

Also, I can see an argument where you'd want to wait and see what keys the server will even let you attempt to authenticate with before trying to decrypt the keys, but that's not how the current code works, and depending the key format it might require access to a separate public key file which isn't always present on the client. Normally, the public key can be determined from the private key, but only if you decrypt it first. I'm just not sure the code disruption that would be needed here is really justified.

Also, I just wanted to point out: if you'd like to also support awaitables instead of just callables (which could be very handy for my use case), you could perhaps use janus internally in a similar fashion.

At the very least, a syntax change would be required here, as there's no such thing as an async constructor. So, instead of co = asyncssh.SSHClientConnectionOptions(args), it would be necessary to introduce something like an async class method that could be used as a factory, such as co = await asyncssh.SSHClientConnectionOptions.construct(args). This method could then do the run_in_executor() that I'm currently doing one level higher in the connect/listen functions, but it would then also be available to you if you wanted to construct your own "options" objects. I think that might be worth doing, but unfortunately the calling code would need to know that it had to do this.

When I started out, this options construction code did not involve any blocking calls, so there was no need for an executor. However, when I added support for things like PKCS#11 smart cards and Fido keys, I ran into the problem that those libraries weren't async. By then, though, there was no way to change the code without breaking backward compatibility. So, this issue isn't really new to adding this callback. It's just another example where it might come up, depending on how you choose to determine the passphrase to return.

[goblin]

If you want to actually use something in the event loop to read the key, you should be able to use run_coroutine_threadsafe() from your callback to run something in the event loop and wait for a response. There's no need for janus

Indeed! I've updated the gist and it's way simpler now, without janus. Thanks!

I can see where you'd want the callback to not be invoked on unencrypted keys, but that turns out to be quite a bit more difficult and invasive.

OK, Revision 3 of the gist now works around this by simply trying to read_private_key in the passphrase callback, and doesn't prompt for the passphrase if that succeeds. Would you like (me) to open a new issue so we can remember this and maybe address this in the future?

Also, I can see an argument where you'd want to wait and see what keys the server will even let you attempt to authenticate with before trying to decrypt the keys, but [...] I'm just not sure the code disruption that would be needed here is really justified.

OK, fair enough. It would be nice to have, but the way it currently works is good enough, I think. At least in my simple case where the correct IdentityFile is simply specified in ~/.ssh/config for each Host to connect to, it works pretty well.

At the very least, a syntax change would be required here, as there's no such thing as an async constructor. So, instead of co = asyncssh.SSHClientConnectionOptions(args), it would be necessary to introduce something like an async class method that could be used as a factory, such as co = await asyncssh.SSHClientConnectionOptions.construct(args).

Yes, however, I'm no longer using SSHClientConnectionOptions, because as you've pointed out, that can lead to the passphrase being asked for the wrong key.

Instead, I'm using async with asyncssh.connect(passphrase=callback), which actually already is an async context manager, so no change to the interface would be needed. This of course would mean, however, that the awaitable can only be passed to .connect, and not to SSHClientConnectionOptions.

[ronf]

Indeed! I've updated the gist and it's way simpler now, without janus. Thanks!

Happy to help!

OK, Revision 3 of the gist now works around this by simply trying to read_private_key in the passphrase callback, and doesn't prompt for the passphrase if that succeeds. Would you like (me) to open a new issue so we can remember this and maybe address this in the future?

Yeah - that should work, though it a bit wasteful in terms of throwing away the key in the case where it is unencrypted and having to read it all over again. I know you wanted to leverage the existing SSH config file to determine which key to load, but if you had that mapping available already in your code, you could just load the keys yourself, prompting for passphrase only if the read without a passphrase failed, and you could also do the retry on a bad passphrase in that case.

I'm still thinking about what it would take to defer calling the passphrase callback until the code was actually doing the public key authentication with an SSH server. That would require adding some way to create an SSHKeyPair object that stored something like a public key, the encrypted version of a private key, and an associated passphrase callback to call when the server accepted the initial public key proposal and it was time to actually do the signing with a private key. This probably wouldn't be possible in all cases, unless the user kept both an encrypted private key and a corresponding public key on the client machine as two separate files (which is often the case, but technically isn't required). The good news is that the new OpenSSH private key format actually lets you do this all in one file, but many people using RSA or ECDSA which are still using PKCS#8 or even PKCS#1 format.

My main concern here is that this is just too big a change, especially when there are other ways to do this today by just having the application read the keys itself and passing the already decrypted version of them into AsyncSSH.

I'm no longer using SSHClientConnectionOptions, because as you've pointed out, that can lead to the passphrase being asked for the wrong key.

Instead, I'm using async with asyncssh.connect(passphrase=callback), which actually already is an async context manager, so no change to the interface would be needed. This of course would mean, however, that the awaitable can only be passed to .connect, and not to SSHClientConnectionOptions.

The problem is actually not with passing in an awaitable. That could be done just fine even in the options constructor case. However, the fact remains that all of the options processing is currently synchronous. None of the functions used there would be able to do an "await" on something if an awaitable was passed in. All that code would need to be reworked to allow awaitables, and even then we'd have the problem that some of the calls currently block, so I'd still need to use an executor for those.

I suppose one option might be to leave everything running in the executor as it is now, but use run_coroutine_threadsafe() from there, like I suggested to you as a way to run an awaitable from another thread and wait for the result. That might not be too big a change. I'll experiment with that.

[goblin]

Revision 3 of the gist now works around this by simply trying to read_private_key in the passphrase callback, and doesn't prompt for the passphrase if that succeeds. Would you like (me) to open a new issue so we can remember this and maybe address this in the future?

Yeah - that should work, though it a bit wasteful in terms of throwing away the key in the case where it is unencrypted and having to read it all over again.

Agreed, it's a bit wasteful, but good enough as a workaround for me for now.

I know you wanted to leverage the existing SSH config file to determine which key to load, but if you had that mapping available already in your code, you could just load the keys yourself, prompting for passphrase only if the read without a passphrase failed, and you could also do the retry on a bad passphrase in that case.

Is there a way for me to get that mapping from OpenSSH config files, short of parsing them all on my own? I mean, can I leverage the existing code in AsyncSSH somehow to aid in this?

I'm still thinking about what it would take to defer calling the passphrase callback

My main concern here is that this is just too big a change

If it was me, I'd create an issue for it and consider doing it later, maybe together with some other bigger changes. Like you say, it's probably too big of a change for what it's worth just for this.

especially when there are other ways to do this today by just having the application read the keys itself and passing the already decrypted version of them into AsyncSSH.

It's just hard for the application to redo all the SSH config parsing work (and check all the conditionals etc.). If the application uses totally it's own, separate config, then yeah, that would be a viable solution.

Unless I'm missing something and the interface for parsing those files and determining which key to use is already available somewhere in AsyncSSH?

I suppose one option might be to leave everything running in the executor as it is now, but use run_coroutine_threadsafe() from there, like I suggested to you as a way to run an awaitable from another thread and wait for the result. That might not be too big a change. I'll experiment with that.

Cool, yeah, that definitely looks like a good option. The async context manager creator (__aenter__) for .connect must be called in an async loop, so it can get access to the currently running loop and pass it on to run_coroutine_threadsafe() later.

[ronf]

Is there a way for me to get that mapping from OpenSSH config files, short of parsing them all on my own? I mean, can I leverage the existing code in AsyncSSH somehow to aid in this?

There is an "SSHConfig" class with a load() method that could be used to parse an OpenSSH config file. However, this class is currently internal to AsyncSSH, and it would probably need some work to be able to cleanly use it from outside. Right now, you need to pass in arguments explicitly specifying the host, port, user, etc. to get the conditionals to work right, and there are some subtle interactions between the use of the options objects and the config objects.

If it was me, I'd create an issue for it and consider doing it later, maybe together with some other bigger changes. Like you say, it's probably too big of a change for what it's worth just for this.

That's fair. If you want to create a new issue to track this, I'm open to that.

[ronf]

I've gone ahead and add a note in SSHClientConnectionOptions and SSHServerConnectionOptions about the fact that a callable/awaitable passed in as a passphrase will be called on every filename set as a client/server key each time one of these objects is constructed, even in the keys are not encrypted or never used for authentication.

[goblin]

I've also updated my gist at https://gist.github.com/goblin/b0651fa0bc90c711b5a0d9bf74adbb08 to Revision 4, which now makes use of the awaitable support you've added. Great stuff, many thanks!

[ronf]

Looks good! Thanks for all your help working through this...