Opened 5 years ago

Closed 5 years ago

#11271 closed enhancement (fixed)

ScrambleSuit and obfsproxy repositories should be merged

Reported by: phw Owned by: phw
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords: scramblesuit, git
Cc: asn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

So far, we copied ScrambleSuit's files to the obfsproxy repository. That's not very sustainable. Instead, we should merge both repositories so that all future development happens directly on obfsproxy. That way, we would also not lose ScrambleSuit's git log.

Some pitfalls:

  • ScrambleSuit has its own git tags for versioning. We must remove these.

Child Tickets

Change History (14)

comment:1 Changed 5 years ago by asn

I agree.

Also feel free to use doc/scramblesuit/ to host a scramblesuit-specific ChangeLog. Important user-facing changes will still be included in the obfsproxy ChangeLog though.

comment:2 Changed 5 years ago by phw

I think there are two ways to do this.

  1. We could rebase all existing ScrambleSuit commits out of the obfsproxy repository and then merge ScrambleSuit's repository as subtree as outlined on this page: http://git-scm.com/book/ch6-7.html. While that would result in a clean git history, it would mean rewriting a public repository which is bad practice and everybody who has unmerged obfsproxy branches will hate us.
  2. On the other hand, we could first remove all existing ScrambleSuit commits (by doing git rm instead of a rebase) and then proceed with the subtree merge. That way, we would avoid future merge conflicts but the git history would be messy.

I would prefer the second option unless there are more options I didn't think about?

comment:3 Changed 5 years ago by asn

You are talking about moving the whole git history of scramblesuit to obfsproxy? That's a _lot_ of commits (> 300 commits) and most of them are raw and unsquashed ('Fix typo', 'Fix indentation', 'Simplify function', etc.). Are we sure we want to do this?

I would personally prefer if we squash (per-ticket) the latest scramblesuit changes and then merge them to obfsproxy. Then from that point, maintain an obfsproxy repo for any future scramblesuit changes. Does this make sense to you?

comment:4 Changed 5 years ago by asn

As an example of what I said in my previous comment, see my branch scramblesuit_merge in https://git.torproject.org/user/asn/obfsproxy.git.

It merges all scramblesuit commits from d4e0d46 to a9ce128. My tactic was to export all those commits as patches, use some regexps to change the paths, and commit everything that applies cleanly. Stuff that doesn't apply cleanly I made new commits for.

Specifically, any commit that changed the ChangeLog or the unittests did not apply cleanly. The unittests commits did not apply because of different import paths etc. I manually commited the unittest changes.

Unittests seem to run fine, and integration tests too. I haven't done further testing.

I would also like to squash some commits together before merging to master since there is too much noise.

Any comments? You think this is a reasonable approach?

Currently all unittests pass and the integration tests seem to work. I haven't done further testing.

comment:5 in reply to:  4 Changed 5 years ago by asn

Replying to asn:

Currently all unittests pass and the integration tests seem to work. I haven't done further testing.

Oops. Scratch that. See comment:8:ticket:10887 .

comment:6 Changed 5 years ago by asn

Philipp, I hear you will be unavailable till mid-May.

What should we do with this? Should I fix #10887 and merge my branch? Any chance you could give a quick review to my branch before I merge?

comment:7 in reply to:  3 ; Changed 5 years ago by phw

Replying to asn:

I would personally prefer if we squash (per-ticket) the latest scramblesuit changes and then merge them to obfsproxy. Then from that point, maintain an obfsproxy repo for any future scramblesuit changes. Does this make sense to you?

Yes, that should be fine. I'm starting to think that discarding ScrambleSuit's git history would also be acceptable. After all, we don't want to keep all of it anyway and depending on how interested you are in its history, at some point you will have to dig into ScrambleSuit's rather than obfsproxy's history. We might as well make that point point 0. Besides, the most recent and important changes are also documented in obfsproxy's changelog. What do you think?

What should we do with this? Should I fix #10887 and merge my branch? Any chance you could give a quick review to my branch before I merge?

Yes, I can review the branch.

comment:8 in reply to:  7 Changed 5 years ago by asn

Replying to phw:

Replying to asn:

I would personally prefer if we squash (per-ticket) the latest scramblesuit changes and then merge them to obfsproxy. Then from that point, maintain an obfsproxy repo for any future scramblesuit changes. Does this make sense to you?

Yes, that should be fine. I'm starting to think that discarding ScrambleSuit's git history would also be acceptable. After all, we don't want to keep all of it anyway and depending on how interested you are in its history, at some point you will have to dig into ScrambleSuit's rather than obfsproxy's history. We might as well make that point point 0. Besides, the most recent and important changes are also documented in obfsproxy's changelog. What do you think?

Makes sense.

What should we do with this? Should I fix #10887 and merge my branch? Any chance you could give a quick review to my branch before I merge?

Yes, I can review the branch.

Thank you. I will wait for your review and then merge.

comment:9 Changed 5 years ago 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.

comment:10 in reply to:  9 Changed 5 years ago by asn

Replying to 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.
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?

I'm also wondering about that. Maybe Philipp knows (unittest changes are from 3b97bd28ff073a048aee016b145447630e20da6c in scramblesuit repo).

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?

Adding a note stating its historical value makes sense to me.

Thanks for the review!

comment:11 Changed 5 years ago by asn

FWIW Philipp I'm still waiting for your review before merging this.

comment:12 Changed 5 years ago by phw

Sorry for the long delay.

asn, it looks like all the files in your scramblesuit_merge branch are identical to ScrambleSuit's master branch (with the single exception of USE_IAT_OBFUSCATION being True, which is what we want) so that's good.

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?

I'm OK with both but would favour removing it. If people are interested in the history, they can get the original repository including its git history.

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

Changes in the code made the test fail and I haven't had time to fix it yet.

Looks good to me, apart from the assert issue.

We might want to fix that before merging this. See https://trac.torproject.org/projects/tor/ticket/10887#comment:12 I could create a patch on top of your bug10887_take2 branch?

comment:13 Changed 5 years ago by asn

OK, merged everything to master except the #10887 stuff.

I'm closing this ticket and let's continue development on #10887.

Thanks! Please reopen if you are not satisfied with the merge.

comment:14 Changed 5 years ago by asn

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.