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

Re: [tor-bugs] #25985 [Obfuscation/Snowflake]: Snowflake rendezvous using AMP cache



#25985: Snowflake rendezvous using AMP cache
-----------------------------------+--------------------------------
 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:  needs_review => needs_revision


Comment:

 This is some review of commit
 [https://github.com/nogoegst/snowflake/commit/b75253ba55bde140be733c4cf5425fc1aab161c4
 b75253ba55bde140be733c4cf5425fc1aab161c4].

 I had trouble testing this change locally because `amper.Client` was
 changing my http into https. This is what I did:
 {{{
 broker$ ./broker -disable-tls -addr 127.0.0.1:8080
 proxy-go$ ./proxy-go -broker http://127.0.0.1:8080/
 client$ TOR_PT_MANAGED_TRANSPORT_VER=1 TOR_PT_CLIENT_TRANSPORTS=snowflake
 ./client -codec amp -url http://127.0.0.1:8080/
 }}}
 The error I get from the client is a result of attempting to parse HTTP as
 TLS:
 {{{
 BrokerChannel Error: tls: oversized record received with length 20527
 }}}
 (On further reflection, what I was trying to do wouldn't have worked
 anyway, because the AMP client code would have sent a URL path of
 `/c/s/.../amp/client/...`, not `/amp/client/...`, but I could have worked
 around that for testing.) It looks like the cause is
 [https://github.com/nogoegst/amper/blob/6dbbc42eb44c1f389591c83a268524d5eceed1d1/client.go#L55
 here in amper/client.go], with a hardcoded `"https"`. Looking at that, I
 also noticed a
 [https://github.com/nogoegst/amper/blob/6dbbc42eb44c1f389591c83a268524d5eceed1d1/client.go#L23
 CDNDomain = "cdn.ampproject.org"]. I have to say, it skeeves me a bit to
 see hardcoded constants like that in a library; it seems like the kind of
 thing that belongs in a configuration file.

 ----

 I like the idea behind
 [https://github.com/nogoegst/snowflake/commit/f358a7696045336ef5e15ff6f8ee2467d8d5f53a
 the commit that adds the -codec flag]: generalize a currently hardcoded
 interface, then provide alternate implementations in a later commit. I
 must say, though, that I don't like the layering of
 [https://github.com/nogoegst/snowflake/blob/f358a7696045336ef5e15ff6f8ee2467d8d5f53a/broker/broker.go#L222
 clientOffersPOST]→[https://github.com/nogoegst/snowflake/blob/f358a7696045336ef5e15ff6f8ee2467d8d5f53a/broker/broker.go#L186
 ctx.handleClientOffers]→[https://github.com/nogoegst/snowflake/blob/f358a7696045336ef5e15ff6f8ee2467d8d5f53a/broker/broker.go#L169
 DecodeError]. Instead of returning an `error` directly,
 `ctx.handleClientOffers` writes a JSON representation of the error into a
 buffer provided by the caller, which the caller must then convert back
 into an `error` by calling `DecodeError`. It seems unnecessarily
 convoluted just to enable the side effect of writing directly to the
 response body in the AMP case. This is a case where I would prefer to see
 duplicated code first, because I suspect we can find a better factoring.

 In my testing, the `DecodeError` error recovery didn't work. It decodes to
 an `Error` that has the same `E` string value but is unequal to
 `ErrNoSnowflakes`, for example. When I run a
 [https://gitweb.torproject.org/pluggable-
 transports/snowflake.git/tree/client?id=25b304a9a856f8c791882ad523df26ffc8fa629c
 master branch client] (with POST) against an
 [https://github.com/nogoegst/snowflake/tree/b75253ba55bde140be733c4cf5425fc1aab161c4/broker
 amp branch broker], it fails because the broker returns status 200 when it
 should return status 503. The culprit seems to be
 [https://github.com/nogoegst/snowflake/blob/b75253ba55bde140be733c4cf5425fc1aab161c4/broker/broker.go#L229-L234
 this error-matching switch]. If I add a `default` case with a panic, I see
 that the `ErrNoSnowflakes` case isn't being matched, even though that is
 the intention:
 {{{
 http: panic serving 127.0.0.1:56174: {"error":"No snowflakes proxies
 available"}
 }}}

 ----

 Here's an example of what the AMP responses look like:
 {{{
 $ wget -q -O- http://127.0.0.1:8080/client/amp
 <!doctype html>
 <html amp>
   <head>
     <meta charset="utf-8">
     <title>amp</title>
     <link rel="canonical" href="https://ampproject.org"; />
     <meta name="viewport" content="width=device-width,minimum-scale=1
 ,initial-scale=1">
     <style>body {opacity: 0}</style><noscript><style>body {opacity:
 1}</style></noscript>
     <script async src="https://cdn.ampproject.org/v0.js";></script>
   </head>
   <body>
     <p>In varietate concordia</p>
     <pre id="data">eyJlcnJvciI6Ik5vIHNub3dmbGFrZXMg
 cHJveGllcyBhdmFpbGFibGUifQo</pre>
   </body>
 </html>
 [The base64 decodes to '{"error":"No snowflakes proxies available"}'.]
 }}}
 The overall AMP container encoding looks pretty reasonable. A few tiny
 questions:
  * [https://www.ampproject.org/docs/getting_started/create/basic_markup
 basic_markup] says the `<script async>` has to be the second child of
 `<head>`, right after `<meta charset="utf-8">`.
  * The `<style>body {opacity: 0}</style><noscript><style>body {opacity:
 1}</style></noscript>` doesn't match the documented
 [https://www.ampproject.org/docs/reference/spec/amp-boilerplate.html AMP
 boilerplate code], which is much longer. Is there a reason?
  * `<link rel="canonical" href="https://ampproject.org"; />`: it would be
 better not to refer to any third parties. The AMP guide says this can be a
 relative URL, so I would prefer one of these if they work:
    * `<link rel="canonical" href="">`
    * `<link rel="canonical" href="#">`
    * `<link rel="canonical" href="?">`

 ----

 About the encoded data inside the AMP HTML container: I'm wondering if we
 should always put the data payload ''inside'' the same JSON object that
 can signal an error, rather than making the payload be sent ''instead of''
 the error object when there is no error. This is how it currently works:
  1. Try to parse the response body as JSON with an `"error"` field. If the
 JSON parsing is successful, handle the error and stop.
  2. Otherwise (JSON parsing failed), return the entire response body as a
 blob of bytes.
 That is, under the base64 encoding, a message can look either like this:
 {{{
 {"error": "No snowflake proxies available"}
 }}}
 or like this:
 {{{
 {"type": "answer", "sdp": "..."}
 }}}
 One problem with this design, as far as amper is concerned, is that it's
 impossible for the server to send a payload that happens to parse as JSON
 with an `"error"` field—it will be interpreted as an error, not a payload.
 This consideration doesn't affect Snowflake, because Snowflake's payloads
 don't have an `"error"` field even though they are JSON. But another
 consideration is that you can't tell the difference between a non-error
 response and an improperly encoded error response (if the body is
 truncated, for example).

 I propose to remove this ambiguity by including the data payload inside
 the JSON object that represents an error. Just like in an HTTP response,
 we have two things—a status code and a message—inside one package. A
 message would either look like this:
 {{{
 {"error": "No snowflake proxies available", "data": null}
 }}}
 or like this:
 {{{
 {"error": null, "data": "eyJ0eXBlIjogImFuc3dlciIsICJzZ..."}
 }}}
 (The `null` fields are included for clarity and could be omitted in
 practice.) To handle this, a client needs to `json.Unmarshal` on a `struct
 { Error, Data: string }`. Any JSON decoding error signifies an error in
 the AMP transport, not a Snowflake-level error. Assuming JSON decoding is
 successful, the `"error"` field indicates whether there was a Snowflake-
 level error.

 About signaling errors, I would prefer to use machine-readable strings
 (e.g. `"ErrNoSnowflakes"`) rather than human-readable strings (e.g. `"No
 snowflake proxies available"`). The latter seems more likely to get broken
 accidentally by someone adding punctuation, for example. The variable
 names you've chosen are good, but I would just them as the actual values,
 not just the identifiers. We can set up a canonical mapping, e.g.
 200⇒`""`, 503⇒`"ErrNoSnowflakes"`, 504⇒`"ErrAnswerTimeout"`, to map the
 status codes from POST into the error strings for common handling.

 ----

 Does
 [https://github.com/nogoegst/snowflake/blob/b75253ba55bde140be733c4cf5425fc1aab161c4/client/rendezvous.go#L69
 NewBrokerChannel] need to instantiate an `amper.Client` when it's not
 using `-codec amp`?

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