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

pkg: refine tls && update tidb version #323

Merged
merged 12 commits into from
Mar 12, 2020

Conversation

WangXiangUSTC
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC commented Mar 11, 2020

What problem does this PR solve?

  1. need to support online reload for tls Support TLS and online reload with new certs dm#467
  2. need to support CN certification Validate CommonName attribute for TLS certificate dm#524

What is changed and how it works?

  1. refine pkg for tls, for example, provide some function for grpc to use reload tls file, most code is copied from https://github.com/pingcap/tidb-lightning/blob/master/lightning/common/security.go
  2. add certification for Common Name in CA

Check List

Tests

  • Unit test

@WangXiangUSTC WangXiangUSTC changed the title pkg: refine tls pkg: refine tls && update tidb version Mar 11, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

pkg/utils/security.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

@kennytm @csuzhangxc PTAL

pkg/utils/security.go Outdated Show resolved Hide resolved
pkg/utils/security.go Outdated Show resolved Hide resolved
pkg/utils/security.go Outdated Show resolved Hide resolved
pkg/utils/tls_test/ca.csr Outdated Show resolved Hide resolved
Comment on lines +136 to +138
// client2 can't visit server
_, err = ClientWithTLS(clientTLS2).Get(url)
c.Assert(err, ErrorMatches, ".*tls: bad certificate")
Copy link
Contributor

Choose a reason for hiding this comment

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

will the server accept client2 if the common name is localhost (the CA's common name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, you will see the log of unit test:

2020/03/12 12:21:44 http: TLS handshake error from 127.0.0.1:51864: client certificate authentication failed. The Common Name from the client certificate [client2 localhost] was not found in the configuration cluster-verify-cn with value: [client1]

the common name already include localhost

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC merged commit f5b83d4 into pingcap:master Mar 12, 2020
@WangXiangUSTC WangXiangUSTC deleted the xiang/tls branch March 12, 2020 11:03
@WangXiangUSTC WangXiangUSTC restored the xiang/tls branch March 12, 2020 11:03
@WangXiangUSTC WangXiangUSTC deleted the xiang/tls branch March 12, 2020 11:03
@WangXiangUSTC WangXiangUSTC restored the xiang/tls branch March 12, 2020 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants