#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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs