Fix a pseudo-bug re: responding to MSG_UNIMPLEMENTED w/ itself

dev-requirements.txt

@@ -9,3 +9,4 @@ twine>=1.11.0
 flake8==2.6.2
 pytest==3.2.1
 pytest_relaxed==1.0.0
+mock==2.0.0

paramiko/transport.py

@@ -51,6 +51,7 @@
     MSG_CHANNEL_WINDOW_ADJUST, MSG_CHANNEL_REQUEST, MSG_CHANNEL_EOF,
     MSG_CHANNEL_CLOSE, MIN_WINDOW_SIZE, MIN_PACKET_SIZE, MAX_WINDOW_SIZE,
     DEFAULT_WINDOW_SIZE, DEFAULT_MAX_PACKET_SIZE, HIGHEST_USERAUTH_MESSAGE_ID,
+    MSG_UNIMPLEMENTED, MSG_NAMES,
 )
 from paramiko.compress import ZlibCompressor, ZlibDecompressor
 from paramiko.dsskey import DSSKey
@@ -1950,12 +1951,20 @@ def run(self):
                         if len(self._expected_packet) > 0:
                             continue
                     else:
-                        err = 'Oops, unhandled type {:d}'.format(ptype)
-                        self._log(WARNING, err)
-                        msg = Message()
-                        msg.add_byte(cMSG_UNIMPLEMENTED)
-                        msg.add_int(m.seqno)
-                        self._send_message(msg)
+                        # Respond with "I don't implement this particular
+                        # message type" message (unless the message type was
+                        # itself literally MSG_UNIMPLEMENTED, in which case, we
+                        # just shut up to avoid causing a useless loop).
+                        name = MSG_NAMES[ptype]
+                        warning = "Oops, unhandled type {} ({!r})".format(
+                            ptype, name
+                        )
+                        self._log(WARNING, warning)
+                        if ptype != MSG_UNIMPLEMENTED:
+                            msg = Message()
+                            msg.add_byte(cMSG_UNIMPLEMENTED)
+                            msg.add_int(m.seqno)
+                            self._send_message(msg)
                     self.packetizer.complete_handshake()
             except SSHException as e:
                 self._log(ERROR, 'Exception: ' + str(e))

sites/www/changelog.rst

@@ -2,6 +2,11 @@
 Changelog
 =========
 
+- :bug:`-` Modify protocol message handling such that ``Transport`` does not
+  respond to ``MSG_UNIMPLEMENTED`` with its own ``MSG_UNIMPLEMENTED``. This
+  behavior probably didn't cause any outright errors, but it doesn't seem to
+  conform to the RFCs and could cause (non-infinite) feedback loops in some
+  scenarios (usually those involving Paramiko on both ends).
 * :support:`1262 backported` Add ``*.pub`` files to the MANIFEST so distributed
   source packages contain some necessary test assets. Credit: Alexander
   Kapshuna.

tests/test_transport.py

@@ -30,6 +30,7 @@
 import random
 from hashlib import sha1
 import unittest
+from mock import Mock
 
 from paramiko import (
     Transport, SecurityOptions, ServerInterface, RSAKey, DSSKey, SSHException,
@@ -38,8 +39,14 @@
 from paramiko import AUTH_FAILED, AUTH_SUCCESSFUL
 from paramiko import OPEN_SUCCEEDED, OPEN_FAILED_ADMINISTRATIVELY_PROHIBITED
 from paramiko.common import (
-    MSG_KEXINIT, cMSG_CHANNEL_WINDOW_ADJUST, MIN_PACKET_SIZE, MIN_WINDOW_SIZE,
-    MAX_WINDOW_SIZE, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_PACKET_SIZE,
+    MSG_KEXINIT,
+    cMSG_CHANNEL_WINDOW_ADJUST,
+    cMSG_UNIMPLEMENTED,
+    MIN_PACKET_SIZE,
+    MIN_WINDOW_SIZE,
+    MAX_WINDOW_SIZE,
+    DEFAULT_WINDOW_SIZE,
+    DEFAULT_MAX_PACKET_SIZE,
 )
 from paramiko.py3compat import bytes
 from paramiko.message import Message
@@ -974,3 +981,26 @@ def test_server_rejects_port_forward_without_auth(self):
             assert "forwarding request denied" in str(e)
         else:
             assert False, "Did not raise SSHException!"
+
+    def _send_unimplemented(self, server_is_sender):
+        self.setup_test_server()
+        sender, recipient = self.tc, self.ts
+        if server_is_sender:
+            sender, recipient = self.ts, self.tc
+        recipient._send_message = Mock()
+        msg = Message()
+        msg.add_byte(cMSG_UNIMPLEMENTED)
+        sender._send_message(msg)
+        # TODO: I hate this but I literally don't see a good way to know when
+        # the recipient has received the sender's message (there are no
+        # existing threading events in play that work for this), esp in this
+        # case where we don't WANT a response (as otherwise we could
+        # potentially try blocking on the sender's receipt of a reply...maybe).
+        time.sleep(0.1)
+        assert not recipient._send_message.called
+
+    def test_server_does_not_respond_to_MSG_UNIMPLEMENTED(self):
+        self._send_unimplemented(server_is_sender=False)
+
+    def test_client_does_not_respond_to_MSG_UNIMPLEMENTED(self):
+        self._send_unimplemented(server_is_sender=True)