Opened 20 months ago

Closed 5 weeks ago

#18628 closed defect (fixed)

Devise some way for the browser proxy to forward metadata to the bridge before the OR data

Reported by: arlolra Owned by: cmm323
Priority: High Milestone:
Component: Obfuscation/Snowflake Version:
Severity: Normal Keywords:
Cc: dcf, serene, cmm32 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

What does a user in that graph refer to? A connection from a unique ip? Part of the Flashproxy/Snowflake model is that the proxies will be ephemeral (and hard to block) so a single user session may rotate through several proxies, corresponding to several unique ips. I can imagine that messing with the numbers.

Connections from multiple ephemeral IP address won't cause a problem for counting the total number of users. As Karsten says, that figure is driven by clients' consensus downloads, which aren't related to the specifics of the connection.

What *will* get messed up is if we report the proxy IP addresses as if they were client IP addresses. Then we'll be counting the countries of the browser proxies, not of real users. My thinking was that for the time being, we just ignore that issue by not reporting an IP address through the ExtORPort. I.e.,

pt.DialOr(&ptInfo, "", "snowflake")
// https://godoc.org/git.torproject.org/pluggable-transports/goptlib.git#DialOr

In this way, we will not get per-country info, but the total number of snowflake users will be correct.

In order to report true client IP addresses, we will need to devise some way for the browser proxy to forward that metadata to the bridge before the OR data.

Child Tickets

Change History (18)

comment:1 in reply to:  description Changed 4 months ago by dcf

Cc: cmm32 added
Priority: MediumHigh

Replying to arlolra:

In order to report true client IP addresses, we will need to devise some way for the browser proxy to forward that metadata to the bridge before the OR data.

I realized a good way to do this: put the client IP address in the WebSocket URL. Currently we have

new WebSocket("wss://snowflake.bamsoftware.com/")

We could just change that to (imagine proper escaping):

new WebSocket("wss://snowflake.bamsoftware.com/?client_ip=" + client_ip)

The WebSocket server can extract the IP address by inspecting the URL it gets in the request, and provide that IP address to the pt.DialOr function.

The alternative of sending the client IP address in an HTTP header à la meek won't work, because the WebSocket API doesn't provide a way to set headers. The only information you can provide to the constructor is a URL and an optional list of sub-protocol names.

Unfortunately the WebSocket implementation used by snowflake-server (the one inherited from flash proxy) doesn't expose the URL of the client request (and in fact requires the path to be `/`). But that shouldn't be hard to change.

comment:2 Changed 4 months ago by dcf

Status: newneeds_review

Hooman is working on patches to address this.

websocket patch to retain the request URL and remove the restriction on paths:

https://gitweb.torproject.org/pluggable-transports/websocket.git/commit/?h=bug18628&id=0446621630483a656f179963ca77451f04aeaf01

snowflake patch to pass the client_ip from the websocket request URL query into pt.DialOr:

https://gitweb.torproject.org/user/dcf/snowflake.git/commit/?h=bug18628&id=0c958d8ffba0c117fb4768133afcbd4d56f61d1f

comment:3 Changed 4 months ago by dcf

Status: needs_reviewneeds_revision

Review of the patches from comment:2:

It's better if the WebSocket struct exposes the entire client http.Request structure, not just Request.URL. That way, consumers can also inspect the headers etc. Compare with Conn.Request() in the x/net websocket package. (You can make it a simple member access, doesn't have to be a function call.)

Completely delete the path check in websocket, don't just comment it out.

Run go fmt.

About the client address:

  • There should be some validation of client_ip, such as parsing with net.ParseIP, before passing the string into tor.
  • The ExtORPort USERADDR command is documented to take an addr:port string, not just an IP address. So snowflake-server needs to add a dummy port number (using net.JoinHostPort) before giving the string to tor. Alternately, rename client_ip to client_addr and have it contain the entire addr:port string.
    • If tor is accepting a plain IP address for USERADDR, it's a bug in tor or in the documentation, and we need to file a separate bug.
  • How does client_ip handle IPv6 addresses? We need to decide whether IPv6 addresses will have square brackets (if the port is included, then yes; if the port is not included, then probably no) and document it at least in a comment.

