feat: add TLS config for MySQL driver#907
Draft
burningalchemist wants to merge 12 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
5cfc36f to
caa5acc
Compare
55aa10d to
b56ffc0
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 introduces support for custom TLS configuration for MySQL connections, allowing users to specify client and CA certificates via DSN parameters. It also adds a new
nomysqlbuild tag for builds without MySQL support, and updates the build system to support this new tag and its dependencies. The most important changes are grouped below:MySQL TLS Support:
sql.goto detect when the DSN requests a custom MySQL TLS configuration (tls=custom). The code registers a unique TLS config using provided DSN parameters (tls-ca,tls-cert,tls-key), strips these parameters from the DSN, and replaces thetlsparameter with the unique config name for the MySQL driver.tls_mysql.go, which implements thread-safe registration of custom MySQL TLS configurations usingsync.Once, and loads CA/client certificates as needed.tls_nomysql.gofor builds with thenomysqltag, disabling MySQL TLS support gracefully.Build System Enhancements:
Makefileto addyqas a build dependency and introduced abuild-nomysqltarget. This target builds the project with thenomysqltag by updating Promu's build flags usingyq, and ensures the correct build configuration for environments without MySQL support. [1] [2] [3]Dependency Management:
encoding/base64insql.goto generate unique config names for MySQL TLS configurations.