[kamailio/kamailio] stirshaken: Properly handle intermediary/chain certificates when caching certificates (PR #3175)
2 Jul
2022
2 Jul
'22
8:18 p.m.
<!-- 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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3175(a)github.com&gt;
4 Jul
4 Jul
4:20 a.m.
Thanks, not sure if @piotr-gregor is still around and wants to review, if not or no other
comments, it will be merged soon.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1173505896
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1173505896(a)github.com>
6:07 a.m.
Thanks! Sure, I'll review.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1173623542
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1173623542(a)github.com>
7:28 a.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#pullrequestreview-1027418018
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/review/1027418018(a)github.com>
8:14 a.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1173749162
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1173749162(a)github.com>
8:25 a.m.
@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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1173759431
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1173759431(a)github.com>
8:47 a.m.
@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.
+ 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;
+
+ 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.
+ LM_ERR("Failed to open %s: %s",
name, strerror(errno));
+ 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);
```
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#pullrequestreview-1027482224
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/review/1027482224(a)github.com>
12:47 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1174000276
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1174000276(a)github.com>
1:24 p.m.
This patch (even in 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..).
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1174021943
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1174021943(a)github.com>
5 Jul
5 Jul
8:03 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1175615404
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1175615404(a)github.com>
8:13 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1175620568
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1175620568(a)github.com>
6 Jul
6 Jul
7:23 a.m.
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.
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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1176103835
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1176103835(a)github.com>
8 Aug
8 Aug
10:17 a.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1208190924
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1208190924(a)github.com>
26 Aug
26 Aug
3:46 a.m.
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).
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1228167237
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1228167237(a)github.com>
8 Sep
8 Sep
1:24 a.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#issuecomment-1240235020
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/c1240235020(a)github.com>
30 Nov
30 Nov
4:08 a.m.
@mrtrev pushed 0 commits.
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3175/files/88b19e9d4fcbf78d0b660a…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/push/11831941331(a)github.com>
4:08 a.m.
Closed #3175.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3175#event-7919967549
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3175/issue_event/7919967549(a)github.com>
758
days inactive
908
days old
16 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Later
-
mrtrev