Stream: git-wasmtime

Topic: wasmtime / PR #7662 [WASI] Dynamic `SocketAddr` check for...

Wasmtime GitHub notifications bot (Dec 08 2023 at 17:29):

rylev requested alexcrichton for a review on PR #7662.

Wasmtime GitHub notifications bot (Dec 08 2023 at 17:29):

rylev opened PR #7662 from rylev:dynamic-socket-addr-check to bytecodealliance:main:

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 underlying cap_std::net::Pool. If the check returns true the traffic is allowed, while if it returns false, the underlying cap_std::net::Pool is then checked.

This also changes the WasiCtx::inherit_network function to not take an AmbientAuthority since all other methods on WasiCtx do not take this value.

Wasmtime GitHub notifications bot (Dec 08 2023 at 17:29):

rylev requested wasmtime-core-reviewers for a review on PR #7662.

Wasmtime GitHub notifications bot (Dec 08 2023 at 18:26):

alexcrichton commented on PR #7662:

Could you expand more on the use case for this? Pool seems to be trying to be pretty full-featured to avoid the need for a dynamic check such as this, so ideally we'd lean on that. This design also raises questions such as what happens when the method is called twice (overwriting seems a bit surprising to me) and what to do when false is returned (falling back on other logic seems non-obvious to me).

Wasmtime GitHub notifications bot (Dec 11 2023 at 10:11):

rylev commented on PR #7662:

@alexcrichton sorry I should have been more clear with use cases this supports! I see several use cases for this:

Use cases

Convenience for complex checks

Some 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 time

Typically the Pool of IP addresses that are supported is instantiated before any component is run. This means that more dynamic use cases are either not possible or much hard to achieve. For example, allowing the environment to control some aspects of what IP addresses are allowed:

ctx.dynamic_socket_addr_check(|a| {
    if let Ok(true) = std::env::var("ALLOW_LOOPBACK")
       a.ip().is_loopback()
    } else {
      false
   }
});

Concerns

Supporting directly in Pool

I agree that adding this support to Pool might make more sense but how exactly to do that does not seem super straight forward. Since the only user facing change here is the dynamic_socket_addr_check, we could potentially punt on adding this to Pool if we're happy with the user facing API.

Multiple Calls

I agree, it's a bit strange that calling this function multiple times overrides the previous calls. There is some precdent of this already in that WasiCtx::inherit_network sort of overrides previous invocations though one could argue that it doesn't invalidate them, it just makes them redundant. Would storing these callbacks in a Vec or SmallVec make sense and be worth the cost of potentially more allocation?

Behavior of false

I understand that false meaning "do other checks" isn't the most intuitive. It does make sense if the predicate is thought of only as "are we allowing this traffic" vs "are we allowing or denying this traffic". The status quo is the former of these two options and mirrors the behavior of Pool (i.e., adding an IP range to Pool does not mean that if an IP falls out of that range, the traffic will be denied. There may be some other range that allows that IP.).

We could work around this completely by changing the closure to return a return type that is a bit more explicit:

enum IpCheck {
  // This is the equivalent of returning `true` in current scheme
  Allow,
  // Being able to deny explicitly in these dynamic checks is new
  Deny,
  // This is the equivalent of returning `false` in current scheme
  Passthrough
}

This makes these more explicit at the cost of ultimately be more complicated (each closure has three possible outcomes as opposed to two).

Wasmtime GitHub notifications bot (Dec 11 2023 at 16:14):

alexcrichton commented on PR #7662:

Ok those use cases make sense to me, and I can see how it's not quite as easy to map those to a Pool.

Perhaps though I could propose an alternative direction to this PR: what if Pool were removed entirely in favor of just a callback? That way a WasiCtx would store a single "is this IP address allowed" callback. That seems like it might be able to capture the best of both worlds:

• No need to consider interactions of callbacks-and-Pool, there's only callbacks.
• Users who want to keep using Pool still easily can.
• No need to worry about multiple callbacks, there's only one (and it's documented as only one).

Does that sound reasonable?

Wasmtime GitHub notifications bot (Dec 11 2023 at 16:38):

rylev commented on PR #7662:

Sounds reasonable to me. This will certainly make the API much simpler as there isn't as much of a need to mirror the Pool API on WasiCtx. I'll give that a try.

Wasmtime GitHub notifications bot (Dec 12 2023 at 13:51):

rylev updated PR #7662.

Wasmtime GitHub notifications bot (Dec 12 2023 at 15:22):

alexcrichton submitted PR review:

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.

Wasmtime GitHub notifications bot (Dec 12 2023 at 18:15):

badeend commented on PR #7662:

I think decoupling the network policies from the actual syscall implementations is a good thing. I already ran into the limitations of Pool and PoolExt multiple times myself. Which is (one of) the reasons why the current implementation ended up using rustix' connect and tokio's send & recv directly, instead of going through cap-std's connect_existing_udp_socket & send_to_udp_socket_addr.

At the same time, this change throws every address on one big pile (called check) and looses the context in which the address is about to be used. Eg. inbound vs outbound, local vs remote, etc. Not a big problem for now, but maybe this could be extended in the future with check_for_tcp_bind, check_for_tcp_connect, check_for_udp_send, ..

Wasmtime GitHub notifications bot (Dec 12 2023 at 18:38):

sunfishcode commented on PR #7662:

When I wrote Pool, my thought was that having the logic for IP namespace sandboxing factored out into an external crate would make that code easier to test and audit. My idea was that we'd replace Pool's initial simplistic implementation once we knew what we needed. In practice, it seems it's been more straightforward to just implement the logic in Wasmtime rather than to modify Pool. In theory I may look into factoring out the code we write in Wasmtime back out into an external crate some day, but I don't think that's something we need to block on here.

Wasmtime GitHub notifications bot (Dec 12 2023 at 19:08):

alexcrichton commented on PR #7662:

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?

Wasmtime GitHub notifications bot (Dec 12 2023 at 22:27):

rylev updated PR #7662.

Wasmtime GitHub notifications bot (Dec 12 2023 at 22:53):

rylev commented on PR #7662:

I've added a SocketAddrUse argument to the closure check. Let me know if that looks good including naming.

Wasmtime GitHub notifications bot (Dec 13 2023 at 08:42):

badeend commented on PR #7662:

Looks fine to me :+1:

Wasmtime GitHub notifications bot (Dec 13 2023 at 09:44):

rylev requested alexcrichton for a review on PR #7662.

Wasmtime GitHub notifications bot (Dec 13 2023 at 15:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2023 at 18:28):

rylev updated PR #7662.

Wasmtime GitHub notifications bot (Dec 13 2023 at 18:28):

rylev requested alexcrichton for a review on PR #7662.

Wasmtime GitHub notifications bot (Dec 13 2023 at 19:19):

alexcrichton merged PR #7662.

Last updated: Dec 23 2024 at 12:05 UTC