[sr-dev] [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)

Henning Westerholt notifications at github.com
Sat Dec 15 13:30:51 CET 2018


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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1765#pullrequestreview-185352625
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20181215/72104d01/attachment.html>


More information about the sr-dev mailing list