- 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
@alexyosifov - given that you contributed recently to this module, do you have any comment on this PR?
@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.
@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.
@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.
@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
@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.
@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
@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!
@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!
@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!
@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?
@riccardv pushed 2 commits.
8ef89c258fceb417621cee694a2871066e06dc04 Merge remote-tracking branch 'origin/master' into ipsec_improve f792b215321314d552f7fa90f17ef25b91cc2c09 ims_ipsec_pcscf: use ksr_clock_gettime()
Thanks @miconda , I did the correction and pushed
@miconda , No comments from my side.
@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.
Merged #2731 into master.
-
alexyosifov -
Daniel-Constantin Mierla
-
Henning Westerholt
-
riccardv