Add cached KnownHosts implementation - continued... #744

donoghuc · 2020-03-01T05:13:22Z

This PR continues the work that @nicklewis did in #686 based on the suggestion from @mfazekas to rebuild cache when file changes occur out of band. There are two approaches for this in the PR. First use file mtime (originally suggested by @mfazekas) and the second to use an md5 hash to determine if a known_hosts file has changed out of band. While writing tests for the original mtime implementation I came across a case where a file write happened so close to the time the file was originally read that comparing mtimes did not detect a change.

This adds a new class Net::SSH::KnownHosts::Cached which loads the known_hosts files once, builds an index, and uses that when searching for keys for a given host. An instance of this object can be passed using the `known_hosts` option in order to avoid loading known_hosts multiple times when making multiple connections to either the same host or several hosts. This behavior is implemented in a new class because it differs from the existing implementation in the case where the content of the known_hosts file changes outside the current process. With the standard implementation, those changes will always be picked up the next time a connection is made, whereas the Cached implementation won't automatically incorporated changes made out-of-band.

donoghuc · 2020-03-01T05:14:09Z

lib/net/ssh/known_hosts/cached.rb

@@ -17,12 +24,10 @@ def search_for(host, options={})
          @hmac_lookups.each do |(hmac, salt), entry|
            digest = OpenSSL::Digest.new('sha1')
            host_hmac = OpenSSL::HMAC.digest(digest, salt, hostname)
-            entries << entry if Base64.encode64(host_hmac.chomp) == hmac
+            entries.concat(entry) if Base64.encode64(host_hmac).chomp == hmac


This was a bug whereby using << would insert an array rather than merging. Also, the newline the chomp method is meant to be used to ignore is in the base64 encoded content so it needs to be called once the content is decoded.

donoghuc · 2020-03-01T05:14:52Z

lib/net/ssh/known_hosts/cached.rb

            file.each_line do |line|
              hosts, type, key_content = line.split(' ')
-              next unless hosts || hosts.start_with?('#')


Pulls in work from #742

donoghuc · 2020-03-01T05:15:09Z

lib/net/ssh/known_hosts/cached.rb

@@ -54,7 +61,7 @@ def parse_known_hosts(source)
              hostlist.each do |host|
                entry = Entry.new(hostlist, key)
                if host.include?('*') || host.include?('?')
-                  regex = regexify_pattern(host)


I think this was just a typo.

donoghuc · 2020-03-01T05:16:29Z

lib/net/ssh/known_hosts/cached.rb

@@ -73,14 +80,17 @@ def parse_known_hosts(source)
          end
        end

-        def add(host, key)
+        def add(host, key, _options = nil)


I needed to keep support for a third positional parameter for Hostskeys#add_host_key

net-ssh/lib/net/ssh/known_hosts.rb

Line 24 in 54476fa

@known_hosts.add(@host, key, @options)

donoghuc · 2020-03-01T05:17:26Z

test/test_known_hosts_cached.rb.rb

+      assert_equal(0, keys.count)
+      f.write(File.read(path("known_hosts/github")))
+      f.close
+      # Ensure mtime is different


This is where I found issues with mtimes being so close that the file would not be re-parsed.

codecov-io · 2020-03-01T05:19:43Z

Codecov Report

Merging #744 into master will increase coverage by 0.37%.
The diff coverage is 99.55%.

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   95.58%   95.95%   +0.37%     
==========================================
  Files         153      155       +2     
  Lines        9963    10241     +278     
==========================================
+ Hits         9523     9827     +304     
+ Misses        440      414      -26

Impacted Files	Coverage Δ
test/test_known_hosts_cached.rb.rb	`100% <100%> (ø)`
lib/net/ssh/known_hosts/cached.rb	`98.68% <98.68%> (ø)`
lib/net/ssh/transport/algorithms.rb	`98% <0%> (-0.5%)`	⬇️
test/authentication/test_agent.rb	`100% <0%> (ø)`	⬆️
lib/net/ssh/test/socket.rb	`96.42% <0%> (ø)`	⬆️
...t/transport/kex/test_diffie_hellman_group1_sha1.rb	`100% <0%> (ø)`	⬆️
lib/net/ssh/test/channel.rb	`81.57% <0%> (ø)`	⬆️
lib/net/ssh/test/remote_packet.rb	`93.75% <0%> (ø)`	⬆️
test/transport/kex/test_ecdh_sha2_nistp256.rb	`100% <0%> (ø)`	⬆️
lib/net/ssh/test/kex.rb	`100% <0%> (ø)`	⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54476fa...8ae4097. Read the comment docs.

fwininger · 2020-03-01T10:24:22Z

lib/net/ssh/known_hosts/cached.rb

+          return unless File.readable?(source)
+
+          File.open(source) do |file|
+            @md5_sums[source] = Digest::MD5.hexdigest(File.read(file))


Please use SHA256 instead of MD5.

This seems like an easy change to make. For now i'd like to see what folks think about using a hash instead of mtime. If we want to move forward with hash I think using sha256 should be just fine.

Updated to used sha256

lib/net/ssh/known_hosts/cached.rb

test/test_known_hosts_cached.rb.rb

When the cache is built, store the file modification times. Only re-build the cache when the mtime does not exist or has changed since last read. Similarly with the add method, only rebuild the cache when the write is the only change since the last read.

mfazekas · 2020-03-01T21:27:06Z

lib/net/ssh/known_hosts/cached.rb

+
+        def cache_invalid?
+          (@user_files + @global_files).each do |file|
+            return true if File.readable?(file) && @md5_sums[file] != Digest::MD5.hexdigest(File.read(file))


I think it also needs to check if the file is removed, that should invalidate cache as well

Updated to invalidate on delete.

mfazekas · 2020-03-01T21:28:06Z

and the second to use an md5 hash to determine if a known_hosts file has changed out of band.

Yes i think using some hash is a great idea, much better than mtime!

Instead of tracking mtime to determine if known_hosts need to be re-parsed, track file sha256 checksums. This avoids cases where mtimes are close enough together that a change goes un-noticed (discovered writing tests). Also based on feedback also invalidate cache if known_hosts file is deleted OOB.

donoghuc · 2020-03-02T17:14:04Z

@nicklewis Did you have any scripts to test performance with this change?

donoghuc · 2020-03-08T01:26:06Z

Looked a bit at what gains there would be from using cached known_hosts. I built up a very large known hosts file with the following strategy: Take an existing known_hosts entry and save everything but the host name. Build up 100K host entries with a www.foo_[1..100k],1.2.3.4 ssh-ed25519 .... then in the last line add the actual "good" known_hosts entry.

Cached

Test script for using cached:

#!/usr/bin/env ruby

require 'net/ssh'
require 'net/ssh/known_hosts/cached'
options = {
  verify_host_key: Net::SSH::Verifiers::Always.new,
  keys: '/home/cas/.ssh/id_rsa-acceptance',
  known_hosts: Net::SSH::KnownHosts::Cached.new({})
}

(0..100).each do 
  Net::SSH.start('tasty-calibre.delivery.puppetlabs.net', 'root', options)
end

Example time:

real	0m42.614s
user	0m8.878s
sys	0m1.049s

Profile: https://gist.github.com/donoghuc/0e13a59b544935f1b87316cbbc2b4f03#file-cached_hosts-svg

Not cached

Script for non-cached:

#!/usr/bin/env ruby

require 'net/ssh'
require 'net/ssh/known_hosts/cached'
options = {
  verify_host_key: Net::SSH::Verifiers::Always.new,
  keys: '/home/cas/.ssh/id_rsa-acceptance'
}

(0..100).each do 
  Net::SSH.start('tasty-calibre.delivery.puppetlabs.net', 'root', options)
end

Time example:

real	0m55.168s
user	0m21.868s
sys	0m0.446s

Profile: https://gist.github.com/donoghuc/0e13a59b544935f1b87316cbbc2b4f03#file-non_cached_hosts-svg

I also noticed that sha256 is a bit slower than md5: Given collisions seem like they would be extremely rare could we just use md5 (or some even faster algorithm) as there are not really any security concerns, just detecting file changes?

sha256

#!/usr/bin/env ruby
require 'digest/sha2'
path = File.expand_path('~/.ssh/known_hosts')
(0..1000).each do 
  Digest::SHA256.hexdigest(File.read(path))
end

real	0m33.535s
user	0m29.791s
sys	0m3.723s

md5

#!/usr/bin/env ruby
require 'digest/md5'
path = File.expand_path('~/.ssh/known_hosts')
(0..1000).each do 
  Digest::MD5.hexdigest(File.read(path))
end

real	0m22.207s
user	0m18.901s
sys	0m3.307s

This commit ensures file paths are expanded given the defaults contain `~`. It also handles the case where *no* known host files are readable by intializsing caches to empty hashes in the init method.

logicminds · 2020-04-13T19:46:15Z

How would this work with centralized based knownhosts database implementations?

ie. https://gitlab.com/nwops/net-ssh-exthosts/-/tree/master

donoghuc commented Mar 1, 2020

View reviewed changes

fwininger requested changes Mar 1, 2020

View reviewed changes

fwininger reviewed Mar 1, 2020

View reviewed changes

lib/net/ssh/known_hosts/cached.rb Outdated Show resolved Hide resolved

test/test_known_hosts_cached.rb.rb Outdated Show resolved Hide resolved

donoghuc force-pushed the cached-known-hosts branch from 7cf78c8 to deaaee0 Compare March 1, 2020 15:11

mfazekas reviewed Mar 1, 2020

View reviewed changes

donoghuc force-pushed the cached-known-hosts branch from deaaee0 to 8ae4097 Compare March 2, 2020 17:11

(Squash me) Improvements based on perf testing

1f5124e

This commit ensures file paths are expanded given the defaults contain `~`. It also handles the case where *no* known host files are readable by intializsing caches to empty hashes in the init method.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cached KnownHosts implementation - continued... #744

Add cached KnownHosts implementation - continued... #744

donoghuc commented Mar 1, 2020

donoghuc Mar 1, 2020 •

edited

donoghuc Mar 1, 2020

donoghuc Mar 1, 2020

donoghuc Mar 1, 2020

donoghuc Mar 1, 2020

codecov-io commented Mar 1, 2020 •

edited

fwininger Mar 1, 2020

donoghuc Mar 1, 2020

donoghuc Mar 2, 2020

mfazekas Mar 1, 2020

donoghuc Mar 2, 2020

mfazekas commented Mar 1, 2020

donoghuc commented Mar 2, 2020

donoghuc commented Mar 8, 2020

logicminds commented Apr 13, 2020

Add cached KnownHosts implementation - continued... #744

Are you sure you want to change the base?

Add cached KnownHosts implementation - continued... #744

Conversation

donoghuc commented Mar 1, 2020

donoghuc Mar 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 1, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfazekas commented Mar 1, 2020

donoghuc commented Mar 2, 2020

donoghuc commented Mar 8, 2020

Cached

Not cached

sha256

md5

logicminds commented Apr 13, 2020

donoghuc Mar 1, 2020 •

edited

codecov-io commented Mar 1, 2020 •

edited