From c85a74194f763b4b087e2368148f8803c09bfa11 Mon Sep 17 00:00:00 2001 From: zonyitoo Date: Sun, 17 Nov 2024 01:18:05 +0800 Subject: [PATCH] feat(shadowsocks): ServerConfig::new returns Result - fix #1770 - allow users to check errors instead of just panic on wrong passwords --- Cargo.lock | 6 +- Cargo.toml | 4 +- crates/shadowsocks-service/Cargo.toml | 4 +- crates/shadowsocks-service/src/config.rs | 24 +++- .../shadowsocks-service/src/manager/server.rs | 8 +- crates/shadowsocks/Cargo.toml | 2 +- crates/shadowsocks/src/config.rs | 109 ++++++++---------- crates/shadowsocks/tests/tcp.rs | 2 +- crates/shadowsocks/tests/tcp_tfo.rs | 2 +- crates/shadowsocks/tests/udp.rs | 2 +- src/service/local.rs | 7 +- src/service/server.rs | 7 +- tests/socks4.rs | 16 +-- tests/socks5.rs | 16 +-- tests/udp.rs | 16 +-- 15 files changed, 116 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc167f20..3b0e5f7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3221,7 +3221,7 @@ dependencies = [ [[package]] name = "shadowsocks" -version = "1.21.1" +version = "1.22.0" dependencies = [ "aes", "arc-swap", @@ -3287,7 +3287,7 @@ dependencies = [ [[package]] name = "shadowsocks-rust" -version = "1.21.3" +version = "1.22.0" dependencies = [ "base64 0.22.1", "build-time", @@ -3328,7 +3328,7 @@ dependencies = [ [[package]] name = "shadowsocks-service" -version = "1.21.3" +version = "1.22.0" dependencies = [ "arc-swap", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index e51055e1..3492fe12 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "shadowsocks-rust" -version = "1.21.3" +version = "1.22.0" authors = ["Shadowsocks Contributors"] description = "shadowsocks is a fast tunnel proxy that helps you bypass firewalls." repository = "https://github.com/shadowsocks/shadowsocks-rust" @@ -260,7 +260,7 @@ jemallocator = { version = "0.5", optional = true } snmalloc-rs = { version = "0.3", optional = true } rpmalloc = { version = "0.2", optional = true } -shadowsocks-service = { version = "1.21.3", path = "./crates/shadowsocks-service" } +shadowsocks-service = { version = "1.22.0", path = "./crates/shadowsocks-service" } windows-service = { version = "0.7", optional = true } diff --git a/crates/shadowsocks-service/Cargo.toml b/crates/shadowsocks-service/Cargo.toml index 18f9781c..0ae5753d 100644 --- a/crates/shadowsocks-service/Cargo.toml +++ b/crates/shadowsocks-service/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "shadowsocks-service" -version = "1.21.3" +version = "1.22.0" authors = ["Shadowsocks Contributors"] description = "shadowsocks is a fast tunnel proxy that helps you bypass firewalls." repository = "https://github.com/shadowsocks/shadowsocks-rust" @@ -215,7 +215,7 @@ serde = { version = "1.0", features = ["derive"] } json5 = "0.4" bson = { version = "2.13.0", optional = true } -shadowsocks = { version = "1.21.1", path = "../shadowsocks", default-features = false } +shadowsocks = { version = "1.22.0", path = "../shadowsocks", default-features = false } # Just for the ioctl call macro [target.'cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd", target_os = "openbsd"))'.dependencies] diff --git a/crates/shadowsocks-service/src/config.rs b/crates/shadowsocks-service/src/config.rs index 4586b0c8..57a4d1f0 100644 --- a/crates/shadowsocks-service/src/config.rs +++ b/crates/shadowsocks-service/src/config.rs @@ -1935,7 +1935,17 @@ impl Config { } }; - let mut nsvr = ServerConfig::new(addr, password, method); + let mut nsvr = match ServerConfig::new(addr, password, method) { + Ok(svr) => svr, + Err(serr) => { + let err = Error::new( + ErrorKind::Malformed, + "server config create failed", + Some(format!("{}", serr)), + ); + return Err(err); + } + }; nsvr.set_source(server_source); nsvr.set_mode(global_mode); @@ -2055,7 +2065,17 @@ impl Config { } }; - let mut nsvr = ServerConfig::new(addr, password, method); + let mut nsvr = match ServerConfig::new(addr, password, method) { + Ok(svr) => svr, + Err(serr) => { + let err = Error::new( + ErrorKind::Malformed, + "server config create failed", + Some(format!("{}", serr)), + ); + return Err(err); + } + }; nsvr.set_source(server_source); // Extensible Identity Header, Users diff --git a/crates/shadowsocks-service/src/manager/server.rs b/crates/shadowsocks-service/src/manager/server.rs index 62359324..81fc18d2 100644 --- a/crates/shadowsocks-service/src/manager/server.rs +++ b/crates/shadowsocks-service/src/manager/server.rs @@ -479,7 +479,13 @@ impl Manager { None => return Ok(AddResponse("method is required")), }; - let mut svr_cfg = ServerConfig::new(addr, req.password.clone(), method); + let mut svr_cfg = match ServerConfig::new(addr, req.password.clone(), method) { + Ok(svr_cfg) => svr_cfg, + Err(err) => { + error!("failed to create ServerConfig, error: {}", err); + return Ok(AddResponse(format!("invalid server"))); + } + }; if let Some(ref plugin) = req.plugin { let p = PluginConfig { diff --git a/crates/shadowsocks/Cargo.toml b/crates/shadowsocks/Cargo.toml index 49f1365d..25dc8c71 100644 --- a/crates/shadowsocks/Cargo.toml +++ b/crates/shadowsocks/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "shadowsocks" -version = "1.21.1" +version = "1.22.0" authors = ["Shadowsocks Contributors"] description = "shadowsocks is a fast tunnel proxy that helps you bypass firewalls." repository = "https://github.com/shadowsocks/shadowsocks-rust" diff --git a/crates/shadowsocks/src/config.rs b/crates/shadowsocks/src/config.rs index 8e475145..dbd13ba9 100644 --- a/crates/shadowsocks/src/config.rs +++ b/crates/shadowsocks/src/config.rs @@ -4,7 +4,6 @@ use std::path::PathBuf; use std::{ collections::HashMap, - error, fmt::{self, Debug, Display}, net::SocketAddr, str::{self, FromStr}, @@ -381,6 +380,18 @@ pub enum ServerSource { OnlineConfig, //< Created from online configuration (SIP008) } +/// Errors when creating a new ServerConfig +#[derive(Debug, Clone, Error)] +pub enum ServerConfigError { + /// Invalid base64 encoding of password + #[error("invalid key encoding for {0}, {1}")] + InvalidKeyEncoding(CipherKind, base64::DecodeError), + + /// Key length mismatch + #[error("invalid key length for {0}, expecting {1} bytes, but found {2} bytes")] + InvalidKeyLength(CipherKind, usize, usize), +} + /// Configuration for a server #[derive(Clone, Debug)] pub struct ServerConfig { @@ -426,29 +437,23 @@ pub struct ServerConfig { } #[inline] -fn make_derived_key(method: CipherKind, password: &str, enc_key: &mut [u8]) { +fn make_derived_key(method: CipherKind, password: &str, enc_key: &mut [u8]) -> Result<(), ServerConfigError> { #[cfg(feature = "aead-cipher-2022")] if method.is_aead_2022() { // AEAD 2022 password is a base64 form of enc_key match AEAD2022_PASSWORD_BASE64_ENGINE.decode(password) { Ok(v) => { if v.len() != enc_key.len() { - panic!( - "{} is expecting a {} bytes key, but password: {} ({} bytes after decode)", - method, - enc_key.len(), - password, - v.len() - ); + return Err(ServerConfigError::InvalidKeyLength(method, enc_key.len(), v.len())); } enc_key.copy_from_slice(&v); } Err(err) => { - panic!("{method} password {password} is not base64 encoded, error: {err}"); + return Err(ServerConfigError::InvalidKeyEncoding(method, err)); } } - return; + return Ok(()); } cfg_if! { @@ -462,6 +467,8 @@ fn make_derived_key(method: CipherKind, password: &str, enc_key: &mut [u8]) { unreachable!("{method} don't know how to make a derived key"); } } + + Ok(()) } /// Check if method supports Extended Identity Header @@ -476,7 +483,7 @@ pub fn method_support_eih(method: CipherKind) -> bool { ) } -fn password_to_keys

(method: CipherKind, password: P) -> (String, Box<[u8]>, Vec) +fn password_to_keys

(method: CipherKind, password: P) -> Result<(String, Box<[u8]>, Vec), ServerConfigError> where P: Into, { @@ -491,7 +498,7 @@ where warn!("method \"none\" doesn't need a password, which should be set as an empty String, but password.len() = {}", password.len()); } - return (password, Vec::new().into_boxed_slice(), Vec::new()); + return Ok((password, Vec::new().into_boxed_slice(), Vec::new())); } #[cfg(feature = "stream-cipher")] @@ -499,7 +506,7 @@ where // TABLE cipher doesn't need key derivation. // Reference implemenation: shadowsocks-libev, shadowsocks (Python) let enc_key = password.clone().into_bytes().into_boxed_slice(); - return (password, enc_key, Vec::new()); + return Ok((password, enc_key, Vec::new())); } #[allow(unreachable_patterns)] @@ -518,7 +525,7 @@ where let upsk = split_iter.next().expect("uPSK"); let mut enc_key = vec![0u8; method.key_len()].into_boxed_slice(); - make_derived_key(method, upsk, &mut enc_key); + make_derived_key(method, upsk, &mut enc_key)?; for ipsk in split_iter { match USER_KEY_BASE64_ENGINE.decode(ipsk) { @@ -533,25 +540,25 @@ where identity_keys.reverse(); - return (upsk.to_owned(), enc_key, identity_keys); + return Ok((upsk.to_owned(), enc_key, identity_keys)); } let mut enc_key = vec![0u8; method.key_len()].into_boxed_slice(); - make_derived_key(method, &password, &mut enc_key); + make_derived_key(method, &password, &mut enc_key)?; - (password, enc_key, Vec::new()) + Ok((password, enc_key, Vec::new())) } impl ServerConfig { /// Create a new `ServerConfig` - pub fn new(addr: A, password: P, method: CipherKind) -> ServerConfig + pub fn new(addr: A, password: P, method: CipherKind) -> Result where A: Into, P: Into, { - let (password, enc_key, identity_keys) = password_to_keys(method, password); + let (password, enc_key, identity_keys) = password_to_keys(method, password)?; - ServerConfig { + Ok(ServerConfig { addr: addr.into(), password, method, @@ -566,21 +573,23 @@ impl ServerConfig { mode: Mode::TcpAndUdp, // Server serves TCP & UDP by default weight: ServerWeight::new(), source: ServerSource::Default, - } + }) } /// Set encryption method - pub fn set_method

