Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure remote-address is allowed when creating UDP stream #7648

Merged
merged 5 commits into from
Dec 8, 2023

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Dec 6, 2023

Until now, the remote-address argument to wasi:sockets/udp-socket#stream was not checked against the Pool of allowed IP addresses. Now, we store the Pool from network into the socket object in wasi:sockets/udp-socket#bind and later use that Pool in wasi:sockets/udp-socket#stream to check that remote-address is allowed.

The check itself is a bit hacky since Pool doesn't seem to expose a way to just check an address so we use PoolExt to create a UdpConnecter which we will fail if we don't have permission to connect to the remote address.

I can add a test once I figure out the testing story in #7647

@rylev rylev requested a review from a team as a code owner December 6, 2023 17:14
@rylev rylev requested review from pchickey and removed request for a team December 6, 2023 17:14
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Dec 6, 2023
crates/wasi/src/preview2/host/udp.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview2/host/udp.rs Outdated Show resolved Hide resolved
@rylev
Copy link
Contributor Author

rylev commented Dec 7, 2023

I've addressed the PR feedback:

  • Pool is now wrapped in an Arc to make cloning cheaper.
  • HostOutgoingDatagramStream will check the provided addr against the Pool before sending a datagram.

@alexcrichton I'm not sure what the testing story should look like here. I don't see where the wasi implementations are being tested against non-default WasiCtxs. I believe all tests are running with full network. I suppose whatever solution we decide on here should also work for #7647 as well.

@alexcrichton
Copy link
Member

I wrote up some thoughts here for how this might be tested (in case anyone lands on this PR in the future)

Comment on lines 47 to 48
/// The pool of allowed address
pub(crate) pool: Arc<Pool>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this start as None to represent that it's bound-late? That could also help overwriting it by accident by only writing to it if it's None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same below too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a change that does this, but I have a question about the exact desired semantics of start-bind. Take the following_example:

// `bad_addr` is an address that is not permitted
assert!(sock.start_bind(bad_addr).is_err());
// `good_addr` is permitted
sock.start_bind(good_addr);

As currently written the second call to start-bind will fail with InvalidState since the pool has already been set on the socket. Is this right? Previously, this would have succeeded. I would imagine that start_bind should be allowed to be called multiple times as long as the socket has not successfully been bound before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the check that the pool was not previously set as this broke some tests. Once we decide on what the semantics should be, I can adjust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @badeend on this you are probably best-equipped to answer this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rylev

From the API's perspective, unbound sockets (Default) never need a pool, and all other states (BindStarted, Bound, Connected) could've only be reached by calling bind at some point, so they always have access to a pool. A way to codify this could be:

pub(crate) enum UdpState {
    Default,
    BindStarted(Arc<Pool>),
    Bound(Arc<Pool>),
    Connected(Arc<Pool>),
}

Also, OutgoingDatagramStream's pool field doesn't have to be optional, because you can never call stream on an unbound socket.


As currently written the second call to start-bind will fail with InvalidState since the pool has already been set on the socket. Is this right?

From the caller's perspective, the start-bind call should preferably succeed or fail atomically. Using the enum layout above would automagically fix that.

@rylev rylev force-pushed the udp-perform-addr-checking branch from 84c51e4 to a9e2dd8 Compare December 8, 2023 09:14
rylev added 4 commits December 8, 2023 15:45
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the udp-perform-addr-checking branch from a9e2dd8 to c33c918 Compare December 8, 2023 14:47
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the udp-perform-addr-checking branch from c33c918 to 350c31c Compare December 8, 2023 15:08
@alexcrichton
Copy link
Member

I'm gonna go ahead and queue this for merge, and @rylev says they're happy to follow-up on semantic changes in a follow-up.

@alexcrichton alexcrichton added this pull request to the merge queue Dec 8, 2023
Merged via the queue into bytecodealliance:main with commit 63e2606 Dec 8, 2023
19 checks passed
@rylev rylev deleted the udp-perform-addr-checking branch December 8, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants