[kamailio/kamailio] ims_ipsec_pcscf: fix issues with UEs not sending certain replies thro… (PR #3029)
…ugh the ipsec tunnel
<!-- 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 - [X] Small bug fix (non-breaking change which fixes an issue) - [ ] 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 --> - [X] 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 --> I've fixed an issue that would ignore legitimate replies that are sent by real-world UEs, causing calls to completely break due to timeouts. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3029
-- Commit Summary --
* ims_ipsec_pcscf: fix issues with UEs not sending certain replies through the ipsec tunnel
-- File Changes --
M src/modules/ims_ipsec_pcscf/cmd.c (108) M src/modules/ims_ipsec_pcscf/ims_ipsec_pcscf_mod.c (13) M src/modules/ims_ipsec_pcscf/ipsec.c (39) D src/modules/ims_ipsec_pcscf/port_gen.c (227) D src/modules/ims_ipsec_pcscf/port_gen.h (41) M src/modules/ims_ipsec_pcscf/sec_agree.c (26) M src/modules/ims_ipsec_pcscf/spi_gen.c (119) M src/modules/ims_ipsec_pcscf/spi_gen.h (6) M src/modules/ims_ipsec_pcscf/spi_list.c (72) M src/modules/ims_ipsec_pcscf/spi_list.h (13) M src/modules/ims_ipsec_pcscf/spi_list_tests.c (12)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3029.patch https://github.com/kamailio/kamailio/pull/3029.diff
Hi, Thank you for your contribution! Please remove all changes that are not related to functional changes and/or bug fixes. Please do not perform code refactoring without any reason, typo and code indentation fixes in a pull request with functional/bug fixes changes. It is hard to check what exactly is fixed. If you want to do typo/code indentation fixes just put them in a separate pull request.
@kristiyan-peychev-flolive , Could you please explain in detail what exactly was the problem and what exactly is the solution. I tried to understand the applied changes. Why did you merge the lists for the SPIs and the PORTs? I will add more comments to the code.
We had conf call with @kristiyan-peychev-flolive. The merge request is not ready to be merged. My advice is this merge request to be closed without merge.
@alexyosifov commented on this pull request.
@@ -652,6 +617,9 @@ int ipsec_create(struct sip_msg* m, udomain_t* d, int _cflags)
struct pcontact_info ci; int ret = IPSEC_CMD_FAIL; // FAIL by default
+ ci.reg_state = PCONTACT_ANY; + ci.num_service_routes = 0; +
no effect because in **fill_contact()** **ci** is memset with 0.
@alexyosifov commented on this pull request.
@@ -779,6 +752,9 @@ int ipsec_forward(struct sip_msg* m, udomain_t* d, int _cflags)
unsigned short src_port = 0; ip_addr_t via_host;
+ ci.reg_state = PCONTACT_ANY;
The same as above comment in ipsec_create().
@alexyosifov commented on this pull request.
// Update contacts only for initial registration, for re-registration the existing contacts shouldn't be updated.
- if(ci.via_port == SIP_PORT){ + if(ci.via_port == SIP_PORT && req_sec_params == NULL){
**ipsec_create()** should be invoked exactly before 401 reply. From my practice in received REGISTER on 5060 there is always **sec-agree** header and this **if** block will never been TRUE.
@alexyosifov commented on this pull request.
@@ -670,16 +638,11 @@ int ipsec_create(struct sip_msg* m, udomain_t* d, int _cflags)
}
// Get security parameters - if(pcontact->security_temp == NULL) { + if(pcontact->security_temp == NULL || pcontact->security_temp->type != SECURITY_IPSEC) {
Keep the old code. The new one cannot distinguish between the errors: "No security parameters found" and "Unsupported security type"
I converted this PR to draft to indicate that its still in progress. If you want to merge it at a later stage, make it ready for review again. Otherwise just close it as discussed already and open a new PR.
I'm closing the PR, it's most likely going to be split into multiple PRs anyway, so there's no point in keeping it open.
Closed #3029.
-
alexyosifov -
Henning Westerholt
-
Kristiyan Peychev
-
kristiyan-peychev-flolive