[FAB-1693] Do not validate modPolicy without mod

https://jira.hyperledger.org/browse/FAB-1693

The previous configuratoin transaction manager verified all signature
policies against signature sets every time regardless of whether they
had been modified.  This caused a problem that if the backing policy
changed, or the certificate expired, etc., an existing policy item might
cause a config transaction to be rejected.  This changeset fixes this
oversight.

Change-Id: Iec2f42d51a9a213650b80a2f735ff2fc8d8d16ac
Signed-off-by: Jason Yellick <jyellick@us.ibm.com>

common/configtx/manager.go

@@ -17,8 +17,8 @@ limitations under the License.
 package configtx
 
 import (
-	"bytes"
 	"fmt"
+	"reflect"
 
 	"github.com/hyperledger/fabric/common/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
@@ -205,45 +205,45 @@ func (cm *configurationManager) processConfig(configtx *cb.ConfigurationEnvelope
 			return nil, fmt.Errorf("Error unmarshaling ConfigurationItem: %s", err)
 		}
 
-		// Get the modification policy for this config item if one was previously specified
-		// or the default if this is a new config item
-		var policy policies.Policy
-		oldItem, ok := cm.configuration[config.Type][config.Key]
-		if ok {
-			policy, _ = cm.pm.GetPolicy(oldItem.ModificationPolicy)
-		} else {
-			policy = defaultModificationPolicy
-		}
-
-		// Get signatures
-		signedData, err := entry.AsSignedData()
-		if err != nil {
-			return nil, err
-		}
-
-		// Ensure the policy is satisfied
-		if err = policy.Evaluate(signedData); err != nil {
-			return nil, err
-		}
-
 		// Ensure the config sequence numbers are correct to prevent replay attacks
 		isModified := false
 
 		if val, ok := cm.configuration[config.Type][config.Key]; ok {
-			// Config was modified if the LastModified or the Data contents changed
-			isModified = (val.LastModified != config.LastModified) || !bytes.Equal(config.Value, val.Value)
+			// Config was modified if any of the contents changed
+			isModified = !reflect.DeepEqual(val, config)
 		} else {
 			if config.LastModified != seq {
 				return nil, fmt.Errorf("Key %v for type %v was new, but had an older Sequence %d set", config.Key, config.Type, config.LastModified)
 			}
 			isModified = true
 		}
 
-		// If a config item was modified, its LastModified must be set correctly
+		// If a config item was modified, its LastModified must be set correctly, and it must satisfy the modification policy
 		if isModified {
 			if config.LastModified != seq {
 				return nil, fmt.Errorf("Key %v for type %v was modified, but its LastModified %d does not equal current configtx Sequence %d", config.Key, config.Type, config.LastModified, seq)
 			}
+
+			// Get the modification policy for this config item if one was previously specified
+			// or the default if this is a new config item
+			var policy policies.Policy
+			oldItem, ok := cm.configuration[config.Type][config.Key]
+			if ok {
+				policy, _ = cm.pm.GetPolicy(oldItem.ModificationPolicy)
+			} else {
+				policy = defaultModificationPolicy
+			}
+
+			// Get signatures
+			signedData, err := entry.AsSignedData()
+			if err != nil {
+				return nil, err
+			}
+
+			// Ensure the policy is satisfied
+			if err = policy.Evaluate(signedData); err != nil {
+				return nil, err
+			}
 		}
 
 		// Ensure the type handler agrees the config is well formed

common/configtx/manager_test.go

@@ -23,8 +23,8 @@ import (
 	"github.com/hyperledger/fabric/common/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
 
-	"github.com/golang/protobuf/proto"
 	"errors"
+	"github.com/golang/protobuf/proto"
 )
 
 var defaultChain = "DefaultChainID"
@@ -40,12 +40,19 @@ func defaultHandlers() map[cb.ConfigurationItem_ConfigurationType]Handler {
 // mockPolicy always returns the error set as policyResult
 type mockPolicy struct {
 	policyResult error
+	// validReplies is the number of times to successfully validate before returning policyResult
+	// it is decremented at each invocation
+	validReplies int
 }
 
 func (mp *mockPolicy) Evaluate(signedData []*cb.SignedData) error {
 	if mp == nil {
 		return errors.New("Invoked nil policy")
 	}
+	if mp.validReplies > 0 {
+		return nil
+	}
+	mp.validReplies--
 	return mp.policyResult
 }
 
@@ -328,7 +335,7 @@ func TestSilentConfigModification(t *testing.T) {
 func TestInvalidInitialConfigByPolicy(t *testing.T) {
 	_, err := NewConfigurationManager(&cb.ConfigurationEnvelope{
 		Items: []*cb.SignedConfigurationItem{makeSignedConfigurationItem("foo", "foo", 0, []byte("foo"), defaultChain)},
-	}, &mockPolicyManager{&mockPolicy{fmt.Errorf("err")}}, defaultHandlers())
+	}, &mockPolicyManager{&mockPolicy{policyResult: fmt.Errorf("err")}}, defaultHandlers())
 	// mockPolicyManager will return non-validating defualt policy
 
 	if err == nil {
@@ -348,7 +355,7 @@ func TestConfigChangeViolatesPolicy(t *testing.T) {
 		t.Fatalf("Error constructing configuration manager: %s", err)
 	}
 	// Set the mock policy to error
-	mpm.policy = &mockPolicy{fmt.Errorf("err")}
+	mpm.policy = &mockPolicy{policyResult: fmt.Errorf("err")}
 
 	newConfig := &cb.ConfigurationEnvelope{
 		Items: []*cb.SignedConfigurationItem{makeSignedConfigurationItem("foo", "foo", 1, []byte("foo"), defaultChain)},
@@ -365,6 +372,41 @@ func TestConfigChangeViolatesPolicy(t *testing.T) {
 	}
 }
 
+// TestUnchangedConfigViolatesPolicy checks to make sure that existing config items are not revalidated against their modification policies
+// as the policy may have changed, certs revoked, etc. since the config was adopted.
+func TestUnchangedConfigViolatesPolicy(t *testing.T) {
+	mpm := &mockPolicyManager{}
+	cm, err := NewConfigurationManager(&cb.ConfigurationEnvelope{
+		Items: []*cb.SignedConfigurationItem{makeSignedConfigurationItem("foo", "foo", 0, []byte("foo"), defaultChain)},
+	}, mpm, defaultHandlers())
+
+	if err != nil {
+		t.Fatalf("Error constructing configuration manager: %s", err)
+	}
+	// Set the mock policy to error
+	mpm.policy = &mockPolicy{
+		policyResult: fmt.Errorf("err"),
+		validReplies: 1,
+	}
+
+	newConfig := &cb.ConfigurationEnvelope{
+		Items: []*cb.SignedConfigurationItem{
+			makeSignedConfigurationItem("foo", "foo", 0, []byte("foo"), defaultChain),
+			makeSignedConfigurationItem("bar", "bar", 1, []byte("foo"), defaultChain),
+		},
+	}
+
+	err = cm.Validate(newConfig)
+	if err != nil {
+		t.Errorf("Should not have errored validating config, but got %s", err)
+	}
+
+	err = cm.Apply(newConfig)
+	if err != nil {
+		t.Errorf("Should not have errored applying config, but got %s", err)
+	}
+}
+
 type failHandler struct{}
 
 func (fh failHandler) BeginConfig()    {}