@miconda the `proxy` protocol was first defined by haproxy but there may be other programs that implement the protocol (not `haproxy` exclusive). i suggest `accept_proxy_protocol`

henningw commented on this pull request.

I Haven't tested it, but the parsing part looks like a solid and good implementation. I have added one small remark in the code, and have some general questions:

- You work on a local pointer src_ip and dst_ip in the parsing part, but directly on the c->rcv.src_port/c->rcv.dst_port - is there a particular reasons for it? - related to error cases, what would happen if you e.g. set in the IPv6 case 0x21 the dst_ip->af = AF_INET6, but later one run unto an error in the parsing. Then you would return an error to the callee, and would go to the read_ip_info path. Here the AF_INET6 would be still be set, or I am wrong? What about just setting all the data into a temporary variable, and then if the parsing was completed successfully, set it. - why you set the c->rcv.bind_address=ba; in line 1221?

• if (likely(local_addr)){

- su2ip_addr(&c->rcv.dst_ip, local_addr); - c->rcv.dst_port=su_getport(local_addr); - }else if (ba){ - c->rcv.dst_ip=ba->address; - c->rcv.dst_port=ba->port_no; + if (unlikely(ksr_tcp_accept_haproxy && state == S_CONN_ACCEPT)) { + ret = tcpconn_read_haproxy(c); + + if (ret == -1) { + LM_ERR("invalid PROXY protocol header\n"); + goto error; + } else if (ret == 1) { + LM_DBG("PROXY protocol did not override IP addresses\n"); + goto read_ip_info; + }

Small suggestion, add a LM_DBG output here, that the tcpconn_read_haproxy call was successful.

related to error cases, what would happen if you e.g. set in the IPv6 case 0x21 the dst_ip->af = AF_INET6, but later one run unto an error in the parsing. Then you would return an error to the callee, and would go to the read_ip_info path. Here the AF_INET6 would be still be set, or I am wrong?

I believe you might be misreading the code here. I've added a comment to clarify things.

There is only a single error case after the `0x21` case, and that's the inability to read from the socket. In this case, we return `-1`, which in the calling function would result in a `goto error;`, which terminates the connection. We only set the `*_ip->af` after we've done most of the v2 parsing. Same thing is true for the v1 parsing: once we get to the `TCP{4,6}` part of the line, only `0` or `-1` are possible outputs for the function.

Your comment did help me discover that I was missing the `LOCAL` case (line 1075), and mistakenly returned `0` in that case instead of `1`, so thanks for that.

You work on a local pointer src_ip and dst_ip in the parsing part, but directly on the c->rcv.src_port/c->rcv.dst_port - is there a particular reasons for it?

Convenience. There is only one pointer access for `c->rcv.src_port` and `c->rcv.dst_port`, whereas there's two pointer accesses in `c->rcv.src_ip->...`, and multiple fields are accessed in `{src,dst}_ip` (`af`, `len`, `u`, etc). I'm happy to use a local pointer if requested.

From my point of view, it can be merged.

Merged #1765 into master.