twisted.conch.ssh.transport has poor tests.

[z3p]

The reasons are too large to enumerate, but twisted.conch.ssh.transport needs better tests. Work is being done in the free-assert-2112-2 branch ATM.

[z3p]

Work has been done on this in branches/free-assert-2112-2. It is ready for review. Note: Work has also been done on #2679 for t.c.s.keys.

[therve]

It would be great if the changes could be extracted into another branch to be sure what's going to be reviewed...

[z3p]

Okay. I've split the changes to keys into a separate branch (keys-2679) and I'll be moving these changes into transport-2678. This branch depends on keys-2679 being merged, so I'm removing this from review.

[z3p]

I've removed the changes (getPublicKeyString -> getPublicKeyBlob) that made this depend on keys-2679. It's ready for review.

[z3p]

Sorry, ready for review in transport-2678.

[z3p]

Actually spell review correctly in the keywords.

[exarkun]

in SSHFactory.getDHPrimes:

        primesKeys.sort(lambda x,y,b=bits:cmp(abs(x-b), abs(y-b)))

is more simply expressed as

        primesKeys.sort(lambda x, y: cmp(abs(x - bits), abs(y - bits)))

Also, that spacing is preferred. This could also be spelled using twisted.python.util.dsu, but since primesKeys is short there's likely little benefit to doing so.

twisted/conch/ssh/service.py has trailing whitespace. The conditional:

            if f:

in packetReceived is probably better written:

            if f is not None:

since the purpose is really to catch the cases where getattr did not return the default value. It would also be nice if you could avoid the duplication between each of the else suites in that method. Also, log.msg() really doesn't want multiple positional arguments. One of these is preferable:

    log.msg("couldn't handle %r" % (messageType,))
    log.msg(format="couldn't handle %(messageType)r", messageType=messageType)

Some long lines remaining in transport.py.

I haven't taken a close look at transport.py or the test changes yet. If I have a chance, I'll do that tomorrow.

[z3p]

I've addressed those comments in the branch. I'll be out of town until the 11th, so take your time on looking at the other stuff. Thanks.

[exarkun]

• twisted/conch/ssh/transport.py
  • SSHTransportBase
    • should have a class docstring - including @ivars for each attribute
    • Why is idea being dropped? Why only comment it out instead of deleting it?
    • sendPacket needs a docstring
    • indentation should agree with emacs' python-mode - generally that means continuation lines get indented to match the open paren which lets them be continuation lines. line 102 should be indented more.
    • whitespace around operators - line 107 doesn't have it around +
    • similar indentation problem on line 117
    • getPacket needs a docstring
    • if/then is nicer than and/or
    • extra whitespace in slice on line 138
    • missing whitespace around % on line 141 and bad indentation of the continued line
    • missing whitespace around + and % on line 148/149 and bad indentation of continued lines
    • missing whitespace around + on line 154 and bad indentation of continued line on 156
    • bad indentation on line 163
    • change from catching zlib.error to catching every exception on line 170 seems wrong - why change this?
    • dataReceived is missing a docstring
    • no simple suites - line 180 should put return on a new line - why bother to search for
    • bad indentation on lines 190 and 191 - 4 space indent for this style of continuation
    • missing whitespace around % on line 210 - and + might be better than % for this kind of string building
    • another if f which probably wants to be if f is not None on line 211
    • missing whitespace around % on line 214
    • ssh_UNIMPLEMENTED is missing a docstring
    • ssh_DEBUG is missing a docstring
    • sendDebug is missing a docstring
    • extra whitespace around = in definition of sendDebug - no space around = which doesn't create or alter a binding (ie, not in function calls or definitions)
    • sendDisconnect is missing a docstring
    • whitespace around operators on 261/262 and bad indentation on that continuation line
    • _getKey is missing a docstring and is missing a bunch of whitespace around operators
    • loseConnection is missing a docstring
    • bad continuation indentation on line 301
    • receiveError is missing a docstring, has bad indentation, missing whitespace around operators
    • same for receiveUnimplemented
    • receiveDebug is missing docstring
    • another bad use of multiple positional arguments to log.msg on line on line 312
  • SSHServerTransport
    • missing a class docstring
    • ssh_KEXINIT is missing a docstring
    • missing whitespace in list comp in on line 326
    • bad indentation for arguments to SSHCiphers
    • bad indentation on 342 and 344
    • missing whitespace around % on line 350/351/354
    • ssh_KEX_DH_GEX_REQUEST_OLD is missing docstring
    • in ssh_KEX_DH_GEX_REQUEST_OLD, e is a pretty bad variable name. I guess this is what it's called in the crypto math definition? would be nice to come up with something better to name it for this code. was clientDHPubKey misleading?
    • bad indentation on lines 380/381
    • ssh_KEX_DH_GEX_INIT is missing docstring
    • bad indentation on lines 429/430/431
    • ssh_NEWKEYS missing docstring
    • bad indentation on line 437
    • bad indentation on line 450
    • bad indentation on line 466
  • SSHClientTransport
    • missing a docstring
    • ssh_KEXINIT is missing a docstring
    • ssh_KEXINIT looks a lot like SSHServerTransport.ssh_KEXINIT - any way to factor away some of this duplication? maybe as part of a different ticket
    • bad indentation for SSHCipher arguments and other stuff that was bad in SSHServerTransport.ssh_KEXINIT
    • ssh_KEX_DH_GEX_GROUP missing docstring
    • It's only slightly simpler than what's there now, but I think:

      ':'.join([ch.encode('hex') for ch in md5.new('hello').digest()])
      

      is a lot easier on the eyes than the map/lambda on 526/527
    • bad indentation on line 531/532
    • _continueKEXDH_REPLY missing docstring
    • bad indentation on line 556
    • ssh_KEX_DH_GEX_REPLY has another copy of that map/lambda - perhaps factor the fingerprint generation into a separate function
    • bad indentation on lines 569/570
    • _continueGEX_REPLY missing docstring
    • bad indentation on line 591
    • _keySetup missing docstring
    • bad indentation on line 605
    • ssh_NEWKEYS missing docstring
    • bad indentation on line 613
    • ssh_SERVICE_ACCEPT missing docstring
    • bad indentation on line 629
  • some messed up whitespace at the end of line 672
  • arcfour and idea going away - how come?
  • SSHCiphers
    • missing class docstring
    • verify_digest_size should use camelCase
    • setKeys has no docstring
    • _getCipher has no docstring
    • _getMAC has no docstring
    • encrypt and decrypt should raise NotImplementedError() and possibly supply a message
  • I assume DH_PRIME is from RFC 3526. A reference to that in the source would probably be helpful.
• twisted/conch/ssh/service.py
  • packetReceived spells "receive" wrong in its docstring and should document the types and meanings of its parameters
  • bad indentation on line 43
• twisted/conch/ssh/factory.py
  • some trailing whitespace
  • getPrimes should raise NotImplementedError()
• twisted/conch/test/test_conch.py
  • ConchTestOpenSSHProcess.processEnded is missing a docstring
  • bad indentation on line 70
• twisted/conch/test/test_transport.py
  • missing module docstring
  • some trailing whitespace
  • too many things in the try/except ImportError - which of these is actually the blocking factor? Just PyCrypto? If so, it should be in the try/except and the rest should be in an else.
  • md5/sha imports should come first - imports go in ascending dependency order
  • docstrings needed for all the fixtures defined here
  • TransportLoopbackTestCase.test_loopback looks like it's really several tests. Also, the way each combination is tested is pretty unfortunate. Wouldn't it be better to be fiddling with instance attributes, not class attributes? All this shared state makes me wary. I don't know what code is actually going to end up using what values (if any of the interaction is async, I'd guess that most of the iterations are going to end up using the wrong values).

Uh I guess that's it for now. This looks like a good start. When all the boring coding-standard type things are fixed up I think this is going to be a nice set of changes.

[z3p]

exarkun: I'm working on this now, but I'm not sure what your line numbers mean which is making it more difficult. They don't seem to map to line numbers in the file.

[z3p]

I tried to fix the indentation problems, but because of the line numbers I may have missed some of them. Anyways, I think that the more important problems with documentation and test layout have been corrected, so I'm putting this back up for review (branches/transport-2678)

[z3p]

Whoops; forgot to tag as review.

[exarkun]

Some more stuff, mostly doc-related:

• factory.py:
  • twisted.conch.ssh.factory.SSHFactory.getPrimes should raise NotImplementedError()
• transport.py:
  • Still wondering why idea is being dropped
  • SSHFactory still has some undocumented attributes:
    • isClient
    • gotVersion
    • ourVersionString
    • buf
    • outgoingPacketSequence
    • incomingPacketSequence
    • outgoingCompression
    • incomingCompression
    • sessionID
    • isAuthorized
    • service
    • kexAlg
    • keyAlg
    • nextEncryptions
    • outgoingCompressionType
    • incomingCompressionType
    • ourKexInitPayload
    • currentEncryptions
    • first
    • otherVersionString
    • otherKexInitPayload
  • SSHFactory.connectionMade has no docstring
  • Typo in SSHFactory.sendPacket docstring - messageTYpe
  • I'm not sure what "authenticate" means in SSHFactory.sendPacket's docstring - I assume it refers to the makeMAC call near the end of the method, but makeMAC's docstring doesn't explain what a MAC is (or even expand the acronym).
  • in SSHFactory.getPacket, dealing with exceptions from arbitrary compression schemes would probably be best done by requiring compression schemes to conform to some interface (ie, make them raise a particular exception, or make them expose the exceptions they raise so they can be handled). For the time being, it might be alright to leave the bare except if there is also a log.err() call inside it (so that any unexpected exception appears in the log), but it might be better to revert this change and file a new ticket for dealing with exceptions from different compression objects (otoh there's test coverage for handling exceptions, so...).
  • On SSHServerTransport some attributes are undocumented:
    • ignoreNextPacket
    • dhGexRequest
    • ideal
    • g
    • p
    • min
    • max
  • On SSHClientTransport, some attributes are undocumented:
    • _gotNewKeys
    • x
    • e
    • p
    • g
    • instance
  • SSHClientTransport._continueKEXDH_REPLY should document its parameters
  • SSHClientTransport._continueGEX_REPLY should have a docstring
  • SSHCiphers still has undocumented attributes:
    • outCipType
    • inCipType
    • outMacType
    • inMacType
    • encBlockSize
    • decBlockSize
    • verifyDigestSize
    • outMAC
    • inMAC
  • SSHCiphers uses inconsistent casing for MAC - eg, inMacType and inMAC. This is pre-existing, but I notice some other attributes were already renamed. If there is no compatibility concern in this class, then it'd be good to fix these other names as well. Based on a quick google code search, it doesn't look like anybody is using these names, so the rename might be okay. The default policy is to assume backward compatibility is required, though.
  • Some functions still raise NotImplementedError instead of raise NotImplementedError(message)
  • There are a bunch of new docstrings in this file which aren't valid epytext. I think these are mostly the "Payload:" and similar strings. epytext requires a double colon to preceed an indented block. You can check this with the doc buildslave.
• test_transport.py
  • TestTransportBase, TestCipher, TestCompression, TestService, TestFactory should be named "Stub*". Those which don't subclass something should subclass object so as to be new-style.
  • Some lines >80 cols
  • functions named _ should have meaningful names (eg stubLoseConnection) to avoid debugging headaches
  • Some test cases and test methods missing docstrings
  • Try to avoid docstrings of the form "Test X properly does Y" - instead describe Y and its relevant side-effects.

Not sure what happened with line numbers in my last comment. Rather than trying that again, I'm attaching a patch of all the formatting fixes suggested by the style guide.

[exarkun]

indentation and other formatting changes

[z3p]

I think I've handled all these things, so I'm putting this back up for review.

[exarkun]

in transport.py, in ssh_KEX_DH_GEX_INIT, the guard against small y has been removed. Is this no longer an issue with OpenSSH? What about support for interaction with older versions of SSH?

Why is IDEA support being removed?

test_transport.py still has classes and methods without docstrings.

The nested function in BaseSSHTransportTestCase.test_badPackets, testBad, does a strange thing with hasattr() and del. What's the point of this code? If it were being executed at all, wouldn't the test be failing? If it's not executing, why is it there?

The loops in the test methods in TransportLoopbackTestCase look a little bit fragile to me. The nested function works properly since it is executed each time through the loop, but since the invocation is non-local, this isn't immediately obvious. Making the binding more explicit would probably reduce the chance of these tests accidentally being changed to repeatedly test only the last element of each list.

[exarkun]

Oh, I forgot another important thing. The conch ssh client won't connect to any of the ssh servers I tested it with, now:

$ conch divmod.org
Connection to divmod.org closed.
conch: exiting with error [Failure instance: Traceback (failure with no frames): twisted.conch.error.ConchError: ('bad packet length 2838643806', 2)
]

[z3p]

1. The random number generator makes sure that small y cannot be generated by making sure the high bit is set.
2. I'm not sure IDEA support was ever actually there, but my version of PyCrypto does not have an IDEA module.
3. I added some more docstrings; what's still missing?
4. I'm not sure what the point of that was; it's gone now.
5. What do you suggest? I did it that way to avoid duplicated code across multiple tests which is more fragile.
6. It should connect now.

[exarkun]

I get several test failures:

===============================================================================
[FAIL]: twisted.conch.test.test_ssh.SSHProtocolTestCase.testOurServerOurClient

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/test/test_ssh.py", line 717, in testOurServerOurClient
    self.serverTransport.clearBuffer)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/log.py", line 36, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/context.py", line 59, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/context.py", line 37, in callWithContext
    return func(*args,**kw)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/loopback.py", line 227, in clearBuffer
    self.target.dataReceived(buffer)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/ssh/transport.py", line 310, in dataReceived
    self.dispatchMessage(messageNum, packet[1:])
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/ssh/transport.py", line 324, in dispatchMessage
    f(payload)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/ssh/transport.py", line 954, in ssh_KEX_DH_GEX_REPLY
    d = self.verifyHostKey(pubKey, fingerprint)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/test/test_ssh.py", line 347, in verifyHostKey
    unittest.assertEquals(key, keys.getPublicKeyString(data = publicRSA_openssh))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1116, in _
    return getattr(_inst, name)(*args, **kwargs)
twisted.trial.unittest.FailTest: "\x00\x00\x00\x07ssh-dss\x00\x00\x00A\x00\x86\xf0L\xe4\xacg\xb0e\xedMJ\xc8\xc3jW]\xd3\xbb\xbc\x91\x02\xd4\xeb\xefC\xd5#t'~\xb3\xee\x99\xfd\x94\xcc()/Ycl\xacR|\x08\xb4\xe5{\xeb\x13\xfdo|!@\xb0V\x17E\x92\xe1C-\x00\x00\x00\x15\x00\xcfz\xe4\t&\xa3\xa7\x80\x8b\xb2\xbe\x93\xda\x83G\x84\xe8\xb4P\x17\x00\x00\x00@\x00\xd9\x96\x94\xd6\xf7\xae!i\x05\x92\xe2\xf0\xb4\xebsFvWS\xeb\x1aT\x187\x03F\xa6\xe6\xf7@\xa8<F\xb6FcN\xb8\xea\xa2\xf2Y\xdb\x85\x8a\xda\x13<\xa9`}\xca\xe0l\xef\xfd\xbcB\x06r\xad\x1d\x15\x00\x00\x00@+\xedh\xb3%\x81KO\x8f\xf7K\xbf\xac\x0b\xa4\xa3\xa7!g\xdf-\x85D\x98\xdaB\xd2\x1e\x9e\xa1,\xac\xb2\xd9\xb8\xe7Ooa\xee\x16\x84,O\x97\x1d\xfa\xb6l\xcf_r@\xf1\xd8R\xceH\x82s\xcb\xcd\xd5\x99" != '\x00\x00\x00\x07ssh-rsa\x00\x00\x00\x01#\x00\x00\x00a\x00\xaf2q\xf0\xe6\x0e\x9c\x99\xb3\x7f\x8b_\x04K\xcb\x8b\xc0\xd5>\xb2w\xfd\xcfd\xd8\x8f\xc0\xcf\xae\x1f\xc61\xdf\xf6)\xb2D\x96\xe2\xc6\xd4!\x94\x7fe|\xd8\xd4#\x1f\xb8.j\xc9\x1f\x94\rF\xc1i\xa2\xb7\x07\x0c\xa3\x93\xc14\xd8.\x1eJ\x99\x1al\x96F\x07F+\xdc%)\x1b\x87\xf0\xbe\x05\x1d\xee\xb44\xb9\xe7\x99\x95'
===============================================================================
[FAIL]: twisted.conch.test.test_conch.OpenSSHClientTestCase.test_localToRemoteForwarding

Traceback (most recent call last):
Failure: twisted.trial.unittest.FailTest: None != 'test\n'
===============================================================================
[FAIL]: twisted.conch.test.test_conch.OpenSSHClientTestCase.test_remoteToLocalForwarding

Traceback (most recent call last):
Failure: twisted.trial.unittest.FailTest: None != 'test\n'
===============================================================================
[ERROR]: twisted.conch.test.test_conch.OpenSSHClientTestCase.test_exec

Traceback (most recent call last):
Failure: twisted.internet.defer.TimeoutError: <twisted.conch.test.test_conch.OpenSSHClientTestCase testMethod=test_exec> (test_exec) still running at 120.0 secs
===============================================================================
[ERROR]: twisted.conch.test.test_conch.OpenSSHClientTestCase.test_exec

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/process.py", line 679, in maybeCallProcessEnded
    self.proto.processEnded(failure.Failure(e))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/test/test_conch.py", line 73, in processEnded
    self._getDeferred().errback(
exceptions.AttributeError: 'module' object has no attribute 'ConchError'
-------------------------------------------------------------------------------

[z3p]

There was a problem with how the KEXINIT method was picking methods. It's fixed now, and ready for review.

[exarkun]

Hey, review this.

[exarkun]

• It is apparently now necessary for SSHFactory subclasses to define a primes attribute, even if only to bind it to an empty dictionary. Is there a reason the base class couldn't just offer an empty dictionary for this attribute? If it did, then the manhole_ssh.py changes would presumably be unnecessary (the obvious positive consequence of not making backwards incompatible changes ;). I'm not entirely sure I understand where this new requirement came from, though. It seems getDHPrimes is the method of SSHFactory which requires the primes attribute; however, startFactory seems to go to some effort to provide the primes attribute (not sure this is really sensible, but whatever). I suppose the issue is that the manhole subclass doesn't implement getPrimes, either? But then, I'm not sure why this wouldn't have caused NotImplementedError to be raised prior to this change.
• Some trailing whitespace is left in transport.py
• There's a print in verifyHostKey in test_ssh.py

Aside from those issues, things look good.

[z3p]

I backed out the changes to factory.py/manhole_ssh.py. At best that's a separate issue, but I don't think it's a problem. Fixed the print/whitespace. Back up for review.

[exarkun]

There are a few conflicts in twisted/conch/ssh/transport.py now, mostly relating to the randbytes change.

[z3p]

(In [22441]) Branching to 'transport-2678-3'

[z3p]

Finally got rid of the conflicts. Up for review.

BTW, I like the new branch feature in Trac.

[therve]

There are some trivial cleanups problems (like some redundant imports in transport.py), but the main problem is that the conch test suite itself is broken. It doesn't fail but is blocked in the first test. I have this error message in the log file str' object has no attribute 'blob'.

[z3p]

(In [22447]) fix the failing tests

Refs #2678

[z3p]

It was caused by a change in the Factory API. I added some code to handle the deprecated interface, and some tests of the deprecation code.

[exarkun]

(In [22456]) Branching to 'transport-2678-4'

[exarkun]

Lots of test failures, mostly like this one:

[ERROR]: twisted.names.test.test_names.ServerDNSTestCase.testSimilarZonesDontInterfere

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/test/test_names.py", line 392, in testSimilarZonesDontInterfere
    self.resolver.lookupAddress("anothertest-domain.com"),
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/common.py", line 35, in lookupAddress
    return self._lookup(name, dns.IN, dns.A, timeout)
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/client.py", line 317, in _lookup
    [dns.Query(name, type, cls)], timeout
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/client.py", line 233, in queryUDP
    return self.protocol.query(used, queries, timeout[0]
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/dns.py", line 1548, in query
    id = self.pickID()
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/dns.py", line 1394, in pickID
    self.id += randomSource() % (2 ** 10)
  File "/home/exarkun/Projects/Twisted/branches/transport-2678-4/twisted/names/dns.py", line 46, in randomSource
    return struct.unpack('H', randbytes.secureRandom(2, fallback=True))[0]
exceptions.TypeError: secureRandom() got an unexpected keyword argument 'fallback'

[z3p]

One of the TestCases wasn't cleaning up after itself. Fixed in 22469 and back up for review.

[therve]

• Pyflakes messages:

  twisted/conch/ssh/transport.py:31: redefinition of unused 'protocol' from line 29
  twisted/conch/ssh/transport.py:31: redefinition of unused 'defer' from line 29
  twisted/conch/ssh/transport.py:32: redefinition of unused 'log' from line 28
  

• isEncrypted and isVerified TypeErrors are not tested
• I wonder if test_isEncrypted doesn't contain an error due to bad copy/paste, because it calls isVerified on the second part
• receiveError and receiveUnimplemented are not tested
• in ssh_KEX_DH_GEX_REQUEST_OLD, last ConchError is not tested
• same thing for ssh_KEXINIT
• the buffer_dump function seems to be unused. Maybe this could be safely removed, I don't know.
• verifyHostKey, connectionSecure, encrypt and decrypt are not tested. Also, they should raise NotImplementedError(), not NotImplementedError.
• the assert in sendPacket should be replaced my a meaningful error, or removed.

That's it for now!

[z3p]

(In [22540]) Fix review comments from therve

Refs #2678

[z3p]

Fixed: * pyflakes errors * added test for ConchErrors in KEXINIT and KEX_DH_GEX_REQUEST_OLD * NotImplementedError() in verifyHostKey, connectionSecure, encrypt, and decrypt * removed buffer_dump and the assertion in sendPacket * logic error in test_isEncrypted * added testing for verifyHostKey in test_KEX_DH_GEX_REPLY and test_KEXDH_REPLY

receiveError is tested in all of the TransportLoopbackTestCase tests, as well as in BaseTransportTestCase.test_receiveDisconnect

receiveUnimplemented is tested in BaseTransportTestCase.test_receiveUnimplemented

[therve]

receiveError and receiveUnimplemented are not really tested: you test that they are called, but the implementation in transport.py is never called (you override them every time). That's not a blocker point though. The buildbot is green, and I have only some aethetic remarks:

• add some blank lines around: the rule now is 3 lines at module level and 2 at class level
• update copyrights of transport and test_transport
• in test_getCipher, please use assertIsInstance
• in MockCipher, encrypt and decrypt should use a meaningful exception instead of using assert
• in transport and test_transport there are some missing space after commas and around some operators (eg in test_sendVersion or SSHCiphers.cipherMap)
• please remove backslashed in SSHTransportBase.sendKexInit

This merge would make the test suite cross the 4000 tests barrier :). Thanks!

[z3p]

(In [22564]) add a small test, and some aesthetic changes

Refs #2678

[z3p]

I think I got all the commands and operators, but I may have missed some. I committed in changes for your other comments as well.

[therve]

• in TransportTestCase.setUp, there is a commented print that should be removed.
• you didn't make the blank lines change. As I feel you're reluctant do it, please at least comply to the 2 / 1 rule :).

I tried using the conch client to make some tests, and faced several problems:

• When login with a password, I got this warning printed:

  /home/th/twisted_dev/twisted/trunk/twisted/conch/client/default.py:161: DeprecationWarning: getPublicKeyString is deprecated since Twisted Conch 0.9.  Use Key.fromString().
    return keys.getPublicKeyString(file)
  

• When logout, this one:

  /home/th/twisted_dev/twisted/trunk/twisted/conch/scripts/conch.py:428: DeprecationWarning: This function is deprecated, use loseWriteConnection instead.
                              self.stdio.closeStdin()
                                                     Connection to localhost closed.
  

• I was unable to connect to a host using key mechanism. It fails with:

    File "/home/th/twisted_dev/twisted/trunk/twisted/conch/ssh/keys.py", line 170, in _fromString_PRIVATE_OPENSSH
      raise EncryptedKeyError('encrypted key with no passphrase')
  twisted.conch.ssh.keys.EncryptedKeyError: encrypted key with no passphrase
  

But it seems these problems are already in trunk... So I guess we have to move on to correct this.

[z3p]

I added some whitespace to ssh/transport.py and test/test_transport.py

The first warning is because of the Keys change. The second warning (I think) has been there for a while. The exception is an actual bug related to the Keys change, but outside the scope of this ticket.

[therve]

Please merge!

[z3p]

(In [22624]) Merge transport-2678-4

Author: z3p Reviewers: therve, exarkun Fixes #2678

Adds better unittests and documentation for twisted.conch.ssh.transport. This work was done as part of Google's Summer of Code 2007.

[therve]

(In [22628]) Revert r22624: regression in test_conch.

Refs #2678

[therve]

(In [22629]) Fix buggy import.

Refs #2678

[therve]

(In [22630]) Merge transport-2678-4

Author: z3p Reviewers: therve, exarkun Fixes #2678

Adds better unittests and documentation for twisted.conch.ssh.transport. This work was done as part of Google's Summer of Code 2007.

This remerge fixes an error in a tearDown method of test_conch.