refactor: make dvc-ssh compatible with asyncssh>=2.19.0 #100
Conversation
|
|
||
| # We are going to automatically add stuff to known_hosts | ||
| # something like paramiko's AutoAddPolicy() | ||
| login_info["known_hosts"] = None |
There was a problem hiding this comment.
This is already set in sshfs.
https://github.com/fsspec/sshfs/blob/47d78ac22daafc6c2aa2dd8de678f1413ef0dadd/sshfs/spec.py#L63
| if config.get("keyfile"): | ||
| raw_keys.append(config.get("keyfile")) | ||
| elif user_ssh_config.get("IdentityFile"): | ||
| raw_keys.extend(user_ssh_config.get("IdentityFile")) |
There was a problem hiding this comment.
If client_keys is specified, it does not use IdentityFile, nor does it try to load keys from default paths.
Same thing happens with IdentityFile, it does not load other keys from default paths.
There was a problem hiding this comment.
so, you are saying it's fine for us to change / drop the existing logic and don't try to use all the keys? (just to clarify)
There was a problem hiding this comment.
so, you are saying it's fine for us to change / drop the existing logic and don't try to use all the keys? (just to clarify)
I am only stating what is happening. In the diff, you can see that either client_keys can be a keyfile that is set in config or falls back to use IdentityFile user's ssh config. We don't use all the keys, it's either-or.
Lines 84 to 88 in cdefaf5
asyncssh already does the latter automatically if no client_keys gets passed.
So, we don't need to do that.
Regarding "not using all keys," I believe this behavior is generally preferred since it avoids the need to test every key. This aligns with asyncssh's approach, where a default set of key file paths is used if client_keys or IdentityFile is not specified. However, if either is set, only the specified keys are used, and the default keys are ignored.
There was a problem hiding this comment.
yes, okay ... that's right. It makes the migration simpler.
| @@ -108,11 +87,6 @@ def _prepare_credentials(self, **config): | |||
|
|
|||
| login_info["gss_auth"] = config.get("gss_auth", False) | |||
| login_info["agent_forwarding"] = config.get("agent_forwarding", True) | |||
| login_info["proxy_command"] = user_ssh_config.get("ProxyCommand") | |||
There was a problem hiding this comment.
We don't have a config for this in dvc. ProxyCommand is supported in asyncssh from openssh config AFAIK.
There was a problem hiding this comment.
I think this was copied straight from paramiko based filesystem.
I see that asyncssh got proxy_command support as well as OpenSSH's ProxyCommand support at the same time: ronf/asyncssh#376 (commit ronf/asyncssh@47375c4).
| for option in ("password", "passphrase"): | ||
| login_info[option] = config.get(option) |
There was a problem hiding this comment.
Note that we don't really need ask_* options, as we have support for keyboard-interactive authentication support. See #12.
But this is not a big issue either. It would be nice to also support password authentication interactively, but that's a nice to have.
| prompt = f"Enter a {desc} for" | ||
| if user: | ||
| prompt += f" {user}@{host}" | ||
| else: | ||
| prompt += f" {host}" | ||
| if port: | ||
| prompt += f":{port}" | ||
| prompt += ":\n" |
There was a problem hiding this comment.
I had to make these conditional, since we don't use the defaults anymore.
These will likely get removed when we start using a proper callback (will do in next PR).
| if keyfile := config.get("keyfile"): | ||
| login_info["client_keys"] = [os.path.expanduser(keyfile)] |
There was a problem hiding this comment.
One thing worth mentioning, if keyfile is set and passphrase/ask_passphrase is not set, DVC fails without asking for a passphrase. This is working as expected, but we do prompt for passphrase for default keyfiles or the one specified in the user's config. So, we are diverging a bit.
asyncssh can take a passphrase callable, so if we pass it, it will also get called for these client_keys. This will unify the behavior and remove the need for eager ask_passphrase callback above.
Will create a PR for this separately.
There was a problem hiding this comment.
nvmd, ignore that. passphrase callable will be called for every key, even unencrpyted ones.
No description provided.