Skip to content

Add tls support for mysql client#186

Merged
Yui-Song merged 13 commits intopingcap:masterfrom
db-will:add-tls-support
Jan 9, 2025
Merged

Add tls support for mysql client#186
Yui-Song merged 13 commits intopingcap:masterfrom
db-will:add-tls-support

Conversation

@db-will
Copy link
Copy Markdown
Contributor

@db-will db-will commented Nov 24, 2024

Close: #185

Add tls config for connecting mysql with tls enabled

Manually test:

[ec2-user@ip-10-0-19-25 cert]$ ./go-tpc -H 10.0.153.232 -U u1 -P 4000 tpcc --warehouses 1 prepare
2024/11/23 21:42:36 maxprocs: Leaving GOMAXPROCS=4: CPU quota undefined
failed to ping db, err Error 1045 (28000): Access denied for user 'u1'@'10.0.19.25' (using password: NO)
panic: failed to connect to database when loading data

goroutine 1 [running]:
github.com/pingcap/go-tpc/tpcc.NewWorkloader(0x59c338?, 0xc0003e9540?)
        /Users/Projects/db-will/go-tpc/tpcc/workload.go:110 +0x8bd
main.executeTpcc({0xb4c776, 0x7})
        /Users/Projects/db-will/go-tpc/cmd/go-tpc/tpcc.go:66 +0x205
main.registerTpcc.func1(0xc000144900?, {0xb49b4c?, 0x4?, 0xb49b50?})
        /Users/Projects/db-will/go-tpc/cmd/go-tpc/tpcc.go:96 +0x1f
github.com/spf13/cobra.(*Command).execute(0xc0003a9b88, {0xc00012a380, 0x8, 0x8})
        /Users/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:846 +0x671
github.com/spf13/cobra.(*Command).ExecuteC(0xc0003a9348)
        /Users/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:950 +0x389
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:887
main.main()
        /Users/Projects/db-will/go-tpc/cmd/go-tpc/main.go:274 +0x846
[ec2-user@ip-10-0-19-25 cert]$ ./go-tpc -H 10.0.153.232 -U u1 -P 4000 --ssl-ca ./root.crt --ssl-cert ./client.crt --ssl-key ./client.key  tpcc --warehouses 1 clean
2024/11/23 21:42:54 maxprocs: Leaving GOMAXPROCS=4: CPU quota undefined
DROP TABLE IF EXISTS item
DROP TABLE IF EXISTS customer
DROP TABLE IF EXISTS district
DROP TABLE IF EXISTS history
DROP TABLE IF EXISTS new_order
DROP TABLE IF EXISTS order_line
DROP TABLE IF EXISTS orders
DROP TABLE IF EXISTS stock
DROP TABLE IF EXISTS warehouse
Finished

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Nov 24, 2024

CLA assistant check
All committers have signed the CLA.

db-will and others added 3 commits January 1, 2025 16:32
Co-authored-by: Daniël van Eeden <github@myname.nl>
Co-authored-by: Daniël van Eeden <github@myname.nl>
Co-authored-by: Daniël van Eeden <github@myname.nl>
switch driver {
case mysqlDriver:
var tlsName string = "preferred"
if len(sslCA) > 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(sslCA) > 0 {
if len(sslCA) > 0 || len(sslKey) > 0 || len(sslCert) > 0 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refer https://docs.pingcap.com/tidb/stable/enable-tls-between-clients-and-servers#enable-authentication

By default, the server-to-client authentication is optional. Even if the client does not present its certificate of identification during the TLS handshake, the TLS connection can be still established.

If sslKey and sslCert are not provided, we will only authenticate the TiDB server from the MySQL client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think valid configurations are:

  • Only sslCA provided
  • sslCA, sslCert, sslKey all provided.

Not valid:

  • sslCert and/or sslKey set, but no sslCA.
  • sslCert set, but not sslKey
  • sslKey set, but not sslCert

So I think we should set the tlsName to custom if any of sslCert, sslKey or sslCA is set and then later on verify if it is valid (which might already be done)

db-will and others added 4 commits January 3, 2025 11:54
Co-authored-by: Daniël van Eeden <github@myname.nl>
Co-authored-by: Daniël van Eeden <github@myname.nl>
Co-authored-by: Daniël van Eeden <github@myname.nl>
@dveeden dveeden requested a review from Yui-Song January 4, 2025 09:53
Nayawana added a commit to Nayawana/go-tpc that referenced this pull request Jan 5, 2025
…ent-2495711184

github.com/spf13/cobra.(*Command).ExecuteC(0xc0003a9348)
db-will and others added 3 commits January 5, 2025 20:41
Co-authored-by: Daniël van Eeden <github@myname.nl>
Co-authored-by: Daniël van Eeden <github@myname.nl>
Copy link
Copy Markdown
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

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

Please make sure this handles the situation correctly if sslKey or sslCert is set, but not sslCA. Otherwise LGTM.

Co-authored-by: Daniël van Eeden <github@myname.nl>
@Yui-Song
Copy link
Copy Markdown
Collaborator

Yui-Song commented Jan 8, 2025

@dveeden Please sign the CLA.

@Yui-Song Yui-Song merged commit 6cd9f74 into pingcap:master Jan 9, 2025
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.

Add support for tls connection

4 participants