ssh: Decouple password and passphrase.
#3
Conversation
dvc_ssh/__init__.py
Outdated
| if ( | ||
| config.get("password_as_passphrase", True) | ||
| and login_info["passphrase"] is None | ||
| ): | ||
| login_info["passphrase"] = login_info["password"] |
There was a problem hiding this comment.
Hm, do we really need this option?
There was a problem hiding this comment.
It was the cleanest way I found to don't break users relying on old behavior while still allowing users to opt-out (i.e. for fixing iterative/dvc#8137).
Felt better than alternatives:
-
Trying to come up with conditions were misusing the asyncssh API might work.
Doesn't seem scalable or right. New edge cases might show in the future. -
Completely dropping old behavior.
Would be a breaking change for users relying on it.
There was a problem hiding this comment.
Got it, thanks for elaborating! Indeed, all options feel like a bit of a compromise. Considering that providing password/passphrase in the config is a bad practice and we even considered removing support for it, we can probably just drop old behaviour for the sake of correctness.
Though, another thing to note is that ask_* options are a bit of a legacy thing coming from paramiko's limitations, and asyncssh might be raising proper exceptions that would allow us to distinguish when we need to ask password or a passphrase. If you could confirm that, maybe we can just make ask_password and password obsolete and not add new options at all.
There was a problem hiding this comment.
Sorry for the late reply
Got it, thanks for elaborating! Indeed, all options feel like a bit of a compromise. Considering that providing password/passphrase in the config is a bad practice and we even considered removing support for it, we can probably just drop old behaviour for the sake of correctness.
Dropped the old behavior and the smelly flag.
Though, another thing to note is that ask_* options are a bit of a legacy thing coming from paramiko's limitations, and asyncssh might be raising proper exceptions that would allow us to distinguish when we need to ask password or a passphrase. If you could confirm that, maybe we can just make ask_password and password obsolete and not add new options at all.
Let's merge this and consider that a follow-up. I opened an issue for that
|
related: ronf/asyncssh#426 |
|
@pmrowla WDYT about this PR, still relevant/correct? |
|
I think this PR is probably fine for now. It would be nice to get rid of the |
|
@daavoo Could you take a look at my comment above, please? #3 (comment) |
daavoo
force-pushed
the
ssh-passphrase
branch
from
582ce8e to
7b4c61e
Compare
Introduce new options `passphrase` and `ask_passphrase`. Depends on updating config_schema on `dvc` repo.
daavoo
force-pushed
the
ssh-passphrase
branch
from
7b4c61e to
6e64da2
Compare
Add
password_as_passphraseflag to indicate whether the old behavior should be preserved or not. Defaults toTrueto keep old behavior by default.Introduce new options
passphraseandask_passphrase.Depends on updating config_schema on
dvcrepo.