Hi Willy, Thank for the look into it. Here are some of my thoughts about your comments, pls check below.
> I do however have some comments, mainly about the way used to retrieve > configuration elements as at the moment it doubles the memory consumption > of each connection, but there are solutions to fix this that I describe > below. > > > diff --git a/doc/SOCKS4.protocol.txt b/doc/SOCKS4.protocol.txt > > new file mode 100644 > > index 00000000..16483dee > > --- /dev/null > > +++ b/doc/SOCKS4.protocol.txt > > @@ -0,0 +1,150 @@ > > + SOCKS: A protocol for TCP proxy across firewalls > > + > > + Ying-Da Lee > > + [email protected] or [email protected] > > (...) > > I'm seeing that you copied a doc retrieved from somewhere else that is > found at various places on the net. Have you checked the license for > this doc to be sure we can copy and distribute it like this ? It might > be perfectly fine but we need to be sure. In any case we'll have to > mention the license if it differs from the other ones. I have the "SOCKS: A protocol for TCP proxy across firewalls" doc found at openssh's website under OpenSSH Sprcifications along with those RFCs. "https://www.openssh.com/specs.html";, "https://www.openssh.com/txt/socks4.protocol"; And OpenSSH is using GPL, so I believe it should be fine. > > > diff --git a/examples/socks4.cfg b/examples/socks4.cfg > > new file mode 100644 > > index 00000000..22cbad44 > > --- /dev/null > > +++ b/examples/socks4.cfg > > (...) > > + server SMTPS1_Via_SocksProxy1 169.38.103.42:25 check inter 30000 > > fastinter 1000 > > + server SMTPS2_Via_SocksProxy2 161.202.148.179:25 socks4 > > 127.0.0.1:1080 check-via-socks check inter 30000 fastinter 1000 backup > > These ones are real mail servers which do respond on the net, it's not > really friendly nor recommended to put real server addresses there, you > can use the 192.0.2.0/24 range instead, which is reserved for > documentation. Will have it changed. > > > diff --git a/include/types/checks.h b/include/types/checks.h > > index f89abcba..b09b5f89 100644 > > --- a/include/types/checks.h > > +++ b/include/types/checks.h > > @@ -189,6 +189,8 @@ struct check { > > char *sni; /* Server name */ > > char *alpn_str; /* ALPN to use for checks */ > > int alpn_len; /* ALPN string length */ > > + > > + int via_socks; /* check the connection via > > socks proxy */ > > }; > > At some point we'll have to think about creating a bit field for all > these booleans because we're progressively inflating the struct by > 32 bits every time we need a new flag. But let's do this later and > not annoy you with this for now. Totally agree, thx. > > > struct check_status { > > diff --git a/include/types/connection.h b/include/types/connection.h > > index 308be7d7..2a1b23b4 100644 > > --- a/include/types/connection.h > > +++ b/include/types/connection.h > > @@ -47,6 +47,15 @@ struct server; > > struct session; > > struct pipe; > > > > +/* socks4 upstream proxy definitions */ > > +struct socks4_request { > > + uint8_t version; /* SOCKS version number, 1 byte, must be 0x04 > > for this version */ > > + uint8_t command; /* 0x01 = establish a TCP/IP stream > > connection */ > > + uint16_t port; /* port number, 2 bytes (in network byte > > order) */ > > + uint32_t ip; /* IP address, 4 bytes (in network byte > > order) */ > > + char user_id[8]; /* the user ID string, variable length, > > terminated with a null (0x00); Using "HAProxy\0" */ > > +}; > > + > > /* Note: subscribing to these events is only valid after the caller has > > really > > * attempted to perform the operation, and failed to proceed or complete. > > */ > > @@ -156,7 +165,7 @@ enum { > > > > CO_FL_EARLY_SSL_HS = 0x00004000, /* We have early data pending, > > don't start SSL handshake yet */ > > CO_FL_EARLY_DATA = 0x00008000, /* At least some of the data are > > early data */ > > - /* unused : 0x00010000 */ > > + CO_FL_SOCKS_HS = 0x00010000, /* handshaking with upstream SOCKS > > proxy */ > > /* unused : 0x00020000 */ > > From what I'm seeing later in the code, it's not exactly a handshake, > you have to send a header and to wait for the response header to arrive. > I'd rather have one flag per direction which save from having to deal > with state management. Actually, I have the same idea as you, using two flags only. But ends up with this design later on. The reason I am doing it in this way, that's because: 1) I might want to add the socks5 protocol support onto it later on, which will become a little more complex, not just one request and response. We will get something like authentication method handshake, login, connection request, etc. 2) Looks like do not have much unused bits leftover within CO_FL_*. > > > +/* flags for use in connection->socks_proxy.status */ > > +enum { > > + CO_SOCKS_NONE, > > + CO_SOCKS_SENDING, > > + CO_SOCKS_SENT, > > + CO_SOCKS_RECEIVING, > > + CO_SOCKS_READY, > > + CO_SOCKS_DENY > > +}; > > In my opinion these ones are easily addressed with 2 bits as mentioned > above. The error status can be used for the deny case upon receipt. I will try to have the error status into conn->err_code. > > > /* possible connection error codes */ > > enum { > > @@ -451,6 +469,13 @@ struct connection { > > struct { > > struct sockaddr_storage from; /* client address, or address > > to spoof when connecting to the server */ > > struct sockaddr_storage to; /* address reached by the > > client, or address to connect to */ > > + struct { > > + unsigned char use; /* socks4 > > proxy enabled */ > > + struct sockaddr_storage addr; /* the > > address of the socks4 proxy */ > > + struct socks4_request req_line; /* the info > > send to socks4 proxy */ > > + unsigned int status; /* CO_SOCKS_* > > */ > > + signed short req_remain_len; /* the length > > remain to sent */ > > + } socks_proxy; > > It is extremely huge for the struct connection unfortunately. We're > currently trying to shrink it, ideally extracting the address field > which is large, and this doubles it. With the two handshake flags > you can cover the status. The "use" has to be found and enabled > based on the server, and the address is fixed so it can also be passed > from the server. The request line could be rebuilt on each attempt as > we do with the proxy protocol header. Maybe the only part remaining > would be req_remain_len, which could probably be merged with the > proxy protocol offset later since we don't need to maintain both at > once. If you can use the PP offset now it's even better ;-) What if we just have a reference here, and using malloc to alloc some memory for those connections which are going to use socks proxy only. It will be NULL for those connections which won't need it. When the socks proxy handshake finished, we can have this part of memory freed, before turn it over for data usage. I will try to look into it, if we can reuse the "proxy protocol offset", if we want to keep the ability to allow using PROXY Protocol and socks proxy at the same time. > > > /* > > * Linux seems to be able to send 253 fds per sendmsg(), not sure > > * about the other OSes. > > diff --git a/include/types/server.h b/include/types/server.h > > index 7835f11c..7a383d44 100644 > > --- a/include/types/server.h > > +++ b/include/types/server.h > > @@ -338,6 +338,9 @@ struct server { > > char reason[128]; > > } op_st_chg; /* operational status > > change's reason */ > > char adm_st_chg_cause[48]; /* administrative status > > change's cause */ > > + > > + unsigned char use_socks4; /* socks4 proxy enabled */ > > There seems to be plenty of unused flags left in the server flags, look > at SRV_F_USE_NS_FROM_PP for example to see how to get another one. Also > and generally speaking, please be careful about alignment, as just > placing one char above, followed by a 8- or 16-byte aligned struct results > in a 7- or 15- bytes hole, and all these holes do have a cost on memory > usage, and performance via cache line thrashing. Understand, will take care of it. > > > + struct sockaddr_storage socks4_addr; /* the address of the socks4 > > proxy, including the port */ > > }; > > > > /* Descriptor for a "server" keyword. The ->parse() function returns 0 in > > case of > > diff --git a/src/backend.c b/src/backend.c > > index d7695bf2..52b74e7c 100644 > > --- a/src/backend.c > > +++ b/src/backend.c > > @@ -888,6 +888,18 @@ int assign_server_address(struct stream *s, struct > > connection *srv_conn) > > srv_conn->addr.to = __objt_server(s->target)->addr; > > set_host_port(&srv_conn->addr.to, > > __objt_server(s->target)->svc_port); > > > > + if (__objt_server(s->target)->use_socks4) { > > + srv_conn->addr.socks_proxy.use = 1; > > + srv_conn->addr.socks_proxy.addr = > > __objt_server(s->target)->socks4_addr; > > + srv_conn->addr.socks_proxy.status = CO_SOCKS_NONE; > > + srv_conn->addr.socks_proxy.req_line.version = 0x04; > > + srv_conn->addr.socks_proxy.req_line.command = 0x01; > > + srv_conn->addr.socks_proxy.req_line.port = > > get_net_port(&(srv_conn->addr.to)); > > + srv_conn->addr.socks_proxy.req_line.ip = > > is_inet_addr(&(srv_conn->addr.to)); > > + strcpy(srv_conn->addr.socks_proxy.req_line.user_id, > > "HAProxy"); > > + srv_conn->addr.socks_proxy.req_remain_len = > > sizeof(srv_conn->addr.socks_proxy.req_line); > > + } > > So for me here instead you should just set on the connection the flags > for the send and the recv part of the SOCKS4 protocol, and set the send > offset to 0 (for example), so that the function in charge for sending > the header knows where to restart from. All the rest is available from > these functions : > - the server struct that you need to retrieve the socks4 address > and port is accessible via objt_server(conn->target) > - the constants (version, command, user_id, ...) can be hard-coded there > - the req_line's ip/port can be taken extracted from conn->addr.to > in these functions like you do here > > Thus none of these fields needs to be set here in the end (which is a > huge memory saver). Good to know we can use this "objt_server(conn->target)" to get the server reference, it can save a lot of memory. Will try to improve it. > > > diff --git a/src/checks.c b/src/checks.c > > index 76bd8e3d..2809fbe2 100644 > > --- a/src/checks.c > > +++ b/src/checks.c > > @@ -1611,6 +1611,25 @@ static int connect_conn_chk(struct task *t) > > conn->addr.to = s->addr; > > } > > > > + if (s->check.via_socks) { > > + if (s->use_socks4) { > > + conn->addr.socks_proxy.use = 1; > > + conn->addr.socks_proxy.addr = s->socks4_addr; > > + conn->addr.socks_proxy.status = CO_SOCKS_NONE; > > + conn->addr.socks_proxy.req_line.version = 0x04; > > + conn->addr.socks_proxy.req_line.command = 0x01; > > + conn->addr.socks_proxy.req_line.port = > > get_net_port(&(conn->addr.to)); > > + conn->addr.socks_proxy.req_line.ip = > > is_inet_addr(&(conn->addr.to)); > > + strcpy(conn->addr.socks_proxy.req_line.user_id, > > "HAProxy"); > > + conn->addr.socks_proxy.req_remain_len = > > sizeof(conn->addr.socks_proxy.req_line); > > + > > + conn->flags |= CO_FL_SOCKS_HS; > > + } > > Exact same principle here. Got it. > > > diff --git a/src/connection.c b/src/connection.c > > index 2a66996b..b7ec820c 100644 > > --- a/src/connection.c > > +++ b/src/connection.c > > @@ -25,6 +25,7 @@ > > #include <proto/proto_tcp.h> > > #include <proto/stream_interface.h> > > #include <proto/sample.h> > > +#include <proto/log.h> > > > > #ifdef USE_OPENSSL > > #include <proto/ssl_sock.h> > > @@ -72,6 +73,21 @@ void conn_fd_handler(int fd) > > if (unlikely(conn->flags & CO_FL_ERROR)) > > goto leave; > > > > + if (conn->flags & CO_FL_SOCKS_HS) { > > + if (conn->addr.socks_proxy.status == CO_SOCKS_NONE || > > conn->addr.socks_proxy.status == CO_SOCKS_SENDING) { > > + //Going to send the request to the socks4 > > proxy > > + conn_send_socks_proxy_request(conn); > > + goto leave; > > + } > > + > > + if (conn->addr.socks_proxy.status == CO_SOCKS_SENT || > > conn->addr.socks_proxy.status == CO_SOCKS_RECEIVING) { > > + //Check the response from the socks4 proxy > > + if (!conn_recv_socks_proxy_response(conn, > > CO_FL_SOCKS_HS)) { > > + goto leave; > > + } > > + } > > + } > > + > > So here instead of coding the state machine there based on the status, > you can simply call each individual function depending on a single flag > for each (say CO_FL_SEND_SOCKS4, CO_FL_RECV_SOCKS4). Just try to make it easier when working with socks5. > > > @@ -966,6 +982,145 @@ int conn_recv_netscaler_cip(struct connection *conn, > > int flag) > > return 0; > > } > > > > + > > +int conn_send_socks_proxy_request(struct connection *conn) > > +{ > > + /* we might have been called just after an asynchronous shutw */ > > + if (conn->flags & CO_FL_SOCK_WR_SH) > > + goto out_error; > > + > > + if (!conn_ctrl_ready(conn)) > > + goto out_error; > > + > > Here you'd start by rebuilding the request line into a temporary local > buffer based on the same elements you used for the checks and for the > server's connect. You know the value is stable over time so you can > redo it and it will give you the same result. You just have to restart > sending from the send offset. Got it looks like you preference memory over CPU time, :-). > > > + while (conn->addr.socks_proxy.req_remain_len) { > > + int ret = 0; > > + > > + conn->addr.socks_proxy.status = CO_SOCKS_SENDING; > > + > > + /* we are sending the socks4_req_line here. If the data layer > > + * has a pending write, we'll also set MSG_MORE. > > + */ > > + ret = conn_sock_send(conn, > > + ((char > > *)(&(conn->addr.socks_proxy.req_line))) + > > + > > (sizeof(conn->addr.socks_proxy.req_line)-(conn->addr.socks_proxy.req_remain_len)), > > + conn->addr.socks_proxy.req_remain_len, > > + (conn->flags & CO_FL_XPRT_WR_ENA) ? > > MSG_MORE : 0); > > + > > + ha_alert("SOCKS PROXY HS FD[%04X]: Before send remain is > > [%d], sent [%d]\n", conn->handle.fd, conn->addr.socks_proxy.req_remain_len, > > ret); > > By the way, I'm assuming that for now this is for debugging, but I just > wanted you to be aware that ha_alert() and ha_warning() are only meant > to be used during config parsing and not run time as they will most > often not be visible due to stdout/stderr being closed and/or redirected > to something else. If you need to emit some errors at run time, please > raise CO_FL_ERROR and set an error code among CO_ER_* into > conn->err_code. You may add other codes but then please add the human > readable version into proto/connection.h (grep for CO_ER_SSL_RENEG for > example). Got it. What if I want to have some trace debug info, which way will you recommend. > > > +int conn_recv_socks_proxy_response(struct connection *conn, unsigned int > > flag) > > +{ > (...) > > + read_count = 0; > > + do { > > + /* SOCKS4 Proxy request granted server response, 0x00 | 0x5A > > | 0x00 0x00 | 0x00 0x00 0x00 0x00 > > + * Try to peek into it, before all 8 bytes of response ready. > > + */ > > + ret = recv(conn->handle.fd, line+read_count, 8-read_count, > > MSG_PEEK); > > Please use sizeof(line) instead of the hard-coded 8 here. I can > guarantee you that your code will end up being copy-pasted or updated > and that someone will end up with something larger than 8 at one point > and you'll get 8-something resulting in an integer overflow for the > limit. Got it. > > > + if (ret > 0) { > > + conn->addr.socks_proxy.status = CO_SOCKS_RECEIVING; > > + if (ret == 8){ > > Same here. Check. > (...) > > > + if (ret < 0) { > > + if (errno == EINTR) { > > + continue; > > + } > > + if (errno == EAGAIN) { > > + fd_cant_recv(conn->handle.fd); > > + return 0; > > + } > > + goto fail; > > + } else { > > + read_count += ret; > > + } > > + } while (read_count < 8); > > This will loop forever due to MSG_PEEK. You must not loop when doing > this. EAGAIN will only be returned as long as there is zero byte in, > but as soon as you have between 1 and 7 bytes you'll be stuck there. > Better simply return and deal with this just like EAGAIN. I think it > is what is already done in the PP/CIP receive code. Get your point. Will try to fix. > > > + if (line[1] != 0x5A) { > > Would be nice to have this 0x5A defined somewhere as a more or less > explicit macro or enum so that whoever reads the code has an idea what > it's supposed to mean exactly. We've had tons of hard-coded values in > the peers code and it's one of the most undebuggable parts of the code. > > > + conn->addr.socks_proxy.status = CO_SOCKS_DENY; > > + conn->flags &= ~flag; > > So here it could be turned to CO_FL_ERROR with CO_ER_SOCKS_DENY for > example. > > > + /* remove the 8 bytes response from the stream */ > > + do { > > + ret = recv(conn->handle.fd, line, 8, 0); > > Just as a reminder, "8" spotted here. > > > + if (ret < 0 && errno == EINTR) { > > + continue; > > + } > > + if (ret != 8) { > > and here. Got it. > > > diff --git a/src/proto_tcp.c b/src/proto_tcp.c > > index d4cad223..54949533 100644 > > --- a/src/proto_tcp.c > > +++ b/src/proto_tcp.c > > @@ -293,6 +293,7 @@ int tcp_connect_server(struct connection *conn, int > > data, int delack) > > struct server *srv; > > struct proxy *be; > > struct conn_src *src; > > + int ret; > > > > conn->flags |= CO_FL_WAIT_L4_CONN; /* connection in progress */ > > > > @@ -496,7 +497,12 @@ int tcp_connect_server(struct connection *conn, int > > data, int delack) > > if (global.tune.server_rcvbuf) > > setsockopt(fd, SOL_SOCKET, SO_RCVBUF, > > &global.tune.server_rcvbuf, sizeof(global.tune.server_rcvbuf)); > > > > - if (connect(fd, (struct sockaddr *)&conn->addr.to, > > get_addr_len(&conn->addr.to)) == -1) { > > + if (conn->addr.socks_proxy.use) { > > + ret = connect(fd, (struct sockaddr > > *)&conn->addr.socks_proxy.addr, get_addr_len(&conn->addr.socks_proxy.addr)); > > + } else { > > + ret = connect(fd, (struct sockaddr *)&conn->addr.to, > > get_addr_len(&conn->addr.to)); > > + } > > This is where you can use the connection's flags instead : > > const struct sockaddr_storage *addr = &conn->addr.to; > > (...) > if (conn->flags & CO_FL_SEND_SOCKS4 && srv) > addr = &srv->socks4_addr; > if (connect(fd, (struct sockaddr *)addr, get_addr_len(addr)) == -1) { > (...) > > And that's about all. I think that the design is light enough to be > merged, I'm not seeing any significant showtopper. Thank you once again for the reviewing, and all the detail comment, which is very helpful for me to get better understand of the project. > > Thanks! > Willy Regards, Alexander Liu
