improvement: use SetIPFilter for local addrs

Closes https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40271.
Supersedes https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/merge_requests/417.

This simplifies the code and (probably) removes the need for
`StripLocalAddresses`, although makes us more dependent on Pion.

Signed-off-by: Cecylia Bocovich <cohosh@torproject.org>

client/lib/rendezvous.go

@@ -127,15 +127,6 @@ func newBrokerChannelFromConfig(config ClientConfig) (*BrokerChannel, error) {
 func (bc *BrokerChannel) Negotiate(offer *webrtc.SessionDescription) (
 	*webrtc.SessionDescription, error,
 ) {
-	// Ideally, we could specify an `RTCIceTransportPolicy` that would handle
-	// this for us.  However, "public" was removed from the draft spec.
-	// See https://developer.mozilla.org/en-US/docs/Web/API/RTCConfiguration#RTCIceTransportPolicy_enum
-	if !bc.keepLocalAddresses {
-		offer = &webrtc.SessionDescription{
-			Type: offer.Type,
-			SDP:  util.StripLocalAddresses(offer.SDP),
-		}
-	}
 	offerSDP, err := util.SerializeSessionDescription(offer)
 	if err != nil {
 		return nil, err

client/lib/webrtc.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"io"
 	"log"
+	"net"
 	"net/url"
 	"sync"
 	"time"
@@ -17,6 +18,7 @@ import (
 
 	"gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/v2/common/event"
 	"gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/v2/common/proxy"
+	"gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/v2/common/util"
 )
 
 // WebRTCPeer represents a WebRTC connection to a remote snowflake proxy.
@@ -166,7 +168,8 @@ func (c *WebRTCPeer) checkForStaleness(timeout time.Duration) {
 // receive an answer from broker, and wait for data channel to open
 func (c *WebRTCPeer) connect(config *webrtc.Configuration, broker *BrokerChannel) error {
 	log.Println(c.id, " connecting...")
-	err := c.preparePeerConnection(config)
+
+	err := c.preparePeerConnection(config, broker.keepLocalAddresses)
 	localDescription := c.pc.LocalDescription()
 	c.eventsLogger.OnNewSnowflakeEvent(event.EventOnOfferCreated{
 		WebRTCLocalDescription: localDescription,
@@ -207,10 +210,25 @@ func (c *WebRTCPeer) connect(config *webrtc.Configuration, broker *BrokerChannel
 
 // preparePeerConnection creates a new WebRTC PeerConnection and returns it
 // after non-trickle ICE candidate gathering is complete.
-func (c *WebRTCPeer) preparePeerConnection(config *webrtc.Configuration) error {
+func (c *WebRTCPeer) preparePeerConnection(
+	config *webrtc.Configuration,
+	keepLocalAddresses bool,
+) error {
 	var err error
 	s := webrtc.SettingEngine{}
-	s.SetICEMulticastDNSMode(ice.MulticastDNSModeDisabled)
+
+	if !keepLocalAddresses {
+		s.SetIPFilter(func(ip net.IP) (keep bool) {
+			// `IsLoopback()` and `IsUnspecified` are likely not neded here,
+			// but let's keep them just in case.
+			// FYI there is similar code in other files in this project.
+			keep = !util.IsLocal(ip) && !ip.IsLoopback() && !ip.IsUnspecified()
+			return
+		})
+		s.SetICEMulticastDNSMode(ice.MulticastDNSModeDisabled)
+	}
+	s.SetIncludeLoopbackCandidate(keepLocalAddresses)
+
 	// Use the SetNet setting https://pkg.go.dev/github.com/pion/webrtc/v3#SettingEngine.SetNet
 	// to get snowflake working in shadow (where the AF_NETLINK family is not implemented).
 	// These two lines of code functionally revert a new change in pion by silently ignoring

common/util/util.go

@@ -74,6 +74,11 @@ func IsLocal(ip net.IP) bool {
 }
 
 // Removes local LAN address ICE candidates
+//
+// This is unused after https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/merge_requests/442,
+// but come in handy later for https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40322
+// Also this is exported, so let's not remove it at least until
+// the next major release.
 func StripLocalAddresses(str string) string {
 	var desc sdp.SessionDescription
 	err := desc.Unmarshal([]byte(str))

probetest/probetest.go

@@ -14,6 +14,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"net"
 	"net/http"
 	"os"
 	"strings"
@@ -54,6 +55,17 @@ func makePeerConnectionFromOffer(stunURL string, sdp *webrtc.SessionDescription,
 	dataChanOpen chan struct{}, dataChanClosed chan struct{}, iceGatheringTimeout time.Duration) (*webrtc.PeerConnection, error) {
 
 	settingsEngine := webrtc.SettingEngine{}
+
+	settingsEngine.SetIPFilter(func(ip net.IP) (keep bool) {
+		// `IsLoopback()` and `IsUnspecified` are likely not neded here,
+		// but let's keep them just in case.
+		// FYI there is similar code in other files in this project.
+		keep = !util.IsLocal(ip) && !ip.IsLoopback() && !ip.IsUnspecified()
+		return
+	})
+	// FYI this is `false` by default anyway as of pion/webrtc@4
+	settingsEngine.SetIncludeLoopbackCandidate(false)
+
 	// Use the SetNet setting https://pkg.go.dev/github.com/pion/webrtc/v3#SettingEngine.SetNet
 	// to functionally revert a new change in pion by silently ignoring
 	// when net.Interfaces() fails, rather than throwing an error
@@ -168,11 +180,7 @@ func probeHandler(stunURL string, w http.ResponseWriter, r *http.Request) {
 		// Otherwise it must be closed below, wherever `closePcOnReturn` is set to `false`.
 	}()
 
-	sdp = &webrtc.SessionDescription{
+	answer, err := util.SerializeSessionDescription(pc.LocalDescription())
-		Type: pc.LocalDescription().Type,
-		SDP:  util.StripLocalAddresses(pc.LocalDescription().SDP),
-	}
-	answer, err := util.SerializeSessionDescription(sdp)
 	if err != nil {
 		log.Printf("Error making WebRTC connection: %s", err)
 		w.WriteHeader(http.StatusInternalServerError)

proxy/lib/proxy-go_test.go

@@ -336,7 +336,7 @@ func TestBrokerInteractions(t *testing.T) {
 
 	Convey("Proxy connections to broker", t, func() {
 		var err error
-		broker, err = newSignalingServer("localhost", false)
+		broker, err = newSignalingServer("localhost")
 		So(err, ShouldBeNil)
 		tokens = newTokens(0)
 

proxy/lib/snowflake.go

@@ -193,15 +193,13 @@ func limitedRead(r io.Reader, limit int64) ([]byte, error) {
 
 // SignalingServer keeps track of the SignalingServer in use by the Snowflake
 type SignalingServer struct {
-	url                *url.URL
+	url       *url.URL
-	transport          http.RoundTripper
+	transport http.RoundTripper
-	keepLocalAddresses bool
 }
 
-func newSignalingServer(rawURL string, keepLocalAddresses bool) (*SignalingServer, error) {
+func newSignalingServer(rawURL string) (*SignalingServer, error) {
 	var err error
 	s := new(SignalingServer)
-	s.keepLocalAddresses = keepLocalAddresses
 	s.url, err = url.Parse(rawURL)
 	if err != nil {
 		return nil, fmt.Errorf("invalid broker url: %s", err)
@@ -271,13 +269,6 @@ func (s *SignalingServer) pollOffer(sid string, proxyType string, acceptedRelayP
 // and wait for its response
 func (s *SignalingServer) sendAnswer(sid string, pc *webrtc.PeerConnection) error {
 	ld := pc.LocalDescription()
-	if !s.keepLocalAddresses {
-		ld = &webrtc.SessionDescription{
-			Type: ld.Type,
-			SDP:  util.StripLocalAddresses(ld.SDP),
-		}
-	}
-
 	answer, err := util.SerializeSessionDescription(ld)
 	if err != nil {
 		return err
@@ -397,6 +388,17 @@ func (d dataChannelHandlerWithRelayURL) datachannelHandler(conn *webRTCConn, rem
 func (sf *SnowflakeProxy) makeWebRTCAPI() *webrtc.API {
 	settingsEngine := webrtc.SettingEngine{}
 
+	if !sf.KeepLocalAddresses {
+		settingsEngine.SetIPFilter(func(ip net.IP) (keep bool) {
+			// `IsLoopback()` and `IsUnspecified` are likely not neded here,
+			// but let's keep them just in case.
+			// FYI there is similar code in other files in this project.
+			keep = !util.IsLocal(ip) && !ip.IsLoopback() && !ip.IsUnspecified()
+			return
+		})
+	}
+	settingsEngine.SetIncludeLoopbackCandidate(sf.KeepLocalAddresses)
+
 	// Use the SetNet setting https://pkg.go.dev/github.com/pion/webrtc/v3#SettingEngine.SetNet
 	// to get snowflake working in shadow (where the AF_NETLINK family is not implemented).
 	// These two lines of code functionally revert a new change in pion by silently ignoring
@@ -727,7 +729,7 @@ func (sf *SnowflakeProxy) Start() error {
 	sf.periodicProxyStats = newPeriodicProxyStats(sf.SummaryInterval, sf.EventDispatcher, sf.bytesLogger)
 	sf.EventDispatcher.AddSnowflakeEventListener(sf.periodicProxyStats)
 
-	broker, err = newSignalingServer(sf.BrokerURL, sf.KeepLocalAddresses)
+	broker, err = newSignalingServer(sf.BrokerURL)
 	if err != nil {
 		return fmt.Errorf("error configuring broker: %s", err)
 	}
@@ -805,7 +807,7 @@ func (sf *SnowflakeProxy) Stop() {
 func (sf *SnowflakeProxy) checkNATType(config webrtc.Configuration, probeURL string) error {
 	log.Printf("Checking our NAT type, contacting NAT check probe server at \"%v\"...", probeURL)
 
-	probe, err := newSignalingServer(probeURL, false)
+	probe, err := newSignalingServer(probeURL)
 	if err != nil {
 		return fmt.Errorf("Error parsing url: %w", err)
 	}