quic: determine local address for connections Properly handling path migration requires QUIC connections to know both the address they are sending from and the address they are sending to. Determine the local address for a path by creating a temporary UDP socket bound to the path; this sends no packets but lets us see what address the kernel thinks should be used on the path. (When using a non-*net.UDPConn connection, we don't attempt to do this on the theory that our PacketConn likely doesn't use the real network.) Update fake-network tests to use valid source/destination addresses for all datagrams. Change-Id: Ia89f7daf479b10df4c1d23f3e64ad3726a6a6964 Reviewed-on: https://go-review.googlesource.com/c/net/+/832365 Reviewed-by: Nicholas Husin <nsh@golang.org> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Damien Neil <dneil@google.com> Reviewed-by: Nicholas Husin <husin@google.com>
diff --git a/quic/conn.go b/quic/conn.go index ca94999..3351237 100644 --- a/quic/conn.go +++ b/quic/conn.go
@@ -101,8 +101,13 @@ } }() + localAddr, err := e.packetConn.LocalAddrFor(peerAddr) + if err != nil { + return nil, err + } c.path = pathAddrs{ - peer: unmapAddrPort(peerAddr), + local: unmapAddrPort(localAddr), + peer: unmapAddrPort(peerAddr), } // A one-element buffer allows us to wake a Conn's event loop as a
diff --git a/quic/conn_id_test.go b/quic/conn_id_test.go index f2094ea..bb1ddb2 100644 --- a/quic/conn_id_test.go +++ b/quic/conn_id_test.go
@@ -794,6 +794,7 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) te.wantIdle("server should ignore Initial with too-long SCID") @@ -826,6 +827,7 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) te.wantIdle("server should ignore Initial with too-long DCID")
diff --git a/quic/conn_recv.go b/quic/conn_recv.go index 81ed5fa..fc6cc89 100644 --- a/quic/conn_recv.go +++ b/quic/conn_recv.go
@@ -12,12 +12,6 @@ ) func (c *Conn) handleDatagram(now time.Time, dgram *datagram) (handled bool) { - if !c.path.local.IsValid() { - // We don't have any way to tell in the general case what address we're - // sending packets from. Set our address from the destination address of - // the first packet received from the peer. - c.path.local = dgram.path.local - } if dgram.path.peer.IsValid() && dgram.path.peer != c.path.peer { if c.side == clientSide { // "If a client receives packets from an unknown server address,
diff --git a/quic/conn_send_test.go b/quic/conn_send_test.go index ec76fe9..a21640c 100644 --- a/quic/conn_send_test.go +++ b/quic/conn_send_test.go
@@ -146,7 +146,7 @@ copy(pkt[6:], dcid) tc.endpoint.write(&datagram{ b: pkt, - path: pathAddrs{peer: tc.conn.path.peer}, + path: tc.path, }) bytesSent += len(pkt)
diff --git a/quic/conn_test.go b/quic/conn_test.go index ccf5b66..b7d8a78 100644 --- a/quic/conn_test.go +++ b/quic/conn_test.go
@@ -13,7 +13,6 @@ "fmt" "log/slog" "math" - "net/netip" "reflect" "strings" "testing" @@ -130,6 +129,7 @@ t *testing.T conn *Conn endpoint *testEndpoint + path pathAddrs // Keys are distinct from the conn's keys, // because the test may know about keys before the conn does. @@ -231,7 +231,14 @@ } } + var path pathAddrs + if side == serverSide { + path = pathAddrs{peer: testClientAddr, local: testServerAddr} + } else { + path = pathAddrs{peer: testServerAddr, local: testClientAddr} + } endpoint := newTestEndpoint(t, config) + endpoint.localAddr = path.local endpoint.configTransportParams = configTransportParams endpoint.configTestConn = configTestConn conn, err := endpoint.e.newConn( @@ -240,7 +247,8 @@ side, cids, "", - netip.MustParseAddrPort("127.0.0.1:443")) + path.peer, + ) if err != nil { t.Fatal(err) } @@ -362,11 +370,12 @@ pad = fmt.Sprintf(" (padded to %v)", d.paddedSize) } t.Logf("%v datagram%v", text, pad) + t.Logf(" %v", d.path) for _, p := range d.packets { var s string switch p.ptype { case packetType1RTT: - s = fmt.Sprintf(" %v pnum=%v", p.ptype, p.num) + s = fmt.Sprintf(" %v pnum=%v dst={%x}", p.ptype, p.num, p.dstConnID) default: s = fmt.Sprintf(" %v pnum=%v ver=%v dst={%x} src={%x}", p.ptype, p.num, p.version, p.dstConnID, p.srcConnID) } @@ -374,7 +383,7 @@ s += fmt.Sprintf(" token={%x}", p.token) } if p.keyPhaseBit { - s += fmt.Sprintf(" KeyPhase") + s += " KeyPhase" } if p.keyNumber != 0 { s += fmt.Sprintf(" keynum=%v", p.keyNumber) @@ -389,12 +398,24 @@ // write sends the Conn a datagram. func (tc *testConn) write(d *testDatagram) { tc.t.Helper() + if d.path == (pathAddrs{}) { + d.path = tc.path + } tc.endpoint.writeDatagram(d) } // writeFrames sends the Conn a datagram containing the given frames. func (tc *testConn) writeFrames(ptype packetType, frames ...debugFrame) { tc.t.Helper() + tc.write(tc.newDatagramWithFrames(ptype, frames...)) +} + +func (tc *testConn) newDatagramWithFrames(ptype packetType, frames ...debugFrame) *testDatagram { + return tc.newDatagramWithPathAndFrames(ptype, tc.path, frames...) +} + +func (tc *testConn) newDatagramWithPathAndFrames(ptype packetType, path pathAddrs, frames ...debugFrame) *testDatagram { + tc.t.Helper() space := spaceForPacketType(ptype) dstConnID := tc.conn.connIDState.local[0].cid if tc.conn.connIDState.local[0].seq == -1 && ptype != packetTypeInitial { @@ -412,12 +433,12 @@ dstConnID: dstConnID, srcConnID: tc.peerConnID, }}, - path: tc.conn.path, + path: path, } if ptype == packetTypeInitial && tc.conn.side == serverSide { d.paddedSize = 1200 } - tc.write(d) + return d } // writeAckForAll sends the Conn a datagram containing an ack for all packets up to the @@ -456,11 +477,12 @@ synctest.Wait() tc.sentPackets = nil tc.sentFrames = nil - buf := tc.endpoint.read() - if buf == nil { + dgram := tc.endpoint.readdgram() + if dgram == nil { return nil } - d := parseTestDatagram(tc.t, tc.endpoint, tc, buf) + d := parseTestDatagram(tc.t, tc.endpoint, tc, dgram.b) + d.path = dgram.path // Log the datagram before removing ignored frames. // When things go wrong, it's useful to see all the frames. logDatagram(tc.t, "-> conn under test sends", d)
diff --git a/quic/endpoint.go b/quic/endpoint.go index e1e3dca..a68beed 100644 --- a/quic/endpoint.go +++ b/quic/endpoint.go
@@ -46,6 +46,13 @@ LocalAddr() netip.AddrPort Read(f func(*datagram)) error Write(datagram) error + + // LocalAddrFor reports the local address to use to send to a remote address. + // If the networking implementation doesn't support asking this question, + // it returns an invalid address and no error. + // If the implementation does support the question but the question cannot be answered + // (because there is no route, say), it returns an error. + LocalAddrFor(remoteAddr netip.AddrPort) (netip.AddrPort, error) } // Listen listens on a local network address.
diff --git a/quic/endpoint_test.go b/quic/endpoint_test.go index 1927823..253fe79 100644 --- a/quic/endpoint_test.go +++ b/quic/endpoint_test.go
@@ -13,6 +13,7 @@ "log/slog" "net" "net/netip" + "slices" "sync" "testing" "testing/synctest" @@ -229,6 +230,7 @@ type testEndpoint struct { t *testing.T e *Endpoint + localAddr netip.AddrPort closeOnce sync.Once recvc chan *datagram idlec chan struct{} @@ -240,15 +242,16 @@ lastInitialDstConnID []byte // for parsing Retry packets sentDatagramsMu sync.Mutex - sentDatagrams [][]byte + sentDatagrams []*datagram } func newTestEndpoint(t *testing.T, config *Config) *testEndpoint { te := &testEndpoint{ - t: t, - recvc: make(chan *datagram), - idlec: make(chan struct{}), - conns: make(map[*Conn]*testConn), + t: t, + recvc: make(chan *datagram), + idlec: make(chan struct{}), + conns: make(map[*Conn]*testConn), + localAddr: testServerAddr, } var err error te.e, err = newEndpoint((*testEndpointUDPConn)(te), config, (*testEndpointHooks)(te)) @@ -275,14 +278,25 @@ } func (te *testEndpoint) write(d *datagram) { + if d.path == (pathAddrs{}) { + d.path = pathAddrs{local: te.localAddr, peer: testClientAddr} + } te.recvc <- d synctest.Wait() } -var testClientAddr = netip.MustParseAddrPort("10.0.0.1:8000") +var ( + testServerAddr = netip.MustParseAddrPort("10.0.0.1:443") + testClientAddr = netip.MustParseAddrPort("10.0.0.2:999") + + defaultEndpointPath = pathAddrs{local: testServerAddr, peer: testClientAddr} +) func (te *testEndpoint) writeDatagram(d *testDatagram) { te.t.Helper() + if d.path == (pathAddrs{}) { + te.t.Fatal("writeDatagram with no path") + } logDatagram(te.t, "<- endpoint under test receives", d) var buf []byte for _, p := range d.packets { @@ -335,6 +349,17 @@ func (te *testEndpoint) read() []byte { te.t.Helper() + d := te.readdgram() + if d == nil { + return nil + } + return d.b +} + +// readdgram is not a good name, but read and readDatagram were taken and I'm not ready to +// refactor all the callers yet. +func (te *testEndpoint) readdgram() *datagram { + te.t.Helper() synctest.Wait() te.sentDatagramsMu.Lock() defer te.sentDatagramsMu.Unlock() @@ -348,13 +373,14 @@ func (te *testEndpoint) readDatagram() *testDatagram { te.t.Helper() - buf := te.read() - if buf == nil { + dgram := te.readdgram() + if dgram == nil { return nil } - p, _ := parseGenericLongHeaderPacket(buf) + p, _ := parseGenericLongHeaderPacket(dgram.b) tc := te.connForSource(p.dstConnID) - d := parseTestDatagram(te.t, te, tc, buf) + d := parseTestDatagram(te.t, te, tc, dgram.b) + d.path = dgram.path logDatagram(te.t, "-> endpoint under test sends", d) return d } @@ -380,6 +406,7 @@ func (te *testEndpointHooks) newConn(c *Conn, cids newServerConnIDs) { tc := newTestConnForConn(te.t, (*testEndpoint)(te), c, cids) + tc.path = pathAddrs{local: te.localAddr, peer: c.path.peer} te.conns[c] = tc } @@ -394,7 +421,11 @@ } func (te *testEndpointUDPConn) LocalAddr() netip.AddrPort { - return netip.MustParseAddrPort("127.0.0.1:443") + return te.localAddr +} + +func (te *testEndpointUDPConn) LocalAddrFor(remote netip.AddrPort) (netip.AddrPort, error) { + return te.localAddr, nil } func (te *testEndpointUDPConn) Read(f func(*datagram)) error { @@ -413,6 +444,11 @@ func (te *testEndpointUDPConn) Write(dgram datagram) error { te.sentDatagramsMu.Lock() defer te.sentDatagramsMu.Unlock() - te.sentDatagrams = append(te.sentDatagrams, append([]byte(nil), dgram.b...)) + dgram.b = slices.Clone(dgram.b) + if !dgram.path.local.IsValid() { + // Sender didn't set a local address to send from, so set it here. + dgram.path.local = te.localAddr + } + te.sentDatagrams = append(te.sentDatagrams, &dgram) return nil }
diff --git a/quic/qlog_test.go b/quic/qlog_test.go index fb120d2..58600cd 100644 --- a/quic/qlog_test.go +++ b/quic/qlog_test.go
@@ -256,7 +256,8 @@ []byte{1, 2, 3, 4}, // random data, to avoid this looking like a stateless reset }, nil) tc.endpoint.write(&datagram{ - b: dgram, + b: dgram, + path: tc.path, }) qr.wantEvents(t, jsonEvent{
diff --git a/quic/retry_test.go b/quic/retry_test.go index 6a0bccd..452edc1 100644 --- a/quic/retry_test.go +++ b/quic/retry_test.go
@@ -53,6 +53,7 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) got := te.readDatagram() @@ -98,6 +99,7 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) tc := te.accept() @@ -144,12 +146,14 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) te.wantDatagram("server closes connection after Initial with invalid Retry token", initialConnectionCloseDatagram( rt.retry.srcConnID, rt.originalSrcConnID, + defaultEndpointPath, errInvalidToken)) } @@ -176,12 +180,14 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) te.wantDatagram("server closes connection after Initial with expired token", initialConnectionCloseDatagram( rt.retry.srcConnID, rt.originalSrcConnID, + defaultEndpointPath, errInvalidToken)) } @@ -195,7 +201,8 @@ rt := newRetryServerTest(t) te := rt.te path := pathAddrs{ - peer: netip.MustParseAddrPort("10.0.0.2:8000"), + local: te.localAddr, + peer: netip.MustParseAddrPort("10.0.0.2:8000"), } te.writeDatagram(&testDatagram{ packets: []*testPacket{{ @@ -218,6 +225,7 @@ initialConnectionCloseDatagram( rt.retry.srcConnID, rt.originalSrcConnID, + path, errInvalidToken)) } @@ -243,12 +251,14 @@ }, }, }}, + path: defaultEndpointPath, paddedSize: 1200, }) te.wantDatagram("server closes connection after Initial from wrong address", initialConnectionCloseDatagram( []byte("short id"), rt.originalSrcConnID, + defaultEndpointPath, errInvalidToken)) } @@ -615,7 +625,7 @@ } } -func initialConnectionCloseDatagram(srcConnID, dstConnID []byte, code transportError) *testDatagram { +func initialConnectionCloseDatagram(srcConnID, dstConnID []byte, path pathAddrs, code transportError) *testDatagram { return &testDatagram{ packets: []*testPacket{{ ptype: packetTypeInitial, @@ -629,5 +639,6 @@ }, }, }}, + path: path, } }
diff --git a/quic/stateless_reset_test.go b/quic/stateless_reset_test.go index c7aff0d..3b02cc3 100644 --- a/quic/stateless_reset_test.go +++ b/quic/stateless_reset_test.go
@@ -255,7 +255,8 @@ } dgram = append(dgram, resetToken[:]...) tc.endpoint.write(&datagram{ - b: dgram, + b: dgram, + path: tc.path, }) if err := tc.conn.Wait(canceledContext()); !errors.Is(err, errStatelessReset) { t.Errorf("conn.Wait() = %v, want errStatelessReset", err)
diff --git a/quic/tls_test.go b/quic/tls_test.go index 3d03b06..4989d8b 100644 --- a/quic/tls_test.go +++ b/quic/tls_test.go
@@ -110,6 +110,7 @@ }, }}, paddedSize: 1200, + path: tc.path, }, { // Server Initial + Handshake + 1-RTT packets: []*testPacket{{ @@ -146,6 +147,7 @@ }, }}, paddedSize: 1200, + path: tc.path, }, { // Client Initial + Handshake + 1-RTT packets: []*testPacket{{ @@ -187,6 +189,7 @@ }, }}, paddedSize: 1200, + path: tc.path, }, { // Server HANDSHAKE_DONE packets: []*testPacket{{ @@ -200,6 +203,7 @@ debugFrameHandshakeDone{}, }, }}, + path: tc.path, }, { // Client ack (after max_ack_delay) packets: []*testPacket{{ @@ -214,6 +218,7 @@ }, }, }}, + path: tc.path, }} } @@ -618,7 +623,8 @@ sendInvalid := func() { t.Logf("<- conn under test receives invalid datagram") tc.conn.sendMsg(&datagram{ - b: invalid, + b: invalid, + path: tc.path, }) synctest.Wait() }
diff --git a/quic/udp.go b/quic/udp.go index cf23c5c..ec30a73 100644 --- a/quic/udp.go +++ b/quic/udp.go
@@ -4,7 +4,10 @@ package quic -import "net/netip" +import ( + "net" + "net/netip" +) // Per-plaform consts describing support for various features. // @@ -26,3 +29,18 @@ } return a } + +func localAddrFor(local, remote netip.AddrPort) (netip.AddrPort, error) { + if local.Addr().IsValid() && !local.Addr().IsUnspecified() { + return local, nil + } + // Simplest portable approach: Bind a socket and see what address it gets. + remoteAddr := net.UDPAddrFromAddrPort(remote) + uc, err := net.DialUDP("udp", nil, remoteAddr) + if err != nil { + return netip.AddrPort{}, err + } + boundAddr := uc.LocalAddr().(*net.UDPAddr).AddrPort() + uc.Close() + return netip.AddrPortFrom(boundAddr.Addr(), local.Port()), nil +}
diff --git a/quic/udp_msg.go b/quic/udp_msg.go index cf8167f..9467d30 100644 --- a/quic/udp_msg.go +++ b/quic/udp_msg.go
@@ -64,6 +64,10 @@ return a.AddrPort() } +func (c *netUDPConn) LocalAddrFor(remote netip.AddrPort) (netip.AddrPort, error) { + return localAddrFor(c.LocalAddr(), remote) +} + func (c *netUDPConn) Read(f func(*datagram)) error { // We shouldn't ever see all of these messages at the same time, // but the total is small so just allocate enough space for everything we use.
diff --git a/quic/udp_other.go b/quic/udp_other.go index 3c9852e..363f74a 100644 --- a/quic/udp_other.go +++ b/quic/udp_other.go
@@ -40,6 +40,10 @@ return a.AddrPort() } +func (c *netUDPConn) LocalAddrFor(remote netip.AddrPort) (netip.AddrPort, error) { + return localAddrFor(c.LocalAddr(), remote) +} + func (c *netUDPConn) Read(f func(*datagram)) error { for { dgram := newDatagram()
diff --git a/quic/udp_packetconn.go b/quic/udp_packetconn.go index 51ad477..5b2062a 100644 --- a/quic/udp_packetconn.go +++ b/quic/udp_packetconn.go
@@ -37,6 +37,14 @@ return c.localAddr } +func (c *netPacketConn) LocalAddrFor(remote netip.AddrPort) (netip.AddrPort, error) { + if c.localAddr.Addr().IsUnspecified() { + // If this is the unspec address, we have no way to pick a better one. + return netip.AddrPort{}, nil + } + return c.localAddr, nil +} + func (c *netPacketConn) Read(f func(*datagram)) error { for { dgram := newDatagram()
diff --git a/quic/version_test.go b/quic/version_test.go index fc5c97e..d7240cc 100644 --- a/quic/version_test.go +++ b/quic/version_test.go
@@ -65,7 +65,8 @@ tc := newTestConn(t, clientSide) p := tc.readPacket() // client Initial packet tc.endpoint.write(&datagram{ - b: appendVersionNegotiation(nil, p.srcConnID, p.dstConnID, 10), + b: appendVersionNegotiation(nil, p.srcConnID, p.dstConnID, 10), + path: tc.path, }) tc.wantIdle("connection does not send a CONNECTION_CLOSE") if err := tc.conn.waitReady(canceledContext()); err != errVersionNegotiation {