Fix SGX delayed host lookup via ToSocketAddr

[jethrogb]

#146541 introduced a regression in the x86_64-fortanix-unknown-sgx target, where arguments to TcpStream::connect and TcpListener::bind are no longer being passed down correctly to the underlying platform, resulting in connection/bind failures

• Add a test for the desired behavior (this test passes on Rust 1.33 through 1.91)
• Refactor lookup_host logic again

Fixes #152848

cc @joboet

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, jhpratt, joboet

[joboet]

I don't think we should fix this in this way. In #146541 (comment) you mentioned Fortanix's specification, but that's not really relevant here. The documentation for std's ToSocketAddrs clearly states that

&str: the string should be either a string representation of a SocketAddr as expected by its FromStr implementation or a string like <host_name>:<port> pair where <port> is a u16 value.

In my opinion, special-casing SGX here is very weird; even incorrect when considering that the "pass the string via the error" hack leads to incorrect code when users try to write manual implementations of ToSocketAddrs (as only the first host will be considered). This whole thing feels to me like you are trying to fit a square peg in a round hole. I recommend you either:

1. Allow host name lookup inside the enclave. Yes, the result of the lookup cannot be relied upon, but retrieving the actual addresses might still be useful for e.g. deciding whether to use a proxy.
2. Add a TcpStreamExt trait with a connect_by_name that has the exact semantics you need.

But I'm not in T-libs-api, let's hear what they think...
@rustbot label +I-libs-api-nominated

[joshtriplett]

We reviewed this in today's @rust-lang/libs-api meeting.

We are willing to consider preserving the SGX behavior (which is self-described as a "terrible, terrible hack" that "leads to buggy code").

However, we felt that this implementation was too invasive in the way it affects all other targets. We'd prefer to see this hack more self-contained in one place.

Separately from that, we'd like to understand more specifically what SGX does with something like TcpSocket::connect("hostname-with-no-port"), and whether it may make sense to deprecate that behavior over time in favor of something like an SGX-specific TcpSocketExt::connect_xyz method in an extension trait.

[joshtriplett]

(Feel free to reach out to me via Zulip if you need more detail on what "more self-contained" means here.)

[joboet]

which is self-described as a "terrible, terrible hack" that "leads to buggy code"

I should point out that I added that line in #145327. But I stand by it, this completely breaks the API of ToSocketAddrs.

[jethrogb]

(which is self-described as a "terrible, terrible hack" that "leads to buggy code").

The comment about the “hack” wasn't introduced by the SGX target maintainers.

Separately from that, we'd like to understand more specifically what SGX does with something like TcpSocket::connect("hostname-with-no-port")

The string is passed directly outside the enclave for the bind/connect usercall. Details on the platform interface are available at https://edp.fortanix.com/docs/api/fortanix_sgx_abi/struct.Usercalls.html#networking

and whether it may make sense to deprecate that behavior over time in favor of something like an SGX-specific TcpSocketExt::connect_xyz method in an extension trait.

This is technically possible, but it would significantly impact compatibility and ease of use. One of the design goals for the target is “Compatible with most Rust code”. When application developers are using a 3rd party crate that creates network connections, it's unlikely that that crate would have support for such an extension trait. So now you'd have to vendor/upstream a change to support this.