hook-worker: deny traffic to internal IPs and IPv6 #35
Conversation
| /// Returns [`true`] if the address appears to be a globally reachable IPv4. | ||
| /// | ||
| /// Trimmed down version of the unstable IpAddr::is_global, move to it when it's stable. | ||
| fn is_global_ipv4(addr: &SocketAddr) -> bool { |
There was a problem hiding this comment.
As clear as it gets, very nice 👍
|
|
||
| // Execute the blocking call in a separate worker thread then process its result asynchronously. | ||
| // spawn_blocking returns a JoinHandle that implements Future<Result<(closure result), JoinError>>. | ||
| let future_result = spawn_blocking(resolve_host).map(|result| match result { |
There was a problem hiding this comment.
Any risk of running out of blocking threads? Should we cache these resolutions in some way?
There was a problem hiding this comment.
-
spawn_blocking's doc indicates that a thread poll is used, and tasks are queued if no thread is available. The default pool max size of 512 is way higher than our concurrency. That's also what the default resolver does.
-
The glibc already handles caching, so caching again here would be a premature optimization. Also, it makes debugging harder: I spend weeks working around grpc-java's internal DNS caching creating errors when services rolled out.
There was a problem hiding this comment.
I had this skeleton with all but the (sync) IP range blocking logic decided, where I stole from the reqwest GaiResolver and didn't use spawn_blocking: 825a9bc
🤷 If it's helpful, maybe I'm missing something there, I haven't woken up yet.
There was a problem hiding this comment.
HyperGaiResolver::call boils down to tokio::task::spawn_blocking(move || { (&*name.host, 0).to_socket_addrs().map(|i| SocketAddrs { iter: i }), which is what I inlined here instead of adding another dependency.
I'm open to erroring out instead of filtering out, which could help with debugging. That would probably require us to implement filtering out private IPV6 too while it's not needed.
| #[envconfig(default = "false")] | ||
| pub allow_internal_ips: bool, |
There was a problem hiding this comment.
Would this ever be true?
There was a problem hiding this comment.
end2end tests against containerized endpoint, local testing, hobby deploy
xvello
force-pushed
the
xvello/privateip
branch
2 times, most recently
from
61a25ce to
34cca52
Compare
xvello
changed the title
Because we preemptively filter out ipv6 addresses, deploying this service on an IPV6-enabled cluster will be a no-op instead of opening a potential security issue. We'll then schedule the work to safely check IPv6 records and enable support for IPv6-only destinations. I think it would be fine to do that work right now if we want, I just wanted to reduce scope to be able to ship this before going out-of-office |
Thanks, that makes sense. I have some very old PTSD around |
|
The I think that you are mentioning the fact that |
Make sure that requests to our internal systems cannot be executed through the hook delivery system. In order to be resilient to DNS rebinding attacks, we need to insert our validation logic between the DNS resolution and the HTTP request execution. In this PR, I am wrapping reqwest's
Resolvinginterface to intercept and validate the IPs returned by the system's DNS resolver.PublicIPv4Resolver, that copies hyper'sGaiResolverlogic + filtering of the results viais_global_ipv4NoPublicIPv4Errorif the filtered lists is empty, making sure we don't get hard to diagnose "network unreachable" errors. Make sure that this error is surfaced in logs