Fine-grained network policies

[rylev]

This follows the design outlined in bytecodealliance/wasmtime#7681 for IpNameLookup.

A new trait IpNameLookup is introduced that factors out the conversion from a String to a list of IP addresses. Another trait, WasiIpNameLookupView is used to configure which concrete type that implements IpNameLookup will be used in the actual IpNameLookup host implementation. Lastly. the SystemIpNameLookup is provided as a default implementation.

Some questions that might guide tweaks:

• Users are now forced to implement WasiIpNameLookupView - it would be nice if there was a to default to the built in SystemIpNameLookup. If there were default associated types for traits, we could get rid of WasiIpNameLookupView and make ip_name_lookup a function on WasiView that defaults to the the SystemIpNameLookup. But alas that's not available to us.

r? @badeend

[badeend]

All in all, looks good to me 🚀

[rylev]

@badeend @alexcrichton

I've moved the implementation towards having a single WasiNetworkView trait where all network based overrides will be implemented. I'm happier with this solution than the previous IpNameLookupView version, but I'm still not convinced we've arrived at a good solution.

The current solution has the following charactertistics:

• Two new methods on WasiView: network_view and network_view_mut which return references to a dyn WasiNetworkView
  • This prevents all the methods necessarily being on WasiView which might get crowded over time. It also makes it easy for users who want to use an off-the-shelf solution to get implement 2 methods instead of the N methods WasiNetworkView will have.
  • The use of dynamic dispatch is new to WasiView but seems appropriate here.
• The various implementors in Wasmtime of WasiView gain a new field with SystemNetwork (the default implementation of WasiNetworkView)

I would said, we move forward with this for now and we can adjust as we add TCP and UDP support (and of course continue to evolve over time). Thoughts?

[badeend]

Having thought about it some more I would like to offer one more suggestion. See code below. This touches multiple discussions above, so tagging @alexcrichton.

The main difference is that this setup doesn't modify WasiView at all, but instead dispatches through WasiCtx::network

---

@rylev: This does have the downside that the WasiCtx setting can be ignored by users.

In setup below that can't happen.

---

The strict separation between the "System*" and "Default*" structs below might be overkill for this PR because of the relative trivial "System*" and "Default*" implementations. However, for TCP/UDP sockets the implementation won't be so trivial anymore.

/// Trait for any WASI-compliant network implementation.
trait Network {
    fn resolve_addresses(&mut self, ) -> ...;

    // fn new_tcp_socket(family) -> ...
    // fn new_udp_socket(family) -> ...
}




/// A WASI-compliant "base" implementation.
/// Without any opinions on how to do filtering, permission checks or whatever.
struct SystemNetwork {}
impl SystemNetwork {
    fn new() -> Self {} // Probably no parameters
}
impl Network for SystemNetwork {
    fn resolve_addresses(&mut self, ) -> {
        // Perform the syscalls without any additional permission checks
    }
}




/// A "good enough" default implementation for Wasmtime users, _with_ permission checks.
struct DefaultNetwork {
    sys: SystemNetwork,
    allowed: bool
}
impl DefaultNetwork {
    /// Whatever parameters we want.
    /// Currently just a simple boolean, but could be extended to any 
    fn new(allowed: bool) -> Self {}
}
impl Network for DefaultNetwork {
    fn resolve_addresses(&mut self, ) -> {
        if self.allowed {
            self.sys.resolve_addresses()
        } else {
            Err(PermissionDenied)
        }
    }
}



pub struct WasiCtx {
    pub(crate) random: Box<dyn RngCore + Send + Sync>,
    pub(crate) insecure_random: Box<dyn RngCore + Send + Sync>,
    pub(crate) insecure_random_seed: u128,
    pub(crate) wall_clock: Box<dyn HostWallClock + Send + Sync>,
    pub(crate) monotonic_clock: Box<dyn HostMonotonicClock + Send + Sync>,
    pub(crate) env: Vec<(String, String)>,
    pub(crate) args: Vec<String>,
    pub(crate) preopens: Vec<(Dir, String)>,
    pub(crate) stdin: Box<dyn StdinStream>,
    pub(crate) stdout: Box<dyn StdoutStream>,
    pub(crate) stderr: Box<dyn StdoutStream>,

    // Remove `socket_addr_check`
    // Remove `allowed_network_uses`
    
    // Add:
    pub(crate) network: Box<dyn Network>,
}





pub struct WasiCtxBuilder {
    // (...)

    network: Box<dyn Network>,
}
impl WasiCtxBuilder {
    pub fn new() -> Self {
        Self {
            // (...)
            network: DefaultNetwork::new(/*allowed: */false),
        }
    }

    pub fn allow_network(&mut self, enable: bool) -> &mut Self {
        self.network = DefaultNetwork::new(enable);
        self
    }

    pub fn custom_network(&mut self, net: Box<dyn Network>) -> &mut Self {
        self.network = net;
        self
    }
}

[alexcrichton]

@badeend your sketch makes sense yeah, but my thoughts at https://github.com/bytecodealliance/wasmtime/issues/7694#issuecomment-1858351478 showcase how something like that breaks down unless all callbacks about configuration go through trait Network, so for example we'd also need to route IP address checks through that trait as well. (not a problem of course, just pointing out that once things are behind a trait everything needs to be behind that trait to share state between callbacks).

Otherwise though I think it'd be reasonable to configure a trait object on WasiCtx vs returning a &mut dyn Trait from WasiView (although it having a default impl returning self.ctx_mut() I think is pretty nifty and makes it almost hidden.

[rylev]

I like @badeend's suggestion. I initially shared the same concern that @alexcrichton pointed out, but I think it should be relatively easy for users to implement the Network trait by delegating all the parts they don't care about to SystemNetwork or DefaultNetwork and only changing what they do care about. The only issue I can see with this is that if the Network trait grows large (which I think we anticipate it will), then there will be a large amount of boiler plate of delegating Network methods to the SystemNetwork or DefaultNetwork. This could be really annoying.

[rylev]

@alexcrichton @badeend I've pushed a new change that moves more towards @badeend's design as well as changing the return type of resolve_addresses to be a ResolveAddressStream.

I think the only way for us to be sure about this design is to keep going and implement UDP and TCP on top of it. With that in mind, I think we should merge this PR and continue the design exploration in a follow up. Thoughts?

[badeend]

something like that breaks down unless all callbacks about configuration go through trait Network, so for example we'd also need to route IP address checks through that trait as well.

Correct. In my mind that was exactly the goal of this (and upcoming) PRs. socket_addr_check & allowed_network_uses would most likely be moved to DefaultNetwork

I think the only way for us to be sure about this design is to keep going and implement UDP and TCP on top of it.

Agree.

With that in mind, I think we should merge this PR and continue the design exploration in a follow up. Thoughts?

I'm not familiar with wasmtime's release process, but if a release gets cut partway through these changes it could be weird for consumers when e.g. IP lookup & TCP use the Network trait but UDP still uses an old & completely different customization mechanism. I'll leave this up for @alexcrichton

[rylev]

I'm fine with not landing this on main. I just think we need to decide which fork/branch is the integration point so that if @badeend and I continue the work, we can bring that all together into one unified branch that will ultimately be merged into main.

[badeend]

@alexcrichton One of the (many..) changes in this PR is that the actual socket code has been separated out from the component trait implementation to its own SystemTcpSocket/SystemUdpSocket. To reduce the size of this PR, do you mind if I split that refactor off from this PR and submit a new one with just those changes? These changes will only be internal reshuffling of code, without affecting the public interface.

[alexcrichton]

Sounds good to me! And yeah refactorings are always good to land at any time