Re: [tor-bugs] #25985 [Obfuscation/Snowflake]: Add AMP cache as another domain fronting option with Google

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#25985: Add AMP cache as another domain fronting option with Google
-----------------------------------+--------------------------------
 Reporter:  twim                   |          Owner:  (none)
     Type:  project                |         Status:  needs_revision
 Priority:  Medium                 |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:                         |  Actual Points:
Parent ID:                         |         Points:
 Reviewer:                         |        Sponsor:
-----------------------------------+--------------------------------

Comment (by twim):

 Replying to [comment:16 dcf]:
 > I've only taken a quick look. Here is some preliminary feedback.
 Thanks for the feedback!
 >  * Changing POST to use only status code 200 breaks backward
 compatibility. I'm opposed to this change. At any rate, it would require
 matching changes in /proxy and /proxy-go. But it's better to remain
 compatible.
 >  * Try to minimize the extent of changes. This ticket shouldn't require
 any changes to `clientOffers`, so I'm skeptical if I see any changes
 there. Don't worry about code duplication. Go ahead and write duplicative
 code. If needed, we can deduplicate in a refactoring. I expect that the
 new feature in the broker will require 1 or 2 new functions and one new
 line in `main`. It's fine if AMP uses JSON and POST does not. If there are
 code architecture changes they need to be in a separate commit from the
 functional changes.

 The point was to retain some of backward compatibility so old clients are
 able to connect to a broker.
 I've added status codes back.


 >  * I'd prefer not to depend on an external amper, if we can reasonably
 include the necessary code here.
 I think it's better to move codec logic out out the scope of snowflake
 itself. In case of amper the library handles domain fronting (which is not
 typical), adding padding and other. Anyway I just don't see the point of
 copy-pasting code if we can just vendor it.

 >  * `-codec=post` and `-codec=amp` is a nice idea. The `-help` hint
 should list all the possible values.
 Fixed.

 >  *
 [https://github.com/nogoegst/snowflake/commit/ce1e2e24d9bd39ad51c82006213dc7a1f1e4776f
 #diff-f9646f10eb8f3471d08ae2de44a3eaf9R100 BrokerChannel.RoundTrip]: it
 seems like this `switch` would be better done in `main` by setting a
 function pointer on `bc`. (Should fail immediately in `main` if the
 command line is bad.)

 Command line should check these values if it needs to. The thing here is
 that this function pointers are messy and are being created in init
 function while the they do depend on BrokerContext state.

 >  *
 [https://github.com/nogoegst/snowflake/commit/ce1e2e24d9bd39ad51c82006213dc7a1f1e4776f
 #diff-79897051d7aac1f314600a930afebe9aR275 In the "/client/amp/" path],
 I'm not sure if a trailing slash is significant, but all the entries
 should either have a trailing slash or not have one.

 This is just how it works. '/client' handles only this exact path, while
 '/client/amp/' handles everything with this prefix and sets up a redirect
 from '/client/amp/' to '/client/amp/'.

 >  * Is it possible to avoid the vendoring changes, or at least get them
 in separate commits so they're separable from the functional changes?

 Right, I decided to leave vendoring for another ticket.

 > The biggest thing for me is to avoid making unnecessary code changes and
 to keep backward compatibility. If you absolutely need a refactoring
 change, you can do it as a separate commit preliminary to making your
 functional changes. It should be of a nature that we would want the
 refactoring even if we weren't going to add the AMP feature. But I feel
 it's less risky (= more likely to get the patch accepted) to add the new
 feature first, in a minimal patch with clearly defined boundaries, and
 then refactor later.

 It's not that much of refactoring though. I'm not liking idea of creating
 duplicate code and adding completely different code - it becomes
 unmaintainable pretty quickly and 'refactor later' never comes.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25985#comment:17>
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