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

SSHClient: request host key type from known_hosts #910

Merged
merged 4 commits into from Jun 9, 2017

Conversation

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Feb 23, 2017

fixes #865

one part of an alternative set of PRs to #899

I only backported this to 2.1 because there were some changes around t.start_client() in 2.0 and 2.1 which make forward-merging have possibly tricky conflicts there. I can backport further if you would like, and won't mind the conflict resolution when forward-merging :)

@ploxiln ploxiln force-pushed the ploxiln:client_hostkey_req_backport branch from 59285b8 to c4519ec Feb 23, 2017
@coveralls
Copy link

@coveralls coveralls commented Feb 23, 2017

Coverage Status

Coverage decreased (-0.08%) to 74.203% when pulling c4519ec on ploxiln:client_hostkey_req_backport into 5061ee6 on paramiko:master.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Feb 23, 2017

Nah, at this point anything that makes backporting to 1.x hard is a good argument for "don't bother".

If someone who's stuck on 1.x (despite the security concerns, which was one reason we even did the backend switch in 2.0) is super upset they don't have such a change, they can submit their own PR :D

@bitprophet bitprophet added this to the 2.0.x only milestone Feb 23, 2017
@ploxiln ploxiln changed the base branch from master to 2.1 Apr 24, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 1, 2017

I can rebase this and resolve conflicts if you like (... #974 causes conflicts for pretty much all pull requests against 2.0+ ...)

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2017

That'd be great, thanks! (And, yes, it does, but I'd rather not let compatibility with PRs prevent necessary change. For example all of Fabric 1's PRs will get broked good once 2.0 becomes the new master... 🙃 )

@ploxiln ploxiln force-pushed the ploxiln:client_hostkey_req_backport branch 2 times, most recently from 281d248 to 5fc547b Jun 2, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 2, 2017

Rebased, resolved conflicts, and also removed a commit which tweaked a log message which I realized is not related to host keys (it's related to user auth keys).

Manually tested and verified, but I'm working on a proper automated test...

@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 2, 2017

Added proper tests for this. If I run them against the 2.1 branch (without this PR), the ones which I expect to fail do so:

test_host_key_negotiation_1 (test_client.SSHClientTest) ... /Users/pierce/scratch/fabwork/paramiko/paramiko/client.py:708: UserWarning: Unknown ssh-rsa host key for [127.0.0.1]:50456: 60733844cb5186657fdedaa22b5a57d5
  key.get_fingerprint())))
FAIL
test_host_key_negotiation_2 (test_client.SSHClientTest) ... ok
test_host_key_negotiation_3 (test_client.SSHClientTest) ... ERROR
test_host_key_negotiation_4 (test_client.SSHClientTest) ... ok
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

Now that #981 is merged, I can revise this to use the SecurityOptions api to change the order of the preferred host key types (to put the available key first). I may do very soon that if it's not too late...

@ploxiln ploxiln force-pushed the ploxiln:client_hostkey_req_backport branch from 10306da to 4fd1af0 Jun 6, 2017
ploxiln added a commit to ploxiln/paramiko-ng that referenced this pull request Jun 6, 2017
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 6, 2017

changed to use SecurityOptions, will squash down a bit if that looks good

ploxiln added 4 commits Jun 2, 2017
If we have a host keys that will be checked, we need to
negotiate for the type we have. Commonly, openssh could
have saved an ecdsa key in known_hosts, but SSHClient will
let the Transport negotiate for an rsa key.

Then it would consider a key of a non-corresponding type to be "missing".
That situation is also now a BadHostKeyException.

Before this change, a man-in-the-middle attack on the paramiko ssh
client was possible by having only a host key type which differs from
what the client has in known_hosts (and then giving any key of that type).
@ploxiln ploxiln force-pushed the ploxiln:client_hostkey_req_backport branch from 7cc1f22 to 4526052 Jun 7, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 9, 2017

Hrm, this appears to conflict with the ResourceManager removal due to the same chunk moving around, but I think it's just re-applying 2 line nukes after merging your changes, so I've done that.

@bitprophet bitprophet merged commit 4526052 into paramiko:2.1 Jun 9, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ploxiln
Copy link
Contributor Author

@ploxiln ploxiln commented Jun 10, 2017

Your merge conflict resolution looks correct - thanks!

@ploxiln ploxiln deleted the ploxiln:client_hostkey_req_backport branch Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants