[tor-bugs] #18628 [Obfuscation/Snowflake]: Devise some way for the browser proxy to forward metadata to the bridge before the OR data

[Tor Bug Tracker & Wiki]

#18628: Devise some way for the browser proxy to forward metadata to the bridge
before the OR data
-----------------------------------+------------------------------
 Reporter:  arlolra                |          Owner:
     Type:  defect                 |         Status:  needs_review
 Priority:  High                   |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:                         |  Actual Points:
Parent ID:                         |         Points:
 Reviewer:                         |        Sponsor:
-----------------------------------+------------------------------

Comment (by dcf):

 Replying to [comment:6 cmm32]:
 > * Most comments addressed.

 Great. Thanks. I think this is getting close. I'll ask arlolra and serene
 to take a look too.

  *
 [https://gitweb.torproject.org/user/dcf/snowflake.git/diff/?h=bug18628&id=2846bbec9adeb7b2e06aa5b204982496c1ece5ce&id2=db2251345d16a530cf27fe951b235719515c2b88
 cumulative bug18628 diff for snowflake]
  * [https://gitweb.torproject.org/pluggable-
 transports/websocket.git/diff/?h=bug18628&id=02a8eb6d7a236f8b805dbd086f1c46f5dfb51149&id2=6dc990ad6a898bc507605c51a5aa860fb9f74201
 cumulative diff for websocket]

 ----

 In websocket:

 {{{
 +       // Request
 +       request http.Request
 }}}
 {{{
 +       ws.request = *req
 }}}
 I'm thinking it would be better to make request a pointer; i.e. `request
 *http.Request`, `ws.request = req`, in order to avoid copying the struct.
 I'm not sure it's safe to shallow-copy a Request struct, which may contain
 other recursive structures.

 ----

 In snowflake:

 I'm a little concerned about parsing the SDP in order to get the remote
 address. Ideally, of course, we'd find another way to do it, or use a
 proper library to parse the SDP. But in the meantime, I
 [https://gitweb.torproject.org/user/dcf/snowflake.git/commit/?h=bug18628&id=485538bcf00bd4ddaeb5f81dd05e3caaa89ffd6d
 pushed some tests] to cover some additional syntax options that I took
 from RFC 4566. Can you pull those changes and update the code so that all
 the tests pass with `go test`?

 {{{
 +       if conn.RemoteAddr() != nil && conn.RemoteAddr().String() != "" {
 }}}
 I don't think we need the `!= ""` comparison here. Unless there's a kind
 of Addr you're thinking of that can return an empty string?

 I know that in comment:5 I made a fuss about moving the SDP parsing logic
 into webrtcConn.RemoteAddr. But for some reason, doing that makes it stop
 working for me to run proxy-go and snowflake-client on the same computer!
 The code hangs forever inside the call to c.pc.RemoteDescription(). I
 don't know why (it might be a bug in go-webrtc), but I bisected and
 [https://gitweb.torproject.org/user/dcf/snowflake.git/commit/?h=bug18628&id=2c0cfdfb953d3b37254c0a4dff5b61ca2be795cf
 2c0cfdfb] is definitely the commit that causes a change. So as a
 workaround, we might need to put the SDP parsing back where it was, until
 we figure out the cause. I can do that, if it's okay with you, because I'm
 easily able to reproduce the problem.

 {{{
 +       if clientIP == nil {
 +               // Set client addr to empty
 +               log.Printf("failed to validate client_ip: %s", addr)
 +               addr = ""
 +
 }}}
 Let's not log an IP address here. We can add it (behind an "unsafe
 logging" option) if we need it later.

 > Note client IP address is now added to `opt.relayURL` before dialing
 websocket.

 I think that doing it this way creates a race condition. You have a bunch
 of goroutines reading a writing global state. Better to make a copy of the
 global relay URL (e.g., via url.Parse) that you can mutate each time. In
 fact, opt.relayURL should probably be removed completely
 ([https://gitweb.torproject.org/pluggable-
 transports/snowflake.git/commit/?id=dbe1ef4fa55569e5d17c405df5707f6eb05bb56c
 I just did that on the master branch]).

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18628#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online