Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix host key checking #656

Merged
merged 2 commits into from
Feb 14, 2019
Merged

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Jan 29, 2019

The known_hosts file may contain keys associated with a hostname, an
ip-address, or both.

When validating a key, the net-ssh gem ensure that both the hostname and
the ip-address match beforce adding a key. Thus, if the known_hosts
file only contains one of these two pieces of information, the host key
verification fails.

Instead of adding keys when both the hostname and the ip-address match,
add them when any of these information match.

@smortex
Copy link
Contributor Author

smortex commented Jan 29, 2019

Steps to reproduce:

#!/usr/bin/env ruby

require 'net/ssh'

Net::SSH.start('example.com', 'username', :verify_host_key=>:always) do |ssh|
  puts ssh.exec!('hostname')
end

Without this patch, will only work if the known_hosts file has a line matching example.com,127.0.0.1 SSH KEY. A line example.com SSH KEY would be ignored.

@mfazekas
Copy link
Collaborator

@smortex thanks for the PR, can you please add a testcase for it?

@smortex
Copy link
Contributor Author

smortex commented Jan 30, 2019

Hi @mfazekas! My previous attempt to add tests failed because of another bug. A fresh view allowed me to spot it (b5af8a8) and add a few tests.

It also allowed me to see a difference between net-ssh and ssh(1), I detailed the issue in 855d617.

Globing is also still not handled for matching entries in the known_hosts file, but I think this can be part of another PR (only do bugfix in tis PR and add adding missing features in another one).

@smortex
Copy link
Contributor Author

smortex commented Jan 31, 2019

Just added a commit that fix the issue detailed in 855d617.

This is ready for review / merging 🎉

Please note that I have anther commit on top of this for matching hostnames against patterns that can be found in the known_host file. I plan to open another PR when this one is merged, but if you prefer, I can include the change in this branch. Just let me know!

@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #656 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
- Coverage   95.82%   95.75%   -0.08%     
==========================================
  Files         144      144              
  Lines        9515     9509       -6     
==========================================
- Hits         9118     9105      -13     
- Misses        397      404       +7
Impacted Files Coverage Δ
lib/net/ssh/transport/session.rb 95.74% <ø> (-0.18%) ⬇️
test/transport/test_session.rb 100% <100%> (ø) ⬆️
test/test_known_hosts.rb 100% <100%> (ø) ⬆️
lib/net/ssh/known_hosts.rb 98.64% <100%> (-0.04%) ⬇️
lib/net/ssh/buffered_io.rb 80.55% <0%> (-9.73%) ⬇️

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 95ec134...c31d059. Read the comment docs.

@donoghuc
Copy link
Contributor

donoghuc commented Feb 6, 2019

This seems like a really nice improvement. Would love to see it adopted.

This method is supposed to transform a relative path into an absolute
one, but ignrore the provided path and always return the full path to
the same fixture file.
The known_hosts file may contain keys associated with a hostname, an
ip-address, or both.

When validating a key, the net-ssh gem ensure that both the hostname and
the ip-address match beforce adding that key.  Thus, if the known_hosts
file only contains one of these two pieces of information, the host key
verification fails.

Instead of adding keys when both the hostname and the ip-address match,
add them when the user-supplied identification of the remote host match
an entry in the known_hosts file.  Optionaly, if `check_host_ip` is set
to true, the resolved IP address of the remote host is also checked.
@mfazekas mfazekas merged commit 2179977 into net-ssh:master Feb 14, 2019
@mfazekas
Copy link
Collaborator

@smortex thanks much for the PR, i'll probably change check_host_ip default to true in 5.1.1, and will change to false in next major version.

@donoghuc
Copy link
Contributor

@mfazekas When do you expect to release 5.1.1? Would like to use this! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants