Merge pull request #1696 from cjwatson/10308-conch-fix-subsystem-close

Author: cjwatson Reviewer: adiroiban Fixes: ticket:10308 If an environment variable passing request and a subsystem request were both sent on the same channel, then SSHSession only cleaned up the ISession adapter when receiving EOF or closing the channel, and did not call loseConnection on the client transport which is the only reasonable way for a subsystem to be notified when a connection is closed. SSHSession now cleans up both the client transport and the ISession adapter if both are set.
twisted · Feb 14, 2022 · 2cd9e00 · 2cd9e00
2 parents 0011ddd + e2b9d55
commit 2cd9e00
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 4 deletions.
diff --git a/src/twisted/conch/newsfragments/10308.bugfix b/src/twisted/conch/newsfragments/10308.bugfix
@@ -0,0 +1 @@
+twisted.conch.ssh.session.SSHSession now cleans up both the client transport and the ISession adapter if both are set.  Previously, a subsystem's connectionLost method was not called if a environment variable passing request was also sent on the same channel.
diff --git a/src/twisted/conch/ssh/session.py b/src/twisted/conch/ssh/session.py
@@ -180,16 +180,16 @@ def extReceived(self, dataType, data):
             log.warn("Weird extended data: {dataType}", dataType=dataType)
 
     def eofReceived(self):
+        if self.client:
+            self.conn.sendClose(self)
         if self.session:
             self.session.eofReceived()
-        elif self.client:
-            self.conn.sendClose(self)
 
     def closed(self):
+        if self.client and self.client.transport:
+            self.client.transport.loseConnection()
         if self.session:
             self.session.closed()
-        elif self.client:
-            self.client.transport.loseConnection()
 
     # def closeReceived(self):
     #    self.loseConnection() # don't know what to do with this

diff --git a/src/twisted/conch/test/test_session.py b/src/twisted/conch/test/test_session.py
@@ -542,6 +542,25 @@ def test_client_closed(self):
         self.assertTrue(self.session.client.transport.close)
         self.session.client.transport.close = False
 
+    def test_client_closed_with_env_subsystem(self):
+        """
+        If the peer requests an environment variable in its setup process
+        followed by requesting a subsystem, SSHSession.closed() should tell
+        the transport connected to the client that the connection was lost.
+        """
+        self.assertTrue(
+            self.session.requestReceived(b"env", common.NS(b"FOO") + common.NS(b"bar"))
+        )
+        self.assertTrue(
+            self.session.requestReceived(
+                b"subsystem", common.NS(b"TestSubsystem") + b"data"
+            )
+        )
+        self.session.client = StubClient()
+        self.session.closed()
+        self.assertTrue(self.session.client.transport.close)
+        self.session.client.transport.close = False
+
     def test_badSubsystemDoesNotCreateClient(self):
         """
         When a subsystem request fails, SSHSession.client should not be set.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		twisted.conch.ssh.session.SSHSession now cleans up both the client transport and the ISession adapter if both are set. Previously, a subsystem's connectionLost method was not called if a environment variable passing request was also sent on the same channel.