security: allow skip-ca without certificates to continue building tls config (#951)

Author: Samriddha9619

lib/util/security/cert.go

@@ -234,14 +234,7 @@ func (ci *CertInfo) buildClientConfig(lg *zap.Logger) (*tls.Config, error) {
 		lg.Warn("specified auto-certs in a client tls config, ignored")
 	}
 
-	if !cfg.HasCA() {
-		if cfg.SkipCA {
-			// still enable TLS without verify server certs
-			return &tls.Config{
-				InsecureSkipVerify: true,
-				MinVersion:         GetMinTLSVer(cfg.MinTLSVersion, lg),
-			}, nil
-		}
+	if !cfg.HasCA() && !cfg.SkipCA {
 		lg.Debug("no CA to verify server connections, disable TLS")
 		return nil, nil
 	}
@@ -251,30 +244,32 @@ func (ci *CertInfo) buildClientConfig(lg *zap.Logger) (*tls.Config, error) {
 		GetCertificate:       ci.getCert,
 		GetClientCertificate: ci.getClientCert,
 		InsecureSkipVerify:   true,
-		VerifyPeerCertificate: func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
-			return ci.verifyCA(rawCerts)
-		},
 	}
 
-	caPEM, err := os.ReadFile(cfg.CA)
-	if err != nil {
-		return nil, err
-	}
-	certPool := x509.NewCertPool()
-	if !certPool.AppendCertsFromPEM(caPEM) {
-		return nil, errors.New("failed to append ca certs")
+	if cfg.HasCA() {
+		tcfg.VerifyPeerCertificate = func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
+			return ci.verifyCA(rawCerts)
+		}
+		caPEM, err := os.ReadFile(cfg.CA)
+		if err != nil {
+			return nil, err
+		}
+		certPool := x509.NewCertPool()
+		if !certPool.AppendCertsFromPEM(caPEM) {
+			return nil, errors.New("failed to append ca certs")
+		}
+		ci.ca.Store(certPool)
+		tcfg.RootCAs = certPool
 	}
-	ci.ca.Store(certPool)
-	tcfg.RootCAs = certPool
 
-	if !cfg.HasCert() {
+	if cfg.Cert == "" || cfg.Key == "" {
 		lg.Debug("no certificates, server may reject the connection")
 		return tcfg, nil
 	}
 
 	cert, err := tls.LoadX509KeyPair(cfg.Cert, cfg.Key)
 	if err != nil {
-		return nil, errors.WithStack(err)
+		return nil, err
 	}
 	ci.cert.Store(&cert)
 

lib/util/security/cert_test.go

@@ -187,6 +187,7 @@ func TestCertServer(t *testing.T) {
 				require.Nil(t, c.RootCAs)
 				require.Nil(t, ci.cert.Load())
 				require.Equal(t, tls.VersionTLS12, int(c.MinVersion))
+				require.NotNil(t, c.GetClientCertificate, "skip-ca should set GetClientCertificate")
 			},
 		},
 		{
@@ -336,6 +337,7 @@ func TestSetConfig(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, tcfg)
 	require.True(t, tcfg.InsecureSkipVerify)
+	require.NotNil(t, tcfg.GetClientCertificate, "skip-ca should set GetClientCertificate")
 
 	cfg = config.TLSConfig{
 		SkipCA: false,

lib/util/security/tls.go

@@ -204,28 +204,24 @@ func CreateTLSConfigForTest() (serverTLSConf *tls.Config, clientTLSConf *tls.Con
 
 func BuildClientTLSConfig(logger *zap.Logger, cfg config.TLSConfig) (*tls.Config, error) {
 	logger = logger.With(zap.String("tls", "client"))
-	if !cfg.HasCA() {
-		if cfg.SkipCA {
-			// still enable TLS without verify server certs
-			return &tls.Config{
-				InsecureSkipVerify: true,
-				MinVersion:         tls.VersionTLS11,
-			}, nil
-		}
+	if !cfg.HasCA() && !cfg.SkipCA {
 		logger.Info("no CA to verify server connections, disable TLS")
 		return nil, nil
 	}
 
 	tcfg := &tls.Config{
 		MinVersion: tls.VersionTLS11,
+		InsecureSkipVerify: cfg.SkipCA,
 	}
-	tcfg.RootCAs = x509.NewCertPool()
-	certBytes, err := os.ReadFile(cfg.CA)
-	if err != nil {
-		return nil, errors.Errorf("failed to read CA: %w", err)
-	}
-	if !tcfg.RootCAs.AppendCertsFromPEM(certBytes) {
-		return nil, errors.Errorf("failed to append CA")
+	if cfg.HasCA() {
+		tcfg.RootCAs = x509.NewCertPool()
+		certBytes, err := os.ReadFile(cfg.CA)
+		if err != nil {
+			return nil, errors.Errorf("failed to read CA: %w", err)
+		}
+		if !tcfg.RootCAs.AppendCertsFromPEM(certBytes) {
+			return nil, errors.Errorf("failed to append CA")
+		}
 	}
 
 	if !cfg.HasCert() {