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

PKey._write_private_key_file: pass the password along. #912

Closed
wants to merge 6 commits into from
Closed

PKey._write_private_key_file: pass the password along. #912

wants to merge 6 commits into from

Conversation

@jhgorrell
Copy link

@jhgorrell jhgorrell commented Feb 24, 2017

Pass the password along so encrypted ssh keys are written.

Also adds AES-256-CBC as that is what was used to write the keys on my system.

@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-1.0%) to 73.284% when pulling 739cbc9 on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage increased (+0.01%) to 74.292% when pulling 02b311c on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 74.264% when pulling ae1293c on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Feb 24, 2017

related: #809 #772

@jhgorrell
Copy link
Author

@jhgorrell jhgorrell commented Feb 24, 2017

Anything I can do to get this, or the other fixes, merged?

@@ -96,6 +96,8 @@ def progress(arg=None):

if options.newphrase:
phrase = getattr(options, 'newphrase')
# Turn it into bytes.
phrase=phrase.encode("utf-8")

This comment has been minimized.

@ploxiln

ploxiln Feb 24, 2017
Contributor

can be simplified to

if options.newphrase:
    phrase = options.newphrase.encode("utf-8")

... but instead, it may be better to change _write_private_key() to use the b() helper from py3compat, which encodes as utf-8 only if the argument is not already bytes.

This comment has been minimized.

@jhgorrell

jhgorrell Feb 24, 2017
Author

Of course! I put it on its own line, so I could comment on the need for it.

Also considered making the change to _write_private_key() at the same time, but figured there might be reasons I didnt know about, so didnt.

Would you like me to make the change to _write_private_key() as part of this PR, as a separate PR, or something else?

This comment has been minimized.

@ploxiln

ploxiln Feb 24, 2017
Contributor

I think the change to accept unicode passwords in _write_private_key() should be in this PR, it's related, and it's not yet too big a PR. But, I'm not authoritative here :)

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Feb 24, 2017

Please try to be patient - I think the maintainer will have time to consider this in the next week or so. With the topic coming back up, I think there will be more movement on it this time.

@jhgorrell
Copy link
Author

@jhgorrell jhgorrell commented Feb 24, 2017

Thanks for the update! I have it in my fork, so I can keep working in the meantime.

@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.9%) to 73.356% when pulling eee7874 on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.9%) to 73.356% when pulling eee7874 on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

@coveralls
Copy link

@coveralls coveralls commented Feb 24, 2017

Coverage Status

Coverage increased (+0.02%) to 74.296% when pulling 76ad840 on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

# Doesnt work on 2.6, 2.7 & pypy
# bad: not a string
# with self.assertRaises(ValueError):
# BestAvailableEncryption("not bytes")

This comment has been minimized.

@ploxiln

ploxiln Feb 25, 2017
Contributor

This commented-out test is no longer applicable.

Also, I don't think we need to unit-test a function that isn't in paramiko.

def test_keyfile_is_actually_encrypted(self):
# Read an existing encrypted private key
file_ = test_path('test_rsa_password.key')
# @todo: The decryptor takes string or bytes; Whereas encryption only takes bytes.

This comment has been minimized.

@ploxiln

ploxiln Feb 25, 2017
Contributor

no longer "todo" :)

@coveralls
Copy link

@coveralls coveralls commented Feb 25, 2017

Coverage Status

Coverage increased (+0.004%) to 74.282% when pulling ca2a15d on jhgorrell:jhgorrell/check-keys-are-encrypted into 5061ee6 on paramiko:master.

@jhgorrell
Copy link
Author

@jhgorrell jhgorrell commented Feb 25, 2017

Thanks for the review!

(Also remembered to remove the import as well.)

@jhgorrell
Copy link
Author

@jhgorrell jhgorrell commented Mar 3, 2017

I tried adding to the changelog, but the tests failed.

@bitprophet bitprophet changed the base branch from 2.0 to master Jun 7, 2017
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 7, 2017

Dunno why I try changing bases, it never works right. Back to finishing up my hand-pull. See #741

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 7, 2017

Hand-picked this back to 2.0+ and gussied it up stylewise, added changelog, etc. Thanks!!

@bitprophet bitprophet closed this Jun 7, 2017
bitprophet added a commit that referenced this pull request Jun 7, 2017
dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request 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 issues

Successfully merging this pull request may close these issues.

None yet

5 participants