comment:4 Changed 4 months ago by dcf

Hooman has also made a commit that makes proxy-go parse the remote address from the PeerConnection SDP string, and send it to the websocket server as a client_ip query item.

https://gitweb.torproject.org/user/dcf/snowflake.git/commit/?h=bug18628&id=abc944ea876a7ba1de2f95d12a78a629805ecc85

comment:5 Changed 4 months ago by dcf

Review of the patch from comment:4:

@@ -50,6 +50,7 @@ type webRTCConn struct {
	dc *webrtc.DataChannel
	pc *webrtc.PeerConnection
	pr *io.PipeReader
+	client_ip string
 }
...
@@ -234,7 +235,22 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc.
				panic("short write")
			}
		}
-		conn := &webRTCConn{pc: pc, dc: dc, pr: pr}
+		conn := &webRTCConn{pc: pc, dc: dc, pr: pr, client_ip: clientIP}

Rather than adding a new member to the webRTCConn struct, it's better to put the IP address extraction logic in the RemoteAddr function. Not only is that what RemoteAddr is for, it will make it easier to write unit test code when the logic is isolated in its own function.

It's better if the remote IP address is represented as a net.IPAddr, rather than a string. You can get a net.IPAddr from a string using net.ParseIP. You can turn it back into a string (so you can add it to the URL query string) by calling String() on it.


-	wsConn, err := websocket.Dial(opt.relay, "", opt.relay)
+	wsConn, err := websocket.Dial(opt.relay + "?client_ip=" + conn.client_ip, "", opt.relay)

It's better to build the URL using net/url functions, rather than concatenating strings. That way, we know that escaping will be correct, and it's easier to extend if we want to set additional values in the query string. Something like this:

	u, err := url.Parse(opt.relay)
	if err != nil {
		panic(err)
	}
	u.Query().Set("client_ip", conn.client_ip)
	wsConn, err := websocket.Dial(u.String(), "", opt.relay)

+		//Parse Remote SDP to pass client IP to the server
+		var clientIP string = ""
+		remoteSDP := pc.RemoteDescription().Sdp
+		splitSDP := strings.Split(remoteSDP, "\r\n")
+		for i := range splitSDP {
+			if strings.HasPrefix(splitSDP[i], "c=") {
+				candidSplit := strings.Split(splitSDP[i], " ")
+				if len(candidSplit) >= 3 {
+					clientIP = candidSplit[2]
+					break
+				}
+			}
+		}

We need to think about what happens when this code fails to extract an IP address. Currently it will leave clientIP set to "", which will cause the websocket URL to be wss://snowflake.bamsoftware.com/?client_ip=. Maybe that's what we want, or maybe we want to omit the client_ip parameter completely in that case. What do you think?

In any case, this code should be moved into webRTCConn.RemoteAddr, like I said in my first note. That function can return nil when there's no match found.

comment:6 Changed 4 months ago by cmm32

Status: needs_revisionneeds_review
  • Most comments addressed. Note client IP address is now added to opt.relayURL before dialing websocket.

comment:7 in reply to:  6 Changed 4 months ago by dcf

Replying to cmm32:

I opened #23080 for the USERADDR documentation inconsistency. It looks like, regardless of what the spec says, it'll be fine to pass in an address with or without a port number, and with or without square brackets for IPv6.

comment:8 in reply to:  6 ; Changed 4 months ago by dcf

Replying to cmm32:

  • Most comments addressed.

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


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 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 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 (I just did that on the master branch).

comment:9 in reply to:  8 Changed 4 months ago by dcf

Replying to dcf:

Replying to cmm32:
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 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.

Okay, I applied a small workaround: just calls conn.RemoteAddr() before entering the goroutine instead of inside. I wonder if there is some kind of locking around RemoteDescription that was making it hang when called from different threads.

comment:10 Changed 3 months ago by dcf

Status: needs_reviewneeds_revision

I added support for client_ip to the JavaScript proxy in the bug18628 branch:

https://gitweb.torproject.org/user/dcf/snowflake.git/diff/?h=bug18628&id=b2be72724a1924bace6af18904a9e53f9094a5fd&id2=1d6109a9eddd4e2d967efbde8aba89b4226aefc2

Comment is welcome. It uses the same technique of parsing RTCPeerConnection.remoteDescription.sdp, because I couldn't find a better way.

cmm32, reminder that we're looking for a few revisions from comment:8, particularly making the tests pass for remoteIPFromSDP.

comment:11 Changed 3 months ago by cmm323

Owner: set to cmm323
Status: needs_revisionassigned

comment:12 in reply to:  8 ; Changed 2 months ago by cmm323

Replying to 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 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 (I just did that on the master branch).

Done.

comment:13 in reply to:  12 Changed 2 months ago by dcf

Replying to cmm323:

Replying to dcf:

In websocket:

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.

Great, merged all the websocket changes to master here:
https://gitweb.torproject.org/pluggable-transports/websocket.git/log/?id=e0bb5efd8d78d372711652ec061923debe7f5cb0


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

Yes, I think it's important. My worry is that if browsers change their SDP so that it is still within the spec but doesn't match what our code expects, we'll suddenly lose a lot of statistics without realizing it. I think the code doesn't need much more tweaking to make go test pass.

Here's JS code that passes the same tests:
https://gitweb.torproject.org/user/dcf/snowflake.git/tree/proxy/util.coffee?h=bug18628#n115


The other changes in the snowflake branch look good. Please also merge/rebase with my commits at https://gitweb.torproject.org/user/dcf/snowflake.git/log/?h=bug18628 (which include the tests for SDP parsing).

git remote add dcf https://git.torproject.org/user/dcf/snowflake.git
git fetch dcf
git merge dcf bug18628 # or rebase, deal with conflicts

comment:14 Changed 2 months ago by cmm323

The other changes in the snowflake branch look good. Please also merge/rebase with my commits at ​https://gitweb.torproject.org/user/dcf/snowflake.git/log/?h=bug18628 (which include the tests for SDP parsing).

git remote add dcf https://git.torproject.org/user/dcf/snowflake.git
git fetch dcf
git merge dcf bug18628 # or rebase, deal with conflicts

Done in private repo. You can push to your repo.

Last edited 2 months ago by cmm323 (previous) (diff)

comment:15 in reply to:  14 ; Changed 2 months ago by dcf

Replying to cmm323:

The other changes in the snowflake branch look good. Please also merge/rebase with my commits at ​https://gitweb.torproject.org/user/dcf/snowflake.git/log/?h=bug18628 (which include the tests for SDP parsing).

git remote add dcf https://git.torproject.org/user/dcf/snowflake.git
git fetch dcf
git merge dcf bug18628 # or rebase, deal with conflicts

Done in private repo. You can push to your repo.

Perfect. Pushed in 421bc583687.

Now all that remains is to make remoteIPFromSDP pass its tests. I have an idea for doing that. Let me know if you want me to do it or if you want to do it yourself.

comment:16 in reply to:  15 Changed 6 weeks ago by dcf

Replying to dcf:

Now all that remains is to make remoteIPFromSDP pass its tests. I have an idea for doing that. Let me know if you want me to do it or if you want to do it yourself.

Here's the change (uses regexp like the JS code does):
https://gitweb.torproject.org/user/dcf/snowflake.git/commit/?h=bug18628&id=319119a6117e43a11297ce24cae498c29087f6d4

