Fix check_host_ip when no IP address is present in the known_hosts file #663

smortex · 2019-03-03T06:14:43Z

Hey!

Just tried to switch to net-ssh 5.2.0.rc1 but still encountering problems after #656.

  Host key verification failed for marvin.blogreen.org: fingerprint SHA256:xqXrTaBMBChJmjvj60uTvOsEQidW2jNOMcQexbs3tEw is unknown for "marvin.blogreen.org,88.161.59.228"

My known_host file contain these entries (no IP address):

sshed25519key-marvin,marvin.blogreen.org,marvin ssh-ed25519 PUBLIC_KEY
sshecdsakey-marvin,marvin.blogreen.org,marvin ecdsa-sha2-nistp256 PUBLIC_KEY
sshdsakey-marvin,marvin.blogreen.org,marvin ssh-dss PUBLIC_KEY

The original code fails because (for the first key, same of course with a different hostlist for the two other):

options[:check_host_ip] #=> true
entries                 #=> ["marvin.blogreen.org", "88.161.59.228"]
hostlist                #=> ["sshed25519key-marvin", "marvin.blogreen.org", "marvin"]

entries[1] is assigned to host_ip at the beginning of the block, it makes it easier to understand that we expect the resolution to have succeeded before trying to check if that IP is present in the hostlist.

A knwon_host file may contain any number of names or IP addresses valid for a given node. An entry may have multiple names but no IP address, in this case we host_ip will never be found in hostlist. Instead, first build the list of IP addresses present in hostlist. Only if this list is not empty, perfom ip host checking.

The same IP addresses may be written differently, so comparing them as string might not work as expected (e.g. the loopback address can be expressed as `::1`, `0000:0000:0000:0000:0000:0000:0000:0001`, `0:0:0:0:0:0:0:1` and a handful of other notations). Parse the addresses as IPAddr before comparison so that the same address written in two different notation still match.

Check if we can locate multiple keys matching a single host with multiple names and no IP address.

codecov-io · 2019-03-03T06:46:26Z

Codecov Report

Merging #663 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
+ Coverage   95.76%   95.77%   +<.01%     
==========================================
  Files         144      144              
  Lines        9644     9653       +9     
==========================================
+ Hits         9236     9245       +9     
  Misses        408      408

Impacted Files	Coverage Δ
test/test_known_hosts.rb	`100% <100%> (ø)`	⬆️
lib/net/ssh/known_hosts.rb	`98.9% <100%> (+0.06%)`	⬆️

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 3eb4a3e...8015be4. Read the comment docs.

mfazekas · 2019-03-04T13:56:46Z

@smortex thanks for the pr. Any reason you cannot pass check_host_ip => false ? I'm going to change this default from current true, but it will be likely 6.0, until that you need to pass :check_host_ip => false to ignore ip checking.

When check_host_ip is true i don't think openssh accepts an entry without an ip. We might want to display better errors, but i'm not sure i like a less strict check_host_ip you've implemented.

smortex · 2019-03-04T17:41:29Z

My goal is to ship https://github.com/puppetlabs/bolt in FreeBSD, which can certainly be modified to skip some checking but if we can avoid it it's the best :-)

I recently updated FreeBSD and assumed CheckHostIP which defaults to 'no' would be updated, and since I experienced no problem I assumed it was fine. I was doubly wrong:

I tried to force it to 'yes'. When connecting to a host which already has it's hostname in the system's known_hosts file, connections succeeds and a new entries is added to my user's known_host file (and a notice is produced).

If I change the IP that was added to my user's known_hosts file, a new line is added on the next connection, and the connection succeeds (with the same notice).

The same is true if everything is in the same known_hosts file.

The same is true if StrictHostKeyChecking is set to true.

So this test is definitively wrong (it should return a key since the hostname is found):

net-ssh/test/test_known_hosts.rb

