[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:  cmm323
     Type:  defect                 |         Status:  assigned
 Priority:  High                   |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:                         |  Actual Points:
Parent ID:                         |         Points:
 Reviewer:                         |        Sponsor:
-----------------------------------+--------------------------

Comment (by cmm323):

 Replying to [comment:8 dcf]:
 >
 > 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.

 Done.

 >
 > ----
 >
 > 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`?

 Let me know if you feel like this is still needed.
 >
 > {{{
 > +       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 removed it.


 >
 > {{{
 > +       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.

 Done.

 >
 > > 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]).

 Done.

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