(&mut self, method: CipherKind, password: P) + pub fn set_method

(&mut self, method: CipherKind, password: P) -> Result<(), ServerConfigError> where P: Into, { self.method = method; - let (password, enc_key, identity_keys) = password_to_keys(method, password); + let (password, enc_key, identity_keys) = password_to_keys(method, password)?; self.password = password; self.enc_key = enc_key; self.identity_keys = Arc::new(identity_keys); + + Ok(()) } /// Set plugin @@ -912,7 +921,7 @@ impl ServerConfig { return Err(UrlParseError::InvalidMethod); } }; - let mut svrconfig = ServerConfig::new(addr, pwd, method); + let mut svrconfig = ServerConfig::new(addr, pwd, method)?; if let Some(q) = parsed.query() { let query = match serde_urlencoded::from_bytes::>(q.as_bytes()) { @@ -961,52 +970,26 @@ impl ServerConfig { } /// Shadowsocks URL parsing Error -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Error)] pub enum UrlParseError { - ParseError(url::ParseError), + #[error("{0}")] + ParseError(#[from] url::ParseError), + #[error("URL must have \"ss://\" scheme")] InvalidScheme, + #[error("unknown encryption method")] InvalidMethod, + #[error("invalid user info")] InvalidUserInfo, + #[error("missing host")] MissingHost, + #[error("invalid authentication info")] InvalidAuthInfo, + #[error("invalid server address")] InvalidServerAddr, + #[error("invalid query string")] InvalidQueryString, -} - -impl From for UrlParseError { - fn from(err: url::ParseError) -> UrlParseError { - UrlParseError::ParseError(err) - } -} - -impl fmt::Display for UrlParseError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - UrlParseError::ParseError(ref err) => fmt::Display::fmt(err, f), - UrlParseError::InvalidScheme => write!(f, "URL must have \"ss://\" scheme"), - UrlParseError::InvalidMethod => write!(f, "unknown encryption method"), - UrlParseError::InvalidUserInfo => write!(f, "invalid user info"), - UrlParseError::MissingHost => write!(f, "missing host"), - UrlParseError::InvalidAuthInfo => write!(f, "invalid authentication info"), - UrlParseError::InvalidServerAddr => write!(f, "invalid server address"), - UrlParseError::InvalidQueryString => write!(f, "invalid query string"), - } - } -} - -impl error::Error for UrlParseError { - fn source(&self) -> Option<&(dyn error::Error + 'static)> { - match *self { - UrlParseError::ParseError(ref err) => Some(err as &dyn error::Error), - UrlParseError::InvalidScheme => None, - UrlParseError::InvalidMethod => None, - UrlParseError::InvalidUserInfo => None, - UrlParseError::MissingHost => None, - UrlParseError::InvalidAuthInfo => None, - UrlParseError::InvalidServerAddr => None, - UrlParseError::InvalidQueryString => None, - } - } + #[error("{0}")] + ServerConfigError(#[from] ServerConfigError), } impl FromStr for ServerConfig { diff --git a/crates/shadowsocks/tests/tcp.rs b/crates/shadowsocks/tests/tcp.rs index 6782723e..cc28c26e 100644 --- a/crates/shadowsocks/tests/tcp.rs +++ b/crates/shadowsocks/tests/tcp.rs @@ -90,7 +90,7 @@ async fn tcp_tunnel_example( password: &str, method: CipherKind, ) -> io::Result<()> { - let svr_cfg_server = ServerConfig::new(server_addr, password, method); + let svr_cfg_server = ServerConfig::new(server_addr, password, method).unwrap(); let svr_cfg_local = svr_cfg_server.clone(); let ctx_server = Context::new_shared(ServerType::Server); diff --git a/crates/shadowsocks/tests/tcp_tfo.rs b/crates/shadowsocks/tests/tcp_tfo.rs index 3fac4d71..b14e8cfb 100644 --- a/crates/shadowsocks/tests/tcp_tfo.rs +++ b/crates/shadowsocks/tests/tcp_tfo.rs @@ -32,7 +32,7 @@ use tokio::{ async fn tcp_tunnel_tfo() { let _ = env_logger::try_init(); - let svr_cfg = ServerConfig::new(("127.0.0.1", 41000), "", CipherKind::NONE); + let svr_cfg = ServerConfig::new(("127.0.0.1", 41000), "", CipherKind::NONE).unwrap(); let svr_cfg_client = svr_cfg.clone(); tokio::spawn(async move { diff --git a/crates/shadowsocks/tests/udp.rs b/crates/shadowsocks/tests/udp.rs index 13afcf8b..de211036 100644 --- a/crates/shadowsocks/tests/udp.rs +++ b/crates/shadowsocks/tests/udp.rs @@ -60,7 +60,7 @@ async fn udp_tunnel_echo( password: &str, method: CipherKind, ) -> io::Result<()> { - let svr_cfg_server = ServerConfig::new(server_addr, password, method); + let svr_cfg_server = ServerConfig::new(server_addr, password, method).unwrap(); let svr_cfg_local = svr_cfg_server.clone(); let ctx_server = Context::new_shared(ServerType::Server); diff --git a/src/service/local.rs b/src/service/local.rs index 8e108e70..a3752fdb 100644 --- a/src/service/local.rs +++ b/src/service/local.rs @@ -653,7 +653,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future().expect("server-addr"); let timeout = matches.get_one::("TIMEOUT").map(|x| Duration::from_secs(*x)); - let mut sc = ServerConfig::new(svr_addr, password, method); + let mut sc = match ServerConfig::new(svr_addr, password, method) { + Ok(sc) => sc, + Err(err) => { + panic!("failed to create ServerConfig, error: {}", err); + } + }; sc.set_source(ServerSource::CommandLine); if let Some(timeout) = timeout { sc.set_timeout(timeout); diff --git a/src/service/server.rs b/src/service/server.rs index d5b49fe0..ad30773b 100644 --- a/src/service/server.rs +++ b/src/service/server.rs @@ -355,7 +355,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future().expect("server-addr"); let timeout = matches.get_one::("TIMEOUT").map(|x| Duration::from_secs(*x)); - let mut sc = ServerConfig::new(svr_addr, password, method); + let mut sc = match ServerConfig::new(svr_addr, password, method) { + Ok(sc) => sc, + Err(err) => { + panic!("failed to create ServerConfig, error: {}", err); + } + }; if let Some(timeout) = timeout { sc.set_timeout(timeout); } diff --git a/tests/socks4.rs b/tests/socks4.rs index cdce51f5..1c1b7037 100644 --- a/tests/socks4.rs +++ b/tests/socks4.rs @@ -39,11 +39,9 @@ impl Socks4TestServer { local_addr, svr_config: { let mut cfg = Config::new(ConfigType::Server); - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - svr_addr, - pwd.to_owned(), - method, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(svr_addr, pwd.to_owned(), method).unwrap(), + )]; cfg }, cli_config: { @@ -52,11 +50,9 @@ impl Socks4TestServer { ServerAddr::from(local_addr), ProtocolType::Socks, ))]; - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - svr_addr, - pwd.to_owned(), - method, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(svr_addr, pwd.to_owned(), method).unwrap(), + )]; cfg }, } diff --git a/tests/socks5.rs b/tests/socks5.rs index 74e6034e..0075c0d0 100644 --- a/tests/socks5.rs +++ b/tests/socks5.rs @@ -40,11 +40,9 @@ impl Socks5TestServer { local_addr, svr_config: { let mut cfg = Config::new(ConfigType::Server); - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - svr_addr, - pwd.to_owned(), - method, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(svr_addr, pwd.to_owned(), method).unwrap(), + )]; cfg.server[0] .config .set_mode(if enable_udp { Mode::TcpAndUdp } else { Mode::TcpOnly }); @@ -57,11 +55,9 @@ impl Socks5TestServer { ProtocolType::Socks, ))]; cfg.local[0].config.mode = if enable_udp { Mode::TcpAndUdp } else { Mode::TcpOnly }; - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - svr_addr, - pwd.to_owned(), - method, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(svr_addr, pwd.to_owned(), method).unwrap(), + )]; cfg }, } diff --git a/tests/udp.rs b/tests/udp.rs index acccb762..06caabeb 100644 --- a/tests/udp.rs +++ b/tests/udp.rs @@ -22,11 +22,9 @@ const METHOD: CipherKind = CipherKind::AES_128_GCM; fn get_svr_config() -> Config { let mut cfg = Config::new(ConfigType::Server); - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - SERVER_ADDR.parse::().unwrap(), - PASSWORD.to_owned(), - METHOD, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(SERVER_ADDR.parse::().unwrap(), PASSWORD.to_owned(), METHOD).unwrap(), + )]; cfg.server[0].config.set_mode(Mode::TcpAndUdp); cfg } @@ -38,11 +36,9 @@ fn get_cli_config() -> Config { ProtocolType::Socks, ))]; cfg.local[0].config.mode = Mode::TcpAndUdp; - cfg.server = vec![ServerInstanceConfig::with_server_config(ServerConfig::new( - SERVER_ADDR.parse::().unwrap(), - PASSWORD.to_owned(), - METHOD, - ))]; + cfg.server = vec![ServerInstanceConfig::with_server_config( + ServerConfig::new(SERVER_ADDR.parse::().unwrap(), PASSWORD.to_owned(), METHOD).unwrap(), + )]; cfg }