Skip to content
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

Feature request: Add option for accepting IPs in server name indication #1127

Closed
Alvenix opened this issue Nov 6, 2022 · 10 comments
Closed

Comments

@Alvenix
Copy link

Alvenix commented Nov 6, 2022

I am having issues connecting to servers which use rustls when the server is IP based. The issue occurs when using reqwest which use native-tls. In this case an SNI is always sent even for IPs which is not compatible with the standard so rustls considers the message corrupted.

There was a previous issue which was on safari but was closed as the bug was fixed in safari. However, It may be better to add an option for this instead of waiting for other TLS implementation to comply with the standard.

@djc
Copy link
Member

djc commented Nov 7, 2022

So you're saying a client using native-tls connects to a server using rustls, and the server fails the connection because the client has an IP in the SNI extension?

Since the native-tls backend depends on the platform, on which platform is the client running in your test case?

@Alvenix
Copy link
Author

Alvenix commented Nov 7, 2022

So you're saying a client using native-tls connects to a server using rustls, and the server fails the connection because the client has an IP in the SNI extension?

Since the native-tls backend depends on the platform, on which platform is the client running in your test case?

Yes that is what I meant. Sorry if I wasn’t clear. The other issues I linked contain some examples which can help to reproduce.

I have been using both Ubuntu 20.04 and 22.04.

@djc
Copy link
Member

djc commented Nov 7, 2022

However, It may be better to add an option for this instead of waiting for other TLS implementation to comply with the standard.

What do you think such an option would do? I think this comment from Brian in #203 still holds:

In general I don't think we should guess at what was intended when there was a malformed input. If there is a (business) need for supporting a lax mode then I hope it is configurable and defaults to the strict behavior.

It might make sense to file this as an issue against the openssl crate and/or the OpenSSL project. (It seems a little surprising you're getting hit by this because if this was a widespread issue this wouldn't be the first report we'd get.)

@Alvenix
Copy link
Author

Alvenix commented Nov 7, 2022

What do you think such an option would do? I think this comment from Brian in #203 still holds:

In general I don't think we should guess at what was intended when there was a malformed input. If there is a (business) need for supporting a lax mode then I hope it is configurable and defaults to the strict behavior.

Maybe add option to ignore the SNI if it is IP? I can try to make a pull request if this is acceptable.

It might make sense to file this as an issue against the openssl crate and/or the OpenSSL project. (It seems a little surprising you're getting hit by this because if this was a widespread issue this wouldn't be the first report we'd get.)

I could only speculate why they are few reports.

-They are workarounds (use rustls with no certificate validation instead or add domain to your hosts, etc..)
-rustls currently doesn’t support usage of IPs in certificates, maybe when the support comes more people would encounter this issue.
-To encounter this issue, I think it means use of TLS is for development because you probably have to disable verification of certificate anyway

So to encounter the problem:
1-Server is based on TLS, use a certificate with subject that is domain name such as localhost (Rustls doesn’t support IPs currently)
2-Client use IP to connect, client must disable some of the certificate verification as the URL doesn’t match the subject of the certificate.

So it is not critical at all, however, it ease development.

Also in the reqwest issue, someone revived the issue saying the also encountered the problem.

@iazel
Copy link

iazel commented Jan 13, 2023

I think it would be nice to add support for IPs, today apps and other IoT devices are a big part of the market and in that case you may not need a domain name, an IP may be enough. Granted, this is more of a "nice-to-have" rather than a "must-have".

@djc
Copy link
Member

djc commented Jan 13, 2023

#184 is in the process of being fixed, so we'll be able to support SNI entries for IP addresses as well.

@ctz
Copy link
Member

ctz commented Jan 13, 2023

I have raised openssl/openssl#20041

@cpu
Copy link
Member

cpu commented Mar 31, 2023

We've landed support for IP address subjects in certificates as of v0.21.0.

My perspective is that we should continue to apply pressure on external projects that incorrectly send an IP address value in SNI to fix their bugs and not relax this crate's processing of SNI values to allow this misuse. Is this perspective shared by the other maintainers or should we continue to hold this issue open?

@jsha
Copy link
Contributor

jsha commented Mar 31, 2023

As an FYI the Rust openssl bindings recently landed a fix to stop sending IP addresses in SNI.

I echo @djc's surprise that this hasn't come up more often, since it seems like a very easy mistake to make in the OpenSSL API and so should be widespread. Perhaps it's because connecting via TLS bare IP addresses is relatively rare?

I'm in favor of closing this issue but inviting further comment on it (or 👍🏻) if other people run into the same problem, so we can gauge how widespread a problem it is.

@cpu cpu closed this as completed Mar 31, 2023
@ctz
Copy link
Member

ctz commented Apr 1, 2023

I think we should watch for additional cases of this and see how much of a problem people find it in the field. We could, for example, slightly relax things by accepting all strings and moving the validation into ClientHello::server_name, ServerConnection::server_name and certificate selection, to continue the property that these functions only return valid DNS names. This would make buggy clients could still make connections and rustls would treat them as if they did not support SNI.

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

6 participants