[FAB-5552] Fix some TODOs in msgprocessor

The restructuring of the msgprocessor code has left some outstanding
TODOs.  In particular, for conditions where a CONFIG_UPDATE is rewrapped
into a containing message, like a CONFIG or an ORDERER_TRANSACTION, this
wrapping message could end up violating rules for the channel, like the
maximum message size.  This could see the message silently dropped by
the consenter (silent from a client perspective).

This CR adds a second pass of filtering in the message flow which
generates a wrapping message.

Also fixes a small comment bug from a previous CR.

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

Author: Jason Yellick

orderer/common/broadcast/broadcast.go

@@ -23,7 +23,7 @@ type Handler interface {
 	Handle(srv ab.AtomicBroadcast_BroadcastServer) error
 }
 
-// ChannelSupportRegistrar provides a way for the Handler to look up the Support for a chain
+// ChannelSupportRegistrar provides a way for the Handler to look up the Support for a channel
 type ChannelSupportRegistrar interface {
 	// BroadcastChannelSupport returns the message channel header, whether the message is a config update
 	// and the channel resources for a message or an error if the message is not a message which can

orderer/common/msgprocessor/standardchannel.go

@@ -107,7 +107,15 @@ func (s *StandardChannel) ProcessConfigUpdateMsg(env *cb.Envelope) (config *cb.E
 		return nil, 0, err
 	}
 
-	// XXX We should probably run at least the size filter against this new tx one more time
+	// We re-apply the filters here, especially for the size filter, to ensure that the transaction we
+	// just constructed is not too large for our consenter.  It additionally reapplies the signature
+	// check, which although not strictly necessary, is a good sanity check, in case the orderer
+	// has not been configured with the right cert material.  The additional overhead of the signature
+	// check is negligable, as this is the reconfig path and not the normal path.
+	err = s.filters.Apply(config)
+	if err != nil {
+		return nil, 0, err
+	}
 
 	return config, seq, nil
 }

orderer/common/msgprocessor/systemchannel.go

@@ -115,7 +115,15 @@ func (s *SystemChannel) ProcessConfigUpdateMsg(envConfigUpdate *cb.Envelope) (co
 		return nil, 0, err
 	}
 
-	// XXX we should verify that this still passes the size filter
+	// We re-apply the filters here, especially for the size filter, to ensure that the transaction we
+	// just constructed is not too large for our consenter.  It additionally reapplies the signature
+	// check, which although not strictly necessary, is a good sanity check, in case the orderer
+	// has not been configured with the right cert material.  The additional overhead of the signature
+	// check is negligable, as this is the channel creation path and not the normal path.
+	err = s.StandardChannel.filters.Apply(wrappedOrdererTransaction)
+	if err != nil {
+		return nil, 0, err
+	}
 
 	return wrappedOrdererTransaction, s.support.Sequence(), nil
 }

orderer/common/msgprocessor/systemchannel_test.go

@@ -78,7 +78,7 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 	t.Run("Missing header", func(t *testing.T) {
 		mscs := &mockSystemChannelSupport{}
 		ms := &mockSystemChannelFilterSupport{}
-		_, _, err := NewSystemChannel(ms, mscs, nil).ProcessConfigUpdateMsg(&cb.Envelope{})
+		_, _, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{AcceptRule})).ProcessConfigUpdateMsg(&cb.Envelope{})
 		assert.NotNil(t, err)
 		assert.Regexp(t, "no header was set", err.Error())
 	})
@@ -108,7 +108,7 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 		ms := &mockSystemChannelFilterSupport{
 			ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
 		}
-		_, _, err := NewSystemChannel(ms, mscs, nil).ProcessConfigUpdateMsg(&cb.Envelope{
+		_, _, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{AcceptRule})).ProcessConfigUpdateMsg(&cb.Envelope{
 			Payload: utils.MarshalOrPanic(&cb.Payload{
 				Header: &cb.Header{
 					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
@@ -128,7 +128,7 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 		ms := &mockSystemChannelFilterSupport{
 			ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
 		}
-		_, _, err := NewSystemChannel(ms, mscs, nil).ProcessConfigUpdateMsg(&cb.Envelope{
+		_, _, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{AcceptRule})).ProcessConfigUpdateMsg(&cb.Envelope{
 			Payload: utils.MarshalOrPanic(&cb.Payload{
 				Header: &cb.Header{
 					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
@@ -146,7 +146,7 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 		ms := &mockSystemChannelFilterSupport{
 			ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
 		}
-		_, _, err := NewSystemChannel(ms, mscs, nil).ProcessConfigUpdateMsg(&cb.Envelope{
+		_, _, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{AcceptRule})).ProcessConfigUpdateMsg(&cb.Envelope{
 			Payload: utils.MarshalOrPanic(&cb.Payload{
 				Header: &cb.Header{
 					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
@@ -157,6 +157,27 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 		})
 		assert.Regexp(t, "Marshal called with nil", err)
 	})
+	t.Run("BadByFilter", func(t *testing.T) {
+		mscs := &mockSystemChannelSupport{
+			NewChannelConfigVal: &mockconfigtx.Manager{
+				ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
+			},
+		}
+		ms := &mockSystemChannelFilterSupport{
+			SequenceVal:            7,
+			ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
+		}
+		_, _, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{RejectRule})).ProcessConfigUpdateMsg(&cb.Envelope{
+			Payload: utils.MarshalOrPanic(&cb.Payload{
+				Header: &cb.Header{
+					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
+						ChannelId: testChannelID + "different",
+					}),
+				},
+			}),
+		})
+		assert.Equal(t, RejectRule.Apply(nil), err)
+	})
 	t.Run("Good", func(t *testing.T) {
 		mscs := &mockSystemChannelSupport{
 			NewChannelConfigVal: &mockconfigtx.Manager{
@@ -167,7 +188,7 @@ func TestSystemChannelConfigUpdateMsg(t *testing.T) {
 			SequenceVal:            7,
 			ProposeConfigUpdateVal: &cb.ConfigEnvelope{},
 		}
-		config, cs, err := NewSystemChannel(ms, mscs, nil).ProcessConfigUpdateMsg(&cb.Envelope{
+		config, cs, err := NewSystemChannel(ms, mscs, NewRuleSet([]Rule{AcceptRule})).ProcessConfigUpdateMsg(&cb.Envelope{
 			Payload: utils.MarshalOrPanic(&cb.Payload{
 				Header: &cb.Header{
 					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{