-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WASI] Dynamic SocketAddr
check for outbound socket traffic
#7662
[WASI] Dynamic SocketAddr
check for outbound socket traffic
#7662
Conversation
The user can now specify a closure which gets run any time a connection to an external address is attempted. If the closure returns true, the address is allowed, but if it returns false, the underlying pool is then checked. In otherwords, false does not imply denied, but rather, check the underlyin pool. Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Could you expand more on the use case for this? |
@alexcrichton sorry I should have been more clear with use cases this supports! I see several use cases for this: Use casesConvenience for complex checksSome checks are easy to express in code but complicated to express as blocks of IP addresses. For example, allowing only non-private IP v4 network traffic is trivial as a predicate function but really difficult as a collection of CIDR blocks: ctx.dynamic_socket_addr_check(|a| match a.ip() {
IpAddr::V4(i) => !i.is_private(),
_ => false
}); Dynamic checks that change over timeTypically the ctx.dynamic_socket_addr_check(|a| {
if let Ok(true) = std::env::var("ALLOW_LOOPBACK")
a.ip().is_loopback()
} else {
false
}
}); ConcernsSupporting directly in
|
Ok those use cases make sense to me, and I can see how it's not quite as easy to map those to a Perhaps though I could propose an alternative direction to this PR: what if
Does that sound reasonable? |
Sounds reasonable to me. This will certainly make the API much simpler as there isn't as much of a need to mirror the |
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
I also want to cc @badeend and @sunfishcode on this as I suspect one or both of y'all were involved in originally adding Pool
. Wanted to make sure to run this by you to double-check that I'm not missing anything by considering this a workable alternative.
I think decoupling the network policies from the actual syscall implementations is a good thing. I already ran into the limitations of At the same time, this change throws every address on one big pile (called |
When I wrote |
Perhaps a second argument could be added to the callback along the lines of: enum Reason {
TcpBind,
TcpConnect,
UdpSend,
...
} to help distinguish why the callback is being invoked? |
I've added a |
Looks fine to me 👍 |
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
e4bb400
to
345aa6d
Compare
This PR adds the ability to do dynamic checks of whether to allow outbound socket traffic to a given
SocketAddr
. The checks are purely additive over the underlyingcap_std::net::Pool
. If the check returnstrue
the traffic is allowed, while if it returns false, the underlyingcap_std::net::Pool
is then checked.This also changes the
WasiCtx::inherit_network
function to not take anAmbientAuthority
since all other methods onWasiCtx
do not take this value.