Skip to content

Commit

Permalink
[FAB-11135] tls certpool lock fix
Browse files Browse the repository at this point in the history 
Change-Id: I3431ecd22097e40785b6e9a7d02506a1d5c2823f
Signed-off-by: Sudesh Shetty <sudesh.shetty@securekey.com>
  • Loading branch information
@sudeshrshetty
sudeshrshetty committed Aug 8, 2018
1 parent c0144e2 commit 4b2d30f
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 41 deletions.
60 changes: 40 additions & 20 deletions pkg/core/config/comm/tls/certpool.go
View file
Expand Up @@ -22,12 +22,12 @@ var logger = logging.NewLogger("fabsdk/core")
// cert pool implementation.
// It optionally allows loading the system trust store.
type certPool struct {
certPool *x509.CertPool
certs []*x509.Certificate
certsByName map[string][]int
lock sync.RWMutex
dirty int32
certQueue []*x509.Certificate
certPool *x509.CertPool
certs []*x509.Certificate
certsByName map[string][]int
lock sync.RWMutex
dirty int32
systemCertPool bool
}

// NewCertPool new CertPool implementation
Expand All @@ -39,8 +39,9 @@ func NewCertPool(useSystemCertPool bool) (fab.CertPool, error) {
}

newCertPool := &certPool{
certsByName: make(map[string][]int),
certPool: c,
certsByName: make(map[string][]int),
certPool: c,
systemCertPool: useSystemCertPool,
}

return newCertPool, nil
Expand All @@ -52,15 +53,11 @@ func (c *certPool) Get() (*x509.CertPool, error) {

//if dirty then add certs from queue to cert pool
if atomic.CompareAndSwapInt32(&c.dirty, 1, 0) {
c.lock.Lock()

//add all new certs in queue to cert pool
for _, cert := range c.certQueue {
c.certPool.AddCert(cert)
//swap certpool if queue is dirty
err := c.swapCertPool()
if err != nil {
return nil, err
}
c.certQueue = []*x509.Certificate{}

c.lock.Unlock()
}

c.lock.RLock()
Expand All @@ -75,16 +72,15 @@ func (c *certPool) Add(certs ...*x509.Certificate) {
return
}

c.lock.Lock()
defer c.lock.Unlock()

//filter certs to be added, check if they already exist or duplicate
certsToBeAdded := c.filterCerts(certs...)

if len(certsToBeAdded) > 0 {

c.lock.Lock()
defer c.lock.Unlock()

for _, newCert := range certsToBeAdded {
c.certQueue = append(c.certQueue, newCert)
// Store cert name index
name := string(newCert.RawSubject)
c.certsByName[name] = append(c.certsByName[name], len(c.certs))
Expand All @@ -96,8 +92,32 @@ func (c *certPool) Add(certs ...*x509.Certificate) {
}
}

func (c *certPool) swapCertPool() error {

newCertPool, err := loadSystemCertPool(c.systemCertPool)
if err != nil {
return err
}

c.lock.Lock()
defer c.lock.Unlock()

//add all new certs in queue to new cert pool
for _, cert := range c.certs {
newCertPool.AddCert(cert)
}

//swap old certpool with new one
c.certPool = newCertPool

return nil
}

//filterCerts remove certs from list if they already exist in pool or duplicate
func (c *certPool) filterCerts(certs ...*x509.Certificate) []*x509.Certificate {
c.lock.RLock()
defer c.lock.RUnlock()

filtered := []*x509.Certificate{}

CertLoop:
Expand Down
42 changes: 21 additions & 21 deletions pkg/core/config/comm/tls/certpool_test.go
View file
Expand Up @@ -101,59 +101,59 @@ func TestTLSCAConfigWithMultipleCerts(t *testing.T) {
//Empty cert pool with just system cert pool certs
pool, err := tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 0, 0, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 0, 0, numberOfSubjects, 0)

//add 2 certs
tlsCertPool.Add(certOrderer, certOrg1)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 2, 2, 2, numberOfSubjects, 1)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 2, 2, numberOfSubjects, 1)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)

//add 1 existing cert, queue should be unchanged and dirty flag should be off
tlsCertPool.Add(certOrg1)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)

//try again, add 1 existing cert, queue should be unchanged and dirty flag should be off
tlsCertPool.Add(certOrg1)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)

//add 2 existing certs, queue should be unchanged and dirty flag should be off
tlsCertPool.Add(certOrderer, certOrg1)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)

//add 3 certs, (2 existing + 1 new), queue should have one extra cert and dirty flag should be on
tlsCertPool.Add(certOrderer, certOrg1, certOrg2)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 3, 3, 1, numberOfSubjects, 1)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 3, 3, numberOfSubjects, 1)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, numberOfSubjects, 0)

//add all 3 existing certs, queue should be unchanged and dirty flag should be off
tlsCertPool.Add(certOrderer, certOrg1, certOrg2)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, numberOfSubjects, 0)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, numberOfSubjects, 0)

}

func verifyCertPoolInstance(t *testing.T, pool *x509.CertPool, fabPool fab.CertPool, numberOfCertsInPool, numberOfCerts, numberOfCertsByName, numbersOfCertsInQueue, numberOfSubjects int, dirty int32) {
func verifyCertPoolInstance(t *testing.T, pool *x509.CertPool, fabPool fab.CertPool, numberOfCertsInPool, numberOfCerts, numberOfCertsByName, numberOfSubjects int, dirty int32) {
assert.NotNil(t, fabPool)
tlsCertPool := fabPool.(*certPool)
assert.Equal(t, dirty, tlsCertPool.dirty)
assert.Equal(t, numberOfCerts, len(tlsCertPool.certs))
assert.Equal(t, numberOfCertsByName, len(tlsCertPool.certsByName))
assert.Equal(t, numbersOfCertsInQueue, len(tlsCertPool.certQueue))
assert.Equal(t, numberOfSubjects+numberOfCertsInPool, len(pool.Subjects()))
assert.Equal(t, numberOfSubjects+numberOfCertsInPool, len(pool.Subjects()))
}

Expand Down Expand Up @@ -183,21 +183,21 @@ func TestAddingDuplicateCertsToPool(t *testing.T) {
//Empty cert pool with just system cert pool certs
pool, err := tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 0, 0, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 0, 0, numberOfSubjects, 0)

//add multiple certs with duplicate
tlsCertPool.Add(certOrderer, certOrg1, certOrderer, certOrg1, certOrg1, certOrg1, certOrderer, certOrderer)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 2, 2, 2, numberOfSubjects, 1)
verifyCertPoolInstance(t, pool, tlsCertPool, 0, 2, 2, numberOfSubjects, 1)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 2, 2, numberOfSubjects, 0)

//add multiple certs with duplicate
tlsCertPool.Add(certOrderer, certOrg1, certOrderer, certOrg1, certOrg2, certOrg2, certOrg2, certOrderer, certOrderer)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 3, 3, 1, numberOfSubjects, 1)
verifyCertPoolInstance(t, pool, tlsCertPool, 2, 3, 3, numberOfSubjects, 1)
pool, err = tlsCertPool.Get()
assert.Nil(t, err)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, 0, numberOfSubjects, 0)
verifyCertPoolInstance(t, pool, tlsCertPool, 3, 3, 3, numberOfSubjects, 0)
}

func TestRemoveDuplicatesCerts(t *testing.T) {
Expand Down Expand Up @@ -293,7 +293,7 @@ func TestConcurrent(t *testing.T) {
concurrency := 1000
certs := createNCerts(concurrency)

fabCertPool, err := NewCertPool(true)
fabCertPool, err := NewCertPool(false)
require.NoError(t, err)

tlsCertPool := fabCertPool.(*certPool)
Expand Down
7 changes: 7 additions & 0 deletions pkg/fab/channel/membership/membership.go
View file
Expand Up @@ -129,6 +129,13 @@ func createMSPManager(ctx Context, cfg fab.ChannelCfg) (msp.MSPManager, []string
}
}

//To make sure tls cert pool is updated in advance with all the new certs being added,
// to avoid delay in first endorsement connection with new peer
_, err := ctx.EndpointConfig.TLSCACertPool().Get()
if err != nil {
return nil, nil, err
}

return mspManager, mspNames, nil
}

Expand Down

0 comments on commit 4b2d30f

Please sign in to comment.