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 <w...@1wt.eu> 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
