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

crypto/tls: set TLSPlaintext.version to MinVersion #31104

Open
riraccuia opened this Issue Mar 28, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@riraccuia
Copy link

commented Mar 28, 2019

In its current state, the crypto/tls package does not allow an implementer to choose whether a TLS 1.2 client should present the same TLS version (0x0303) in either record and handshake layers of the ClientHello message.

The current default behavior just sets the version in the record layer to 1.0 to maintain compatibility with legacy/buggy TLS server implementations.

This problem is described in RFC 5246 Appendix E.1

This may apply to TLS 1.1 as well

@FiloSottile @bradfitz

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Apologies for responding to #31079 before seeing this.

I still don't see the compelling use case for this config option. I'm aware of no servers that require 0x0303 as the protocol version, which would be off-spec behavior.

When we have the option to select a default that works for everyone, we always prefer it to exposing a config option and delegating the complexity and learning requirement to the application.

@FiloSottile FiloSottile changed the title crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 proposal: crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 Mar 28, 2019

@gopherbot gopherbot added this to the Proposal milestone Mar 28, 2019

@gopherbot gopherbot added the Proposal label Mar 28, 2019

@gopherbot gopherbot added the Proposal label Mar 28, 2019

@dmagyar

This comment has been minimized.

Copy link

commented Mar 28, 2019

In our use-case an IPS is blocking negotiation when attempted at <TLS1.2. Being a network appliance this decision is made by observing ClientHello and therefore even if I set MinVersion to 1.2 I'm getting blocked. Understand that the default works for most use-cases but there could be edge cases where a proper version in ClientHello is required. All modern browsers adhere to proper TLS versions, so far the only issue I had is with Golangs' TLS :(

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

If the IPS is abusing TLSPlaintext.version as a sort of minimum supported version, it is not compliant with RFC 5246. From Section E.1:

   Earlier versions of the TLS specification were not fully clear on
   what the record layer version number (TLSPlaintext.version) should
   contain when sending ClientHello (i.e., before it is known which
   version of the protocol will be employed).  Thus, TLS servers
   compliant with this specification MUST accept any value {03,XX} as
   the record layer version number for ClientHello.

   TLS clients that wish to negotiate with older servers MAY send any
   value {03,XX} as the record layer version number.  Typical values
   would be {03,00}, the lowest version number supported by the client,
   and the value of ClientHello.client_version.  No single value will
   guarantee interoperability with all old servers, but this is a
   complex topic beyond the scope of this document.

I don't know what you mean about browsers, but my Chrome instance sends 0x0301 as TLSPlaintext.version. I suppose we could set it to tls.Config.MinVersion, rather than make it configurable, if that's what you need.

@dmagyar

This comment has been minimized.

Copy link

commented Mar 28, 2019

I would love that too. If MinVersion is not set then default to current behavior.

@FiloSottile FiloSottile changed the title proposal: crypto/tls: add StrictVersionNegotiation config field for TLS 1.2 crypto/tls: set TLSPlaintext.version to MinVersion Mar 28, 2019

@FiloSottile FiloSottile modified the milestones: Proposal, Go1.13 Mar 28, 2019

@riraccuia

This comment has been minimized.

Copy link
Author

commented Mar 29, 2019

Thanks @FiloSottile ,
setting it to MinVersion is actually perfect and i don't see any downsides with this approach.
I should have thought of that before submitting a new API proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.