[FAB-9601] Make system cert pool access thread safe

We currently only synchronize writes but not reads.

The comm package uses this cert pool for all outgoing
connections making it hard to sync reads.

As the x509 std lib does not allow deep copies of its
cert pool, we reload the cert pool whenever a new cert
is encountered, writing only to the fresh cert pool.

The attached benchmark verifies performance for the common
case:

BenchmarkTLSCertPool-8                30000000	        50.4 ns/op
BenchmarkTLSCertPoolSameCert-8        10000000	       123 ns/op
BenchmarkTLSCertPoolDifferentCert-8         50	  25575114 ns/op

This also partially reverts commit 65a56e2 as TLSCACertPool()
can now return an error.

Change-Id: I72a1f2a1eb21036e5e382fad208c9d44b6f09b96
Signed-off-by: Divyank Katira <Divyank.Katira@securekey.com>

Author: d1vyank

pkg/client/resmgmt/resmgmt_test.go

@@ -1643,7 +1643,7 @@ func getInvalidOrdererBackend(backend core.ConfigBackend) *mocks.MockConfigBacke
 	}
 	exampleOrderer := networkConfig.Orderers["orderer.example.com"]
 	exampleOrderer.TLSCACerts.Path = "/some/invalid/path"
-	exampleOrderer.TLSCACerts.Pem = ""
+	exampleOrderer.TLSCACerts.Pem = validRootCA
 	networkConfig.Orderers["orderer.example.com"] = exampleOrderer
 
 	mockConfigBackend := getCustomBackend(backend)
@@ -1671,3 +1671,20 @@ func getNoEventSourceBackend(backend core.ConfigBackend) *mocks.MockConfigBacken
 
 	return mockConfigBackend
 }
+
+var validRootCA = `-----BEGIN CERTIFICATE-----
+MIICYjCCAgmgAwIBAgIUB3CTDOU47sUC5K4kn/Caqnh114YwCgYIKoZIzj0EAwIw
+fzELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh
+biBGcmFuY2lzY28xHzAdBgNVBAoTFkludGVybmV0IFdpZGdldHMsIEluYy4xDDAK
+BgNVBAsTA1dXVzEUMBIGA1UEAxMLZXhhbXBsZS5jb20wHhcNMTYxMDEyMTkzMTAw
+WhcNMjExMDExMTkzMTAwWjB/MQswCQYDVQQGEwJVUzETMBEGA1UECBMKQ2FsaWZv
+cm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEfMB0GA1UEChMWSW50ZXJuZXQg
+V2lkZ2V0cywgSW5jLjEMMAoGA1UECxMDV1dXMRQwEgYDVQQDEwtleGFtcGxlLmNv
+bTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABKIH5b2JaSmqiQXHyqC+cmknICcF
+i5AddVjsQizDV6uZ4v6s+PWiJyzfA/rTtMvYAPq/yeEHpBUB1j053mxnpMujYzBh
+MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQXZ0I9
+qp6CP8TFHZ9bw5nRtZxIEDAfBgNVHSMEGDAWgBQXZ0I9qp6CP8TFHZ9bw5nRtZxI
+EDAKBggqhkjOPQQDAgNHADBEAiAHp5Rbp9Em1G/UmKn8WsCbqDfWecVbZPQj3RK4
+oG5kQQIgQAe4OOKYhJdh3f7URaKfGTf492/nmRmtK+ySKjpHSrU=
+-----END CERTIFICATE-----
+`

pkg/common/providers/fab/provider.go

@@ -96,7 +96,7 @@ type EndpointConfig interface {
 	ChannelConfig(name string) (*ChannelNetworkConfig, error)
 	ChannelPeers(name string) ([]ChannelPeer, error)
 	ChannelOrderers(name string) ([]OrdererConfig, error)
-	TLSCACertPool(certConfig ...*x509.Certificate) *x509.CertPool
+	TLSCACertPool(certConfig ...*x509.Certificate) (*x509.CertPool, error)
 	EventServiceType() EventServiceType
 	TLSClientCerts() ([]tls.Certificate, error)
 	CryptoConfigPath() string

pkg/common/providers/test/mockfab/mockconfig.go

@@ -35,8 +35,9 @@ const ErrorMessage = "default error message"
 func DefaultMockConfig(mockCtrl *gomock.Controller) *MockEndpointConfig {
 	config := NewMockEndpointConfig(mockCtrl)
 
-	config.EXPECT().TLSCACertPool(GoodCert).Return(CertPool).AnyTimes()
-	config.EXPECT().TLSCACertPool().Return(CertPool).AnyTimes()
+	config.EXPECT().TLSCACertPool(GoodCert).Return(CertPool, nil).AnyTimes()
+	config.EXPECT().TLSCACertPool(BadCert).Return(CertPool, errors.New(ErrorMessage)).AnyTimes()
+	config.EXPECT().TLSCACertPool().Return(CertPool, nil).AnyTimes()
 	config.EXPECT().Timeout(fab.EndorserConnection).Return(time.Second * 5).AnyTimes()
 	config.EXPECT().TLSClientCerts().Return([]tls.Certificate{TLSCert}, nil).AnyTimes()
 
@@ -47,8 +48,9 @@ func DefaultMockConfig(mockCtrl *gomock.Controller) *MockEndpointConfig {
 func BadTLSClientMockConfig(mockCtrl *gomock.Controller) *MockEndpointConfig {
 	config := NewMockEndpointConfig(mockCtrl)
 
-	config.EXPECT().TLSCACertPool(GoodCert).Return(CertPool).AnyTimes()
-	config.EXPECT().TLSCACertPool().Return(CertPool).AnyTimes()
+	config.EXPECT().TLSCACertPool(GoodCert).Return(CertPool, nil).AnyTimes()
+	config.EXPECT().TLSCACertPool(BadCert).Return(CertPool, errors.New(ErrorMessage)).AnyTimes()
+	config.EXPECT().TLSCACertPool().Return(CertPool, nil).AnyTimes()
 	config.EXPECT().Timeout(fab.EndorserConnection).Return(time.Second * 5).AnyTimes()
 	config.EXPECT().TLSClientCerts().Return(nil, errors.Errorf(ErrorMessage)).AnyTimes()
 

pkg/common/providers/test/mockfab/mockfab.gen.go

@@ -19,13 +19,21 @@ import (
 // TLSConfig returns the appropriate config for TLS including the root CAs,
 // certs for mutual TLS, and server host override. Works with certs loaded either from a path or embedded pem.
 func TLSConfig(cert *x509.Certificate, serverName string, config fab.EndpointConfig) (*tls.Config, error) {
-	certPool := config.TLSCACertPool()
+	certPool, err := config.TLSCACertPool()
+	if err != nil {
+		return nil, err
+	}
+
 	if cert == nil && (certPool == nil || len(certPool.Subjects()) == 0) {
 		//Return empty tls config if there is no cert provided or if certpool unavailable
 		return &tls.Config{}, nil
 	}
 
-	tlsCaCertPool := config.TLSCACertPool(cert)
+	tlsCaCertPool, err := config.TLSCACertPool(cert)
+
+	if err != nil {
+		return nil, err
+	}
 
 	clientCerts, err := config.TLSClientCerts()
 	if err != nil {

pkg/core/config/comm/comm.go

@@ -21,6 +21,22 @@ import (
 	"github.com/hyperledger/fabric-sdk-go/pkg/common/providers/test/mockfab"
 )
 
+func TestTLSConfigErrorAddingCertificate(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	config := mockfab.DefaultMockConfig(mockCtrl)
+
+	_, err := TLSConfig(mockfab.BadCert, "", config)
+	if err == nil {
+		t.Fatal("Expected failure adding invalid certificate")
+	}
+
+	if !strings.Contains(err.Error(), mockfab.ErrorMessage) {
+		t.Fatalf("Expected error: %s", mockfab.ErrorMessage)
+	}
+}
+
 func TestTLSConfigErrorFromClientCerts(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()

pkg/core/config/comm/comm_test.go

@@ -439,7 +439,22 @@ peers:
       #will be taken into consideration if address has no protocol defined, if true then grpc or else grpcs
       allow-insecure: false
     tlsCACerts:
-      pem:
+      # pem supersedes path
+      pem: |
+        -----BEGIN CERTIFICATE-----
+        MIICNjCCAdygAwIBAgIRAILSPmMB3BzoLIQGsFxwZr8wCgYIKoZIzj0EAwIwbDEL
+        MAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNhbiBG
+        cmFuY2lzY28xFDASBgNVBAoTC2V4YW1wbGUuY29tMRowGAYDVQQDExF0bHNjYS5l
+        eGFtcGxlLmNvbTAeFw0xNzA3MjgxNDI3MjBaFw0yNzA3MjYxNDI3MjBaMGwxCzAJ
+        BgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJh
+        bmNpc2NvMRQwEgYDVQQKEwtleGFtcGxlLmNvbTEaMBgGA1UEAxMRdGxzY2EuZXhh
+        bXBsZS5jb20wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQfgKb4db53odNzdMXn
+        P5FZTZTFztOO1yLvCHDofSNfTPq/guw+YYk7ZNmhlhj8JHFG6dTybc9Qb/HOh9hh
+        gYpXo18wXTAOBgNVHQ8BAf8EBAMCAaYwDwYDVR0lBAgwBgYEVR0lADAPBgNVHRMB
+        Af8EBTADAQH/MCkGA1UdDgQiBCBxaEP3nVHQx4r7tC+WO//vrPRM1t86SKN0s6XB
+        8LWbHTAKBggqhkjOPQQDAgNIADBFAiEA96HXwCsuMr7tti8lpcv1oVnXg0FlTxR/
+        SQtE5YgdxkUCIHReNWh/pluHTxeGu2jNCH1eh6o2ajSGeeizoapvdJbN
+        -----END CERTIFICATE-----
       path:
       #path: ${GOPATH}/src/github.com/hyperledger/fabric-sdk-go/${CRYPTOCONFIG_FIXTURES_PATH}/peerOrganizations/org2.example.com/tlsca/tlsca.org2.example.com-cert.pem
 
@@ -534,4 +549,4 @@ certificateAuthorities:
       enrollId: admin
       enrollSecret: adminpw
     # [Optional] The optional name of the CA.
-    caName: ca-org2
+    caName: ca-org2

pkg/core/config/testdata/config_test_embedded_pems.yaml

@@ -272,7 +272,22 @@ peers:
       ssl-target-name-override: peer0.org2.example.com
       allow-insecure: false
     tlsCACerts:
-      pem:
+      # pem supersedes path
+      pem: |
+        -----BEGIN CERTIFICATE-----
+        MIICNjCCAdygAwIBAgIRAILSPmMB3BzoLIQGsFxwZr8wCgYIKoZIzj0EAwIwbDEL
+        MAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNhbiBG
+        cmFuY2lzY28xFDASBgNVBAoTC2V4YW1wbGUuY29tMRowGAYDVQQDExF0bHNjYS5l
+        eGFtcGxlLmNvbTAeFw0xNzA3MjgxNDI3MjBaFw0yNzA3MjYxNDI3MjBaMGwxCzAJ
+        BgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJh
+        bmNpc2NvMRQwEgYDVQQKEwtleGFtcGxlLmNvbTEaMBgGA1UEAxMRdGxzY2EuZXhh
+        bXBsZS5jb20wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQfgKb4db53odNzdMXn
+        P5FZTZTFztOO1yLvCHDofSNfTPq/guw+YYk7ZNmhlhj8JHFG6dTybc9Qb/HOh9hh
+        gYpXo18wXTAOBgNVHQ8BAf8EBAMCAaYwDwYDVR0lBAgwBgYEVR0lADAPBgNVHRMB
+        Af8EBTADAQH/MCkGA1UdDgQiBCBxaEP3nVHQx4r7tC+WO//vrPRM1t86SKN0s6XB
+        8LWbHTAKBggqhkjOPQQDAgNIADBFAiEA96HXwCsuMr7tti8lpcv1oVnXg0FlTxR/
+        SQtE5YgdxkUCIHReNWh/pluHTxeGu2jNCH1eh6o2ajSGeeizoapvdJbN
+        -----END CERTIFICATE-----
       path:
       #path: ${GOPATH}/src/github.com/hyperledger/fabric-sdk-go/${CRYPTOCONFIG_FIXTURES_PATH}/peerOrganizations/org2.example.com/tlsca/tlsca.org2.example.com-cert.pem
 
@@ -366,4 +381,4 @@ certificateAuthorities:
       enrollId: admin
       enrollSecret: adminpw
     # [Optional] The optional name of the CA.
-    caName: ca.org2.example.com
+    caName: ca.org2.example.com

pkg/core/config/testdata/config_test_pem.yaml

@@ -99,15 +99,13 @@ func createMSPManager(ctx Context, cfg fab.ChannelCfg) (msp.MSPManager, error) {
 		if err := mspManager.Setup(msps); err != nil {
 			return nil, errors.WithMessage(err, "MSPManager Setup failed")
 		}
-
+		var certs [][]byte
 		for _, msp := range msps {
-			for _, cert := range msp.GetTLSRootCerts() {
-				addCertsToConfig(ctx.EndpointConfig, cert)
-			}
-
-			for _, cert := range msp.GetTLSIntermediateCerts() {
-				addCertsToConfig(ctx.EndpointConfig, cert)
-			}
+			certs = append(certs, msp.GetTLSRootCerts()...)
+			certs = append(certs, msp.GetTLSIntermediateCerts()...)
+		}
+		if len(certs) > 0 {
+			addCertsToConfig(ctx.EndpointConfig, certs)
 		}
 	}
 
@@ -185,25 +183,33 @@ func getFabricConfig(config *mb.MSPConfig) (*mb.FabricMSPConfig, error) {
 }
 
 //addCertsToConfig adds cert bytes to config TLSCACertPool
-func addCertsToConfig(config fab.EndpointConfig, pemCerts []byte) {
-	for len(pemCerts) > 0 {
-		var block *pem.Block
-		block, pemCerts = pem.Decode(pemCerts)
-		if block == nil {
-			break
-		}
-		if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
-			continue
-		}
+func addCertsToConfig(config fab.EndpointConfig, pemCertsList [][]byte) {
+	var certs []*x509.Certificate
+	for _, pemCerts := range pemCertsList {
+		for len(pemCerts) > 0 {
+			var block *pem.Block
+			block, pemCerts = pem.Decode(pemCerts)
+			if block == nil {
+				break
+			}
+			if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
+				continue
+			}
 
-		cert, err := x509.ParseCertificate(block.Bytes)
-		if err != nil {
-			continue
-		}
-		err = verifier.ValidateCertificateDates(cert)
-		if err != nil {
-			logger.Warn("%v", err)
+			cert, err := x509.ParseCertificate(block.Bytes)
+			if err != nil {
+				continue
+			}
+			err = verifier.ValidateCertificateDates(cert)
+			if err != nil {
+				logger.Warn("%v", err)
+			}
+
+			certs = append(certs, cert)
 		}
-		_ = config.TLSCACertPool(cert)
+	}
+	_, err := config.TLSCACertPool(certs...)
+	if err != nil {
+		logger.Warnf("TLSCACertPool failed %v", err)
 	}
 }