Sync global request response handling between Client/Server

Eugeny · Feb 12, 2024 · 5f424bc · 5f424bc
1 parent 223c821
commit 5f424bc
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 8 deletions.
diff --git a/russh/src/client/encrypted.rs b/russh/src/client/encrypted.rs
@@ -815,15 +815,24 @@ impl Session {
                     Err(crate::Error::Inconsistent.into())
                 }
             }
-            Some(&msg::REQUEST_SUCCESS | &msg::REQUEST_FAILURE)
-                if self.common.alive_timeouts > 0 =>
-            {
-                // TODO what other things might need to happen in response to these two opcodes?
-                self.common.alive_timeouts = 0;
+            Some(&msg::REQUEST_SUCCESS) => {
+                trace!("Global Request Success");
                 Ok(())
             }
-            _ => {
-                info!("Unhandled packet: {:?}", buf);
+            Some(&msg::REQUEST_FAILURE) => {
+                // Right now, the only global request we send with a request for reply is keepalive,
+                // which just needs to be ignored.
+                // If there are other global requests with reply implemented,
+                // we'll need to build infrastructure to filter the expected request failures from the keepalive
+                // The following works as long as only a single keepalive request was sent before a reply:
+                // `if self.common.alive_timeouts > 0`
+                // since any data received will reset alive_timeouts back to zero,
+                // even if multiple keepalives will be processed due to TCP delivering all of them after connectivity was restored
+                trace!("Global Request Failure");
+                Ok(())
+            }
+            m => {
+                debug!("unknown message received: {:?}", m);
                 Ok(())
             }
         }

diff --git a/russh/src/client/session.rs b/russh/src/client/session.rs
@@ -292,7 +292,7 @@ impl Session {
         if let Some(ref mut enc) = self.common.encrypted {
             push_packet!(enc.write, {
                 enc.write.push(msg::GLOBAL_REQUEST);
-                enc.write.extend_ssh_string(b"keepalive@openssh.org");
+                enc.write.extend_ssh_string(b"keepalive@openssh.com");
                 enc.write.push(want_reply as u8);
             });
         }

diff --git a/russh/src/server/encrypted.rs b/russh/src/server/encrypted.rs
@@ -1059,6 +1059,22 @@ impl Session {
 
                 Ok(())
             }
+            Some(&msg::REQUEST_SUCCESS) => {
+                trace!("Global Request Success");
+                Ok(())
+            }
+            Some(&msg::REQUEST_FAILURE) => {
+                // Right now, the only global request we send with a request for reply is keepalive,
+                // which just needs to be ignored.
+                // If there are other global requests with reply implemented,
+                // we'll need to build infrastructure to filter the expected request failures from the keepalive
+                // The following works as long as only a single keepalive request was sent before a reply:
+                // `if self.common.alive_timeouts > 0`
+                // since any data received will reset alive_timeouts back to zero,
+                // even if multiple keepalives will be processed due to TCP delivering all of them after connectivity was restored
+                trace!("Global Request Failure");
+                Ok(())
+            }
             m => {
                 debug!("unknown message received: {:?}", m);
                 Ok(())