[FAB-5606] (Backport) Failed ctxu may mutate cache

CR Number 2: Backported for v1.0.2

The configtx code maintains a map of the current config, as derived from
the Config proto structure.  This map stores references to a cached
Config proto structure which is used when constructing the next Config
structure.

The problem arises when this map is used to construct a new Config to be
applied, that it mutates the cached version of of the Config.  This is
generally fine, so long as the new Config applies successfully, but in
the event of bad inputs, such as a bad certificate, the config update
fails to apply and is rolled back, but the cache has been mutated and
will not be rolled back with it.

The observed issue occurs because this Config cache is also used in
creating the new channel config template.  So, because there is a bad
certificate in the config cache, the new channel template attempts to
bootstrap using the bad key material, detects the error, and aborts.

As noted in the issue, restarting the orderer rebuilds this cache, and
channel creation can occur normally once more.

This CR fixes the code which constructs a new Config from the config map
to create a copy of the cached config in-process, rather than taint the
cache with potentially invalid data.

Note, there may be novel ways to corrupt this cache which could cause
other undesirable behavior.  However, prior to the operation which
mutates the cache, the config update has been validated to adheer to the
security constraints of the channel (including all necessary admin
signatures), so it requires in a sense, a conspiracy of channel
administrators attempting to corrupt their own channel, so the security
implications are limited or non-existant.

Change-Id: I56bf6c8bc204785ef6634fd0352466ad3ab6d2af
Signed-off-by: Jason Yellick <[email protected]>

Author: Jason Yellick

common/configtx/configmap.go

@@ -21,6 +21,8 @@ import (
 	"strings"
 
 	cb "github.com/hyperledger/fabric/protos/common"
+
+	"github.com/golang/protobuf/proto"
 )
 
 const (
@@ -114,7 +116,7 @@ func configMapToConfig(configMap map[string]comparable) (*cb.ConfigGroup, error)
 }
 
 // recurseConfigMap is used only internally by configMapToConfig
-// Note, this function mutates the cb.Config* entrieswithin configMap, so errors are generally fatal
+// Note, this function no longer mutates the cb.Config* entries within configMap
 func recurseConfigMap(path string, configMap map[string]comparable) (*cb.ConfigGroup, error) {
 	groupPath := GroupPrefix + path
 	group, ok := configMap[groupPath]
@@ -126,12 +128,15 @@ func recurseConfigMap(path string, configMap map[string]comparable) (*cb.ConfigG
 		return nil, fmt.Errorf("ConfigGroup not found at group path: %s", groupPath)
 	}
 
+	newConfigGroup := cb.NewConfigGroup()
+	proto.Merge(newConfigGroup, group.ConfigGroup)
+
 	for key, _ := range group.Groups {
 		updatedGroup, err := recurseConfigMap(path+PathSeparator+key, configMap)
 		if err != nil {
 			return nil, err
 		}
-		group.Groups[key] = updatedGroup
+		newConfigGroup.Groups[key] = updatedGroup
 	}
 
 	for key, _ := range group.Values {
@@ -143,7 +148,7 @@ func recurseConfigMap(path string, configMap map[string]comparable) (*cb.ConfigG
 		if value.ConfigValue == nil {
 			return nil, fmt.Errorf("ConfigValue not found at value path: %s", valuePath)
 		}
-		group.Values[key] = value.ConfigValue
+		newConfigGroup.Values[key] = proto.Clone(value.ConfigValue).(*cb.ConfigValue)
 	}
 
 	for key, _ := range group.Policies {
@@ -155,8 +160,9 @@ func recurseConfigMap(path string, configMap map[string]comparable) (*cb.ConfigG
 		if policy.ConfigPolicy == nil {
 			return nil, fmt.Errorf("ConfigPolicy not found at policy path: %s", policyPath)
 		}
-		group.Policies[key] = policy.ConfigPolicy
+		newConfigGroup.Policies[key] = proto.Clone(policy.ConfigPolicy).(*cb.ConfigPolicy)
+		logger.Debugf("Setting policy for key %s to %+v", key, group.Policies[key])
 	}
 
-	return group.ConfigGroup, nil
+	return newConfigGroup, nil
 }

common/configtx/configmap_test.go

@@ -19,11 +19,16 @@ package configtx
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
-
 	cb "github.com/hyperledger/fabric/protos/common"
+
+	logging "github.com/op/go-logging"
+	"github.com/stretchr/testify/assert"
 )
 
+func init() {
+	logging.SetLevel(logging.DEBUG, "")
+}
+
 func TestBadKey(t *testing.T) {
 	assert.Error(t, addToMap(comparable{key: "[Label]", path: []string{}}, make(map[string]comparable)),
 		"Should have errored on key with illegal characters")
@@ -90,4 +95,7 @@ func TestMapConfigBack(t *testing.T) {
 	assert.NoError(t, err, "Should not have errored building config")
 
 	assert.Equal(t, config, newConfig, "Should have transformed config map back from confMap")
+
+	newConfig.Values["Value"] = &cb.ConfigValue{}
+	assert.NotEqual(t, config, newConfig, "Mutating the new config should not mutate the existing config")
 }

common/configtx/manager.go

@@ -18,13 +18,14 @@ package configtx
 
 import (
 	"fmt"
-	"reflect"
 	"regexp"
 
 	"github.com/hyperledger/fabric/common/configtx/api"
 	"github.com/hyperledger/fabric/common/flogging"
 	cb "github.com/hyperledger/fabric/protos/common"
 	"github.com/hyperledger/fabric/protos/utils"
+
+	"github.com/golang/protobuf/proto"
 )
 
 var logger = flogging.MustGetLogger("common/configtx")
@@ -232,7 +233,8 @@ func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (*configResu
 		return nil, fmt.Errorf("Could not turn configMap back to channelGroup: %s", err)
 	}
 
-	if !reflect.DeepEqual(channelGroup, configEnv.Config.ChannelGroup) {
+	// reflect.Equal will not work here, because it considers nil and empty maps as different
+	if !proto.Equal(channelGroup, configEnv.Config.ChannelGroup) {
 		return nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
 	}
 

orderer/multichain/systemchain.go

@@ -18,7 +18,6 @@ package multichain
 
 import (
 	"fmt"
-	"reflect"
 
 	"github.com/hyperledger/fabric/common/config"
 	"github.com/hyperledger/fabric/common/configtx"
@@ -140,8 +139,9 @@ func (scf *systemChainFilter) authorize(configEnvelope *cb.ConfigEnvelope) (conf
 func (scf *systemChainFilter) inspect(proposedManager, configManager configtxapi.Manager) error {
 	proposedEnv := proposedManager.ConfigEnvelope()
 	actualEnv := configManager.ConfigEnvelope()
-	if !reflect.DeepEqual(proposedEnv.Config, actualEnv.Config) {
-		return fmt.Errorf("The config proposed by the channel creation request did not match the config received with the channel creation request")
+	// reflect.DeepEqual will not work here, because it considers nil and empty maps as unequal
+	if !proto.Equal(proposedEnv.Config, actualEnv.Config) {
+		return fmt.Errorf("config proposed by the channel creation request did not match the config received with the channel creation request")
 	}
 	return nil
 }