[kamailio/kamailio] stirshaken: Properly handle intermediary/chain certificates when caching certificates (PR #3175)
<!-- 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 --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] 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) - [ ] 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 - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> When the stirshaken module is in use and configured to cache certifications, validation will succeed on the very first attempt but will then fail every time the certificate is loaded from cache. The reason is because this module only saves the certificate and discards the any supplied chain certificates. This patch causes the module to save all supplied certificates and properly loads them upon retrieval.
For the loading to work a patch is required in libstirshaken. A PR has already been submitted and is linked below. Without that patch the problem will persist but no other harm is done. This is a safe change to make that does not break existing behaviour.
- save all certificates provided by signor to the disk cache - properly load all certificates when loading from cache - requires patch to libstirshaken (PR 123); this patch causes no harm (but no benefit) without it - resolve unrelated compiler warnings on 32bit systems
https://github.com/signalwire/libstirshaken/pull/123
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3175
-- Commit Summary --
* stirshaken: Properly handle intermediary/chain certificates when caching certificates * stirshaken: close file in write failure cases
-- File Changes --
M src/modules/stirshaken/stirshaken_mod.c (106)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3175.patch https://github.com/kamailio/kamailio/pull/3175.diff
Thanks, not sure if @piotr-gregor is still around and wants to review, if not or no other comments, it will be merged soon.
Thanks! Sure, I'll review.
@piotrgregor commented on this pull request.
@mrtrev Thank you very much for the PR. Indeed, certificate chain is not handled in libstirshaken when doing disk I/O. These changes would ideally go there. Methods that need to be changed (in libstirshaken) are:
`stir_shaken_load_x509_from_file` - read complete cert/chain object with `PEM_read_X509`/`sk_X509_push` just as you're doing that in `stirshaken_handle_cache_from_disk`
`stir_shaken_x509_to_disk` - write complete cett/chain object with `PEM_write_X509`/`sk_X509_num` just as you're doing this in `stirshaken_handle_cache_to_disk`
Can you please suggest these changes to [libstirshaken](https://github.com/signalwire/libstirshaken)? Then we do not need to make changes to this module (maybe just the logging related).
@miconda I suggest this is handled in libstirshaken, then optionally cosmetic changes (@mrtrev proposed also some more logging) are merged.
@piotrgregor: thanks for looking into it!
One question: I understand that the change would be better in the libstirshaken, but till that is done, because it may take some time, we can have this in Kamailio module, right?
I mean, from the c code point of view, all is fine with the PR.
Once the library is updated, the code of Kamailio module can be changed again.
Yes, maybe with some cosmetic changes, I'll point them out now
@piotrgregor requested changes on this pull request.
@@ -158,6 +157,96 @@ static int get_cert_name_hashed(const char *name, char *buf, int buf_len)
return 0; }
+static int stirshaken_handle_cache_to_disk(X509 *x, STACK_OF(X509) *xchain, const char *cert_full_path) +{ + int i = 0; + int w = 0; + FILE *fp = NULL; + X509 *xc = NULL; + + if (!x) { + LM_ERR("Refusing to save an empty cert\n"); + return -1; + }
Please also check `xchain` for NULL and call `sk_X509_num` only if it's not.
@@ -158,6 +157,96 @@ static int get_cert_name_hashed(const char *name, char *buf, int buf_len)
return 0; }
+static int stirshaken_handle_cache_to_disk(X509 *x, STACK_OF(X509) *xchain, const char *cert_full_path) +{ + int i = 0; + int w = 0; + FILE *fp = NULL; + X509 *xc = NULL; + + if (!x) { + LM_ERR("Refusing to save an empty cert\n"); + return -1; + } +
Please also check `cert_full_path` against NULL pointer or pointer to an empty string.
+ fp = NULL; + + return 0; + +fail: + if (fp) fclose(fp); + return -1; +} + +static int stirshaken_handle_cache_from_disk(stir_shaken_context_t *ss, stir_shaken_cert_t *cert, const char *name) +{ + FILE *fp = NULL; + X509 *wcert = NULL; + + LM_DBG("Handle cache from disk; %s", name);
Please move it below line 214, after `stir_shaken_zstr` check.
+ if (fp) fclose(fp); + fp = NULL; + + return 0; + +fail: + if (fp) fclose(fp); + return -1; +} + +static int stirshaken_handle_cache_from_disk(stir_shaken_context_t *ss, stir_shaken_cert_t *cert, const char *name) +{ + FILE *fp = NULL; + X509 *wcert = NULL; +
Please check `cert` against NULL.
+ goto fail; + } + + cert->xchain = sk_X509_new_null(); + + while ((wcert = PEM_read_X509(fp, NULL, NULL, NULL))) { + if (!cert->x) { + cert->x = wcert; + } + else { + sk_X509_push(cert->xchain, wcert); + } + } + + LM_DBG("done reading file, got %d certs and %d chains", cert->x ? 1 : 0, sk_X509_num(cert->xchain));
Let's handle plural/singular form in a log message. Maybe something like: ``` n = sk_X509_num(cert->xchain)); LM_DBG("done reading file, got %d cert%s and %d chain%s", cert->x ? 1 : 0, cert->x ? "", "s", n == 1 ? "" : "s", n); ```
Also a minor clarification @miconda This patch (even with this current form) needs a patch to libstirshaken (to process chain when copying cert). So, maybe instead let's have a proper commit to libstirshaken, that would handle also a read/write to disk I/O, then there is no need to introduce new methods to Kamailio module in this commit.
If there is the need for a patch anyway in libstirshaken that this works, then it would be also my preferred way to solve it there. Then we do not need to change the code in the kamailio module and also do not need to remove it later again (which might be forgotten etc..).
Hi Folks,
I chose to do this on the Kamailio side as I'm less sure about the impacts to other applications that may use libstirshaken, whereas I understand the full use-case of the kamailio module. I've watched a few other PRs and bug reports to libstirshaken sit untouched for many months at a time so also less optimistic about that leading to a quick resolution.
The PR I already submitted (a one-line change) has already been accepted and merged, fyi.
@piotrgregor If these changes were go to into libstirshaken and then Kamailio fully outsources that work to the library, Kamailio would need to enforce a minimum version of libstirshaken be installed to know that it will work. I don't believe the library has versioning at the moment. Ideas on how to solve that for users who are using a newer Kamailio with a yet-to-be-patched libstirshaken?
Yes, libstirshaken library wasn't maintained very actively, however your last PR could have been merged much more smoothly, so maybe you could try with the complete patch? To solve versioning, we would add versioning to libstirshaken AC_INIT macro in configure.ac, then we would check version with configure and set a define to be used in Kamailio module, if libstirshaken version is old then module would use your new functions, otherwise libstirshaken can be used.
Yet another solution is to use a fork (e.g. this my forked version https://github.com/dataandsignal/libstirshaken) so we can merge this and other commits quicker. Effect would be the same as patching Kamailio module, new Kamailio version would rely on a fork with new feature, old version would not have the feature.
So: 1. You can try to merge versionng commit, and chain changes to signalwire/libstirshaken 2. Use my fork, and I can help with merging this quickly
@mrtrev @piotrgregor Any conclusion from the previous discussion? I see there are some open remarks from the review, and also the library discussion might be still open.
It seems the needed PR in libstirshaken was merged pretty quickly:
- https://github.com/signalwire/libstirshaken/pull/123
Not sure about other things that need to be clarified for this PR to be merged.
I see that @piotrgregor is also willing to go with a forked repository of libstirshaken that he wants to maintain at a faster pace. As I am using Kamailio with secsipid module for STIR/SHAKEN, thus not really impacted by any change to stirshaken module, I feel it can bring confusion and conflicts over the time if both `libstirshaken` (the original and the fork) are developed further and diverge. Usually people tend to go to the original project or have that lib also installed because of other dependencies.
For example, the original libstirshaken is used by FreeSwitch, which will be installed as a deb/rpm dependency. Then we can't get Kamailio installed on the same system having a dependency with same name but from another repo. I faced something similar as FreeSwitch forked (and diverged) libspandsp (iirc), which conflicts with the original one that is needed by RTPEngine.
IMO, a fork of a repository should retain the name if wants to contribute back to the original. If wants to diverge in development and offer another viable alternative, it should be renamed to something else to avoid conflicts like I exemplified above.
Again, with not much personal interest in this module, it is probably better that @piotrgregor renames its repo then also "forks" the Kamailio module to another one using the new name, then works on it. That adds some overhead, but we have many other modules for similar purpose (e.g., lcr, drouting) and over the time maybe it becomes clear which one to keep or remove (like it happened with jabber vs xmpp module).
I'm content with refactoring the code to be merged in the libstirshaken side, I'm just not sure when I will have the chance to.
Having said that I am using the submitted PR in production for a number of months now and haven't had any trouble with it.
@mrtrev pushed 0 commits.
Closed #3175.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Later
-
mrtrev