p2p/discover/v5wire: use Whoareyou.ChallengeData instead of storing encoded packet (#31547)

This changes the challenge resend logic again to use the existing
`ChallengeData` field of `v5wire.Whoareyou` instead of storing a second
copy of the packet in `Whoareyou.Encoded`. It's more correct this way
since `ChallengeData` is supposed to be the data that is used by the ID
verification procedure.

Also adapts the cross-client test to verify this behavior.

Follow-up to #31543

cmd/devp2p/internal/v5test/discv5tests.go

@@ -52,7 +52,7 @@ func (s *Suite) AllTests() []utesting.Test {
 		{Name: "Ping", Fn: s.TestPing},
 		{Name: "PingLargeRequestID", Fn: s.TestPingLargeRequestID},
 		{Name: "PingMultiIP", Fn: s.TestPingMultiIP},
-		{Name: "PingHandshakeInterrupted", Fn: s.TestPingHandshakeInterrupted},
+		{Name: "HandshakeResend", Fn: s.TestHandshakeResend},
 		{Name: "TalkRequest", Fn: s.TestTalkRequest},
 		{Name: "FindnodeZeroDistance", Fn: s.TestFindnodeZeroDistance},
 		{Name: "FindnodeResults", Fn: s.TestFindnodeResults},
@@ -158,22 +158,20 @@ the attempt from a different IP.`)
 	}
 }
 
-// TestPingHandshakeInterrupted starts a handshake, but doesn't finish it and sends a second ordinary message
+// TestHandshakeResend starts a handshake, but doesn't finish it and sends a second ordinary message
-// packet instead of a handshake message packet. The remote node should respond with
+// packet instead of a handshake message packet. The remote node should repeat the previous WHOAREYOU
-// another WHOAREYOU challenge for the second packet.
+// challenge for the first PING.
-func (s *Suite) TestPingHandshakeInterrupted(t *utesting.T) {
+func (s *Suite) TestHandshakeResend(t *utesting.T) {
-	t.Log(`TestPingHandshakeInterrupted starts a handshake, but doesn't finish it and sends a second ordinary message
-packet instead of a handshake message packet. The remote node should respond with
-another WHOAREYOU challenge for the second packet.`)
-
 	conn, l1 := s.listen1(t)
 	defer conn.close()
 
 	// First PING triggers challenge.
 	ping := &v5wire.Ping{ReqID: conn.nextReqID()}
 	conn.write(l1, ping, nil)
+	var challenge1 *v5wire.Whoareyou
 	switch resp := conn.read(l1).(type) {
 	case *v5wire.Whoareyou:
+		challenge1 = resp
 		t.Logf("got WHOAREYOU for PING")
 	default:
 		t.Fatal("expected WHOAREYOU, got", resp)
@@ -181,9 +179,16 @@ another WHOAREYOU challenge for the second packet.`)
 
 	// Send second PING.
 	ping2 := &v5wire.Ping{ReqID: conn.nextReqID()}
-	switch resp := conn.reqresp(l1, ping2).(type) {
+	conn.write(l1, ping2, nil)
-	case *v5wire.Pong:
+	switch resp := conn.read(l1).(type) {
-		checkPong(t, resp, ping2, l1)
+	case *v5wire.Whoareyou:
+		if resp.Nonce != challenge1.Nonce {
+			t.Fatalf("wrong nonce %x in WHOAREYOU (want %x)", resp.Nonce[:], challenge1.Nonce[:])
+		}
+		if !bytes.Equal(resp.ChallengeData, challenge1.ChallengeData) {
+			t.Fatalf("wrong ChallengeData in resent WHOAREYOU (want %x)", resp.ChallengeData, challenge1.ChallengeData)
+		}
+		resp.Node = conn.remote
 	default:
 		t.Fatal("expected WHOAREYOU, got", resp)
 	}

p2p/discover/v5_udp_test.go

@@ -856,10 +856,10 @@ type testCodecFrame struct {
 }
 
 func (c *testCodec) Encode(toID enode.ID, addr string, p v5wire.Packet, _ *v5wire.Whoareyou) ([]byte, v5wire.Nonce, error) {
-	// To match the behavior of v5wire.Codec, we return the cached encoding of
+	if wp, ok := p.(*v5wire.Whoareyou); ok && len(wp.ChallengeData) > 0 {
-	// WHOAREYOU challenges.
+		// To match the behavior of v5wire.Codec, we return the cached encoding of
-	if wp, ok := p.(*v5wire.Whoareyou); ok && len(wp.Encoded) > 0 {
+		// WHOAREYOU challenges.
-		return wp.Encoded, wp.Nonce, nil
+		return wp.ChallengeData, wp.Nonce, nil
 	}
 
 	c.ctr++
@@ -874,7 +874,7 @@ func (c *testCodec) Encode(toID enode.ID, addr string, p v5wire.Packet, _ *v5wir
 	// Store recently sent challenges.
 	if w, ok := p.(*v5wire.Whoareyou); ok {
 		w.Nonce = authTag
-		w.Encoded = frame
+		w.ChallengeData = frame
 		if c.sentChallenges == nil {
 			c.sentChallenges = make(map[enode.ID]*v5wire.Whoareyou)
 		}
@@ -911,6 +911,7 @@ func (c *testCodec) decodeFrame(input []byte) (frame testCodecFrame, p v5wire.Pa
 	case v5wire.WhoareyouPacket:
 		dec := new(v5wire.Whoareyou)
 		err = rlp.DecodeBytes(frame.Packet, &dec)
+		dec.ChallengeData = bytes.Clone(input)
 		p = dec
 	default:
 		p, err = v5wire.DecodeMessage(frame.Ptype, frame.Packet)

p2p/discover/v5wire/encoding.go

@@ -190,10 +190,16 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar
 	)
 	switch {
 	case packet.Kind() == WhoareyouPacket:
-		// just send the WHOAREYOU packet raw again, rather than the re-encoded challenge data
 		w := packet.(*Whoareyou)
-		if len(w.Encoded) > 0 {
+		if len(w.ChallengeData) > 0 {
-			return w.Encoded, w.Nonce, nil
+			// This WHOAREYOU packet was encoded before, so it's a resend.
+			// The unmasked packet content is stored in w.ChallengeData.
+			// Just apply the masking again to finish encoding.
+			c.buf.Reset()
+			c.buf.Write(w.ChallengeData)
+			copy(head.IV[:], w.ChallengeData)
+			enc := applyMasking(id, head.IV, c.buf.Bytes())
+			return enc, w.Nonce, nil
 		}
 		head, err = c.encodeWhoareyou(id, packet.(*Whoareyou))
 	case challenge != nil:
@@ -228,7 +234,6 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar
 		if err != nil {
 			return nil, Nonce{}, err
 		}
-		challenge.Encoded = bytes.Clone(enc)
 		c.sc.storeSentHandshake(id, addr, challenge)
 		return enc, head.Nonce, err
 	}
@@ -246,14 +251,10 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar
 
 // EncodeRaw encodes a packet with the given header.
 func (c *Codec) EncodeRaw(id enode.ID, head Header, msgdata []byte) ([]byte, error) {
+	// header
 	c.writeHeaders(&head)
-
+	applyMasking(id, head.IV, c.buf.Bytes())
-	// Apply masking.
+	// message data
-	masked := c.buf.Bytes()[sizeofMaskingIV:]
-	mask := head.mask(id)
-	mask.XORKeyStream(masked[:], masked[:])
-
-	// Write message data.
 	c.buf.Write(msgdata)
 	return c.buf.Bytes(), nil
 }
@@ -463,7 +464,7 @@ func (c *Codec) Decode(inputData []byte, addr string) (src enode.ID, n *enode.No
 	// Unmask the static header.
 	var head Header
 	copy(head.IV[:], input[:sizeofMaskingIV])
-	mask := head.mask(c.localnode.ID())
+	mask := createMask(c.localnode.ID(), head.IV)
 	staticHeader := input[sizeofMaskingIV:sizeofStaticPacketData]
 	mask.XORKeyStream(staticHeader, staticHeader)
 
@@ -679,10 +680,17 @@ func (h *StaticHeader) checkValid(packetLen int, protocolID [6]byte) error {
 }
 
 // mask returns a cipher for 'masking' / 'unmasking' packet headers.
-func (h *Header) mask(destID enode.ID) cipher.Stream {
+func createMask(destID enode.ID, iv [16]byte) cipher.Stream {
 	block, err := aes.NewCipher(destID[:16])
 	if err != nil {
 		panic("can't create cipher")
 	}
-	return cipher.NewCTR(block, h.IV[:])
+	return cipher.NewCTR(block, iv[:])
+}
+
+func applyMasking(destID enode.ID, iv [16]byte, packet []byte) []byte {
+	masked := packet[sizeofMaskingIV:]
+	mask := createMask(destID, iv)
+	mask.XORKeyStream(masked[:], masked[:])
+	return packet
 }

p2p/discover/v5wire/encoding_test.go

@@ -269,6 +269,35 @@ func TestHandshake_BadHandshakeAttack(t *testing.T) {
 	net.nodeB.expectDecodeErr(t, errUnexpectedHandshake, findnode)
 }
 
+func TestEncodeWhoareyouResend(t *testing.T) {
+	t.Parallel()
+	net := newHandshakeTest()
+	defer net.close()
+
+	// A -> B   WHOAREYOU
+	challenge := &Whoareyou{
+		Nonce:     Nonce{1, 2, 3, 4},
+		IDNonce:   testIDnonce,
+		RecordSeq: 0,
+	}
+	enc, _ := net.nodeA.encode(t, net.nodeB, challenge)
+	net.nodeB.expectDecode(t, WhoareyouPacket, enc)
+	whoareyou1 := bytes.Clone(enc)
+
+	if len(challenge.ChallengeData) == 0 {
+		t.Fatal("ChallengeData not assigned by encode")
+	}
+
+	// A -> B   WHOAREYOU
+	// Send the same challenge again. This should produce exactly
+	// the same bytes as the first send.
+	enc, _ = net.nodeA.encode(t, net.nodeB, challenge)
+	whoareyou2 := bytes.Clone(enc)
+	if !bytes.Equal(whoareyou2, whoareyou1) {
+		t.Fatal("re-encoded challenge not equal to first")
+	}
+}
+
 // This test checks some malformed packets.
 func TestDecodeErrorsV5(t *testing.T) {
 	t.Parallel()

p2p/discover/v5wire/msg.go

@@ -63,19 +63,20 @@ type (
 
 	// WHOAREYOU contains the handshake challenge.
 	Whoareyou struct {
-		ChallengeData []byte   // Encoded challenge
+		Nonce     Nonce    // Nonce of request packet
-		Nonce         Nonce    // Nonce of request packet
+		IDNonce   [16]byte // Identity proof data
-		IDNonce       [16]byte // Identity proof data
+		RecordSeq uint64   // ENR sequence number of recipient
-		RecordSeq     uint64   // ENR sequence number of recipient
 
 		// Node is the locally known node record of recipient.
 		// This must be set by the caller of Encode.
-		Node *enode.Node
+		Node *enode.Node `rlp:"-"`
+
+		// ChallengeData stores the unmasked encoding of the whole packet. This is the
+		// input data for verification. It is assigned by both Encode and Decode
+		// operations.
+		ChallengeData []byte `rlp:"-"`
 
 		sent mclock.AbsTime // for handshake GC.
-
-		// Encoded is packet raw data for sending out, but should not be include in the RLP encoding.
-		Encoded []byte `rlp:"-"`
 	}
 
 	// PING is sent during liveness checks.