-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: remove TLS1.0, TLS1.1 support #50348
Conversation
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -187,7 +187,7 @@ func (d *DBStore) adjust( | |||
if d.Security.TLSConfig == nil { | |||
/* #nosec G402 */ | |||
d.Security.TLSConfig = &tls.Config{ | |||
MinVersion: tls.VersionTLS10, | |||
MinVersion: tls.VersionTLS12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumpling and lightning may connect to older version of TiDB and MySQL, and their TLS version is older than v1.2. Should we support this case @Frank945946
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLSv1.2 is introduced in year 2008, 15 years ago, that's old enough.
And as long as those software support TLSv1.2 above, change this MinVersion will not cause any trouble.
Without doubt, that's the case for TiDB.
And then I check the release history of MySQL ...
Release General availability Latest release
5.5 LTS 3 December 2010 2018-10-22
5.6 LTS 5 February 2013 2021-01-20
5.7 LTS 21 October 2015 2023-10-25
8.0 LTS 19 April 2018 2023-10-25
I'm sure it's pretty safe to do this change according to the above information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not so 'safe' actually. We do know many TiDB users still using old clients. However, it is reasonable to remove the support of 'TLS 1.0/1.1'.
Thanks @lance6716
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the support of 'TLS 1.0/1.1' LTGM and please leave a note to the corresponding docs then @lance6716 .
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #50348 +/- ##
=================================================
- Coverage 71.8223% 55.4665% -16.3559%
=================================================
Files 1444 1555 +111
Lines 346984 587587 +240603
=================================================
+ Hits 249212 325914 +76702
- Misses 77425 238950 +161525
- Partials 20347 22723 +2376
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[LGTM Timeline notifier]Timeline:
|
@@ -187,7 +187,7 @@ func (d *DBStore) adjust( | |||
if d.Security.TLSConfig == nil { | |||
/* #nosec G402 */ | |||
d.Security.TLSConfig = &tls.Config{ | |||
MinVersion: tls.VersionTLS10, | |||
MinVersion: tls.VersionTLS12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the support of 'TLS 1.0/1.1' LTGM and please leave a note to the corresponding docs then @lance6716 .
@Frank945946: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, Frank945946, hawkingrei, lance6716, okJiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: ref #36036
Problem Summary:
What changed and how does it work?
TLS1.0/1.1 has security issues, remove the support of them.
Check List
Tests
Generate the certificates according to this page https://docs.pingcap.com/tidb/stable/generate-self-signed-certificates
Modify pkg/config/config.toml.example
Run tidb-server:
Verify connection with mysql client:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.