[FAB-8936] making cert pool threadsafe

- provided locks on adding certs to config tls
ca cert pool
- removed function for replacing whole cert pool
- removed 'mock_core' package name to avoid lint
warnings


Change-Id: I975a7e7773b5899dbe4d59bc55a0fa27955609f7
Signed-off-by: Sudesh Shetty <sudesh.shetty@securekey.com>

Makefile

@@ -339,7 +339,7 @@ dockerenv-latest-up: clean
 
 .PHONY: mock-gen
 mock-gen:
-	mockgen -build_flags '$(GO_LDFLAGS_ARG)' github.com/hyperledger/fabric-sdk-go/pkg/common/providers/core Config,Providers | sed "s/github.com\/hyperledger\/fabric-sdk-go\/vendor\///g" | goimports > pkg/common/providers/core/mocks/mockcoreapi.gen.go
+	mockgen -build_flags '$(GO_LDFLAGS_ARG)' -package mocks github.com/hyperledger/fabric-sdk-go/pkg/common/providers/core Config,Providers | sed "s/github.com\/hyperledger\/fabric-sdk-go\/vendor\///g" | goimports > pkg/common/providers/core/mocks/mockcoreapi.gen.go
 	mockgen -build_flags '$(GO_LDFLAGS_ARG)' github.com/hyperledger/fabric-sdk-go/pkg/common/providers/msp IdentityManager,Providers | sed "s/github.com\/hyperledger\/fabric-sdk-go\/vendor\///g" | goimports > pkg/common/providers/msp/mocks/mockmspapi.gen.go
 	mockgen -build_flags '$(GO_LDFLAGS_ARG)' github.com/hyperledger/fabric-sdk-go/pkg/common/providers/fab ProposalProcessor,Providers | sed "s/github.com\/hyperledger\/fabric-sdk-go\/vendor\///g" | goimports > pkg/common/providers/fab/mocks/mockfabapi.gen.go
 	mockgen -build_flags '$(GO_LDFLAGS_ARG)' github.com/hyperledger/fabric-sdk-go/pkg/common/providers/context Providers,Client | sed "s/github.com\/hyperledger\/fabric-sdk-go\/vendor\///g" | goimports > pkg/common/providers/context/mocks/mockcontext.gen.go

pkg/common/providers/core/mocks/mockconfig.go

@@ -4,7 +4,7 @@ Copyright SecureKey Technologies Inc., Unchain B.V. All Rights Reserved.
 SPDX-License-Identifier: Apache-2.0
 */
 
