[kamailio/kamailio] stirshaken: Properly handle intermediary/chain certificates when caching certificates (PR #3289)
stirshaken: Properly handle intermediary/chain certificates when caching certificates - requires patch to libstirshaken (https://github.com/signalwire/libstirshaken/pull/124) to do anything - if patched version of libstirshaken detected, uses new methods to store all intermediary certs - unrelated minor logging tweaks
<!-- 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 -->
The guts of this PR have been submitted to the libstirshaken side based on the suggestions of my previous PR. I didn't mean to close it but since I hadn't created a branch, it got closed when I updated my repo. Original PR is https://github.com/kamailio/kamailio/pull/3175
I've been running this in production for a day and it seems to be performing the same as my previous attempt.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3289
-- Commit Summary --
* stirshaken: Properly handle intermediary/chain certificates when caching certificates
-- File Changes --
M src/modules/stirshaken/stirshaken_mod.c (24)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3289.patch https://github.com/kamailio/kamailio/pull/3289.diff
@miconda @piotrgregor @henningw Tagging you as I accidentally closed the previous PR you commented on.
just curious if you see a memleak with that code and libstirshaken with PR124.
I applied the patch to branch 5.6 and can see the free memory going down since the last restart.
It seems to be caused by the call to stirshaken_check_identity(), as commenting it and using secsipid_check_identity("") no leaks are present ( did an A/B test with same config just a line commented out ).
Params tested and still leaking: - cache certificate on or off. - root chain validation on or off. - vs_*_pvname set or unset.
I haven't noticed the leak myself but I'm not running on high volume.
Upon closer inspection of libstirshaken it doesn't seem to care about freeing the certificate chain in many cases which could explain it regardless if any of the recent PRs are applied. Notably stir_shaken_cert_destroy() and stir_shaken_cert_deinit() seem to free the cert->x but not cert->xchain. https://github.com/signalwire/libstirshaken/blob/master/src/stir_shaken_ssl....
I'll see if I can get another PR submitted to that project to deal with it.
@gled-rs https://github.com/signalwire/libstirshaken/pull/125 should help.
Upon closer inspection of libstirshaken it doesn't seem to care about freeing the certificate chain in many cases which could explain it regardless if any of the recent PRs are applied.
Correct. libstirshaken didn't make a use of `xchain` only after your PR @mrtrev
in particular this was never used https://github.com/signalwire/libstirshaken/blob/e8289b0967a7b008dd666ee8880...
so it is on your behalf to make sure it's correctly initialised and freed
Correct. libstirshaken didn't make a use of xchain only after your PR
Actually before I started tinkering, it was using that code you linked to, which is why it could validate certificates the first time yet failed when they loaded from cache. It leaks memory even when loading a certificate+intermediary the first time. It simply leaks even more now that it can load certificate+intermediary from cache.
At any rate I have submitted https://github.com/signalwire/libstirshaken/pull/125 that will ensure xchain is cleaned up in all use cases.
All 3 of these PRs can be merged independently of each other.
I got no time to test that right now, but that could be the case. Thanks.
test in progress, but it's been only a couple hours, so will need more time to be sure.
This looks good though, thanks @mrtrev :)
seems like the leak is gone, well done @mrtrev !
Thanks for the pull request and the feedback/tests. This seems to be a small extension, which also has backward compatiblity if the library does not support it. If there are no more comments, it will be merged in the next days.
@henningw Any chance this could also be cherry picked into the 5.6 branch as well?
Merged #3289 into master.
I've merged it into git master. Regarding chery picking into stable branch, as far as I understood it, it seems to work generally, just without caching the certificates. So its seems to be not a bug fix, but a feature extension, and these are not introduced into stable branches.
Thanks for merging to master.
As far as the problem, I do see it as a bug fix. Currently if you choose to cache certificates, validation will fail on any cached certificate (at least in cases where an intermediary cert is required, which is most if not all cases in Canada). The workaround is to disable caching, which is a very high cost workaround.
It would not fail on any cached cetificates, that's not a problem this solved... What this fixes instead, is handling of cached certificates with intermediary certs and probably a memory leak that would manifestate for any cached certificate.
Hi @piotrgregor
The whole reason I started digging into this is precisely because I was seeing this in Kamailio logs
ERROR: stirshaken [stirshaken_mod.c:488]: ki_stirshaken_check_identity(): SIP Identity Header did not pass verification ERROR: stirshaken [stirshaken_mod.c:560]: ki_stirshaken_check_identity(): identity check: fail
Upon investigation I determined this happened any time a certificate was cached that relied on a chain. The first call will succeed but any subsequent calls fail until the cached certificate expires or is manually deleted. Then the next call will validate ok, but subsequent calls fail.
This happens today with 5.6.2 on my unpatched machine.
Ok, if it fails in the caching mode then its indeed a bug fix. I've cherry-picked it to the 5.6 and 5.5 branches.
-
Henning Westerholt -
Later
-
mrtrev
-
Tristan Mahé