SSH agent not used when IdentityFile is set #394
Comments
|
I think that line in the documentation for agent_path is outdated and no longer correct. That used to be the behavior, but I think that was changed when I added support for agent_identities and identities_only. The new expected behavior is that agent keys will be used even when client_keys is set, but subject to the identities_only setting. According to the docs for agent_identities (which will be indirectly filled in from client_keys/IdentityFile when not set):
Looking at the code, agent_identities is set to If client_keys is not set, it should load the keys from the default locations in your .ssh directory, and that list can then be used in combination with identities_only being set to limit which agent keys are allowed. You can also set agent_identities explicitly along with identities_only if you want to only use specific identities in the agent, and this can be done even with client_keys set to some different set of local keys. If you're not seeing it use the agent, there might be a bug in here somewhere. Which version of AsyncSSH are you using and which arguments are you setting in the connect() call and via the config file? |
|
I'm using asyncssh version 2.7.0. config file entry: code: conn, client = await asyncssh.create_connection(None, args.host, config=config_path)
error: and it succeeds, using I think this comes from setting asyncssh/asyncssh/connection.py Line 6234 in e1c3654 load_keypairs without using an agent, which will raise an error if it's encrypted asyncssh/asyncssh/connection.py Line 6255 in e1c3654 side-note: I switched from
in the docs, and I wasn't sure if ssh agent support was supposed to work with |
|
Yeah - create_connection() and connect() have the same behavior in this regard, just as create_server() and listen() do. There are other variants as well, such as connect_reverse() and listen_reverse(), but they all share the same infrastructure. While the client keys are loaded locally in the code you found, that same code also populates "agent_identities", which is then used in public_key_auth_requested() to later read keys from an agent, if it is able to connect to one. These agent keys are populated in front of the locally loaded keys, so they should be used first. Something similar is done with PKCS11 keys in that same method, if a PKCS11 provider is set. If you print out the value of self._agent_identities, is it set to anything? |
|
Regarding the line in the docs, it looks like that's a place where I missed switching references over to the new connect() function, which is now considered the "main" API function for making outbound connections. Thanks for pointing that out. There are a few other references to that and to create_server() in docs/api.rst which should probably also be updated. |
|
I added print(agent_identities, getattr(self, "agent_identities", "notfound"), getattr(self, "_agent_identities", "notfound"))to lines 6241 and 6255, right before and after the block involving |
|
Sorry, on _agent_identities, I actually meant the one on the SSHConnection object, not the connection options. A good place to print that would be in the public_key_auth_requested() method, just before the call to get_keys(). However, I think it's safe to assume in this case that it would be Passing in |
|
I stuck a print statement in Not what you asked for, but if it helps, to test the agent part I ran import asyncio
import asyncssh
async def getkeys():
async with asyncssh.connect_agent() as agent:
return await agent.get_keys()
print(asyncio.run(getkeys()))and it output |
|
Ok - I think I know what's going on here. When you explicitly specify client keys, AsyncSSH requires that you provide a passphrase if any of those keys are encrypted. If you don't specify any client keys, AsyncSSH will ignore keys in the standard locations in your .ssh directory if they are encrypted and you choose not to provide a passphrase, but it intentionally errors out if you tell it to explicitly load a key and it finds that it can't. If you set IdentitiesOnly to true in addition to IdentityFile, I think you'll find that this will work. Alternately, if you just want to load all of the keys in the agent, you could just remove the IdentityFile config entry, as it really isn't needed. Note that with IdentitiesOnly set to true, the keys specified via IdentityFile should actually be public key files, not private. The new OpenSSH private key file format allows you to use it as a public key file, but that's not true for older key formats like PKCS#1 and PKCS#8. If you have encrypted private key files in one of those formats, you MUST specify a public key file if you want IdentitiesOnly to work, since the public key can't be recovered from the older encrypted private key files without knowing the passphrase. |
|
The point of this issue is that there are configs that work with OpenSSH but not with asyncssh, and I would like to be able to use asyncssh in those scenarios without making a custom config on disk just for asyncssh. For an extreme example, with a valid key loaded into the agent and a config with an invalid IdentityFile like the following
and the connection will succeed without even a warning. For my actual use-case, OpenSSH won't attempt to decrypt an encrypted ssh key set with IdentityFile before using the agent keys (and this is a practically useful config because it will work with or without the ssh agent running). I think that asyncssh should have the same behavior (or at least have an option to make it work that way). |
|
Generally speaking, I do try to make AsyncSSH as compatible as possible with OpenSSH, particularly when it comes to the meaning of settings in the config file, since the whole point of supporting that was to allow that config to be shared between the two. Unfortunately, in this case, I made an explicit choice in AsyncSSH to try and do as much checking as possible on the input arguments before actually making the connection to the SSH server and beginning the handshake. Among other things, that includes reading all of the configured key files. It seems like the only way to achieve compatibility with OpenSSH in this case would be to defer looking at the key arguments until it comes time to use each of them. This avoids errors in reading incorrectly specified keys if an earlier key ends up working, but it's a significant change in the current behavior that I don't think I could easily limit to just handling of keys specified in a config file. Also, putting aside the compatibility with OpenSSH's current behavior for a moment, I'm not sure it would be a desirable change to not raise an error early in all cases. If I were to do this, I think I'd probably have to add a new client connection option to request delayed key loading. This would allow existing behavior to be preserved for anyone depending on that. If you set this new option, it could defer loading keys until it's about to use each one, and that would apply to both the client_keys option as well as other ways of specifying keys such as the IdentityFile config option. I think this could make the behavior similar to OpenSSH, allowing you to share a config file. If I can make it work, does that sound ok to you? There's still some trickiness here, especially give that IdentityFile is overloaded to specify agent keys in the IdentitiesOnly case. So, I would either need to still load all of those files up front in that case to know which of the SSH agent keys are allowed or try to do the filtering of those keys one at a time, which would mean changing the order I attempt to use the keys to match the order of the specified identity files rather than the order the SSH agent returns the keys. I'm willing to investigate this further if you think the proposed solution would work for you, but I can't promise I'll be able to get it working. Note that in the specific case you mention, AsyncSSH could work when an SSH agent is available, but it still won't prompt you for the passphrase of the encrypted key when it's not. So, unlike OpenSSH, it will still fail when there's no agent available or when the server doesn't accept any of the agent keys. You'd have to specify the passphrase in your call to AsyncSSH to make that case work, at which point you wouldn't need this change. |
|
I think your proposal would work well. A variant that might have an easier implementation would be to load the keys up front like it does currently, but have an option to ignore key loading errors. Do you think that would work?
This makes sense--in my code I already have a branch that will handle the case where an ssh agent isn't being used and will pass in the passphrase. I'm just trying to get the ssh agent branch to work in all cases. |
|
That's an interesting thought. What about an option to just ignore encrypted keys? That way, other errors such as specifying the wrong filename or permission issues could still be properly flagged, but errors caused by trying to load a key without a passphrase would be ignored and those keys would simply not be loaded. I already do that when loading keys from the default location. This could extend that to also apply to explicitly specified key locations. |
|
Yeah I think that would be great! |
|
Here's a first attempt - could you give it a try and see if it works for you? You just need to add the new config option diff --git a/asyncssh/connection.py b/asyncssh/connection.py
index 6f35b7c..159b768 100644
--- a/asyncssh/connection.py
+++ b/asyncssh/connection.py
@@ -5834,6 +5834,13 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
if they are encrypted. If this is not specified, only unencrypted
client keys can be loaded. If the keys passed into client_keys
are already loaded, this argument is ignored.
+ :param ignore_encrypted: (optional)
+ Whether or not to ignore encrypted keys when no passphrase is
+ provided. This is intended to allow encrypted keys specified via
+ the IdentityFile config option to be ignored if a passphrase
+ is not specified, loading only unencrypted local keys. Note
+ that encrypted keys loaded into an SSH agent can still be used
+ when this option is set.
:param agent_identities: (optional)
A list of identities used to restrict which SSH agent keys may
be used. These may be specified as byte strings in binary SSH
@@ -6067,6 +6074,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
:type client_keys: *see* :ref:`SpecifyingPrivateKeys`
:type client_certs: *see* :ref:`SpecifyingCertificates`
:type passphrase: `str`
+ :type ignore_encrypted: `bool`
:type host_based_auth: `bool`
:type public_key_auth: `bool`
:type kbdint_auth: `bool`
@@ -6129,9 +6137,10 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
server_host_key_algs=(), username=(), password=None,
client_host_keysign=(), client_host_keys=None,
client_host_certs=(), client_host=None, client_username=(),
- client_keys=(), client_certs=(), passphrase=None, gss_host=(),
- gss_kex=(), gss_auth=(), gss_delegate_creds=(),
- preferred_auth=(), disable_trivial_auth=False, agent_path=(),
+ client_keys=(), client_certs=(), passphrase=None,
+ ignore_encrypted=False, gss_host=(), gss_kex=(), gss_auth=(),
+ gss_delegate_creds=(), preferred_auth=(),
+ disable_trivial_auth=False, agent_path=(),
agent_identities=(), agent_forwarding=(), pkcs11_provider=(),
pkcs11_pin=None, command=(), subsystem=None, env=(),
send_env=(), request_pty=(), term_type=None, term_size=None,
diff --git a/asyncssh/public_key.py b/asyncssh/public_key.py
index 0d136e2..99d9e67 100644
--- a/asyncssh/public_key.py
+++ b/asyncssh/public_key.py
@@ -3071,7 +3071,8 @@ def read_certificate_list(filename):
return _decode_list(read_file(filename), _decode_certificate)
-def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
+def load_keypairs(keylist, passphrase=None, certlist=(),
+ skip_public=False, ignore_encrypted=False):
"""Load SSH private keys and optional matching certificates
This function loads a list of SSH keys and optional matching
@@ -3151,8 +3152,9 @@ def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
key, certs = import_private_key_and_certs(key, passphrase)
else:
key = import_private_key(key, passphrase)
- except KeyImportError:
- if skip_public:
+ except KeyImportError as exc:
+ if skip_public or \
+ (ignore_encrypted and str(exc).startswith('Passphrase')):
continue
raise
@@ -3223,12 +3225,11 @@ def load_default_keypairs(passphrase=None, certlist=()):
for file in _DEFAULT_KEY_FILES:
try:
file = Path('~', '.ssh', file).expanduser()
- result.extend(load_keypairs(file, passphrase, certlist))
+ result.extend(load_keypairs(file, passphrase, certlist,
+ ignore_encrypted=True))
except KeyImportError as exc:
- # Ignore encrypted default keys if a passphrase isn't provided
- # and unknown key types that might not be supported
- if (not str(exc).startswith('Passphrase') and
- str(exc) != 'Unknown OpenSSH private key algorithm'):
+ # Ignore unknown key types that might not be supported
+ if str(exc) != 'Unknown OpenSSH private key algorithm':
raise
except OSError:
pass |
|
I updated the patch to pass diff --git a/asyncssh/connection.py b/asyncssh/connection.py
index 2b35b57..d74abc3 100644
--- a/asyncssh/connection.py
+++ b/asyncssh/connection.py
@@ -5834,6 +5834,13 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
if they are encrypted. If this is not specified, only unencrypted
client keys can be loaded. If the keys passed into client_keys
are already loaded, this argument is ignored.
+ :param ignore_encrypted: (optional)
+ Whether or not to ignore encrypted keys when no passphrase is
+ provided. This is intended to allow encrypted keys specified via
+ the IdentityFile config option to be ignored if a passphrase
+ is not specified, loading only unencrypted local keys. Note
+ that encrypted keys loaded into an SSH agent can still be used
+ when this option is set.
:param agent_identities: (optional)
A list of identities used to restrict which SSH agent keys may
be used. These may be specified as byte strings in binary SSH
@@ -6067,6 +6074,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
:type client_keys: *see* :ref:`SpecifyingPrivateKeys`
:type client_certs: *see* :ref:`SpecifyingCertificates`
:type passphrase: `str`
+ :type ignore_encrypted: `bool`
:type host_based_auth: `bool`
:type public_key_auth: `bool`
:type kbdint_auth: `bool`
@@ -6129,9 +6137,10 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
server_host_key_algs=(), username=(), password=None,
client_host_keysign=(), client_host_keys=None,
client_host_certs=(), client_host=None, client_username=(),
- client_keys=(), client_certs=(), passphrase=None, gss_host=(),
- gss_kex=(), gss_auth=(), gss_delegate_creds=(),
- preferred_auth=(), disable_trivial_auth=False, agent_path=(),
+ client_keys=(), client_certs=(), passphrase=None,
+ ignore_encrypted=False, gss_host=(), gss_kex=(), gss_auth=(),
+ gss_delegate_creds=(), preferred_auth=(),
+ disable_trivial_auth=False, agent_path=(),
agent_identities=(), agent_forwarding=(), pkcs11_provider=(),
pkcs11_pin=None, command=(), subsystem=None, env=(),
send_env=(), request_pty=(), term_type=None, term_size=None,
@@ -6221,7 +6230,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
client_host_keys = load_default_host_public_keys()
else:
client_host_keys = load_keypairs(client_host_keys, passphrase,
- client_host_certs)
+ client_host_certs, ignore_encrypted=ignore_encrypted)
if client_username == ():
client_username = local_username
@@ -6295,7 +6304,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
if client_keys:
self.client_keys = load_keypairs(client_keys, passphrase,
- client_certs, identities_only)
+ client_certs, identities_only, ignore_encrypted=ignore_encrypted)
else:
if client_keys == ():
client_keys = load_default_keypairs(passphrase, client_certs)
diff --git a/asyncssh/public_key.py b/asyncssh/public_key.py
index 0d136e2..99d9e67 100644
--- a/asyncssh/public_key.py
+++ b/asyncssh/public_key.py
@@ -3071,7 +3071,8 @@ def read_certificate_list(filename):
return _decode_list(read_file(filename), _decode_certificate)
-def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
+def load_keypairs(keylist, passphrase=None, certlist=(),
+ skip_public=False, ignore_encrypted=False):
"""Load SSH private keys and optional matching certificates
This function loads a list of SSH keys and optional matching
@@ -3151,8 +3152,9 @@ def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
key, certs = import_private_key_and_certs(key, passphrase)
else:
key = import_private_key(key, passphrase)
- except KeyImportError:
- if skip_public:
+ except KeyImportError as exc:
+ if skip_public or \
+ (ignore_encrypted and str(exc).startswith('Passphrase')):
continue
raise
@@ -3223,12 +3225,11 @@ def load_default_keypairs(passphrase=None, certlist=()):
for file in _DEFAULT_KEY_FILES:
try:
file = Path('~', '.ssh', file).expanduser()
- result.extend(load_keypairs(file, passphrase, certlist))
+ result.extend(load_keypairs(file, passphrase, certlist,
+ ignore_encrypted=True))
except KeyImportError as exc:
- # Ignore encrypted default keys if a passphrase isn't provided
- # and unknown key types that might not be supported
- if (not str(exc).startswith('Passphrase') and
- str(exc) != 'Unknown OpenSSH private key algorithm'):
+ # Ignore unknown key types that might not be supported
+ if str(exc) != 'Unknown OpenSSH private key algorithm':
raise
except OSError:
passThis works great for me (I asked someone else to test that it works for them too, and I'm waiting to hear back). Thank you very much for your time in understanding and creating a solution for this! |
|
Thanks for trying it out. Do you need the call on client_host_keys? I intentionally left this argument out there (which is used for "hostbased" auth), as those can't come from an SSH agent, so you pretty much need to either load the private keys directly for that feature to work or you need to use the "client_host_keysign" option, which acts a bit like an SSH agent for that case but always calls load_public_keys() or load_default_host_public_keys() so there's no overloading like there is with client_keys/IdentityFile. That said, I see the diff I sent you was missing one section. I thought I fixed that shortly after posting it here, but here's what I actually tested and intended to send: diff --git a/asyncssh/connection.py b/asyncssh/connection.py
index 6f35b7c..265c253 100644
--- a/asyncssh/connection.py
+++ b/asyncssh/connection.py
@@ -5834,6 +5834,13 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
if they are encrypted. If this is not specified, only unencrypted
client keys can be loaded. If the keys passed into client_keys
are already loaded, this argument is ignored.
+ :param ignore_encrypted: (optional)
+ Whether or not to ignore encrypted keys when no passphrase is
+ provided. This is intended to allow encrypted keys specified via
+ the IdentityFile config option to be ignored if a passphrase
+ is not specified, loading only unencrypted local keys. Note
+ that encrypted keys loaded into an SSH agent can still be used
+ when this option is set.
:param agent_identities: (optional)
A list of identities used to restrict which SSH agent keys may
be used. These may be specified as byte strings in binary SSH
@@ -6067,6 +6074,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
:type client_keys: *see* :ref:`SpecifyingPrivateKeys`
:type client_certs: *see* :ref:`SpecifyingCertificates`
:type passphrase: `str`
+ :type ignore_encrypted: `bool`
:type host_based_auth: `bool`
:type public_key_auth: `bool`
:type kbdint_auth: `bool`
@@ -6129,9 +6137,10 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
server_host_key_algs=(), username=(), password=None,
client_host_keysign=(), client_host_keys=None,
client_host_certs=(), client_host=None, client_username=(),
- client_keys=(), client_certs=(), passphrase=None, gss_host=(),
- gss_kex=(), gss_auth=(), gss_delegate_creds=(),
- preferred_auth=(), disable_trivial_auth=False, agent_path=(),
+ client_keys=(), client_certs=(), passphrase=None,
+ ignore_encrypted=False, gss_host=(), gss_kex=(), gss_auth=(),
+ gss_delegate_creds=(), preferred_auth=(),
+ disable_trivial_auth=False, agent_path=(),
agent_identities=(), agent_forwarding=(), pkcs11_provider=(),
pkcs11_pin=None, command=(), subsystem=None, env=(),
send_env=(), request_pty=(), term_type=None, term_size=None,
@@ -6295,7 +6304,8 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
if client_keys:
self.client_keys = load_keypairs(client_keys, passphrase,
- client_certs, identities_only)
+ client_certs, identities_only,
+ ignore_encrypted)
else:
if client_keys == ():
client_keys = load_default_keypairs(passphrase, client_certs)
diff --git a/asyncssh/public_key.py b/asyncssh/public_key.py
index 0d136e2..99d9e67 100644
--- a/asyncssh/public_key.py
+++ b/asyncssh/public_key.py
@@ -3071,7 +3071,8 @@ def read_certificate_list(filename):
return _decode_list(read_file(filename), _decode_certificate)
-def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
+def load_keypairs(keylist, passphrase=None, certlist=(),
+ skip_public=False, ignore_encrypted=False):
"""Load SSH private keys and optional matching certificates
This function loads a list of SSH keys and optional matching
@@ -3151,8 +3152,9 @@ def load_keypairs(keylist, passphrase=None, certlist=(), skip_public=False):
key, certs = import_private_key_and_certs(key, passphrase)
else:
key = import_private_key(key, passphrase)
- except KeyImportError:
- if skip_public:
+ except KeyImportError as exc:
+ if skip_public or \
+ (ignore_encrypted and str(exc).startswith('Passphrase')):
continue
raise
@@ -3223,12 +3225,11 @@ def load_default_keypairs(passphrase=None, certlist=()):
for file in _DEFAULT_KEY_FILES:
try:
file = Path('~', '.ssh', file).expanduser()
- result.extend(load_keypairs(file, passphrase, certlist))
+ result.extend(load_keypairs(file, passphrase, certlist,
+ ignore_encrypted=True))
except KeyImportError as exc:
- # Ignore encrypted default keys if a passphrase isn't provided
- # and unknown key types that might not be supported
- if (not str(exc).startswith('Passphrase') and
- str(exc) != 'Unknown OpenSSH private key algorithm'):
+ # Ignore unknown key types that might not be supported
+ if str(exc) != 'Unknown OpenSSH private key algorithm':
raise
except OSError:
pass |
|
You're right, only the second |
|
Thanks for the confirmation. This change is now available as commit 79abdab in the "develop" branch. As part of the final version of this change, I removed the handling of the remaining KeyImportError in load_default_keypairs() by instead checking if the key type is supported or not before attempting to load the corresponding file. So, if the necessary modules are not available to enable FIDO "sk" keys or Ed25519/Ed448 keys, the system will automatically skip trying to load the corresponding files. |
|
Thank you @ronf ! We're seeing the same Traceback when using the ssh-agent as well: https://tracker.ceph.com/issues/52516 Looking forward! |
|
This change is now available in AsyncSSH version 2.7.1. Let me know if you run into any problems with it. |
When an IdentityFile is set in an ssh config file, asyncssh will only try using that key without contacting the SSH agent. If the key is encrypted, this will fail (assuming passphrase isn't passed), even if using the agent would succeed.
According to the docs, "If client_keys is specified or this argument is explicitly set to None, an ssh-agent will not be used." Currently, this should also include "or if an IdentityFile is set". And it would be helpful if that was added to the ssh agent section too.
Personally, I think that the ssh agent should be used even if client keys are specified or an Identityfile is set. OpenSSH will use ssh agents instead of an IdentityFile or even
-ipassed to it.The text was updated successfully, but these errors were encountered: