Fixing incorrect default TLS min and TLS max versions #5480

bushwhackr · 2022-06-06T04:05:06Z

Description: Fixing a bug, default TLS version and description does not match

Link to tracking Issue: #5430

linux-foundation-easycla · 2022-06-06T04:05:09Z

The committers listed above are authorized under a signed CLA.

✅ login: bushwhackr / name: Chester (24c3467)

bushwhackr · 2022-06-06T04:47:26Z

wondering if just using the crypto/tls defaults of TLS1.2 as client and TLS1.0 as server for min_version, and TLS1.3 as max_version would be a better solution. This would require otelcol to not set a default value for "min_version" and "max_version" during its creation of tls.Config.

Default in crypto/tls
https://github.com/golang/go/blob/47f806ce81aac555946144f112b9f8733e2ed871/src/crypto/tls/common.go#L684

crypto/tls function that sets the allowed TLS versions
https://github.com/golang/go/blob/47f806ce81aac555946144f112b9f8733e2ed871/src/crypto/tls/common.go#L984

codecov · 2022-06-07T07:20:49Z

Codecov Report

Merging #5480 (9642cef) into main (b094dbd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9642cef differs from pull request most recent head d67c723. Consider uploading reports for the commit d67c723 to get more accurate results

@@           Coverage Diff           @@
##             main    #5480   +/-   ##
=======================================
  Coverage   91.29%   91.29%           
=======================================
  Files         191      191           
  Lines       11291    11292    +1     
=======================================
+ Hits        10308    10309    +1     
  Misses        782      782           
  Partials      201      201

Impacted Files	Coverage Δ
config/configtls/configtls.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b094dbd...d67c723. Read the comment docs.

bogdandrutu

Can we add tests for this?

bogdandrutu

Also, you are not fixing the description, you are fixing code, so please modify the title accordingly.

config/configtls/configtls.go

bogdandrutu · 2022-06-09T15:22:36Z

config/configtls/configtls.go

+	// Setting default TLS minVersion
+	if c.MinVersion == "" {
+		c.MinVersion = "1.0"
+	}
+	// Setting default TLS maxVersion
+	if c.MaxVersion == "" {
+		c.MaxVersion = "1.3"
+	}


We should adopt the same guarantees as the golang TLS (I looked at their config, and here is what I found):

package tls type Config struct { // By default, TLS 1.2 is currently used as the minimum when acting as a // 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. MinVersion uint16 // By default, the maximum version supported by this package is used, // which is currently TLS 1.3. MaxVersion uint16 }

Currently we are enforcing "1.2" as min/max:

Increasing max on both client/server is backwards compatible since supposedly greater version is more secure.

Decreasing min on the server (backend) is backwards, allows more clients to connect.

Decreasing min on the client is not OK I think, since that may result in a connection established with lower security guarantees.

@tigrannajaryan @codeboten do you have an opinion here?

@bogdandrutu your analysis look correct to me. So, essentially you are saying that we want:

On clients if MinVersion is missing then set it to 1.2.

On servers if MinVersion is missing then set it to 1.0.

Both on clients and servers if MaxVersion is missing then set it to 1.3.

refering to https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L984

Not setting a default value at all crypto/tls will handle the TLS minVersion and maxVersions for us. Should I proceed with updating the PR?

Not setting a default value at all crypto/tls will handle the TLS minVersion and maxVersions for us. Should I proceed with updating the PR?

That's a good point. Just pass the zero values and let crypto/tl handle the defaults.

I've updated the PR so that we do not add a default via opentelemetry, crypto/tls will handle the default TLS versions.

My understand is that this PR is about fixing the defaults to match the default of what the Go's standard library defines, so let's keep it that way. The code in this PR looks correct to me as it is.

If we want to require higher version number than Go's standard library then it is best to discuss it in in a separate issue/PR.

I agree that we should follow the Go standard library approach, but it's important to clarify what Go version we are talking about. As discussed in golang/go/issues/45428, and as can be seen in golang/go@035963c the situation differs depending on the Go version:

In Go 1.17, the default is TLS 1.0 for both the client and the server

In Go 1.18+, the default is TLS 1.2 for the client and TLS 1.0 for the server

So this PR will mean

a downgrade on the server min version for binaries compiled with all Go versions

a downgrade in the client min version when used in a binary compiled with Go 1.17

Are we fine with that? I would feel more comfortable switching to build the official binaries with Go 1.18 first and after that doing this change, so that we don't decrease the client min version, but I don't feel very strongly about this

This change just delegates to the golang compiler/vm to pick the min/max TLS. Am I wrong? We are already building the binaries with golang 1.8 (see https://github.com/open-telemetry/opentelemetry-collector-releases/blob/main/.github/workflows/ci-goreleaser.yaml#L35), but we have "go 1.17" in the go.mod. Does that mean that the min version is 1.0?

Oh, I didn't realize we were already using Go 1.18 on the official binaries. Then I believe regardless of what go.mod says, this will not mean a change on the official binaries, so I feel like doing this is okay

Just rebased the PR to main, is there anything blocking this PR now? I'll be happy to follow through with the changes.

The reason to block this PR is fixed, still comments that need to be resolve.

codeboten

Just one comment

config/configtls/configtls.go

mx-psi

LGTM, but there is one unrelated change. Could you move it to a different PR?

docs/release.md

codeboten · 2022-06-24T20:56:48Z

@tigrannajaryan @bogdandrutu please review and approve the PR if your concerns have been addressed.

jpkrohling · 2022-07-04T18:57:57Z

config/configtls/README.md

@@ -36,10 +36,11 @@ won't use TLS at all.

 Minimum and maximum TLS version can be set:

- `min_version` (default = "1.2"): Minimum acceptable TLS version.
-It's recommended to use at least 1.2 as the minimum version.
+- `min_version` (default = "" handled by [crypto/tls](https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L694)): Minimum acceptable TLS version.


As I understood it, 1.0 is used as the default version on the server-side. This is not what we want here, on a newer server/library: the recommendation for a few years has been already to default to 1.2 as the minimum version, allowing users to use 1.0 or SSLvX only when absolutely needed.

bushwhackr requested review from a team and mx-psi June 6, 2022 04:05

bogdandrutu reviewed Jun 7, 2022

View reviewed changes

bogdandrutu previously requested changes Jun 7, 2022

View reviewed changes

bushwhackr force-pushed the main branch from 24c3467 to 4499f59 Compare June 7, 2022 18:49

bushwhackr changed the title ~~Fix incorrect description for default TLS versions~~ Fixing incorrect default TLS min and TLS max versions Jun 7, 2022

bushwhackr commented Jun 7, 2022

View reviewed changes

config/configtls/configtls.go Outdated Show resolved Hide resolved

bushwhackr requested a review from bogdandrutu June 8, 2022 05:17

bogdandrutu reviewed Jun 9, 2022

View reviewed changes

codeboten reviewed Jun 9, 2022

View reviewed changes

config/configtls/configtls.go Outdated Show resolved Hide resolved

bushwhackr added 6 commits June 23, 2022 13:20

Fixing incorrect TLS min and TLS max Versions by default

1231434

Added test cases for TLS min and TLS max versions

ecb7cb0

Added entry to changelog

486bdce

Added Invalid TLS test cases

f6b19f5

TLS MinVersion and TLS MaxVersion defaults to be handled by crypto/tls

5158851

Removed description that can go out of sync

d67c723

bushwhackr force-pushed the main branch from be793ad to 9642cef Compare June 23, 2022 05:24

mx-psi reviewed Jun 23, 2022

View reviewed changes

docs/release.md Outdated Show resolved Hide resolved

bushwhackr force-pushed the main branch from 9642cef to d67c723 Compare June 23, 2022 07:35

mx-psi approved these changes Jun 23, 2022

View reviewed changes

codeboten approved these changes Jun 23, 2022

View reviewed changes

dmitryax approved these changes Jun 23, 2022

View reviewed changes

bogdandrutu approved these changes Jun 27, 2022

View reviewed changes

bogdandrutu merged commit c0a735f into open-telemetry:main Jun 27, 2022

jpkrohling reviewed Jul 4, 2022

View reviewed changes

jpkrohling mentioned this pull request Jul 4, 2022

Revert change to min TLS version #5627

Closed

rapphil mentioned this pull request Aug 24, 2022

Use TLS 1.2 by default when min_version is not defined #5956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing incorrect default TLS min and TLS max versions #5480

Fixing incorrect default TLS min and TLS max versions #5480

bushwhackr commented Jun 6, 2022

linux-foundation-easycla bot commented Jun 6, 2022 •

edited

Loading

bushwhackr commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading

bogdandrutu left a comment

bogdandrutu left a comment

bogdandrutu Jun 9, 2022

tigrannajaryan Jun 9, 2022

bushwhackr Jun 9, 2022

tigrannajaryan Jun 9, 2022

bushwhackr Jun 9, 2022

tigrannajaryan Jun 20, 2022

mx-psi Jun 21, 2022

bogdandrutu Jun 21, 2022

mx-psi Jun 22, 2022

bushwhackr Jun 23, 2022

codeboten left a comment

mx-psi left a comment

codeboten commented Jun 24, 2022

jpkrohling Jul 4, 2022

Fixing incorrect default TLS min and TLS max versions #5480

Fixing incorrect default TLS min and TLS max versions #5480

Conversation

bushwhackr commented Jun 6, 2022

linux-foundation-easycla bot commented Jun 6, 2022 • edited Loading

bushwhackr commented Jun 6, 2022 • edited Loading

codecov bot commented Jun 7, 2022 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

codeboten commented Jun 24, 2022

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jun 6, 2022 •

edited

Loading

bushwhackr commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading