Hi Alec,
Overall it looks nice. I didn't know that SOCKS4 only included a header
in each direction, thus of course your approach is simpler than creating
a new mux.
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
> + yin...@best.com or yin...@esd.sgi.com
(...)
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.
> 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.
> 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.
> 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.
> +/* 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.
> /* 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 ;-)
> /*
> * 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.
> + 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).
> 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.
> 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).
> @@ -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.
> + 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).
> +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.
> + if (ret > 0) {
> + conn->addr.socks_proxy.status = CO_SOCKS_RECEIVING;
> + if (ret == 8){
Same here.
(...)
> + 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.
> + 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.
> 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.
Thanks!
Willy
