[WIP] PR: Finish support for OpenSSH client config file specification (Remote Client)

[dalthviz]

Description of Changes

A preview:

Some resources that could be useful to add for more info on what config is supported by AsyncSSH: https://asyncssh.readthedocs.io/en/latest/api.html#supportedclientconfigoptions

Some other notes:

• Should other fields (username, port, password, client_keys and passpharse) be available but optional? Without those being provided I guess the only way to connect via a config file would be to set all of them in the config file. Also, something to note is that password and passphrase aren't part of the OpenSSH client config specification, so the config will need to have the IdentityFile keyword set to a key without passphrase/or to a key with a passphrase but that is being handle via ssh-agent and in a session where the passphrase has already been provided if I understand correctly (at least that seems needed from my local testingwhen trying to establish a connection via a config file 🤔).

Issue(s) Resolved

Fixes #22464

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

[ccordoba12]

Should other fields (username, port, password, client_keys and passpharse) be available but optional? Without those being provided I guess the only way to connect via a config file would be to set all of them in the config file.

Ok, in that case I think we must provide those fields so users can optionally set them. I thought they were provided by the config file only, but it is (of course) a security risk to set a password/passphrase in a plain text file.

So, please do that as part of this PR.

[ccordoba12]

@dalthviz, this still needs more work to do more validatations for the info introduced in this new page.

[ccordoba12]

Suggested change

	host,
	host,
	username,
	password,
	keyfile,
	passphrase,

Add these other widgets to perform more validations:

• If a user introduces a username, then we need to check that password is not empty, and viceversa.
• If they introduce passphrase, then keyfile can't be empty, and viceversa.
• They can't introduce username/password and keyfile/passphrase at the same time because only one method is necessary for authentication.
• We should check that none of those fields are available in the SSH config file (if possible). Otherwise, we should show a warning about it.

Please implement those validations in the validate_page method.

[dalthviz]

So thinking about these validations, I think there are a couple of cases that should work and kind of go against the validations listed:

• Providing only password when the config file provides the username
• Not providing a passphrase when the config file provides the keyfile or even when the keyfile is provided by the user inside the connections dialog but the keyfile doesn't require a passphrase.

Is there a reason to not allow such cases? Or maybe I'm not fully understanding the extra validations definitions? Also, following the config file validation item, the idea is that users should not set the info that can be provided via the connection dialog in the config file (i.e values for username and keyfile)? Or maybe the idea is to somehow add a message when this happens (values where provided via the dialog and also a value is available from the config file)? From my understanding the config file values act as a fallback to the other values so when a value is not provided (in our case no value is given via de connection dialog fields) asyncssh tries to get the missing options from the config file provided.

[ccordoba12]

So thinking about these validations, I think there are a couple of cases that should work and kind of go against the validations listed:

• Providing only password when the config file provides the username
• Not providing a passphrase when the config file provides the keyfile or even when the keyfile is provided by the user inside the connections dialog but the keyfile doesn't require a passphrase.

Is there a reason to not allow such cases?

Nop, I just didn't think about them. The validations I proposed should work when username/keyfile are not present in the config file.

Also, following the config file validation item, the idea is that users should not set the info that can be provided via the connection dialog in the config file (i.e values for username and keyfile)? Or maybe the idea is to somehow add a message when this happens (values where provided via the dialog and also a value is available from the config file)?

I think we need to parse the config file, fill those fields (i.e. username and keyfile) if they're present in it, and make them read-only (see below). Also, when the passphrase is not required in the config file, then we should disable that field too.

From my understanding the config file values act as a fallback to the other values so when a value is not provided (in our case no value is given via de connection dialog fields) asyncssh tries to get the missing options from the config file provided.

I understand that, but I think config files are distributed to people to make complex SSH connections. So, in that case we should load the info present in them to our UI and not allow users to edit it so they can perform a successful connection. If they want to change that info, then I'd say they should edit the config file (or request a new one from IT).

[dalthviz]

In the latest commit I went with an approach where the info in the config file is shown as placeholder for fields like username and keyfile. That, to me, seems like goes more in line with the concept of the config file working as a fallback. However, if that doesn't make sense to you let me know and I will go ahead with just forcing the values in the config file into the connection dialog widgets. As an example:

[dalthviz]

What do you think above the above way to make things work @ccordoba12 ?

[ccordoba12]

I think it looks good (sorry for the delay to reply back).

As far as I can see, you decided to do the validation using SSHClientConfig from asyncssh. Is that correct?

[dalthviz]

I think so, from what I remember and checking the code here, I think I used SSHClientConfig.load to parse the selected config file and validate if it was possible to parse it. Also, if the file was parsed successfully, then use any parsed values available to set placeholders for the page widgets

[ccordoba12]

Ok, no problem. We can add more validations later.

[ccordoba12]

Last comments for you @dalthviz then this should be ready.

Quick note about the merge conflicts: I moved all connection pages out of connectiondialog.py to connectionpages.py, so you'll also need to move most of your additions to that module.

[ccordoba12]

I think it looks good (sorry for the delay to reply back).

As far as I can see, you decided to do the validation using SSHClientConfig from asyncssh. Is that correct?

[ccordoba12]

Unfortunately this change introduces a scrollbar for all pages, which would require additional UI adjustments on all operating systems.

Furthermore, perhaps we should rethink having a single height for all pages to instead adjust it according to the one shown at the moment. That's because, also with this change, pages like the Connection status one end up with a lot of unnecessary blank space.

Since we're really close to the 6.1 final release, I think it's better to leave that for 6.1.1 so we can address it more carefully.

[dalthviz]

Thinking about a way to prevent this extra height as well as not always show fields that are not usable for simple use cases of a config file maybe adding an Advanced configuration collapsible section could help:

What do you think @ccordoba12 ?

[jitseniesen]

I was asked to comment on the connection configuration window.

In terms of which fields are necessary: I always use the default values for the ssh config file and the key file, so for my usage we don't need to ask for those. For simple cases, I specify the hostname and possibly the username and the port. For more complicated cases, I store everything in the ssh config file so I only specify the host.

I am also rather uncomfortable having Spyder store my password, especially the passphrase for my key file. I store important passwords in one dedicated password manager. I don't like random programs storing my passwords because security is hard. I did not actually play with the code, so maybe there is a way around having Spyder store the password.

It is confusing to have a field "Host" and a field "Host name". Also the distinction between "password" and "passphrase" is not clear.

[CAM-Gerlach]

Hey @dalthviz , I talked with @ccordoba12 and he really liked my idea and I think we should go ahead with it. Thanks!

• At least passphrase + password is required fields even with config file
• Like the expandable details section idea, but...
  • Collapse section when switching tabs to prevent scrollbar
• Right now "archivo de configuration" is broken to two lines for some reason despite having tons of horizontal space
• Big issue is currently conflation of authentication method and config file usage in the top dropdown
• Suggestion instead just add a field for config file to both authentication dropdown options + check host if config file is specified
  • This would avoid the need for the collapse section as it fits on one screen
• Check validation for config file host supports globs

[dalthviz]

I am also rather uncomfortable having Spyder store my password, especially the passphrase for my key file. I store important passwords in one dedicated password manager. I don't like random programs storing my passwords because security is hard. I did not actually play with the code, so maybe there is a way around having Spyder store the password.

Thinking about this, I believe at some point Carlos or Hendrik mentioned that it should be possible to prompt the user to type their password/passphrase when doing the authentication. Checking asyncssh documentation, indeed I think we could eventually offer an option to let the users type their password/passphrase and consume it without storing it (seems like is possible to pass a callable -maybe a function that shows a modal QMessageBox- instead of retriving the password/passphrase from the Spyder options). From the asyncssh docs (https://asyncssh.readthedocs.io/en/stable/api.html#asyncssh.SSHClientConnectionOptions):

• password (str) – (optional) The password to use for client password authentication or keyboard-interactive authentication which prompts for a password, or a callable or coroutine which returns the password to use. If this is not specified or set to None, client password authentication will not be performed.
• passphrase (str or [bytes]https://docs.python.org/3/library/stdtypes.html#bytes)) – (optional) The passphrase to use to decrypt client keys if they are encrypted, or a callable or coroutine which takes a filename as a parameter and returns the passphrase to use to decrypt that file. If not specified, only unencrypted client keys can be loaded. If the keys passed into client_keys are already loaded, this argument is ignored.

I'm unsure if an issue for that is already open but having one to address that in the future makes sense to me 👍

[dalthviz]

/show binder

[github-actions]

👈 Launch a Binder instance on this branch

[dalthviz]

Note: Although more work is needed, leaving this again as ready for review in case someone wants to leave more comments/suggestions or wants to give the current state of this PR a check