conch's 'bad host key' warning is inscrutable

[twisted-trac]

	@glyph reported
Trac ID	trac#3495
Type	enhancement
Created	2008-10-24 15:14:06Z

In many places, conch goes to a lot of trouble to look exactly like OpenSSH. It should do so here too. Compare:

$ ssh twistedmatrix.com
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@       WARNING: POSSIBLE DNS SPOOFING DETECTED!          @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
The RSA host key for twistedmatrix.com has changed,
and the key for the according IP address 66.35.39.65
is unknown. This could either mean that
DNS SPOOFING is happening or the IP address for the host
and its host key have changed at the same time.
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
Someone could be eavesdropping on you right now (man-in-the-middle attack)!
It is also possible that the RSA host key has just been changed.
The fingerprint for the RSA key sent by the remote host is
fc:c3:c0:23:02:cf:bf:ef:df:fd:c1:4f:8d:86:19:a7.
Please contact your system administrator.
Add correct host key in /home/glyph/.ssh/known_hosts to get rid of this message.
Offending key in /home/glyph/.ssh/known_hosts:1
RSA host key for twistedmatrix.com has changed and you have requested strict checking.
Host key verification failed.

to:

$ conch twistedmatrix.com
Connection to twistedmatrix.com closed.
conch: exiting with error [Failure instance: Traceback (failure with no frames): <class 'twisted.conch.error.ConchError'>: ('bad host key', 9)
]

Searchable metadata

``` trac-id__3495 3495 type__enhancement enhancement reporter__glyph glyph priority__normal normal milestone__ branch__ branch_author__ status__new new resolution__None None component__core core keywords__ time__1224861246000000 1224861246000000 changetime__1246350192000000 1246350192000000 version__None None owner__ cc__exarkun cc__glyph ```

[twisted-trac]

	@glyph set owner to z3p

Pretty please?

[twisted-trac]

	Automation removed owner

[twisted-trac]

	@exarkun commented

Oh man. The HostKeyChanged or UserRejectedKey is so far away from the code that actually does this reporting.

Here's what I found. twisted/conch/scripts/conch.py sets up the connection in doConnect. doConnect uses twisted.conch.client.connect.connect which returns a Deferred which fires when the connection's done. It errbacks if the SSH connection doesn't end cleanly. doConnect adds _doExit as an errback. _doExit ignores the failure passed to it, except to write out the str() of it in a message before shutting down the reactor. Meanwhile, twisted/conch/ssh/transport.py interacts with the host key verification code. If host key verification fails for any reason, then this code sends a disconnect with a host-key-verify-failed reason (that's the 9 you see in the ticket description, fwiw). This happens in two different places, ssh_KEX_DH_GEX_GROUP and ssh_KEX_DH_GEX_REPLY. Whatever the exception from host key verification was is discarded in either case. Later when the connection ends, the Deferred returned by connect.connect errbacks with a ConchError that just includes the code and reason passed to twisted.conch.client.direct.SSHClientTransport.sendDisconnect.

If the API for sendDisconnect were somehow widened, it might be possible to pass more details through. This might mean passing an object which includes code and reason attributes, but also potentially more information or a method for formatting itself to the user.

Also, I see that the ticket description includes the super bad host key verification error report which indicates two things:

1. the host key of the ip reached is unknown
2. there is an ip associated with the hostname used for which a host key is known

I don't see how to get both of these out of the exception raised by KnownHostsFile.verifyHostKey. HostKeyChanged can indicate that a key is known for a host and the received key does not match it, but I don't see how it can indicate that a key is known for a host, a connection was established to a different ip for that host, and the key for that ip is unknown. Am I overlooking something?

[twisted-trac]

	@glyph commented

Replying to exarkun:

Oh man. The HostKeyChanged or UserRejectedKey is so far away from the code that actually does this reporting.

Yeah. Hence the separate ticket :). I hit this in #10942 and figured that addressing it there was just getting too far out of scope.

Here's what I found. twisted/conch/scripts/conch.py sets up the connection in doConnect. doConnect uses twisted.conch.client.connect.connect which returns a Deferred which fires when the connection's done. It errbacks if the SSH connection doesn't end cleanly. doConnect adds _doExit as an errback. _doExit ignores the failure passed to it, except to write out the str() of it in a message before shutting down the reactor.

I think that _doExit, or something near it, is the right place to print this error and notify the user.

Perhaps the exception itself should be able to format this error, though, since other code may want to handle its presentation differently. At least, the formatting code should be exposed somewhere as a separate API from the print-it-and-exit.

Meanwhile, twisted/conch/ssh/transport.py interacts with the host key verification code. If host key verification fails for any reason, then this code sends a disconnect with a host-key-verify-failed reason (that's the 9 you see in the ticket description, fwiw). This happens in two different places, ssh_KEX_DH_GEX_GROUP and ssh_KEX_DH_GEX_REPLY. Whatever the exception from host key verification was is discarded in either case.

I don't really understand why these don't just propagate the exception. It would be less code, and it wouldn't destroy information that would be potentially useful to the caller.

We can probably even adjust the inheritance hierarchy to make this a completely compatible change; ConchError is not a particularly descriptive type of exception.

Later when the connection ends, the Deferred returned by connect.connect errbacks with a ConchError that just includes the code and reason passed to twisted.conch.client.direct.SSHClientTransport.sendDisconnect.

If the API for sendDisconnect were somehow widened, it might be possible to pass more details through. This might mean passing an object which includes code and reason attributes, but also potentially more information or a method for formatting itself to the user.

sendDisconnect seems pathologically narrow to me right now. An ad-hoc integer error code and opaque string? I also note that there are two different methods called sendDisconnect involved here that seem to do very subtly different things, and although their signatures are (positionally) compatible, they call the parameters different things and one of them isn't documented. I certainly wouldn't shed a tear if this API just went away (yes yes, deprecation and so on) or changed completely.

Also, I see that the ticket description includes the super bad host key verification error report which indicates two things:

1. the host key of the ip reached is unknown
2. there is an ip associated with the hostname used for which a host key is known

Right, I intentionally included the worst (i.e. most complete) form of error for reference.

I don't see how to get both of these out of the exception raised by KnownHostsFile.verifyHostKey. HostKeyChanged can indicate that a key is known for a host and the received key does not match it, but I don't see how it can indicate that a key is known for a host, a connection was established to a different ip for that host, and the key for that ip is unknown. Am I overlooking something?

The simplest solution here is that verifyHostKey (and HostKeyChanged) should simply be expanded to include this information when it's available. Perhaps we could even add a new type of exception to convey this additional information.

If adding this information is even a little bit challenging though, then I think the right thing to do is to split up the work and report the simpler case this way first, then add handling for a new, more specific type of exception later.

That does overlook the case where maybe conch wants to emit a warning about a new IP with the same key, but I think that making the warning a completely different code-path from the error is fine.