feat: add TLS config for MySQL driver#907
Draft
burningalchemist wants to merge 10 commits intomasterfrom
Draft
Conversation
b925d47 to
3a49a4e
Compare
3a49a4e to
75cab2b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom TLS configurations when connecting to MySQL databases, enabling secure connections with user-provided CA certificates, client certificates, and client keys. The implementation uses build tags to conditionally include or exclude MySQL TLS support.
Changes:
- Adds
tls_mysql.gowithregisterMySQLTLSConfigfunction to create and register custom TLS configurations based on DSN parameters (tls-ca,tls-cert,tls-key) - Updates
OpenConnectioninsql.goto invoke TLS registration whentls=customis specified and strips custom TLS parameters from the DSN before connecting - Adds
tls_nomysql.gowith a stub implementation that returns an error when built with theno_mysqlbuild tag
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tls_mysql.go | Implements MySQL TLS configuration registration with CA, client cert, and client key support |
| tls_nomysql.go | Provides stub implementation for builds without MySQL support |
| sql.go | Integrates TLS registration into connection flow and strips custom TLS parameters from DSN |
Comments suppressed due to low confidence (3)
tls_mysql.go:44
- The error message "failed to append PEM" is too vague and doesn't explain why the operation failed. This could be due to invalid PEM format, corrupted data, or other issues. Consider providing more context, such as "failed to append CA certificate from PEM: invalid PEM format or no valid certificates found".
return errors.New("failed to append PEM")
tls_mysql.go:64
- The TLS configuration does not set
InsecureSkipVerifyexplicitly tofalse, which is good. However, when no CA certificate is provided (caCert is empty), theRootCAsfield will benil, which causes Go's TLS to use the system's default CA pool. This behavior should be documented or explicitly validated to ensure users understand that whentls-cais omitted, system root CAs will be used for verification. Consider adding a comment explaining this behavior or validate that at least one TLS parameter is provided when tls=custom is used.
tlsConfig := &tls.Config{
RootCAs: rootCertPool,
Certificates: certs,
MinVersion: tls.VersionTLS12,
}
sql.go:48
- After stripping the custom TLS parameters (tls-ca, tls-cert, tls-key), the "tls" parameter with value "custom" remains in the DSN. This is correct and necessary because the MySQL driver needs to know which registered TLS configuration to use. However, this should be documented in a comment to clarify why the "tls" parameter is not stripped along with the other TLS parameters.
// Strip TLS parameters from the URL as they are interpreted as system variables by the MySQL driver which
// causes connection failure. The TLS configuration is already registered globally.
q := url.Query()
for _, param := range mysqlTLSParams {
q.Del(param)
}
url.URL.RawQuery = q.Encode()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
43e68a0 to
f4a7b1f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #906
This pull request adds support for custom TLS configurations when connecting to MySQL databases, allowing users to specify their own CA certificate, client certificate, and client key via connection parameters. It also introduces conditional compilation to handle builds without MySQL support.
MySQL TLS support enhancements:
registerMySQLTLSConfigfunction intls_mysql.goto register a custom TLS configuration for MySQL using DSN parameters (tls-ca,tls-cert,tls-key). This enables secure connections with custom certificates and keys.OpenConnectioninsql.goto callregisterMySQLTLSConfigwhen thetlsparameter is set tocustom, and to strip custom TLS parameters from the DSN before connecting to avoid driver errors.Build tag support:
tls_nomysql.gowith a stub implementation ofregisterMySQLTLSConfigthat returns an error if MySQL TLS support is disabled via thenomysqlbuild tag.