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

libssh2: add support for forcing a specific host key #4747

Closed
wants to merge 4 commits into from

Conversation

SantinoKeupp
Copy link

@SantinoKeupp SantinoKeupp commented Dec 20, 2019

Currently, curl (with libssh2) does not take keys from your known_hosts file into account when talking to a server. With this patch the known_hosts file will be searched for an entry matching the hostname and, if found, libssh2 will be told to claim this key type from the server.

... if it is present in the known_hosts file.
infof(data, "Set \"%s\" as SSH hostkey type\n", hostkey_method);
/*TODO: should we add LIBSSH2_ERROR_METHOD_NOT_SUPPORTED
* and LIBSSH2_ERROR_INVAL to libssh2_session_error_to_CURLE()
* and is this even the right function at all? */
Copy link
Member

Choose a reason for hiding this comment

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

Well, if you can think of a better CURLE code to use for those error codes than CURLE_SSH, then I think they should be added. Otherwise I think the default is fine. The error string is what's going to help users mostly here I think.

Copy link
Author

Choose a reason for hiding this comment

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

I could think of CURLE_BAD_FUNCTION_ARGUMENT, but I do not know if it would be useful at all. As you mentioned, the error string will help the most.

@bagder
Copy link
Member

bagder commented Dec 21, 2019

Don't be alarmed by the red CI build(s), that's just due to flaky/bad environments and not because of any flaw in your PR. Ignore them.

@bagder
Copy link
Member

bagder commented Dec 21, 2019

As I'm about to go offline for two weeks I won't merge this now. If nobody else does it while I'm away, I'll do it when I return.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

I think the TODOs and questions in the code should be cleared up before this is added. I'd wait until the next feature window for this. Also you might need a guard for HAVE_LIBSSH2_KNOWNHOST_API because it looks like otherwise there's no sshc->kh

Comment on lines 695 to 707
/* For non-standard ports, the name will be enclosed in */
/* square brackets, followed by a colon and the port */
if(store->name[0] == '[') {
kh_name_end = strstr(store->name, "]:");
port = atoi(kh_name_end + 2);
if(kh_name_end && (port == conn->remote_port)) {
kh_name_size = strlen(store->name) - 1 - strlen(kh_name_end);
if(strncmp(store->name + 1, conn->host.name, kh_name_size) == 0) {
found = true;
break;
}
}
}
Copy link
Author

Choose a reason for hiding this comment

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

I think this would be better preserved in libssh2 so that the libssh2_knownhost struct has an additional port member.

@SantinoKeupp SantinoKeupp requested a review from jay January 9, 2020 15:43
@SantinoKeupp
Copy link
Author

Thank you for your feedback and sorry for the late response, I have been on vacation as well. I think I clarified the TODOs and will appreciate it if you could take another look at the changes.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

minor stuff

#endif
#ifdef LIBSSH2_KNOWNHOST_KEY_ECDSA_384
static const char * const hostkey_method_ssh_ecdsa_384
= "ecdsa-sha2-nistp284";
Copy link
Member

Choose a reason for hiding this comment

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

typo


if(found) {
infof(data, "Found host %s in %s\n",
store->name, data->set.str[STRING_SSH_KNOWNHOSTS]);
Copy link
Member

Choose a reason for hiding this comment

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

style issue we align on opening parenthesis when there's enough room

      infof(data, "Did not find host %s in %s\n",
          conn->host.name, data->set.str[STRING_SSH_KNOWNHOSTS]);

should be

      infof(data, "Did not find host %s in %s\n",
            conn->host.name, data->set.str[STRING_SSH_KNOWNHOSTS]);

Copy link
Member

Choose a reason for hiding this comment

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

looks like all your infof and failf do that please fix all of them

break;
case LIBSSH2_KNOWNHOST_KEY_RSA1:
failf(data,
"Found host key type RSA1 - no clue what to do with it...\n");
Copy link
Member

Choose a reason for hiding this comment

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

Lose the "no clue". For example "Key type RSA1 is unsupported" or something like that.

infof(data, "Set \"%s\" as SSH hostkey type\n", hostkey_method);
result = libssh2_session_error_to_CURLE(
libssh2_session_method_pref(
sshc->ssh_session, LIBSSH2_METHOD_HOSTKEY, hostkey_method));
Copy link
Member

Choose a reason for hiding this comment

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

Break this up usually it's like
err = libssh2_session_last_errno(sshc->ssh_session);
if(err)
sshc->actualcode = libssh2_session_error_to_CURLE(err);

Which reminds me should you set actualcode here?

/cc @bagder

@jay jay closed this in 272282a Jan 12, 2020
@jay
Copy link
Member

jay commented Jan 12, 2020

Thanks

xquery added a commit to xquery/curl that referenced this pull request Mar 8, 2020
This fix adds a defensive check for the case where the
char *name in struct libssh2_knownhost is NULL

Fixes curl#5041
bagder pushed a commit that referenced this pull request Mar 9, 2020
This fix adds a defensive check for the case where the char *name in
struct libssh2_knownhost is NULL

Fixes #5041
Closes #5062
sthagen added a commit to sthagen/curl-curl that referenced this pull request Mar 9, 2020
sftp: fix segfault regression introduced by curl#4747
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants