Bug #4692
SAT>IP: Print external NAT ip and port
100%
Description
Hi,
After some weeks testing it, I publish here the last part of my patch for NAT support (it continues with #4617).
The remaining part it’s print the correct IP and PORT values when connecting over a NAT redirection. In this case, the used values need to be the external one. Furthermore, this patch fixes the NAT values not only with the PLAY command but too with the DESCRIBE cmd.
Some clients doesn’t interpret these values. However, some others check them and closes the connection when they aren’t correct.
So, please commit this patch!
diff --git a/src/satip/rtsp.c b/src/satip/rtsp.c
index 7b366c8..a455628 100644
--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -92,6 +92,7 @@ static pthread_mutex_t rtsp_lock;
static void rtsp_close_session(session_t *rs);
static void rtsp_free_session(session_t *rs);
+static char any_ip[] = "127.0.0.1\0";
/*
*
@@ -1322,9 +1323,9 @@ rtsp_process_describe(http_connection_t *hc)
char *u = tvh_strdupa(hc->hc_url);
session_t *rs;
htsbuf_queue_t q;
- char buf[96];
+ char buf[96], *used_ip = NULL;
int r = HTTP_STATUS_BAD_REQUEST;
- int stream, first = 1, valid;
+ int stream, first = 1, valid, used_port;
htsbuf_queue_init(&q, 0);
@@ -1381,10 +1382,19 @@ rtsp_process_describe(http_connection_t *hc)
http_arg_init(&args);
if (hc->hc_session)
http_arg_set(&args, "Session", hc->hc_session);
- if (stream > 0)
- snprintf(buf, sizeof(buf), "rtsp://%s/stream=%i", rtsp_ip, stream);
+ if (rtsp_nat_ip != NULL)
+ used_ip = (rtsp_nat_ip[0] == '*' || rtsp_nat_ip[0] == '\0')? any_ip : rtsp_nat_ip;
else
- snprintf(buf, sizeof(buf), "rtsp://%s", rtsp_ip);
+ used_ip = rtsp_ip;
+ used_port = (rtsp_nat_port <= 0) ? rtsp_port : rtsp_nat_port;
+ if ((stream > 0) && (used_port != 554))
+ snprintf(buf, sizeof(buf), "rtsp://%s:%d/stream=%i", used_ip, used_port, stream);
+ else if ((stream > 0) && (used_port == 554))
+ snprintf(buf, sizeof(buf), "rtsp://%s/stream=%i", used_ip, stream);
+ else if (used_port != 554)
+ snprintf(buf, sizeof(buf), "rtsp://%s:%d", used_ip, used_port);
+ else
+ snprintf(buf, sizeof(buf), "rtsp://%s", used_ip);
http_arg_set(&args, "Content-Base", buf);
http_send_begin(hc);
http_send_header(hc, HTTP_STATUS_OK, "application/sdp", q.hq_size,
@@ -1408,8 +1418,8 @@ static int
rtsp_process_play(http_connection_t *hc, int cmd)
{
session_t *rs;
- int errcode = HTTP_STATUS_BAD_REQUEST, valid = 0, i, stream;
- char buf[256], *u = tvh_strdupa(hc->hc_url);
+ int errcode = HTTP_STATUS_BAD_REQUEST, valid = 0, i, stream, used_port;
+ char buf[256], *u = tvh_strdupa(hc->hc_url), *used_ip = NULL;
http_arg_list_t args;
http_arg_init(&args);
@@ -1467,10 +1477,15 @@ rtsp_process_play(http_connection_t *hc, int cmd)
snprintf(buf, sizeof(buf), "%d", rs->stream);
http_arg_set(&args, "com.ses.streamID", buf);
} else {
- if (rtsp_port != 554)
- snprintf(buf, sizeof(buf), "url=rtsp://%s:%d/stream=%d", rtsp_ip, rtsp_port, rs->stream);
+ if (rtsp_nat_ip != NULL)
+ used_ip = (rtsp_nat_ip[0] == '*' || rtsp_nat_ip[0] == '\0')? any_ip : rtsp_nat_ip;
+ else
+ used_ip = rtsp_ip;
+ used_port = (rtsp_nat_port <= 0) ? rtsp_port : rtsp_nat_port;
+ if (used_port != 554)
+ snprintf(buf, sizeof(buf), "url=rtsp://%s:%d/stream=%d", used_ip, used_port, rs->stream);
else
- snprintf(buf, sizeof(buf), "url=rtsp://%s/stream=%d", rtsp_ip, rs->stream);
+ snprintf(buf, sizeof(buf), "url=rtsp://%s/stream=%d", used_ip, rs->stream);
http_arg_set(&args, "RTP-Info", buf);
}
Regards and I'm sorry for posting a patch here instead of PR in GitHub!
Files
History
Updated by Mono Polimorph about 7 years ago
Updated by Hi Jaroslav,
This patch sounds right to you?
If so, please commit it! 
Thank you!
Updated by Jaroslav Kysela about 7 years ago
Updated by There's missing detection, if the incoming connection is external or local. The local clients will get the wrong answers for your code.
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
There's missing detection, if the incoming connection is external or local. The local clients will get the wrong answers for your code.
Ahh!
However, the idea is this: If the user configures NAT IP and NAT PORT, then he is using NAT everywhere.
You know any method to get the original IP and PORT (without using kernel hacks or the PROXY protocol)?
I don't know anything!
So, my suggestion is accept this code. The only limitation is: only NAT or LOCAL users will receive "correct" values. In any case, we can implement another configuration value for selecting "preferable LOCAL or preferable NAT". But, for me if the NAT is configured the users like to use NAT and not LOCAL.
So, please, accept now this "clean" patch. And in the future we can address a more complex solution. Remember that if the user doesn't configures NAT (IP or PORT) then all LOCAL connection will receive the correct value. So, this patch is consistent.
Regards!
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
There's missing detection, if the incoming connection is external or local. The local clients will get the wrong answers for your code.
Hi Jaroslav,
No response after a week. Please, comment it! I'm using this patch and it works.
If you're worried with the LOCAL/NAT detection, I have this suggestion: Include one boolean configuration value for using (printing) the LOCAL or the NAT values. I found this the most simple solution. In the current implementation of my patch the use of NAT configuration implies to print the NAT values. But with this change the user can select to one or the other. The reason it's because it's impossible to detect if the connection is LOCAL or NAT (even if the IP is not in the local LAN range you can't be sure that it not comes from another routed network not using nating).
What you think? I need to add also this configuration switch?
I'm really interested in the commiting of this patch. Please, tell me what I have to do.
Regards.
Updated by Jaroslav Kysela about 7 years ago
Usually, the server behind DNAT sees the source IP address, so you can do a check against the local IP ranges (see 'man getifaddrs') to determine, if the client is local or behind NAT.
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
Usually, the server behind DNAT sees the source IP address, so you can do a check against the local IP ranges (see 'man getifaddrs') to determine, if the client is local or behind NAT.
Hi Jaroslav,
This is true only if you don't have any routed network!
You can have three different sources:
a) LOCAL
b) over NAT
c) over ROUTE
And you can't distinguish between "b" and "c".
However, if you like to apply this solution, the only solution I see it's add a config parameter with a list of addresses for checking with the SRC. So, the code will be:
if SRC in range of NAT-LIST
then use NAT parameters
if not
then use LOCAL parametersBut, I feel it quite complex. And I don't know how to parse and compare the address list (in the form of "0.0.0.0/0; !192.168.1.0/24; 10.0.0.0/16" etc.). Or use an exclude list, instead of NAT list.
Please, let me know what solution you prefer!
Updated by Mono Polimorph about 7 years ago
Hi Jaroslav,
Please, help me!
In order to test if the client is using a LOCAL address (for implementing the most simple solution), I need to do a simple check: if the SRC is LOCAL. Ans this needs to be done inside these two functions:
rtsp_process_describe(http_connection_t *hc) { ... }
rtsp_process_play(http_connection_t *hc, int cmd) { ... }And there using the "hc" parameter I can obtain the peer address with "hc->hc_peer_ipstr".
However, I don't know how to compare it!
I need to compare with the listening address (where is stored?) or with interface addresses (I don't know how to do it)?
Any idea/help?
Updated by Jaroslav Kysela about 7 years ago
Use hc->hc_peer and hc->hc_self for the comparison. The network mask can be obtained when you match IP address of the server (hc_self) with the getifaddrs(). Put this function to src/tcp.c (it might be reused).
And you can't distinguish between "b" and "c".
True. We may cover the extended local ranges (routes) later with an user configuration. My main opinion is that local PnP clients connected to the local network segment should work without any user intervention.
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
Use hc->hc_peer and hc->hc_self for the comparison. The network mask can be obtained when you match IP address of the server (hc_self) with the getifaddrs(). Put this function to src/tcp.c (it might be reused).
I'll try! But I don't know how to get the interface of the listening address. I'm using multiple interfaces (eth0, eth1, and localhost). So, the problem is that "hc_self" gives an IP address (or "0.0.0.0" or a hostname), and I can't know (without help) how to obtain the corresponding interface.
And you can't distinguish between "b" and "c".
True. We may cover the extended local ranges (routes) later with an user configuration. My main opinion is that local PnP clients connected to the local network segment should work without any user intervention.
OK. I agree with your suggestion. Now I feel this way more robust: we assure now 100% local access, the next step will be support simple NAT, and finally expand to full access lists. Great!
Updated by Jaroslav Kysela about 7 years ago
Mono Polimorph wrote:
Jaroslav Kysela wrote:
Use hc->hc_peer and hc->hc_self for the comparison. The network mask can be obtained when you match IP address of the server (hc_self) with the getifaddrs(). Put this function to src/tcp.c (it might be reused).
I'll try! But I don't know how to get the interface of the listening address. I'm using multiple interfaces (eth0, eth1, and localhost). So, the problem is that "hc_self" gives an IP address (or "0.0.0.0" or a hostname), and I can't know (without help) how to obtain the corresponding interface.
hc_self is the real IPv4 or IPv6 on the server side including the real used TCP port in the binary form (getsockname on the socket file descriptor). It's not the bind address. Use macros in src/tcp.h like IP_IN_ADDR to access the binary IP address data.
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
hc_self is the real IPv4 or IPv6 on the server side including the real used TCP port in the binary form (getsockname on the socket file descriptor). It's not the bind address. Use macros in src/tcp.h like IP_IN_ADDR to access the binary IP address data.
Great!
Now the problem is how to know the netmask. The "hc_self" doesn't have any info about the bind interface. Any idea?
(and sorry for this request, I ask because I'm not sure about the best method for achieve this objective).
Regards.
Updated by Jaroslav Kysela about 7 years ago
getifaddrs() - 'man getifaddrs' - if you know IPv46, you can determine the network from getifaddrs...
Updated by Mono Polimorph about 7 years ago
- File SAT-IP-Print-external-NAT-ip-and-port-v2.diff SAT-IP-Print-external-NAT-ip-and-port-v2.diff added
Hi Jaroslav,
I updated my patch! I hope now you feel it right.
Let me to explain the changes:
- I added the checking of the "peer" address over the local "self" address.
- Also, some new helper functions are added to "src/tcp.h". You can reuse them if you like.
- A configuration value for forcing NAT values is added. This is required for scenarios when you connect to the TVH server using tunnels. In this case the socket address can be local, but you like to force the NAT address.
- It is compatible with IPV4 and IPV6.
However, I have one suggestion for an optimization:
- The function "check_is_local_address()" is called multiple times. So, I suggest to call it only one time when the user connects to the server, and save the status in some new variable like "hc->use_local_address". Then you can check this value in "rtsp_announced_port()" & "rtsp_announced_ip()" instead of call to the function. This will be more efficient. But I don't implemented it beacuse I'm not sure about the best location for the first call and setting the "hc->use_local_address" variable. However, I'm sure this can be trivial for you. You agree?
And finally, I need to comment that this implementation uses the function "<ifaddrs.h>/getifaddrs()". This compiles right in Linux. However, as indicated in the code not all platforms have this function. Perhaps you like to use some preprocessor check, and return always 1 from "check_is_local_address()" when the system function is not available. In this case the regular behaviour will be like a "local" connection. And with forcing the NAT values you can obtain the same behaviour as "remote" connections.
I hope you will commit this patch soon.
Regards!
Updated by Mono Polimorph about 7 years ago
Jaroslav Kysela wrote:
Pushed to master with my cleanups. Please, test.
Hi Jaroslav,
Thank you for the commit! And good cleanups... you're a great programmer!
However, after testing the master I have some comments:
- You don't do any optimization in the call to "ip_check_is_local_address()". Take into account that each time you call it you are listing the interfaces, searching over the list and comparing with the "self" address. I suspect it's required to only do it one time for each connection. You agree?
- One bug with the current code is this: When connecting locally (that is when the client is in the same network) and the THV is listening in ANY_IP (0.0.0.0), then it's printed "RTP-Info: url=rtsp://0.0.0.0/stream=1". The used IP should be the "local" address and not the "listening" one. You can fix it?
- Another but is introduced with you cleared the code around the "127.0.0.1" address when "NAT-IP"=="*". If you're forcing NAT values (because for example you're using tunnels) then the printed address is the "self" address. I feel quite annoying to use this address when connecting remotely. So, when NAT-IP is ANY_IP (that is "*") I prefer to use a more "global" or "neutral" value (like "localhost" or "127.0.0.1"). If you aggree then I can prepare a patch for it.
- I see another different bug when testing this code. I see that any modification in the SAT>IP preferences is only applyed when restarting. This is annoying. You can't restart the SAT>IP server thread when the config is updated?
- Finally I noted a new side effect. Now when the listening address is a different network than the default gateway, then the RTP send fails. For example, if you listen in the "loopback" and connect throught a NAT tunnel, then the UDP bind fails. Different solutions can be applyed. For example, introduce a NAT UDP Bind address in the configuration of SAT>IP server. What you think?
Please, comment about these issues.
Regards.
Updated by Jaroslav Kysela about 7 years ago
Mono Polimorph wrote:
Jaroslav Kysela wrote:
- You don't do any optimization in the call to "ip_check_is_local_address()". Take into account that each time you call it you are listing the interfaces, searching over the list and comparing with the "self" address. I suspect it's required to only do it one time for each connection. You agree?
Not yet. The only way to cache those values is to use http_connection_t .
- One bug with the current code is this: When connecting locally (that is when the client is in the same network) and the THV is listening in ANY_IP (0.0.0.0), then it's printed "RTP-Info: url=rtsp://0.0.0.0/stream=1". The used IP should be the "local" address and not the "listening" one. You can fix it?
Could you try the new code?
- Another but is introduced with you cleared the code around the "127.0.0.1" address when "NAT-IP"=="*". If you're forcing NAT values (because for example you're using tunnels) then the printed address is the "self" address. I feel quite annoying to use this address when connecting remotely. So, when NAT-IP is ANY_IP (that is "*") I prefer to use a more "global" or "neutral" value (like "localhost" or "127.0.0.1"). If you aggree then I can prepare a patch for it.
Yes, you're probably right that the neutral IP address should be visible in this case. Fixed.
- I see another different bug when testing this code. I see that any modification in the SAT>IP preferences is only applyed when restarting. This is annoying. You can't restart the SAT>IP server thread when the config is updated?
If you set the RTSP port as the command line argument, the re-config is blocked. It's mainly because binding to ports < 1024 requires root priviledges.
- Finally I noted a new side effect. Now when the listening address is a different network than the default gateway, then the RTP send fails. For example, if you listen in the "loopback" and connect throught a NAT tunnel, then the UDP bind fails. Different solutions can be applyed. For example, introduce a NAT UDP Bind address in the configuration of SAT>IP server. What you think?
For local ranges, we can probably use getifaddrs() to determine the exact local IP now.
Updated by Mono Polimorph almost 7 years ago
- File SAT-IP-Cache-ip_check_is_local_address-call.diff SAT-IP-Cache-ip_check_is_local_address-call.diff added
Jaroslav Kysela wrote:
Mono Polimorph wrote:
Jaroslav Kysela wrote:
- You don't do any optimization in the call to "ip_check_is_local_address()". Take into account that each time you call it you are listing the interfaces, searching over the list and comparing with the "self" address. I suspect it's required to only do it one time for each connection. You agree?
Not yet. The only way to cache those values is to use http_connection_t .
Hi,
I prepared a simple patch to do the optimization (caching the value).
Here it is:
diff --git a/src/http.h b/src/http.h
index 7f26148..b045205 100644
--- a/src/http.h
+++ b/src/http.h
@@ -136,6 +136,7 @@ typedef struct http_connection {
char *hc_peer_ipstr;
struct sockaddr_storage *hc_self;
char *hc_representative;
+ int is_local_address_cache;
pthread_mutex_t *hc_paths_mutex;
http_path_list_t *hc_paths;
diff --git a/src/satip/rtsp.c b/src/satip/rtsp.c
index bd8c978..f8cb68c 100644
--- a/src/satip/rtsp.c
+++ b/src/satip/rtsp.c
@@ -322,14 +322,18 @@ static inline const char *
rtsp_conn_ip(http_connection_t *hc, char *buf, size_t buflen, int *port)
{
const char *used_ip = rtsp_ip;
- int used_port = rtsp_port, local;
+ int used_port = rtsp_port;
+ int local = hc->is_local_address_cache;
struct sockaddr_storage self;
if (rtsp_nat_ip[0] == '\0')
goto end;
self = *hc->hc_self;
/* note: call ip_check at first to initialize self (ip any) */
- local = ip_check_is_local_address(hc->hc_peer, hc->hc_self, &self);
+ if (local < 0) {
+ hc->is_local_address_cache = ip_check_is_local_address(hc->hc_peer, hc->hc_self, &self);
+ local = hc->is_local_address_cache;
+ }
if (local || satip_server_conf.satip_nat_name_force) {
used_ip = rtsp_nat_ip;
if (rtsp_nat_port > 0)
@@ -1664,6 +1668,7 @@ rtsp_serve(int fd, void **opaque, struct sockaddr_storage *peer,
hc.hc_self = self;
hc.hc_process = rtsp_process_request;
hc.hc_cseq = 1;
+ hc.is_local_address_cache = -1;
http_serve_requests(&hc);
Updated by Mono Polimorph almost 7 years ago
Jaroslav Kysela wrote:
Mono Polimorph wrote:
Jaroslav Kysela wrote:
- One bug with the current code is this: When connecting locally (that is when the client is in the same network) and the THV is listening in ANY_IP (0.0.0.0), then it's printed "RTP-Info: url=rtsp://0.0.0.0/stream=1". The used IP should be the "local" address and not the "listening" one. You can fix it?
Could you try the new code?
- Another but is introduced with you cleared the code around the "127.0.0.1" address when "NAT-IP"=="*". If you're forcing NAT values (because for example you're using tunnels) then the printed address is the "self" address. I feel quite annoying to use this address when connecting remotely. So, when NAT-IP is ANY_IP (that is "*") I prefer to use a more "global" or "neutral" value (like "localhost" or "127.0.0.1"). If you aggree then I can prepare a patch for it.
Yes, you're probably right that the neutral IP address should be visible in this case. Fixed.
Hi,
I'm not sure, but as the function returns a pointer... it's safe to use a literal here?
http://github.com/tvheadend/tvheadend/blob/master/src/satip/rtsp.c#L344
I feel it's best to set a static value, and then return the pointer to the static value. No?
Updated by Jaroslav Kysela almost 7 years ago
Mono Polimorph wrote:
Hi,
I'm not sure, but as the function returns a pointer... it's safe to use a literal here?
http://github.com/tvheadend/tvheadend/blob/master/src/satip/rtsp.c#L344I feel it's best to set a static value, and then return the pointer to the static value. No?
It's static string which is placed to the BSS section. If you define an extra pointer to a static string, it's only wasting of the space.
I'll push the cache code (rewritten - again - moved code to src/http.c) in the next git push.
Updated by Jaroslav Kysela almost 7 years ago
- Status changed from New to Fixed
- % Done changed from 0 to 100
Applied in changeset commit:tvheadend|bafd78560c6fc2a0691c07708c32931bb1cac97b.
Updated by Mono Polimorph almost 7 years ago
Jaroslav Kysela wrote:
Applied in changeset commit:tvheadend|bafd78560c6fc2a0691c07708c32931bb1cac97b.
Hi,
Interesting use of http_check_local_ip() function. Thank you!
I'll test it.