[tor-bugs] #29734 [Obfuscation/Snowflake]: Broker should receive country stats information from Proxy and Client

[Tor Bug Tracker & Wiki]

#29734: Broker should receive country stats information from Proxy and Client
-------------------------------------+-----------------------------
 Reporter:  cohosh                   |          Owner:  cohosh
     Type:  enhancement              |         Status:  merge_ready
 Priority:  Medium                   |      Milestone:
Component:  Obfuscation/Snowflake    |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:  snowflake, geoip, stats  |  Actual Points:  2
Parent ID:  #29207                   |         Points:  1
 Reviewer:  ahf                      |        Sponsor:  Sponsor19
-------------------------------------+-----------------------------
Changes (by dcf):

 * status:  needs_review => merge_ready


Comment:

 Looks good! Nice work on this.

 Replying to [comment:27 cohosh]:
 > Replying to [comment:24 dcf]:
 > > IMO annotating the error message with the problematic line should be
 done in `GeoIPLoadFile`, not in `parseEntry`. This will eliminate the
 duplication of the common `"Provided geoip file is incorrectly formatted"`
 string and ensure that all the error paths get annotated. What I mean is,
 in the `scanner.Scan()` loop, you can replace `return err` with `return
 fmt.Errorf("Provided geoip file is incorrectly formatted: %s. Line is:
 %+q")`.
 > Done, is there a better way of handling the error stubs in `parseEntry`
 other than returning `fmt.Errorf("")`?

 There's nothing wrong with returning a specific "couldn't parse IP: "
 error message IMO, as long as the message gets printed at the top level in
 `GeoIPLoadFile`, not in `parseEntry`. Or, if you just want s generic error
 sentinel without a meaningful message, you can create an error object at
 the top-level scope and just return that:
 {{{
 var errParseError error = errors.New("parse error")
 }}}

 > Replying to [comment:25 dcf]:
 > > Oh and it looks like country stats don't get incremented whenever
 `GetCountryByAddr` doesn't find a match. I'm afraid this will make
 analysis difficult if a large fraction of proxies aren't covered by the
 geoip database, or the database is improperly installed, or something.
 Could we count them with a country code of `"??"` or something?
 >
 > Okay, so I've changed the GetCountryByAddr signature to what you've
 suggested and then just removed the error return value from
 UpdateCountryStats. I've also added setting the country to `"??"` in
 UpdateCountryStats.

 The documentation for `GetCountryByAddr` should explain the second return
 value.

 I think you can remove this condition now:
 {{{
         if country != "" {
 }}}

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