Verify SignedIP.Timestamp from PeerList messages (#2587)

Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

network/network.go

@@ -625,7 +625,8 @@ func (n *network) track(ip *ips.ClaimedIPPort) error {
 		},
 		Signature: ip.Signature,
 	}
-	if err := signedIP.Verify(ip.Cert); err != nil {
+	maxTimestamp := n.peerConfig.Clock.Time().Add(n.peerConfig.MaxClockDifference)
+	if err := signedIP.Verify(ip.Cert, maxTimestamp); err != nil {
 		return err
 	}
 

network/peer/ip.go

@@ -6,13 +6,21 @@ package peer
 import (
 	"crypto"
 	"crypto/rand"
+	"errors"
+	"fmt"
+	"time"
 
 	"github.com/ava-labs/avalanchego/staking"
 	"github.com/ava-labs/avalanchego/utils/hashing"
 	"github.com/ava-labs/avalanchego/utils/ips"
 	"github.com/ava-labs/avalanchego/utils/wrappers"
 )
 
+var (
+	errTimestampTooFarInFuture = errors.New("timestamp too far in the future")
+	errInvalidSignature        = errors.New("invalid signature")
+)
+
 // UnsignedIP is used for a validator to claim an IP. The [Timestamp] is used to
 // ensure that the most updated IP claim is tracked by peers for a given
 // validator.
@@ -49,10 +57,24 @@ type SignedIP struct {
 	Signature []byte
 }
 
-func (ip *SignedIP) Verify(cert *staking.Certificate) error {
-	return staking.CheckSignature(
+// Returns nil if:
+// * [ip.Timestamp] is not after [maxTimestamp].
+// * [ip.Signature] is a valid signature over [ip.UnsignedIP] from [cert].
+func (ip *SignedIP) Verify(
+	cert *staking.Certificate,
+	maxTimestamp time.Time,
+) error {
+	maxUnixTimestamp := uint64(maxTimestamp.Unix())
+	if ip.Timestamp > maxUnixTimestamp {
+		return fmt.Errorf("%w: timestamp %d > maxTimestamp %d", errTimestampTooFarInFuture, ip.Timestamp, maxUnixTimestamp)
+	}
+
+	if err := staking.CheckSignature(
 		cert,
 		ip.UnsignedIP.bytes(),
 		ip.Signature,
-	)
+	); err != nil {
+		return fmt.Errorf("%w: %w", errInvalidSignature, err)
+	}
+	return nil
 }

network/peer/ip_test.go

@@ -0,0 +1,108 @@
+// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package peer
+
+import (
+	"crypto"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ava-labs/avalanchego/staking"
+	"github.com/ava-labs/avalanchego/utils/ips"
+)
+
+func TestSignedIpVerify(t *testing.T) {
+	tlsCert1, err := staking.NewTLSCert()
+	require.NoError(t, err)
+	cert1 := staking.CertificateFromX509(tlsCert1.Leaf)
+	require.NoError(t, staking.ValidateCertificate(cert1))
+
+	tlsCert2, err := staking.NewTLSCert()
+	require.NoError(t, err)
+	cert2 := staking.CertificateFromX509(tlsCert2.Leaf)
+	require.NoError(t, staking.ValidateCertificate(cert2))
+
+	now := time.Now()
+
+	type test struct {
+		name         string
+		signer       crypto.Signer
+		expectedCert *staking.Certificate
+		ip           UnsignedIP
+		maxTimestamp time.Time
+		expectedErr  error
+	}
+
+	tests := []test{
+		{
+			name:         "valid (before max time)",
+			signer:       tlsCert1.PrivateKey.(crypto.Signer),
+			expectedCert: cert1,
+			ip: UnsignedIP{
+				IPPort: ips.IPPort{
+					IP:   net.IPv4(1, 2, 3, 4),
+					Port: 1,
+				},
+				Timestamp: uint64(now.Unix()) - 1,
+			},
+			maxTimestamp: now,
+			expectedErr:  nil,
+		},
+		{
+			name:         "valid (at max time)",
+			signer:       tlsCert1.PrivateKey.(crypto.Signer),
+			expectedCert: cert1,
+			ip: UnsignedIP{
+				IPPort: ips.IPPort{
+					IP:   net.IPv4(1, 2, 3, 4),
+					Port: 1,
+				},
+				Timestamp: uint64(now.Unix()),
+			},
+			maxTimestamp: now,
+			expectedErr:  nil,
+		},
+		{
+			name:         "timestamp too far ahead",
+			signer:       tlsCert1.PrivateKey.(crypto.Signer),
+			expectedCert: cert1,
+			ip: UnsignedIP{
+				IPPort: ips.IPPort{
+					IP:   net.IPv4(1, 2, 3, 4),
+					Port: 1,
+				},
+				Timestamp: uint64(now.Unix()) + 1,
+			},
+			maxTimestamp: now,
+			expectedErr:  errTimestampTooFarInFuture,
+		},
+		{
+			name:         "sig from wrong cert",
+			signer:       tlsCert1.PrivateKey.(crypto.Signer),
+			expectedCert: cert2, // note this isn't cert1
+			ip: UnsignedIP{
+				IPPort: ips.IPPort{
+					IP:   net.IPv4(1, 2, 3, 4),
+					Port: 1,
+				},
+				Timestamp: uint64(now.Unix()),
+			},
+			maxTimestamp: now,
+			expectedErr:  errInvalidSignature,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			signedIP, err := tt.ip.Sign(tt.signer)
+			require.NoError(t, err)
+
+			err = signedIP.Verify(tt.expectedCert, tt.maxTimestamp)
+			require.ErrorIs(t, err, tt.expectedErr)
+		})
+	}
+}

network/peer/peer.go

@@ -906,8 +906,9 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) {
 		return
 	}
 
