diff --git a/Cargo.lock b/Cargo.lock index 7732ead28..039a70c02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3312,6 +3312,7 @@ dependencies = [ "hex-literal", "thiserror", "tor-bytes", + "tor-error", ] [[package]] diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs index 36e2f7e80..0eea384e1 100644 --- a/crates/tor-error/src/lib.rs +++ b/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 diff --git a/crates/tor-socksproto/Cargo.toml b/crates/tor-socksproto/Cargo.toml index 627747d27..16f9c05ae 100644 --- a/crates/tor-socksproto/Cargo.toml +++ b/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" diff --git a/crates/tor-socksproto/src/err.rs b/crates/tor-socksproto/src/err.rs index 7648c646a..bcf6eddf7 100644 --- a/crates/tor-socksproto/src/err.rs +++ b/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> or a Result> ? + todo!() + } + E::Syntax | E::BadProtocol(_) => EK::ProtocolViolation, + E::NoSupport => EK::NoSupport, + E::AlreadyFinished(_) => EK::Internal, + E::Internal(_) => EK::Internal, + } + } } impl From for Error { diff --git a/crates/tor-socksproto/src/handshake.rs b/crates/tor-socksproto/src/handshake.rs index b4f4ff58c..414231082 100644 --- a/crates/tor-socksproto/src/handshake.rs +++ b/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::Failed, _) => Err(Error::AlreadyFinished), + (State::Done, _) => Err(Error::AlreadyFinished(internal!( + "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(_)))); } }