SSHKit opens two connections instead of one when using SSHKit::Backend::Netssh::KnownHosts

[mattbrictson]

This bug was first reported here, with the symptom of Capistrano prompting for the SSH password twice: capistrano/capistrano#1774

This got me thinking that perhaps the SSHKit connection pool is actually creating two connections instead of one. And in fact, this is indeed the case.

I added a puts statement to connection_pool.rb like this:

@@ -56,6 +56,7 @@ class SSHKit::Backend::ConnectionPool
   def with(connection_factory, *args)
     cache = find_cache(args)
     conn = cache.pop || begin
+      puts "*** Opening new SSH connection with args: #{args.to_s}"
       connection_factory.call(*args)
     end
     yield(conn)

And then ran cap production deploy:check. Sure enough, two connections were opened.

The interesting thing is that args.to_s generates two different results. Since args.to_s is used to generate a key for the connection pool, this is why the pool thinks it needs to create two separate connections.

to_s is changing because SSHKit::Backend::Netssh::KnownHosts has mutable state and is changing state in between the two SSH executions. You can see that in the second case, my ~/.ssh/known_hosts has now been loaded and cached by KnownHosts, which changes its to_s output.

*** Opening new SSH connection with args: ["xx.xx.xx.xx", "deployer", {:user=>"deployer", :forward_agent=>true, :known_hosts=>#<SSHKit::Backend::Netssh::KnownHosts:0x007f9b78b0abb0 @_mutex=#<Thread::Mutex:0x007f9b78b0ab10>, @files={}>, :compression=>true, :keepalive=>true}]
.
.
.
*** Opening new SSH connection with args: ["xx.xx.xx.xx", "deployer", {:user=>"deployer", :forward_agent=>true, :known_hosts=>#<SSHKit::Backend::Netssh::KnownHosts:0x007f9b78b0abb0 @_mutex=#<Thread::Mutex:0x007f9b78b0ab10>, @files={"~/.ssh/known_hosts"=>#<SSHKit::Backend::Netssh::KnownHostsKeys:0x007f9b7a979b28 @_mutex=#<Thread::Mutex:0x007f9b7a979a88>, @path="/Users/mbrictson/.ssh/known_hosts", @hosts_keys={REDACTED...

I think we need to either:

• modify KnownHosts#to_s to be a stable value; or
• change how the pool key is calculated so that it is not sensitive to KnownHosts#to_s

/cc @byroot

[byroot]

So it's pretty much identical to #289.

Since it's been twice we've been bit by this, maybe option 2 would be better.

[will-in-wi]

Is anyone working on this? I'll take a crack at it if not.

[mattbrictson]

@will-in-wi Go for it!

[mattbrictson]

I reopened #290 "Improve connection pooling cache key scheme", which seems to be the umbrella issue.

[will-in-wi]

I started looking into this, and it is tricky. The biggest issue is that we are trying to generate a cache key from arbitrary arguments. I'm going to look into where the arguments are used and whether they can be defined in such a way as makes it easier to generate a cache key. Failing that, I'll look into whether a protocol can be expressed such that the args array and hash get traversed one layer deep and #cache_key || #to_s gets called on each element. This way, any part of the args array which is causing issues can implement #cache_key and solve it for that object.

Thoughts?

[will-in-wi]

Do we want to close this as a dup of #290 or vice versa?

[mattbrictson]

I think we should keep both issues open, since we could potentially fix this with a specific workaround without solving it for all cases.

[mattbrictson]

Thoughts?

Well, the trouble is that the netssh_options that are passed in can contain any number of complex, mutable objects that affect how the connection is established. Even if we could 100% reliably choose an appropriate cache key for the net-ssh implementations of these options, some of these options are strategy objects that could have custom implementations. We cannot predict how these will affect the resulting SSH connection or what their cache keys should be.

Further, we can't take a shortcut and assume the object ID (memory address) is a decent proxy for these objects, because Capistrano makes a deep copy of the entire host and netssh_options, so the memory addresses are constantly changing.

We could take a conservative approach and calculate the cache key by inspecting only the "primitive" option values, like String, Symbol, Integer, True, False, ignoring complex options like :known_hosts and :proxy. This would probably work 99% of the time. In the rare case that this leads to a key collision (i.e. a connection is reused when it shouldn't be), then the user can disable the connection pool. The risk is that that 1% of users could see some really undesirable behavior, like a command being run on the wrong server in the worst case.

The current behavior is much less risky: the worst case is we create an extra connection.

[will-in-wi]

My gut reaction upon hearing this is that we are probably approaching this backwards. We probably shouldn't be caching an object we know nothing about and for which we cannot generate a reasonable cache key. In this case, we should probably keep track of the object at point of use, and simply not deep-copy it, instead retaining the object in an immutable state, or something like that.

This gets into deep architectural questions that I don't know much about. I am not sure I have the time to dive into the code and understand it enough to do this right. I am concerned about just patching over it, however, as this will probably result in more bugs down the road.

I'll think about it more.

[mattbrictson]

Fixed via #392 🎉