Backport TLS13-KDF to 1.24 release branch#1662
Conversation
|
I like the approach taken in this PR to filter TLS 1.3 KDF on the backend package. After reviewing this PR, I now think that using a goexperiment is an overkill, better use a good old build tag to gate the new KDF. |
|
Should I change the string |
My two cents... there's an odd tradeoff here for a one-major-version-only build tag:
Another option would be a That said, I don't think it's unreasonable to start with a tag. (Simplest.)
Yeah, to avoid the freshly untrue implication that the GOEXPERIMENT system is involved. |
| +// Use of this source code is governed by a BSD-style | ||
| +// license that can be found in the LICENSE file. | ||
| + | ||
| +//go:build goexperiment.opensslcrypto && goexperiment.ms_tls13kdf && linux && cgo |
There was a problem hiding this comment.
I feel it would be better to make this independent. Tying ms_tls13kdf to the return value of SupportsTLS13KDF means "splitting" openssl_linux.go into more files, making the backend system more complicated to maintain. I don't think tls13kdf_default.go and tls13kdf_ms_kdftls13.go need to care about goexperiment.opensslcrypto, linux, or cgo. Adding another check to if boring.Enabled && boring.SupportsTLS13KDF() { seems like it would also convey the intent more clearly.
(It's not as if setting this tag actually affects whether this algorithm is supported, it only means that the algorithm should be used if it is supported.)
There was a problem hiding this comment.
Ok, reworked this. These files are now only controlled by //go:build ms_tls13kdf. And, they only set a new global boolean, TLS13KDFEnabled. crypto/tls/internal/tls13/tls13.go was updated to check the new boolean, before making a runtime support check.
This:
if boring.Enabled && boring.SupportsTLS13KDF() {
was changed to this:
if boring.Enabled && boring.TLS13KDFEnabled && boring.SupportsTLS13KDF() {
b62413e to
df6436e
Compare
| + | ||
| +package backend | ||
| + | ||
| +const TLS13KDFEnabled = true |
There was a problem hiding this comment.
Apologies if you preferred the original @qmuntal. I just saw in your comment that you might have. 😄
My semi-outside perspective is that this is more "use the TLS13KDF backend function for TLS 1.3", and there could in theory be other uses for the backend function, so it doesn't necessarily make sense to include it in SupportsTLS13KDF and a more direct reference is reasonable. But maybe that's not a reasonable way to look at it given the OpenSSL API's name matching the scenario exactly.
A middle ground would be to change OpenSSL's SupportsTLS13KDF to return TLS13KDFEnabled && openssl.SupportsTLS13KDF(). This would preserve the independent files and their maintainability, but incorporate the tag's const into SupportsTLS13KDF rather than handling it in src/crypto/tls/internal/tls13/tls13.go.
I'd be fine with either one.
46d6bb6
into
microsoft:microsoft/release-branch.go1.24
|
Thanks for the help!! |
Issue: #1658
internal/backend/tls13kdf_default.goandinternal/backend/tls13kdf_ms_kdftls13.goinpatches/0003-Implement-crypto-internal-backend.patch. The files are conditionally compiled via//go:build goexperiment.opensslcrypto && !goexperiment.ms_tls13kdf && linux && cgo//go:build !ms_tls13kdfand//go:build goexperiment.opensslcrypto && goexperiment.ms_tls13kdf && linux && cgo//go:build ms_tls13kdf, respectively. So, without adding the new build flag,ms_tls13kdf,TLS13-KDFwill not be used.