advancedTLS: Rename {Min/Max}Version to {Min/Max}TLSVersion #7173
Conversation
| // client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum | ||
| // supported by this package, both as a client and as a server. This | ||
| // default may be changed over time affecting backwards compatibility. |
There was a problem hiding this comment.
Are we reserving the right to override the user's setting? Like if they set 1.0 and we decide to set the min to 1.2, then we'll automatically upgrade them? Or no?
There was a problem hiding this comment.
I don't think so - if we decide to change the minimum supported version from 1.0 to 1.2, I think we'd add in a check that throws an error rather than override the user setting.
There was a problem hiding this comment.
That's fine. It seems like when they made a change like this in Go's tls package they added an env var so users could get the old 1.0-default behavior back (just FYI).
Also, I assume this is supposed to be a tls.VersionTLSxx value? (I wish they had made a type name for it.) That might be worth documenting.
There was a problem hiding this comment.
I think we could explore adding an env var like that down the line if it ever happens that we need it
Added notes in the docs about using tls.VersionTLSxx
There was a problem hiding this comment.
Added notes in the docs about using tls.VersionTLSxx
What doc? The godocs? If so, I think you forgot to push the change. It seems worth documenting in these comments, since this isn't inside the tls package, but I won't argue too strongly.
There was a problem hiding this comment.
Whoops I didn't actually push the change to my remote branch, done now
This PR renames the
MinVersionandMaxVersiononClientOptionsandServerOptionstoMinTLSVersionandMaxTLSVersionto make it clear that this field represents the TLS version.It also adds logic to set the default min and max to TLS12 and TLS13 respectively, and a tests for that logic.
Previously our code comments indicated this was the default, but we really just delegated to the defaults of the crypto/tls package (which are currently the same). Now we explicitly set to the defaults that we say we do, rather than depending on a dependency's defaults to match ours.
RELEASE NOTES: none