From ae5bd528211f07ca4be8582571b5a33f11fcf853 Mon Sep 17 00:00:00 2001 From: WofWca Date: Mon, 25 Nov 2024 01:43:29 +0400 Subject: [PATCH] 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 --- client/lib/rendezvous.go | 9 --------- client/lib/webrtc.go | 24 +++++++++++++++++++++--- common/util/util.go | 5 +++++ probetest/probetest.go | 18 +++++++++++++----- proxy/lib/proxy-go_test.go | 2 +- proxy/lib/snowflake.go | 30 ++++++++++++++++-------------- 6 files changed, 56 insertions(+), 32 deletions(-) diff --git a/client/lib/rendezvous.go b/client/lib/rendezvous.go index 27c0d4f..79bda46 100644 --- a/client/lib/rendezvous.go +++ b/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 diff --git a/client/lib/webrtc.go b/client/lib/webrtc.go index fc46f2f..b648f59 100644 --- a/client/lib/webrtc.go +++ b/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 diff --git a/common/util/util.go b/common/util/util.go index a180692..f66c69f 100644 --- a/common/util/util.go +++ b/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)) diff --git a/probetest/probetest.go b/probetest/probetest.go index d003806..fb59cb7 100644 --- a/probetest/probetest.go +++ b/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{ - Type: pc.LocalDescription().Type, - SDP: util.StripLocalAddresses(pc.LocalDescription().SDP), - } - answer, err := util.SerializeSessionDescription(sdp) + answer, err := util.SerializeSessionDescription(pc.LocalDescription()) if err != nil { log.Printf("Error making WebRTC connection: %s", err) w.WriteHeader(http.StatusInternalServerError) diff --git a/proxy/lib/proxy-go_test.go b/proxy/lib/proxy-go_test.go index 11096f8..e8e50db 100644 --- a/proxy/lib/proxy-go_test.go +++ b/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) diff --git a/proxy/lib/snowflake.go b/proxy/lib/snowflake.go index 6643196..59afd54 100644 --- a/proxy/lib/snowflake.go +++ b/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 - transport http.RoundTripper - keepLocalAddresses bool + url *url.URL + transport http.RoundTripper } -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) }