[tor-bugs] #29734 [Obfuscation/Snowflake]: Broker should receive country stats information from Proxy and Client
Tor Bug Tracker & Wiki
blackhole at torproject.org
Mon Apr 8 20:35:38 UTC 2019
#29734: Broker should receive country stats information from Proxy and Client
-------------------------------------+--------------------------------
Reporter: cohosh | Owner: cohosh
Type: enhancement | Status: needs_revision
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
-------------------------------------+--------------------------------
Comment (by cohosh):
Replying to [comment:18 dcf]:
Thanks, I went with most of these suggestions. Here's a new PR:
[https://github.com/cohosh/snowflake/compare/geoip geoip] There are a few
items that require additional notes.
> * This branch only tracks client accesses, not proxy accesses. What will
happen with proxy accesses, are they planned to go in the same metrics.log
file?
We're now just tracking proxy accesses.
> * Is there any way to disable the geoip feature, if I don't have the
necessary files, other than by providing an invalid path? Maybe we don't
care to provide a way to disable the feature and it's the operator's
responsibility to provide some kind of geoip files; but in that case, it
may be better to exit with an error if the files cannot be read, rather
than logging the error and continuing on. It is perhaps surprising that
you can explicitly ask for a certain file on the command line with
`-geoipdb path`, and the broker will run even if the path cannot be read.
> * It looks to me like if a geoip lookup fails, it is silently
ignored—this could be misleading. It would be good to distinguish three
cases: geoip file present and lookup found a country code; geoip file
present but lookup found nothing; geoip file absent so no lookup is
possible.
Added a -disablegeoip option that will not load geoip tables and will not
return an error if UpdateCountryStats fails. Otherwise UpdateCountryStats
will return an error if it fails and loading the table will panic if the
file does not exist or if it is incorrectly formatted.
> * The `parseEntry` functions need error checking on `geoipStringToIP`
and `net.ParseIP`, or else they will store a `nil` that will result in a
panic later on.
[https://github.com/cohosh/snowflake/blob/4de81ed039ce28264a98a040a0fde2237844e779/broker/geoip.go#L93
This error] will be more useful if it includes a line number. Let me
suggest a different factoring of the geoip parsing code. Have `parseEntry`
be a plain function, not a method on `GeoIPv4Table`, and have it return
`(GeoIPEntry, error)` rather than inserting into the table itself.
GeoIPLoadFile can do the entry insertion, count lines and annotate the
error result from `parseEntry`.
The difficulty in refactoring it this way is that ipv4 and ipv6 tables
have differently formated entries that need to be parse differently.
I added error checking to parseEntry and have it return `(GeoIPEntry,
error)` as suggested, but leave it as a method on GeoIPv4Table and
GeoIPv6Table.
> * If `NewMetrics` is called more than once, there will be multiple open
file handles to metrics.log (and only the first one will actually work, I
think). Is it possible that `NewMetrics` could be called more than once?
Making a singleton *Metrics variable causes problems with how Convey does
tests. It shouldn't be called more than once, but for now I'm using
sync.Once on the logging at least so it's explicit
> *
[https://github.com/cohosh/snowflake/blob/4de81ed039ce28264a98a040a0fde2237844e779/broker
/snowflake-broker_test.go#L274-L279 snowflake-broker_test.go]: I'm not
crazy about having the tests depend on external files, especially ones
that may change. Ideally the test code would include its own small geoip
files, either as separate files or as literals in the source code. But at
least, the test should [https://golang.org/pkg/testing/#hdr-Skipping
t.Skip] if a file is missing, rather than report spurious failure. I
suggest adding a layer of abstraction: a `GeoIPLoad(io.Reader)` that
parses any `io.Reader` (for examples a `bytes.Reader`), and write
`GeoIPLoadFile` in terms of it.
> * It would be nice to also have tests that exercise the error paths in
geoip parsing.
I haven't made this abstraction at the moment and went for the small geoip
files option.
> * I would like to see unit tests for basic functions like
`GeoIPRangeClosure` and `GeoIPCheckRange`. They don't have to be long,
just test the edge conditions of an instance.
I replaced these functions with much simpler one line statements, but I
added edge cases to the existing geoip test to make sure we correctly
categorize IPs at the edge of a range
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/29734#comment:22>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list