Fine-grained network policies #7705

rylev · 2023-12-19T15:48:47Z

This follows the design outlined in #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 · 2023-12-19T16:00:24Z

crates/wasi/src/preview2/ip_name_lookup.rs

    fn resolve_addresses(
        &mut self,
        network: Resource<Network>,
        name: String,
    ) -> Result<Resource<ResolveAddressStream>, SocketError> {
        let network = self.table().get(&network)?;

-        let host = parse(&name)?;
-
        if !network.allow_ip_name_lookup {


With these changes, can network.allow_ip_name_lookup now be removed?

It could be potentially. Right now the default is to not allow ip name lookups, so if we remove this, the default will flip. Additionally, the new way to turn off name lookups would be through a trait implementation (i.e., impl IpNameLookup for BlockedIpNameLookup or similar) and not through WasiCtx/WasiCtxBuilder.

Though it's probably somewhat confusing to implement WasiIpNameLookupView and then not have name lookups work because you forgot to flip the bit on WasiCtx.

Perhaps we remove the allow_ip_name_lookup and provide an additional implementation of IpNameLookup: BlockedIpNameLookup so that users can easily turn off IP name lookup if they like. Thoughts?

Hmm. Interesting.
I think the WASI translation layer (preview2/ip_name_lookup.rs in this case) should only depend on a single source of truth: WasiIpNameLookupView. It should not also go peeking into network.allow_ip_name_lookup on its own.

It might be an idea to provide an additional implementation of IpNameLookup based on WasiCtx, that dynamically checks allowed_network_uses. This could supersede the BlockedIpNameLookup you suggested

This does have the downside that the WasiCtx setting can be ignored by users. If their implementation of IpNameLookup doesn't check for allowed_network_uses, that option will simply be ignored. I guess it seems fine that the user can ignore that option (they're the ones setting it anyway), but it does seem somewhat error prone.

In any case, I pushed a commit that moves the check into SystemIpNameLookup.

crates/wasi/src/preview2/ip_name_lookup.rs

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. Signed-off-by: Ryan Levick <[email protected]>

badeend · 2023-12-19T18:15:59Z

All in all, looks good to me 🚀

…mIpNameLookup Signed-off-by: Ryan Levick <[email protected]>

Signed-off-by: Ryan Levick <[email protected]>

rylev · 2024-01-04T13:51:10Z

@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?

alexcrichton · 2024-01-04T16:51:53Z

crates/wasi/src/preview2/ip_name_lookup.rs

-impl<T: WasiView> Host for T {
+/// A trait for providing a `IpNameLookup` implementation.
+pub trait WasiNetworkView: Send {
+    /// Given a name, resolve to a list of IP addresses
    fn resolve_addresses(


This might be best to model as an async function rather than returning an AbortOnDropJoinHandle?

This is indeed very annoying... The value returned from resolve_address must be Send + Sync as it is used in ResolveAddressStream which impls Subscribe which requires the type implementing it to be Send + Sync.

The #[async_trait] macro puts a Send bound on the return type but not Sync and there is no way to instruct it to do so. We could move to using Box<dyn Future> but that means we loose the developer experience of async_trait. We could potentially move to using impl Future but that requires the latest version of Rust, and I'm not sure if that's ok.

Hm that's true, and yeah I see how there's no great way to manage this right now.

I suppose this can stay for now, but I'm still worried that this will inevitably cause some pain down the road. The best alternative I can think of is to return a oneshot::Receiver<T> instead but that's still pretty cumbersome to work with and wouldn't work great here.

We could take a page out of WasiHttpView's playbook and have the function return a wasmtime::Result<Resource<ResolveAddressStream>> just like WasiHttpView::send_request returns a wasmtime::Result<Resource<HostFutureIncomingResponse>>. I don't really love this either, but at least it's consistent?

Of all the options mentioned so far, the Box<dyn Future> route seems the least worst to me.

The fact that we currently use a quick&dirty implementation using spawn_blocking should be an implementation detail of SystemNetwork

Returning a Resource<ResolveAddressStream> would be nice yeah but it seems like that would have all the same design questions around "how do you actually create one of those".

Although as I say that we could perhaps get around async trait by having something like fn from_future<T: Future + Send + Sync + 'static> (...) on the ResolveAddressStream type. That way you could pass an async block which would have bounds generated correctly and additioanlly the handle from spawn_blocking could also be passed in? That way we might be able to avoid async traits altogether and rely entirely on that.

Thinking about this more, returning Resource<ResolveAddressStream> would mean that the implementor would have to also implement WasiCtx or otherwise have multiple access to the ResourceTable. Right now we don't require that from the implementor of WasiNetworkView, and I'm not sure we want to.

ResolveAddressStream itself could be returned though leaving the table management bits to the glue?

crates/wasi/src/preview2/ip_name_lookup.rs

Signed-off-by: Ryan Levick <[email protected]>

alexcrichton · 2024-01-04T20:22:14Z

crates/wasi/src/preview2/ip_name_lookup.rs


-        if !network.allow_ip_name_lookup {
-            return Err(ErrorCode::PermanentResolverFailure.into());
+impl SystemNetwork {


Could this be removed in lieu of implementing WasiNetworkView for WasiCtx itself? (given that WasiCtx is needed to create this structure anyway it may not buy a whole lot)

If a user wants to have some logic where they conditionally switch between either their own network implementation and SystemNetwork, they need some way of constructing the SystemNetwork. Without this constructor, they would not be able to do so. We could do the following:

make all fields of SystemNetwork public

Make the constructor just take a simple bool instead of a WasiCtx

Leave it as is

I think I'd be most in favor of the last two options.

Hm I'm not sure I understand? Given impl WasiNetworkView for WasiCtx then if users are themselves implementing WasiNetworkView for their own custom type then conditionally using the default behavior would be calling self.ctx.the_method_name() so I'm not sure where the need for having this separate SystemNetwork structure arises?

You're right that SystemNetwork isn't strictly necessary. It felt nice to have a distinct type that lives in the ip_name_lookup module that can handle name lookups, but you're right that we can just implement that logic directly on WasiCtx. It does make the code a wee bit simpler.

alexcrichton · 2024-01-04T20:28:27Z

crates/wasi/src/preview2/ip_name_lookup.rs

-impl<T: WasiView> Host for T {
+/// A trait for providing a `IpNameLookup` implementation.
+pub trait WasiNetworkView: Send {
+    /// Given a name, resolve to a list of IP addresses
    fn resolve_addresses(


Hm that's true, and yeah I see how there's no great way to manage this right now.

I suppose this can stay for now, but I'm still worried that this will inevitably cause some pain down the road. The best alternative I can think of is to return a oneshot::Receiver<T> instead but that's still pretty cumbersome to work with and wouldn't work great here.

badeend · 2024-01-06T12:53:09Z

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 · 2024-01-08T16:02:51Z

@badeend your sketch makes sense yeah, but my thoughts at #7694 (comment) 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 · 2024-01-08T17:20:57Z

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.

Signed-off-by: Ryan Levick <[email protected]>

rylev · 2024-01-08T17:39:59Z

@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 · 2024-01-08T20:02:26Z

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

alexcrichton

Looks reasonable to me!

Unless this is particularly urgent though I would personally agree with @badeend in that I'd prefer to see a more complete picture before landing if possible. If this is needed to unblock work I think it's ok to land, but if you're ok working with a fork for now though I think it'd be best to sketch out the other checks you're thinking of implementing to be able to see them altogether at once.

alexcrichton · 2024-01-09T15:25:53Z

crates/wasi/src/preview2/ip_name_lookup.rs

 }

 #[async_trait::async_trait]
-impl<T: WasiView> Host for T {
+impl<T: WasiView + Sized> Host for T {


I think the + Sized may not be necessary here?

rylev · 2024-01-09T17:00:53Z

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.

- BoxSyncFuture: for public facing signatures. This is just a regular `Pin<Box<dyn Future ...>>` - Preview2Future: utility type for use within the WASI wrapper code

- Remove AllowedNetworkUses in favor of just socket_addr_checks - Let all permission checks go through self.ctx(), instead of NetworkHandle

Co-authored-by: Ryan Levick <[email protected]>

…to tcp-override

TCP overrides

…nto tcp-override

…ocketAddrFamily anymore

Sync with upstream

Signed-off-by: Ryan Levick <[email protected]>

UDP override

badeend · 2024-02-11T14:14:32Z

@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 · 2024-02-12T15:14:52Z

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

rylev requested a review from a team as a code owner December 19, 2023 15:48

rylev requested review from alexcrichton and removed request for a team December 19, 2023 15:48

badeend reviewed Dec 19, 2023

View reviewed changes

crates/wasi/src/preview2/ip_name_lookup.rs Outdated Show resolved Hide resolved

rylev force-pushed the lookup-override branch from cb1fe0b to 3420ba0 Compare December 19, 2023 16:15

Move the check for allowed_network_uses.ip_name_lookup into the Syste…

dd7a684

…mIpNameLookup Signed-off-by: Ryan Levick <[email protected]>

rylev force-pushed the lookup-override branch from 7085038 to dd7a684 Compare December 19, 2023 19:20

github-actions bot added the wasi Issues pertaining to WASI label Dec 19, 2023

Move to a single WasiNetworkView trait

83403ea

Signed-off-by: Ryan Levick <[email protected]>

alexcrichton reviewed Jan 4, 2024

View reviewed changes

Default impl for network_view methods

f23adca

Signed-off-by: Ryan Levick <[email protected]>

rylev force-pushed the lookup-override branch from d581b41 to f23adca Compare January 4, 2024 18:23

alexcrichton reviewed Jan 4, 2024

View reviewed changes

A trait object Network

5355139

Signed-off-by: Ryan Levick <[email protected]>

alexcrichton reviewed Jan 9, 2024

View reviewed changes

badeend added 5 commits January 10, 2024 10:31

Reorder code blocks

d41494b

Move tcp_create_socket impl

a59482c

Move preview2/tcp.rs into preview2/host/tcp.rs

69c1132

Rename types to include *Wrapper suffix

66e0ade

Migrate to a AsyncRead+AsyncWrite based implementation

9ac3c4f

badeend and others added 18 commits January 11, 2024 23:31

Move bind into SystemTcpSocket

663c62e

Inline TCP-specific utilities

6053b7f

Return the streams from connect&accept

ae90b6e

Refactor DynFuture away into two types:

6343c9e

- BoxSyncFuture: for public facing signatures. This is just a regular `Pin<Box<dyn Future ...>>` - Preview2Future: utility type for use within the WASI wrapper code

Introduce TcpSocket trait

a2ee9a9

Refactor:

31ea773

- Remove AllowedNetworkUses in favor of just socket_addr_checks - Let all permission checks go through self.ctx(), instead of NetworkHandle

Implement DefaultTcpSocket

196a69a

Fix error in MacOS-only code

3877aee

Resolve ambiguity between SocketAddrFamily and SocketAddressFamily

d3039a5

Update comments

9a5ebdb

Document why these public methods exist

0aaaf66

Explicitly call it deny

484e777

Update crates/wasi/src/preview2/host/network.rs

bebc91f

Co-authored-by: Ryan Levick <[email protected]>

Update crates/wasi/src/preview2/host/network.rs

78a48bd

Co-authored-by: Ryan Levick <[email protected]>

Merge branch 'tcp-override' of https://github.com/badeend/wasmtime in…

36e8190

…to tcp-override

Give all WASI wrappers the same suffix

19f6ec4

Eliminate fall-throughs from resolve_next_address

7b3fbbf

Merge pull request #2 from badeend/tcp-override

afcaae1

TCP overrides

rylev changed the title ~~Abstract out IpNameLookup core functionality so that users can override it~~ Fine-grained network policies Jan 16, 2024

rylev mentioned this pull request Jan 16, 2024

WASI: Fine-grained network policies #7681

Open

badeend and others added 7 commits January 18, 2024 17:42

Merge branch 'main' of https://github.com/bytecodealliance/wasmtime i…

95145b9

…nto tcp-override

After 9124e03 there's no distinction between SocketProtocolMode and S…

47bd556

…ocketAddrFamily anymore

Merge pull request #3 from badeend/tcp-override

d796ad4

Sync with upstream

Move everything over from UDP but streams

bc23975

Signed-off-by: Ryan Levick <[email protected]>

Get it all compiling

2ac3638

Signed-off-by: Ryan Levick <[email protected]>

Allow overriding streams

cc1e62f

Signed-off-by: Ryan Levick <[email protected]>

Merge pull request #4 from rylev/udp-override

109efa1

UDP override

rylev mentioned this pull request Apr 2, 2024

Separate TCP implementation from the host bindings #8282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fine-grained network policies #7705

Fine-grained network policies #7705

rylev commented Dec 19, 2023 •

edited

Loading

badeend Dec 19, 2023

rylev Dec 19, 2023

badeend Dec 19, 2023

rylev Dec 19, 2023 •

edited

Loading

badeend commented Dec 19, 2023

rylev commented Jan 4, 2024

alexcrichton Jan 4, 2024

rylev Jan 4, 2024

alexcrichton Jan 4, 2024

rylev Jan 5, 2024

badeend Jan 6, 2024

alexcrichton Jan 8, 2024

rylev Jan 8, 2024

alexcrichton Jan 8, 2024

alexcrichton Jan 4, 2024

rylev Jan 5, 2024

alexcrichton Jan 8, 2024

rylev Jan 8, 2024

alexcrichton Jan 4, 2024

badeend commented Jan 6, 2024

alexcrichton commented Jan 8, 2024

rylev commented Jan 8, 2024

rylev commented Jan 8, 2024

badeend commented Jan 8, 2024

alexcrichton left a comment

alexcrichton Jan 9, 2024

rylev commented Jan 9, 2024

badeend commented Feb 11, 2024

alexcrichton commented Feb 12, 2024

Fine-grained network policies #7705

Are you sure you want to change the base?

Fine-grained network policies #7705

Conversation

rylev commented Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

badeend commented Dec 19, 2023

rylev commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badeend commented Jan 6, 2024

alexcrichton commented Jan 8, 2024

rylev commented Jan 8, 2024

rylev commented Jan 8, 2024

badeend commented Jan 8, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Jan 9, 2024

badeend commented Feb 11, 2024

alexcrichton commented Feb 12, 2024

rylev commented Dec 19, 2023 •

edited

Loading

rylev Dec 19, 2023 •

edited

Loading