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

proposal: crypto/tls: make server name extension optional #28754

Closed
lvht opened this issue Nov 13, 2018 · 9 comments
Closed

proposal: crypto/tls: make server name extension optional #28754

lvht opened this issue Nov 13, 2018 · 9 comments

Comments

@lvht
Copy link

@lvht lvht commented Nov 13, 2018

The current tls stack will always send the server name extension to the server. And the config.ServerName filed is also used to verify the certificate domain name of server.

It is well know that the server name extension is transfer in plaintext. So we should only transfer them if we do need them. As we can put multiple domain name into one certificate, the SNI Extension should not be needed widely, and make sending them as a default behavior seems not a good choice.

I propose to introduce an option like config.SkipSNI bool to let tls client not send this field.

@gopherbot gopherbot added this to the Proposal milestone Nov 13, 2018
@gopherbot gopherbot added the Proposal label Nov 13, 2018
@lvht

This comment has been minimized.

Copy link
Author

@lvht lvht commented Nov 14, 2018

Hi @FiloSottile @bradfitz, would you like to make you comment?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 14, 2018

Proposals are reviewed about once weekly. I don't know about crypto proposal reviews, but you might have to wait about the same, or even two weeks.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 18, 2018

Change https://golang.org/cl/150139 mentions this issue: crypto/tls: add SkipSNI option

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 18, 2018

RFC 8446 upgraded server_name to Mandatory To Implement (Section 9.2) and depending on how you read made it a SHOULD or a MUST to send it as an HTTP client.

Additionally, all implementations MUST support the use of the
"server_name" extension with applications capable of using it.
Servers MAY require clients to send a valid "server_name" extension.

[...] As servers
MAY require the presence of the "server_name" extension, clients
SHOULD send this extension, when applicable.

As an ecosystem position, we struggled for years with low SNI adoption, and now that it's ubiquitous it finally enables packing multiple websites on depleting IPv4s without tradeoffs. IPv6 is not even close in adoption. Now is not the time to roll SNI back.

Even if the server you are contacting doesn't require SNI now, not sending it puts it in a difficult position should it decide to start depending on it later. I don't think we should make it easy.

Finally, tls.Config has already too many options.

If you really want to defeat it, there is a way: unset ServerName, set InsecureSkipVerify, and do the verification yourself with VerifyPeerCertificate. It's hard and uncomfortable, but I argue that disabling an important ecosystem feature should be.

@lvht

This comment has been minimized.

Copy link
Author

@lvht lvht commented Nov 18, 2018

hi @FiloSottile , thank you for reviewing this propose.

As far as i see, the RFC doe not require the client always send the SNI extension, does it?

RFC 8446 upgraded server_name to Mandatory To Implement (Section 9.2) and depending on how you read made it a SHOULD or a MUST to send it as an HTTP client.

What the RFC require is both the server and client must implement the SNI extension, but not the client must always send the SNI information.

And the RFC also reads

The "server_name" [RFC6066] and "certificate_authorities"
extensions are used to guide certificate selection. As servers
MAY require the presence of the "server_name" extension, clients
SHOULD send this extension, when applicable.

The when applicable here should not be always or must.

And, this propose just make the tls ecosystem has the ability to not always send the SNI extension, but not recommend to use it. However, why the SkipSNI is needed? Because, the server name is transferred in the plain text and could be used by some government for auditing.

Introduce the SkipSNI option does not break the RFC, does not break widely-SNI-adopt ecosystem, but protect user's privacy. So i beg you to reconsider this proposal.

People in certain country need your help.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 18, 2018

I understand the purpose of this option, and I respect it, however if an option is made visible in tls.Config a percentage of users will see it and enable it.

SNI stops being useful as soon as even a small percentage of clients don't send it, because if the server can't afford to break those clients it can't start hosting multiple sites on the same IP.

As I mentioned, applications that are sure they need to remove SNI (like anti-censorship applications) can use InsecureSkipVerify + VerifyPeerCertificate, and leave ServerName empty. SNI will not be sent in that case.

@lvht

This comment has been minimized.

Copy link
Author

@lvht lvht commented Nov 18, 2018

if an option is made visible in tls.Config a percentage of users will see it and enable it.

I do not think the see-it-and-enable-it style make sense. For example, user also see the MinVersion and set it to a higher verson like TLS1.3, and the server with only TLSv1.2 support will be break. Besides, the SkipSNI is closed by default. Setting it to be true without thinking just like write loop without a break condition.

And the RFC also reads

Additionally, all implementations MUST support the use of the
"server_name" extension with applications capable of using it.
Servers MAY require clients to send a valid "server_name" extension.
Servers requiring this extension SHOULD respond to a ClientHello
lacking a "server_name" extension by terminating the connection with
a "missing_extension" alert.

If the server depends the SNI extension, it should alert the "missing_extension".

@lvht

This comment has been minimized.

Copy link
Author

@lvht lvht commented Nov 18, 2018

Anyway, thanks for your @FiloSottile InsecureSkipVerify + VerifyPeerCertificate solution.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 5, 2019

Change https://golang.org/cl/193620 mentions this issue: crypto/tls: add ExampleConfig_VerifyPeerCertificate

gopherbot pushed a commit that referenced this issue Nov 9, 2019
Setting InsecureSkipVerify and VerifyPeerCertificate is the recommended
way to customize and override certificate validation.

However, there is boilerplate involved and it usually requires first
reimplementing the default validation strategy to then customize it.
Provide an example that does the same thing as the default as a starting
point.

Examples of where we directed users to do something similar are in
issues #35467, #31791, #28754, #21971, and #24151.

Fixes #31792

Change-Id: Id033e9fa3cac9dff1f7be05c72dfb34b4f973fd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/193620
Reviewed-by: Adam Langley <agl@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.