Many interfaces such as Listeners::add_tcp take a &str for the IP address, and rely on into/try_into conversions (sometimes unwrap-ing, such as BasicPeer::new) to convert to pingora-core::SocketAddr or std::net::SocketAddr.
Ideally we would not unwrap: this means that river will want to pre-validate the IP address to avoid runtime panics. However, it might be better to explicitly take the intended internal format, e.g. pingora-core::SocketAddr, or T: Into<pingora-core::SocketAddr>, to avoid cases where frontends like river use different parsing or validation logic to pingora, which may lead to bugs.
Security bugs have happened in the past (at least for URLs/URIs and paths, not sure about IP Addresses) where different components used different validation/parsing rules, e.g. different levels of allowed escaping, even if this is not exploitable it might cause unexpected panics if the config has an ip address format that river allows but pingora rejects.
Additional context
This is similar to #182, basically everywhere "stringly typed data" (that isn't actually a text string) is used, it should be pushed out to the edge and validated immediately, potentially removing it entirely from pingora/pingora-*, and handled ONLY in the frontend/configuration handling crates.
Describe the bug
Many interfaces such as
Listeners::add_tcptake a&strfor the IP address, and rely oninto/try_intoconversions (sometimesunwrap-ing, such asBasicPeer::new) to convert topingora-core::SocketAddrorstd::net::SocketAddr.Ideally we would not
unwrap: this means thatriverwill want to pre-validate the IP address to avoid runtime panics. However, it might be better to explicitly take the intended internal format, e.g.pingora-core::SocketAddr, orT: Into<pingora-core::SocketAddr>, to avoid cases where frontends likeriveruse different parsing or validation logic topingora, which may lead to bugs.Security bugs have happened in the past (at least for URLs/URIs and paths, not sure about IP Addresses) where different components used different validation/parsing rules, e.g. different levels of allowed escaping, even if this is not exploitable it might cause unexpected panics if the config has an ip address format that river allows but pingora rejects.
Additional context
This is similar to #182, basically everywhere "stringly typed data" (that isn't actually a text string) is used, it should be pushed out to the edge and validated immediately, potentially removing it entirely from
pingora/pingora-*, and handled ONLY in the frontend/configuration handling crates.