[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>
hyperledger · Apr 24, 2018 · 830bdea · 830bdea
1 parent e328e9d
commit 830bdea
Show file tree

Hide file tree

Showing 19 changed files with 336 additions and 117 deletions.
diff --git a/pkg/client/resmgmt/resmgmt_test.go b/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-----
+`
diff --git a/pkg/common/providers/fab/provider.go b/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

diff --git a/pkg/common/providers/test/mockfab/mockconfig.go b/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()
 

diff --git a/pkg/common/providers/test/mockfab/mockfab.gen.go b/pkg/common/providers/test/mockfab/mockfab.gen.go
diff --git a/pkg/core/config/comm/comm.go b/pkg/core/config/comm/comm.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 {

diff --git a/pkg/core/config/comm/comm_test.go b/pkg/core/config/comm/comm_test.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()

diff --git a/pkg/core/config/testdata/config_test_embedded_pems.yaml b/pkg/core/config/testdata/config_test_embedded_pems.yaml
@@ -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
diff --git a/pkg/core/config/testdata/config_test_pem.yaml b/pkg/core/config/testdata/config_test_pem.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
diff --git a/pkg/fab/channel/membership/membership.go b/pkg/fab/channel/membership/membership.go
@@ -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)
 	}
 }