Resolve coderabbit comments

Author: pourtorabehsan

README.md

@@ -434,7 +434,7 @@ Valid Values:   true, false, skip-verify, preferred, <name>
 Default:        false
 ```
 
-`tls=true` enables TLS / SSL encrypted connection to the server with full certificate verification (including hostname). Use `skip-verify` if you want to use a self-signed or invalid certificate (server side) or use `preferred` to use TLS only when advertised by the server. This is similar to `skip-verify`, but additionally allows a fallback to a connection which is not encrypted. Neither `skip-verify` nor `preferred` add any reliable security. You can use a custom TLS config after registering it with [`mysql.RegisterTLSConfig`](https://godoc.org/github.com/go-sql-driver/mysql#RegisterTLSConfig).
+`tls=true` enables TLS / SSL encrypted connection to the server with full certificate verification (including hostname). Use `skip-verify` if you want to use a self-signed or invalid certificate (server-side) or use `preferred` to use TLS only when advertised by the server. This is similar to `skip-verify`, but additionally allows a fallback to a connection which is not encrypted. Neither `skip-verify` nor `preferred` add any reliable security. You can use a custom TLS config after registering it with [`mysql.RegisterTLSConfig`](https://godoc.org/github.com/go-sql-driver/mysql#RegisterTLSConfig).
 
 **TLS Verification Modes:**
 
@@ -449,10 +449,12 @@ The `tls-verify` parameter controls how certificates are verified (works with bo
 - `tls-verify=ca`: Verifies CA only, skips hostname check - Equivalent to MySQL's VERIFY_CA mode
 
 **Examples:**
-- `?tls=true` - System CA with full verification (default behavior)
-- `?tls=true&tls-verify=ca` - System CA with CA-only verification
-- `?tls=custom` - Custom CA with full verification (default behavior)
-- `?tls=custom&tls-verify=ca` - Custom CA with CA-only verification
+```text
+?tls=true - System CA with full verification (default behavior)
+?tls=true&tls-verify=ca - System CA with CA-only verification
+?tls=custom - Custom CA with full verification (default behavior)
+?tls=custom&tls-verify=ca - Custom CA with CA-only verification
+```
 
 ##### `tls-verify`
 
@@ -462,7 +464,7 @@ Valid Values:   identity, ca
 Default:        identity
 ```
 
-Controls the TLS certificate verification level. This parameter works in conjunction with the `tls` parameter:
+Controls the TLS certificate verification level. This parameter works with the `tls` parameter:
 - `identity`: Full verification including hostname (default, most secure)
 - `ca`: CA verification only, without hostname checking (MySQL VERIFY_CA equivalent)
 

driver_test.go

@@ -1520,6 +1520,16 @@ func TestTLS(t *testing.T) {
 		InsecureSkipVerify: true,
 	})
 	runTests(t, dsn+"&tls=custom-skip-verify", tlsTestReq)
+
+	// Test tls-verify parameter with system CA
+	runTests(t, dsn+"&tls=true&tls-verify=ca", tlsTestReq)
+	runTests(t, dsn+"&tls=true&tls-verify=identity", tlsTestReq)
+
+	// Test tls-verify parameter with custom TLS config
+	RegisterTLSConfig("custom-ca-verify", &tls.Config{
+		InsecureSkipVerify: true,
+	})
+	runTests(t, dsn+"&tls=custom-ca-verify&tls-verify=ca", tlsTestReq)
 }
 
 func TestReuseClosedConnection(t *testing.T) {

dsn.go

@@ -207,7 +207,7 @@ func (cfg *Config) normalize() error {
 		case "true":
 			// System CA pool
 			if cfg.TLSVerify == "ca" {
-				cfg.TLS = createVerifyCAConfig(nil)
+				cfg.TLS = createVerifyCAConfig(nil, nil)
 			} else {
 				cfg.TLS = &tls.Config{}
 			}
@@ -225,9 +225,9 @@ func (cfg *Config) normalize() error {
 
 			// Apply tls-verify to custom config
 			if cfg.TLSVerify == "ca" {
-				// Extract RootCAs from the custom config and create a CA-only verification config
+				// Preserve all settings from custom config, only modify verification behavior
 				rootCAs := cfg.TLS.RootCAs
-				cfg.TLS = createVerifyCAConfig(rootCAs)
+				cfg.TLS = createVerifyCAConfig(cfg.TLS, rootCAs)
 			}
 		}
 	}

dsn_test.go

@@ -10,6 +10,7 @@ package mysql
 
 import (
 	"crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"net/url"
 	"reflect"
@@ -80,6 +81,12 @@ var testDSNs = []struct {
 }, {
 	"foo:bar@tcp(192.168.1.50:3307)/baz?timeout=10s&connectionAttributes=program_name:MySQLGoDriver%2FTest,program_version:1.2.3",
 	&Config{User: "foo", Passwd: "bar", Net: "tcp", Addr: "192.168.1.50:3307", DBName: "baz", Loc: time.UTC, Timeout: 10 * time.Second, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, ConnectionAttributes: "program_name:MySQLGoDriver/Test,program_version:1.2.3"},
+}, {
+	"user:password@tcp(localhost:5555)/dbname?tls=true&tls-verify=ca",
+	&Config{User: "user", Passwd: "password", Net: "tcp", Addr: "localhost:5555", DBName: "dbname", Loc: time.UTC, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, TLSConfig: "true", TLSVerify: "ca"},
+}, {
+	"user:password@tcp(localhost:5555)/dbname?tls=true&tls-verify=identity",
+	&Config{User: "user", Passwd: "password", Net: "tcp", Addr: "localhost:5555", DBName: "dbname", Loc: time.UTC, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, TLSConfig: "true", TLSVerify: "identity"},
 },
 }
 
@@ -456,15 +463,19 @@ func TestTLSVerifySystemCA(t *testing.T) {
 				if cfg.TLS.VerifyPeerCertificate == nil {
 					t.Error("ca mode should have VerifyPeerCertificate callback set")
 				}
-				// ca should not set ServerName (hostname verification is skipped)
+				// ca mode does not auto-set ServerName (hostname verification is skipped)
+				// ServerName remains empty unless explicitly set
 				if cfg.TLS.ServerName != "" {
-					t.Errorf("ca mode should not set ServerName, got %q", cfg.TLS.ServerName)
+					t.Errorf("ca mode with system CA should not have ServerName set, got %q", cfg.TLS.ServerName)
 				}
 			} else {
 				// identity (default) should set ServerName
 				if cfg.TLS.ServerName != "example.com" {
 					t.Errorf("identity mode should set ServerName to 'example.com', got %q", cfg.TLS.ServerName)
 				}
+				if cfg.TLS.VerifyPeerCertificate != nil {
+					t.Error("identity mode should not have VerifyPeerCertificate callback set")
+				}
 			}
 		})
 	}
