tor-socksproto: Implement HasKind

(This error isn't yet wrapped in TorError, but it will be eventually
when we implement socks proxy and PT support.)

Cargo.lock

@@ -3312,6 +3312,7 @@ dependencies = [
  "hex-literal",
  "thiserror",
  "tor-bytes",
+ "tor-error",
 ]
 
 [[package]]

crates/tor-error/src/lib.rs

@@ -147,6 +147,31 @@ pub enum ErrorKind {
     #[display(fmt = "could not find a home directory")]
     NoHomeDirectory,
 
+    /// A requested operation was not implemented by Arti.
+    ///
+    /// This kind of error can happen when calling an API that isn't available
+    /// at runtime, or when requesting a piece of protocol functionality that is
+    /// not implemented.
+    ///
+    /// If it happens as a result of a user activity, it's fine to ignore, log,
+    /// or report the error. If it happens as a result of direct API usage, it
+    /// may indicate that you're using something that isn't implemented yet, or
+    /// hasn't been turned on for your build environment.
+    #[display(fmt = "operation not supported")]
+    NoSupport,
+
+    /// Someone or something violated a network protocol.
+    ///
+    /// This kind of error can happen when a remote Tor instance behaves in a
+    /// way we don't expect, or when a local program accessing us over some
+    /// other protocol violates the protocol's requirements.
+    ///
+    /// It usually indicates a programming error: either in their implementation
+    /// of the protocol, or in ours.  It can also indicate an attempted attack,
+    /// though that can be hard to diagnose.
+    #[display(fmt = "network protocol violation")]
+    ProtocolViolation,
+
     /// Internal error (bug) in Arti.
     ///
     /// A supposedly impossible problem has arisen.  This indicates a bug in

crates/tor-socksproto/Cargo.toml

@@ -13,6 +13,7 @@ repository="https://gitlab.torproject.org/tpo/core/arti.git/"
 [dependencies]
 caret = { path="../caret", version = "0.0.3"}
 tor-bytes = { path="../tor-bytes", version = "0.0.3"}
+tor-error = { path="../tor-error", version = "0.0.1" }
 
 thiserror = "1"
 

crates/tor-socksproto/src/err.rs

@@ -1,6 +1,8 @@
 //! Declare an error type for tor_socksproto
 use thiserror::Error;
 
+use tor_error::{ErrorKind, HasKind, InternalError};
+
 /// An error that occurs while negotiating a SOCKS handshake.
 #[derive(Error, Debug)]
 #[non_exhaustive]
@@ -31,11 +33,48 @@ pub enum Error {
     /// Tried to progress the SOCKS handshake when it was already
     /// finished.  This is a programming error.
     #[error("SOCKS handshake was finished; no need to call this again")]
-    AlreadyFinished,
+    AlreadyFinished(InternalError),
 
     /// Something went wrong with the programming of this module.
     #[error("Internal programming error while handling SOCKS handshake")]
-    Internal,
+    Internal(InternalError),
+}
+
+// Note: at present, tor-socksproto isn't used in any settings where ErrorKind
+// is used.  This is provided for future-proofing, since someday we'll want to
+// have SOCKS protocol support internally as well as in the `arti` proxy.
+impl HasKind for Error {
+    fn kind(&self) -> ErrorKind {
+        use Error as E;
+        use ErrorKind as EK;
+        match self {
+            E::Truncated => {
+                // XXXX: This is not truly an error; it just means that you need
+                // to read more and try again.
+                //
+                // So... what do we do with ErrorKind here?
+                //
+                // Should we add a new "NotAnError" kind, and document that if
+                // you ever see it, you treated some "normal" error as a real
+                // error?  [Users hate seeing: "FATAL: not an error" so I don't
+                // love that].
+                //
+                // Should we add a new "MessageTruncated" or "ReadMore" or
+                // "WantRead" kind?  That might be our best bet; we can document
+                // that it's an error something should have handled before it
+                // propagated to TorError.
+                //
+                // Should we remove this error case from `Error`, and change the
+                // return type for all the functions in handshake(), so that
+                // they return an Option<Result<>> or a Result<Result<>> ?
+                todo!()
+            }
+            E::Syntax | E::BadProtocol(_) => EK::ProtocolViolation,
+            E::NoSupport => EK::NoSupport,
+            E::AlreadyFinished(_) => EK::Internal,
+            E::Internal(_) => EK::Internal,
+        }
+    }
 }
 
 impl From<tor_bytes::Error> for Error {

crates/tor-socksproto/src/handshake.rs

@@ -6,6 +6,7 @@ use crate::{Error, Result};
 use tor_bytes::Error as BytesError;
 use tor_bytes::Result as BytesResult;
 use tor_bytes::{Readable, Reader, Writeable, Writer};
+use tor_error::internal;
 
 use std::convert::TryInto;
 use std::net::IpAddr;
@@ -89,8 +90,12 @@ impl SocksHandshake {
             (State::Initial, v) => Err(Error::BadProtocol(v)),
             (State::Socks5Username, 1) => self.s5_uname(input),
             (State::Socks5Wait, 5) => self.s5(input),
-            (State::Done, _) => Err(Error::AlreadyFinished),
+            (State::Done, _) => Err(Error::AlreadyFinished(internal!(
-            (State::Failed, _) => Err(Error::AlreadyFinished),
+                "called handshake() after handshaking was done"
+            ))),
+            (State::Failed, _) => Err(Error::AlreadyFinished(internal!(
+                "called handshake() after handshaking failed"
+            ))),
             (_, _) => Err(Error::Syntax),
         };
         match rv {
@@ -108,7 +113,10 @@ impl SocksHandshake {
         let mut r = Reader::from_slice(input);
         let version = r.take_u8()?.try_into()?;
         if version != SocksVersion::V4 {
-            return Err(Error::Internal);
+            return Err(Error::Internal(internal!(
+                "called s4 on wrong type {:?}",
+                version
+            )));
         }
 
         let cmd: SocksCmd = r.take_u8()?.into();
@@ -149,7 +157,10 @@ impl SocksHandshake {
         let mut r = Reader::from_slice(input);
         let version: SocksVersion = r.take_u8()?.try_into()?;
         if version != SocksVersion::V5 {
-            return Err(Error::Internal);
+            return Err(Error::Internal(internal!(
+                "called on wrong handshake type {:?}",
+                version
+            )));
         }
 
         /// Constant for Username/Password-style authentication.
@@ -209,14 +220,20 @@ impl SocksHandshake {
 
         let version: SocksVersion = r.take_u8()?.try_into()?;
         if version != SocksVersion::V5 {
-            return Err(Error::Internal);
+            return Err(Error::Internal(internal!(
+                "called s5 on non socks5 handshake with type {:?}",
+                version
+            )));
         }
         let cmd = r.take_u8()?.into();
         let _ignore = r.take_u8()?;
         let addr = r.extract()?;
         let port = r.take_u16()?;
 
-        let auth = self.socks5_auth.take().ok_or(Error::Internal)?;
+        let auth = self
+            .socks5_auth
+            .take()
+            .ok_or_else(|| Error::Internal(internal!("called s5 without negotiating auth")))?;
 
         let request = SocksRequest::new(version, cmd, addr, port, auth)?;
 
@@ -552,13 +569,13 @@ mod test {
         let r = h.handshake(&hex!("06 01 00"));
         assert!(r.is_err());
         let r = h.handshake(good_socks4a);
-        assert!(matches!(r, Err(Error::AlreadyFinished)));
+        assert!(matches!(r, Err(Error::AlreadyFinished(_))));
 
         // Can't try again after success
         let mut h = SocksHandshake::new();
         let r = h.handshake(good_socks4a);
         assert!(r.is_ok());
         let r = h.handshake(good_socks4a);
-        assert!(matches!(r, Err(Error::AlreadyFinished)));
+        assert!(matches!(r, Err(Error::AlreadyFinished(_))));
     }
 }