network/records: fix panic due to insufficient subnets length (#1804)

* network/records: fix panic due to insufficient subnets length

* add tests

* Revert "add tests"

This reverts commit 832cc71.

* Revert "network/records: fix panic due to insufficient subnets length"

This reverts commit 9a1496d.

* fail metadata encoding/decoding if subnets length is invalid

* add tests

* fix some cases in TestHandshakeTestData

* fix TestHandshakeTestData

* fix remaining unit tests

* add recover to acceptConnection

* remove redundant comment

network/peers/connections/conn_handler.go

@@ -83,7 +83,13 @@ func (ch *connHandler) Handle(logger *zap.Logger) *libp2pnetwork.NotifyBundle {
 	}
 
 	var ignoredConnection = errors.New("ignored connection")
-	acceptConnection := func(logger *zap.Logger, net libp2pnetwork.Network, conn libp2pnetwork.Conn) error {
+	acceptConnection := func(logger *zap.Logger, net libp2pnetwork.Network, conn libp2pnetwork.Conn) (err error) {
+		defer func() {
+			if r := recover(); r != nil {
+				err = errors.Errorf("panic: %v", r)
+			}
+		}()
+
 		pid := conn.RemotePeer()
 
 		if !beginHandshake(pid) {
@@ -144,8 +150,7 @@ func (ch *connHandler) Handle(logger *zap.Logger) *libp2pnetwork.NotifyBundle {
 		// Connection is outbound -> Initiate handshake.
 		logger.Debug("initiating handshake")
 		ch.peerInfos.SetState(pid, peers.StateConnecting)
-		err := ch.handshaker.Handshake(logger, conn)
-		if err != nil {
+		if err := ch.handshaker.Handshake(logger, conn); err != nil {
 			return errors.Wrap(err, "could not handshake")
 		}
 		return nil

network/peers/connections/helpers_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/stretchr/testify/require"
 
+	"github.com/ssvlabs/ssv/network/commons"
 	"github.com/ssvlabs/ssv/network/peers"
 	"github.com/ssvlabs/ssv/network/peers/connections/mock"
 	"github.com/ssvlabs/ssv/network/records"
@@ -48,7 +49,7 @@ func getTestingData(t *testing.T) TestData {
 			NodeVersion:   "some-node-version",
 			ExecutionNode: "some-execution-node",
 			ConsensusNode: "some-consensus-node",
-			Subnets:       "some-subnets",
+			Subnets:       records.AllSubnets,
 		},
 	}
 
@@ -59,7 +60,7 @@ func getTestingData(t *testing.T) TestData {
 				NodeVersion:   "test-node-version",
 				ExecutionNode: "test-execution-node",
 				ConsensusNode: "test-consensus-node",
-				Subnets:       "test-subnets",
+				Subnets:       records.AllSubnets,
 			},
 		},
 		MockSelfSealed: []byte("something"),
@@ -92,6 +93,7 @@ func getTestingData(t *testing.T) TestData {
 		ctx:                context.Background(),
 		nodeInfos:          nii,
 		peerInfos:          ns,
+		subnetsIdx:         peers.NewSubnetsIndex(commons.Subnets()),
 		ids:                ids,
 		net:                net,
 		streams:            sc,

network/records/metadata.go

@@ -2,8 +2,13 @@ package records
 
 import (
 	"encoding/json"
+	"fmt"
+
+	"github.com/ssvlabs/ssv/network/commons"
 )
 
+const subnetsLength = int(commons.SubnetsCount) / 4 // each char in the string encodes status of 4 subnets
+
 // NodeMetadata holds node's general information
 type NodeMetadata struct {
 	// NodeVersion is the ssv-node version, it is a required field
@@ -18,6 +23,10 @@ type NodeMetadata struct {
 
 // Encode encodes the metadata into bytes
 func (nm *NodeMetadata) Encode() ([]byte, error) {
+	if len(nm.Subnets) != subnetsLength {
+		return nil, fmt.Errorf("invalid subnets length %d", len(nm.Subnets))
+	}
+
 	return json.Marshal(nm)
 }
 
@@ -26,6 +35,9 @@ func (nm *NodeMetadata) Decode(data []byte) error {
 	if err := json.Unmarshal(data, nm); err != nil {
 		return err
 	}
+	if len(nm.Subnets) != subnetsLength {
+		return fmt.Errorf("invalid subnets length %d", len(nm.Subnets))
+	}
 	return nil
 }
 

network/records/metadata_test.go

@@ -0,0 +1,87 @@
+package records
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNodeMetadata_Encode(t *testing.T) {
+	tests := []struct {
+		name      string
+		subnets   string
+		errString string
+	}{
+		{
+			name:      "Subnets too short",
+			subnets:   strings.Repeat("0", 31),
+			errString: "invalid subnets length 31",
+		},
+		{
+			name:      "Subnets too long",
+			subnets:   strings.Repeat("0", 33),
+			errString: "invalid subnets length 33",
+		},
+		{
+			name:      "Subnets exact and valid",
+			subnets:   strings.Repeat("0", 32),
+			errString: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			nodeMeta := NodeMetadata{
+				NodeVersion:   "1.0.0",
+				ExecutionNode: "geth/v1.10.8",
+				ConsensusNode: "lighthouse/v1.5.0",
+				Subnets:       tt.subnets,
+			}
+
+			_, err := nodeMeta.Encode()
+			if tt.errString != "" {
+				require.EqualError(t, err, tt.errString)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestNodeMetadata_Decode(t *testing.T) {
+	tests := []struct {
+		name      string
+		encoded   string
+		errString string
+	}{
+		{
+			name:      "Subnets too short",
+			encoded:   `{"NodeVersion":"1.0.0","ExecutionNode":"geth/v1.10.8","ConsensusNode":"lighthouse/v1.5.0","Subnets":"1234123412341234123412341234123"}`,
+			errString: "invalid subnets length 31",
+		},
+		{
+			name:      "Subnets too long",
+			encoded:   `{"NodeVersion":"1.0.0","ExecutionNode":"geth/v1.10.8","ConsensusNode":"lighthouse/v1.5.0","Subnets":"123412341234123412341234123412341"}`,
+			errString: "invalid subnets length 33",
+		},
+		{
+			name:      "Subnets exact and valid",
+			encoded:   `{"NodeVersion":"1.0.0","ExecutionNode":"geth/v1.10.8","ConsensusNode":"lighthouse/v1.5.0","Subnets":"12341234123412341234123412341234"}`,
+			errString: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			nodeMeta := NodeMetadata{}
+
+			err := nodeMeta.Decode([]byte(tt.encoded))
+			if tt.errString != "" {
+				require.EqualError(t, err, tt.errString)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

network/records/node_info_test.go

@@ -18,6 +18,7 @@ func TestNodeInfo_Seal_Consume(t *testing.T) {
 			NodeVersion:   "v0.1.12",
 			ExecutionNode: "geth/x",
 			ConsensusNode: "prysm/x",
+			Subnets:       AllSubnets,
 		},
 	}
 
@@ -31,14 +32,15 @@ func TestNodeInfo_Seal_Consume(t *testing.T) {
 }
 
 func TestNodeInfo_Marshal_Unmarshal(t *testing.T) {
-	oldSerializedData := []byte(`{"Entries":["", "testnet", "{\"NodeVersion\":\"v0.1.12\",\"ExecutionNode\":\"geth/x\",\"ConsensusNode\":\"prysm/x\",\"OperatorsID\":\"xxx\"}"]}`)
+	oldSerializedData := []byte(`{"Entries":["", "testnet", "{\"NodeVersion\":\"v0.1.12\",\"ExecutionNode\":\"geth/x\",\"ConsensusNode\":\"prysm/x\",\"Subnets\":\"ffffffffffffffffffffffffffffffff\"}"]}`)
 
 	currentSerializedData := &NodeInfo{
 		NetworkID: "testnet",
 		Metadata: &NodeMetadata{
 			NodeVersion:   "v0.1.12",
 			ExecutionNode: "geth/x",
 			ConsensusNode: "prysm/x",
+			Subnets:       AllSubnets,
 		},
 	}