rylev opened PR #7647 from rylev:toggle-udp-tcp
to bytecodealliance:main
:
This adds options to
WasiCtx
that allow disabling TCP and UDP wholesale. For TCP this prevents usage of binding, connecting, listening, and accepting while for UDP it prevents binding.
rylev requested wasmtime-core-reviewers for a review on PR #7647.
rylev requested alexcrichton for a review on PR #7647.
alexcrichton submitted PR review:
Thanks! Mind adding a test for this as well?
If you're up for it too it'd be nice to get this hooked up to
-S
/--wasi
options on the CLI, for example-S tcp
would enable TCP connections while-S udp
would enable UDP. (similarly-S tcp=n
would disable TCP.
alexcrichton submitted PR review:
Thanks! Mind adding a test for this as well?
If you're up for it too it'd be nice to get this hooked up to
-S
/--wasi
options on the CLI, for example-S tcp
would enable TCP connections while-S udp
would enable UDP. (similarly-S tcp=n
would disable TCP.
alexcrichton created PR review comment:
These I think may want to default to off like IP lookups?
alexcrichton created PR review comment:
Or, alternatively, ideally these are all the same default since it's the same family of interfaces. I'm not sure how best to design the "permissions" around this with respect to things like defaults and CLI flags myself.
alexcrichton created PR review comment:
Could this be configured similar to the other options here in the positive where it's
allow_{tcp,udp}
instead ofdisallow
?
rylev submitted PR review.
rylev created PR review comment:
I wasn't sure how that wold be feel given that the default is that TCP and UDP are allowed. Given that, most users will do
ctx.allow_tcp(false)
which feels stranger to me than the admittedly inconsistentctx.disallow_tcp(true)
. But happy to switch over for consistency sake.
rylev submitted PR review.
rylev created PR review comment:
I'm also not sure - it sort of depends on what the default security posture of wasmtime is? Are we ok with users who want to do networking needing to jump through a few more hoops to get things working? Do we allow TCP and disallow UDP by default because TCP is more common than UDP?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
It might be more of a personal preference thing on my part then, but I find it easier to read
allow_tcp(false)
thandisallow_tcp(false)
since the latter has two negatives and I have to invert it in my head to realizing that it's allowing TCP.I think it's ok to not tie the method name to the default value since whenever it's configured in code it'll be easy to read what's happening and the default would be documented in the documentation as well.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Currently the CLI/
WasiCtx
disallow everything by default and require opt-ins to enable anything. The CLI flags/knobs for networking has been sort of ad-hoc so far. Right now it's-S inherit-network
which enables doing anything and then there's separately-S allow-ip-name-lookup
. Both knobs primarily only exist to disable things by default, though.Given that I'd actually say this should be left as-is. I dont think it should be verbose to opt-in to things and despite TCP/UDP being enabled by default they still won't work unless
-S inherit-network
is passed. In the future we can probably figure out how to unify this with name lookups.
rylev updated PR #7647.
@alexcrichton I've addressed your feedback for everything but the test. Looking at the testing structure in wasmtime, I couldn't find a place where a test for this should clearly go. For example, I don't see a place where name lookups not being allowed is being tested. Any pointers on where to test this would be most appreciated.
alexcrichton commented on PR #7647:
Ah yes that's a good point that name lookups not being allowed isn't tested right now. Since that was added it'd become easier to add tests though so I think the best way to test this would be something along the lines of a
cli_*
tests incrates/test-programs/src/bin/*.rs
and then a corresponding test function in this module. You could then pass-S ...
flags to disable/enable the network and/or validate it's enabled/disabled by default.Alternatively a
preview2_*
test could be added and this function could be modified to look at thename
and configure disallowing options based on that.
rylev updated PR #7647.
@alexcrichton I added a test for TCP - I will add a test in https://github.com/bytecodealliance/wasmtime/pull/7648 for UDP since those tests would fail without the fixes there.
alexcrichton submitted PR review.
alexcrichton merged PR #7647.
vicspina commented on PR #7647:
Help
Last updated: Dec 23 2024 at 12:05 UTC