Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls_wolfssl: new module TLS stack based on wolfSSL #3144

Merged
merged 1 commit into from Jun 16, 2022

Conversation

space88man
Copy link
Contributor

New module: add wolfSSL as alternate TLS stack.

Pre-Submission Checklist

  • 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:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

This a new module: an alternate TLS implementation based on wolfSSL. The current tls module based on OpenSSL has many multi-process workarounds and can be quite fragile.

This is the initial code dump which is a copy of tls/ and edited to compile with wolfSSL by using the OpenSSL compatibility layer. The doc/ directory has not been changed.

The proposal is to get it into the code base as soon as possible so as to sync up with any ongoing changes in the tls/ module. Any shared features can be extracted out into a common module: like certificate and configuration.

In the short-term the steps are:

  • testing
  • use native wolfSSL APIs (remove OpenSSL compatibility layer)
  • remove OpenSSL multi-process hacks in this module

This module is inspired by the tls_wolfssl module in the sister SIP project.

@miconda
Copy link
Member

miconda commented Jun 14, 2022

Thanks for this contribution!

Is the module at the phase of "it compiles only" or was it also tested a bit with some tls connections and SIP traffic?

@space88man
Copy link
Contributor Author

space88man commented Jun 14, 2022

It is at "it compiles only" + server starts without segfault. Currently it terminates TLSv1.2 connections. The naive port at this stage does not achieve TLSv1.3. Need to initialise the SSL_CTX appropriately.

Update: the PR is locally tested and terminates TLSv1.2/TLSv1.3 and passes SIP messages.

Is the module at the phase of "it compiles only" or was it also tested a bit with some tls connections and SIP traffic?

@space88man space88man force-pushed the wolfssl branch 4 times, most recently from a8cea2e to 57464e8 Compare June 16, 2022 03:05
Initial support. Use OpenSSL-compatiblity layer to achieve
compilation.
@miconda
Copy link
Member

miconda commented Jun 16, 2022

Thanks for the details and further work on it! I can merge it all grant you access to the kamailio repo in order to continue the development directly on the main source tree. You can still do pull requests if you want other developers to comment on changes to this module. If you want to push commits to other modules or components, it is recommended to make pull requests, so the developers have a chance to review and avoid conflicts on working on same code part at that moment.

@miconda miconda merged commit c08a7cb into kamailio:master Jun 16, 2022
@space88man
Copy link
Contributor Author

Thanks for the details and further work on it! I can merge it all grant you access to the kamailio repo in order to continue the development directly on the main source tree. You can still do pull requests if you want other developers to comment on changes to this module. If you want to push commits to other modules or components, it is recommended to make pull requests, so the developers have a chance to review and avoid conflicts on working on same code part at that moment.

Yes - I would appreciate access for further development of this module. Thank you.

@henningw
Copy link
Contributor

Hello, thanks for the pull request. It was merged really fast. I would appreciate some comments on how to address the code duplication from the existing tls module. Just checked briefly, but several header files are completely identical, in some cases just a few differences (tls_rpc.h, tls_ct_q.h etc..) . Also some implementation is idential (tls_verify.c) The approach from tlsa module to just include the headers if they are not modified seems better to me than to just copy them completely. This is one of the more complicated modules, and now we are having three tls modules.

@miconda
Copy link
Member

miconda commented Jun 16, 2022

@henningw - if you read the PR description: Any shared features can be extracted out into a common module: like certificate and configuration.

But first is to get the module working properly, if proved not feasible at the end, then it can be removed. If ok, then think about next steps.

@henningw
Copy link
Contributor

Thanks @miconda for the clarification. If this is work in progress and will be then further improved in the git master, fine with me. I did not got from the description that this extraction or refactoring regarding code duplication was planned from the original author.

@space88man
Copy link
Contributor Author

space88man commented Jun 16, 2022

@henningw @miconda - a somewhat related question before a hypothetical tls_common (or tls_mgm in OpenSIPS) intermediate module: in master and 5.6 do we really want to continue to support older versions of OpenSSL (< 1.1.1)? This backward compatibility leads to a lot of special casing and gnarly code.

If the argument is for building newer kamailio on systems where the packaged or system version is OpenSSL ≤ 1.0.2 then the argument could also be made that such users could build OpenSSL ≥ 1.1.1 first.

OpenSSL 1.0.2/1.1.0 are EoL in 2019 and perhaps we should not facilitate such outdated libraries.

Some earlier crash reports from the 1.0.2 to 1.1.x upgrade were due to changes in multi-process handling in OpenSSL 1.1.x; this has been stabilised now (e.g. duplicating SSL_CTX per worker, atexit hook, pthread polyfill in core) so keeping 1.0.2 and earlier support does not seem worthwhile .

@miconda
Copy link
Member

miconda commented Jun 16, 2022

@space88man: there are many deployments on older systems, so I would not remove anything from tls module right now.

I think it is better to get first the tls_wolfssl working properly as an alternative to the tls (openssl) module. But, being a new module, it can be developed to only support the newer versions of libwolfssl and without being compatible with old versions of libssl (openssl). In other words, you can remove the parts that are related to OpenSSL < 1.1.1 in the new module tls_wolfssl.

Once tested and results are satisfactory, then we can look what are the common parts still left between the two modules and decide what would be a best way to deal with. But I won't spend time now to think about nor even attempt to extract code from tls module to another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants