Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
+165 −68
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
3ddba69
Remote Client: Finish support for OpenSSH client config file specific…
dalthviz May 2, 2025
8e0c598
Remote Client: Improve fields tips for config file based connection page
dalthviz May 2, 2025
ed3079f
Merge branch 'master' into fixes_issue_22464
dalthviz May 5, 2025
f8957be
Remote Client: Add optional fields for config file based connection
dalthviz May 9, 2025
83965c7
Apply suggestions from code review
dalthviz May 20, 2025
2e55e08
Merge branch 'master' into fixes_issue_22464
dalthviz May 20, 2025
4625411
Remote client: Add initial extra validations using OpenSSH client con…
dalthviz May 24, 2025
89b125f
Merge branch 'master' into fixes_issue_22464
dalthviz May 30, 2025
09ebedf
Merge branch 'master' into fixes_issue_22464
dalthviz Jun 1, 2025
e45a823
Merge branch 'master' into fixes_issue_22464
dalthviz Jun 17, 2025
59ba911
Remote Client: Add more default values to connect dialog for config f…
dalthviz Jun 17, 2025
8f11ebb
Merge branch 'master' into fixes_issue_22464
dalthviz Jul 9, 2025
c9c4097
Merge branch 'master' into fixes_issue_22464
dalthviz Aug 4, 2025
2dbeabb
Apply suggestions from code review
dalthviz Sep 22, 2025
08191f0
Merge branch 'master' into fixes_issue_22464
dalthviz Sep 22, 2025
9aae452
Merge branch 'master' into fixes_issue_22464
dalthviz Oct 23, 2025
f1cd227
Remote client: Revert height change and add advanced config section
dalthviz Oct 23, 2025
033cbc9
Merge branch 'master' into fixes_issue_22464
dalthviz Oct 27, 2025
2038673
Remote client: Remove config file as authentication method. Add confi…
dalthviz Oct 31, 2025
294e628
Merge branch 'fixes_issue_22464' of https://github.com/dalthviz/spyde…
dalthviz Oct 31, 2025
ad0fbe0
Merge branch 'master' into fixes_issue_22464
dalthviz Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion spyder/plugins/remoteclient/api/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,11 @@ async def __create_new_connection(self) -> bool:
self._ssh_connection = await asyncssh.connect(
**connect_kwargs, client_factory=self.client_factory
)
except (OSError, asyncssh.Error) as e:
except (
OSError,
asyncssh.Error,
asyncssh.public_key.KeyImportError,
) as e:
self.logger.error(f"Failed to open ssh connection: {e}")
self.__emit_connection_status(
ConnectionStatus.Error,
Expand Down
31 changes: 27 additions & 4 deletions spyder/plugins/remoteclient/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,16 +213,39 @@ def load_conf(self, config_id):
if not options:
return {}

if options["client_keys"]:
if options["config"]:
# Only `host` and list with path to config file as `config`
# are required
options["config"] = [options["config"]]

# Optional configuration vslues mapping (`port`, `username`,
# `client_keys`, `passphrase` and `password`)
if options["port"] == 0:
# If 0 is set (`From file` option) ignore value
options.pop("port")

if not options["username"]:
options.pop("username")

if not options["client_keys"]:
options.pop("client_keys")
else:
options["client_keys"] = [options["client_keys"]]

passphrase = self.get_conf(f"{config_id}/passphrase", secure=True)
if passphrase:
options["passphrase"] = passphrase

password = self.get_conf(f"{config_id}/password", secure=True)
if password:
options["password"] = password
elif options["client_keys"]:
passphrase = self.get_conf(f"{config_id}/passphrase", secure=True)
options["client_keys"] = [options["client_keys"]]

# Passphrase is optional
if passphrase:
options["passphrase"] = passphrase
elif options["config"]:
# TODO: Check how this needs to be handled
pass
else:
# Password is mandatory in this case
password = self.get_conf(f"{config_id}/password", secure=True)
Expand Down
118 changes: 111 additions & 7 deletions spyder/plugins/remoteclient/widgets/connectiondialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class ValidationReasons(TypedDict):
class BaseConnectionPage(SpyderConfigPage, SpyderFontsMixin):
"""Base class to create connection pages."""

MIN_HEIGHT = 450
MIN_HEIGHT = 700
NEW_CONNECTION = False
CONF_SECTION = "remoteclient"

Expand Down Expand Up @@ -186,12 +186,10 @@ def create_connection_info_widget(self):
intro_layout.addWidget(intro_tip)

# Authentication methods
# TODO: The config file method is not implemented yet, so we need to
# disable it for now.
methods = (
(_('Password'), AuthenticationMethod.Password),
(_('Key file'), AuthenticationMethod.KeyFile),
# (_('Configuration file'), AuthenticationMethod.ConfigFile),
(_('Configuration file'), AuthenticationMethod.ConfigFile),
)

self._auth_methods = self.create_combobox(
Expand Down Expand Up @@ -426,28 +424,127 @@ def _create_configfile_subpage(self):
configfile = self.create_browsefile(
text=_("Configuration file *"),
option=f"{self.host_id}/configfile",
tip=_("File with the OpenSSH client configuration to use"),
alignment=Qt.Vertical,
status_icon=ima.icon("error"),
)

host = self.create_lineedit(
text=_("Host *"),
option=f"{self.host_id}/{AuthenticationMethod.ConfigFile}/address",
default="",
tip=_(
"This is the host (<i><b>Host</b></i> configuration keyword) "
"that encapsulates the options to use for the connection"
),
status_icon=ima.icon("error"),
)

port = self.create_spinbox(
prefix=_("Port"),
suffix="",
option=f"{self.host_id}/{AuthenticationMethod.ConfigFile}/port",
min_=0,
max_=65535,
tip=_("Introduce a port to use for this connection. "
"Set <i><b>0</b></i> (<i><b>From file</b></i> value) to use "
"the port specified in the configuration file"),
)
port.spinbox.setSpecialValueText(_("From file"))
port.spinbox.setStyleSheet("margin-left: 5px")

username = self.create_lineedit(
text=_("Username"),
option=f"{self.host_id}/{AuthenticationMethod.ConfigFile}/username",
status_icon=ima.icon("error"),
)

password = self.create_lineedit(
text=_("Password"),
option=f"{self.host_id}/password",
tip=(
_("Your password will be saved securely by Spyder")
if self.NEW_CONNECTION
else _("Your password is saved securely by Spyder")
),
status_icon=ima.icon("error"),
password=True
)

keyfile = self.create_browsefile(
text=_("Key file"),
option=f"{self.host_id}/keyfile",
alignment=Qt.Vertical,
status_icon=ima.icon("error"),
)

passphrase = self.create_lineedit(
text=_("Passphrase"),
option=f"{self.host_id}/passphrase",
tip=(
_("Your passphrase will be saved securely by Spyder")
if self.NEW_CONNECTION
else _("Your passphrase is saved securely by Spyder")
),
password=True
)

validation_label = MessageLabel(self)

# Add widgets to their required dicts
self._name_widgets[AuthenticationMethod.ConfigFile] = name
self._widgets_for_validation[AuthenticationMethod.ConfigFile] = [
name,
host,
Copy link
Member

@ccordoba12 ccordoba12 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@dalthviz dalthviz May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ccordoba12 ccordoba12 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@dalthviz dalthviz May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

info_placeholder

Copy link
Member Author

@dalthviz dalthviz Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ccordoba12 ccordoba12 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@dalthviz dalthviz Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ccordoba12 ccordoba12 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

configfile,
]
self._validation_labels[
AuthenticationMethod.ConfigFile
] = validation_label

# Layout
# Hide container widgets for host and port since we only use their
# components
host.hide()
port.hide()

# Layout for the host label
host_label_layout = QHBoxLayout()
host_label_layout.setSpacing(0)
host_label_layout.addWidget(host.label)
host_label_layout.addWidget(host.help_label)
host_label_layout.addStretch()

# Layout for the port label
port_label_layout = QHBoxLayout()
port_label_layout.setSpacing(0)
port_label_layout.addWidget(port.plabel)
port_label_layout.addWidget(port.help_label)

# host + port layout
host_layout = QGridLayout()
host_layout.setContentsMargins(0, 0, 0, 0)

host_layout.addLayout(host_label_layout, 0, 0)
host_layout.addWidget(host.textbox, 1, 0)
host_layout.addLayout(port_label_layout, 0, 1)
host_layout.addWidget(port.spinbox, 1, 1)

configfile_layout = QVBoxLayout()
configfile_layout.setContentsMargins(0, 0, 0, 0)
configfile_layout.addWidget(name)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addWidget(configfile)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addLayout(host_layout)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addWidget(username)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addWidget(password)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addWidget(keyfile)
configfile_layout.addSpacing(5 * AppStyle.MarginSize)
configfile_layout.addWidget(passphrase)
configfile_layout.addSpacing(7 * AppStyle.MarginSize)
configfile_layout.addWidget(validation_label)
configfile_layout.addStretch()
Expand Down Expand Up @@ -619,11 +716,14 @@ def save_server_info(self):
host=self.get_option(
f"{self.host_id}/{self.auth_method()}/address"
),
port=self.get_option(f"{self.host_id}/{self.auth_method()}/port"),
port=self.get_option(
f"{self.host_id}/{self.auth_method()}/port",
),
username=self.get_option(
f"{self.host_id}/{self.auth_method()}/username"
f"{self.host_id}/{self.auth_method()}/username",
default=""
),
client_keys=self.get_option(f"{self.host_id}/keyfile"),
client_keys=self.get_option(f"{self.host_id}/keyfile", default=""),
config=self.get_option(f"{self.host_id}/configfile"),
)

Expand All @@ -650,6 +750,10 @@ def remove_config_options(self):
"keyfile_login/port",
"keyfile_login/username",
"keyfile",
"configfile_login/name",
"configfile_login/address",
"configfile_login/port",
"configfile_login/username",
"configfile",
]
for option in options:
Expand Down
Loading