Skip to content

autonatv2: fix server dial data request policy #3247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sukunrt merged 1 commit into from
Mar 24, 2025
Merged

autonatv2: fix server dial data request policy #3247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions p2p/protocol/autonatv2/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var (
errDialDataRefused = errors.New("dial data refused")
)

type dataRequestPolicyFunc = func(s network.Stream, dialAddr ma.Multiaddr) bool
type dataRequestPolicyFunc = func(observedAddr, dialAddr ma.Multiaddr) bool

type EventDialRequestCompleted struct {
Error error
Expand Down Expand Up @@ -212,7 +212,7 @@ func (as *server) serveDialRequest(s network.Stream) EventDialRequestCompleted {

nonce := msg.GetDialRequest().Nonce

isDialDataRequired := as.dialDataRequestPolicy(s, dialAddr)
isDialDataRequired := as.dialDataRequestPolicy(s.Conn().RemoteMultiaddr(), dialAddr)
if isDialDataRequired && !as.limiter.AcceptDialDataRequest(p) {
msg = pb.Message{
Msg: &pb.Message_DialResponse{
Expand Down Expand Up @@ -517,11 +517,11 @@ func (r *rateLimiter) Close() {

// amplificationAttackPrevention is a dialDataRequestPolicy which requests data when the peer's observed
// IP address is different from the dial back IP address
func amplificationAttackPrevention(s network.Stream, dialAddr ma.Multiaddr) bool {
Copy link
Collaborator

@MarcoPolo MarcoPolo Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dialAddr was previously unused? That seems like it should have been caught by a lint check.

Copy link
Member Author

@sukunrt sukunrt Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. Made #3248 to track this.

I've configured my editor, but we should enable lint checks in ci too.

connIP, err := manet.ToIP(s.Conn().RemoteMultiaddr())
func amplificationAttackPrevention(observedAddr, dialAddr ma.Multiaddr) bool {
observedIP, err := manet.ToIP(observedAddr)
if err != nil {
return true
}
dialIP, _ := manet.ToIP(s.Conn().LocalMultiaddr()) // must be an IP multiaddr
return !connIP.Equal(dialIP)
dialIP, _ := manet.ToIP(dialAddr) // must be an IP multiaddr
return !observedIP.Equal(dialIP)
}
76 changes: 73 additions & 3 deletions p2p/protocol/autonatv2/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func TestServerDataRequest(t *testing.T) {
dialer := bhost.NewBlankHost(swarmt.GenSwarm(t, swarmt.OptDisableTCP))
// ask for dial data for quic address
an := newAutoNAT(t, dialer, allowPrivateAddrs, withDataRequestPolicy(
func(s network.Stream, dialAddr ma.Multiaddr) bool {
func(_, dialAddr ma.Multiaddr) bool {
if _, err := dialAddr.ValueForProtocol(ma.P_QUIC_V1); err == nil {
return true
}
Expand Down Expand Up @@ -197,7 +197,7 @@ func TestServerMaxConcurrentRequestsPerPeer(t *testing.T) {
doneChan := make(chan struct{})
an := newAutoNAT(t, dialer, allowPrivateAddrs, withDataRequestPolicy(
// stall all allowed requests
func(s network.Stream, dialAddr ma.Multiaddr) bool {
func(_, dialAddr ma.Multiaddr) bool {
<-doneChan
return true
}),
Expand Down Expand Up @@ -255,7 +255,7 @@ func TestServerDataRequestJitter(t *testing.T) {
dialer := bhost.NewBlankHost(swarmt.GenSwarm(t, swarmt.OptDisableTCP))
// ask for dial data for quic address
an := newAutoNAT(t, dialer, allowPrivateAddrs, withDataRequestPolicy(
func(s network.Stream, dialAddr ma.Multiaddr) bool {
func(_, dialAddr ma.Multiaddr) bool {
if _, err := dialAddr.ValueForProtocol(ma.P_QUIC_V1); err == nil {
return true
}
Expand Down Expand Up @@ -520,6 +520,76 @@ func TestReadDialData(t *testing.T) {
}
}

func TestServerDataRequestWithAmplificationAttackPrevention(t *testing.T) {
// server will skip all tcp addresses
dialer := bhost.NewBlankHost(swarmt.GenSwarm(t, swarmt.OptDisableTCP))
// ask for dial data for quic address
an := newAutoNAT(t, dialer, allowPrivateAddrs,
WithServerRateLimit(10, 10, 10, 2),
withAmplificationAttackPreventionDialWait(0),
)
defer an.Close()
defer an.host.Close()

c := newAutoNAT(t, nil, allowPrivateAddrs)
defer c.Close()
defer c.host.Close()

idAndWait(t, c, an)

err := c.host.Network().Listen(ma.StringCast("/ip6/::1/udp/0/quic-v1"))
if err != nil {
// machine doesn't have ipv6
t.Skip("skipping test because machine doesn't have ipv6")
}

var quicv4Addr ma.Multiaddr
var quicv6Addr ma.Multiaddr
for _, a := range c.host.Addrs() {
if _, err := a.ValueForProtocol(ma.P_QUIC_V1); err == nil {
if _, err := a.ValueForProtocol(ma.P_IP4); err == nil {
quicv4Addr = a
} else {
quicv6Addr = a
}
}
}
res, err := c.GetReachability(context.Background(), []Request{{Addr: quicv4Addr, SendDialData: false}})
require.NoError(t, err)
require.Equal(t, Result{
Addr: quicv4Addr,
Reachability: network.ReachabilityPublic,
Status: pb.DialStatus_OK,
}, res)

// ipv6 address should require dial data
_, err = c.GetReachability(context.Background(), []Request{{Addr: quicv6Addr, SendDialData: false}})
require.Error(t, err)
require.ErrorContains(t, err, "invalid dial data request: low priority addr")

// ipv6 address should work fine with dial data
res, err = c.GetReachability(context.Background(), []Request{{Addr: quicv6Addr, SendDialData: true}})
Copy link
Collaborator

@MarcoPolo MarcoPolo Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check actually asserts that dial data was requested, right?

Is there a way to assert that we were requested dial data?

Copy link
Member Author

@sukunrt sukunrt Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this to the checks:
require.ErrorContains(t, err, "invalid dial data request: low priority addr")

require.NoError(t, err)
require.Equal(t, Result{
Addr: quicv6Addr,
Reachability: network.ReachabilityPublic,
Status: pb.DialStatus_OK,
}, res)
}

func TestDefaultAmplificationAttackPrevention(t *testing.T) {
q1 := ma.StringCast("/ip4/1.2.3.4/udp/1234/quic-v1")
q2 := ma.StringCast("/ip4/1.2.3.4/udp/1235/quic-v1")
t1 := ma.StringCast("/ip4/1.2.3.4/tcp/1234")

require.False(t, amplificationAttackPrevention(q1, q1))
require.False(t, amplificationAttackPrevention(q1, q2))
require.False(t, amplificationAttackPrevention(q1, t1))

t2 := ma.StringCast("/ip4/1.1.1.1/tcp/1235") // different IP
require.True(t, amplificationAttackPrevention(q2, t2))
}

func FuzzServerDialRequest(f *testing.F) {
a := newAutoNAT(f, nil, allowPrivateAddrs, WithServerRateLimit(math.MaxInt32, math.MaxInt32, math.MaxInt32, 2))
c := newAutoNAT(f, nil)
Expand Down
Loading