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

*: support reload tls used by mysql protocol in place #14749

Merged
merged 7 commits into from
Mar 3, 2020
Merged

*: support reload tls used by mysql protocol in place #14749

merged 7 commits into from
Mar 3, 2020

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Feb 12, 2020

What problem does this PR solve?

ref #14666

preliminary support reload tls used by mysql protocol

this PR doesn't try to full support mysql's dynamic modify "ssl_ca/ssl_key/ssl_cert" value, but can reload tls used old file path specified by old "ssl_ca/ssl_key/ssl_cert" value(so ssl_cert/ssl_ca/ssl_key keep read-only after this PR).

so user can:

  1. start TiDB with ssl-ca, ssl-key and ssl-cert config like https://pingcap.com/docs/stable/reference/security/cert-based-authentication/#install-openssl
  2. replace new file specified in ssl-ca, ssl-key and ssl-cert
  3. use super user(here need new priv in following pr) to execute alter instance reload tls

then all new db connection will use new cert file, old connection will keep work just like mysql does

What is changed and how it works?

  • extract common method LoadTLSCertificates
  • make server.tlsConfig can be atomic swap
  • let alter instance reload tls do reload

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • impl change

Side effects

  • n/a

Related changes

  • 4.0 only

Release note

  • support reload tls used by mysql protocol in place.

This change is Reviewable

@lysu lysu added type/enhancement The issue or PR belongs to an enhancement. status/WIP component/server labels Feb 12, 2020
@lysu lysu requested a review from a team as a code owner February 12, 2020 12:35
@ghost ghost requested review from alivxxx and winoros and removed request for a team February 12, 2020 12:35
@lysu lysu requested review from a team and removed request for alivxxx and winoros February 12, 2020 12:35
@ghost ghost requested review from eurekaka and lzmhhh123 and removed request for a team February 12, 2020 12:35
@lysu lysu removed the status/WIP label Feb 18, 2020
@lysu lysu requested review from tiancaiamao and jackysp February 18, 2020 09:27
executor/simple.go Outdated Show resolved Hide resolved
executor/simple.go Outdated Show resolved Hide resolved
@shenli shenli added the security Everything related with security label Feb 18, 2020
server/server.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

[2020-02-19T06:31:54.630Z] ----------------------------------------------------------------------
[2020-02-19T06:31:54.630Z] FAIL: tidb_test.go:468: tidbTestSuite.TestReloadTLS
[2020-02-19T06:31:54.630Z] 
[2020-02-19T06:31:54.630Z] tidb_test.go:524:
[2020-02-19T06:31:54.630Z]     c.Assert(err, IsNil)
[2020-02-19T06:31:54.631Z] ... value tls.RecordHeaderError = tls.RecordHeaderError{Msg:"first record does not look like a TLS handshake", RecordHeader:[5]uint8{0x17, 0x0, 0x0, 0x2, 0xff}, Conn:(*net.TCPConn)(0xc003beb3b0)} ("tls: first record does not look like a TLS handshake")
[2020-02-19T06:31:54.631Z] 
[2020-02-19T06:31:54.631Z] 
[2020-02-19T06:31:54.631Z] ----------------------------------------------------------------------

@kolbe
Copy link
Contributor

kolbe commented Feb 27, 2020

I'd like to see this backported to 3.1 and maybe 3.0.

@lysu
Copy link
Contributor Author

lysu commented Feb 28, 2020

/rebuild

1 similar comment
@lysu
Copy link
Contributor Author

lysu commented Feb 28, 2020

/rebuild

@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #14749 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14749   +/-   ##
===========================================
  Coverage   80.6497%   80.6497%           
===========================================
  Files           502        502           
  Lines        133409     133409           
===========================================
  Hits         107594     107594           
  Misses        17426      17426           
  Partials       8389       8389           

@lysu
Copy link
Contributor Author

lysu commented Feb 28, 2020

/rebuild

@lysu
Copy link
Contributor Author

lysu commented Mar 2, 2020

@jackysp @tiancaiamao finally it passes all test, PTAL tks~

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 2, 2020
@lysu lysu requested a review from imtbkcat March 2, 2020 14:08
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 3, 2020
@tiancaiamao
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 3, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot sre-bot merged commit 5c68d53 into pingcap:master Mar 3, 2020
@lysu lysu deleted the dev-support-reload-client-tls branch March 3, 2020 02:02
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 3, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

cherry pick to release-3.0 in PR #15080

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

cherry pick to release-4.0 in PR #15081

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @lysu PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server security Everything related with security status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants