Basic client support for SOCKS version 5 (no authentication)

[dmap]

Issue: https://issues.apache.org/jira/browse/HTTPCORE-563

[ok2c]

@dmap My first impression is very positive. Changes look concise and non-intrusive. I have one conceptual question though. Was there any particular reason for putting SOCKS protocol logic into InternalChannel instead of a custom IOEventHandler (similarly to ClientHttpProtocolNegotiator)?

[dmap]

@dmap My first impression is very positive. Changes look concise and non-intrusive. I have one conceptual question though. Was there any particular reason for putting SOCKS protocol logic into InternalChannel instead of a custom IOEventHandler (similarly to ClientHttpProtocolNegotiator)?

No, no particular reason. I am not that familiar with the codebase (which is pretty large) and this was the simplest way I found to implement the feature. I am happy for it to be done another way.

What I will say is that the SOCKS protocol negotiation is VERY low level. It has to happen on the wire before anything else... before TLS negotiation, before HTTP connects etc. I found it difficult to navigate my way through the source code well enough to achieve this any other way besides using the InternalChannel approach.

It is theoretically possible to want to establish an SSL connection to SOCKS proxy, then do the SOCKS negotiation, but in practice virtually no SOCKS proxy servers support this. That could be a feature for another day... It could mean negotiating SSL with the SOCKS proxy and then again (over the same connection) to the target server (tunnelled through the SOCKS proxy). What fun that would be. In practice it is unnecessary because if you are negotiating SSL with the target server all the traffic is secured end-to-end anyway, the SOCKS proxy just tunnels it through unchanged.

[ok2c]

No, no particular reason.

I understand the codebase is large and mostly undocumented. I will not insist but it would be nice to factor the SOCKS protocol logic out into a IOEventHandler The basic idea is to have individual IOEventHandler do their job, like HTTP protocol negotiation or SOCKS handshake, and then pass control to another IOEventHandler.

[garydgregory]

Hi @dmap
Thank you for your contribution!
How about adding some unit tests?
Gary

[dmap]

I will not insist but it would be nice to factor the SOCKS protocol logic out into a IOEventHandler The basic idea is to have individual IOEventHandler do their job, like HTTP protocol negotiation or SOCKS handshake, and then pass control to another IOEventHandler.

I attempted this approach. It worked fine for plain connections but for SSL connections the SSL handling triggered in the InternalDataChannel got in the way before the IOEventHandler implementation got a chance to do its job. As I mentioned previously the SOCKS negotiation needs to happen before the TLS/SSL handshaking, not after. Without refactoring of InternalDataChannel it isn't possible to use an IOEventHandler to do the SOCKS negotiation.

[dmap]

Hi @dmap
Thank you for your contribution!
How about adding some unit tests?
Gary

Will do. I still have to also add the SOCKS protocol username/password support. The PR isn't finished yet, I just raised it to get some initial feedback.

[ok2c]

Without refactoring of InternalDataChannel it isn't possible to use an IOEventHandler to do the SOCKS negotiation.

@dmap I would prefer to refactor InternalDataChannel in order to accomplish that if required. Please note, though, that InternalDataChannel can be upgraded to TLS at any point of its life time by calling #startTls method. All InternalDataChannel start off as plain and get upgraded to TLS at a later point. All you might need to do is to delay TLS upgrade until SOCKS handshake has been completed.

[dmap]

@ok2c I have spent several hours looking at refactoring InternalDataChannel to achieve what we want... it is possible but I think the result will be worse than what we have with the InternalConnectSocksChannel. The problem is that we need to delay several actions (the completion of the IOSessionRequest currently in InternalConnectChannel and the triggering of the connected call on the IOEventHandler in InternalDataChannel) that are triggered upon the establishment of the connection until after the SOCKS protocol negotiation is complete. In order to do this we either need to put a bunch of special case logic into InternalDataChannel just for SOCKS or invent some mechanism to allow the IOEventHandler to signal the connection has been made. But that feels backwards, the IOEventHandler currently gets told when the connection is established, not the other way around. It seems awkward to change this just to implement SOCKS, and I can't imagine any future use cases. SOCKS is somewhat unique in that it is purely a connection time enhancement. Basically everything else, even SSL or HTTP/2 etc is a protocol on top of an established TCP/IP connection.

I'm not trying to be difficult but after spending some time on it I honestly feel the InternalConnectSocksChannel is the better way. As you have observed the implementation is nicely contained in one place and cleanly integrated into the existing framework.

[ok2c]

@dmap Do you mind also committing your implementation IOEventHandler? I would like to take a stab at refactoring the code and see if I get anywhere.

[dmap]

Do you mind also committing your implementation IOEventHandler? I would like to take a stab at refactoring the code and see if I get anywhere.

@ok2c Updated the IOEventHandler impl to also support username/password auth and pushed as you requested.

The immediate problem is the completion of the IOSessionRequest on line 89 of InternalConnectChannel triggers the starting of the SSL negotiation via a callback. This needs to be somehow prevented in the SOCKS case (fairly easy with just a magic flag) and then actually completed when SOCKS is finished. I couldn't find an elegant solution to this second step, you essentially need some way of letting the IOEventHandler signal to the InternalDataChannel the connection is established so the right steps can be done. It is certainly possible, I just thought it more intrusive than the current approach.

[dmap]

@ok2c @garydgregory So I have just pushed a set of tests for the new code. These are reasonably extensive in that they simply inherit from the existing integration tests but using a SOCKS proxy... and yes that means I wrote a simple SOCKS proxy server for the test framework. It means we get all the current integration tests for Http1 and Http2 run again, but through a SOCKS proxy.

These tests all pass perfectly using the InternalConnectSocksChannel implementation. If you switch to the SocksProxyProtocolHandler implementation instead a large number of them fail with all sorts of strange behaviour. I expected the SSL ones to fail, but many of the plain HTTP tests also fail with various things hanging and timing out. That means there is probably a race condition in there somewhere, but I can't see where. It might not be in my code, like I said the InternalConnectSocksChannel works fine.

I feel my work here is essentially done. I would love to see this merged into the codebase, but that ball is in your court now. I can quite happily drive my own testing from my own fork.

[ok2c]

The immediate problem is the completion of the IOSessionRequest on line 89 of InternalConnectChannel triggers the starting of the SSL negotiation via a callback.

I am quite certain this should be fixable. HttpClient 5.0, for instance, is capable of upgrading tunneled connection to SSL/TLS after a CONNECT handshake. We should be able to do something similar here.

[ok2c]

I feel my work here is essentially done. I would love to see this merged into the codebase, but that ball is in your court now.

I'll get your code committed. You have my word on that. I would like to cut a BETA release soon, though, and would not want to publish SOCKS support too early without a bit more API polish.

[ok2c]

@dmap David, I think I have resolved the problem with TLS upgrade with 5e88e51. I moved TLS upgrade logic from IOSession request callback to individual IOEventHandler factories where it should have been put from the start. Could you please take another look at making SOCKS handshake work as a part of IOEventHandler chain?

[ok2c]

@dmap Another minor thing. Could you please remove @author tags. Per ASF policy there should be no authorship attribution in the source code. The contribution will be attributed to the original author in the commit message.

[dmap]

@ok2c ok I will have another look. It might take me a few days but I will get to it. My household has been hit by a terrible stomach flu, we have several very unwell people here at the moment...

[ok2c]

@dmap Please take care of your family. This can wait.

[dmap]

@ok2c So this is now working... but I'm not totally happy with it. There are a couple of strange interactions required between the SocksProxyProtocolHandler and the InternalDataChannel to have things work.

1. The only way to have the connected() method called on the next event handler in the chain at the right time for both the SSL and non-SSL cases was to have the InternalDataChannel do it (because it knows if there is a tlsSession or not), so I had to create the reconnectHandler() method to reset the state of the connected flag so it would actually make the call. The InternalDataChannel had already called connected() on the SocksProxyProtocolHandler and so didn't think it needed to call it again.

2. There is something awkward about the handling of the selection key interest ops. Specifically IOSessionImpl.enqueue() adds OP_WRITE to the current event mask, but the current IO event handler may have no ability to process the enqueued command. It took me some time to work out why my SocksProxyProtocolHandler was suddenly getting outputReady callbacks when it hadn't specified any interest in writing output. I think it would be cleaner to fire some other type of event when a command is queued and let the current IO event handler make the appropriate changes to the interest ops.

[dmap]

I see my tests fail with Java 11. I will investigate.

[dmap]

Hah, so the failing tests were caused by the haphazard management of the interest ops. The ClientHttpProtocolNegotiator didn't set the OP_WRITE event on the session even when it knew it had a preface to write out... so if it didn't happen to be set already then it didn't get written out.

[ok2c]

@dmap I was able to get rid of reconnectHandler ugliness with d6b9205. I still would like to understand why ClientHttpProtocolNegotiator needed those changes but it looks like we are almost there.

See my latest changes here
https://github.com/ok2c/httpcore/commits/feature/HTTPCORE-563

[ok2c]

@dmap It all makes sense. Please take one last glance at the final change-set here and I'll commit it as soon as I get a nod from you
https://github.com/ok2c/httpcore/commit/693f9273d8f856af2328cd0c0cb45833f49d45cf

[dmap]

@ok2c I'm happy with that. Thanks for the help.

The changes in ClientHttpProtocolNegotiator were required because it obviously needs to write content (when it needs to write the preface) but never set the OP_WRITE flag on the session. This used to work 'by accident' because of the way that an enqueued command automatically sets the OP_WRITE flag. But when the SOCKS protocol handler runs it also sets and clears this flag several times, and when finished leaves it cleared (because the last thing it does is read the SOCKS connect repsonse). So without explicitly setting the flag in the ClientHttpProtocolNegotiator it could hang forever waiting to write the preface. I reproduced this in some of the unit tests under Java 11.

This is an example of what I referred to as the awkward handling of the interest ops. I believe that in general only the IOEventHandlers should set the interest ops because they are the only actual consumers of the IO events. In particular I don't like the way the enqueue command sets the OP_WRITE flag, it should instead fire a different event that an interested IOEventHandler can listen for and then the IOEventHandler can set OP_WRITE if it wishes.

Because of this awkward management of the interest ops an IOEventHandler can receive an inputReady() or (in particular) an outputReady() callback when it is not expecting it. I had to add additional code to my SOCKS handler to avoid an infinite loop when I got an outputReady() (because of the enqueueCommand done by another thread) when the SOCKS handler was actually wanting input, not output.

Anyway, that is all probably best done as a different change. It wouldn't be hard to clean up, but it isn't strictly speaking required for this PR.

[dmap]

@ok2c So, to be clear, I am happy for this to be committed.

[ok2c]

Committed as 42d992f. Please review and close.

[dmap]

Great