Enabling support for TLS-verified mysql server#84
Conversation
| if db, err := sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s?parseTime=true", | ||
| mySQLCfg.Username, mySQLCfg.Password, mySQLCfg.Server, mySQLCfg.Database)); err != nil { | ||
| if db, err := sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s?tls=%t&parseTime=true", | ||
| mySQLCfg.Username, mySQLCfg.Password, mySQLCfg.Server, mySQLCfg.Database, mySQLCfg.Tls)); err != nil { |
There was a problem hiding this comment.
"Note: the corresponding CA certificate should be added to the system's trust store for TLS verification to work." Agree. It has to add the CA in the system truststore. It's possible to add a complete solution so that customized CA can be specified as well(a ca path maybe)？
There was a problem hiding this comment.
Yes, thanks for the comment Yabin. I have added logic to handle custom certificate path. For completeness, I also added support for mutual TLS.
ethanyzhang
left a comment
ethanyzhang
left a comment
There was a problem hiding this comment.
Thanks for the patch. There should also be tests for this as well.
| } | ||
| if ok := rootCertPool.AppendCertsFromPEM(pem); !ok { | ||
| log.Error().Msg("failed to append CA certificate") | ||
| return nil, err |
There was a problem hiding this comment.
err here will be nil.
There was a problem hiding this comment.
This is the path for !ok. The intent is to return an error if appending the certificate fails.
|
Please update the PR, commit messages should follow semantic commit message convention. |
ethanyzhang
requested review from
FelixYBW,
wanglinsong,
xpengahana and
yhwang
as code owners
|
We can simplify the PR by removing mTLS; pbench is not a typical mTLS use case. |
g-saransh
force-pushed
the
mysql_tls
branch
2 times, most recently
from
c4e5122 to
30dc9db
Compare
|
@ethanyzhang I have updated the PR, addressing your comments. I also rebased and updated the commit message to follow semantic commit message convention. |
I added mTLS for completeness. But if we feel it's not relevant here, I can remove it. |
Yes, please. The hosts that run the pbench and pbench itself are not provisioned/built with client certificates. |
- Update mysql.template.json with new TLS options - Handle custom TLS configuration - Add tests
Got it. Removed mutual TLS code. Thanks @wanglinsong. |
4 participants
This PR adds support for simple TLS-verified mysql server. By default, it maintains the original behavior of disabling TLS verification.
Add the following fields in mysql.json for TLS,