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

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

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

rapphil
Copy link
Contributor

@rapphil rapphil commented Aug 24, 2022

Description:

Use TLS 1.2 by default when min_version is not defined.

TLS 1.0 and 1.1 are widely deprecated. It does not make sense to offer users with an insecure default.

This PR changes the default TLS min_version for both server and client to 1.2. This is reverting part of a recent change: #5480

Link to tracking Issue: #5627

Testing: Unit tests were modified to account for this case.

Documentation: Documentation was updated describing the new default.

@rapphil rapphil requested review from a team and jpkrohling August 24, 2022 01:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 24, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rapphil / name: Raphael Philipe Mendes da Silva (05aa1c9)

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I'm all for this change, which has been the case in the past, but a recent change made our code relay the default to go's crypto/tls package. Would you be able to dig that issue and link it here? A git blame on the "// Defaults will ..." comment should reveal the PR/issue that changed that.

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG.md entry.

@rapphil rapphil force-pushed the rapphil-use-tls1_2_default branch from 05aa1c9 to fb90e67 Compare August 24, 2022 17:57
@rapphil
Copy link
Contributor Author

rapphil commented Aug 24, 2022

I'm all for this change, which has been the case in the past, but a recent change made our code relay the default to go's crypto/tls package. Would you be able to dig that issue and link it here? A git blame on the "// Defaults will ..." comment should reveal the PR/issue that changed that.

I added the previous PR in the description.

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #5956 (08ac857) into main (940943c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5956   +/-   ##
=======================================
  Coverage   92.24%   92.24%           
=======================================
  Files         212      212           
  Lines       13263    13263           
=======================================
  Hits        12235    12235           
  Misses        811      811           
  Partials      217      217           
Impacted Files Coverage Δ
config/configtls/configtls.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jpkrohling
Copy link
Member

@bogdandrutu, in the previous PR, you seemed to favor delegating the default protocol to the underlying library. I believe setting 1.2 as the min version is the right thing to do here, despite the underlying library setting a lower min version, but it would be good to have your OK on this before moving forward.

@bogdandrutu
Copy link
Member

Usually I prefer to be consistent with the standard library. Not an expert in security, and not sure what it means to use the deprecated version.

@rapphil
Copy link
Contributor Author

rapphil commented Aug 25, 2022

Usually I prefer to be consistent with the standard library. Not an expert in security, and not sure what it means to use the deprecated version.

I agree with your approach, but I think it is not applicable in this case because promoting security best practices with secure defaults has higher priority.

I'm not a security expert either but I think we can trust open standards to guide us here:
https://datatracker.ietf.org/doc/rfc8996/

Removing support for older versions from implementations reduces the
attack surface, reduces opportunity for misconfiguration, and
streamlines library and product maintenance.

The RFC references other sources that describe possible attacks which TLS1.0 and 1.1 are vulnerable to.

I think we should even remove the option of using TLS1.0 and 1.1 altogether, but that is another discussion :)

@jpkrohling
Copy link
Member

In security circles, people just assume TLS 1.0 and 1.1 are effectively broken. There might be people out there with good reasons to use those versions, so I wouldn't artificially limit its usage, but I would definitely have 1.2 as the default min version.

CHANGELOG.md Outdated Show resolved Hide resolved
@rapphil rapphil force-pushed the rapphil-use-tls1_2_default branch from 42c6daa to 08d5599 Compare August 25, 2022 17:20
@rapphil
Copy link
Contributor Author

rapphil commented Aug 26, 2022

@bogdandrutu @jpkrohling
Is there anything else that I can do to get your approvals?

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