9 May
2021
9 May
'21
9:34 p.m.
- add aes-cbc, des-ede3-cbc ipsec encryption algoriyhms
- improve nlmsg_seq choice for concurrent multi UEs Registrations at
same time
- before sending replies over IPsec check the existing
of opened TCP sockets
<!-- 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 -->
- [v] Commit message has the format required by CONTRIBUTING guide
- [v] Commits are split per component (core, individual modules, libs, utils, ...)
- [v] Each component has a single commit (if not, squash them into one commit)
- [v] 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
- [v] Small bug fix (non-breaking change which fixes an issue)
- [v] 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
- [v] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
<!-- Describe your changes in detail -->
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2731
-- Commit Summary --
* ims_ipsec_pcscf: more algorithms, SA improvements
-- File Changes --
M src/modules/ims_ipsec_pcscf/cmd.c (29)
M src/modules/ims_ipsec_pcscf/ipsec.c (34)
M src/modules/ims_ipsec_pcscf/ipsec.h (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2731.patch
https://github.com/kamailio/kamailio/pull/2731.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/2731
10 May
10 May
12:59 p.m.
@alexyosifov - given that you contributed recently to this module, do you have any comment
on this PR?
--
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/2731#issuecomment-836553802
1:10 p.m.
@henningw commented on this pull request.
Thanks for the pull request, i have added one comment regarding the time handling. Are
there module any documentation updates necessary as well?
@@ -100,8 +100,15 @@ static void string_to_key(char*
dst, const str key_string)
}
}
+static uint choose_nlmsg_seq (void)
Any particular reason why you choose to implement this function instead of the previous
time(0) call? The clock_gettime() function is not available at Mac OS, and probably need a
wrapper around it as e.g. in the cdp module. If not necessary, we should probably stay
with the previous way.
--
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/2731#pullrequestreview-655517861
2:13 p.m.
@alexyosifov approved this pull request.
Added some comments
@@ -846,11 +846,20 @@ int ipsec_forward(struct
sip_msg* m, udomain_t* d, int _cflags)
// for Reply get the dest proto from
the received request
dst_proto = req->rcv.proto;
- // for Reply and TCP sends from P-CSCF server port, for Reply and UDP sends from
P-CSCF client port
- src_port = dst_proto == PROTO_TCP ? s->port_ps : s->port_pc;
+ // Check send socket
Why you have to check "send socket" here? You perform the check with zero
src_port and dst_proto.
The "send socket" always is present. If you scroll down at row 877 there is the
same check and if "send socket" is not present the function returns an error.
I do not see value from this change and I am not sure this will work properly.
Correct me if I am wrong.
strcpy(l_enc_algo->alg_name,"cipher_null");
+ if
(strncasecmp(r_ealg.s,"aes-cbc",r_ealg.len) == 0) {
Is it a good idea cipher algorithm to be optional? Just add a new value in _cflags
parameter in int ipsec_create(struct sip_msg* m, udomain_t* d, int _cflags) method.
--
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/2731#pullrequestreview-655554158
2:16 p.m.
@alexyosifov approved this pull request.
- // for Reply and TCP sends from P-CSCF server
port, for Reply and UDP sends from P-CSCF client port
- src_port = dst_proto
== PROTO_TCP ? s->port_ps : s->port_pc;
+ // Check send socket
+ struct socket_info * client_sock = grep_sock_info(via_host.af == AF_INET ?
&ipsec_listen_addr : &ipsec_listen_addr6, src_port, dst_proto);
+ if(client_sock) {
+ // for Reply and TCP sends from P-CSCF server port, for Reply and UDP sends
from P-CSCF client port
+ src_port = dst_proto == PROTO_TCP ? s->port_ps : s->port_pc;
- // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
- dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
+ dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ }
+ else
+ {
+ src_port = s->port_pc;
+ dst_port = s->port_us;
+ }
Why you have to check "send socket" here? You perform the check with zero
src_port and dst_proto.
The "send socket" always is present. If you scroll down at row 877 there is the
same check and if "send socket" is not present the function returns an error.
I do not see value from this change and I am not sure this will work properly.
Correct me if I am wrong.
+ // cipher_null, des, des3_ede, aes
strcpy(l_enc_algo->alg_name,"cipher_null");
+ if (strncasecmp(r_ealg.s,"aes-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: AES\n");
+ strcpy(l_enc_algo->alg_name,"aes");
+ l_enc_algo->alg_key_len = ck.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck);
+ }
+ else if (strncasecmp(r_ealg.s,"des-ede3-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: DES, ck.len=%d\n",ck.len);
+ strcpy(l_enc_algo->alg_name,"des3_ede");
+ str ck1;
+ ck1.s = pkg_malloc (128);
+ strncpy(ck1.s,ck.s,32);
+ strncat(ck1.s,ck.s,16);
+ ck1.len=32+16;
+
+ l_enc_algo->alg_key_len = ck1.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck1);
+
+ pkg_free(ck1.s);
+ }
Is it a good idea cipher algorithm to be optional? Just add a new value in _cflags
parameter in int ipsec_create(struct sip_msg* m, udomain_t* d, int _cflags) method.
--
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/2731#pullrequestreview-655573226
2:42 p.m.
@riccardv commented on this pull request.
@@ -100,8 +100,15 @@ static void string_to_key(char*
dst, const str key_string)
}
}
+static uint choose_nlmsg_seq (void)
Hi @henningw ,
I choose the 1 micro seconds granulation due to the commit comment:
"improve nlmsg_seq choice for concurrent multi UEs Registrations at same time"
The reason is the previous choice of time(NULL) return a time value on seconds and it can
be too large in presence of multiple UE access in the same time. In high load condition it
can be happens.
IPsec Security Association must have this parameter as a unique identifier, since kamailio
can be configured to listen on single local ip:port for IPsec, it can be a problem,
resulting in a failure on activation of SA, and then a failure on VoLTE SIP REGISTRATION.
A time based choice is a good choice, but 1 second is not enough, so I propose the choice
of 1 micro second granularity that can be protect the registrations in a reasonable way.
Regarding other platforms that don't have the funtion, feel free to add a pre
processor condition on the 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/2731#discussion_r629325968
2:57 p.m.
@riccardv commented on this pull request.
- // for Reply and TCP sends from P-CSCF server
port, for Reply and UDP sends from P-CSCF client port
- src_port = dst_proto
== PROTO_TCP ? s->port_ps : s->port_pc;
+ // Check send socket
+ struct socket_info * client_sock = grep_sock_info(via_host.af == AF_INET ?
&ipsec_listen_addr : &ipsec_listen_addr6, src_port, dst_proto);
+ if(client_sock) {
+ // for Reply and TCP sends from P-CSCF server port, for Reply and UDP sends
from P-CSCF client port
+ src_port = dst_proto == PROTO_TCP ? s->port_ps : s->port_pc;
- // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
- dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
+ dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ }
+ else
+ {
+ src_port = s->port_pc;
+ dst_port = s->port_us;
+ }
Hi @alexyosifov ,
without the patch the ports in case of TCP are always:
src_port = s->port_ps
dst_port = s->port_uc
That are the ones used in case the user agent open the connection.
A pre check is necessary because if this socket is not already open, the use the ports:
src_port = s->port_pc;
dst_port = s->port_us;
that are the port from Proxy Client (pc) -> User Server (us).
Is a attempt to fallback to the other opened socket.
This case can be happens when INVITE transaction is very long due to long Ringing time
phase.
--
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/2731#discussion_r629336057
2:58 p.m.
@riccardv commented on this pull request.
+ // cipher_null, des, des3_ede, aes
strcpy(l_enc_algo->alg_name,"cipher_null");
+ if (strncasecmp(r_ealg.s,"aes-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: AES\n");
+ strcpy(l_enc_algo->alg_name,"aes");
+ l_enc_algo->alg_key_len = ck.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck);
+ }
+ else if (strncasecmp(r_ealg.s,"des-ede3-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: DES, ck.len=%d\n",ck.len);
+ strcpy(l_enc_algo->alg_name,"des3_ede");
+ str ck1;
+ ck1.s = pkg_malloc (128);
+ strncpy(ck1.s,ck.s,32);
+ strncat(ck1.s,ck.s,16);
+ ck1.len=32+16;
+
+ l_enc_algo->alg_key_len = ck1.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck1);
+
+ pkg_free(ck1.s);
+ }
I simply add the algorithm in the original code, nothing more
--
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/2731#discussion_r629336886
3:24 p.m.
@alexyosifov commented on this pull request.
- // for Reply and TCP sends from P-CSCF server
port, for Reply and UDP sends from P-CSCF client port
- src_port = dst_proto
== PROTO_TCP ? s->port_ps : s->port_pc;
+ // Check send socket
+ struct socket_info * client_sock = grep_sock_info(via_host.af == AF_INET ?
&ipsec_listen_addr : &ipsec_listen_addr6, src_port, dst_proto);
+ if(client_sock) {
+ // for Reply and TCP sends from P-CSCF server port, for Reply and UDP sends
from P-CSCF client port
+ src_port = dst_proto == PROTO_TCP ? s->port_ps : s->port_pc;
- // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
- dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
+ dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ }
+ else
+ {
+ src_port = s->port_pc;
+ dst_port = s->port_us;
+ }
I agree. Thanks for the clarification!
--
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/2731#discussion_r629355293
3:24 p.m.
@alexyosifov commented on this pull request.
+ // cipher_null, des, des3_ede, aes
strcpy(l_enc_algo->alg_name,"cipher_null");
+ if (strncasecmp(r_ealg.s,"aes-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: AES\n");
+ strcpy(l_enc_algo->alg_name,"aes");
+ l_enc_algo->alg_key_len = ck.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck);
+ }
+ else if (strncasecmp(r_ealg.s,"des-ede3-cbc",r_ealg.len) == 0) {
+ LM_DBG("Creating security associations: DES, ck.len=%d\n",ck.len);
+ strcpy(l_enc_algo->alg_name,"des3_ede");
+ str ck1;
+ ck1.s = pkg_malloc (128);
+ strncpy(ck1.s,ck.s,32);
+ strncat(ck1.s,ck.s,16);
+ ck1.len=32+16;
+
+ l_enc_algo->alg_key_len = ck1.len * 4;
+ string_to_key(l_enc_algo->alg_key, ck1);
+
+ pkg_free(ck1.s);
+ }
OK, Thanks!
--
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/2731#discussion_r629355503
3:45 p.m.
@riccardv commented on this pull request.
- // for Reply and TCP sends from P-CSCF server
port, for Reply and UDP sends from P-CSCF client port
- src_port = dst_proto
== PROTO_TCP ? s->port_ps : s->port_pc;
+ // Check send socket
+ struct socket_info * client_sock = grep_sock_info(via_host.af == AF_INET ?
&ipsec_listen_addr : &ipsec_listen_addr6, src_port, dst_proto);
+ if(client_sock) {
+ // for Reply and TCP sends from P-CSCF server port, for Reply and UDP sends
from P-CSCF client port
+ src_port = dst_proto == PROTO_TCP ? s->port_ps : s->port_pc;
- // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
- dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ // for Reply and TCP sends to UE client port, for Reply and UDP sends to UE
server port
+ dst_port = dst_proto == PROTO_TCP ? s->port_uc : s->port_us;
+ }
+ else
+ {
+ src_port = s->port_pc;
+ dst_port = s->port_us;
+ }
Welcome!
--
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/2731#discussion_r629372001
19 May
19 May
8:26 a.m.
@riccardv - I added `ksr_clock_gettime()` in the `core/ut.h`, which is portable version of
`clock_gettime()` that works for MacOS as well. Push another commit to this PR to use it.
Any other comments from devs for this PR?
--
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/2731#issuecomment-843784315
11:31 p.m.
@riccardv pushed 2 commits.
8ef89c258fceb417621cee694a2871066e06dc04 Merge remote-tracking branch
'origin/master' into ipsec_improve
f792b215321314d552f7fa90f17ef25b91cc2c09 ims_ipsec_pcscf: use ksr_clock_gettime()
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2731/files/7133cb20ad46aafc98e16a…
11:34 p.m.
Thanks @miconda , I did the correction and pushed
--
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/2731#issuecomment-844490224
24 May
24 May
10:15 p.m.
@miconda , No comments from my side.
--
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/2731#issuecomment-847306443
10:19 p.m.
@henningw commented on this pull request.
@@ -100,8 +100,15 @@ static void string_to_key(char*
dst, const str key_string)
}
}
+static uint choose_nlmsg_seq (void)
Was resolved as per other discussions.
--
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/2731#discussion_r638247096
10:28 p.m.
Merged #2731 into master.
--
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/2731#event-4788688269
1286
days inactive
1301
days old
16 comments
4 participants
participants (4)
-
alexyosifov -
Daniel-Constantin Mierla
-
Henning Westerholt
-
riccardv