@@ -473,6 +484,7 @@ func TestTLSVerifySystemCA(t *testing.T) {
 func TestTLSVerifyCustomConfig(t *testing.T) {
 	// Register a custom TLS config
 	customConfig := &tls.Config{
+		MinVersion: tls.VersionTLS12,
 		ServerName: "customServer",
 		RootCAs:    nil, // Use system CA pool for this test
 	}
@@ -505,15 +517,18 @@ func TestTLSVerifyCustomConfig(t *testing.T) {
 				if cfg.TLS.VerifyPeerCertificate == nil {
 					t.Error("ca mode should have VerifyPeerCertificate callback set")
 				}
-				// ca should not set ServerName (hostname verification is skipped)
-				if cfg.TLS.ServerName != "" {
-					t.Errorf("ca mode should not set ServerName, got %q", cfg.TLS.ServerName)
+				// ca mode should preserve custom config's ServerName for SNI
+				if cfg.TLS.ServerName != "customServer" {
+					t.Errorf("ca mode should preserve custom ServerName 'customServer', got %q", cfg.TLS.ServerName)
 				}
 			} else {
 				// identity (default) should preserve custom config's ServerName
 				if cfg.TLS.ServerName != "customServer" {
 					t.Errorf("identity mode should preserve custom ServerName 'customServer', got %q", cfg.TLS.ServerName)
 				}
+				if cfg.TLS.VerifyPeerCertificate != nil {
+					t.Error("identity mode should not have VerifyPeerCertificate callback set")
+				}
 			}
 		})
 	}
