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? */

This comment has been minimized.

@bagder

bagder Dec 20, 2019
Member

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.

This comment has been minimized.

@SantinoKeupp

SantinoKeupp Dec 20, 2019
Author

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 bagder added the SCP/SFTP label Dec 20, 2019
@bagder
Copy link
Member

@bagder 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
bagder approved these changes Dec 21, 2019
@bagder
Copy link
Member

@bagder 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

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

SantinoKeupp added 2 commits Jan 8, 2020
…ownhost_key_type().
…_hosts file to ssh_force_knownhosts_key_type()
/* 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;
}
}
}
Comment on lines 695 to 707

This comment has been minimized.

@SantinoKeupp

SantinoKeupp Jan 8, 2020
Author

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 Jan 9, 2020
@SantinoKeupp
Copy link
Author

@SantinoKeupp SantinoKeupp commented Jan 9, 2020

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

minor stuff

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

This comment has been minimized.

@jay

jay Jan 9, 2020
Member

typo


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

This comment has been minimized.

@jay

jay Jan 9, 2020
Member

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]);

This comment has been minimized.

@jay

jay Jan 9, 2020
Member

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");

This comment has been minimized.

@jay

jay Jan 9, 2020
Member

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

This comment has been minimized.

@jay

jay Jan 9, 2020
Member

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 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 added 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 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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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