PMM-12880 Proper skip for MySQL TLS certs. (#2909)

* PMM-12880 Proper skip for TLS certs. * PMM-12880 Lint. * PMM-12880 Add comment. * PMM-12880 Another comment.
percona · May 29, 2024 · 1ea2627 · 1ea2627
1 parent e4bf62a
commit 1ea2627
Show file tree

Hide file tree

Showing 13 changed files with 28 additions and 23 deletions.
diff --git a/agent/agents/mysql/perfschema/perfschema.go b/agent/agents/mysql/perfschema/perfschema.go
@@ -129,7 +129,7 @@ func getPerfschemaHistorySize(q reform.Querier, l *logrus.Entry) uint {
 // New creates new PerfSchema QAN service.
 func New(params *Params, l *logrus.Entry) (*PerfSchema, error) {
 	if params.TextFiles != nil {
-		err := tlshelpers.RegisterMySQLCerts(params.TextFiles.Files)
+		err := tlshelpers.RegisterMySQLCerts(params.TextFiles.Files, params.TLSSkipVerify)
 		if err != nil {
 			return nil, err
 		}

diff --git a/agent/agents/mysql/slowlog/slowlog.go b/agent/agents/mysql/slowlog/slowlog.go
@@ -82,7 +82,7 @@ type slowLogInfo struct {
 // New creates new SlowLog QAN service.
 func New(params *Params, l *logrus.Entry) (*SlowLog, error) {
 	if params.TextFiles != nil {
-		err := tlshelpers.RegisterMySQLCerts(params.TextFiles.Files)
+		err := tlshelpers.RegisterMySQLCerts(params.TextFiles.Files, params.TLSSkipVerify)
 		if err != nil {
 			return nil, err
 		}

diff --git a/agent/connectionchecker/connection_checker.go b/agent/connectionchecker/connection_checker.go
@@ -96,12 +96,12 @@ func (cc *ConnectionChecker) sqlPing(ctx context.Context, db *sql.DB) error {
 	return err
 }
 
-func (cc *ConnectionChecker) checkMySQLConnection(ctx context.Context, dsn string, files *agentpb.TextFiles, tlsSkipVerify bool, id uint32) *agentpb.CheckConnectionResponse { //nolint:lll,unparam,revive
+func (cc *ConnectionChecker) checkMySQLConnection(ctx context.Context, dsn string, files *agentpb.TextFiles, tlsSkipVerify bool, id uint32) *agentpb.CheckConnectionResponse { //nolint:lll
 	var res agentpb.CheckConnectionResponse
 	var err error
 
 	if files != nil {
-		err = tlshelpers.RegisterMySQLCerts(files.Files)
+		err = tlshelpers.RegisterMySQLCerts(files.Files, tlsSkipVerify)
 		if err != nil {
 			cc.l.Debugf("checkMySQLConnection: failed to register cert: %s", err)
 			res.Error = err.Error()

diff --git a/agent/runner/actions/common.go b/agent/runner/actions/common.go
@@ -58,9 +58,9 @@ func jsonRows(columns []string, dataRows [][]interface{}) ([]byte, error) {
 }
 
 // mysqlOpen returns *sql.DB for given MySQL DSN.
-func mysqlOpen(dsn string, tlsFiles *agentpb.TextFiles) (*sql.DB, error) {
+func mysqlOpen(dsn string, tlsFiles *agentpb.TextFiles, tlsSkipVerify bool) (*sql.DB, error) {
 	if tlsFiles != nil {
-		err := tlshelpers.RegisterMySQLCerts(tlsFiles.Files)
+		err := tlshelpers.RegisterMySQLCerts(tlsFiles.Files, tlsSkipVerify)
 		if err != nil {
 			return nil, err
 		}

diff --git a/agent/runner/actions/mysql_explain_action.go b/agent/runner/actions/mysql_explain_action.go
@@ -104,7 +104,7 @@ func (a *mysqlExplainAction) Run(ctx context.Context) ([]byte, error) {
 	// query has a copy of the original params.Query field if the query is a SELECT or the equivalent
 	// SELECT after converting DML queries.
 	query, changedToSelect := dmlToSelect(a.params.Query)
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/runner/actions/mysql_query_select_action.go b/agent/runner/actions/mysql_query_select_action.go
@@ -62,7 +62,7 @@ func (a *mysqlQuerySelectAction) DSN() string {
 
 // Run runs an Action and returns output and error.
 func (a *mysqlQuerySelectAction) Run(ctx context.Context) ([]byte, error) {
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/runner/actions/mysql_query_show_action.go b/agent/runner/actions/mysql_query_show_action.go
@@ -62,7 +62,7 @@ func (a *mysqlQueryShowAction) DSN() string {
 
 // Run runs an Action and returns output and error.
 func (a *mysqlQueryShowAction) Run(ctx context.Context) ([]byte, error) {
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/runner/actions/mysql_show_create_table_action.go b/agent/runner/actions/mysql_show_create_table_action.go
@@ -60,7 +60,7 @@ func (a *mysqlShowCreateTableAction) DSN() string {
 
 // Run runs an Action and returns output and error.
 func (a *mysqlShowCreateTableAction) Run(ctx context.Context) ([]byte, error) {
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/runner/actions/mysql_show_index_action.go b/agent/runner/actions/mysql_show_index_action.go
@@ -61,7 +61,7 @@ func (a *mysqlShowIndexAction) DSN() string {
 
 // Run runs an Action and returns output and error.
 func (a *mysqlShowIndexAction) Run(ctx context.Context) ([]byte, error) {
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/runner/actions/mysql_show_table_status_action.go b/agent/runner/actions/mysql_show_table_status_action.go
@@ -65,7 +65,7 @@ func (a *mysqlShowTableStatusAction) DSN() string {
 
 // Run runs an Action and returns output and error.
 func (a *mysqlShowTableStatusAction) Run(ctx context.Context) ([]byte, error) {
-	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles)
+	db, err := mysqlOpen(a.params.Dsn, a.params.TlsFiles, a.params.TlsSkipVerify)
 	if err != nil {
 		return nil, err
 	}

diff --git a/agent/serviceinfobroker/service_info_broker.go b/agent/serviceinfobroker/service_info_broker.go
@@ -69,7 +69,7 @@ func (sib *ServiceInfoBroker) GetInfoFromService(ctx context.Context, msg *agent
 
 	switch msg.Type {
 	case inventorypb.ServiceType_MYSQL_SERVICE:
-		return sib.getMySQLInfo(ctx, msg.Dsn, msg.TextFiles, id)
+		return sib.getMySQLInfo(ctx, msg.Dsn, msg.TextFiles, msg.TlsSkipVerify, id)
 	case inventorypb.ServiceType_MONGODB_SERVICE:
 		return sib.getMongoDBInfo(ctx, msg.Dsn, msg.TextFiles, id)
 	case inventorypb.ServiceType_POSTGRESQL_SERVICE:
@@ -84,12 +84,12 @@ func (sib *ServiceInfoBroker) GetInfoFromService(ctx context.Context, msg *agent
 	}
 }
 
-func (sib *ServiceInfoBroker) getMySQLInfo(ctx context.Context, dsn string, files *agentpb.TextFiles, id uint32) *agentpb.ServiceInfoResponse {
+func (sib *ServiceInfoBroker) getMySQLInfo(ctx context.Context, dsn string, files *agentpb.TextFiles, tlsSkipVerify bool, id uint32) *agentpb.ServiceInfoResponse {
 	var res agentpb.ServiceInfoResponse
 	var err error
 
 	if files != nil {
-		err = tlshelpers.RegisterMySQLCerts(files.Files)
+		err = tlshelpers.RegisterMySQLCerts(files.Files, tlsSkipVerify)
 		if err != nil {
 			sib.l.Debugf("getMySQLInfo: failed to register cert: %s", err)
 			res.Error = err.Error()

diff --git a/agent/tlshelpers/mysql.go b/agent/tlshelpers/mysql.go
@@ -24,7 +24,7 @@ import (
 )
 
 // RegisterMySQLCerts is used for register TLS config before sql.Open is called.
-func RegisterMySQLCerts(files map[string]string) error {
+func RegisterMySQLCerts(files map[string]string, tlsSkipVerify bool) error {
 	if files == nil {
 		return nil
 	}
@@ -36,9 +36,10 @@ func RegisterMySQLCerts(files map[string]string) error {
 	}
 
 	if ok := ca.AppendCertsFromPEM([]byte(files["tlsCa"])); ok {
-		err = mysql.RegisterTLSConfig("custom", &tls.Config{ //nolint:gosec
-			RootCAs:      ca,
-			Certificates: []tls.Certificate{cert},
+		err = mysql.RegisterTLSConfig("custom", &tls.Config{
+			RootCAs:            ca,
+			Certificates:       []tls.Certificate{cert},
+			InsecureSkipVerify: tlsSkipVerify, // #nosec G402
 		})
 		if err != nil {
 			return errors.Wrap(err, "register MySQL CA cert failed")

diff --git a/managed/models/agent_model.go b/managed/models/agent_model.go
@@ -339,11 +339,13 @@ func (s *Agent) DSN(service *Service, dsnParams DSNParams, tdp *DelimiterPair) s
 		cfg.DBName = dsnParams.Database
 		cfg.Params = make(map[string]string)
 		if s.TLS {
+			// It is mandatory to have "custom" as the first case.
+			// Skip verify for "custom" is handled on pmm-agent side.
 			switch {
-			case s.TLSSkipVerify:
-				cfg.Params["tls"] = skipVerify
 			case len(s.Files()) != 0:
 				cfg.Params["tls"] = "custom"
+			case s.TLSSkipVerify:
+				cfg.Params["tls"] = skipVerify
 			default:
 				cfg.Params["tls"] = trueStr
 			}
@@ -368,11 +370,13 @@ func (s *Agent) DSN(service *Service, dsnParams DSNParams, tdp *DelimiterPair) s
 		cfg.DBName = dsnParams.Database
 		cfg.Params = make(map[string]string)
 		if s.TLS {
+			// It is mandatory to have "custom" as the first case.
+			// Skip verify for "custom" is handled on pmm-agent side.
 			switch {
-			case s.TLSSkipVerify:
-				cfg.Params["tls"] = "skip-verify"
 			case len(s.Files()) != 0:
 				cfg.Params["tls"] = "custom"
+			case s.TLSSkipVerify:
+				cfg.Params["tls"] = skipVerify
 			default:
 				cfg.Params["tls"] = trueStr
 			}