[tor-bugs] #25598 [Circumvention/Snowflake]: Let the broker inform proxies how often to poll

[Tor Bug Tracker & Wiki]

#25598: Let the broker inform proxies how often to poll
-------------------------------------+--------------------------------
 Reporter:  dcf                      |          Owner:  (none)
     Type:  enhancement              |         Status:  needs_revision
 Priority:  Medium                   |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:  starter                  |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:                           |        Sponsor:
-------------------------------------+--------------------------------
Changes (by dcf):

 * status:  new => needs_revision


Comment:

 Replying to [comment:5 serna]:
 > Here's how it would work with a hardcoded 20sec poll rate:
 https://github.com/BubuAnabelas/snowflake/pull/1

 Thanks for this patch.

 https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-
 ba4bd8a4477426567c409d66c2cf8a28R175-R176

 {{{
 +                       nextPoll, _ := strconv.Atoi(resp.Header.Get
 ("Snowflake-Next-Poll"))
 +                       pollInterval = time.Duration(nextPoll) *
 time.Second
 }}}

 The `Atoi` needs an error check. Otherwise a parse error or missing header
 may return a value of 0 and make the proxy start trying to DoS the broker
 :)

 There should also be a safety minimum, so the proxy will not obey if the
 broker asks it to go ''too'' fast. Cf.
 [https://gitweb.torproject.org/flashproxy.git/tree/proxy/flashproxy.js?id=0c5ea1b04c4725fccc5a98a25bede5c419e07fcd#n545
 the corresponding code in flashproxy].

 I propose factoring out a separate function that takes a `string` and
 returns a `time.Duration`. It parses the string and enforces a minimum. In
 case of a parse error, it returns a default.

 https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-
 ba4bd8a4477426567c409d66c2cf8a28L29-R30

 {{{
 -const pollInterval = 5 * time.Second
 +var pollInterval = 20 * time.Second
 }}}

 Now that this is variable, it is better to limit its scope to the context
 in which it is used (not a global variable if possible). The constant
 default value can remain global.

 https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-
 177f47700613253ab3ba906035e86714R49

 {{{
         snowflake.config.brokerPollInterval = xhr.getResponseHeader
 ('Snowflake-Next-Poll') * 1000 || snowflake.config.brokerPollInterval;
 }}}

 It looks like the check for a missing header needs to go before the
 multiplication by 1000, not after. Here too I recommend factoring out a
 function that checks the syntax and enforces a minimum. (Think of the
 proxy trying to defend itself from a malfunctioning broker.)

 It's better if the code doesn't modify things in the global `Config`
 object. The more you can localize the poll interval, the better.

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