Knownhost API handles comments badly

[alamaison]

OpenSSH-format known_hosts lines can include a comment after the key, e.g.:

host2.example.com,10.0.0.1 ssh-rsa AAAAB3NzsnipAfglyt5/w== comment

libssh2 treats this comment as part of the key rather than a separate entry. The most annoying consequence of this is that libssh2_knownhost_check always finds a mismatch if the key has a comment (as server hostkeys don't have comments).

Fixing this properly for the knownhost API would require an ABI change but I propose the following fix that takes care of libssh2_knownhost_check:

src/knownhost.c

@@ -327 +327 @@
             break;
         }
         if(match) {
+            char* comment = strchr(node->key, ' ');
+
             /* host name match, now compare the keys */
-            if(!strcmp(key, node->key)) {
+            if((!comment && !strcmp(key, node->key)) ||
+               (comment && !strncmp(key, node->key, comment - node->key))) {
                 /* they match! */
                 *ext = knownhost_to_external(node);
                 badkey = NULL;

This requires, however, that the key passed to libssh2_knownhost_check doesn't have a comment. Would this break anyone's code? I imagine it's unlikely as typically the key would have come from libssh2_session_hostkey.

[bagder]

Surely libssh2_knownhost_readline() can be fixed to also parse and store the comment internally, separately from the key and that won't break any ABI.

knownhost_writeline() could then be similarly adjusted to write the comment when such keys are written again.

Or did I miss something?

[alamaison]

Replying to bagder:

Surely libssh2_knownhost_readline() can be fixed to also parse and store the comment internally, separately from the key and that won't break any ABI.

libssh2_knownhost_readline eventually delegates to libssh2_knownhost_add. I suppose we could replace that with a call to an internal version of add that take a comment parameter or we could change add so that it parses the key into two pieces. But we're still left with libssh2_knownhost_get that returns the key and the comment as one.

[bagder]

Right, we would use a new version of libssh2_knownhost_add() that supports the comment part. Since we internally would store the key and comment separately, we would probably just make libssh2_knownhost_get not return any comment at all since the public struct has no such field.

Not ideal, but it would make the file parsing know comments and it could make the key comparison stuff to work. And it would be the same ABI.

[alamaison]

Replying to bagder:

Right, we would use a new version of libssh2_knownhost_add() that supports the comment part.

Do you mean adding an extra parameter to add (this wouldn't change the ABI, right?) or adding a new version of add, say libssh2_knownhost_add_with_comment that takes a separate comment parameter. I must admit I'm not entirely clear on which changes are ABI preserving and which aren't.

[bagder]

Replying to alamaison:

Replying to bagder:

Right, we would use a new version of libssh2_knownhost_add() that supports the comment part.

Do you mean adding an extra parameter to add (this wouldn't change the ABI, right?)

I'm quite sure we can't safely add arguments without changing the ABI. So I meant:

or adding a new version of add, say libssh2_knownhost_add_with_comment that
takes a separate comment parameter.

That's what I meant. And when we add that, we mark the first one as deprecated and we add a mention in the TODO to remove it completely at the next soname bump.

[alamaison]

Committed a fix for this: [9abf81de97f5496e904eb855170026dd9fd1a4b8].

It leaves most of the API unchanged but adds a new function libssh2_knowhost_addc that takes an optional comment and changes readline to use it.

The fix isn't perfect as parsing the comment is harder than it seems. ssh-rsa and ssh-dsa keys can't contain spaces but rsa1 blocks can. As we don't support checking these anyway it shouldn't matter.

[bagder]

Thanks, I'm setting this entry to "resolve as fixed" then!