Skip to content
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.

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

Allow processing of a specific configuration file for the connection #479

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

donnerhacke
Copy link

SUMMARY

Depending on the destination, libssh might need to be configured in a way, that the algorithms and methods need to be adjusted. Instead of adding each possible configuration option into the wrapper, including a config file (as it exists for ordinary OpenSSH) is much easier. This allows ansible to connect to devices with less well supported algorithms.

This is a required step to fix connection issues with ansible.netcommon.libssh. The calling ansible.netcommon.libssh need to provide the new optional argument config_file. But this is a separate patch, which needs to be applied there.

ISSUE TYPE
  • Feature Pull Request
ADDITIONAL INFORMATION

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/ansible-pylibssh-479
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 4, 2023
@donnerhacke donnerhacke force-pushed the config-file branch 2 times, most recently from 5eb8b7d to dcc5784 Compare January 4, 2023 16:19
@donnerhacke
Copy link
Author

Link to the patch on the other side: ansible-collections/ansible.netcommon#498

@webknjaz
Copy link
Member

@donnerhacke it would be really helpful to have test for this. Currently, there's no coverage for the new lines, which is... suboptimal.

lutz and others added 5 commits January 26, 2024 10:05
Depending on the destination, libssh might need to be configured in a
way, that the algorithms and methods need to be adjusted. Instead of
adding each possible configuration option into the wrapper, including
a config file (as it exists for ordinary OpenSSH) is much easier. This
allows ansible to connect to devices with less well supported
algorithms.
file_name = kwargs['config_file']
rc = libssh.ssh_options_parse_config(self._libssh_session, file_name.encode())
if rc != libssh.SSH_OK or self._get_session_error_str() != "":
libssh.ssh_disconnect(self._libssh_session)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the disconnect here is not needed as we did not connect yet.

@@ -243,6 +243,16 @@ cdef class Session(object):
if (key in OPTS_MAP or key in OPTS_DIR_MAP) and (kwargs[key] is not None):
self.set_ssh_options(key, kwargs[key])

if 'config_file' in kwargs:
Copy link
Contributor

Choose a reason for hiding this comment

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

This new key needs to be documented in the doc text above with the description of how the corner cases are handled.

The libssh itself reads the OpenSSH configuration files by default and the openssh configuration files have awkward precedence, so this documentation needs to clarify if this configuration file is parsed before the openssh ones, after them and if the same option is specified in both config and direct option, which one is effective.

@donnerhacke Please, let me know if you plan to get back to make this working or you abandoned this PR in favor of different solution (such as using the OpenSSH config directly).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants