[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #11271 [Obfsproxy]: ScrambleSuit and obfsproxy repositories should be merged

#11271: ScrambleSuit and obfsproxy repositories should be merged
-----------------------------+-------------------------------
     Reporter:  phw          |      Owner:  phw
         Type:  enhancement  |     Status:  new
     Priority:  normal       |  Milestone:
    Component:  Obfsproxy    |    Version:
   Resolution:               |   Keywords:  scramblesuit, git
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+-------------------------------

Comment (by yawning):

 I told asn that I will also do a review of the branch since I'm somewhat
 familiar with most of the changes and the codebase.

 Spec changes:
  * "Trivial semantic improvement."
 `301895818d7e86d4869c01b261f6ad0aa258d420`
  * "Elaborate on server's behaviour."
 `6d7b5750a09730c0fe8038340f09435a44b4eb8b`
  * "Fix typo." `22af6078eb881f6250e4c3e7164b170b504302f9`
  * "Fix problems in UniformDH spec."
 `81b64fc9d0935823e74e156089bdcf660bf907f5`
  * "Let the server echo the epoch."
 `a706313d71b5aa9c7084e909177e7398287f232f`
  * "Add missing reference." `083a5241413ea7c1e1b8518e85cc0f974c4d0362`
  * "Fix ticket handshake spec." `3f436b4bf67b0bbcbc091c55a9082f1b0fa0f326`
  * "Elaborate on protocol polymorphism."
 `d4d34f7a871ce55e93160be489a4a34e316cd442`
  * "Add missing markers to HMACs."
 `cef0b8ac2e32ce238382b24809398781e55b546c`

 All look fine to me.  There's another thing in the spec that can be
 clarified, but that's orthogonal to this (Section 2.3 describes key
 derivation from the client perspective, server side flips the keys, see
 `ScrambleSuitTransport.deriveSecrets()`) and can wait till after the
 merge.

 #11092 related non-spec changes:
  * "Close connection if authentication fails."
 `d5b8a8923b0f36461c5e28838e6499943520926d`
  * "Add ChangeLog entry for #11092."
 `811730a870a002568eff8e890f474131d586db01`
  * "Only search for mark in expected space."
 `404fa805dc67a0f41f36945083b1108953aaa14e`
  * "Add and use const.MAX_HANDSHAKE_LENGTH."
 `e1705e40b1cac0bac9f5e00c0b4e07b6b627e8a1`
  * "Stop processing data after authentication failed."
 `c2192aecdfc5a812f4d3e0fd99b85b3015e00e4b`
  * "Increase closing threshold."
 `3a4361806efc2cd7d3046292768f48bef4e9e02c`

 Looks good to me.

 #10893 related non-spec changes:
  * "Make the server simply echo the client's epoch."
 `9f44a239877a3d0ecace3724414ff5afaccfa755`
  * "When authenticating, also test epoch boundaries."
 `c5496839c9a1b8990400ed6d03178368fdb54223`
  * "Add ChangeLog entry about scramblesuit spec improvements."
 `f1bba132ce19cff9282473e8ccce79a24adda000`

 Looks good to me.  I assume "fix several issues" covers this in the
 ChangeLog?

 #10991 related changes:
  * "Improve packet morphing algorithm."
 `025416b36327493e479990ae4d67b7f81783291d`
  * "Add PacketMorpher unittests."
 `61a62a588b07ecb480d8752a037350e37d115d2f`
  * "Add ChangeLog entry about scramblesuit's packetmorpher improvements."
 `cb7efe11d8e01133a737f503371d8e37372d7b45`

 Looks good to me.

 Misc fixes:
  * "Use more readable error messages."
 `151bf8a1e525b93c431af8c9bec170ef8879e4f7`
  * "Delete misplaced comma." `6b1c847e6f58e515f05fa17e0279e3936e5709ac`

 Do these need ChangeLog entries?  Maybe for the first, the 2nd is rather
 trivial...

 #10887 related:
  * "Help operators learn their server descriptor."
 `64473c14e86e881a9d20f41249f599104d9feec8`
  * "Add a ChangeLog entry for scramblesuit's #10887."
 `e0a61565e9654da54395735225c69afbf5a90cd7`

 Looks good to me, apart from the assert issue.  Maybe we should discourage
 running without a state directory configured since there will be no
 backing store for Session Tickets.

 Test case stuff:
  * "Add scramblesuit unittests for the state module."
 `8ac1dfab7f0a58de632fbea94ff9d543405b062c`
  * "Add TicketTest unittest." `634a0fa1b44eacf2115fe90f84b911c8497053ed`
  * "Use TransportConfig in scramblesuit unittests."
 `ccc70ba4ceedef7d99a6a0dc1c34661d56c654eb`

 Why was `def test4_ioerrorFail( self ):` dropped from the state tests?

 Random thoughts:
  * Are we going to maintain the `doc/scramblesuit/ChangeLog` file?  Should
 we add a note there saying that it's for historical purposes only or
 remove it?

 That's all from me, hopefully I didn't miss anything.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/11271#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs