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

OpenSSL 3.0: Offer TLS providers as an alternative to TLS engines in DNSdist #12423

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

fredmorcos
Copy link
Contributor

@fredmorcos fredmorcos commented Jan 16, 2023

Short description

OpenSSL 3.0 replaced engines with providers. This change offers loadTLSProvider as an alternative to loadTLSEngine. This is currently used for e.g. HW accelerated crypto: https://github.com/intel/QAT_Engine/blob/master/docs/qat_common.md#openssl-30-provider-support

Providers are disabled by default. The configure flag --enable-tls-providers is available to force-enable them over engines. It is marked as experimental though.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@fredmorcos fredmorcos requested a review from rgacogne January 16, 2023 15:02
@fredmorcos fredmorcos self-assigned this Jan 16, 2023
@fredmorcos
Copy link
Contributor Author

fredmorcos commented Jan 16, 2023

Regarding build guards (i.e. #ifdefs)

  • Add an experimental --enable-tls-providers build flag
  • Ensure that loadTLSEngine and related functions are only built when #if defined(HAVE_LIBSSL) && !defined(ENABLE_TLS_PROVIDERS)
  • Ensure that loadTLSProvider and related functions are only built when #if defined(HAVE_LIBSSL) && OPENSSL_VERSION_MAJOR >= 3 && defined(ENABLE_TLS_PROVIDERS): This is due to the Intel QAT OpenSSL provider still being experimental.

Documentation

  • Documentation needs to be double-checked.
  • Which DNSdist version will this be introduced in?

Tests

  • Run tests for loadTLSEngine with those changes.
  • Add tests for loadTLSProvider.

@rgacogne
Copy link
Member

* [ ]  Add an experimental `--enable-tls-provider` build flag

I was pondering doing it the other way around: have a --enable-deprecated-tls-engine that would use the TLS engine interface when building against OpenSSL 3.0.x. It's officially deprecated but it might be the only way to support some functionalities for which a TLS provider is not available.

* [ ]  Which DNSdist [version](https://github.com/PowerDNS/pdns/pull/12423/files#diff-abd4de1691f1c874111fbd923e679855f845526ea13bca77f6084be3d187f7b1R1860) will this be introduced in?

I'm aiming for 1.8.0.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick test by loading the 'legacy' provider and that works. I'll try with QAT later today.

@rgacogne
Copy link
Member

I'll try with QAT later today.

So.. Technically I managed to load the QAT provider, and to handle a few TLS connections with it. However it crashed pretty quickly in the QAT provider itself, but I don't think this is on us, the QAT provider is very experimental and from what I read not really tested.
I manually edited the guard to restore the ability to load an OpenSSL engine even with OpenSSL >= 3.0 and it works as expected. So I believe we really need to make it easy to have the ability to use OpenSSL engines even when compiled against OpenSSL >= 3.0, as providers might not be production-ready for a while. How about adding --enable-tls-providers and --enable-tls-engines? I'm not even sure we need to make them exclusive, to be honest, so we might be able to just implement --enable-tls-engines, disabled by default, and only enable providers when OpenSSL >= 3.0?

@fredmorcos
Copy link
Contributor Author

fredmorcos commented Feb 6, 2023

so we might be able to just implement --enable-tls-engines, disabled by default, and only enable providers when OpenSSL >= 3.0?

I'm not sure this is a good idea given the sorry state of the Intel QAT provider (which is the only use-case I know of so far). I would prefer having engines enabled by default and --enable-tls-providers which only works with OpenSSL >= 3.0. Is that OK?

@rgacogne
Copy link
Member

rgacogne commented Feb 6, 2023

so we might be able to just implement --enable-tls-engines, disabled by default, and only enable providers when OpenSSL >= 3.0?

I'm not sure this is a good idea given the sorry state of the Intel QAT provider (which is the only use-case I know of so far). I would prefer having engines enabled by default and --enable-tls-providers which only works with OpenSSL >= 3.0. Is that OK?

Yes, I think you are right that this is a better default. I'm a bit sad because it means we will have some remaining warnings by default when building with OpenSSL >= 3.0, because of the deprecated engines, but in practice this is likely what most users are going to want for now.

@fredmorcos fredmorcos marked this pull request as ready for review February 7, 2023 18:11
@fredmorcos fredmorcos changed the title OpenSSL 3.0: Replace TLS engines with TLS providers in DNSdist OpenSSL 3.0: Offer TLS providers as an alternative to TLS engines in DNSdist Feb 8, 2023
@rgacogne rgacogne merged commit 0d2c913 into PowerDNS:master Feb 9, 2023
@fredmorcos fredmorcos deleted the ddist-tls-provider branch February 9, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants