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

configtls: TLS 1.2 is the new default min version #4503

Merged

Conversation

jpkrohling
Copy link
Member

Addresses CWE-327 by setting the minimum TLS version to 1.2. Lower versions can still be set for backwards compatibility with older network appliances, although I don't think any of them are that old to still not have 1.2 available.

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling jpkrohling requested review from a team and Aneurysm9 December 2, 2021 09:35
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #4503 (565102e) into main (7402e94) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4503   +/-   ##
=======================================
  Coverage   90.67%   90.67%           
=======================================
  Files         179      179           
  Lines       10414    10414           
=======================================
  Hits         9443     9443           
  Misses        755      755           
  Partials      216      216           
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 7402e94...565102e. Read the comment docs.

@@ -178,7 +178,7 @@ func (c TLSServerSetting) LoadTLSConfig() (*tls.Config, error) {

func convertVersion(v string) (uint16, error) {
if v == "" {
return 0, nil // default
return tls.VersionTLS12, nil // default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change in the CHANGELOG?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change, though? I wouldn't expect any OpenTelemetry Collector setup to break because of this change. Only setups using very old firewalls/load balancers would break, causing the operator to have the need to specify a lower version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If folks relied on TLS < 1.2, then upgrading to this new collector would break their deployments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relied on TLS

Right, but this is the part that I find unlikely: who's relying on TLS nowadays? I can only think of legacy applications that were built a long time ago when 1.2 wasn't available as part of the underlying TLS libraries (openssl and alike). Everything around it, like routers, proxies, and load balancers likely support 1.2 already. Given that the collector is the piece that sits behind a proxy/router/load balancer, and we know it supports 1.2, I find it highly unlikely we'll have problems.

If you think it's worth being extra-cautious, I can send in a PR that moves this entry to the breaking change part.

@bogdandrutu bogdandrutu merged commit 9309a1f into open-telemetry:main Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants