goptlib should expose a SOCKS5 server instead of SOCKS4a.

added component::circumvention/pluggable transport goptlib owner::yawning priority::medium resolution::fixed severity::normal socks status::closed type::defect labels

Trac:
goptlib-socks5-draft.diff

First pass implementation (needs tests).

Trac:
goptlib-socks5-draft2.diff

Take 2 at the implementation.

Ok, this still needs tests but it resolves most of the things in first revision that I was unhappy about by using the magic of goroutines and channels.

Now, each SocksListener has a goroutine that handles accepting new connections, and dispatches another goroutine that does the SOCKS handshake and places the SocksConn into a channel. Accept consumes off the channel instead (so can't fail unless trying to Accept on a SocksListener that has been closed). Error handling is done entirely by the goroutine(s).

This also fixes a bug inherited from the original code. If either of the SetDeadline calls in AcceptSocks fail (unlikely), the file descriptor will be leaked (Unless the GC will happen to take care of closing them).

Eratta: Line 136 should be ln.wg.Wait().

I'm not sure if I should be using a mutex + waitgroup to make the close process race condition free or if there's a more idiomatic channel based way to do this.

I really want this complete before obfs4proxy sees use by the general public, so adding the obfs4 deployment bug as a parent so I don't forget.

Trac:
Parent: N/A to #12130 (moved)

Trac:
goptlib-socks5-draft3.diff

Take 3, with some bugs fixed.

The previous versions had some brain damage that would cause a deadlock on Close(), that I fixed because I've been using this code for my obfs4 testing. Seems to work ok, once I'm done with the obfs4 refactor I'll add unit tests to this. Sorry for the delay.

Trac:
goptlib-socks5-draft4.diff

Now with unit tests (stolen from obfsproxy), and minor bugfixes.

Ok, I cribbed the obfsproxy socks5 unit tests and fixed some minor bugs and cleaned up behavior in a few places. The SOCKS5 protocol handling code should have reasonable coverage now, though the goroutine/channel related goo does not (not sure what the best way to test that sort of thing is).

Trac:
Status: new to needs_review

Replying to yawning:

This also fixes a bug inherited from the original code. If either of the SetDeadline calls in AcceptSocks fail (unlikely), the file descriptor will be leaked (Unless the GC will happen to take care of closing them).

Could you fix this bug in a separate commit? (Just go ahead and commit it.) It's important to me to keep bug fixes separate from new features. This way the bug ends up getting recorded in the commit history too.

I'm taking a look at this patch now. Do you have have a branch that's factored into separate commits? I don't find it in your github nor in git.torproject.org/user/yawning.

Nice patch. It suffers somewhat from a lack of separation of concerns—not everything is directly related to SOCKS5 support. The biggest thing is the handling of concurrent connections, which is a new feature, and should be committed separately, if at all. Apart from that I just have some minor questions. I haven't looked at the connection logic yet nor the tests. See my comments below.

Please avoid rearranging function order and adding blank lines to existing functions. It's fine to make those kinds of changes directly on the trunk; they shouldn't show up in a feature branch.

 // The package implements a SOCKS4a server sufficient for a Tor client transport
 // plugin.
 //
 // http://ftp.icm.edu.pl/packages/socks/socks4/SOCKS4.protocol

Put a URL to good SOCKS5 documentation in pt.go. I guess RFC 1928.

+	socksVersionString = "socks5"
...
+// Returns "socks5", suitable to be included in a call to Cmethod.
+func (ln *SocksListener) Version() string {
+	return socksVersionString
+}

Let's remove socksVersionString and put the magic string inside the function.

+	// the password string sent by the client.
+	Password string

Capitalize the comment.

 // Send a message to the proxy client that access to the given address is
-// granted. If the IP field inside addr is not an IPv4 address, the IP portion
-// of the response will be four zero bytes.
+// granted.  For interface backwards compatibility reasons, this does not set
+// BND.ADDR/BND.PORT correctly, however very few if any clients examine the
+// values of this field.
 func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
-	return sendSocks4aResponseGranted(conn, addr)
+	// Addr in the SOCKS 4 code was the destination address, which is not sent
+	// in SOCKS 5.
+	return sendSocks5ResponseGranted(conn, nil)
 }

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

-// Send a message to the proxy client that access was rejected or failed.
+// Send a message to the proxy client that access was rejected or failed.  This
+// method exists for backwards compatibility and will only send the "General
+// Failure" error code.
 func (conn *SocksConn) Reject() error {
-	return sendSocks4aResponseRejected(conn)
+	return conn.RejectReason(SocksRepGeneralFailure)
+}
+
+// Send a message to the proxy client that access was rejected with the
+// specified reason.
+func (conn *SocksConn) RejectReason(reason byte) error {
+	return sendSocks5ResponseRejected(conn, reason)
 }

Adding a RejectReason function is fine. It does tie us to SOCKS5 forever, but realistically we're not ever likely to change from SOCKS5. Don't characterize plain Reject as being for backward compatibility only; just document that it sends a general error; I don't think people should be required to use a specific code if they don't care.

Here's related reading on the whole net.Error/err.Temporary thing (i.e. "Does this happen ever?"). https://gitweb.torproject.org/pluggable-transports/goptlib.git/commitdiff/3030f080eecf72b0e896236fca5fabd245c00bdb

The most troubling part of the patch is the new complex code surrounding AcceptSocks. First, thank you for taking the time to explore different ways of handling concurrent connections in a non-blocking way. However, if you leave that part out, the SOCKS5 code works just as well as the SOCKS4a code did, right? That is, it blocks a second connection while the first is still pending, but otherwise works correctly as long as SOCKS handshakes finish quickly? If so, then the new additional concurrency really needs to be in a separate patch. It's not intrinsically related to SOCKS5, and in fact you could make the same kind of enhancement to the SOCKS4a code as it stands.

So I'd like to ask you to remove the new concurrency code and thereby simplify the patch. Even then, we should spend some more time thinking about the best way to implement the concurrency feature.

 type SocksListener struct {
 	net.Listener
+	sync.Mutex
+
+	isClosed bool
+	ch       chan *SocksConn
+	wg       sync.WaitGroup
 }

A Mutex, a WaitGroup, a new boolean flag—there has to be a simpler way. If there's not a simpler way, maybe the new feature isn't worth it. Correctness is a virtue, but simplicity is also a virtue. In particular, you seem to be doing a lot of work to ensure that you can do a RejectReason to all concurrently connecting clients—I'm not sure that's worth the infrastructure required. We should aim for an implementation that does not require overloading the Close method in SocksListener.

Trac:
Status: needs_review to needs_revision

Replying to dcf:

Nice patch. It suffers somewhat from a lack of separation of concerns—not everything is directly related to SOCKS5 support. The biggest thing is the handling of concurrent connections, which is a new feature, and should be committed separately, if at all. Apart from that I just have some minor questions. I haven't looked at the connection logic yet nor the tests. See my comments below.

Snipping stuff that's "ok, will fix".

 // Send a message to the proxy client that access to the given address is
-// granted. If the IP field inside addr is not an IPv4 address, the IP portion
-// of the response will be four zero bytes.
+// granted.  For interface backwards compatibility reasons, this does not set
+// BND.ADDR/BND.PORT correctly, however very few if any clients examine the
+// values of this field.
 func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
-	return sendSocks4aResponseGranted(conn, addr)
+	// Addr in the SOCKS 4 code was the destination address, which is not sent
+	// in SOCKS 5.
+	return sendSocks5ResponseGranted(conn, nil)
 }
}}}

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

Despite what certain things say (eg: Wikipedia), it's the local address/port of the outgoing socket from the SOCKS server to the destination.

