SSHClientConnectionOptions incorrectly verifies the passphrase of the wrong key

[goblin]

Assuming we have two client keys: ~/.ssh/id_ed25519 (encrypted with, say, passphrase1) and ~/.ssh/another_keyfile (encrypted with passphrase2), and the following piece of ~/.ssh/config:

Host example
   IdentityFile ~/.ssh/another_keyfile

and we try to connect to example using this sample client program:

#! /usr/bin/env python3

import asyncio, asyncssh, sys

async def main(passphrase):
    co = asyncssh.SSHClientConnectionOptions(ignore_encrypted=False, passphrase=passphrase)
    async with asyncssh.connect(sys.argv[1], options=co) as conn:
        result = await conn.run('ls')
        print(result.stdout)

print('input passphrase:')
passphrase=input()
asyncio.run(main(passphrase))

then the asyncssh.SSHClientConnectionOptions will incorrectly check the passphrase of ~/.ssh/id_ed25519 and raise KeyEncryptionError: Incorrect passphrase. It shouldn't check the passphrase yet, as it doesn't even know which key will be used.

If we remove that line, and don't pass options=co to .connect, then the connection will work fine, using the right key as specified in the config.

It appears there is no way to specify a passphrase to a non-standard key file.

[ronf]

I think the problem you are running into here is that keys are loaded when you do the initial assignment to "co". As you said, this can cause a problem if you don't know the proper passphrase to use yet. It will try the alternate key after trying the default, but it never gets that far when you use ignored_encrypted=False. Without that, it'll try to load the default keys but continue on if the passphrase is wrong, eventually trying another_keyfile.

You can avoid this problem by not creating your own SSHClientConnectionOptions. Instead, just do this in one step:

async with asyncssh.connect(sys.argv[1], ignore_encrypted=False, passphrase=passphrase) as conn:

Note that this only fails when you include the ignore_unencrypted=False. It will attempt to load the default keys without that option, but it won't return an error if the passphrase is wrong, and eventually it will try the other key files.

In general, trying to mix the config file settings with Options settings is a bit dangerous and may not always do what you want. It's better to keep all your options in either the arguments to connect() or in the config file and not try to mix in intermediate SSHConnectionOptions objects.

[goblin]

It will try the alternate key after trying the default, but it never gets that far when you use ignored_encrypted=False. Without that, it'll try to load the default keys but continue on if the passphrase is wrong, eventually trying another_keyfile.

OK, not sure about this. When I tried it like this:

    co = asyncssh.SSHClientConnectionOptions(passphrase=passphrase)
    async with asyncssh.connect(sys.argv[1], options=co) as conn:

it raised asyncssh.pbe.KeyEncryptionError: Incorrect passphrase. However, the solution you gave, i.e. to provide everything as arguments to connect, works great:

    async with asyncssh.connect(sys.argv[1], ignore_encrypted=False, passphrase=passphrase) as conn:

I didn't realize I could do this because I initially missed the very last sentence of the docs at https://asyncssh.readthedocs.io/en/latest/api.html#connect, that is:

These options can be specified either through this parameter or as direct keyword arguments to this function.

Perhaps an additional note would be useful, explaining this difference between passing the passphrase via a separate SSHClientConnectionOptions object and directly to asyncssh.connect.

Note that this only fails when you include the ignore_unencrypted=False. It will attempt to load the default keys without that option, but it won't return an error if the passphrase is wrong, and eventually it will try the other key files.

I'm guessing you meant ignore_encrypted=False here. Without this, if I understand correctly, it won't try the encrypted keys at all? Or am I wrong?

Overall I'm happy with your solution, I don't really want to create a separate SSHClientConnectionOptions object. But the behaviour is somewhat surprising, especially this nuanced difference between the way options are passed. So I'm leaving this open in case you want to either change the code, or mention it more clearly in the docs.

Many thanks for the very quick response!

[ronf]

I'm guessing you meant ignore_encrypted=False here. Without this, if I understand correctly, it won't try the encrypted keys at all? Or am I wrong?

You're right that i meant ignore_encrypted. That doesn't prevent AsyncSSH from attempting to load encrypted keys, though. If you provide a passphrase, it will try to load encrypted keys regardless of this setting. It's only when you pass in None for a passphrase that this argument matters, telling it whether to return an error when encountering encrypted keys vs. skipping over encrypted keys. It defaults to skipping encrypted keys if passphrase is None and the default key locations are being used. For non-default locations, it errors out if you fail to include a passphrase, but you can set this option if you'd prefer to skip encrypted keys instead.

The assumption here is that you should know whether your keys need a passphrase, and only provide one when they do. If you provide a passphrase and get it wrong, or if you explicitly specify a non-default location to an encrypted key without providing a passphrase, AsyncSSH will raise an error.

Rather than prompting the user to enter a passphrase for a specific key from within AsyncSSH, have you considered using ssh-agent and something like ssh-askpass? That's designed to provide ways to prompt the user out of band for a passphrase when one is needed.

