From 7bbc0842b079ad1d0743100fd18e4745b6bf6ba0 Mon Sep 17 00:00:00 2001 From: Wenjian Qiao Date: Wed, 18 Nov 2020 15:40:03 -0500 Subject: [PATCH] [FAB-18339] CLI: rename GetCerificate to GetClientCertificate 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 --- internal/peer/chaincode/common.go | 2 +- internal/peer/common/common.go | 34 +++++---- internal/peer/common/peerclient.go | 69 ++++++++----------- internal/peer/common/peerclient_test.go | 63 ++++++++++++++++- internal/peer/common/testdata/certs/empty.crt | 0 internal/peer/common/testdata/certs/empty.key | 0 .../lifecycle/chaincode/client_connections.go | 2 +- 7 files changed, 110 insertions(+), 60 deletions(-) create mode 100644 internal/peer/common/testdata/certs/empty.crt create mode 100644 internal/peer/common/testdata/certs/empty.key diff --git a/internal/peer/chaincode/common.go b/internal/peer/chaincode/common.go index 88e6eb3ea2d..2723e2ef684 100644 --- a/internal/peer/chaincode/common.go +++ b/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") } diff --git a/internal/peer/common/common.go b/internal/peer/common/common.go index 46654fdc454..dbddbf5a467 100644 --- a/internal/peer/common/common.go +++ b/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 diff --git a/internal/peer/common/peerclient.go b/internal/peer/common/peerclient.go index 9c6d5548159..767194b4ca5 100644 --- a/internal/peer/common/peerclient.go +++ b/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,26 +128,29 @@ 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 @@ -160,13 +158,7 @@ func GetCertificate() (tls.Certificate, error) { // 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() +} diff --git a/internal/peer/common/peerclient_test.go b/internal/peer/common/peerclient_test.go index 7881153d27b..05c67a1a2ca 100644 --- a/internal/peer/common/peerclient_test.go +++ b/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) { diff --git a/internal/peer/common/testdata/certs/empty.crt b/internal/peer/common/testdata/certs/empty.crt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/internal/peer/common/testdata/certs/empty.key b/internal/peer/common/testdata/certs/empty.key new file mode 100644 index 00000000000..e69de29bb2d diff --git a/internal/peer/lifecycle/chaincode/client_connections.go b/internal/peer/lifecycle/chaincode/client_connections.go index 36ec3a8dbf1..3fc8ec46037 100644 --- a/internal/peer/lifecycle/chaincode/client_connections.go +++ b/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") }