[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 dcf):

 Replying to [comment:14 arlolra]:
 > Here's a branch for this now,
 > https://github.com/keroserene/snowflake/compare/trac19026

 https://github.com/keroserene/snowflake/compare/trac19026#diff-
 0f3a063993ea3b440ad2ce0abb6ac195R144

 Could
 {{{
         if !bc.keepLocalAddresses {
                 offer = &webrtc.SessionDescription{
                         Type: offer.Type,
                         SDP:  stripLocalAddresses(offer.SDP),
                 }
         }
 }}}
 be instead
 {{{
         if !bc.keepLocalAddresses {
                 offer.SDP = stripLocalAddresses(offer.SDP)
         }
 }}}
 ?

 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`?

 ----

 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)
 }
 }}}

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