Hi Alec, On Thu, May 23, 2019 at 12:43:54AM +0800, Alec Liu wrote: > Finally got sometime to have the patch revised.
And finally got some time to review it :-) > I am reusing the "send_proxy_ofs" right now. OK. > Due to it was used as some kind of flag for send Proxy Protocol in some > places. > I have to replace those usage to check the srv->pp_opts. Hmmm this will break health checks then, since you don't know whether a connection is made for data traffic or for a check. That makes me think that we've been quite annoyed for a while with these problems of finding all relevant connection parameters, and that maybe it could be useful to think about defining a "connection settings" structure with a lot of stuff that we want to use in various situations. A server would then have 2 or 3 of these (one for regular traffic or dataplane, one for health checks, one for agent checks). We could then use this for the source bindings, TLS/PP options etc. When setting up a connection, we'd just have to pass a pointer to the desired structure to provide all the settings. This definitely is something to think about for 2.1. > From 6767c73a40a9d242cce061cc7feaf480ad416314 Mon Sep 17 00:00:00 2001 > From: Alexander Liu <[email protected]> > Date: Wed, 22 May 2019 19:44:48 +0800 > Subject: [PATCH] MEDIUM: connection: Upstream SOCKS4 proxy support > > Have "socks4" and "check-via-socks4" server keyword added. > Implement handshake with SOCKS4 proxy server for tcp stream connection. > See issue #82. > > I have the "SOCKS: A protocol for TCP proxy across firewalls" doc found > at openssh's website under OpenSSH Sprcifications along with other RFCs, > at "https://www.openssh.com/specs.html";, > and "https://www.openssh.com/txt/socks4.protocol";. > > OpenSSH is using GPL, so I believe it should be fine to have the doc here. ^^^^^^^^^^^^^^^^^^^^^ It's written exactly the opposite on their site and in the license file :-) LICENSE: "OpenSSH contains no GPL code." Well, I think you'll have to find the license covering the doc (probably BSD), include it in the doc/ directory and mention in the LICENSE file that this doc is provided under that license. Or we consider that it's too much hassle for just a protocol specification and you simply put a link to a stable URL in the commit message, which will be much simpler (and will also support fixes to the spec, that a copy doesn't provide). > diff --git a/include/proto/connection.h b/include/proto/connection.h > index ede372638..915be87bf 100644 > --- a/include/proto/connection.h > +++ b/include/proto/connection.h > @@ -60,6 +60,10 @@ int conn_sock_send(struct connection *conn, const void > *buf, int len, int flags) > /* drains any pending bytes from the socket */ > int conn_sock_drain(struct connection *conn); > > +/* scoks4 proxy handshake */ > +int conn_send_socks4_proxy_request(struct connection *conn); > +int conn_recv_socks4_proxy_response(struct connection *conn); > + > /* returns true is the transport layer is ready */ > static inline int conn_xprt_ready(const struct connection *conn) > { > @@ -889,6 +893,11 @@ static inline const char *conn_err_code_str(struct > connection *c) > case CO_ER_SSL_HANDSHAKE_HB: return "SSL handshake failure after > heartbeat"; > case CO_ER_SSL_KILLED_HB: return "Stopped a TLSv1 heartbeat attack > (CVE-2014-0160)"; > case CO_ER_SSL_NO_TARGET: return "Attempt to use SSL on an unknown > target (internal error)"; > + > + case CO_ER_SOCKS4_SEND: return "SOCKS4 Proxy write error during > handshake"; > + case CO_ER_SOCKS4_RECV: return "SOCKS4 Proxy read error during > handshake"; > + case CO_ER_SOCKS4_DENY: return "SOCKS4 Proxy deny the request"; ^^^^ denied > + case CO_ER_SOCKS4_ABORT: return "SOCKS4 Proxy handshake aborted by > server"; > } > return NULL; > } > diff --git a/include/types/connection.h b/include/types/connection.h > index be590f098..9a362f05d 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. > */ > @@ -155,8 +164,8 @@ 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 */ > - /* unused : 0x00020000 */ > + CO_FL_SOCKS4_SEND = 0x00010000, /* handshaking with upstream SOCKS4 > proxy, going to send the handshake */ > + CO_FL_SOCKS4_RECV = 0x00020000, /* handshaking with upstream SOCKS4 > proxy, going to check if handshake succeed */ > > /* flags used to remember what shutdown have been performed/reported */ > CO_FL_SOCK_RD_SH = 0x00040000, /* SOCK layer was notified about > shutr/read0 */ > @@ -182,7 +191,7 @@ enum { > CO_FL_ACCEPT_CIP = 0x08000000, /* receive a valid NetScaler Client > IP header */ > > /* below we have all handshake flags grouped into one */ > - CO_FL_HANDSHAKE = CO_FL_SEND_PROXY | CO_FL_SSL_WAIT_HS | > CO_FL_ACCEPT_PROXY | CO_FL_ACCEPT_CIP, > + CO_FL_HANDSHAKE = CO_FL_SEND_PROXY | CO_FL_SSL_WAIT_HS | > CO_FL_ACCEPT_PROXY | CO_FL_ACCEPT_CIP | CO_FL_SOCKS4_SEND | CO_FL_SOCKS4_RECV, > > /* when any of these flags is set, polling is defined by socket-layer > * operations, as opposed to data-layer. Transport is explicitly not > @@ -205,8 +214,10 @@ enum { > * must be done after clearing this flag. > */ > CO_FL_XPRT_TRACKED = 0x80000000, > -}; > > + /* below we have all SOCKS handshake flags grouped into one */ > + CO_FL_SOCKS4 = CO_FL_SOCKS4_SEND | CO_FL_SOCKS4_RECV, > +}; 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. > diff --git a/include/types/server.h b/include/types/server.h > index 0d53d2600..219f5ab1d 100644 > --- a/include/types/server.h > +++ b/include/types/server.h > @@ -141,6 +141,7 @@ enum srv_initaddr { > #define SRV_F_AGENTADDR 0x0080 /* this server has a agent addr > configured */ > #define SRV_F_COOKIESET 0x0100 /* this server has a cookie > configured, so don't generate dynamic cookies */ > #define SRV_F_FASTOPEN 0x0200 /* Use TCP Fast Open to connect to > server */ > +#define SRV_F_SOCKS4_PROXY 0x0400 /* this server uses SOCKS4 proxy */ So you'll need a distinct flag for the checks. > diff --git a/src/backend.c b/src/backend.c > index 467ef57a9..09623e884 100644 > --- a/src/backend.c > +++ b/src/backend.c > @@ -1532,6 +1532,10 @@ int connect_server(struct stream *s) > } > > assign_tproxy_address(s); > + > + if (srv && (srv->flags & SRV_F_SOCKS4_PROXY)) { > + srv_conn->send_proxy_ofs = 1; > + } > } > else if (!conn_xprt_ready(srv_conn)) { > if (srv_conn->mux->reset) > diff --git a/src/checks.c b/src/checks.c > index d264aecf8..5dbfd0e9a 100644 > --- a/src/checks.c > +++ b/src/checks.c > @@ -1612,6 +1612,15 @@ static int connect_conn_chk(struct task *t) > conn->addr.to = s->addr; > } > > + if (s->check.via_socks4) { > + if (s->flags & SRV_F_SOCKS4_PROXY) { > + conn->send_proxy_ofs = 1; > + conn->flags |= CO_FL_SOCKS4_SEND; > + } > + }else{ > + conn->flags &= (~CO_FL_SOCKS4); > + } > + > proto = protocol_by_family(conn->addr.to.ss_family); > conn->target = &s->obj_type; > > diff --git a/src/connection.c b/src/connection.c > index 908d098e4..e0663534b 100644 > --- a/src/connection.c > +++ b/src/connection.c > @@ -27,6 +27,8 @@ > #include <proto/sample.h> > #include <proto/ssl_sock.h> > > +#include <common/debug.h> > + > DECLARE_POOL(pool_head_connection, "connection", sizeof(struct connection)); > DECLARE_POOL(pool_head_connstream, "conn_stream", sizeof(struct > conn_stream)); > > @@ -69,6 +71,16 @@ void conn_fd_handler(int fd) > if (unlikely(conn->flags & CO_FL_ERROR)) > goto leave; > > + if (conn->flags & CO_FL_SOCKS4_SEND) { > + //Going to send the request to the socks4 proxy > + conn_send_socks4_proxy_request(conn); > + goto leave; > + } Here you're immediately quitting, I suppose it's because you're expecting to wait for a response ? In any case it's not correct, you should proceed like for others, i.e. "if (!conn_send_...) goto leave", and leave it to the next stage to decide to wait. Otherwise you risk to not even subscribe to receive events because you don't try. Also in some rare cases (localhost, VM, heavily loaded machine), you might be scheduled out for a while and have the response ready when you get back here and you want to be sure not to miss such events either. > + if (conn->flags & CO_FL_SOCKS4_RECV) > + if (!conn_recv_socks4_proxy_response(conn)) > + goto leave; > + > if (conn->flags & CO_FL_ACCEPT_CIP) > if (!conn_recv_netscaler_cip(conn, CO_FL_ACCEPT_CIP)) > goto leave; > @@ -959,6 +971,182 @@ int conn_recv_netscaler_cip(struct connection *conn, > int flag) > return 0; > } > > + > +int conn_send_socks4_proxy_request(struct connection *conn) > +{ > + struct socks4_request req_line; > + > + /* we might have been called just after an asynchronous shutw */ > + if (conn->flags & CO_FL_SOCK_WR_SH) > + goto out_error; I *think* (but could be wrong) that when you have CO_FL_ERROR, you also have the shutdown flag set. In this case be careful, as you're overwriting the error code which might already have been set by the layer which actually faced the error Either you first check for CO_FL_ERROR and you don't set your code when it's set, or you can decide to conditionally set your error code only when conn->err_code is not set. The former is cleaner, the latter is fine since already done at other places. > + if (!conn_ctrl_ready(conn)) > + goto out_error; It's probably the same here. > + req_line.version = 0x04; > + req_line.command = 0x01; > + req_line.port = get_net_port(&(conn->addr.to)); > + req_line.ip = is_inet_addr(&(conn->addr.to)); > + strcpy(req_line.user_id,"HAProxy"); Just a small nit, you can use memcpy(req_line.user_id, "HAProxy\0", 8) here instead, it will make it easier for the compiler to verify that it fits and will avoid a warning on systems where strcpy() systematically emits one. > + if (conn->send_proxy_ofs > 0) { > + /* > + * This is the first call to send the request > + */ > + conn->send_proxy_ofs = -((int)(sizeof(req_line))); > + } > + > + while (conn->send_proxy_ofs < 0) { > + int ret = 0; > + > + /* 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 *)(&req_line)) + > (sizeof(req_line)+conn->send_proxy_ofs), > + -conn->send_proxy_ofs, > + (conn->flags & CO_FL_XPRT_WR_ENA) ? MSG_MORE : > 0); > + > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: Before send remain is > [%d], sent [%d]\n", > + conn->handle.fd, -conn->send_proxy_ofs, ret); > + > + if (ret < 0) { > + goto out_error; > + } > + > + conn->send_proxy_ofs += ret; /* becomes zero once complete */ > + if (conn->send_proxy_ofs != 0) { > + goto out_wait; > + } > + > + /* 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). > + > + /* 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. > + /* mark it for receive the response */ > + __conn_sock_want_recv(conn); > + break; > + } > + > + if (conn->flags & CO_FL_SEND_PROXY) { > + /* > + * Get the send_proxy_ofs ready for the send_proxy due to we are > + * reusing the "send_proxy_ofs", and SOCKS4 handshake should be > done > + * before sending PROXY Protocol. > + */ > + conn->send_proxy_ofs = 1; looks good. > + } > + return 1; > + > + out_error: > + /* Write error on the file descriptor */ > + conn->flags |= CO_FL_ERROR; > + conn->err_code = CO_ER_SOCKS4_SEND; > + return 0; > + > + out_wait: > + __conn_sock_stop_recv(conn); > + return 0; > +} > + > +int conn_recv_socks4_proxy_response(struct connection *conn) > +{ > + char line[SOCKS4_HS_RSP_LEN]; > + int ret; > + > + /* we might have been called just after an asynchronous shutr */ > + if (conn->flags & CO_FL_SOCK_RD_SH) > + goto fail; > + > + if (!conn_ctrl_ready(conn)) > + goto fail; > + > + if (!fd_recv_ready(conn->handle.fd)) > + return 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, SOCKS4_HS_RSP_LEN, MSG_PEEK); > + > + if (ret == 0) { > + /* the socket has been closed or shutdown for send */ > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: Received > ret[%d], errno[%d], looks like the socket has been closed or shutdown for > send\n", > + conn->handle.fd, ret, errno); > + conn->err_code = CO_ER_SOCKS4_RECV; > + goto fail; > + } > + > + if (ret > 0) { > + if (ret == SOCKS4_HS_RSP_LEN){ > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: > Received 8 bytes, the response is [%02X|%02X|%02X %02X|%02X %02X %02X > %02X]\n", > + conn->handle.fd, line[0], > line[1], line[2], line[3], line[4], line[5], line[6], line[7]); > + }else{ > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: > Received ret[%d], first byte is [%02X], last bye is [%02X]\n", > conn->handle.fd, ret, line[0], line[ret-1]); > + } > + } else { > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: Received > ret[%d], errno[%d]\n", conn->handle.fd, ret, errno); > + } > + > + if (ret < 0) { > + if (errno == EINTR) { > + continue; > + } > + if (errno == EAGAIN) { > + fd_cant_recv(conn->handle.fd); > + return 0; > + } > + goto recv_abort; > + } > + } while (0); > + > + if (ret < SOCKS4_HS_RSP_LEN) { > + /* Missing data. Since we're using MSG_PEEK, we can only poll > again if > + * we have not able to read enough data. > + */ > + fd_cant_recv(conn->handle.fd); > + return 0; > + } > + > + 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). > + conn->flags &= (~CO_FL_SOCKS4_RECV); > + > + DPRINTF(stderr, "SOCKS PROXY HS FD[%04X]: FAIL, the response is > [%02X|%02X|%02X %02X|%02X %02X %02X %02X]\n", > + conn->handle.fd, line[0], line[1], line[2], > line[3], line[4], line[5], line[6], line[7]); > + conn->err_code = CO_ER_SOCKS4_DENY; > + goto fail; > + } > + > + /* remove the 8 bytes response from the stream */ > + do { > + ret = recv(conn->handle.fd, line, SOCKS4_HS_RSP_LEN, 0); > + if (ret < 0 && errno == EINTR) { > + continue; > + } > + if (ret != SOCKS4_HS_RSP_LEN) { > + conn->err_code = CO_ER_SOCKS4_RECV; > + goto fail; > + } > + } while (0); > + > + conn->flags &= (~CO_FL_SOCKS4_RECV); ^ ^ > + return 1; > + > + recv_abort: > + conn->err_code = CO_ER_SOCKS4_ABORT; > + conn->flags |= (CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH); ^ ^ > + goto fail; > + > + fail: > + __conn_sock_stop_both(conn); > + conn->flags |= CO_FL_ERROR; > + return 0; > +} > + > /* Note: <remote> is explicitly allowed to be NULL */ > int make_proxy_line(char *buf, int buf_len, struct server *srv, struct > connection *remote) > { > diff --git a/src/proto_sockpair.c b/src/proto_sockpair.c > index 97a93480a..af77f17ef 100644 > --- a/src/proto_sockpair.c > +++ b/src/proto_sockpair.c > @@ -240,12 +240,19 @@ int send_fd_uxst(int fd, int send_fd) > static int sockpair_connect_server(struct connection *conn, int flags) > { > int sv[2], fd, dst_fd = -1; > + struct server *srv; > > /* the FD is stored in the sockaddr struct */ > dst_fd = ((struct sockaddr_in *)&conn->addr.to)->sin_addr.s_addr; > > - if (obj_type(conn->target) != OBJ_TYPE_PROXY && > - obj_type(conn->target) != OBJ_TYPE_SERVER) { > + switch (obj_type(conn->target)) { > + case OBJ_TYPE_PROXY: > + srv = NULL; > + break; > + case OBJ_TYPE_SERVER: > + srv = objt_server(conn->target); > + break; > + default: > conn->flags |= CO_FL_ERROR; > return SF_ERR_INTERNAL; > } > @@ -289,7 +296,7 @@ static int sockpair_connect_server(struct connection > *conn, int flags) > } > > /* if a send_proxy is there, there are data */ > - if (conn->send_proxy_ofs) > + if (srv && srv->pp_opts) So here it breaks the checks, they will always run with the same proxy protocol settings as the data traffic. I think that you should keep this test as-is since you're still sending something. > 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. > conn_ctrl_init(conn); /* registers the FD */ > diff --git a/src/proto_tcp.c b/src/proto_tcp.c > index 95068ee6c..d5471c8ef 100644 > --- a/src/proto_tcp.c > +++ b/src/proto_tcp.c > @@ -294,6 +294,7 @@ int tcp_connect_server(struct connection *conn, int flags) > struct proxy *be; > struct conn_src *src; > int use_fastopen = 0; > + int ret; > > conn->flags |= CO_FL_WAIT_L4_CONN; /* connection in progress */ > > @@ -492,7 +493,7 @@ int tcp_connect_server(struct connection *conn, int flags) > */ > if (flags & (CONNECT_DELACK_ALWAYS) || > ((flags & CONNECT_DELACK_SMART_CONNECT || > - (flags & CONNECT_HAS_DATA) || conn->send_proxy_ofs) && > + (flags & CONNECT_HAS_DATA) || (srv && srv->pp_opts)) && So same issue here with pp_opts. > (be->options2 & PR_O2_SMARTCON))) > setsockopt(fd, IPPROTO_TCP, TCP_QUICKACK, &zero, > sizeof(zero)); > #endif > @@ -514,7 +515,12 @@ int tcp_connect_server(struct connection *conn, int > flags) > 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 (srv->flags & SRV_F_SOCKS4_PROXY) { Same problem with no distinction between check and traffic. Please use conn->flags & CO_FL_SEND_SOCKS4 instead since it will already have been set as needed. > + ret = connect(fd, (struct sockaddr *)&(srv->socks4_addr), > get_addr_len(&(srv->socks4_addr))); > + } else { > + ret = connect(fd, (struct sockaddr *)&conn->addr.to, > get_addr_len(&conn->addr.to)); > + } > + if (ret == -1) { I haven't looked at the emitted code, but I suspect it will be cleaner if you add this : struct sockaddr_storage *addr; ... addr = (conn->flags & CO_FL_SEND_SOCKS4) ? &srv->socks4_addr : &conn->addr.to; if (connect(fd, (const struct sockaddr *)addr, get_addr_len(addr)) == -1) { ... > @@ -568,9 +574,13 @@ int tcp_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 comment as for the socketpair sockets here. > + /* Prepare to send socks proxy related handshakes */ > + if (srv && srv->flags & SRV_F_SOCKS4_PROXY) > + conn->flags |= CO_FL_SOCKS4_SEND; and here. > diff --git a/src/proto_uxst.c b/src/proto_uxst.c > index 42e0dc878..3f8213347 100644 > --- a/src/proto_uxst.c > +++ b/src/proto_uxst.c > @@ -510,7 +510,7 @@ static int uxst_connect_server(struct connection *conn, > int flags) > } > > /* if a send_proxy is there, there are data */ > - if (conn->send_proxy_ofs) > + if (srv && srv->pp_opts) > flags |= CONNECT_HAS_DATA; > > if (global.tune.server_sndbuf) > @@ -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 ? Thanks, Willy
