Wasmtime GitHub notifications bot (Dec 06 2023 at 16:19): Wasmtime GitHub notifications bot (Dec 06 2023 at 16:19):rylev opened PR #7647 from rylev:toggle-udp-tcp to bytecodealliance:main:
This adds options to
WasiCtxthat allow disabling TCP and UDP wholesale. For TCP this prevents usage of binding, connecting, listening, and accepting while for UDP it prevents binding.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:19):rylev requested wasmtime-core-reviewers for a review on PR #7647.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:19):rylev requested alexcrichton for a review on PR #7647.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:28):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/--wasioptions on the CLI, for example-S tcpwould enable TCP connections while-S udpwould enable UDP. (similarly-S tcp=nwould disable TCP.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:28):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/--wasioptions on the CLI, for example-S tcpwould enable TCP connections while-S udpwould enable UDP. (similarly-S tcp=nwould disable TCP.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:28):alexcrichton created PR review comment:
These I think may want to default to off like IP lookups?
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:28):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.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:28):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?
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:30):rylev submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:30):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.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:33):rylev submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2023 at 16:33):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?
Wasmtime GitHub notifications bot (Dec 06 2023 at 19:15):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2023 at 19:15):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.
Wasmtime GitHub notifications bot (Dec 06 2023 at 19:19):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2023 at 19:19):alexcrichton created PR review comment:
Currently the CLI/
WasiCtxdisallow 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-networkwhich 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-networkis passed. In the future we can probably figure out how to unify this with name lookups.
Wasmtime GitHub notifications bot (Dec 07 2023 at 09:49):rylev updated PR #7647.
Wasmtime GitHub notifications bot (Dec 07 2023 at 09:52):@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.
Wasmtime GitHub notifications bot (Dec 07 2023 at 15:27):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/*.rsand 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 thenameand configure disallowing options based on that.
Wasmtime GitHub notifications bot (Dec 07 2023 at 18:02):rylev updated PR #7647.
Wasmtime GitHub notifications bot (Dec 07 2023 at 18:04):@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.
Wasmtime GitHub notifications bot (Dec 07 2023 at 18:48):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 07 2023 at 19:20):alexcrichton merged PR #7647.
Wasmtime GitHub notifications bot (Dec 08 2023 at 23:37):vicspina commented on PR #7647:
Help
Last updated: Nov 22 2024 at 16:03 UTC