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

Support TLS 1.3 #8718

Merged
merged 1 commit into from
Jun 29, 2021
Merged

Support TLS 1.3 #8718

merged 1 commit into from
Jun 29, 2021

Conversation

julianbrost
Copy link
Contributor

@julianbrost julianbrost commented Apr 14, 2021

This PR adds support for TLS 1.3.

Unfortunately, there's no method value for ssl::context that specifies TLS 1.2 or newer but only any TLS version, i.e. TLS 1.0 or newer (https://www.boost.org/doc/libs/1_75_0/doc/html/boost_asio/reference/ssl__context/method.html). That's not too much of an issue though as the call to SetupSslContext() will disable TLS 1.0 and 1.1.

fixes #8715

@icinga-probot icinga-probot bot added this to the 2.13.0 milestone Apr 14, 2021
@julianbrost julianbrost force-pushed the feature/tls-1.3 branch 4 times, most recently from 447b3d6 to 8c2706d Compare April 15, 2021 10:23
@julianbrost julianbrost added the enhancement New feature or request label Apr 15, 2021
@julianbrost julianbrost marked this pull request as ready for review April 15, 2021 11:01
@julianbrost
Copy link
Contributor Author

Noteworthy limitation of this PR: There is currently no way to configure the set of accepted cipher suites for TLS 1.3. Icinga currently uses SSL_CTX_set_cipher_list() which only allows to configure ciphers for TLS 1.2 and lower, for TLS 1.3 there is SSL_CTX_set_ciphersuites() but there seems to be no API that supports both at the same time.

So there are a few options now:

  1. Say that this is fine as TLS 1.3 only has modern crypto
  2. Add a second config attribute for TLS 1.3 cipher suites
  3. Attempt to figure out which cipher Name goes into which function and try to split the value of existing config attribute (I don't like this option as the old cipher strings are quite complex and we'd have to put in quite some parsing effort not to break them)
  4. Do something like nginx did and add an attribute that uses the SSL_CONF_cmd API of OpenSSL to allow setting all kind of options.

@lippserd
Copy link
Member

lippserd commented Jun 7, 2021

  1. Say that this is fine as TLS 1.3 only has modern crypto

I would do it that way.

@Al2Klimov Al2Klimov mentioned this pull request Jun 8, 2021
@julianbrost julianbrost requested a review from Al2Klimov June 21, 2021 09:23
lib/base/tlsutility.cpp Outdated Show resolved Hide resolved
lib/base/tlsutility.cpp Show resolved Hide resolved
lib/base/tlsutility.cpp Show resolved Hide resolved
lib/base/tlsutility.cpp Outdated Show resolved Hide resolved
lib/base/tlsutility.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for TLS 1.3
3 participants