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

Warn on parse failure when reading known_hosts #153

Merged
merged 1 commit into from Apr 29, 2013

Conversation

@glasserc
Copy link
Contributor

@glasserc glasserc commented Mar 25, 2013

This should fix #67 in that it should be much more obvious why a server is unknown: to wit, a key is being ignored.

I wasn't sure how to pass log information from client or transport to hostkeys so I just called out to util.get_logger again. If you have a better way in mind, please let me know and I'll try to implement it.

fields = line.split(' ')
if len(fields) < 3:
# Bad number of fields
log.warn("Can't understand known_hosts line format %s"%(line,))

This comment has been minimized.

@bitprophet

bitprophet Apr 28, 2013
Member

I'd change this to be something like:

"Not enough fields found in known_hosts in line %r" % line

Slightly clearer, also just uses repr to display the string.

(If we can get the line number at this point, that'd also be great to add.)

@@ -78,6 +81,7 @@ def from_line(cls, line):
elif keytype == 'ssh-dss':
key = DSSKey(data=base64.decodestring(key))
else:
log.warn("Can't handle key type %s"%(keytype,))

This comment has been minimized.

@bitprophet

bitprophet Apr 28, 2013
Member

I'd say:

"Unable to handle key of type %s" % keytype
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 28, 2013

Thanks for this!

  • I made some nitpicky language/PEP8 suggestions, if you could incorporate those :)
  • No big deal re: using logger in this way, right now anything is better than nothing wrt this problem area (poor communication re: keys).
  • Please rebase this against the 1.10 branch, and add a changelog entry (don't forget to credit yourself!) in the NEWS file in the 1.10 section.

I'll merge once those changes are in. Thanks again!

@glasserc
Copy link
Contributor Author

@glasserc glasserc commented Apr 28, 2013

Hi, I understand the language changes, but which changes are the PEP8 ones? Unwrapping one-element tuples?

I can easily add lineno to from_line and use that to pass the line number information for logging purposes. There's only one caller of from_line, so that should be fine. Is that OK?

Ethan

@glasserc
Copy link
Contributor Author

@glasserc glasserc commented Apr 28, 2013

Ah, I think I see -- spacing around the % operator. I'm leaving the one-element tuple (PEP8 is silent on that ;) ) and pushing --force onto this branch. Thanks for the review!

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Apr 29, 2013

Yes, it was mostly the spacing. I personally think one-item tuples in interpolation is ugly and prefer only to use it when required for disambiguating things, but won't fight over it. Also, no worries on the line number thing, that works for me. Thanks again!

bitprophet added a commit that referenced this pull request Apr 29, 2013
Warn on parse failure when reading known_hosts
@bitprophet bitprophet merged commit 675d79d into paramiko:master Apr 29, 2013
1 check passed
1 check passed
@bitprophet
default The Travis build passed
Details
@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 5, 2013

N.B. even with my personal, pretty boring known_hosts, I was seeing No logger defined for paramiko.hostkeys once this change landed, because I had some line in my file that was slightly off (and because the logging module default is to spew WARN and above).

I think it should probably be an info, not a warning, or lots of folks are going to start seeing this in their previously "working fine" setups and wonder what's up. It's still logged, and so will still be visible to somebody explicitly troubleshooting their known_hosts file, IMO.

Making that change myself now.

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.

2 participants