Hi Willy, Here is the updated patch, please help to look into it, thank you!
Regards, Alexander Liu On Thu, May 30, 2019 at 6:20 PM Alec Liu <[email protected]> wrote: > > Hi Willy, > > I am still doing some testing. Once finished, I will try to submit the > new patch ASAP. > Hope can get it done before Olivier's code merge, to save you some work load. > > On Thu, May 30, 2019 at 4:18 PM Willy Tarreau <[email protected]> wrote: > > > > Hi Alec, > > > > On Thu, May 30, 2019 at 01:15:47AM +0800, Alec Liu wrote: > > > Hi Willy, > > > > > > Thank you once again for all the detail reply. > > > > You're welcome. > > > > > > Just so that you know, Olivier is finishing his changes in this area to > > > > make SSL totally self-sustaining (will be needed for QUIC) so at some > > > > point we'll have a conflict here. He'll probably know how to adapt this > > > > however. > > > > > > I will try to read the code from Olivier for the SSL part , to learn > > > more about it. > > > > OK so I discussed about this with Olivier, as I'm about to merge his > > cleanups. What I'll propose you is to only address your issues WITHOUT > > rebasing your code, then send the updated patch. We'll manually rebase it > > on the modified code then so that you don't have to waste your time trying > > to figure where things moved. > > > > > > > + /* OK we've the whole request sent */ > > > > > + conn->flags &= (~CO_FL_SOCKS4_SEND); > > > > ^ ^ > > > > Please avoid these useless parenthesis above, I noticed them at a few > > > > places and they are even inconsistent (you don't have them below for > > > > example). > > > > > > > I just want to make it easier to read, "LEFTPART &= RIGHTPART", both > > > left and right will be a whole one, but > > > "LEFTPART &= RIGHTPART1 x RIGHTPART2 x RIGHTPART3". > > > > For me it's the opposite, since "&=" is an assignment, I don't see why > > the right part has to be enclosed and since this is not natural I visually > > flag it as suspicious. > Ok, I will try to follow it in the project. > > > > > > > > + /* Turn to receiving response */ > > > > > + conn->flags |= CO_FL_SOCKS4_RECV; > > > > > > > > This one should be set where you set CO_FL_SOCKS4_SEND because when > > > > you decide you'll send it, you also know you want to wait for the > > > > response. > > > > > > > Yep you are right, when it sending the socks4 request, it is waiting for > > > reply. > > > But if the sending part get failed, it won't going to expect for the > > > response, and > > > we should not getting anything before sending the request, if we do, > > > it should be something wrong. > > > > It's the same for all other handshake flags in fact. Whenever you face an > > error in any of them, the error is immediately raised and the other ones > > are not executed. > > > > > May be it should be ok to have both flags set at the same time for most > > > cases, but I think setting it separately will be fine too. > > > > It is trickier to figure which ones are expected when they are set > > dynamically. For example, during a connect() we could decide about some > > options depending on whether or not we expect to send only or to > > send+receive. That's why it's better to have them set right as soon > > as possible. > What will happen if the server just send you the correct response > without your request? > In the SOCKS4 proxy case, you get the 0x5A response, but you have not > send the request yet. > How can we identify it, and reject the response, mark it as error, and close > it? > > > > > > > > + if (line[1] != 0x5A) { > > > > > > > > Please either use #define/enums if this corresponds to something > > > > particular, > > > > or at least indicate in a comment what you're trying to verify (what's > > > > in > > > > line[1] and what 0x5A corresponds to). > > > Actually I have comment for it at " /* SOCKS4 Proxy request granted > > > server response, 0x00 | 0x5A | 0x00 0x00 | 0x00 0x00 0x00 0x00". > > > Anyway, I will make thing clearer here. > > > > Thanks :-) > > > > > > > flags |= CONNECT_HAS_DATA; > > > > > > > > > > if (global.tune.server_sndbuf) > > > > > @@ -315,7 +322,7 @@ static int sockpair_connect_server(struct > > > > > connection *conn, int flags) > > > > > conn->flags |= CO_FL_ADDR_TO_SET; > > > > > > > > > > /* Prepare to send a few handshakes related to the on-wire > > > > > protocol. */ > > > > > - if (conn->send_proxy_ofs) > > > > > + if (srv && srv->pp_opts) > > > > > conn->flags |= CO_FL_SEND_PROXY; > > > > > > > > And here, we should just move the CO_FL_SEND_PROXY flag setting to the > > > > place where send_proxy_ofs is set and remove this test. It looks like > > > > checks already do it correctly and that it's only needed in backend.c. > > > > Then with this it becomes clean : the one who wants to emit a header > > > > just sets his CO_FL_SEND_XXX flag with send_proxy_ofs=1, then each other > > > > one in turn will simply have to recheck this. We could even go slightly > > > > further and let the connect() functions set send_proxy_ofs to 1 > > > > themselves > > > > when any of the SEND flags are set so that the upper layer only has to > > > > focus on setting the appropriate flags. > > > > > > > > Please do that in a separate preliminary patch, which will allow to > > > > bisect later if some problems are reported after your patches are > > > > merged. > > > > > > > I will keep it this way first, and then try to submit another patch for > > > the idea > > > to make the "send_proxy_ofs" clearer. > > > > OK. That's something I can handle once Olivier's patchset is merged anyway. > > > > > > > @@ -566,7 +566,7 @@ static int uxst_connect_server(struct connection > > > > > *conn, int flags) > > > > > conn->flags |= CO_FL_ADDR_TO_SET; > > > > > > > > > > /* Prepare to send a few handshakes related to the on-wire > > > > > protocol. */ > > > > > - if (conn->send_proxy_ofs) > > > > > + if (srv && srv->pp_opts) > > > > > conn->flags |= CO_FL_SEND_PROXY; > > > > > > > > Same comments here. You didn't implement SOCKS4 over unix sockets ? > > > > Is there any particular reason or was it just forgotten ? > > > > > > Actually, the socks4 protocol is just for TCP normally. > > > > Well, for the destination, but there is no reason for the server not to be > > able to receive over a unix socket. You could even have a local unix socket > > handled by haproxy forwarding to a remote socks4 server, and use this local > > socket as your socks4 server everywhere. The protocol you connect with > > doesn't have to be the same you're trying to use on the end-to-end path. > > Just like you already support having your socks4 server over IPv6 while it > > probably only supports IPv4. > > To make thing simpler, I will just leave it with TCP first, due to do not have > any known SOCKS proxy implement running over unix socket, difficult to > do the testing. > > > > > Thanks, > > Willy > > Thank you. > > Regards, > Alexander Liu
0001-MEDIUM-connection-Upstream-SOCKS4-proxy-support.patch
Description: Binary data
