alexcrichton commented on issue #7109:
cc @sunfishcode and @badeend
This is based on https://github.com/bytecodealliance/wasmtime/pull/7029 so only the last commits here are relevant.
I'm particularly keen to get y'alls input and thoughts on the implementation here. Was the intention to be able to use
getaddrinfo
? Or was the intention that implementations must avoidgetaddrinfo
? For example the documentation of theip-name-lookup
functions indicates that addresses shouldn't be parsed (which I assume means that resolving"127.0.0.1"
should fail), but I couldn't find the option to specify togetaddrinfo
that this must fail. I was basically wondering if y'all had an implementation for this in mind ahead of time, and if so I'd be happy to switch to that.
alexcrichton commented on issue #7109:
Also I'm curious for y'all's thoughts on how to make this capability-based. For example right now I've added a global flag for "allow any name lookups at all" but that's quite coarse. Did y'all have thoughts already about how this might look like to configure from a CLI and how embedders might use it?
alexcrichton edited a comment on issue #7109:
cc @sunfishcode and @badeend
This is based on https://github.com/bytecodealliance/wasmtime/pull/7029 so only the last commits here are relevant.I'm particularly keen to get y'alls input and thoughts on the implementation here. Was the intention to be able to use
getaddrinfo
? Or was the intention that implementations must avoidgetaddrinfo
? For example the documentation of theip-name-lookup
functions indicates that addresses shouldn't be parsed (which I assume means that resolving"127.0.0.1"
should fail), but I couldn't find the option to specify togetaddrinfo
that this must fail. I was basically wondering if y'all had an implementation for this in mind ahead of time, and if so I'd be happy to switch to that.
badeend commented on issue #7109:
Hi!
I see the
name
parameter is directly passed on to.to_socket_addrs()
. Which is understandable, but that doesn't quite reflect the proposal.was the intention that implementations must avoid getaddrinfo?
No, but to keep the WASI API surface area as small & simple as possible, it isn't a drop-in replacement either. From the Readme:
Why not getaddrinfo?
The proposed wasi-ip-name-lookup module focuses strictly on translating internet domain names to ip addresses and nothing else.
Like BSD sockets,
getaddrinfo
is very generic and multipurpose by design. The proposed WASI API is not. This eliminates many of the other "hats" getaddrinfo has (and potential security holes), like:
- Mapping service names to port numbers ("https"
->443
)
- Mapping service names/ports to socket types ("https"
->SOCK_STREAM
)
- Network interface name translation (%eth0
->1
)
- IP address deserialization ("127.0.0.1"
->Ipv4Address(127, 0, 0, 1)
)
- IP address string canonicalization ("0:0:0:0:0:0:0:1"
->"::1"
)
- Constants lookup forINADDR_ANY
,INADDR_LOOPBACK
,IN6ADDR_ANY_INIT
andIN6ADDR_LOOPBACK_INIT
.Many of these functionalities can be shimmed in the libc implementation. Though some require future WASI additions. An example is network interface name translation. That requires a future
if_nametoindex
-like syscall.
(...) I've added a global flag for "allow any name lookups at all" but that's quite coarse. Did y'all have thoughts already about how this might look like to configure from a CLI and how embedders might use it?
This issue might be relevant: https://github.com/WebAssembly/wasi-sockets/issues/32
Also, this document exists: https://github.com/WebAssembly/wasi-sockets/blob/main/GrantingAccess.md but keep in mind that it hasn't been updated since the first draft of the proposal.For the initial implementation, a simple boolean (allow/deny everything) seems fine to me. :+1:
badeend commented on issue #7109:
I've added input validation here: https://github.com/badeend/wasmtime/commits/wasi-ip-name-lookup
alexcrichton commented on issue #7109:
Ok thanks! I saw the note about getaddrinfo yeah but I wasn't sure whether it was intended that a particular flag was passed or whether parsing was going to be required. I think your branch works well though, and I'll integrate that here, thanks for it!
Also sounds good on access restrictions for now, I like the
GrantingAccess.md
link and agree that it's probably best to start off with a simple boolean for now and it can be bikeshedded/expanded later.
fitzgen commented on issue #7109:
Redirecting review to Pat since I'm not really familiar with WASI bits.
Last updated: Dec 23 2024 at 13:07 UTC