@@ -569,10 +584,81 @@ func TestTLSVerifyInvalidValue(t *testing.T) {
 	if err == nil {
 		t.Error("expected error for invalid tls-verify value")
 	}
+	expectedMsg := "invalid value for tls-verify"
+	if err != nil && !containsString(err.Error(), expectedMsg) {
+		t.Errorf("error message should contain %q, got: %v", expectedMsg, err)
+	}
+}
+
+// containsString checks if s contains substr (simple implementation to avoid importing strings)
+func containsString(s, substr string) bool {
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}
+
+func TestTLSVerifyPreservesCustomConfig(t *testing.T) {
+	// Register a custom TLS config with various settings
+	customConfig := &tls.Config{
+		MinVersion:   tls.VersionTLS12,
+		MaxVersion:   tls.VersionTLS13,
+		ServerName:   "customServer",
+		CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+		NextProtos:   []string{"h2", "http/1.1"},
+		RootCAs:      x509.NewCertPool(),
+	}
+	RegisterTLSConfig("custom-full", customConfig)
+	defer DeregisterTLSConfig("custom-full")
+
+	dsn := "tcp(example.com:1234)/?tls=custom-full&tls-verify=ca"
+	cfg, err := ParseDSN(dsn)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if cfg.TLS == nil {
+		t.Fatal("cfg.TLS should not be nil")
+	}
+
+	// Verify VERIFY_CA mode is enabled
+	if !cfg.TLS.InsecureSkipVerify {
+		t.Error("ca mode should have InsecureSkipVerify=true")
+	}
+	if cfg.TLS.VerifyPeerCertificate == nil {
+		t.Error("ca mode should have VerifyPeerCertificate callback set")
+	}
+
+	// Verify all custom settings are preserved
+	if cfg.TLS.MinVersion != tls.VersionTLS12 {
+		t.Errorf("MinVersion not preserved: got %v, want %v", cfg.TLS.MinVersion, tls.VersionTLS12)
+	}
+	if cfg.TLS.MaxVersion != tls.VersionTLS13 {
+		t.Errorf("MaxVersion not preserved: got %v, want %v", cfg.TLS.MaxVersion, tls.VersionTLS13)
+	}
+	if cfg.TLS.ServerName != "customServer" {
+		t.Errorf("ServerName not preserved: got %q, want 'customServer'", cfg.TLS.ServerName)
+	}
+	if len(cfg.TLS.CipherSuites) != 1 || cfg.TLS.CipherSuites[0] != tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 {
+		t.Error("CipherSuites not preserved")
+	}
+	if len(cfg.TLS.NextProtos) != 2 || cfg.TLS.NextProtos[0] != "h2" || cfg.TLS.NextProtos[1] != "http/1.1" {
+		t.Error("NextProtos not preserved")
+	}
+	if cfg.TLS.RootCAs == nil {
+		t.Error("RootCAs not preserved")
+	}
 }
 
 func TestRegisterTLSConfigReservedKey(t *testing.T) {
-	reservedKeys := []string{"true", "false", "skip-verify", "preferred"}
+	reservedKeys := []string{
+		"true", "True", "TRUE",
+		"false", "False", "FALSE",
+		"skip-verify", "Skip-Verify", "SKIP-VERIFY",
+		"preferred", "Preferred", "PREFERRED",
+	}
 
 	for _, key := range reservedKeys {
 		err := RegisterTLSConfig(key, &tls.Config{})

utils.go

@@ -88,21 +88,30 @@ func getTLSConfigClone(key string) (config *tls.Config) {
 	return
 }
 
-// createVerifyCAConfig creates a TLS config that verifies the CA certificate
-// but does not verify the hostname. This implements MySQL's VERIFY_CA mode.
+// createVerifyCAConfig creates or modifies a TLS config to verify the CA certificate
+// but not the hostname. This implements MySQL's VERIFY_CA mode.
 // It uses the recommended Go pattern from issues #21971, #31791, #31792, #35467:
 // 1. Set InsecureSkipVerify to disable default verification
 // 2. Use VerifyPeerCertificate callback to manually verify CA without hostname
 //
-// If rootCAs is nil, the system's default CA pool will be used.
-// If rootCAs is provided, it will be used for verification instead.
-func createVerifyCAConfig(rootCAs *x509.CertPool) *tls.Config {
-	return &tls.Config{
-		InsecureSkipVerify: true,
-		VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
-			return verifyCACallback(rawCerts, verifiedChains, rootCAs)
-		},
+// If baseConfig is nil, creates a new minimal config.
+// If baseConfig is provided, clones it and preserves all settings except verification.
+// The rootCAs parameter specifies which CA pool to use for verification.
+func createVerifyCAConfig(baseConfig *tls.Config, rootCAs *x509.CertPool) *tls.Config {
+	var cfg *tls.Config
+	if baseConfig != nil {
+		cfg = baseConfig.Clone()
+	} else {
+		cfg = &tls.Config{}
 	}
+
+	// Override only the verification-related fields for VERIFY_CA mode
+	cfg.InsecureSkipVerify = true
+	cfg.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+		return verifyCACallback(rawCerts, verifiedChains, rootCAs)
+	}
+
+	return cfg
 }
 
 // verifyCACallback implements CA-only verification without hostname checking.
@@ -112,7 +121,7 @@ func createVerifyCAConfig(rootCAs *x509.CertPool) *tls.Config {
 //
 // If rootCAs is nil, the system's default CA pool will be used.
 // If rootCAs is provided, it will be used for verification instead.
-func verifyCACallback(rawCerts [][]byte, verifiedChains [][]*x509.Certificate, rootCAs *x509.CertPool) error {
+func verifyCACallback(rawCerts [][]byte, _ [][]*x509.Certificate, rootCAs *x509.CertPool) error {
 	if len(rawCerts) == 0 {
 		return errors.New("tls: no certificates from server")
 	}
@@ -139,6 +148,7 @@ func verifyCACallback(rawCerts [][]byte, verifiedChains [][]*x509.Certificate, r
 	opts := x509.VerifyOptions{
 		Roots:         rootCAs, // nil = use system CA pool
 		Intermediates: intermediates,
+		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		// DNSName is intentionally not set to skip hostname verification
 	}