ckeygen --changepass truncates private keys to zero bytes

[ltaylor.volks]

ckeygen --changepass does not work and exhibits different failing behavior for unencrypted and encrypted keys.

1. Unencrypted keys are truncated to zero bytes because the key is opened and written to in one call, which throws a TypeError (no data is written).
2. Encrypted keys don't get destroyed, but the passphrase change also fails to complete due to an uncaught EncryptedKeyError. The user is never prompted for a passphrase and old/new passphrases provided on the command line are not used

1. Unencrypted key - TypeError, nulls your private key

$ ckeygen -f /Users/blah/.ssh/id_rsa -p
Enter new passphrase (empty for no passphrase): 
Enter same passphrase again: 
Traceback (most recent call last):
  File "/Users/blah/Develop/virtualenvs/lodgeprox/bin/ckeygen", line 7, in <module>
    execfile(__file__)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/bin/conch/ckeygen", line 15, in <module>
    run()
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/scripts/ckeygen.py", line 66, in run
    changePassPhrase(options)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/scripts/ckeygen.py", line 144, in changePassPhrase
    keys.Key(key).toString(passphrase=options['newpass']))
TypeError: toString() got an unexpected keyword argument 'passphrase'

Fix:

• The call to toString needs to be fixed to provide the passphrase using the correct arg
• The private key should not be opened for writing until the key has been regenerated with the passphrase
• Needs tests

2. Encrypted key - EncryptedKeyError

$ ckeygen -f /Users/blah/.ssh/id_rsa -p
Traceback (most recent call last):
  File "/Users/blah/Develop/virtualenvs/lodgeprox/bin/ckeygen", line 7, in <module>
    execfile(__file__)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/bin/conch/ckeygen", line 15, in <module>
    run()
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/scripts/ckeygen.py", line 66, in run
    changePassPhrase(options)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/scripts/ckeygen.py", line 125, in changePassPhrase
    key = keys.Key.fromFile(options['filename']).keyObject
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 66, in fromFile
    return Class.fromString(file(filename, 'rb').read(), type, passphrase)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 95, in fromString
    return method(data, passphrase)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 218, in _fromString_PRIVATE_OPENSSH
    raise EncryptedKeyError('encrypted key with no passphrase'
twisted.conch.ssh.keys.EncryptedKeyError: encrypted key with no passphrase

Fix:

• Catch and deal with EncryptedKeyError
• If user provides old and new passphrases, use them
• Needs tests

[ltaylor.volks]

Fix ckeygen --changepass; add tests

[ltaylor.volks]

Added topfile NEWS entry

[ltaylor.volks]

Change passphrase issues fixed, tests added. Note that this patch includes cumulative changes from #5889 and #5890 as the issues are related.

However there are intermittent failures in this new twisted.conch.test.test_ckeygen.KeyGenTests.test_changePassPhrase, specifically the test for providing a bad passphrase for an encrypted key.

Single runs of the test usually pass, but continuously running it will eventually bomb with a variety of errors that seem to stem from pyasn1.codec.ber.decoder() returning 'weird stuff'. I don't know if this is a problem with pyasn1 or the data being provided to it.

trial -u twisted.conch.test.test_ckeygen.KeyGenTests.test_changePassPhrase
...
Traceback (most recent call last):
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/test/test_ckeygen.py", line 172, in test_changePassPhrase
    {'filename':filename, 'pass':'wrongpassphrase'})
twisted.trial.unittest.FailTest: <type 'exceptions.AttributeError'> raised instead of SystemExit:
 Traceback (most recent call last):
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/trial/unittest.py", line 1143, in _run
    utils.runWithWarningsSuppressed, self._getSuppress(), method)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/internet/defer.py", line 134, in maybeDeferred
    result = f(*args, **kw)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/internet/utils.py", line 191, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/test/test_ckeygen.py", line 172, in test_changePassPhrase
    {'filename':filename, 'pass':'wrongpassphrase'})
--- <exception caught here> ---
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/trial/unittest.py", line 243, in failUnlessRaises
    result = f(*args, **kwargs)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/scripts/ckeygen.py", line 132, in changePassPhrase
    options['filename'], passphrase=options['pass']).keyObject
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 67, in fromFile
    return Class.fromString(file(filename, 'rb').read(), type, passphrase)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 96, in fromString
    return method(data, passphrase)
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 264, in _fromString_PRIVATE_OPENSSH
    if len(decodedKey) == 2:  # alternate RSA key
exceptions.AttributeError: EndOfOctets instance has no attribute '__len__'

Subsequent runs produce an identical traceback except for the most recent call, which varies in 2 patterns:

1. AttributeError because the value returned from pyasn1.codec.ber.decoder() varies...sometimes it's an EndofOctets instance, sometimes a Boolean, sometimes an Integer, sometimes None
2. ValueError because the value returned from pyasn1.codec.ber.decoder() contains invalid literals...many different values seem to be returned.

1. Different types are returned from pyasn1.codec.ber.decoder()

  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 264, in _fromString_PRIVATE_OPENSSH
    if len(decodedKey) == 2:  # alternate RSA key
exceptions.AttributeError: Boolean instance has no attribute '__len__'

2. decoded value contains invalid literals

...
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 269, in _fromString_PRIVATE_OPENSSH
    n, e, d, p, q = [long(value) for value in decodedKey[1:6]]
exceptions.ValueError: invalid literal for long() with base 10: '\x87'

...
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 269, in _fromString_PRIVATE_OPENSSH
    n, e, d, p, q = [long(value) for value in decodedKey[1:6]]
exceptions.ValueError: invalid literal for long() with base 10: '\xe0'

...
  File "/Users/blah/Develop/code/3rdparty/twisted-hg/twisted/conch/ssh/keys.py", line 269, in _fromString_PRIVATE_OPENSSH
    n, e, d, p, q = [long(value) for value in decodedKey[1:6]]
exceptions.ValueError: invalid literal for long() with base 10: '+'

[ltaylor.volks]

Updated tests

[ltaylor.volks]

Revised and simplified tests. Issues described above:ltaylor.volks for previous patch have been fixed by using separate TestCases

[ltaylor.volks]

Updated patch to apply cleanly against r35757

[therve]

(In [36277]) Branching to 'ckeygen-changepass-5894'

[therve]

Thanks for the great patch, and sorry for not giving you feedback earlier. There are still about 3 sys.exit calls not covered by tests, it'd be great if you could provide those as a patch against the branch I created. Everything else looked good.

[therve]

I made the changes myself, build results here: buildbot.twistedmatrix.com/boxes-supported?branch=/branches/ckeygen-changepass-5894

[glyph]

There were some spurious looking failures and the build was a little old so I just updated it.

[tom.prince]

1. _make_getpass:
   1. The name doesn't follow coding standard.
   2. @param passphrases:?
   3. The code seems more complicated the necesarry: return iter(pass).next
2. I don't see any tests for the first case described in the ticket.
   • Relatedly, given the errors described in the ticket, I'd be inclined to want more assertions about the file contents after various cases.
3. test_fromStringErrors
   1. I think the comment # trying to decrypt an unencrypted key is backwards, which is relevant to the code changes.
4. Is there a reason

   #!
   sys.exit('Could not change passphrase: old passphrase error')
   

doesn't include the message from the exception?

5. Is there a reason for changing options['filename'] to use get?
   • Incidentally, it seems that the prompt suggests there is a default, but that doesn't seem to be implemented.

Please resubmit for review after addressing the above points.