fix: Sound impl of Session::userauth_agent

Author: pbar1

src/agent.rs

@@ -32,6 +32,9 @@ impl<C: RuntimeContext> Agent<C> {
 }
 
 /// Async wrappers
+///
+/// [`Agent::userauth`] is the one agent function that can be async-wrapped
+/// because it writes to the session.
 #[allow(clippy::missing_errors_doc)]
 impl<C: RuntimeContext> Agent<C> {
     /// See [`ssh2::Agent::userauth`]
@@ -43,21 +46,22 @@ impl<C: RuntimeContext> Agent<C> {
 }
 
 /// Sync wrappers
+///
+/// `libssh2` SSH agent socket connections are inherently synchronous and can't
+/// be made async. The internal socket is not set to nonblocking and overriding
+/// this is not exposed.
 #[allow(clippy::missing_errors_doc)]
 impl<C: RuntimeContext> Agent<C> {
-    // FIXME: Async
     /// See [`ssh2::Agent::connect`]
     pub fn connect(&mut self) -> Result<(), Error> {
         self.inner.connect()
     }
 
-    // FIXME: Async
     /// See [`ssh2::Agent::disconnect`]
     pub fn disconnect(&mut self) -> Result<(), Error> {
         self.inner.disconnect()
     }
 
-    // FIXME: Async
     /// See [`ssh2::Agent::list_identities`]
     pub fn list_identities(&mut self) -> Result<(), Error> {
         self.inner.list_identities()

src/lib.rs

@@ -34,6 +34,7 @@ mod consts {
 
     pub const ERROR_EAGAIN: ErrorCode = ErrorCode::Session(-37);
     pub const ERROR_BAD_SOCKET: ErrorCode = ErrorCode::Session(-45);
+    pub const ERROR_INVAL: ErrorCode = ErrorCode::Session(-34);
 }
 
 /// Tests are adapted from the [`ssh2`] crate examples.
@@ -51,7 +52,7 @@ mod tests {
     type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
 
     #[tokio::test]
-    async fn inspecting_ssh_agent() -> Result<()> {
+    async fn inspect_agent() -> Result<()> {
         let _agent = testing::agent().await?;
 
         testing::with_server(|server_addr| async move {
@@ -80,7 +81,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn authenticating_with_ssh_agent() -> Result<()> {
+    async fn authenticate_agent() -> Result<()> {
         let _agent = testing::agent().await?;
 
         testing::with_server(|server_addr| async move {
@@ -90,11 +91,7 @@ mod tests {
             sess.handshake().await?;
 
             // Try to authenticate with the first identity in the agent.
-            let mut agent = sess.agent()?;
-            agent.connect()?;
-            agent.list_identities()?;
-            let identity = agent.identities()?.remove(0);
-            agent.userauth("username", &identity).await?;
+            sess.userauth_agent("username").await?;
 
             // Make sure we succeeded
             assert!(sess.authenticated());
@@ -105,7 +102,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn authenticate_with_password() -> Result<()> {
+    async fn authenticate_password() -> Result<()> {
         testing::with_server(|server_addr| async move {
             // Connect to the SSH server
             let tcp = TcpStream::connect(server_addr).await?;

src/session.rs

@@ -19,6 +19,7 @@ use crate::Sftp;
 #[cfg(feature = "tokio")]
 use crate::TokioContext;
 use crate::consts::ERROR_BAD_SOCKET;
+use crate::consts::ERROR_INVAL;
 
 /// Async wrapper for [`ssh2::Session`].
 pub struct Session<C: RuntimeContext> {
@@ -158,10 +159,23 @@ impl<C: RuntimeContext> Session<C> {
             .await
     }
 
+    // It is unsound to simply wrap `ssh2::Session::userauth_agent` with async
+    // like the rest of the methods since that would recreate the agent
+    // connection if called with retries, which is not reentrant safe. Instead
+    // we mimic the implementation in terms of our own async methods and avoid
+    // duplicating the agent connection.
     pub async fn userauth_agent(&self, username: &str) -> Result<(), Error> {
-        self.ctx
-            .with_async(|| self.inner.userauth_agent(username))
-            .await
+        let mut agent = self.agent()?;
+        agent.connect()?;
+        agent.list_identities()?;
+        let identities = agent.identities()?;
+        let Some(identity) = identities.first() else {
+            return Err(Error::new(
+                ERROR_INVAL,
+                "no identities found in the ssh agent",
+            ));
+        };
+        agent.userauth(username, identity).await
     }
 
     pub async fn auth_methods(&self, username: &str) -> Result<&str, Error> {