Skip to content

Comments

feat: add TLS config for MySQL driver#907

Draft
burningalchemist wants to merge 12 commits intomasterfrom
fix/mysql-tls
Draft

feat: add TLS config for MySQL driver#907
burningalchemist wants to merge 12 commits intomasterfrom
fix/mysql-tls

Conversation

@burningalchemist
Copy link
Owner

@burningalchemist burningalchemist commented Feb 20, 2026

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 nomysql build 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:

  • Added a new mechanism in sql.go to 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 the tls parameter with the unique config name for the MySQL driver.
  • Introduced tls_mysql.go, which implements thread-safe registration of custom MySQL TLS configurations using sync.Once, and loads CA/client certificates as needed.
  • Provided a stub implementation in tls_nomysql.go for builds with the nomysql tag, disabling MySQL TLS support gracefully.

Build System Enhancements:

  • Updated the Makefile to add yq as a build dependency and introduced a build-nomysql target. This target builds the project with the nomysql tag by updating Promu's build flags using yq, and ensures the correct build configuration for environments without MySQL support. [1] [2] [3]

Dependency Management:

  • Added imports for encoding/base64 in sql.go to generate unique config names for MySQL TLS configurations.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.go with registerMySQLTLSConfig function to create and register custom TLS configurations based on DSN parameters (tls-ca, tls-cert, tls-key)
  • Updates OpenConnection in sql.go to invoke TLS registration when tls=custom is specified and strips custom TLS parameters from the DSN before connecting
  • Adds tls_nomysql.go with a stub implementation that returns an error when built with the no_mysql build 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 InsecureSkipVerify explicitly to false, which is good. However, when no CA certificate is provided (caCert is empty), the RootCAs field will be nil, 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 when tls-ca is 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.

Copy link

@eenchev eenchev left a comment

Choose a reason for hiding this comment

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

Hi, small nitpick about comment

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.

Feature request: Support TLS file references for MySQL connection

2 participants