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

libssh: OpenSSH config files can make curl ignore CURLOPT_SSH_KNOWNHOSTS #4972

Closed
fds242 opened this issue Feb 23, 2020 · 6 comments
Closed

libssh: OpenSSH config files can make curl ignore CURLOPT_SSH_KNOWNHOSTS #4972

fds242 opened this issue Feb 23, 2020 · 6 comments

Comments

@fds242
Copy link

@fds242 fds242 commented Feb 23, 2020

LibSSH is a little too smart and eager, which can result in highly unexpected behavior. By default, it goes ahead and follows any instructions in the default OpenSSH configuration file, ~/.ssh/config.
Consider, for example, ~/.ssh/config containing this directive:

UserKnownHostsFile ~/Desktop/whatever

Now you can try all you want to override the known hosts file location in libcurl, it will be summarily ignored:

curl_easy_setopt(curl, CURLOPT_SSH_KNOWNHOSTS, "im-just-gonna-ignore-this");

I posit this is rather unwelcome, unexpected behavior for the common libcurl developer, bound to cause much head-scratching as to what is going on. On the other hand, I feel reading the OpenSSH configuration files is a welcome feature for people using the command-line curl tool.
As such, I'm not entirely sure of the right solution here.

I have modified my lib/vssh/libssh.c like this:

  if(data->set.str[STRING_SSH_KNOWNHOSTS]) {
+    bool bval = 0;
+    ssh_options_set(ssh->ssh_session, SSH_OPTIONS_PROCESS_CONFIG, &bval);
    infof(data, "Known hosts: %s\n", data->set.str[STRING_SSH_KNOWNHOSTS]);
    ssh_options_set(ssh->ssh_session, SSH_OPTIONS_KNOWNHOSTS,
                    data->set.str[STRING_SSH_KNOWNHOSTS]);
  }

And that gives me back ultimate control over the known hosts location, making this libcurl developer happy, while also not entirely throwing away the OpenSSH config parsing feature in all use cases.

Still, if even this much is considered too drastic, I'd recommend adding a notice to the documentation page, something along the lines of: “When curl is built with libssh, this option will be entirely ignored if overridden by a UserKnownHostsFile directive in ~/.ssh/config“

None of these strange surprises of course if curl is compiled with libssh2 instead.

@ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Feb 24, 2020

Hello @fds242,

This happens because of the order the configuration options are set. The priority order of the settings in libssh are:

  1. Local user configurations (defaults to ~/.ssh/config)
  2. System-wide configurations (defaults to /etc/ssh/ssh_config)
  3. Application defaults (set through ssh_options_set(), if any)

The idea behind overriding the applications defaults with the global configurations is to keep the list of used algorithms up-to-date even when the application code is not frequently updated. And for the local configurations overriding the system-wide configurations is to give the user the option to easily override the system-wide defaults.

The problem here is that there are options set through curl_easy_setopt() which are expected to override even the local user configurations. The way to make it to behave as expected is to:

  1. Set libcurl defaults through ssh_options_set()
  2. Manually call ssh_options_parse_config() to parse the configuration files
    • The automatic configuration parsing will not happen if this is called. Therefore, it would be necessary to manually call the parsing for all configuration files to be considered
    • Options set in a configuration file cannot be overridden by options set in a configuration file parsed later, IOW, the first value seen is kept. As a result, the local configuration files should be parsed before the system-wide configuration files
  3. Override anything set before by calling ssh_options_set() after the configuration parsing
    • This is where options passed through curl_easy_setopt() should be set
@fds242
Copy link
Author

@fds242 fds242 commented Feb 25, 2020

That proposed new ordering sounds perfect. Would be great if it worked like that with libcurl, keeping the OpenSSH config files as a source of defaults, but still allowing the developer – and in turn, the users of that developer's software – to have the last say over settings. What's a little curious is that it already behaves like this with certain options, such as taking a User username setting from ~/.ssh/config as the default, but still allowing a username specified by the libcurl user to override that.

Only minor observation I have is that it feels unfortunate if simply calling ssh_options_parse_config with NULL for a file name is not sufficient to fully recreate all the default OpenSSH config loading logic libssh does or might do in the future, reading both the system-wide and the user config files. For example, /etc/ssh/ssh_config might be a reasonable choice on a Unix-like system, but it's probably more akin to %programdata%\ssh\ssh_config on Windows. Likewise, just determining the user's home folder can vary by OS. If libssh normally already handles all those differences, or might in the future, it would be a shame to keep duplicating that in any libssh client program that need to be able to override options.

@ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Feb 26, 2020

That proposed new ordering sounds perfect. Would be great if it worked like that with libcurl, keeping the OpenSSH config files as a source of defaults, but still allowing the developer – and in turn, the users of that developer's software – to have the last say over settings. What's a little curious is that it already behaves like this with certain options, such as taking a User username setting from ~/.ssh/config as the default, but still allowing a username specified by the libcurl user to override that.

Only minor observation I have is that it feels unfortunate if simply calling ssh_options_parse_config with NULL for a file name is not sufficient to fully recreate all the default OpenSSH config loading logic libssh does or might do in the future, reading both the system-wide and the user config files.

Actually libssh does exactly this: if NULL is provided as the file name, it parses the user configuration file followed by the global configuration file. The libssh documentation lacks this information and I'm sorry about this.

For example, /etc/ssh/ssh_config might be a reasonable choice on a Unix-like system, but it's probably more akin to %programdata%\ssh\ssh_config on Windows. Likewise, just determining the user's home folder can vary by OS. If libssh normally already handles all those differences, or might in the future, it would be a shame to keep duplicating that in any libssh client program that need to be able to override options.

The option to override the global default configuration file is provided as a libssh build configuration option. You can define GLOBAL_CLIENT_CONFIG when configuring the build to provide the system-wide default configuration file path for the client and GLOBAL_BIND_CONFIG for the server. It would look like this:

$ cmake -DGLOBAL_CLIENT_CONFIG=/path/to/client/config -DGLOBAL_BIND_CONFIG=/path/to/server/config /path/to/libssh/repo/clone

These configuration options were provided exactly to allow the responsible for building libssh (normally a package maintainer) to set the defaults for the target system.

@ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Feb 26, 2020

I forgot to mention that you will need libssh >= 0.9.0 to have such behaviour. I understand it is quite new to be adopted as the minimum requirement as it was released last year. Maybe a solution would be to parse manually the configuration files for older versions, but I'm afraid there is no way to get the right system defaults for the global configuration files.

@bagder bagder added the help wanted label Apr 2, 2020
ansasaki added a commit to ansasaki/curl that referenced this issue Apr 22, 2020
Previously, options set explicitly through command line options could be
overridden by the configuration files parsed automatically when
ssh_connect() was called.

By calling ssh_options_parse_config() explicitly, the configuration
files are parsed before setting the options, avoiding the options
override.  Once the configuration files are parsed, the automatic
configuration parsing is not executed.

Fixes curl#4972

Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
@ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Apr 22, 2020

Hello @fds242, could you please check if the PR #5283 fixes the issue?

ansasaki added a commit to ansasaki/curl that referenced this issue Apr 23, 2020
Previously, options set explicitly through command line options could be
overridden by the configuration files parsed automatically when
ssh_connect() was called.

By calling ssh_options_parse_config() explicitly, the configuration
files are parsed before setting the options, avoiding the options
override.  Once the configuration files are parsed, the automatic
configuration parsing is not executed.

Fixes curl#4972

Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
@fds242
Copy link
Author

@fds242 fds242 commented Apr 23, 2020

Thank you @ansasaki, tested & can confirm it does work as expected now! By no means an exhaustive test concerning all the options that could be affected. My original issue, overriding the known hosts file, does work now in that the curl option now gets the final say over an OpenSSH config file.

@bagder bagder closed this in 7bc709f Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.