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

Identity file (key) not found if there is a blank at the end of config file line #499

Closed
Aestu opened this issue Mar 16, 2015 · 4 comments
Closed

Comments

@Aestu
Copy link

@Aestu Aestu commented Mar 16, 2015

Step to reproduce:

1.- Add a space at the end of IdentityFile line in ~/.ssh/config file
2.- Connect to this host using standard ssh: Everything is ok
2.- Try to connect to this host using paramiko: Error raises:

BackendException: ssh connection to root@xxxxxxx:22 failed: [Errno 2] No such file or directory: '/home/xxxxxx/.ssh/id_rsa '

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jan 8, 2016

Thought I saw a recent patch about trailing whitespace, can't find it but still looking.

Did find #375 which implies we did want this behavior but there's no actual rationale listed...

EDIT: yea, there's a test that #375 was just making sure didn't get accidentally broken:

{'host': ['*'], 'config': {'crazy': 'something dumb '}},
- walking down the blame on that line leads me back to 02e8178 ca 2006 by robey, which clearly means it's been "intended" behavior since the beginning of the codebase or near enough.

May dig farther in case there is rationale but usually code that old doesn't have a solid reason in the commit message. I honestly can't think of many reasons why trailing whitespace is something anybody wants, and as per #653's investigation, OpenSSH itself performs trailing whitespace stripping.

Pondering whether we should be super careful re: backwards compat, and fix this in the upcoming 2.0 release. But my gut says it's probably hurting many more people than it could possibly be helping, so maybe treating as a regular bug is the right approach.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jan 8, 2016

Yea I can't find the patch I was remembering so I'm probably crazy or it was posted to another tracker. @56quarters if you want to draft a PR as follows, I'd merge it:

  • Based on 1.13 branch, not master!
  • Updates the existing test re: preserved trailing whitespace to prove that it removes trailing whitespace instead
  • Makes sure the \r\n stuff still has tests & isn't broken (though I'm assuming .strip() would remove it as both chars are whitespace...). tho I dunno when that would even show up, might be a sign of codebase age heh.
@56quarters
Copy link
Contributor

@56quarters 56quarters commented Jan 8, 2016

Thanks for the guidance @bitprophet, I'll probably come up with a PR this weekend.

@56quarters
Copy link
Contributor

@56quarters 56quarters commented Jan 14, 2016

PR that fixes this: #656

Thanks!

bitprophet added a commit that referenced this issue Jan 19, 2016
Closes #499
dkhapun pushed a commit to cyberx-labs/paramiko that referenced this issue Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants