- Add new function to clear OpenSSL errors prior to any SSL_* call - Apply function where appropriate
<!-- 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 --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [x] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> This PR proposes similar fixes like https://github.com/kamailio/kamailio/pull/3607.
OpenSSL docs suggest that the error stack must be empty for [SSL_get_error() to work reliably](https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html).
_Note: This has not been tested like previous PR and any feedback of course is appreciated._ You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3612
-- Commit Summary --
* cdp: Add and apply cdp_openssl_clear_errors function
-- File Changes --
M src/modules/cdp/cdp_tls.c (2) M src/modules/cdp/receiver.c (3) A src/modules/cdp/utils.c (21) M src/modules/cdp/utils.h (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3612.patch https://github.com/kamailio/kamailio/pull/3612.diff
@lbalaceanu commented on this pull request.
Hello @xkaraman ,
Thank you for your contribution. I will also test minimally the code and give an input after that as well.
+#include "../../core/dprint.h"
+#include "utils.h" + +/* + * Get any leftover errors from OpenSSL and print them. + * ERR_get_error() also removes the error from the OpenSSL error stack. + * This is useful to call before any SSL_* IO calls to make sure + * we don't have any leftover errors from previous calls (OpenSSL docs). + */ +void cdp_openssl_clear_errors(void) +{ + int i; + char err[160]; + while((i = ERR_get_error())) { + ERR_error_string(i, err); + INFO("clearing leftover error before SSL_* calls: %s", err);
Hello,
Just a cosmetic change, but maybe this should be a LM_INFO call.
+#include <openssl/ssl.h>
+#include <openssl/err.h> + +#include "../../core/dprint.h" +#include "utils.h" + +/* + * Get any leftover errors from OpenSSL and print them. + * ERR_get_error() also removes the error from the OpenSSL error stack. + * This is useful to call before any SSL_* IO calls to make sure + * we don't have any leftover errors from previous calls (OpenSSL docs). + */ +void cdp_openssl_clear_errors(void) +{ + int i; + char err[160];
Maybe 256 should be a better value, or maybe use ERR_error_string_n? I saw in https://www.openssl.org/docs/man3.0/man3/ERR_error_string.html they suggest 256 for the buffer.
@xkaraman commented on this pull request.
+#include "../../core/dprint.h"
+#include "utils.h" + +/* + * Get any leftover errors from OpenSSL and print them. + * ERR_get_error() also removes the error from the OpenSSL error stack. + * This is useful to call before any SSL_* IO calls to make sure + * we don't have any leftover errors from previous calls (OpenSSL docs). + */ +void cdp_openssl_clear_errors(void) +{ + int i; + char err[160]; + while((i = ERR_get_error())) { + ERR_error_string(i, err); + INFO("clearing leftover error before SSL_* calls: %s", err);
Sure thing!
@xkaraman commented on this pull request.
+#include <openssl/ssl.h>
+#include <openssl/err.h> + +#include "../../core/dprint.h" +#include "utils.h" + +/* + * Get any leftover errors from OpenSSL and print them. + * ERR_get_error() also removes the error from the OpenSSL error stack. + * This is useful to call before any SSL_* IO calls to make sure + * we don't have any leftover errors from previous calls (OpenSSL docs). + */ +void cdp_openssl_clear_errors(void) +{ + int i; + char err[160];
Yep sounds more inline with docs, ofc! Initial 160 was inspired by the fix found in [rtpengine itself](https://github.com/sipwise/rtpengine/blob/eaf437615b772745208da61e51311b72df...)
@xkaraman pushed 1 commit.
eb36512a644a92e6764ad53a8b4c4f23740c96ba cdp: Expand buffer and change info call
Hello,
Thanks for the changes.
I did manage to get some extra information with this new function so the addition is welcome.
Sorry to come just now with another suggestion: we can use cdp_tls.h/cdp_tls.c to house the cdp_openssl_clear_errors() function. This would get rid of the extra utils.c file.
LM_INFO also requires \n at end of line.
Sure thing, I will revise it! Any specific order you may want it to appear?
In the cdp_tls.c I would put the function before the first one that uses it, possibly SSL *init_ssl_conn(int client_fd, SSL_CTX *ctx). In the cdp_tls.h, maybe just under tls_parse_method(). Much appreciated.
@xkaraman pushed 1 commit.
79ba6edcaea2b31823ffbfbfc79cc41d4c1ced24 cdp: Move function to cdp_tls
Done.
@xkaraman pushed 1 commit.
7d0b7e0bd49b8534b35c8c10a41c4d6ca86b9164 cdp: Remove extra line
From my point of view this PR can be squashed and merged.
Merged #3612 into master.
Thanks, merged
-
Henning Westerholt -
Lucian Balaceanu
-
Xenofon Karamanos