Remove InsecureSkipVerify from setupTLSCertificateOnly, use VerifyPeerCertificate in setupTLSCommonName

Co-authored-by: shueybubbles <[email protected]>

Author: Copilot

msdsn/conn_str_go115.go

@@ -13,51 +13,50 @@ func setupTLSCommonName(config *tls.Config, pem []byte) error {
 	// fix for https://github.com/denisenkom/go-mssqldb/issues/704
 	// A SSL/TLS certificate Common Name (CN) containing the ":" character
 	// (which is a non-standard character) will cause normal verification to fail.
-	// Since the VerifyConnection callback runs after normal certificate
-	// verification, confirm that SetupTLS() has been called
-	// with "insecureSkipVerify=false", then InsecureSkipVerify must be set to true
-	// for this VerifyConnection callback to accomplish certificate verification.
+	// We use VerifyPeerCertificate to perform custom verification.
+	// This is required because standard TLS verification in Go doesn't handle ":" in CN.
+	
+	// Create a certificate pool with the provided certificate as the root CA
+	roots := x509.NewCertPool()
+	roots.AppendCertsFromPEM(pem)
+	
+	// We must use InsecureSkipVerify=true for this specific edge case because
+	// normal verification will fail for certificates with ":" in the CN.
+	// The VerifyPeerCertificate callback performs proper certificate chain verification.
 	config.InsecureSkipVerify = true
-	config.VerifyConnection = func(cs tls.ConnectionState) error {
-		if len(cs.PeerCertificates) == 0 {
+	config.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+		if len(rawCerts) == 0 {
 			return fmt.Errorf("no peer certificates provided")
 		}
-		commonName := cs.PeerCertificates[0].Subject.CommonName
-		if commonName != cs.ServerName {
-			return fmt.Errorf("invalid certificate name %q, expected %q", commonName, cs.ServerName)
+		
+		// Parse the peer certificate
+		cert, err := x509.ParseCertificate(rawCerts[0])
+		if err != nil {
+			return fmt.Errorf("failed to parse certificate: %w", err)
 		}
-		// Create a certificate pool with the provided certificate as the root CA
-		roots := x509.NewCertPool()
-		roots.AppendCertsFromPEM(pem)
-
+		
+		// Check the common name matches the expected server name
+		commonName := cert.Subject.CommonName
+		if commonName != config.ServerName {
+			return fmt.Errorf("invalid certificate name %q, expected %q", commonName, config.ServerName)
+		}
+		
+		// Verify the certificate chain against the provided root CA
 		opts := x509.VerifyOptions{
 			Roots:         roots,
 			Intermediates: x509.NewCertPool(),
 		}
-		_, err := cs.PeerCertificates[0].Verify(opts)
+		_, err = cert.Verify(opts)
 		return err
 	}
 	return nil
 }
 
 // setupTLSCertificateOnly validates the certificate chain without checking the hostname
 func setupTLSCertificateOnly(config *tls.Config, pem []byte) error {
-	// Skip hostname validation but still verify the certificate chain
-	config.InsecureSkipVerify = true
-	config.VerifyConnection = func(cs tls.ConnectionState) error {
-		if len(cs.PeerCertificates) == 0 {
-			return fmt.Errorf("no peer certificates provided")
-		}
-		// Create a certificate pool with the provided certificate as the root CA
-		roots := x509.NewCertPool()
-		roots.AppendCertsFromPEM(pem)
-
-		opts := x509.VerifyOptions{
-			Roots:         roots,
-			Intermediates: x509.NewCertPool(),
-		}
-		_, err := cs.PeerCertificates[0].Verify(opts)
-		return err
-	}
+	// Skip hostname validation by setting ServerName to empty string
+	// The certificate chain will still be verified against RootCAs (set later in SetupTLS)
+	// This is the secure way to skip hostname validation without using InsecureSkipVerify
+	config.ServerName = ""
 	return nil
 }

msdsn/conn_str_go115pre.go

@@ -13,7 +13,8 @@ func setupTLSCommonName(config *tls.Config, pem []byte) error {
 
 // setupTLSCertificateOnly validates the certificate chain without checking the hostname
 func setupTLSCertificateOnly(config *tls.Config, pem []byte) error {
-	// Prior to Go 1.15, we can't use VerifyConnection callback
-	// So we rely on InsecureSkipVerify being set in SetupTLS
+	// Skip hostname validation by setting ServerName to empty string
+	// The certificate chain will still be verified against RootCAs (set later in SetupTLS)
+	config.ServerName = ""
 	return nil
 }

msdsn/conn_str_test.go

@@ -390,8 +390,8 @@ f0kGHKQEAFYGJLqJdK4KsGQDKLfZr9fqvXCCAA==
 	assert.Nil(t, err, "Expected no error parsing connection string")
 	assert.Equal(t, Encryption(EncryptionStrict), config.Encryption, "Expected EncryptionStrict")
 	assert.NotNil(t, config.TLSConfig, "Expected TLSConfig to be set")
-	assert.True(t, config.TLSConfig.InsecureSkipVerify, "Expected InsecureSkipVerify to be true when certificate is provided with strict encryption")
-	assert.NotNil(t, config.TLSConfig.VerifyConnection, "Expected VerifyConnection callback to be set")
+	assert.Equal(t, "", config.TLSConfig.ServerName, "Expected ServerName to be empty when certificate is provided with strict encryption")
+	assert.False(t, config.TLSConfig.InsecureSkipVerify, "Expected InsecureSkipVerify to be false")
 
 	// Test 2: encrypt=strict without certificate should NOT skip hostname validation
 	connStr2 := "server=somehost;encrypt=strict"