Respect transport parameters and allowed net types by sirzooro · Pull Request #901 · pion/ice

sirzooro · 2026-03-21T14:03:54Z

Summary

This change makes transport/network-type handling consistent across candidate gathering and remote candidate admission.

It fixes cases where the ICE agent could use STUN/TURN URLs with mismatched transport or accept remote candidates on disabled network types.

What changed

1) Remote candidate filtering by configured network types

Added a guard in addRemoteCandidate to ignore candidates whose NetworkType is not enabled in AgentConfig.NetworkTypes.
Added logging for ignored candidates.

2) URL transport interpretation and filtering

Added helpers:
- effectiveURLProtoType (applies default transport when URI transport is omitted)
- urlSupportsSrflxGathering
- relayNetworkTypesForURL
- configuredNetworkTypes
Applied this logic to srflx gathering (gatherCandidatesSrflx, gatherCandidatesSrflxUDPMux) so only valid UDP URLs are used.

3) Relay gathering honors network types and filtered local binds

Refactored relay gathering to select per-URL/per-network-type behavior instead of hardcoded network assumptions.
Kept support for interface/IP filtered local binding addresses in relay/srflx paths.
Replaced manual host:port formatting in srflx paths with net.JoinHostPort(..., strconv.Itoa(...)) for safer address construction.

4) IPv6 TURN status (important)

IPv6 TURN code paths were updated and covered by tests, but full IPv6 TURN support is not complete yet.
For now, IPv6 TURN relay gathering is intentionally disabled in runtime path.
Related IPv6 relay test scenario is present but currently skipped (t.Skip("IPv6 TURN is not supported yet")).

Reference issue

Fixes #367

codecov · 2026-03-21T14:18:34Z

Codecov Report

❌ Patch coverage is 71.01449% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.13%. Comparing base (e29653e) to head (b764b3a).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
gather.go	70.58%	41 Missing and 19 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
- Coverage   88.36%   88.13%   -0.23%     
==========================================
  Files          45       45              
  Lines        5654     5715      +61     
==========================================
+ Hits         4996     5037      +41     
- Misses        452      468      +16     
- Partials      206      210       +4

