@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.
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 :-).
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?
:grinning:Yea that certainly sounds simpler than adding full set algebra to Pool
.
I can look into adding that if you would be ok with that.
Sounds good to me!
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.
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;
This doesn't really solve the issue as originally described, but it does make it easier to describe slightly more complicated use cases.
For example, if you don't want to allow for loopback addresses:
ctx.dynamic_socket_addr_check(|addr| !addr.ip().is_loopback())
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
What about hooking the start-connect
funcs?
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?
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
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: Dec 23 2024 at 13:07 UTC