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

Add tls support for mysql client #186

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

db-will
Copy link

@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/[email protected]/command.go:846 +0x671
github.com/spf13/cobra.(*Command).ExecuteC(0xc0003a9348)
        /Users/go/pkg/mod/github.com/spf13/[email protected]/command.go:950 +0x389
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/go/pkg/mod/github.com/spf13/[email protected]/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

CLAassistant commented Nov 24, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ db-will
❌ dveeden
You have signed the CLA already but the status is still pending? Let us recheck it.

cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
db-will and others added 3 commits January 1, 2025 16:32
Co-authored-by: Daniël van Eeden <[email protected]>
Co-authored-by: Daniël van Eeden <[email protected]>
Co-authored-by: Daniël van Eeden <[email protected]>
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
cmd/go-tpc/main.go Show resolved Hide resolved
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
for i, addr := range targets {
hash.Write([]byte(addr))
switch driver {
case mysqlDriver:
var tlsName string = "preferred"
if len(sslCA) > 0 {
Copy link

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
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

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)

cmd/go-tpc/main.go Show resolved Hide resolved
db-will and others added 4 commits January 3, 2025 11:54
@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
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
Copy link

@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 <[email protected]>
cmd/go-tpc/main.go Outdated Show resolved Hide resolved
@Yui-Song
Copy link
Collaborator

Yui-Song commented Jan 8, 2025

@dveeden Please sign the CLA.

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