[FAB-1349] Enforce restrictions on chain IDs

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

This changeset introduces a function which rejects chain IDs (i.e.
channel names) that do NOT comply with the following restrictions:

1. Contain only ASCII alphanumerics, dots '.', dashes '-'.
2. Are shorter than 250 characters.
3. Are not "." or "..".

Our hand here is forced by Kafka, since a channel in Fabric maps to a
Kafka topic, and there are restrictions for the allowed topic names [1].

Note that underscores are allowed in Kafka, but topics with a period or
underscore could collide. (Thanks to Luis Sanchez for bringing this to my
attention.) We therefore remove support for underscores altoghether to
keep the checks simpler.

UPDATE: Switched from a filter to a function because filters are not
evaluated during the system chain creation.

[1] https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/common/Topic.scala#L29

Change-Id: I14b95477e485fea27868338e2f33772588b8a770
Signed-off-by: Kostas Christidis <kostas@christidis.io>

common/configtx/manager.go

@@ -19,17 +19,29 @@ package configtx
 import (
 	"fmt"
 	"reflect"
+	"regexp"
 
 	"github.com/hyperledger/fabric/common/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	"errors"
+
 	"github.com/golang/protobuf/proto"
 	logging "github.com/op/go-logging"
 )
 
 var logger = logging.MustGetLogger("common/configtx")
 
+// Constraints for valid chain IDs
+var (
+	allowedChars = "[a-zA-Z0-9.-]+"
+	maxLength    = 249
+	illegalNames = map[string]struct{}{
+		".":  struct{}{},
+		"..": struct{}{},
+	}
+)
+
 // Handler provides a hook which allows other pieces of code to participate in config proposals
 type Handler interface {
 	// BeginConfig called when a config proposal is begun
@@ -91,8 +103,7 @@ func computeChainIDAndSequence(configtx *cb.ConfigurationEnvelope) (string, uint
 
 	for _, signedItem := range configtx.Items {
 		item := &cb.ConfigurationItem{}
-		err := proto.Unmarshal(signedItem.ConfigurationItem, item)
-		if err != nil {
+		if err := proto.Unmarshal(signedItem.ConfigurationItem, item); err != nil {
 			return "", 0, fmt.Errorf("Error unmarshaling signedItem.ConfigurationItem: %s", err)
 		}
 
@@ -115,11 +126,45 @@ func computeChainIDAndSequence(configtx *cb.ConfigurationEnvelope) (string, uint
 				return "", 0, fmt.Errorf("Mismatched chainIDs in envelope %s != %s", chainID, item.Header.ChainID)
 			}
 		}
+
+		if err := validateChainID(chainID); err != nil {
+			return "", 0, err
+		}
 	}
 
 	return chainID, m, nil
 }
 
+// validateChainID makes sure that proposed chain IDs (i.e. channel names)
+// comply with the following restrictions:
+//      1. Contain only ASCII alphanumerics, dots '.', dashes '-'
+//      2. Are shorter than 250 characters.
+//      3. Are not the strings "." or "..".
+//
+// Our hand here is forced by:
+// https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/common/Topic.scala#L29
+func validateChainID(chainID string) error {
+	re, _ := regexp.Compile(allowedChars)
+	// Length
+	if len(chainID) <= 0 {
+		return fmt.Errorf("chain ID illegal, cannot be empty")
+	}
+	if len(chainID) > maxLength {
+		return fmt.Errorf("chain ID illegal, cannot be longer than %d", maxLength)
+	}
+	// Illegal name
+	if _, ok := illegalNames[chainID]; ok {
+		return fmt.Errorf("name '%s' for chain ID is not allowed", chainID)
+	}
+	// Illegal characters
+	matched := re.FindString(chainID)
+	if len(matched) != len(chainID) {
+		return fmt.Errorf("Chain ID '%s' contains illegal characters", chainID)
+	}
+
+	return nil
+}
+
 // NewManagerImpl creates a new Manager unless an error is encountered, each element of the callOnUpdate slice
 // is invoked when a new configuration is committed
 func NewManagerImpl(configtx *cb.ConfigurationEnvelope, initializer Initializer, callOnUpdate []func(Manager)) (Manager, error) {
@@ -218,7 +263,7 @@ func (cm *configurationManager) processConfig(configtx *cb.ConfigurationEnvelope
 		}
 
 		// Ensure the config sequence numbers are correct to prevent replay attacks
-		isModified := false
+		var isModified bool
 
 		if val, ok := cm.configuration[config.Type][config.Key]; ok {
 			// Config was modified if any of the contents changed

common/configtx/manager_test.go

@@ -26,6 +26,7 @@ import (
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	"errors"
+
 	"github.com/golang/protobuf/proto"
 )
 
@@ -118,8 +119,8 @@ func TestCallback(t *testing.T) {
 	}
 }
 
-// TestWrongChainID tests that a configuration update for a different chain ID fails
-func TestWrongChainID(t *testing.T) {
+// TestDifferentChainID tests that a configuration update for a different chain ID fails
+func TestDifferentChainID(t *testing.T) {
 	cm, err := NewManagerImpl(&cb.ConfigurationEnvelope{
 		Items: []*cb.SignedConfigurationItem{makeSignedConfigurationItem("foo", "foo", 0, []byte("foo"), defaultChain)},
 	}, &mockconfigtx.Initializer{PolicyManagerVal: &mockPolicyManager{&mockPolicy{}}, HandlersVal: defaultHandlers()}, nil)

common/configtx/template.go

@@ -163,6 +163,9 @@ func MakeChainCreationTransaction(creationPolicy string, chainID string, signer
 	}
 
 	manager, err := NewManagerImpl(&cb.ConfigurationEnvelope{Items: items}, NewInitializer(), nil)
+	if err != nil {
+		return nil, err
+	}
 
 	newChainTemplate := NewChainCreationTemplate(creationPolicy, manager.ChainConfig().HashingAlgorithm(), composite)
 	signedConfigItems, err := newChainTemplate.Items(chainID)

common/configtx/util_test.go

@@ -0,0 +1,71 @@
+/*
+Copyright IBM Corp. 2017 All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+                 http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package configtx
+
+import (
+	"math/rand"
+	"testing"
+)
+
+// TestValidchainID checks that the constraints on chain IDs are enforced properly
+func TestValidChainID(t *testing.T) {
+	acceptMsg := "Should have accepted valid chain ID"
+	rejectMsg := "Should have rejected invalid chain ID"
+
+	t.Run("ZeroLength", func(t *testing.T) {
+		if err := validateChainID(""); err == nil {
+			t.Fatal(rejectMsg)
+		}
+	})
+
+	t.Run("LongerThanMaxAllowed", func(t *testing.T) {
+		if err := validateChainID(randomAlphaString(maxLength + 1)); err == nil {
+			t.Fatal(rejectMsg)
+		}
+	})
+
+	t.Run("HasIllegalName", func(t *testing.T) {
+		for illegalName := range illegalNames {
+			if err := validateChainID(illegalName); err == nil {
+				t.Fatal(rejectMsg)
+			}
+		}
+	})
+
+	t.Run("ContainsIllegalCharacter", func(t *testing.T) {
+		if err := validateChainID("foo_bar"); err == nil {
+			t.Fatal(rejectMsg)
+		}
+	})
+
+	t.Run("ValidName", func(t *testing.T) {
+		if err := validateChainID("foo.bar"); err != nil {
+			t.Fatal(acceptMsg)
+		}
+	})
+}
+
+// Helper functions
+
+func randomAlphaString(size int) string {
+	letters := []rune("abcdefghijklmnopqrstuvwxyz")
+	output := make([]rune, size)
+	for i := range output {
+		output[i] = letters[rand.Intn(len(letters))]
+	}
+	return string(output)
+}