New feature to get the real source IP when kamailio websocket is behind a load balancer transmitting X-Forwarded-For and/or X-Real-IP headers: Detect HTTP headers X-Forwarded-For and X-Real-IP during the Websocket handshake. The pseudo variables $ws_real_src_ip is populated during ws_handshake() or null if not set. $ws_real_src_ip is indeed readonly. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1002
-- Commit Summary --
* websocket: X-Forwarded-For and X-Real-IP support
-- File Changes --
M src/modules/websocket/websocket.c (40) M src/modules/websocket/ws_conn.h (2) M src/modules/websocket/ws_handshake.c (51)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1002.patch https://github.com/kamailio/kamailio/pull/1002.diff
@miconda This patch is working for 5.x and master branch. I have also 2 backported patches for 4.4 and 4.3. Should I create 2 pull requests?
miconda commented on this pull request.
+ if ((wsc = wsconn_get(msg->rcv.proto_reserved1)) == NULL) { + LM_ERR("$ws_real_src_ip cannot retrieve websocket connection\n"); + return pv_get_null(msg, param, res); + } + + wscid = wsc->id; + + if (wsc->real_src_ip[0] == 0) { + wsconn_put(wsc); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is not set\n", wscid); + return pv_get_null(msg, param, res); + } + + LM_DBG("$ws_real_src_ip ws_conid %d wsc->real_src_ip is %s (%d)\n", wscid, wsc->real_src_ip, (int)strlen(wsc->real_src_ip)); + len = snprintf(buff, IP_ADDR_MAX_STR_SIZE, "%s", wsc->real_src_ip);
I noticed the use of snprintf(...) for storing just a single string value in another buffer. Is this chosen for any particular reason instead of strcpy()/strncpy()? Afaik, functions with variadic parameters tend to be slower.
- if (wsc->real_src_ip[0] == 0) {
+ wsconn_put(wsc); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is not set\n", wscid); + return pv_get_null(msg, param, res); + } + + LM_DBG("$ws_real_src_ip ws_conid %d wsc->real_src_ip is %s (%d)\n", wscid, wsc->real_src_ip, (int)strlen(wsc->real_src_ip)); + len = snprintf(buff, IP_ADDR_MAX_STR_SIZE, "%s", wsc->real_src_ip); + buff[len] = 0; + + wsconn_put(wsc); + + ip_str.s = (char *)buff; + ip_str.len = strlen(buff); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is %s (%d)\n", wscid, ip_str.s, ip_str.len); + return pv_get_strval(msg, param, res, &ip_str);
The need for ip_str variable can be avoided by using pv_get_strzval(msg, param, buf), if I haven't missed any other use of ip_str.
Backporting of new features in stable branches is not allowed, unless it is actually a fix or solving major security needs. Others can comment their opinion.
One more or less cosmetic opinion, I would make a class of variables, $ws(name), so if the future new fields can be added to it (like the implementation of $time(...) from pv modue). Also, for a short name of the attribute, maybe it can be used just 'orig_ip' instead of 'real_src_ip'.
tamiel commented on this pull request.
+ if ((wsc = wsconn_get(msg->rcv.proto_reserved1)) == NULL) { + LM_ERR("$ws_real_src_ip cannot retrieve websocket connection\n"); + return pv_get_null(msg, param, res); + } + + wscid = wsc->id; + + if (wsc->real_src_ip[0] == 0) { + wsconn_put(wsc); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is not set\n", wscid); + return pv_get_null(msg, param, res); + } + + LM_DBG("$ws_real_src_ip ws_conid %d wsc->real_src_ip is %s (%d)\n", wscid, wsc->real_src_ip, (int)strlen(wsc->real_src_ip)); + len = snprintf(buff, IP_ADDR_MAX_STR_SIZE, "%s", wsc->real_src_ip);
Right strncpy should do the job here.
tamiel commented on this pull request.
- if (wsc->real_src_ip[0] == 0) {
+ wsconn_put(wsc); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is not set\n", wscid); + return pv_get_null(msg, param, res); + } + + LM_DBG("$ws_real_src_ip ws_conid %d wsc->real_src_ip is %s (%d)\n", wscid, wsc->real_src_ip, (int)strlen(wsc->real_src_ip)); + len = snprintf(buff, IP_ADDR_MAX_STR_SIZE, "%s", wsc->real_src_ip); + buff[len] = 0; + + wsconn_put(wsc); + + ip_str.s = (char *)buff; + ip_str.len = strlen(buff); + LM_DBG("$ws_real_src_ip ws_conid %d real_src_ip is %s (%d)\n", wscid, ip_str.s, ip_str.len); + return pv_get_strval(msg, param, res, &ip_str);
Thanks I didn't know about this function, will fix it.
@miconda I found a simple way to get the value of the X-Forwarded-For header without changing the code in the websocket module. I thought about this after reading again the comment for Juha. I'm retrieving the X-Forwarded-For header in the event:xhttp and use htable to track the conid associated: ```shell event_route[xhttp:request] { .... $var(orig_ip) = $hdr(X-Forwarded-For); if (($var(orig_ip) != $null) && ($var(orig_ip) != "")) { $sht(ws_src=>$conid) = $var(orig_ip); xlog("L_INFO", "WebSocket $conid original source ip is $var(orig_ip)\n"); } ```
And in any context where $conid is available, I can retrieve the value with (in my case to set xavp attr in the location_attrs table when the phone registers) : ```shell $var(real_src_ip) = $sht(ws_src=>$conid); if (($var(real_src_ip) != "") && ($var(real_src_ip) != $null)) { xavp_params_explode("real_src_ip=$var(real_src_ip)", "contact_extras"); } ```
Closed #1002.
-
Daniel-Constantin Mierla -
Juha Heinanen -
Michael Ricordeau