rylev opened PR #7648 from rylev:udp-perform-addr-checking
to bytecodealliance:main
:
Until now, the
remote-address
argument towasi:sockets/udp-socket#stream
was not checked against thePool
of allowed IP addresses. Now, we store thePool
fromnetwork
into thesocket
object inwasi:sockets/udp-socket#bind
and later use thatPool
inwasi:sockets/udp-socket#stream
to check thatremote-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 usePoolExt
to create aUdpConnecter
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 https://github.com/bytecodealliance/wasmtime/pull/7647
rylev requested wasmtime-core-reviewers for a review on PR #7648.
rylev requested pchickey for a review on PR #7648.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this be done after the state validation below? That'd help prevent odd situations such as calling this multiple times just to switch the network while having the method return an error.
badeend submitted PR review.
badeend submitted PR review.
badeend created PR review comment:
Might not be the prettiest solution, but: could we use this same workaround to validate the remote address in unconnected outgoing datagram streams?
badeend created PR review comment:
@rylev Don't know if this was your intention, but this creates a full copy of the entire pool for each socket.
rylev submitted PR review.
rylev created PR review comment:
Yea, the clone shouldn't be too expensive, but if we're worried about allocations here, I can wrap this in an
Arc
.
rylev updated PR #7648.
I've addressed the PR feedback:
- Pool is now wrapped in an
Arc
to make cloning cheaper.HostOutgoingDatagramStream
will check the provided addr against thePool
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
WasiCtx
s. 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 closed without merge PR #7648.
alexcrichton commented on PR #7648:
I wrote up some thoughts here for how this might be tested (in case anyone lands on this PR in the future)
alexcrichton reopened PR #7648.
rylev updated PR #7648.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
(same below too)
alexcrichton created PR review comment:
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'sNone
rylev updated PR #7648.
rylev updated PR #7648.
rylev submitted PR review.
rylev created PR review comment:
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 withInvalidState
since the pool has already been set on the socket. Is this right? Previously, this would have succeeded. I would imagine thatstart_bind
should be allowed to be called multiple times as long as the socket has not successfully been bound before.
rylev updated PR #7648.
rylev updated PR #7648.
rylev submitted PR review.
rylev created PR review comment:
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.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
cc @badeend on this you are probably best-equipped to answer this
alexcrichton commented on PR #7648:
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 merged PR #7648.
badeend submitted PR review.
badeend created PR review comment:
@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 callingbind
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 callstream
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.
Last updated: Dec 23 2024 at 13:07 UTC