-	myTime := p.Clock.Unix()
-	clockDifference := math.Abs(float64(msg.MyTime) - float64(myTime))
+	myTime := p.Clock.Time()
+	myTimeUnix := uint64(myTime.Unix())
+	clockDifference := math.Abs(float64(msg.MyTime) - float64(myTimeUnix))
 
 	p.Metrics.ClockSkew.Observe(clockDifference)
 
@@ -916,13 +917,13 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) {
 			p.Log.Warn("beacon reports out of sync time",
 				zap.Stringer("nodeID", p.id),
 				zap.Uint64("peerTime", msg.MyTime),
-				zap.Uint64("myTime", myTime),
+				zap.Uint64("myTime", myTimeUnix),
 			)
 		} else {
 			p.Log.Debug("peer reports out of sync time",
 				zap.Stringer("nodeID", p.id),
 				zap.Uint64("peerTime", msg.MyTime),
-				zap.Uint64("myTime", myTime),
+				zap.Uint64("myTime", myTimeUnix),
 			)
 		}
 		p.StartClose()
@@ -974,18 +975,6 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) {
 		return
 	}
 
-	// Note that it is expected that the [ipSigningTime] can be in the past. We
-	// are just verifying that the claimed signing time isn't too far in the
-	// future here.
-	if float64(msg.IpSigningTime)-float64(myTime) > p.MaxClockDifference.Seconds() {
-		p.Log.Debug("peer attempting to connect with version timestamp too far in the future",
-			zap.Stringer("nodeID", p.id),
-			zap.Uint64("ipSigningTime", msg.IpSigningTime),
-		)
-		p.StartClose()
-		return
-	}
-
 	// handle subnet IDs
 	for _, subnetIDBytes := range msg.TrackedSubnets {
 		subnetID, err := ids.ToID(subnetIDBytes)
@@ -1089,11 +1078,24 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) {
 		},
 		Signature: msg.Sig,
 	}
-	if err := p.ip.Verify(p.cert); err != nil {
-		p.Log.Debug("signature verification failed",
-			zap.Stringer("nodeID", p.id),
-			zap.Error(err),
-		)
+	maxTimestamp := myTime.Add(p.MaxClockDifference)
+	if err := p.ip.Verify(p.cert, maxTimestamp); err != nil {
+		if _, ok := p.Beacons.GetValidator(constants.PrimaryNetworkID, p.id); ok {
+			p.Log.Warn("beacon has invalid signature or is out of sync",
+				zap.Stringer("nodeID", p.id),
+				zap.Uint64("peerTime", msg.MyTime),
+				zap.Uint64("myTime", myTimeUnix),
+				zap.Error(err),
+			)
+		} else {
+			p.Log.Debug("peer has invalid signature or is out of sync",
+				zap.Stringer("nodeID", p.id),
+				zap.Uint64("peerTime", msg.MyTime),
+				zap.Uint64("myTime", myTimeUnix),
+				zap.Error(err),
+			)
+		}
+
 		p.StartClose()
 		return
 	}