Rebased and merged to master:
https://gitweb.torproject.org/pluggable-transports/snowflake.git/diff/?id=9e5eb7f5ee1c15122425ab4fe47fa6ad6ec75f92&id2=82d7f16babcdacdd120465fdd80b3468ea03246c

The new code is running live now, the server and six proxy instances at snowflake.bamsoftware.com. So we should start seeing the first uploaded descriptors with country-level information within a day or so.

comment:17 Changed 6 weeks ago by dcf

Here's the last bridge-extra-info descriptor before we pushed the code to the bridge (excerpted):

@type bridge-extra-info 1.3
extra-info flakey 5481936581E23D2D178105D44DB6915AB06BFB7F
dirreq-stats-end 2017-10-13 17:04:29 (86400 s)
dirreq-v3-ips ??=8
dirreq-v3-reqs ??=16
dirreq-v3-resp ok=16,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=24,busy=0
637461,q3=644613,d8=722983,d9=919679,max=1219266
transport snowflake
bridge-stats-end 2017-10-13 17:04:40 (86400 s)
bridge-ips ??=8
bridge-ip-versions v4=8,v6=0
bridge-ip-transports snowflake=8

Here are the first few after pushing the change. So far there are users from Canada (that was us testing), Morocco, Russia, Taiwan, the United States, and Turkey. The numbers are rounded to a multiple of 8, so 8 can be anything between 1 and 8. There are still some ?? entries—maybe those are from the JavaScript proxy, where we haven't pushed the updated code yet.

@type bridge-extra-info 1.3
extra-info flakey 5481936581E23D2D178105D44DB6915AB06BFB7F
dirreq-stats-end 2017-10-15 19:33:55 (86400 s)
dirreq-v3-ips ??=8,ca=8,cn=8,ma=8,ru=8,tw=8,us=8
dirreq-v3-reqs ??=8,ca=8,cn=8,ma=8,ru=8,tw=8,us=8
dirreq-v3-resp ok=32,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=40,busy=0
transport snowflake
bridge-stats-end 2017-10-15 19:34:09 (86400 s)
bridge-ips ??=8,ca=8,cn=8,ma=8,ru=8,tw=8,us=8
bridge-ip-versions v4=16,v6=0
bridge-ip-transports snowflake=16
@type bridge-extra-info 1.3
extra-info flakey 5481936581E23D2D178105D44DB6915AB06BFB7F
dirreq-stats-end 2017-10-16 19:33:55 (86400 s)
dirreq-v3-ips ??=8,ma=8,ru=8,tr=8,us=8
dirreq-v3-reqs ??=8,ma=8,ru=8,tr=8,us=8
dirreq-v3-resp ok=24,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=0,busy=0
transport snowflake
bridge-stats-end 2017-10-16 19:34:09 (86400 s)
bridge-ips ??=8,cn=8,ma=8,ru=8,tr=8,us=8
bridge-ip-versions v4=8,v6=0
bridge-ip-transports snowflake=8

comment:18 Changed 5 weeks ago by dcf

Resolution: fixed
Status: assignedclosed

I added some logging code that reports, every 24 hours, how many incoming connections had client_ip set and how many did not. Here are the first two reports (they are not 24 hours apart because I had to restart the server in between). The fractions are 81% and 82%.

2017/10/20 07:03:00 in the past 9e+04 s, 782/968 connections had client_ip
2017/10/22 15:19:03 in the past 86400 s, 507/618 connections had client_ip

We have 3 standalone proxy-go instances running around the clock, reporting client_ip. I would guess that the connections without client_ip come from the JavaScript code, the live copy of which does not yet have the client_ip change. (See #23947.) If my guess is right, and we assume that a client has an equal probability of getting any available proxy, that means that there are on average 0.75 JavaScript proxies running at any time (3 / 3.75 = 80%).

I'm going to close this ticket now. Upgrading the JavaScript proxy code will happen as a side effect of #23947.

Note: See TracTickets for help on using tickets.