[tor-bugs] #29206 [Circumvention/Snowflake]: New design for client -- server protocol for Snowflake

[Tor Bug Tracker & Wiki]

#29206: New design for client -- server protocol for Snowflake
-------------------------------------------------+-------------------------
 Reporter:  cohosh                               |          Owner:  cohosh
     Type:  task                                 |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Circumvention/Snowflake              |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ex-sponsor-19, anti-censorship-      |  Actual Points:
  roadmap                                        |
Parent ID:                                       |         Points:  6
 Reviewer:  dcf                                  |        Sponsor:
                                                 |  Sponsor28-must
-------------------------------------------------+-------------------------

Comment (by cohosh):

 Replying to [comment:17 dcf]:
 > Replying to [comment:13 cohosh]:
 > > I'm going to ask for an incremental review on this one, mostly just to
 get another pair of eyes on what I've done and my next steps before I sink
 more time into going possibly the wrong direction:
 https://github.com/cohosh/snowflake/compare/sequencing
 >
 Thanks for the feedback! I've addressed most of the feedback so far here:
 https://github.com/cohosh/snowflake/tree/sequencing
 > The fields would be better as `uint32`, because the size of `int`
 [https://golang.org/ref/spec#Numeric_types may vary] (though I think you
 handled it correctly by always specifying 32 bits in serialization
 functions). `length` may be better as `uint16`; that's plenty for one call
 to `Write` and it avoids the risk that an implementation will try to
 buffer 4 GB in memory.
 Fixed in
 [https://github.com/cohosh/snowflake/commit/c102894bb903857061e7705e9525612eb9a99b4b
 c102894]
 >
 > IMO `length` should be ''exclusive'' of the header size. Otherwise you
 have to define what it means if `length` < 12. (I believe the current code
 doesn't work in this case because `header.length - snowflakeHeaderLen`
 underflows.) The packets sent by `sendAck` should have `length` = 0.
 >
 Fixed in
 [https://github.com/cohosh/snowflake/commit/02bfa1b4611db39a99379eefd29d569563490a0a
 02bfa1b]
 > Big-endian is more usual for network protocols.
 >
 Fixed in
 [https://github.com/cohosh/snowflake/commit/7566cbc9e3d289a901d717e2ed5bc10a335915d7
 7566cb]
 > I'm not sure about the implementation of `Read`:
 > I would prefer to start with an implementation of `Read` that is
 simpler, if less efficient. I am picturing something like this [...]
 I like this implementation a lot more. Fixed in
 [https://github.com/cohosh/snowflake/commit/b6fb987ba9aca02c6740231df6c9db60b68993a6
 b6fb987] and
 [https://github.com/cohosh/snowflake/commit/9c1b12f7c645e0a8a95199190d89b84ed677e3f8
 9c1b12f]

 I'm going to start on implementing fixed size send windows and cleaning up
 the unit tests a bit.

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