Lines 51 to 55 in 3683301

    
           def test_search_for_with_hostname_and_wrong_ip_with_check_host_ip 
        
             options = { user_known_hosts_file: path("known_hosts/gitlab"), check_host_ip: true } 
        
             keys = Net::SSH::KnownHosts.search_for('gitlab.com,192.0.2.1',options) 
        
             assert_equal(0, keys.count) 
        
           end

That makes me think that CheckHostIP is only useful when you connect to a host by hostname, and then later reach it via it's IP address.

Maybe we can close this PR and:

Fix the above test so that it behaves like ssh(1) (since the hostname match, the key is accepted);
Maybe like ssh(1) add the IP address to the known_hosts file if options[:check_host_ip] is set to true.

What do you think?

mfazekas · 2019-03-04T18:26:02Z

@smortex Let me think about that.

In my testing you seems to be right if the name and matching key there with different ip openssh will add the new ip with a warning message even with strict host key checking. ssh -oCheckHostIp=true -oStrictHostKeyChecking=yes

I see the following options:

change check_host_ip to false globally
read check_host_ip from /etc/ssh_config
tweak check_host_ip so if no match we give a warning and add a new entry with the ip.

I don't see much value in check_host_ip therefor i see 3.) a bit of a wasted energy.

smortex · 2019-03-05T18:19:05Z

I am not sure to follow you, are 1, 2, 3 three different options or the three steps of a single option? The second hypothesis makes more sense to me but I might be misunderstanding…

If you are saying that doing the three steps above do not worth it, I quite agree: as far as I am concerned, CheckHostIp adds no value at all, and I would prefer the ssh(1) command be the only one in charge of updating my ssh configuration files if needed be.

In this case, I would suggest dropping completely the :check_host_ip option; removing the IP address lookup; simplifying the matching of keys by just checking if the user specified hostname / IP address is present in the known_hosts entry; and removing the bogus tests (because net-ssh currently refuse connections to host ssh would connect to without issue).

Does that look reasonable?

mfazekas · 2019-03-05T19:02:34Z

@smortex that meant 3 alternatives. I'd prefer to keep check_host_ip if someone needs it and remove it from future version.

Would 2.) work for you? Does your config has /etc/ssh_config has CheckHostIp false

smortex · 2019-03-05T20:22:16Z

Ah, sorry for the misunderstanding.

I'd prefer to keep check_host_ip if someone needs it and remove it from future version.

I am not sure to understand what you want to keep? As far as I can see, the only think that check_host_ip does is enforcing that the IP address of the remote side exist in the knwon_hosts file, which is not done by ssh. Isn't this a bug in net-ssh that should be fixed to behave like ssh(1)? Do I miss another feature of check_host_ip?

Would 2.) work for you?

The default FreeBSD ssh_config file is similar to the upstream one, with CheckHostIp default value in a comment. Tuning it would be a valid workaround in my case, disabling the extra-check done by net-ssh and not done by ssh(1), but I guess it would surprise other users.

Shouldn't CheckHostIp have the same impact in net-ssh and ssh(1)? In an ideal world, wouldn't 3 and 2 both be implemented?

This is the reason why I propose to just nuke CheckHostIp completely: remove the extra checks done by net-ssh and not have to implement 2 and 3. I hope you understand my reasoning.

donoghuc · 2019-03-05T22:39:37Z

Seems like requiring both hostname and IP in known_hosts was/is a bug in net-ssh. Given the un-hashed known_hosts entry:

bq9lexi8mzxe4k6.delivery.puppetlabs.net,10.16.124.224 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNo...

With following ssh(1) version:

cas@cas-ThinkPad-T460p:~/.ssh$ ssh -V
OpenSSH_7.2p2 Ubuntu-4ubuntu2.7, OpenSSL 1.0.2g  1 Mar 2016

Both of the following will work with ssh(1) as well as with net-ssh 5.1.0

cas@cas-ThinkPad-T460p:~/.ssh$ ssh -i ~/.ssh/id_rsa-acceptance -oStrictHostKeyChecking=yes root@10.16.124.224
cas@cas-ThinkPad-T460p:~/.ssh$ ssh -i ~/.ssh/id_rsa-acceptance -oStrictHostKeyChecking=yes root@bq9lexi8mzxe4k6.delivery.puppetlabs.net

However with known_hosts entry (missing IP address)

bq9lexi8mzxe4k6.delivery.puppetlabs.net ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNo...

works for ssh(1)

cas@cas-ThinkPad-T460p:~/working_dir/bolt$ ssh -i ~/.ssh/id_rsa-acceptance -oStrictHostKeyChecking=yes root@bq9lexi8mzxe4k6.delivery.puppetlabs.net

but not for net-ssh 5.1.0

  Host key verification failed for bq9lexi8mzxe4k6.delivery.puppetlabs.net: fingerprint SHA256:Bot3AN9lHgpNnLgre+hWLvnfQlOQCOxCfhtSzNqdog0 is unknown for "bq9lexi8mzxe4k6.delivery.puppetlabs.net,10.16.124.224"

The check_host_ip option seems to be a new feature. The ideal behavior would be to read CheckHostIP from ssh config and mirror the behavior. Specifically if the host name is known but there is not yet an IP, add a new entry to the list and always check it. So for the example above with the IP ommited from known_hosts the following

cas@cas-ThinkPad-T460p:~/.ssh$ ssh -i ~/.ssh/id_rsa-acceptance -oStrictHostKeyChecking=yes -oCheckHostIP=yes root@bq9lexi8mzxe4k6.delivery.puppetlabs.net

would result in

bq9lexi8mzxe4k6.delivery.puppetlabs.net ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNo
10.16.124.224 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNo...

I think that treating them a separate issues, the first a bug fix and the second a new feature would help the distinguish the two.

Note tested using Net::SSH::Verifiers::Always: https://github.com/puppetlabs/bolt/blob/f14edf4f5d7606cf10d5b65a9613efdd30161284/lib/bolt/transport/ssh/connection.rb#L107-L117

mfazekas · 2019-03-06T06:00:49Z

Under check_host_ip i mean the current check_host_ip: true implementation in net-ssh, which is stricter than the CheckHostIP in openssh. It's not a new feature it's been there in net-ssh for like 10 years.

I think that CheckHostIP in openssh and esp in net-ssh is not an usefull feature, and i'd like to remove it in the longer term. The current impl was in net-ssh for years without complaint therefore i don't want to remove it in a minor version. It can be considered either buggy or just stricter than OpenSSH version. This is the plan:

1.) 5.2.0 introduce check_host_ip: false flag, and set check_host_ip to false if CheckHostIP false in net-ssh configs
2.) 6.0 make check_host_ip: false the default and give deprecation warning for check_host_ip: true usage
3.) 7.0 remove check_host_ip

I don't want to tweak current check_host_ip to be more openssh as i don't see it as usefull.

donoghuc · 2019-03-06T15:25:31Z

I agree that replicating the behavior of CheckHostIP in net-ssh is not a useful feature. Thank you for clarifying that the host-key-check in 'net-ssh' is simply more strict than ssh and is not a bug but rather a slightly different implementation. I think the plan you have outlined is reasonable and appreciate the ability to modify the behavior with a check_host_ip flag. Thanks!

smortex · 2019-03-06T16:38:49Z

Under check_host_ip i mean the current check_host_ip: true implementation in net-ssh, which is stricter than the CheckHostIP in openssh. It's not a new feature it's been there in net-ssh for like 10 years.

Ah, I see! Thank you for clarifying and providing the roadmap: I was feeling confused, but this seems consistent.

I will close this issue for now, and ping the puppetlabs/bolt team to rely on check_host_ip: false as soon as a 5.2.0 is out 😉.

@mfazekas do you want me to open a new PR with a backport of d276c72 (should only be usefull when check_host_ip: true and using IPv6 addresses)?

mfazekas · 2019-03-07T07:38:11Z

@smortex can you check 5.2.0.rc2? It should set check_host_ip: false if your ssh config files (~/.ssh/config /etc/ssh_config /etc/ssh/ssh_config) has CheckHostIp no

