move HMAC import out of receiver thread to avoid import deadlock

[gernot-h]

Hi there!

Is there a special reason to do the Crypto.HMAC import from within the compute_hmac function? As this is ran in context of a new thread, it can lead to a deadlock if the paramiko user himself tries to create and connect a paramiko SSHClient in import context. The following small change would have saved me a lot of hassle:

diff --git a/paramiko/packet.py b/paramiko/packet.py
index 38a6d4b..74628d6 100644
--- a/paramiko/packet.py
+++ b/paramiko/packet.py
@@ -38,11 +38,10 @@ try:
     import r_hmac
     got_r_hmac = True
 except ImportError:
-    pass
+    from Crypto.Hash import HMAC
 def compute_hmac(key, message, digest_class):
     if got_r_hmac:
         return r_hmac.HMAC(key, message, digest_class).digest()
-    from Crypto.Hash import HMAC
     return HMAC.HMAC(key, message, digest_class).digest()

I tested that change with an old version (paramiko 1.7.7.1 and Python 2.7.3) and it solved that deadlock for me. Please let me know if you're considering it - I can then happily test with the current versions!

Background: I'm working on a Python (2) test framework module which automatically passes on tests to a remote host via SSH using paramiko. To make life easier for the user, I created one SSHClient instance immediately on import of my module and connected it to the target. This lead to a deadlock - and costed me more than a day to understand.

One could discuss if connecting in context of an import was a good idea, but I had no idea that paramiko would create a new thread there - and about the global Python import lock. So my import was blocked in paramiko's connect function which was itself blocked in read_message->compute_hmac.

One can also argue that Python 3.3 fixes that issue by removing the global import lock, but like me, I think many people are still using Python 2.x

So I'd vote for applying the little change above - or is there any negative side effect I currently don't see?

[bitprophet]

Thinking about side effects:

• Frequently when imports are placed within functions it's to avoid circular imports - but I don't see how that'd be the case here as the nested import is to Crypto, not anything within Paramiko.
• Other times it's to make imports "optional" and only fire on a given code path - but PyCrypto is not optional for Paramiko so again, moot.
• A third reason might be name or module-level code execution conflicts (e.g. if both imports attempted to load up the name HMAC) but that's also not happening, nor would moving the import to the except ImportError clause change things even if there was a conflict.

I also checked on the history of that chunk of code and it was all added in one go by Robey in 2006 and unchanged since; the commit message only mentions adding the option of using the native HMAC lib (before it was just straight up importing Crypto's HMAC) and nothing about why it was implemented this way.

So, all of that tells me this was just a suboptimal way of implementing the 'import one, if fails, import the other' pattern and your change is probably fine! I'll test it out real quick and then merge. Thanks!

[bitprophet]

Backporting this to the 1.10 line:

• Paramiko's (small) internal test suite: OK
• Fabric's main test suite: OK
• Fabric's integration test suite: OK

Forward-merged to 1.11 and master.