Stream: wasi

Topic: Network pool subtractions

Ryan Levick (rylev) (Dec 06 2023 at 14:21):

@Dan Gohman I'm looking to limit the IP pool of addresses that users of my runtime can connect to. Rather than building up a pool of allowed IpGrants, I'd love to be able to grant all access and then subtract away. For example,

pool.insert_ip_net_port_any(
            IpNet::new(Ipv4Addr::UNSPECIFIED.into(), 0).unwrap(),
            ambient_authority,
);
pool.remove_ip_net_port_any("127.0.0.0/8".parse().unwrap(), ambient_authority);
pool.remove_ip_net_port_any("192.168.0.0/16".parse().unwrap(), ambient_authority);
pool.remove_ip_net_port_any("172.16.0.0/12".parse().unwrap(), ambient_authority);
pool.remove_ip_net_port_any("10.0.0.0/8".parse().unwrap(), ambient_authority);

Does something like this sound reasonable? Looking at the implementation of Pool this doesn't look too easy though as the pool is a simple collection of ranges that are allowed to overlap. Doing set difference doesn't seem straight forward.

Dan Gohman (Dec 06 2023 at 15:13):

Yeah, the current Pool logic is very simple, with the idea that it can be extended once we have a better idea what people need, and now here we are :-).

Dan Gohman (Dec 06 2023 at 15:17):

It seems like it could be extended with a separate denylist, which would override anything granted. Then instead of grants.iter().any(|grant| grant.contains(addr)), the code could do grants.iter().any(|grant| grant.contains(addr)) && !denies.iter().any(|deny| deny.contains(addr)). Does that sound like it would work?

Ryan Levick (rylev) (Dec 06 2023 at 15:18):

:grinning:Yea that certainly sounds simpler than adding full set algebra to Pool.

Ryan Levick (rylev) (Dec 06 2023 at 15:18):

I can look into adding that if you would be ok with that.

Dan Gohman (Dec 06 2023 at 15:18):

Sounds good to me!

Dave Bakker (badeend) (Dec 06 2023 at 19:58):

Without care, this could end up being a quite confusing API:

pool.allow("127.0.0.1");
pool.deny("127.0.0.1");
pool.allow("127.0.0.1");
// at this point, 127.0.0.1 is _not_ allowed.

Ryan Levick (rylev) (Dec 07 2023 at 14:28):

Hmmm... I wonder if adding the ability to do a dynamic check to the wasmtime WasiCtx would be a better short term solution? Effectively I'm thinking about adding the following to WasiCtx:

    /// Add a dynamic check of whether a socket addr is allowed
    pub fn dynamic_socket_addr_check<F>(&mut self, check: F) -> &mut Self
    where
        F: Fn(SocketAddr) -> bool + Send + Sync + 'static;

Ryan Levick (rylev) (Dec 07 2023 at 14:29):

This doesn't really solve the issue as originally described, but it does make it easier to describe slightly more complicated use cases.

Ryan Levick (rylev) (Dec 07 2023 at 14:33):

For example, if you don't want to allow for loopback addresses:

ctx.dynamic_socket_addr_check(|addr| !addr.ip().is_loopback())

Ryan Levick (rylev) (Dec 07 2023 at 14:37):

This might still have confusing usage though:

ctx.dynamic_socket_addr_check(|addr| !addr.ip().is_loopback())
ctx.insert("127.0.0.1".parse().unwrap());

Not sure if 127.0.0.1 should be allowed then

Lann Martin (Dec 07 2023 at 14:38):

What about hooking the start-connect funcs?

Ryan Levick (rylev) (Dec 07 2023 at 14:39):

We need to check more than starting a connection (e.g., 'unconnected' UDP streams). The addr check happens inside of start-connect but also in other places, so I'm not sure that would work?

Ryan Levick (rylev) (Dec 07 2023 at 14:42):

I think if we make dynamic_socket_addr_check purely additive, it should work if the semantics of dynamic_socket_addr_check are: if true is returned, no additional checks are done, but if false, we check the underlying Pool

Ryan Levick (rylev) (Dec 07 2023 at 14:59):

Here's what the implementation would look like: https://github.com/rylev/wasmtime/compare/udp-perform-addr-checking...rylev:wasmtime:dynamic-socket-addr-check

Last updated: Oct 23 2024 at 20:03 UTC