transition twisted.conch.ssh.keys.Key to using a real ASN.1 parser

[bash]

I am getting ValueError exceptions when reading some private DSA keys when reading them with Key.fromString. This is a new problem since upgrading Twisted-8.1.0 (core). Twisted-2.5.0 (core) does not exhibit the same problem. Attaching sample DSA private key.

Python 2.5.2 (r252:60911, Jul 11 2008, 11:11:29) 
[GCC 4.1.2 20071124 (Red Hat 4.1.2-42)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from twisted.conch.ssh.keys import Key
>>> Key.fromString(file("/tmp/id_dsa").read())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Intelerad/3rd_Party/python-2.5/lib/python2.5/site-packages/Twisted-8.1.0_r24580-py2.5-linux-i686.egg/twisted/conch/ssh/keys.py", line 88, in fromString
    return method(data, passphrase)
  File "/usr/local/Intelerad/3rd_Party/python-2.5/lib/python2.5/site-packages/Twisted-8.1.0_r24580-py2.5-linux-i686.egg/twisted/conch/ssh/keys.py", line 231, in _fromString_PRIVATE_OPENSSH
    p, q, g, y, x = decodedKey[1: 6]
ValueError: need more than 1 value to unpack

[bash]

Sample private DSA key.

[exarkun]

reformatting description

[exarkun]

Thanks. As a temporary workaround, you can strip the

[exarkun]

ahem, strip the input and I think you'll get a valid Key object back (you'll certainly get a key object back).

[bash]

Thanks exarkun. Your temporary workaround did the trick!

[z3p]

(In [25678]) Branching to 'dsa-key-failure-3391'

[z3p]

This turns out to be an annoying bug. Sometimes, if there's an extra newline or several, the private key parsing code doesn't pull off the bottom line correctly. Sometimes, that makes the base64 decode fail, but not always. In your case, it does make the decode fail, but for the current DSA and RSA keys that are used as test data, it doesn't.

[exarkun]

How about switching to a real ASN1 parser? pyasn1 seems pretty good and it's easy to parse SSH keys with it.

[z3p]

That's probably a good idea. Changing the summary to reflect new nature of the bug.

[exarkun]

Just as far as issue-tracker workflow goes, http://twistedmatrix.com/trac/ticket/414#comment:13

[exarkun]

#3448 was a duplicate of this.

[z3p]

This is ready for review. Two notes for the reviewer:

1. This removes the twisted/conch/ssh/asn1.py module and adds a new dependency, which is a backwards-incompatible change. Do I need to keep the old one around and support not having PyASN1? 1. The change in twisted/python/rebuild.py: I don't understand the code very well, but it certainly seems like the problem is with rebuild and not conch.

[glyph]

1. The change in rebuild needs to be accompanied by its own independent test. Perhaps in a separate branch if you would be so kind :). It's a problem with rebuild which is exercised by the buggy __getattr__ implementations in pyasn1.
2. I'm not totally sure about the new dependency vs. compatibility: but you definitely need to keep parse and pack. My suggestion for the time being would be to implement them in terms of pyasn1, and emit deprecation warnings from them; don't add any usages of them into conch.
3. it would be nice if the dependency-alert skip message would tell me specifically which packages I am missing; not just "Crypto and asn1" regardless of which I have installed.

I wouldn't worry too much about the dependency. Worst case scenario, we can bundle in a mirror of pyasn1 for the next release, like what Imaginary did for pyparsing; it's tiny and its license is compatible. It's also pure python, so it's easy to install. However, the buildbots aren't going to be happy, and I'm sure people will complain.

[z3p]

1 was filed as #3817 (and backed out as r26727). 2 & 3 were addressed in r26828.

Back up for review. test_rebuild is failing again, but that's a separate bug now.

[z3p]

(In [26911]) Branching to 'dsa-key-failure-3391-2'

[fijal]

Hm. So except Exception: is not correct on any python 2.5 or newer, because you should catch BaseException. Now how do I write a test that does not do if sys.version ....? Or simply use except:?

[therve]

#3630 has been closed as a duplicate of this one.

[therve]

OK, I think some progress has been made. The dependencies are a mess, but I guess we can live with it for now.

Fijal: for the "except Exception", it's not new here, so please open a new bug if you care.

[jml]

Looks fine to me. You changed some stuff that I commented on face-to-face.

[therve]

(In [27079]) Merge dsa-key-failure-3391-2

Authors: z3p, therve Reviewers: glyph, jml Fixes #3391

Replace Conch custom ASN1 parser by pyasn1. It introduces a new dependency for Conch, and solve a bunch of problems related to the previously custom parser.

[thierry.chich]

I don't know what do you project to do exacltly (change the ASN Parser in the next version ?). However, the problem still exist in the 8.2 version. It is still ennoying.

I have just added the following lines

--- /tmp/orgi 2009-10-31 11:37:51.000000000 +0100 +++ /usr/share/pyshared/twisted/conch/ssh/keys.py 2009-10-31 11:38:07.000000000 +0100 @@ -226,6 +226,8 @@

p, q = q, p

return Class(RSA.construct((n, e, d, p, q)))

elif kind == 'DSA':

+ # following line added to solve bug 3391 + decodedKey = decodedKey[0]

p, q, g, y, x = decodedKey[1:6] return Class(DSA.construct((y, g, p, q, x)))

_fromString_PRIVATE_OPENSSH = classmethod(_fromString_PRIVATE_OPENSSH)

It seems that it solve the problem.

[exarkun]

This wasn't fixed in 8.2. It was fixed after 8.2. To get the fix, you need to wait for the next release or use a development version of Twisted from after the fix - r27079 (as mentioned in the comment before yours) or later.