FAB-17177] Config block shouldn't verify itself in block replication (#…

…355)

The cluster replication verifies blocks by pulling them in batches,
and performs hash chain verification + verifies the signatures of the last block in the chain.

It uses the bundle stored in the global config structures,
but in case it comes across a config block in the middle of the batch,
it switches to use a bundle constructed from that config block.

However, if the config block is the last block in the batch -
it accidentally verifies it twice:
 - once with the config block before it (as expected)
 - once with the config block itself (which is unexpected).

This change set simply adds a check to see if the last block in the batch
is a config block, and if so - doesn't verify it if it was already verified.

Change-Id: Id123c36e445a21b3081273ef0395aae759162818
Signed-off-by: yacovm <yacovm@il.ibm.com>

orderer/common/cluster/util.go

@@ -201,12 +201,14 @@ func VerifyBlocks(blockBuff []*common.Block, signatureVerifier BlockVerifier) er
 	}
 
 	var config *common.ConfigEnvelope
+	var isLastBlockConfigBlock bool
 	// Verify all configuration blocks that are found inside the block batch,
 	// with the configuration that was committed (nil) or with one that is picked up
 	// during iteration over the block batch.
 	for _, block := range blockBuff {
 		configFromBlock, err := ConfigFromBlock(block)
 		if err == errNotAConfig {
+			isLastBlockConfigBlock = false
 			continue
 		}
 		if err != nil {
@@ -217,10 +219,17 @@ func VerifyBlocks(blockBuff []*common.Block, signatureVerifier BlockVerifier) er
 			return err
 		}
 		config = configFromBlock
+		isLastBlockConfigBlock = true
 	}
 
 	// Verify the last block's signature
 	lastBlock := blockBuff[len(blockBuff)-1]
+
+	// If last block is a config block, we verified it using the policy of the previous block, so it's valid.
+	if isLastBlockConfigBlock {
+		return nil
+	}
+
 	return VerifyBlockSignature(lastBlock, signatureVerifier, config)
 }
 

orderer/common/cluster/util_test.go

@@ -357,10 +357,11 @@ func TestVerifyBlocks(t *testing.T) {
 	}
 
 	for _, testCase := range []struct {
-		name                string
-		configureVerifier   func(*mocks.BlockVerifier)
-		mutateBlockSequence func([]*common.Block) []*common.Block
-		expectedError       string
+		name                  string
+		configureVerifier     func(*mocks.BlockVerifier)
+		mutateBlockSequence   func([]*common.Block) []*common.Block
+		expectedError         string
+		verifierExpectedCalls int
 	}{
 		{
 			name: "empty sequence",
@@ -388,7 +389,8 @@ func TestVerifyBlocks(t *testing.T) {
 				var nilEnvelope *common.ConfigEnvelope
 				verifier.On("VerifyBlockSignature", mock.Anything, nilEnvelope).Return(errors.New("bad signature"))
 			},
-			expectedError: "bad signature",
+			expectedError:         "bad signature",
+			verifierExpectedCalls: 1,
 		},
 		{
 			name: "block that its type cannot be classified",
@@ -436,10 +438,11 @@ func TestVerifyBlocks(t *testing.T) {
 				proto.Unmarshal(protoutil.MarshalOrPanic(configEnvelope1), confEnv1)
 				verifier.On("VerifyBlockSignature", sigSet2, confEnv1).Return(errors.New("bad signature")).Once()
 			},
-			expectedError: "bad signature",
+			expectedError:         "bad signature",
+			verifierExpectedCalls: 2,
 		},
 		{
-			name: "config block in the sequence needs to be verified, and it isproperly signed",
+			name: "config block in the sequence needs to be verified, and it is properly signed",
 			mutateBlockSequence: func(blockSequence []*common.Block) []*common.Block {
 				var err error
 				// Put a config transaction in block n / 4
@@ -465,6 +468,48 @@ func TestVerifyBlocks(t *testing.T) {
 				verifier.On("VerifyBlockSignature", sigSet1, nilEnvelope).Return(nil).Once()
 				verifier.On("VerifyBlockSignature", sigSet2, confEnv1).Return(nil).Once()
 			},
+			// We have a single config block in the 'middle' of the chain, so we have 2 verifications total:
+			// The last block, and the config block.
+			verifierExpectedCalls: 2,
+		},
+		{
+			name: "last two blocks are config blocks, last block only verified once",
+			mutateBlockSequence: func(blockSequence []*common.Block) []*common.Block {
+				var err error
+				// Put a config transaction in block n-2 and in n-1
+				blockSequence[len(blockSequence)-2].Data = &common.BlockData{
+					Data: [][]byte{protoutil.MarshalOrPanic(configTransaction(configEnvelope1))},
+				}
+				blockSequence[len(blockSequence)-2].Header.DataHash = protoutil.BlockDataHash(blockSequence[len(blockSequence)-2].Data)
+
+				blockSequence[len(blockSequence)-1].Data = &common.BlockData{
+					Data: [][]byte{protoutil.MarshalOrPanic(configTransaction(configEnvelope2))},
+				}
+				blockSequence[len(blockSequence)-1].Header.DataHash = protoutil.BlockDataHash(blockSequence[len(blockSequence)-1].Data)
+
+				assignHashes(blockSequence)
+
+				sigSet1, err = cluster.SignatureSetFromBlock(blockSequence[len(blockSequence)-2])
+				assert.NoError(t, err)
+
+				sigSet2, err = cluster.SignatureSetFromBlock(blockSequence[len(blockSequence)-1])
+				assert.NoError(t, err)
+
+				return blockSequence
+			},
+			configureVerifier: func(verifier *mocks.BlockVerifier) {
+				var nilEnvelope *common.ConfigEnvelope
+				confEnv1 := &common.ConfigEnvelope{}
+				proto.Unmarshal(protoutil.MarshalOrPanic(configEnvelope1), confEnv1)
+				verifier.On("VerifyBlockSignature", sigSet1, nilEnvelope).Return(nil).Once()
+				// We ensure that the signature set of the last block is verified using the config envelope of the block
+				// before it.
+				verifier.On("VerifyBlockSignature", sigSet2, confEnv1).Return(nil).Once()
+				// Note that we do not record a call to verify the last block, with the config envelope extracted from the block itself.
+			},
+			// We have 2 config blocks, yet we only verify twice- the first config block, and the next config block, but no more,
+			// since the last block is a config block.
+			verifierExpectedCalls: 2,
 		},
 	} {
 		testCase := testCase
@@ -478,6 +523,8 @@ func TestVerifyBlocks(t *testing.T) {
 			err := cluster.VerifyBlocks(blockchain, verifier)
 			if testCase.expectedError != "" {
 				assert.EqualError(t, err, testCase.expectedError)
+			} else {
+				assert.NoError(t, err)
 			}
 		})
 	}

orderer/common/server/onboarding_test.go

@@ -635,7 +635,7 @@ func TestReplicate(t *testing.T) {
 		},
 		{
 			name:               "Explicit replication is requested, but the channel shouldn't be pulled",
-			verificationCount:  20,
+			verificationCount:  10,
 			shouldConnect:      true,
 			systemLedgerHeight: 10,
 			bootBlock:          &bootBlock,