rustls / rustls-ffi

Use Rustls from any language
Other
124 stars 31 forks source link

ServerName can now have two kinds of value #292

Closed jsha closed 1 year ago

jsha commented 1 year ago

In rustls_verify_server_cert_params, we treat ServerName as if it will only contain a DnsName. However, as of rustls 0.21, we can also see IpAddress. We need to update the interface to account for that.

jsha commented 1 year ago

Also, rustls_client_connection_new takes a hostname string. We need to add support for IP addresses.

The upstream API takes advantage of Rust's enums to take a ServerName, which can hold either a hostname (stringish) or an Ipv4Addr / Ipv6Addr. The Rust stdlib does us the favor of providing handy cross-platform IP address representations.

Since we don't have fielded enums in the FFI interface, and don't have a cross-platform IP address struct, we have to make some tweaks. Some options:

  1. Keep the rustls_client_connection_new interface the same, and accept both hostnames and textual representations of IP addresses. Try parsing as an IP address; if that fails, treat as a hostname.
    1. Keep the rustls_client_connection_new interface the same, and introduce a new function rustls_client_connection_new_ip_address that always expects an IP address. 2a. The IP address parameter is a set of bytes - 4 bytes for IPv4 or 16 bytes for IPv6. 2b. The IP address parmeter is a string, and rustls-ffi parses it into an Ipv4Addr / IPv6Addr.
  2. Introduce a new function, and also rename rustls_client_connection_new.

I think I favor 2a. I don't like the implicit magic of (1). On the other hand, I suspect many downstream users want the behavior of "interpret this string as a hostname or an IP address, depending on what it contains."

Thoughts @cpu?

cpu commented 1 year ago

Since we don't have fielded enums in the FFI interface, and don't have a cross-platform IP address struct, we have to make some tweaks. ... On the other hand, I suspect many downstream users want the behavior of "interpret this string as a hostname or an IP address, depending on what it contains."

I'm less familiar with what's ergonomic for the C consumers of this API (so take all of this with many grains of salt!) but my intuition matches yours that it's probably common to have a string in-hand. It also seems like since there's not a cross-platform IP address representation that all of the consumers would have to carry some platform specific code of their own to map from a struct in_addr or equivalent into a byte slice for 2a. Having trustworthy Rust code handle that job for all supported platforms makes me think 2b could be appealing. I'd guess that there's always an inet_ntoa type helper handy where there might not be the same for going from something like a uint32_t to a network order &[u8] :thinking:

Do you think there are any users that would want an API that guarantees they connect to a hostname and never treat an input string as an IP address?

jsha commented 1 year ago

Your argument for 2b makes sense to me.

Do you think there are any users that would want an API that guarantees they connect to a hostname and never treat an input string as an IP address?

I don't think so. Mainly my intuition for 2a was that it's a shame to take an API that explicitly splits things out into their own types, and then collapse it into a stringly-typed API. But in this case I think crossing outside of a given language and its amenities winds up making that the right thing to do.

If an application arises that does want to ensure just hostnames or just IP addresses, we can provide separate constructors for those.

cpu commented 1 year ago

If an application arises that does want to ensure just hostnames or just IP addresses, we can provide separate constructors for those.

That makes sense to me :+1:

jsha commented 1 year ago

Aha, actually rustls upstream makes the same call. There is an impl TryFrom<&str> for ServerName that tries hostname parsing first, then tries IpAddress parsing. We use that TryFrom impl in rustls-ffi, so I think we get this behavior for free and just need to document it and rename the parameter.

cpu commented 1 year ago

Nice find :bulb: I'm glad it lines up with what we're thinking.

jsha commented 1 year ago

Fixed by #302.