-package mock_core
+package mocks
 
 import (
 	tls "crypto/tls"

pkg/common/providers/core/provider.go

@@ -40,7 +40,6 @@ type Config interface {
 	ChannelConfig(name string) (*ChannelConfig, error)
 	ChannelPeers(name string) ([]ChannelPeer, error)
 	ChannelOrderers(name string) ([]OrdererConfig, error)
-	SetTLSCACertPool(*x509.CertPool)
 	TLSCACertPool(certConfig ...*x509.Certificate) (*x509.CertPool, error)
 	IsSecurityEnabled() bool
 	SecurityAlgorithm() string

pkg/core/config/comm/comm_test.go

@@ -25,44 +25,44 @@ func TestTLSConfigErrorAddingCertificate(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
-	config := mock_core.DefaultMockConfig(mockCtrl)
+	config := mocks.DefaultMockConfig(mockCtrl)
 
-	_, err := TLSConfig(mock_core.BadCert, "", config)
+	_, err := TLSConfig(mocks.BadCert, "", config)
 	if err == nil {
 		t.Fatal("Expected failure adding invalid certificate")
 	}
 
-	if !strings.Contains(err.Error(), mock_core.ErrorMessage) {
-		t.Fatalf("Expected error: %s", mock_core.ErrorMessage)
+	if !strings.Contains(err.Error(), mocks.ErrorMessage) {
+		t.Fatalf("Expected error: %s", mocks.ErrorMessage)
 	}
 }
 
 func TestTLSConfigErrorFromClientCerts(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
-	config := mock_core.BadTLSClientMockConfig(mockCtrl)
+	config := mocks.BadTLSClientMockConfig(mockCtrl)
 
-	_, err := TLSConfig(mock_core.GoodCert, "", config)
+	_, err := TLSConfig(mocks.GoodCert, "", config)
 
 	if err == nil {
 		t.Fatal("Expected failure from loading client certs")
 	}
 
-	if !strings.Contains(err.Error(), mock_core.ErrorMessage) {
-		t.Fatalf("Expected error: %s", mock_core.ErrorMessage)
+	if !strings.Contains(err.Error(), mocks.ErrorMessage) {
+		t.Fatalf("Expected error: %s", mocks.ErrorMessage)
 	}
 }
 
 func TestTLSConfigHappyPath(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
-	config := mock_core.DefaultMockConfig(mockCtrl)
+	config := mocks.DefaultMockConfig(mockCtrl)
 
 	serverHostOverride := "servernamebeingoverriden"
 
-	tlsConfig, err := TLSConfig(mock_core.GoodCert, serverHostOverride, config)
+	tlsConfig, err := TLSConfig(mocks.GoodCert, serverHostOverride, config)
 	if err != nil {
 		t.Fatalf("Unexpected error: %s", err)
 	}
@@ -71,23 +71,23 @@ func TestTLSConfigHappyPath(t *testing.T) {
 		t.Fatal("Incorrect server name!")
 	}
 
-	if tlsConfig.RootCAs != mock_core.CertPool {
+	if tlsConfig.RootCAs != mocks.CertPool {
 		t.Fatal("Incorrect cert pool")
 	}
 
 	if len(tlsConfig.Certificates) != 1 {
 		t.Fatal("Incorrect number of certs")
 	}
 
-	if !reflect.DeepEqual(tlsConfig.Certificates[0], mock_core.TLSCert) {
+	if !reflect.DeepEqual(tlsConfig.Certificates[0], mocks.TLSCert) {
 		t.Fatal("Certs do not match")
 	}
 }
 
 func TestNoTlsCertHash(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
-	config := mock_core.NewMockConfig(mockCtrl)
+	config := mocks.NewMockConfig(mockCtrl)
 
 	config.EXPECT().TLSClientCerts().Return([]tls.Certificate{}, nil)
 
@@ -101,7 +101,7 @@ func TestNoTlsCertHash(t *testing.T) {
 func TestEmptyTlsCertHash(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
-	config := mock_core.NewMockConfig(mockCtrl)
+	config := mocks.NewMockConfig(mockCtrl)
 
 	emptyCert := tls.Certificate{}
 	config.EXPECT().TLSClientCerts().Return([]tls.Certificate{emptyCert}, nil)
@@ -116,7 +116,7 @@ func TestEmptyTlsCertHash(t *testing.T) {
 func TestTlsCertHash(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
-	config := mock_core.NewMockConfig(mockCtrl)
+	config := mocks.NewMockConfig(mockCtrl)
 
 	cert, err := tls.LoadX509KeyPair("testdata/server.crt", "testdata/server.key")
 	if err != nil {

pkg/core/config/config.go

@@ -32,6 +32,8 @@ import (
 
 	"regexp"
 
+	"sync"
+
 	cs "github.com/hyperledger/fabric-sdk-go/pkg/core/cryptosuite"
 )
 
@@ -52,14 +54,15 @@ var logModules = [...]string{"fabsdk", "fabsdk/client", "fabsdk/core", "fabsdk/f
 
 // Config represents the configuration for the client
 type Config struct {
-	tlsCertPool         *x509.CertPool
+	tlsCerts            []*x509.Certificate
 	networkConfig       *core.NetworkConfig
 	networkConfigCached bool
 	configViper         *viper.Viper
 	peerMatchers        map[int]*regexp.Regexp
 	ordererMatchers     map[int]*regexp.Regexp
 	caMatchers          map[int]*regexp.Regexp
 	opts                options
+	certPoolLock        sync.Mutex
 }
 
 type options struct {
@@ -213,13 +216,8 @@ func newViper(cmdRootPrefix string) *viper.Viper {
 
 func initConfig(c *Config) (*Config, error) {
 	setLogLevel(c.configViper)
-	tlsCertPool, err := getCertPool(c.configViper)
-	if err != nil {
-		return nil, err
-	}
-	c.tlsCertPool = tlsCertPool
 
-	if err = c.cacheNetworkConfiguration(); err != nil {
+	if err := c.cacheNetworkConfiguration(); err != nil {
 		return nil, errors.WithMessage(err, "network configuration load failed")
 	}
 
@@ -240,18 +238,6 @@ func initConfig(c *Config) (*Config, error) {
 	return c, nil
 }
 
-func getCertPool(myViper *viper.Viper) (*x509.CertPool, error) {
-	tlsCertPool := x509.NewCertPool()
-	if myViper.GetBool("client.tlsCerts.systemCertPool") == true {
-		var err error
-		if tlsCertPool, err = x509.SystemCertPool(); err != nil {
-			return nil, err
-		}
-		logger.Debugf("Loaded system cert pool of size: %d", len(tlsCertPool.Subjects()))
-	}
-	return tlsCertPool, nil
-}
-
 // setLogLevel will set the log level of the client
 func setLogLevel(myViper *viper.Viper) {
 	loggingLevelString := myViper.GetString("client.logging.level")
@@ -1314,25 +1300,54 @@ func (c *Config) verifyPeerConfig(p core.PeerConfig, peerName string, tlsEnabled
 	return nil
 }
 
-// SetTLSCACertPool allows a user to set a global cert pool with a set of
-// root TLS CAs that will be used for all outgoing connections
-func (c *Config) SetTLSCACertPool(certPool *x509.CertPool) {
-	if certPool == nil {
-		certPool = x509.NewCertPool()
-	}
-	c.tlsCertPool = certPool
-}
-
 // TLSCACertPool returns the configured cert pool. If a certConfig
 // is provided, the certficate is added to the pool
 func (c *Config) TLSCACertPool(certs ...*x509.Certificate) (*x509.CertPool, error) {
-	for _, cert := range certs {
-		if cert != nil {
-			c.tlsCertPool.AddCert(cert)
+
+	c.certPoolLock.Lock()
+	defer c.certPoolLock.Unlock()
+
+	//add cert if it is not nil and doesn't exists already
+	for _, newCert := range certs {
+		if newCert != nil && !c.containsCert(newCert) {
+			c.tlsCerts = append(c.tlsCerts, newCert)
 		}
 	}
 
-	return c.tlsCertPool, nil
+	//get new cert pool
+	tlsCertPool, err := c.getCertPool()
+	if err != nil {
+		return nil, errors.WithMessage(err, "failed to create cert pool")
+	}
+
+	//add all tls ca certs to cert pool
+	for _, cert := range c.tlsCerts {
+		tlsCertPool.AddCert(cert)
+	}
+
+	return tlsCertPool, nil
+}
+
+func (c *Config) containsCert(newCert *x509.Certificate) bool {
+	//TODO may need to maintain separate map of {cert.RawSubject, cert} to improve performance on search
+	for _, cert := range c.tlsCerts {
+		if cert.Equal(newCert) {
+			return true
+		}
+	}
+	return false
+}
+
+func (c *Config) getCertPool() (*x509.CertPool, error) {
+	tlsCertPool := x509.NewCertPool()
+	if c.configViper.GetBool("client.tlsCerts.systemCertPool") == true {
+		var err error
+		if tlsCertPool, err = x509.SystemCertPool(); err != nil {
+			return nil, err
+		}
+		logger.Debugf("Loaded system cert pool of size: %d", len(tlsCertPool.Subjects()))
+	}
+	return tlsCertPool, nil
 }
 
 // IsSecurityEnabled ...

pkg/core/config/config_test.go

@@ -291,16 +291,23 @@ func TestTLSCAConfig(t *testing.T) {
 	certConfig := endpoint.TLSConfig{Path: certFile}
 
 	cert, err := certConfig.TLSCert()
+	if err != nil {
+		t.Fatalf("Failed to get TLS CA Cert, reason: %v", err)
+	}
 
+	_, err = configImpl.TLSCACertPool(cert)
 	if err != nil {
 		t.Fatalf("TLS CA cert pool fetch failed, reason: %v", err)
 	}
 
+	//Try again with same cert
 	_, err = configImpl.TLSCACertPool(cert)
-
 	if err != nil {
 		t.Fatalf("TLS CA cert pool fetch failed, reason: %v", err)
 	}
+
+	assert.False(t, len(configImpl.tlsCerts) > 1, "number of certs in cert list shouldn't accept duplicates")
+
 	//Test TLSCA Cert Pool (Negative test case)
 
 	badCertConfig := endpoint.TLSConfig{Path: "some random invalid path"}
@@ -976,10 +983,12 @@ func TestSystemCertPoolDisabled(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := configProvider.(*Config)
-
+	certPool, err := configProvider.TLSCACertPool()
+	if err != nil {
+		t.Fatal("not supposed to get error")
+	}
 	// cert pool should be empty
-	if len(c.tlsCertPool.Subjects()) > 0 {
+	if len(certPool.Subjects()) > 0 {
 		t.Fatal("Expecting empty tls cert pool due to disabled system cert pool")
 	}
 }
@@ -992,25 +1001,23 @@ func TestSystemCertPoolEnabled(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := configProvider.(*Config)
+	certPool, err := configProvider.TLSCACertPool()
+	if err != nil {
+		t.Fatal("not supposed to get error")
+	}
 
-	if len(c.tlsCertPool.Subjects()) == 0 {
+	if len(certPool.Subjects()) == 0 {
 		t.Fatal("System Cert Pool not loaded even though it is enabled")
 	}
 
 	// Org2 'mychannel' peer is missing cert + pem (it should not fail when systemCertPool enabled)
-	_, err = c.ChannelPeers("mychannel")
+	_, err = configProvider.ChannelPeers("mychannel")
 	if err != nil {
 		t.Fatalf("Should have skipped verifying ca cert + pem: %s", err)
 	}
 
 }
 
-func TestSetTLSCACertPool(t *testing.T) {
-	configImpl.SetTLSCACertPool(nil)
-	t.Log("TLSCACertRoot must be created. Nothing additional to verify..")
-}
-
 func TestInitConfigFromRawWithPem(t *testing.T) {
 	// get a config byte for testing
 	cBytes, err := loadConfigBytesFromFile(t, configPemTestFilePath)

pkg/core/cryptosuite/bccsp/multisuite/cryptosuiteimpl_test.go

@@ -21,7 +21,7 @@ func TestBadConfig(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
-	mockConfig := mock_core.NewMockConfig(mockCtrl)
+	mockConfig := mocks.NewMockConfig(mockCtrl)
 	mockConfig.EXPECT().SecurityProvider().Return("UNKNOWN")
 	mockConfig.EXPECT().SecurityProvider().Return("UNKNOWN")
 
@@ -36,7 +36,7 @@ func TestCryptoSuiteByConfigSW(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
-	mockConfig := mock_core.NewMockConfig(mockCtrl)
+	mockConfig := mocks.NewMockConfig(mockCtrl)
 	mockConfig.EXPECT().SecurityProvider().Return("SW")
 	mockConfig.EXPECT().SecurityProvider().Return("SW")
 	mockConfig.EXPECT().SecurityAlgorithm().Return("SHA2")
@@ -61,7 +61,7 @@ func TestCryptoSuiteByConfigPKCS11(t *testing.T) {
 	//Prepare Config
 	providerLib, softHSMPin, softHSMTokenLabel := pkcs11.FindPKCS11Lib()
 
-	mockConfig := mock_core.NewMockConfig(mockCtrl)
+	mockConfig := mocks.NewMockConfig(mockCtrl)
 	mockConfig.EXPECT().SecurityProvider().Return("PKCS11")
 	mockConfig.EXPECT().SecurityProvider().Return("PKCS11")
 	mockConfig.EXPECT().SecurityAlgorithm().Return("SHA2")