{{{ In the reply to a CONNECT, BND.PORT contains the port number that the server assigned to connect to the target host, while BND.ADDR contains the associated IP address. The supplied BND.ADDR is often different from the IP address that the client uses to reach the SOCKS server, since such servers are often multi-homed.


It's presumably done this way because clients can't call `getsockname()` on connections through a proxy, but in practice not many SOCKS clients require or use this information.  We could go and modify the application code that is affected by the switch to send back the right address as well.

> The most troubling part of the patch is the new complex code surrounding AcceptSocks. First, thank you for taking the time to explore different ways of handling concurrent connections in a non-blocking way. However, if you leave that part out, the SOCKS5 code works just as well as the SOCKS4a code did, right? That is, it blocks a second connection while the first is still pending, but otherwise works correctly as long as SOCKS handshakes finish quickly? If so, then the new additional concurrency really needs to be in a separate patch. It's not intrinsically related to SOCKS5, and in fact you could make the same kind of enhancement to the SOCKS4a code as it stands.
> 
> So I'd like to ask you to remove the new concurrency code and thereby simplify the patch. Even then, we should spend some more time thinking about the best way to implement the concurrency feature.
> {{{
>  type SocksListener struct {
>  	net.Listener
> +	sync.Mutex
> +
> +	isClosed bool
> +	ch       chan *SocksConn
> +	wg       sync.WaitGroup
>  }
> }}}
> A Mutex, a WaitGroup, a new boolean flag—there has to be a simpler way. If there's not a simpler way, maybe the new feature isn't worth it. Correctness is a virtue, but simplicity is also a virtue. In particular, you seem to be doing a lot of work to ensure that you can do a RejectReason to all concurrently connecting clients—I'm not sure that's worth the infrastructure required. We should aim for an implementation that does not require overloading the Close method in SocksListener.

It's kind of complicated because on ln.Close(), the channel of pending connections needs to get flushed since there will be no further AcceptSocks() calls by the application code.  The "clean" thing to do is to fail the pending requests gracefully, though rudely closing off all the connections that are waiting pending Accept() is also an option.

I'd be ok with removing the concurrency support, since the "incorrect" code will work well enough for our purposes (handshakes should get to the point where the command was received rather quickly).

Replying to yawning:

Replying to dcf:

{{{ // Send a message to the proxy client that access to the given address is -// granted. If the IP field inside addr is not an IPv4 address, the IP portion -// of the response will be four zero bytes. +// granted. For interface backwards compatibility reasons, this does not set +// BND.ADDR/BND.PORT correctly, however very few if any clients examine the +// values of this field. func (conn *SocksConn) Grant(addr *net.TCPAddr) error {

return sendSocks4aResponseGranted(conn, addr)

// Addr in the SOCKS 4 code was the destination address, which is not sent

// in SOCKS 5.

return sendSocks5ResponseGranted(conn, nil) } }}}

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

Despite what certain things say (eg: Wikipedia), it's the local address/port of the outgoing socket from the SOCKS server to the destination.

{{{ In the reply to a CONNECT, BND.PORT contains the port number that the server assigned to connect to the target host, while BND.ADDR contains the associated IP address. The supplied BND.ADDR is often different from the IP address that the client uses to reach the SOCKS server, since such servers are often multi-homed. }}}

It's presumably done this way because clients can't call getsockname() on connections through a proxy, but in practice not many SOCKS clients require or use this information. We could go and modify the application code that is affected by the switch to send back the right address as well.

Oh fascinating. That does seem pretty useless for our purposes. I think you made the right call with your design. In the function comment, state explicitly that addr is ignored. I think I would remove the special handling for addr == nil in sendSocks5Response and push emptyAddr into the callers, just for the sake of explicitness. And in fact, it looks like you could just hard-code an IPv4 emptyAddr in sendSocks5Response and remove the addr parameter, as there doesn't seem to be any external way to cause a non-nil addr to be set. You can make the call.

I decoupled the concurrency changes from the SOCKS5 code and started using github to stage goptlib stuff.

https://github.com/Yawning/goptlib/compare/bug12535

Sorry for the changes around the function formerly know as readSocks4aConnect looking a bit tangled in the compare output, SOCKS5 processing is a bit more involved so there's more code (and the handshake process got split into multiple functions to reflect what SOCKS5 needs to do), and it confuses diff.

The tests were updated to account for no longer being able to send back responses with non-zero address/port (and my tests pass), and obfs4proxy works with my branch.

On a side note: I may be able to simplify the concurrency stuff a bit after thinking about it but if I ever decide to poke at that again, I'll file a separate ticket for it.

Trac:
Status: needs_revision to needs_review

Ok, this branch apparently has gotten a silly amount of testing because the psiphon people forked my github repo and have been using it for a while. Since we haven't merged it yet, I made this branch to apply it as a patch when we build Tor Browser. Ideally we can ship 4.5a5 with this, and get field testing on our own.

Would one of the kind tor browser people please pull this in? <3

https://github.com/Yawning/tor-browser-bundle/compare/bug12535

Trac:
Cc: dcf to dcf, gk
Keywords: N/A deleted, tbb-4.5-alpha, TorBrowserTeam201503 added

goptlib should expose a SOCKS5 server instead of SOCKS4a.

Child items 0

Activity