[kamailio/kamailio] nathelper: test sdp c line and sip source ip address is matched. (#2045)
26 Aug
2019
26 Aug
'19
9:16 a.m.
<!-- 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
--
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/2045
26 Aug
26 Aug
9:48 a.m.
Thanks Yasin, looks good to me.
--
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/2045#issuecomment-524756254
10:04 a.m.
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.
--
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/2045#issuecomment-524761894
11 a.m.
@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;
}
```
--
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/2045#issuecomment-524781702
11:49 a.m.
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.
--
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/2045#issuecomment-524797556
12:13 p.m.
@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(a)192.168.1.39} nathelper [nathelper.c:1370]:
test_sdp_cline(): SDP CLINE2 [ ][LEN: 12]
9(4455) ERROR: {1 1 INVITE 1-4478(a)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
]
```
--
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/2045#issuecomment-524804946
12:52 p.m.
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.
--
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/2045#issuecomment-524814875
27 Aug
27 Aug
9:56 a.m.
@ycaner06 pushed 1 commit.
388fda36f81e486134f4b3c4acaa51ace97f6645 test sdp c line and sip source ip address is
matched
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2045/files/6060b1399eee16fcd593d6…
10:05 a.m.
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.
--
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/2045#issuecomment-525189583
10:06 a.m.
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;
}
```
--
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/2045#issuecomment-525189906
10:56 a.m.
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.
--
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/2045#issuecomment-525208294
10:59 a.m.
OK, so the title of PR is misleading, the test is true if there is mismatch, not when IPs
are matched.
--
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/2045#issuecomment-525209585
11:08 a.m.
@ycaner06 pushed 2 commits.
1137b2c44eacbf795e340317b49b1f0e7effd9c5 nathelper : removed unnecessary variable and
fixed indentation
6ad9790a79984e8d5bfe9d747c386a2dc9840689 nathelper: added more information about 256 flag
to documentation
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2045/files/388fda36f81e486134f4b3…
12:39 p.m.
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.
--
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/2045#issuecomment-525244952
12:59 p.m.
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.
--
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/2045#issuecomment-525251153
1 Sep
1 Sep
4:27 p.m.
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.
--
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/2045#issuecomment-526923700
4:54 p.m.
@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)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2045/files/6ad9790a79984e8d5bfe9d…
4:59 p.m.
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.
--
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/2045#issuecomment-526926151
3 Sep
3 Sep
11:37 p.m.
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.
--
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/2045#issuecomment-527650756
11:37 p.m.
Closed #2045.
--
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/2045#event-2605074521
1709
days inactive
1717
days old
19 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Yasin CANER