[goblin]

If you provide a passphrase, it will try to load encrypted keys regardless of this setting. It's only when you pass in None for a passphrase that this argument matters

Ahh, yes, you're absolutely right, I've missed this bit from the docs. Thanks for clarifying! I can leave out ignore_encrypted=False from asyncssh.connect and it works just as well.

The assumption here is that you should know whether your keys need a passphrase

Right, unfortunately in my case, I don't know that. I'm leaving this for the user to configure via ssh_config, and I like that AsyncSSH parses this config and works out which key to use based on that.

Rather than prompting the user to enter a passphrase for a specific key from within AsyncSSH, have you considered using ssh-agent and something like ssh-askpass?

Same story, I'd rather leave it for the user to choose whether they want to use ssh-agent or provide the passphrase directly in my app. I myself don't particularly like ssh-agent, as I prefer the keys to be wiped from memory as soon as possible. Also not sure how well it'd work i.e. in Termux.

[goblin]

So, to reiterate the actual issue: I'm quite happy with the way asyncssh.connect works, however I do find it odd that going via SSHClientConnectionOptions produces different behaviour, and I believe this should be mentioned in the docs.

[ronf]

I've been thinking about this last point. I think maybe the reason you saw this was because there was no host information available when it tried to resolve the set of keys to use when you constructed the options object, before passing that to asyncssh.connect(). So, if your config file had conditionals in it to choose different keys depending on host, it might match the wrong values initially. Later, when it re-evaluated the config from within the connect() itself, it would get the right value, but your callback could end up being called initially with whatever keys matched when no host was provided.

This kind of mismatch in the options can happen with any kind of conditional configuration, but generally the developer would never see this, since they don't really get to see what the intermediate values are when building partial options objects. So, I'm not sure if this is really worth documenting this edge case. It might take a lot of explanation that wouldn't be relevant to most people.

Perhaps the better thing to document here is that a passphrase callback may be called each time an options object is constructed, based on what keys the caller has selected to load, even though some of those keys might not be encrypted or might not be used later for the actual authentication.

[goblin]

I think maybe the reason you saw this was because there was no host information available when it tried to resolve the set of keys to use when you constructed the options object

Yes, it was precisely for this reason. Hostname to connect to is not specified in SSHClientConnectionOptions, so it can't know which key to load at this stage.

But I wouldn't expect the SSHClientConnectionOptions to load and verify keys and passphrases, just store them and pass them on to the connection method. I could kinda do this by just storing them in a dict and expanding it to **kwargs when calling connect, and to be honest that's what I expected SSHClientConnectionOptions to do too.

Perhaps the better thing to document here is that a passphrase callback may be called each time an options object is constructed, based on what keys the caller has selected to load

Yes, something along those lines would be good, I think. To let the programmer know that this is a smart object that will attempt to load default keys, and that they might be the wrong keys if there's conditional host configuration present later.

[ronf]

But I wouldn't expect the SSHClientConnectionOptions to load and verify keys and passphrases, just store them and pass them on to the connection method. I could kinda do this by just storing them in a dict and expanding it to **kwargs when calling connect, and to be honest that's what I expected SSHClientConnectionOptions to do too.

The original version of this options support didn't support an SSH config file, and so there was no notion of conditional values. The idea was to provide feedback as early as possible to the developer when they specified config options, and to do any computation or validation only once for any given options object you created, potentially reusing this collection of config settings for many connections over time without redoing all the work to reload them on every connect. In particular, loading keys can be quite expensive, and avoiding having to reload the keys each time can provide a substantial performance improvement.

Now that the config file has to be re-evaluated every time, some of this benefit is lost, and it can also lead to issues like you saw here when one of the config file options depends on a value which is changing between requests. However, callers can still get the benefit here for whatever options they choose to pass in as keyword args instead of config file entries.

Perhaps the better thing to document here is that a passphrase callback may be called each time an options object is constructed, based on what keys the caller has selected to load

Yes, something along those lines would be good, I think. To let the programmer know that this is a smart object that will attempt to load default keys, and that they might be the wrong keys if there's conditional host configuration present later.

I'll see if I can come up with a note in the documentation to make this clearer.

In the meantime, I've added an async construct() method you can use now in place of using the constructor directly when creating your own SSHClientConnectionOptions and SSHServerConnectionOptions objects. Also, I've added support for both callables and coroutines to be usable as a passphrase argument, avoiding the need for your code to makes its own call to asyncio.run_coroutine_threadsafe(). If you are loading keys when constructing one of these options objects (either as direct arguments to connect/listen/etc. or using the new async construct() method, the load_keypairs() function will automatically detect callables and awaitables and do the right thing with them.

[goblin]

You've added the notes to documentation in 23b3bbf, and a possible improvement is tracked in #644, so there's nothing more to be done here. Closing.

Many thanks for all your help and hard work!