Flag	Coverage Δ
go	`88.13% <71.01%> (-0.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR makes ICE transport/network-type handling consistent across candidate gathering and remote candidate admission, addressing cases where STUN/TURN URLs with mismatched transport were used and where remote candidates could be accepted on disabled network types.

Changes:

Filter remote candidates by AgentConfig.NetworkTypes in addRemoteCandidate, with logging for ignored candidates.
Introduce URL transport interpretation helpers and apply them to srflx gathering to only use supported (UDP) URLs.
Refactor relay gathering to honor configured network types and local bind filtering, with added/updated tests (including a skipped IPv6 TURN scenario).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`gather.go`	Adds URL/network-type helper logic; filters srflx URLs by effective transport; refactors TURN relay gathering to select behavior per URL and network type.
`gather_test.go`	Updates interface mask handling for IPv6 and adds tests for relay transport/network-type selection and srflx URL transport filtering.
`agent.go`	Ignores remote candidates whose `NetworkType` is disabled in agent configuration.
`agent_test.go`	Adds tests validating remote/prflx candidates are discarded when network types are disabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-21T14:20:00Z

-					conn, connectErr := a.net.DialTCP(NetworkTypeTCP4.String(), nil, tcpAddr)
-					if connectErr != nil {
-						a.log.Warnf("Failed to dial TCP address %s: %v", turnServerAddr, connectErr)
+					turnServerAddr := fmt.Sprintf("%s:%d", url.Host, url.Port)


turnServerAddr is built via fmt.Sprintf("%s:%d", url.Host, url.Port), which can produce an invalid address for IPv6 hosts (missing brackets) and is inconsistent with the net.JoinHostPort usage in the srflx paths. Use net.JoinHostPort(url.Host, strconv.Itoa(url.Port)) here as well so TURN over IPv6 literals (and future IPv6 TURN support) works correctly.

Suggested change

turnServerAddr := fmt.Sprintf("%s:%d", url.Host, url.Port)

turnServerAddr := net.JoinHostPort(url.Host, strconv.Itoa(url.Port))

Copilot · 2026-03-21T14:20:00Z

+						udpConn, dialErr := a.net.DialUDP(network, nil, udpAddr)
+						if dialErr != nil {
+							a.log.Warnf("Failed to dial DTLS address %s: %v", turnServerAddr, dialErr)

-						return
-					}
+							return
+						}

-					conn := tls.Client(tcpConn, &tls.Config{
-						ServerName:         url.Host,
-						InsecureSkipVerify: a.insecureSkipVerify, //nolint:gosec
-					})
+						conn, connectErr := dtls.ClientWithOptions(&fakenet.PacketConn{Conn: udpConn}, udpConn.RemoteAddr(),
+							dtls.WithServerName(url.Host),
+							dtls.WithInsecureSkipVerify(a.insecureSkipVerify), //nolint:gosec
+							dtls.WithLoggerFactory(a.loggerFactory),
+						)
+						if connectErr != nil {
+							a.log.Warnf("Failed to create DTLS client: %v", turnServerAddr, connectErr)

-					if hsErr := conn.HandshakeContext(ctx); hsErr != nil {
-						if closeErr := tcpConn.Close(); closeErr != nil {
-							a.log.Errorf("Failed to close relay connection: %v", closeErr)
+							return
 						}
-						a.log.Warnf("Failed to connect to relay: %v", hsErr)

-						return
-					}
+						if connectErr = conn.HandshakeContext(ctx); connectErr != nil {
+							a.log.Warnf("Failed to create DTLS client: %v", turnServerAddr, connectErr)

-					relAddr = conn.LocalAddr().(*net.TCPAddr).IP.String() //nolint:forcetypeassert
-					relPort = conn.LocalAddr().(*net.TCPAddr).Port        //nolint:forcetypeassert
-					relayProtocol = relayProtocolTLS
-					locConn = turn.NewSTUNConn(conn)
-				default:
-					a.log.Warnf("Unable to handle URL in gatherCandidatesRelay %v", url)
+							return


In the DTLS (TURNS+UDP) branch, udpConn is opened before dtls.ClientWithOptions/HandshakeContext, but on connectErr or handshake failure the code returns without closing udpConn (and potentially without closing conn). This can leak sockets during gathering retries/timeouts; ensure the UDP socket and DTLS conn are closed on all error paths.

Copilot · 2026-03-21T14:20:01Z

+					rAddr := relayConn.LocalAddr().(*net.UDPAddr) //nolint:forcetypeassert
+					if shouldFilterLocationTracked(rAddr.IP) {
+						a.log.Warnf("TURN address %s is somehow filtered for location tracking reasons", rAddr.IP)
+


If shouldFilterLocationTracked(rAddr.IP) is true, the function returns without cleaning up resources from client.Listen()/Allocate() (e.g., relayConn, locConn, and the TURN client). This can leak connections; close the allocated relay connection and underlying client/locConn before returning on this path.

Suggested change

if relayConErr := relayConn.Close(); relayConErr != nil {

a.log.Warnf("Failed to close relay %v", relayConErr)

}

client.Close()

if locCloseErr := locConn.Close(); locCloseErr != nil {

a.log.Warnf("Failed to close local connection %v", locCloseErr)

}

Copilot · 2026-03-21T14:20:01Z

+			if !useFilteredLocalAddrs { // nolint:nestif
+				if networkType.IsIPv6() {
+					bindAddrs = append(bindAddrs, "[::]:0")
+				} else {
+					bindAddrs = append(bindAddrs, "0.0.0.0:0")
+				}


The if networkType.IsIPv6() branch inside if !useFilteredLocalAddrs is currently unreachable because IPv6 network types are already skipped at the top of the loop. Either remove this dead branch for clarity, or move the IPv6 skip closer to where IPv6 is actually unsupported so the bind address selection logic stays coherent when IPv6 TURN is implemented.

Suggested change

if !useFilteredLocalAddrs { // nolint:nestif

if networkType.IsIPv6() {

bindAddrs = append(bindAddrs, "[::]:0")

} else {

bindAddrs = append(bindAddrs, "0.0.0.0:0")

}

if !useFilteredLocalAddrs {

bindAddrs = append(bindAddrs, "0.0.0.0:0")

left as-is, it will be needed later for IPv6 TURN

Copilot · 2026-03-21T14:20:01Z

+func TestAddRemoteCandidateRespectsNetworkTypes(t *testing.T) {
+	defer test.CheckRoutines(t)()
+
+	agent, err := NewAgentWithOptions(
+		WithNetworkTypes([]NetworkType{NetworkTypeUDP4}),
+	)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, agent.Close())
+	}()
+
+	tcpCandidate, err := UnmarshalCandidate("1052353102 1 tcp 1675624447 192.0.2.1 8080 typ host tcptype passive")
+	require.NoError(t, err)
+	require.NoError(t, agent.AddRemoteCandidate(tcpCandidate))
+
+	remoteCandidates, err := agent.GetRemoteCandidates()
+	require.NoError(t, err)
+	require.Len(t, remoteCandidates, 0)
+}


AddRemoteCandidate processes candidates asynchronously (it spawns a goroutine that calls a.loop.Run(...)). This test calls GetRemoteCandidates() immediately after AddRemoteCandidate(...), so it may pass even if the candidate would be incorrectly accepted later. Use require.Eventually (as in TestAddRemoteCandidateHonorsRemoteIPFilter) or run addRemoteCandidate via agent.loop.Run to deterministically assert the candidate was rejected.

Make transport/network-type handling consistent across candidate gathering and remote candidate admission. This fixes cases where the ICE agent could use STUN/TURN URLs with mismatched transport or accept remote candidates on disabled network types.

JoTurk

we should add more tests here but we can mere it as its, thank you.

sirzooro force-pushed the filter_transport branch from 6596f67 to 48404fa Compare March 21, 2026 14:09

sirzooro requested a review from Copilot March 21, 2026 14:14

Copilot started reviewing on behalf of sirzooro March 21, 2026 14:14 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

sirzooro force-pushed the filter_transport branch from 48404fa to 6ca4fc9 Compare March 21, 2026 14:55

sirzooro force-pushed the filter_transport branch from 6ca4fc9 to b764b3a Compare March 21, 2026 15:08

sirzooro requested a review from JoTurk March 21, 2026 15:19

JoTurk approved these changes Mar 23, 2026

View reviewed changes

sirzooro merged commit fdd86c4 into pion:master Mar 23, 2026
26 of 27 checks passed

sirzooro deleted the filter_transport branch March 23, 2026 18:03

	turnServerAddr := fmt.Sprintf("%s:%d", url.Host, url.Port)
	turnServerAddr := net.JoinHostPort(url.Host, strconv.Itoa(url.Port))

+						if relayConErr := relayConn.Close(); relayConErr != nil {
+							a.log.Warnf("Failed to close relay %v", relayConErr)
+						}
+						client.Close()
+						if locCloseErr := locConn.Close(); locCloseErr != nil {
+							a.log.Warnf("Failed to close local connection %v", locCloseErr)
+						}

Conversation

sirzooro commented Mar 21, 2026

Summary

What changed

1) Remote candidate filtering by configured network types

2) URL transport interpretation and filtering

3) Relay gathering honors network types and filtered local binds

4) IPv6 TURN status (important)

Reference issue

Uh oh!

codecov Bot commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

sirzooro Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

sirzooro Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

sirzooro Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

sirzooro Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

sirzooro Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

JoTurk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Mar 21, 2026 •

edited

Loading