smortex · 2019-03-07T15:31:19Z

I am afraid something is not working as expected, with the rc2, connection always succeeds even if I set Host *\n CheckHostIP yes in my ~/.ssh/config 🤔 .

smortex · 2019-03-07T15:42:37Z

Ah, forget my previous comment: I was trying to connect to a host with just it's hostname in the known_hosts file which is skipped by check_host_ip.

With the previously used hostname, I am still seeing the same thing happening: CheckHostIP no seems to not be catched and the verification fails.

donoghuc · 2019-03-07T18:43:07Z

Yeah the setting appears to be read correctly but seems to get ignored. Changing

net-ssh/lib/net/ssh/config.rb

Lines 80 to 83 in a1c7b36

    
           translated_globals = { 
        
             "checkhostip" => "check_host_ip" 
        
           }

to something like:

translated_globals = {
  "checkhostip" => "checkhostip"
}

Seems to work with

net-ssh/lib/net/ssh/config.rb

Line 212 in a1c7b36

checkhostip: :check_host_ip

but i'm not sure I follow the pattern that setting up.

mfazekas · 2019-03-07T21:09:01Z

@donoghuc sorry that translated_globals was my bad. There is rc3 which should load check_host_ip correctly - see #666

mfazekas · 2019-03-07T21:25:49Z

@smortex @donoghuc i've just released 5.2.0.rc3 can you try that?

donoghuc · 2019-03-07T21:37:30Z

@mfazekas 5.2.0.rc3 works as described. Thanks so much!

smortex · 2019-03-07T21:57:28Z

@mfazekas Yes, it does work as expected with rc3, thank you!

smortex added 5 commits March 2, 2019 19:52

Simplify check for IP address verification

111a109

entries[1] is assigned to host_ip at the beginning of the block, it makes it easier to understand that we expect the resolution to have succeeded before trying to check if that IP is present in the hostlist.

Add a test for net-ssh#663

19ef962

Check if we can locate multiple keys matching a single host with multiple names and no IP address.

Make rubocop happier

8015be4

smortex closed this Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check_host_ip when no IP address is present in the known_hosts file #663

Fix check_host_ip when no IP address is present in the known_hosts file #663

smortex commented Mar 3, 2019

codecov-io commented Mar 3, 2019

mfazekas commented Mar 4, 2019

smortex commented Mar 4, 2019

mfazekas commented Mar 4, 2019

smortex commented Mar 5, 2019

mfazekas commented Mar 5, 2019

smortex commented Mar 5, 2019

donoghuc commented Mar 5, 2019

mfazekas commented Mar 6, 2019

donoghuc commented Mar 6, 2019

smortex commented Mar 6, 2019

mfazekas commented Mar 7, 2019

smortex commented Mar 7, 2019

smortex commented Mar 7, 2019

donoghuc commented Mar 7, 2019

mfazekas commented Mar 7, 2019

mfazekas commented Mar 7, 2019

donoghuc commented Mar 7, 2019

smortex commented Mar 7, 2019

Fix check_host_ip when no IP address is present in the known_hosts file #663

Fix check_host_ip when no IP address is present in the known_hosts file #663

Conversation

smortex commented Mar 3, 2019

codecov-io commented Mar 3, 2019

Codecov Report

mfazekas commented Mar 4, 2019

smortex commented Mar 4, 2019

mfazekas commented Mar 4, 2019

smortex commented Mar 5, 2019

mfazekas commented Mar 5, 2019

smortex commented Mar 5, 2019

donoghuc commented Mar 5, 2019

mfazekas commented Mar 6, 2019

donoghuc commented Mar 6, 2019

smortex commented Mar 6, 2019

mfazekas commented Mar 7, 2019

smortex commented Mar 7, 2019

smortex commented Mar 7, 2019

donoghuc commented Mar 7, 2019

mfazekas commented Mar 7, 2019

mfazekas commented Mar 7, 2019

donoghuc commented Mar 7, 2019

smortex commented Mar 7, 2019