Skip to content

ServerName can now have two kinds of value #292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Tracked by #294
jsha opened this issue Mar 21, 2023 · 7 comments
Closed
Tracked by #294

ServerName can now have two kinds of value #292

jsha opened this issue Mar 21, 2023 · 7 comments
Assignees

Comments

@jsha
Copy link
Collaborator

jsha commented Mar 21, 2023

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 jsha self-assigned this Mar 22, 2023
@jsha
Copy link
Collaborator Author

jsha commented Mar 22, 2023

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.
  2. 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.
  3. 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
Copy link
Member

cpu commented Mar 22, 2023

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] 🤔

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
Copy link
Collaborator Author

jsha commented Mar 22, 2023

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
Copy link
Member

cpu commented Mar 22, 2023

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 👍

@jsha
Copy link
Collaborator Author

jsha commented Mar 22, 2023

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
Copy link
Member

cpu commented Mar 22, 2023

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

@jsha
Copy link
Collaborator Author

jsha commented Mar 22, 2023

Fixed by #302.

@jsha jsha closed this as completed Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants