Ticket #5894 defect closed fixed

Opened 20 months ago

Last modified 10 months ago

ckeygen --changepass truncates private keys to zero bytes

Reported by: ltaylor.volks Owned by: therve
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/ckeygen-changepass-5894
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

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

Attachments

5894-ckeygen.diff Download (21.8 KB) - added by ltaylor.volks 20 months ago.
Fix ckeygen --changepass; add tests
5894.bugfix Download (143 bytes) - added by ltaylor.volks 20 months ago.
Added topfile NEWS entry
5894-ckeygen.1.diff Download (12.7 KB) - added by ltaylor.volks 20 months ago.
Updated tests
5894-ckeygen.2.diff Download (11.1 KB) - added by ltaylor.volks 19 months ago.
Updated patch to apply cleanly against r35757

Change History

1

  Changed 20 months ago by DefaultCC Plugin

  • cc z3p added

Changed 20 months ago by ltaylor.volks

Fix ckeygen --changepass; add tests

Changed 20 months ago by ltaylor.volks

Added topfile NEWS entry

2

follow-up: ↓ 3   Changed 20 months ago by ltaylor.volks

  • keywords review added

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: '+'

Changed 20 months ago by ltaylor.volks

Updated tests

3

in reply to: ↑ 2   Changed 20 months ago by ltaylor.volks

  • keywords easy added

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

Changed 19 months ago by ltaylor.volks

Updated patch to apply cleanly against r35757

4

  Changed 18 months ago by therve

  • owner set to therve

5

  Changed 18 months ago by therve

  • branch set to branches/ckeygen-changepass-5894
  • branch_author set to therve

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

6

  Changed 18 months ago by therve

  • owner changed from therve to ltaylor.volks
  • keywords review easy removed

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.

7

  Changed 12 months ago by therve

  • owner ltaylor.volks deleted
  • keywords review added

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

8

  Changed 11 months ago by glyph

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

9

follow-up: ↓ 10   Changed 10 months ago by tom.prince

  • owner set to therve
  • keywords review removed
  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?

  1. 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.

10

in reply to: ↑ 9   Changed 10 months ago by therve

  • keywords review added
  • owner therve deleted

Replying to tom.prince:

1. _make_getpass: 1. The name doesn't follow coding standard.

I changed it, but it's somewhat borderline here, as it's a prefixed method name.

2. @param passphrases:?

Documented.

3. The code seems more complicated the necesarry: return iter(pass).next

I don't think that works.

2. I don't see any tests for the first case described in the ticket.

I'm pretty sure it's test_changePassphraseCreateError

- Relatedly, given the errors described in the ticket, I'd be inclined to want more assertions about the file contents after various cases.

Added some.

3. test_fromStringErrors 1. I think the comment # trying to decrypt an unencrypted key is backwards, which is relevant to the code changes.

Somewhat, changed.

4. Is there a reason {{{ #! sys.exit('Could not change passphrase: old passphrase error') }}} doesn't include the message from the exception?

The error message can be pretty cryptic, I think the message here is a bit better.

5. Is there a reason for changing options['filename'] to use get?

Probably not, reverted.

- Incidentally, it seems that the prompt suggests there is a default, but that doesn't seem to be implemented.

Right we can open a bug about that.

Please resubmit for review after addressing the above points.

Thanks!

11

follow-up: ↓ 12   Changed 10 months ago by tom.prince

  • keywords review removed
  • owner set to therve

3. The code seems more complicated the necesarry: return iter(pass).next

I don't think that works

How about:

    passphrases = iter(passphrases)
    def fakeGetpass(_):
        return passphrases.next()
    return fakeGetpass

Using a generator and .send seems too clever.

Applying the following patch doesn't cause any tests to fail:

  • twisted/conch/scripts/ckeygen.py

     
    160160        sys.exit('Could not change passphrase: %s' % (e,)) 
    161161 
    162162    fd = open(options['filename'], 'w') 
    163     fd.write(newkeydata) 
     163    fd.write('') 
    164164    fd.close() 
    165165 
    166166    print 'Your identification has been saved with the new passphrase.' 

- Incidentally, it seems that the prompt suggests there is a default, but that doesn't seem to be implemented.

Right we can open a bug about that.

Sure.

Please merge after addressing the above three points.

12

in reply to: ↑ 11   Changed 10 months ago by ltaylor.volks

Replying to tom.prince:

3. The code seems more complicated the necesarry: return iter(pass).next

I don't think that works

How about:

    passphrases = iter(passphrases)
    def fakeGetpass(_):
        return passphrases.next()
    return fakeGetpass

Using a generator and .send seems too clever.

Agreed. Your solution is simpler and works.

Applying the following patch doesn't cause any tests to fail:

  • twisted/conch/scripts/ckeygen.py

     
    160160        sys.exit('Could not change passphrase: %s' % (e,)) 
    161161 
    162162    fd = open(options['filename'], 'w') 
    163     fd.write(newkeydata) 
     163    fd.write('') 
    164164    fd.close() 
    165165 
    166166    print 'Your identification has been saved with the new passphrase.' 

By the time newkeydata is written it has already successfully survived roundtrip generation as a string from a Key object and converted to a valid Key object. More specifically, if newkeydata is an empty string (returned from toString), the subsequent call to fromString will raise BadKeyError and ckeygen will exit. This is covered by twisted.conch.test.test_keys.KeyTestCase.test_fromStringErrors

Here's a patch to address the review comments. I added a test to address the newkeydata concern (test_changePassphraseEmptyStringError), but I don't really think it's necessary.

therve: Thanks for picking this up and running with it (and thanks for the reviews tom.prince)!

  • twisted/conch/test/test_ckeygen.py

     
    3434 
    3535    @param passphrases: The list of passphrases returned, one per each call. 
    3636    """ 
    37     def _getpass(): 
    38         yield None 
    39         for phrase in passphrases: 
    40             yield phrase 
    41     getpassResult = _getpass() 
    42     next(getpassResult) 
    43     return getpassResult.send 
     37    passphrases = iter(passphrases) 
     38    def fakeGetpass(_): 
     39        return passphrases.next() 
     40    return fakeGetpass 
    4441 
    4542 
    4643 
     
    284281        self.assertEqual(privateRSA_openssh, FilePath(filename).getContent()) 
    285282 
    286283 
     284    def test_changePassphraseEmptyStringError(self): 
     285        """ 
     286        L{changePassPhrase} doesn't modify the key file if toString 
     287        returns an empty string 
     288        """ 
     289        filename = self.mktemp() 
     290        FilePath(filename).setContent(privateRSA_openssh) 
     291 
     292        def toString(*args, **kwargs): 
     293            return '' 
     294 
     295        self.patch(Key, 'toString', toString) 
     296 
     297        error = self.assertRaises( 
     298            SystemExit, changePassPhrase, 
     299            {'filename': filename, 
     300             'newpass': 'newencrypt'}) 
     301 
     302        self.assertEqual( 
     303            "Could not change passphrase: cannot guess the type of ''", str(error)) 
     304 
     305        self.assertEqual(privateRSA_openssh, FilePath(filename).getContent()) 
     306 
     307 
    287308    def test_changePassphrasePublicKey(self): 
    288309        """ 
    289310        L{changePassPhrase} exits when trying to change the passphrase on a 

13

  Changed 10 months ago by therve

(In [38998]) Apply the given patch

Refs #5894

14

  Changed 10 months ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [38999]) Merge ckeygen-changepass-5894

Authors: ltaylor.volks, therve Reviewers: tom.prince, therve Fixes: #5894

Fix ckeygen --changepass behavior, so that it doesn't accidently delete unencrypted keys or fail with encrypted ones.

Note: See TracTickets for help on using tickets.