Merge "[FAB-3450] Prevent panic on msg signing"

gossip/comm/comm_impl.go

@@ -435,7 +435,10 @@ func (c *commImpl) authenticateRemotePeer(stream stream) (*proto.ConnectionInfo,
 		return nil, errors.New("No TLS certificate")
 	}
 
-	cMsg = c.createConnectionMsg(c.PKIID, c.selfCertHash, c.peerIdentity, signer)
+	cMsg, err = c.createConnectionMsg(c.PKIID, c.selfCertHash, c.peerIdentity, signer)
+	if err != nil {
+		return nil, err
+	}
 
 	c.logger.Debug("Sending", cMsg, "to", remoteAddress)
 	stream.Send(cMsg.Envelope)
@@ -582,7 +585,7 @@ func readWithTimeout(stream interface{}, timeout time.Duration, address string)
 	}
 }
 
-func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer proto.Signer) *proto.SignedGossipMessage {
+func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer proto.Signer) (*proto.SignedGossipMessage, error) {
 	m := &proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: 0,
@@ -597,8 +600,8 @@ func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte,
 	sMsg := &proto.SignedGossipMessage{
 		GossipMessage: m,
 	}
-	sMsg.Sign(signer)
-	return sMsg
+	_, err := sMsg.Sign(signer)
+	return sMsg, err
 }
 
 type stream interface {

gossip/comm/comm_test.go

@@ -156,7 +156,7 @@ func handshaker(endpoint string, comm Comm, t *testing.T, connMutator msgMutator
 
 	pkiID := common.PKIidType(endpoint)
 	assert.NoError(t, err, "%v", err)
-	msg := c.createConnectionMsg(pkiID, clientCertHash, []byte(endpoint), func(msg []byte) ([]byte, error) {
+	msg, _ := c.createConnectionMsg(pkiID, clientCertHash, []byte(endpoint), func(msg []byte) ([]byte, error) {
 		mac := hmac.New(sha256.New, hmacKey)
 		mac.Write(msg)
 		return mac.Sum(nil), nil
@@ -423,7 +423,7 @@ func TestCloseConn(t *testing.T) {
 	assert.NoError(t, err, "%v", err)
 	c := &commImpl{}
 	tlsCertHash := certHashFromRawCert(tlsCfg.Certificates[0].Certificate[0])
-	connMsg := c.createConnectionMsg(common.PKIidType("pkiID"), tlsCertHash, api.PeerIdentityType("pkiID"), func(msg []byte) ([]byte, error) {
+	connMsg, _ := c.createConnectionMsg(common.PKIidType("pkiID"), tlsCertHash, api.PeerIdentityType("pkiID"), func(msg []byte) ([]byte, error) {
 		mac := hmac.New(sha256.New, hmacKey)
 		mac.Write(msg)
 		return mac.Sum(nil), nil
@@ -711,13 +711,14 @@ func TestPresumedDead(t *testing.T) {
 }
 
 func createGossipMsg() *proto.SignedGossipMessage {
-	return (&proto.GossipMessage{
+	msg, _ := (&proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: uint64(rand.Int()),
 		Content: &proto.GossipMessage_DataMsg{
 			DataMsg: &proto.DataMessage{},
 		},
 	}).NoopSign()
+	return msg
 }
 
 func remotePeer(port int) *RemotePeer {

gossip/comm/mock/mock_comm.go

@@ -85,10 +85,11 @@ func NewCommMock(id string, members map[string]*socketMock) comm.Comm {
 
 // Respond sends a GossipMessage to the origin from which this ReceivedMessage was sent from
 func (packet *packetMock) Respond(msg *proto.GossipMessage) {
+	sMsg, _ := msg.NoopSign()
 	packet.src.socket <- &packetMock{
 		src: packet.dst,
 		dst: packet.src,
-		msg: msg.NoopSign(),
+		msg: sMsg,
 	}
 }
 

gossip/comm/mock/mock_comm_test.go

@@ -44,12 +44,13 @@ func TestMockComm(t *testing.T) {
 	comm2 := NewCommMock(second.endpoint, members)
 	defer comm2.Stop()
 
-	comm2.Send((&proto.GossipMessage{
+	sMsg, _ := (&proto.GossipMessage{
 		Content: &proto.GossipMessage_StateRequest{&proto.RemoteStateRequest{
 			StartSeqNum: 1,
 			EndSeqNum:   3,
 		}},
-	}).NoopSign(), &comm.RemotePeer{"first", common.PKIidType("first")})
+	}).NoopSign()
+	comm2.Send(sMsg, &comm.RemotePeer{"first", common.PKIidType("first")})
 
 	msg := <-msgCh
 
@@ -73,15 +74,16 @@ func TestMockComm_PingPong(t *testing.T) {
 	rcvChA := peerA.Accept(all)
 	rcvChB := peerB.Accept(all)
 
-	peerA.Send((&proto.GossipMessage{
+	sMsg, _ := (&proto.GossipMessage{
 		Content: &proto.GossipMessage_DataMsg{
 			&proto.DataMessage{
 				&proto.Payload{
 					SeqNum: 1,
 					Data:   []byte("Ping"),
 				},
 			}},
-	}).NoopSign(), &comm.RemotePeer{"peerB", common.PKIidType("peerB")})
+	}).NoopSign()
+	peerA.Send(sMsg, &comm.RemotePeer{"peerB", common.PKIidType("peerB")})
 
 	msg := <-rcvChB
 	dataMsg := msg.GetGossipMessage().GetDataMsg()

gossip/comm/msg.go

@@ -38,7 +38,12 @@ func (m *ReceivedMessageImpl) GetSourceEnvelope() *proto.Envelope {
 
 // Respond sends a msg to the source that sent the ReceivedMessageImpl
 func (m *ReceivedMessageImpl) Respond(msg *proto.GossipMessage) {
-	m.conn.send(msg.NoopSign(), func(e error) {})
+	sMsg, err := msg.NoopSign()
+	if err != nil {
+		m.conn.logger.Error("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	m.conn.send(sMsg, func(e error) {})
 }
 
 // GetGossipMessage returns the inner GossipMessage

gossip/discovery/discovery_impl.go

@@ -172,9 +172,17 @@ func (d *gossipDiscoveryImpl) Connect(member NetworkMember, id identifier) {
 				Endpoint:         member.Endpoint,
 				PKIid:            id.ID,
 			}
-			req := d.createMembershipRequest(id.SelfOrg).NoopSign()
+			req, err := d.createMembershipRequest(id.SelfOrg).NoopSign()
+			if err != nil {
+				d.logger.Warning("Failed creating SignedGossipMessage:", err)
+				continue
+			}
 			req.Nonce = util.RandomUInt64()
-			req.NoopSign()
+			req, err = req.NoopSign()
+			if err != nil {
+				d.logger.Warning("Failed adding NONCE to SignedGossipMessage", err)
+				continue
+			}
 			go d.sendUntilAcked(peer, req)
 			return
 		}
@@ -221,19 +229,16 @@ func (d *gossipDiscoveryImpl) sendUntilAcked(peer *NetworkMember, message *proto
 	}
 }
 
-func (d *gossipDiscoveryImpl) somePeerIsKnown() bool {
-	d.lock.RLock()
-	defer d.lock.RUnlock()
-	return len(d.aliveLastTS) != 0
-}
-
 func (d *gossipDiscoveryImpl) InitiateSync(peerNum int) {
 	if d.toDie() {
 		return
 	}
 	var peers2SendTo []*NetworkMember
-	memReq := d.createMembershipRequest(true).NoopSign()
-
+	memReq, err := d.createMembershipRequest(true).NoopSign()
+	if err != nil {
+		d.logger.Warning("Failed creating SignedGossipMessage:", err)
+		return
+	}
 	d.lock.RLock()
 
 	n := d.aliveMembership.Size()
@@ -404,7 +409,11 @@ func (d *gossipDiscoveryImpl) sendMemResponse(targetMember *proto.Member, intern
 		InternalEndpoint: internalEndpoint,
 	}
 
-	memResp := d.createMembershipResponse(targetPeer)
+	aliveMsg := d.createAliveMessage(true)
+	if aliveMsg == nil {
+		return
+	}
+	memResp := d.createMembershipResponse(aliveMsg, targetPeer)
 	if memResp == nil {
 		errMsg := `Got a membership request from a peer that shouldn't have sent one: %v, closing connection to the peer as a result.`
 		d.logger.Warningf(errMsg, targetMember)
@@ -414,18 +423,22 @@ func (d *gossipDiscoveryImpl) sendMemResponse(targetMember *proto.Member, intern
 
 	defer d.logger.Debug("Exiting, replying with", memResp)
 
-	d.comm.SendToPeer(targetPeer, (&proto.GossipMessage{
+	msg, err := (&proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: nonce,
 		Content: &proto.GossipMessage_MemRes{
 			MemRes: memResp,
 		},
-	}).NoopSign())
+	}).NoopSign()
+	if err != nil {
+		d.logger.Warning("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	d.comm.SendToPeer(targetPeer, msg)
 }
 
-func (d *gossipDiscoveryImpl) createMembershipResponse(targetMember *NetworkMember) *proto.MembershipResponse {
+func (d *gossipDiscoveryImpl) createMembershipResponse(aliveMsg *proto.SignedGossipMessage, targetMember *NetworkMember) *proto.MembershipResponse {
 	shouldBeDisclosed, omitConcealedFields := d.disclosurePolicy(targetMember)
-	aliveMsg := d.createAliveMessage(true)
 
 	if !shouldBeDisclosed(aliveMsg) {
 		return nil
@@ -594,24 +607,29 @@ func (d *gossipDiscoveryImpl) periodicalReconnectToDead() {
 }
 
 func (d *gossipDiscoveryImpl) sendMembershipRequest(member *NetworkMember, includeInternalEndpoint bool) {
-	d.comm.SendToPeer(member, d.createMembershipRequest(includeInternalEndpoint))
+	req, err := d.createMembershipRequest(includeInternalEndpoint).NoopSign()
+	if err != nil {
+		d.logger.Error("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	d.comm.SendToPeer(member, req)
 }
 
-func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) *proto.SignedGossipMessage {
+func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) *proto.GossipMessage {
 	req := &proto.MembershipRequest{
 		SelfInformation: d.createAliveMessage(includeInternalEndpoint).Envelope,
 		// TODO: sending the known peers is not secure because the remote peer might shouldn't know
 		// TODO: about the known peers. I'm deprecating this until a secure mechanism will be implemented.
 		// TODO: See FAB-2570 for tracking this issue.
 		Known: [][]byte{},
 	}
-	return (&proto.GossipMessage{
+	return &proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: uint64(0),
 		Content: &proto.GossipMessage_MemReq{
 			MemReq: req,
 		},
-	}).NoopSign()
+	}
 }
 
 func (d *gossipDiscoveryImpl) copyLastSeen(lastSeenMap map[string]*timestamp) []NetworkMember {
@@ -693,7 +711,11 @@ func (d *gossipDiscoveryImpl) periodicalSendAlive() {
 	for !d.toDie() {
 		d.logger.Debug("Sleeping", getAliveTimeInterval())
 		time.Sleep(getAliveTimeInterval())
-		d.comm.Gossip(d.createAliveMessage(true))
+		msg := d.createAliveMessage(true)
+		if msg == nil {
+			return
+		}
+		d.comm.Gossip(msg)
 	}
 }
 
@@ -726,9 +748,13 @@ func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) *
 		},
 	}
 
+	envp := d.crypt.SignMessage(msg2Gossip, internalEndpoint)
+	if envp == nil {
+		return nil
+	}
 	signedMsg := &proto.SignedGossipMessage{
 		GossipMessage: msg2Gossip,
-		Envelope:      d.crypt.SignMessage(msg2Gossip, internalEndpoint),
+		Envelope:      envp,
 	}
 
 	if !includeInternalEndpoint {