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

Fixes for Host key handling #87

Closed
wants to merge 6 commits into from
Closed

Fixes for Host key handling #87

wants to merge 6 commits into from

Conversation

@sunweaver
Copy link
Contributor

@sunweaver sunweaver commented Oct 12, 2012

Hi Jeff,

please review + pull the patches against Paramiko we use in Python X2Go.

In Python X2Go I monkey patch the methods that get patched by the pull request. I would be glad, if I could stop monkey patching for Paramiko >= 1.8.0.

Thanks for taking over the Paramiko project.

Mike

sunweaver added 4 commits Oct 12, 2012
…es. Also assures that the host entries in known_hosts get saved in hashed format as it is currently standard in OpenSSH.
…sly if keys from known_hosts are loaded via HostKeys.load() more than once (e.g. for refreshing the list of known hosts during runtime).
…e from RAM to disk. Avoids loss of host entries in case other SSH clients have written to the known_hosts file(s) meanwhile.
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 15, 2012

@sunweaver can you provide me with some instructions for testing/verifying your changes? A changelog entry (in NEWS -- see the latest version of that file, as of tonight, in the 1.8 branch) would be great too!

I'm also curious if you think this could be related to #67.

@sunweaver
Copy link
Contributor Author

@sunweaver sunweaver commented Oct 16, 2012

Hi Jeff,

On Mo 15 Okt 2012 09:14:37 CEST Jeff Forcier wrote:

I'm also curious if you think this could be related to #67.

giving an answer to this one only for now. About your other question,
I will have to think about a bit.

This issue (#87) is a completely different cup of tea from issue #67.
Host name hashing in known_hosts files has nothing to do with the host
key algorithm in use on individual servers.

Host name hashing simply camouflages the hostnames, ports and public
host keys in the individual host entry lines of a known_hosts file so
that it becomes more difficult to analyse known_hosts files in case
such a file (or a machine with such a file on it) gets hijacked.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 28, 2013

Hi,

Finally got back to this - thanks again! I agree it's orthogonal to #67, so no worries.

Reviewed the changes and they all seem safe enough to merge in. Will tweak + merge in a bit.

@@ -141,6 +141,8 @@ def add(self, hostname, keytype, key):
if (hostname in e.hostnames) and (e.key.get_name() == keytype):
e.key = key
return
if not hostname.startswith('|1|') and hash_hostname:

This comment has been minimized.

@bitprophet

bitprophet Apr 28, 2013
Member

I don't see hash_hostname anywhere else in the codebase (even with your changes applied). As expected, that causes the test suite (python test.py in projecy root) to fail. I don't think this particular change is required by the others so I'm backing it out for now.

Please submit a new PR if you want to fix that up & resubmit it - assume you were trying to implement a "always hash hostnames" feature as per the commit msg.

This comment has been minimized.

@sunweaver

sunweaver May 8, 2013
Author Contributor

Hi Jeff,

the provided patch had an error. I am about to provide a patch that works. Basically, hash_hostname is a kwargs in HostKeyEntry.add(..., hash_hostname=True) .

Mike

bitprophet added a commit that referenced this pull request Apr 28, 2013
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 28, 2013

Rebased on latest master for now (hard call, decided to just go with master vs release branch). Also see comment. 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants