[kamailio/kamailio] nathelper: test sdp c line and sip source ip address is matched. (#2045)
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> test sdp c line and sip source ip address is matched.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2045
-- Commit Summary --
* nathelper: test sdp c line and sip source ip address is matched * nathelper : flag 256 is added to nathelper documentation
-- File Changes --
M src/modules/nathelper/doc/nathelper_admin.xml (72) M src/modules/nathelper/nathelper.c (47)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2045.patch https://github.com/kamailio/kamailio/pull/2045.diff
Thanks Yasin, looks good to me.
I see that the comparison of the IPs is done with string operations. That's ok for ipv4, but can be a mismatch with ipv6 because there can be different forms of the same address.
I suggest to convert the the ip in sdp to binary struct and compare with the src ip which is in binary struct -- see str2ipxbuf() or str2ipx() and ip_addr_cmp() functions/macros.
@miconda
I tried to convert string to ip_addr struct as you mentioned but , functions cannot convert it. In addition , when i try to log "sdp ip address " it gives empty. I didnt understand reason.
converting with str2ip gives NULL. ``` struct ip_addr* cline_addr;
if(session->pf==AF_INET6){ cline_addr=str2ip(&session->ip_addr); }else if(session->pf==AF_INET){ cline_addr=str2ip6(&session->ip_addr); }else{ LM_ERR("Couldn t get sdp address type \n"); break; }
if(!cline_addr){ LM_ERR("Sdp ip address is empty \n"); break; }
```
Have you tested that session->ip_addr.len>0? Can you print the str value for session->ip_addr and see if it is a valid IP?
Then it would be better to use str2ipx(), because it tries internally both ipv4/6.
@miconda I dont understand reason but it gives empty line. loggin: ``` LM_ERR("SDP CLINE2 [%*.s][LEN: %d] \n",session->ip_addr.len,&session->ip_addr.s[0],session->ip_addr.len); LM_ERR("SDP CLINE2 [%s] \n",&session->ip_addr.s[0]); ```
here is result
``` 9(4455) ERROR: {1 1 INVITE 1-4478@192.168.1.39} nathelper [nathelper.c:1370]: test_sdp_cline(): SDP CLINE2 [ ][LEN: 12] 9(4455) ERROR: {1 1 INVITE 1-4478@192.168.1.39} nathelper [nathelper.c:1371]: test_sdp_cline(): SDP CLINE2 [192.168.1.39 t=0 0 m=audio 6000 RTP/AVP 8 a=rtpmap:8 PCMA/8000 ]
```
It has to be `%.*s`, you used `*` before the `.` -- also you do not need that expression with address of the first char, just give `session->ip_addr.s` instead of `&session->ip_addr.s[0]`, it should avoid array access and get address of it -- probably there is not much difference in performance, but I find it easier to follow up.
@ycaner06 pushed 1 commit.
388fda36f81e486134f4b3c4acaa51ace97f6645 test sdp c line and sip source ip address is matched
First a note for who is going to merge: the last commit doesn't have proper formatted commit message. So this PR should be squashed and merged or merged manually.
Then, I need a clarification: when this test is true: when all IPs in SDP match the source IP? If my logic is not missing something, the ip_addr_cmp() returns true if the two ips are the same, but you return -1 in that case.
Also, the result variable is set to 1 and used only for the return at the end. If you need it only for that, then remove it and just do return 1 at the end.
Hello,
I used str2ipxbuf() instead of stripx().
Tested
- IPv6 short and long versions - IPv4 versions.
``` ip_addr_t* str2ip6(str* st) { static ip_addr_t ip;
if(str2ip6buf(st, &ip)<0) { return NULL; }
return &ip; } ```
First a note for who is going to merge: the last commit doesn't have proper formatted commit message. So this PR should be squashed and merged or merged manually.
Then, I need a clarification: when this test is true: when all IPs in SDP match the source IP? If my logic is not missing something, the ip_addr_cmp() returns true if the two ips are the same, but you return -1 in that case.
yes ofc. If Connection Line IP address in SDP is matched with SIP source address, there isnt any problem about NAT. For example;
UE has 192.168.1.18 A Router that has ability Source NAT . so translate SIP Source IP address to 192.168.16.1
UE SIP request address is 192.168.16.1, Connection Lİne 192.168.1.18 so RTP flows cannot reach to UE. For solution , we add to rtpengine_manage function "SIP Source Address" flag.
256 flag helps to understand this scenario.
I will add more information to document.
OK, so the title of PR is misleading, the test is true if there is mismatch, not when IPs are matched.
@ycaner06 pushed 2 commits.
1137b2c44eacbf795e340317b49b1f0e7effd9c5 nathelper : removed unnecessary variable and fixed indentation 6ad9790a79984e8d5bfe9d747c386a2dc9840689 nathelper: added more information about 256 flag to documentation
As I looked at the entire diff -- do not change the formatting of existing code/docs files when adding a new features. Now the diff has a lot of changes in the xml doc files making hard to spot what was changed. Patches to fix existing indentation and whitespaces are more than welcome, but must be dedicated commits/pull requests. It can stay here, but have in mind for the future.
I think further clarifications are needed, because there can be many c= lines in SDP and you return -1 with the first one when IPs matches, but maybe before or after that c= line it is a mismatch (e.g., audio session matches, video session mismatches). How should this be considered?
You can leave it like now, just explain in the docs.
Or, an idea, you can add one (or two) variable(s) to count the c= lines that match (and those that mismatch). Then at the end of the function you test if matches is same value as the number of sessions and return different value if all matched, or there was some partial matches, or no matches.
Also, I think there should be like return -2 (or other negative value) if SDP fails to be parsed, to be able to differentiate between error and match.
As I looked at the entire diff -- do not change the formatting of existing code/docs files when adding a new features. Now the diff has a lot of changes in the xml doc files making hard to spot what was changed. Patches to fix existing indentation and whitespaces are more than welcome, but must be dedicated commits/pull requests. It can stay here, but have in mind for the future.
i dont change any indentation , vim can do it automatically. Sorry for it.
I think further clarifications are needed, because there can be many c= lines in SDP and you return -1 with the first one when IPs matches, but maybe before or after that c= line it is a mismatch (e.g., audio session matches, video session mismatches). How should this be considered?
Well , i never see more c line so i didnt think about it. I will change it as you mentioned.
You can leave it like now, just explain in the docs.
Or, an idea, you can add one (or two) variable(s) to count the c= lines that match (and those that mismatch). Then at the end of the function you test if matches is same value as the number of sessions and return different value if all matched, or there was some partial matches, or no matches.
Also, I think there should be like return -2 (or other negative value) if SDP fails to be parsed, to be able to differentiate between error and match.
I will add more option but I hope it dont change other test results. For example , if there is a problem parsing sdp return 2 from nat_uac_test() function.
Hi Yasin, any update about the patch? if you don't change the code regarding the multiple c lines - some extensions in the docs would be necessary. A second point would be to add a return -2 if the SDP can't be parsed.
Let me know, then I can do a manual merge e.g. on Tuesday evening in time for the freeze.
@ycaner06 pushed 2 commits.
70200b9a135e1feebde20fcdb285c6ce754eeac8 nathelper : added 256 to nat_uac_test function to test cline and src_ip 55f4c697a464cad71f1f56e222c0999d94f6de7d nathelper : added information for new nat_uac_test(256)
Hello, I couldnt test multiple connection address but i am sure it works . I think parse_sdp cannot parse it so it always give me 1 address. function on error returns -2. if all address are matched , returns -1 instead of 0 (zero). Because if returns 0 , script exits.(I dont know why)
added more information to documentation.
I merged your pull request, with some adaptions. I removed the white-space changes from the docs.
The nat_uac_test function return 1 when a NAT check matches for all existing checks and -1 otherwise. So i also adapted the check to work like this. A second reason for the change was that the way it was implemented would stop working if another new flag would be added later on. I also adapted the c function return codes to the other existing code.
The additionally discussed functionality to also return the number of matches lines could be easily exported as a dedicated function. This could be done with a small wrapper like the existing function is_rfc1918.
Closed #2045.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Yasin CANER