<!-- 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 - [ ] Tested changes locally - [X] Related to issue #3570
#### Description <!-- Describe your changes in detail --> Added optional AoR argument to ipsec_destroy(); to permit insertion of a real AoR from a cfg script. Otherwise, ipsec_destroy() uses the dummy AoR you@kamailio.org. The dummy AoR does not identify the IPSec SA of any UE present in the registrar, so no IPSec SA can be torn down. The real AoR, if present as an argument to ipsec_destroy() replaces the dummy AoR in m->first_line.u.request.uri.s, before calling fill_contact() (see changes to cmd.c / cmd.h). All other changes address ims_ipsec_pcscf_mod.c only, as required to implement the ipsec_destroy() optional argument. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3645
-- Commit Summary --
* Add AoR argument to ims_ipsec_pcscf ipsec_destroy()
-- File Changes --
M src/modules/ims_ipsec_pcscf/cmd.c (8) M src/modules/ims_ipsec_pcscf/cmd.h (2) M src/modules/ims_ipsec_pcscf/ims_ipsec_pcscf_mod.c (52)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3645.patch https://github.com/kamailio/kamailio/pull/3645.diff
Thanks, I will merge it manually because the commit message does not follow the format from contributing guidelines, use it for next commits that you want to push in order to be able to do the merging from PR portal:
- https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md#com...
Merged manually. However, as I did the merge, I noticed:
- the docs were not updated to reflect the new parameter for ipsec_destroy() - you have to update the xml files inside `doc/` subfolder when adding/changing module parameters or functions. This time I did it, you can review to see if you want to add more there - the change of msg structure ruri is not safe by just changing the string pointer and the length, you have to use C function `rewrite_uri(...)` from the core because old value might need to be freed and r-uri-parsed structure invalidated. But such change then it is permanet, if you need it to stay like that, then use `$ru = ...;` in config, functions like `ipsec_destroy()` must not change it permanently (at least not without documenting such behaviour). Therefore I pushed another commit to the module which is no longer replacing msg structure r-uri field, but uses the config parameter to build the contact info.
I also changed the fixup code to use an existing helper function from core, which simplifies that part.
You have to test with latest git master branch and see if it works as expected, if not, then open an issue.
Closed #3645.
Thank you for taking the time to correct my input and provide this guidance. I hope to make better contributions in future. I knew my “solution” was a quick hack; but as I couldn’t find out (quickly) where the dummy message was instantiated, I couldn’t work out what its side-effects might be. What matters is that the ipsec_destroy() function will now actually tear down an IPSec SA, because none were being removed before. Output from “ip xfrm state list” would indicate perhaps 10 SA’s while only one ipsec-3gpp capable UE was active (and unreachable in a VoLTE call).
From: Daniel-Constantin Mierla ***@***.***> Sent: Wednesday, November 22, 2023 10:17 AM To: kamailio/kamailio ***@***.***> Cc: BISHOP James (JRC-ISPRA) ***@***.***>; Author ***@***.***> Subject: Re: [kamailio/kamailio] Add AoR argument to ims_ipsec_pcscf ipsec_destroy() (PR #3645)
Merged manually. However, as I did the merge, I noticed:
* the docs were not updated to reflect the new parameter for ipsec_destroy() - you have to update the xml files inside doc/ subfolder when adding/changing module parameters or functions. This time I did it, you can review to see if you want to add more there * the change of msg structure ruri is not safe by just changing the string pointer and the length, you have to use C function rewrite_uri(...) from the core because old value might need to be freed and r-uri-parsed structure invalidated. But such change then it is permanet, if you need it to stay like that, then use $ru = ...; in config, functions like ipsec_destroy() must not change it permanently (at least not without documenting such behaviour). Therefore I pushed another commit to the module which is no longer replacing msg structure r-uri field, but uses the config parameter to build the contact info.
I also changed the fixup code to use an existing helper function from core, which simplifies that part.
You have to test with latest git master branch and see if it works as expected, if not, then open an issue.
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/kamailio/kamailio/pull/3645*issuecomment-1822383640__;Iw!!DOxrgLBm!GFbbiUFbkTtqNYXnyfvYp32RnIiQvNg0Wgy7SN0EN0XlJeRWW4qy6h5Z4bxw0qjNlAReI5ILiIA3HmUbMA6C0xsL7SS-Boc$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AM4GRXMJFHNNUK6W7N5AUEDYFW7KHAVCNFSM6AAAAAA7PS6Q3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSGM4DGNRUGA__;!!DOxrgLBm!GFbbiUFbkTtqNYXnyfvYp32RnIiQvNg0Wgy7SN0EN0XlJeRWW4qy6h5Z4bxw0qjNlAReI5ILiIA3HmUbMA6C0xsLfrlYonk$. You are receiving this because you authored the thread.Message ID: ***@***.***>
-
Daniel-Constantin Mierla -
jbipre2