[tor-bugs] #19026 [Circumvention/Snowflake]: Remove local LAN address ICE candidates

[Tor Bug Tracker & Wiki]

#19026: Remove local LAN address ICE candidates
-------------------------------------+------------------------------
 Reporter:  dcf                      |          Owner:  arlolra
     Type:  enhancement              |         Status:  needs_review
 Priority:  Medium                   |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:  cohosh                   |        Sponsor:
-------------------------------------+------------------------------

Comment (by arlolra):

 Replying to [comment:16 dcf]:
 > Could
 > {{{
 >       if !bc.keepLocalAddresses {
 >               offer = &webrtc.SessionDescription{
 >                       Type: offer.Type,
 >                       SDP:  stripLocalAddresses(offer.SDP),
 >               }
 >       }
 > }}}
 > be instead
 > {{{
 >       if !bc.keepLocalAddresses {
 >               offer.SDP = stripLocalAddresses(offer.SDP)
 >       }
 > }}}
 > ?

 It could, but since `offer *webrtc.SessionDescription` comes from a call
 to `pc.LocalDescription()`, I didn't want to invalidate the cached parsed
 description in that structure,
 https://github.com/pion/webrtc/blob/master/sessiondescription.go#L10-L13


 > https://github.com/keroserene/snowflake/compare/trac19026#diff-
 0f3a063993ea3b440ad2ce0abb6ac195R105
 >
 > {{{
 >                       if a.IsICECandidate() {
 > }}}
 >
 > Is it necessary to test `IsICECandidate`, or could you skip it and just
 check the `err` result of `ToICECandidate`?

 You could skip it, yes, but I felt the cheap string check was preferable
 attempting a parse,
 https://github.com/pion/sdp/blob/03441e3c706c7c3b719ee75194049a31cbb2eb7e/common_description.go#L112-L122

 > ----
 >
 > The attributes loop is structured like this, with `attrs = append(attrs,
 a)` in three places:
 > {{{
 > for a in attributes {
 >       if a.IsICECandidate() {
 >               ice, err = a.ToICECandidate()
 >               if err != nil {
 >                       attrs = append(attrs, a)
 >                       continue
 >               }
 >               if ice.Typ == "host" {
 >                       ip = net.ParseIP(ice.Address)
 >                       if ip == nil {
 >                               attrs = append(attrs, a)
 >                               continue
 >                       }
 >                       if IsLocal(ip) {
 >                               /* no append in this case */
 >                               continue
 >                       }
 >               }
 >       }
 >       attrs = append(attrs, a)
 > }
 > }}}
 >
 > Consider restructuring so that you only `continue` in the "skip" case,
 and all other cases fall through to `attrs = append(attrs, a)`. Expressing
 the logic: if a candidate, and type=="host", and an IP address, and IP
 address is local, skip; otherwise keep.
 > {{{
 > for a in attributes {
 >       if a.IsICECandidate() {
 >               ice, err = a.ToICECandidate()
 >               if err == nil && ice.Typ == "host" {
 >                       ip = net.ParseIP(ice.Address)
 >                       if ip != nil && IsLocal(ip) {
 >                               /* no append in this case */
 >                               continue
 >                       }
 >               }
 >       }
 >       attrs = append(attrs, a)
 > }
 > }}}
 >
 > But also possibly with my note about `ToICECandidate` above:
 > {{{
 > for a in attributes {
 >       if ice, err = a.ToICECandidate(); err == nil {
 >               if ice.Typ == "host" {
 >                       ip = net.ParseIP(ice.Address)
 >                       if ip != nil && IsLocal(ip) {
 >                               /* no append in this case */
 >                               continue
 >                       }
 >               }
 >       }
 >       attrs = append(attrs, a)
 > }
 > }}}

 Yeah, that was ugly.  I pushed commit for this suggestion,
 https://github.com/keroserene/snowflake/commit/edd53af92ac868cf3ba57988e14de887f088a47b

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19026#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online