[FAB-18339] CLI: rename GetCerificate to GetClientCertificate (#2146)

GetCertificate() should be renamed to GetClientCertificate() as it only returns
the client certificate. Currently GetCertificate() gets a PeerClient that unnecessarily
loads server tls root file based on env vars. Update the function to only load client certificate.

Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>

internal/peer/chaincode/common.go

@@ -469,7 +469,7 @@ func InitCmdFactory(cmdName string, isEndorserRequired, isOrdererRequired bool,
 			return nil, errors.New("no endorser clients retrieved - this might indicate a bug")
 		}
 	}
-	certificate, err := common.GetCertificateFnc()
+	certificate, err := common.GetClientCertificateFnc()
 	if err != nil {
 		return nil, errors.WithMessage(err, "error getting client certificate")
 	}

internal/peer/common/common.go

@@ -73,8 +73,8 @@ var (
 	GetOrdererEndpointOfChainFnc func(chainID string, signer Signer,
 		endorserClient pb.EndorserClient, cryptoProvider bccsp.BCCSP) ([]string, error)
 
-	// GetCertificateFnc is a function that returns the client TLS certificate
-	GetCertificateFnc func() (tls.Certificate, error)
+	// GetClientCertificateFnc is a function that returns the client TLS certificate
+	GetClientCertificateFnc func() (tls.Certificate, error)
 )
 
 type CommonClient struct {
@@ -90,7 +90,7 @@ func init() {
 	GetOrdererEndpointOfChainFnc = GetOrdererEndpointOfChain
 	GetDeliverClientFnc = GetDeliverClient
 	GetPeerDeliverClientFnc = GetPeerDeliverClient
-	GetCertificateFnc = GetCertificate
+	GetClientCertificateFnc = GetClientCertificate
 }
 
 // InitConfig initializes viper config
@@ -255,25 +255,29 @@ func configFromEnv(prefix string) (address, override string, clientConfig comm.C
 		secOpts.ServerRootCAs = [][]byte{caPEM}
 	}
 	if secOpts.RequireClientCert {
-		keyPEM, res := ioutil.ReadFile(config.GetPath(prefix + ".tls.clientKey.file"))
-		if res != nil {
-			err = errors.WithMessage(res,
-				fmt.Sprintf("unable to load %s.tls.clientKey.file", prefix))
+		secOpts.Key, secOpts.Certificate, err = getClientAuthInfoFromEnv(prefix)
+		if err != nil {
 			return
 		}
-		secOpts.Key = keyPEM
-		certPEM, res := ioutil.ReadFile(config.GetPath(prefix + ".tls.clientCert.file"))
-		if res != nil {
-			err = errors.WithMessage(res,
-				fmt.Sprintf("unable to load %s.tls.clientCert.file", prefix))
-			return
-		}
-		secOpts.Certificate = certPEM
 	}
 	clientConfig.SecOpts = secOpts
 	return
 }
 
+// getClientAuthInfoFromEnv reads client tls key file and cert file and returns the bytes for the files
+func getClientAuthInfoFromEnv(prefix string) ([]byte, []byte, error) {
+	keyPEM, err := ioutil.ReadFile(config.GetPath(prefix + ".tls.clientKey.file"))
+	if err != nil {
+		return nil, nil, errors.WithMessagef(err, "unable to load %s.tls.clientKey.file", prefix)
+	}
+	certPEM, err := ioutil.ReadFile(config.GetPath(prefix + ".tls.clientCert.file"))
+	if err != nil {
+		return nil, nil, errors.WithMessagef(err, "unable to load %s.tls.clientCert.file", prefix)
+	}
+
+	return keyPEM, certPEM, nil
+}
+
 func InitCmd(cmd *cobra.Command, args []string) {
 	err := InitConfig(CmdRoot)
 	if err != nil { // Handle errors reading the config file

internal/peer/common/peerclient.go

@@ -13,7 +13,6 @@ import (
 	"time"
 
 	pb "github.com/hyperledger/fabric-protos-go/peer"
-	"github.com/hyperledger/fabric/core/config"
 	"github.com/hyperledger/fabric/internal/pkg/comm"
 	"github.com/pkg/errors"
 	"github.com/spf13/viper"
@@ -54,16 +53,12 @@ func NewPeerClientForAddress(address, tlsRootCertFile string) (*PeerClient, erro
 	}
 
 	if secOpts.RequireClientCert {
-		keyPEM, err := ioutil.ReadFile(config.GetPath("peer.tls.clientKey.file"))
+		var err error
+		secOpts.Key, secOpts.Certificate, err = getClientAuthInfoFromEnv("peer")
 		if err != nil {
-			return nil, errors.WithMessage(err, "unable to load peer.tls.clientKey.file")
+			return nil, err
 		}
-		secOpts.Key = keyPEM
-		certPEM, err := ioutil.ReadFile(config.GetPath("peer.tls.clientCert.file"))
-		if err != nil {
-			return nil, errors.WithMessage(err, "unable to load peer.tls.clientCert.file")
-		}
-		secOpts.Certificate = certPEM
+
 	}
 	clientConfig.SecOpts = secOpts
 
@@ -133,40 +128,37 @@ func (pc *PeerClient) Certificate() tls.Certificate {
 // from the configuration settings for "peer.address" and
 // "peer.tls.rootcert.file"
 func GetEndorserClient(address, tlsRootCertFile string) (pb.EndorserClient, error) {
-	var peerClient *PeerClient
-	var err error
-	if address != "" {
-		peerClient, err = NewPeerClientForAddress(address, tlsRootCertFile)
-	} else {
-		peerClient, err = NewPeerClientFromEnv()
-	}
+	peerClient, err := newPeerClient(address, tlsRootCertFile)
 	if err != nil {
 		return nil, err
 	}
 	return peerClient.Endorser()
 }
 
-// GetCertificate returns the client's TLS certificate
-func GetCertificate() (tls.Certificate, error) {
-	peerClient, err := NewPeerClientFromEnv()
+// GetClientCertificate returns the client's TLS certificate
+func GetClientCertificate() (tls.Certificate, error) {
+	if !viper.GetBool("peer.tls.clientAuthRequired") {
+		return tls.Certificate{}, nil
+	}
+
+	key, certificate, err := getClientAuthInfoFromEnv("peer")
 	if err != nil {
 		return tls.Certificate{}, err
 	}
-	return peerClient.Certificate(), nil
+
+	cert, err := tls.X509KeyPair(certificate, key)
+	if err != nil {
+		return tls.Certificate{}, errors.WithMessage(err, "failed to load client certificate")
+	}
+	return cert, nil
 }
 
 // GetDeliverClient returns a new deliver client. If both the address and
 // tlsRootCertFile are not provided, the target values for the client are taken
 // from the configuration settings for "peer.address" and
 // "peer.tls.rootcert.file"
 func GetDeliverClient(address, tlsRootCertFile string) (pb.Deliver_DeliverClient, error) {
-	var peerClient *PeerClient
-	var err error
-	if address != "" {
-		peerClient, err = NewPeerClientForAddress(address, tlsRootCertFile)
-	} else {
-		peerClient, err = NewPeerClientFromEnv()
-	}
+	peerClient, err := newPeerClient(address, tlsRootCertFile)
 	if err != nil {
 		return nil, err
 	}
@@ -178,13 +170,7 @@ func GetDeliverClient(address, tlsRootCertFile string) (pb.Deliver_DeliverClient
 // from the configuration settings for "peer.address" and
 // "peer.tls.rootcert.file"
 func GetPeerDeliverClient(address, tlsRootCertFile string) (pb.DeliverClient, error) {
-	var peerClient *PeerClient
-	var err error
-	if address != "" {
-		peerClient, err = NewPeerClientForAddress(address, tlsRootCertFile)
-	} else {
-		peerClient, err = NewPeerClientFromEnv()
-	}
+	peerClient, err := newPeerClient(address, tlsRootCertFile)
 	if err != nil {
 		return nil, err
 	}
@@ -205,15 +191,16 @@ func (pc *PeerClient) SnapshotClient() (pb.SnapshotClient, error) {
 // from the configuration settings for "peer.address" and
 // "peer.tls.rootcert.file"
 func GetSnapshotClient(address, tlsRootCertFile string) (pb.SnapshotClient, error) {
-	var peerClient *PeerClient
-	var err error
-	if address != "" {
-		peerClient, err = NewPeerClientForAddress(address, tlsRootCertFile)
-	} else {
-		peerClient, err = NewPeerClientFromEnv()
-	}
+	peerClient, err := newPeerClient(address, tlsRootCertFile)
 	if err != nil {
 		return nil, err
 	}
 	return peerClient.SnapshotClient()
 }
+
+func newPeerClient(address, tlsRootCertFile string) (*PeerClient, error) {
+	if address != "" {
+		return NewPeerClientForAddress(address, tlsRootCertFile)
+	}
+	return NewPeerClientFromEnv()
+}

internal/peer/common/peerclient_test.go

@@ -161,13 +161,72 @@ func TestPeerClientTimeout(t *testing.T) {
 		cert := pClient.Certificate()
 		require.NotNil(t, cert)
 	})
-	t.Run("GetCertificate()", func(t *testing.T) {
+}
+
+func TestGetClientCertificate(t *testing.T) {
+	t.Run("GetClientCertificate_clientAuthRequired_disabled", func(t *testing.T) {
 		cleanup := initPeerTestEnv(t)
 		defer cleanup()
-		cert, err := common.GetCertificate()
+		cert, err := common.GetClientCertificate()
 		require.NotEqual(t, cert, &tls.Certificate{})
 		require.NoError(t, err)
 	})
+
+	t.Run("GetClientCertificate_clientAuthRequired_enabled", func(t *testing.T) {
+		viper.Set("peer.tls.clientAuthRequired", true)
+		viper.Set("peer.tls.clientKey.file", filepath.Join("./testdata/certs", "client.key"))
+		viper.Set("peer.tls.clientCert.file", filepath.Join("./testdata/certs", "client.crt"))
+		defer viper.Reset()
+
+		cert, err := common.GetClientCertificate()
+		require.NoError(t, err)
+		require.NotEqual(t, cert, tls.Certificate{})
+	})
+
+	t.Run("GetClientCertificate_empty_keyfile", func(t *testing.T) {
+		viper.Set("peer.tls.clientAuthRequired", true)
+		viper.Set("peer.tls.clientKey.file", filepath.Join("./testdata/certs", "empty.key"))
+		viper.Set("peer.tls.clientCert.file", filepath.Join("./testdata/certs", "client.crt"))
+		defer viper.Reset()
+
+		cert, err := common.GetClientCertificate()
+		require.EqualError(t, err, "failed to load client certificate: tls: failed to find any PEM data in key input")
+		require.Equal(t, cert, tls.Certificate{})
+	})
+
+	t.Run("GetClientCertificate_empty_certfile", func(t *testing.T) {
+		viper.Set("peer.tls.clientAuthRequired", true)
+		viper.Set("peer.tls.clientKey.file", filepath.Join("./testdata/certs", "client.key"))
+		viper.Set("peer.tls.clientCert.file", filepath.Join("./testdata/certs", "empty.crt"))
+		defer viper.Reset()
+
+		cert, err := common.GetClientCertificate()
+		require.EqualError(t, err, "failed to load client certificate: tls: failed to find any PEM data in certificate input")
+		require.Equal(t, cert, tls.Certificate{})
+	})
+
+	t.Run("GetClientCertificate_bad_keyfilepath", func(t *testing.T) {
+		// bad key file path
+		viper.Set("peer.tls.clientAuthRequired", true)
+		viper.Set("peer.tls.clientKey.file", filepath.Join("./testdata/certs", "nokey.key"))
+		viper.Set("peer.tls.clientCert.file", filepath.Join("./testdata/certs", "client.crt"))
+		defer viper.Reset()
+
+		cert, err := common.GetClientCertificate()
+		require.EqualError(t, err, "unable to load peer.tls.clientKey.file: open testdata/certs/nokey.key: no such file or directory")
+		require.Equal(t, cert, tls.Certificate{})
+	})
+
+	t.Run("GetClientCertificate_missing_certfilepath", func(t *testing.T) {
+		// missing cert file path
+		viper.Set("peer.tls.clientAuthRequired", true)
+		viper.Set("peer.tls.clientKey.file", filepath.Join("./testdata/certs", "client.key"))
+		defer viper.Reset()
+
+		cert, err := common.GetClientCertificate()
+		require.EqualError(t, err, "unable to load peer.tls.clientCert.file: open : no such file or directory")
+		require.Equal(t, cert, tls.Certificate{})
+	})
 }
 
 func TestNewPeerClientForAddress(t *testing.T) {

internal/peer/lifecycle/chaincode/client_connections.go

@@ -186,7 +186,7 @@ func (c *ClientConnectionsInput) appendPeerConfig(n *common.NetworkConfig, peer
 }
 
 func (c *ClientConnections) setCertificate() error {
-	certificate, err := common.GetCertificate()
+	certificate, err := common.GetClientCertificate()
 	if err != nil {
 		return errors.WithMessage(err, "failed to retrieve client cerificate")
 	}