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.
> > > + /* 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.
> > > + 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.
Thanks,
Willy
