Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IP addresses are often specified as String/&str #183

Open
jamesmunns opened this issue Apr 4, 2024 · 0 comments
Open

IP addresses are often specified as String/&str #183

jamesmunns opened this issue Apr 4, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request

Comments

@jamesmunns
Copy link
Contributor

jamesmunns commented Apr 4, 2024

Describe the bug

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.

@drcaramelsyrup drcaramelsyrup added the enhancement New feature or request label Apr 4, 2024
@andrewhavck andrewhavck self-assigned this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants