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

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

#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:
-----------------------------------+--------------------------------
Changes (by dcf):

 * status:  new => needs_revision


Comment:

 Thanks for this.

 I've only taken a quick look. Here is some preliminary 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.
  * I'd prefer not to depend on an external amper, if we can reasonably
 include the necessary code here.
  * `-codec=post` and `-codec=amp` is a nice idea. The `-help` hint should
 list all the possible values.
  *
 [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.)
  *
 [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.
  * Is it possible to avoid the vendoring changes, or at least get them in
 separate commits so they're separable from the functional changes?

 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.

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