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() {

pkg/proxy/net/packetio.go

@@ -28,7 +28,6 @@ import (
 	"crypto/tls"
 	"io"
 	"net"
-	"sync"
 	"time"
 
 	"github.com/pingcap/tiproxy/lib/config"
@@ -43,19 +42,6 @@ var (
 	ErrInvalidSequence = errors.New("invalid sequence")
 )
 
-var (
-	readerPool = sync.Pool{
-		New: func() any {
-			return bufio.NewReaderSize(nil, DefaultConnBufferSize)
-		},
-	}
-	writerPool = sync.Pool{
-		New: func() any {
-			return bufio.NewWriterSize(nil, DefaultConnBufferSize)
-		},
-	}
-)
-
 const (
 	DefaultConnBufferSize = 32 * 1024
 )
@@ -100,27 +86,16 @@ type basicReadWriter struct {
 	inBytes  uint64
 	outBytes uint64
 	sequence uint8
-	pooled   bool
 }
 
 func newBasicReadWriter(conn net.Conn, bufferSize int) *basicReadWriter {
 	if bufferSize == 0 {
 		bufferSize = DefaultConnBufferSize
 	}
-	brw := &basicReadWriter{
-		Conn: conn,
-	}
-	if bufferSize == DefaultConnBufferSize {
-		r := readerPool.Get().(*bufio.Reader)
-		r.Reset(conn)
-		w := writerPool.Get().(*bufio.Writer)
-		w.Reset(conn)
-		brw.ReadWriter = bufio.NewReadWriter(r, w)
-		brw.pooled = true
-	} else {
-		brw.ReadWriter = bufio.NewReadWriter(bufio.NewReaderSize(conn, bufferSize), bufio.NewWriterSize(conn, bufferSize))
+	return &basicReadWriter{
+		Conn:       conn,
+		ReadWriter: bufio.NewReadWriter(bufio.NewReaderSize(conn, bufferSize), bufio.NewWriterSize(conn, bufferSize)),
 	}
-	return brw
 }
 
 func (brw *basicReadWriter) Read(b []byte) (n int, err error) {
@@ -178,16 +153,6 @@ func (brw *basicReadWriter) ResetSequence() {
 	brw.sequence = 0
 }
 
-func (brw *basicReadWriter) Free() {
-	if brw.pooled {
-		brw.pooled = false
-		brw.ReadWriter.Reader.Reset(nil)
-		brw.ReadWriter.Writer.Reset(nil)
-		readerPool.Put(brw.ReadWriter.Reader)
-		writerPool.Put(brw.ReadWriter.Writer)
-	}
-}
-
 func (brw *basicReadWriter) TLSConnectionState() tls.ConnectionState {
 	return tls.ConnectionState{}
 }
@@ -523,19 +488,6 @@ func (p *packetIO) GracefulClose() error {
 	return nil
 }
 
-func freeBasicReadWriter(rw packetReadWriter) {
-	switch v := rw.(type) {
-	case *basicReadWriter:
-		v.Free()
-	case *tlsReadWriter:
-		freeBasicReadWriter(v.packetReadWriter)
-	case *compressedReadWriter:
-		freeBasicReadWriter(v.packetReadWriter)
-	case *proxyReadWriter:
-		freeBasicReadWriter(v.packetReadWriter)
-	}
-}
-
 func (p *packetIO) Close() error {
 	var errs []error
 	/*
@@ -544,7 +496,6 @@ func (p *packetIO) Close() error {
 			errs = append(errs, err)
 		}
 	*/
-	freeBasicReadWriter(p.readWriter)
 	if err := p.readWriter.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
 		errs = append(errs, errors.WithStack(err))
 	}

pkg/proxy/net/packetio_test.go

@@ -719,55 +719,3 @@ func runForwardBenchmark(b *testing.B, f func(packetIO1, packetIO2 *packetIO)) {
 	_ = packetIO2.Close()
 	wg.Wait()
 }
-
-func TestPacketIOPooling(t *testing.T) {
-	testTCPConn(t,
-		func(t *testing.T, cli *packetIO) {
-			brw, ok := cli.readWriter.(*basicReadWriter)
-			require.True(t, ok)
-			require.True(t, brw.pooled, "pooled flag should be true for default buffer size")
-
-			require.NoError(t, cli.WritePacket([]byte("pooltest"), true))
-		},
-		func(t *testing.T, srv *packetIO) {
-			brw, ok := srv.readWriter.(*basicReadWriter)
-			require.True(t, ok)
-			require.True(t, brw.pooled, "pooled flag should be true for default buffer size")
-
-			data, err := srv.ReadPacket()
-			require.NoError(t, err)
-			require.Equal(t, []byte("pooltest"), data)
-		},
-		1,
-	)
-
-	lg, _ := logger.CreateLoggerForTest(t)
-	cli, srv := net.Pipe()
-	cliIO := NewPacketIO(cli, lg, DefaultConnBufferSize*2) // non-default
-	srvIO := NewPacketIO(srv, lg, DefaultConnBufferSize*2)
-	brw, ok := cliIO.readWriter.(*basicReadWriter)
-	require.True(t, ok)
-	require.False(t, brw.pooled, "pooled flag should be false for non-default buffer size")
-	_ = cliIO.Close()
-	_ = srvIO.Close()
-
-	testTCPConn(t,
-		func(t *testing.T, cli *packetIO) {
-			require.NoError(t, cli.Close())
-			require.NoError(t, cli.Close())
-		},
-		func(t *testing.T, srv *packetIO) {
-			require.NoError(t, srv.Close())
-			require.NoError(t, srv.Close())
-		},
-		1,
-	)
-
-	for i := 0; i < 100; i++ {
-		c1, c2 := net.Pipe()
-		p1 := NewPacketIO(c1, lg, DefaultConnBufferSize)
-		p2 := NewPacketIO(c2, lg, DefaultConnBufferSize)
-		_ = p1.Close()
-		_ = p2.Close()
-	}
-}