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

Add configuration option for TLS 1.3 ciphersuites #1118

Merged
merged 5 commits into from
Dec 16, 2022

Conversation

sysvinit
Copy link
Contributor

@sysvinit sysvinit commented Dec 8, 2022

There are two different API's in OpenSSL for configuring TLS ciphers, one for TLS 1.2 and below, and another for TLS 1.3. coturn only calls the TLS 1.2 API when handling the --cipher-list configuration option, which means that it's not possible to use non-default ciphersuites with TLS 1.3 connections.

This PR introduces a new --ciphersuites option, which calls the appropriate OpenSSL API to allow TLS 1.3 ciphersuites to be configured.

@eakraly
Copy link
Collaborator

eakraly commented Dec 9, 2022

Thank you @sysvinit !

I do not like the idea of having additional configuration option of TLS 1.3 - there is a lot of duplication with no value

It can be configured by the same string: according to openssl - same cipher list/suits can be passed to both APIs and the values that are unknown will be ignored.

So when having TLS 1.3 available and enabled - also call SSL_set_ciphersuites with the same list

See https://www.openssl.org/docs/manmaster/man3/OSSL_default_ciphersuites.html#NOTES

Also better use OSSL_default_cipher_list() and OSSL_default_ciphersuites() as

OSSL_default_cipher_list() and OSSL_default_ciphersuites() replace SSL_DEFAULT_CIPHER_LIST and 
TLS_DEFAULT_CIPHERSUITES, respectively. The cipher list defines are deprecated as of 3.0.

@sysvinit
Copy link
Contributor Author

I do not like the idea of having additional configuration option of TLS 1.3 - there is a lot of duplication with no value

It can be configured by the same string: according to openssl - same cipher list/suits can be passed to both APIs and the values that are unknown will be ignored.

So when having TLS 1.3 available and enabled - also call SSL_set_ciphersuites with the same list

Understandable, I can remove the separate option and call both API's with the same cipher string.

Also better use OSSL_default_cipher_list() and OSSL_default_ciphersuites() as

OSSL_default_cipher_list() and OSSL_default_ciphersuites() replace SSL_DEFAULT_CIPHER_LIST and 
TLS_DEFAULT_CIPHERSUITES, respectively. The cipher list defines are deprecated as of 3.0.

These functions were introduced in OpenSSL 3.0 and do not exist in OpenSSL 1.1 -- in particular, Debian stable ships with 1.1, which is what one of coturn's container images uses. We therefore can't just switch over to these new functions, as this would break compiling with older OpenSSL versions. Either we can continue using the deprecated defines, or test the version of OpenSSL in the preprocessor to select either the defines or the functions as appropriate.

OpenSSL has separate API's for cipher configuration for TLSv1.2 (and below)
and TLSv1.3. Previously, only the former API was called, which left no way
to configure TLSv1.3 ciphersuites. The OpenSSL documentation says that the
configuration strings for the 1.2 and 1.3 API's are compatible (and passing 1.2
ciphers to the 1.3 API will be ignored, and vice versa), so this commit extends
the processing of the `--cipher-list` option to additionally configure TLS 1.3
ciphersuites where available.
strncpy(turn_params.cipher_list, DEFAULT_CIPHER_LIST, TURN_LONG_STRING_SIZE);
#if TLSv1_3_SUPPORTED
strncat(turn_params.cipher_list, ":", TURN_LONG_STRING_SIZE - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Buffer overflow: max size allowed to be appended at this point is TURN_LONG_STRING_SIZE - strlen(turn_params.cipher_list)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4f20d01?

strncpy(turn_params.cipher_list, DEFAULT_CIPHER_LIST, TURN_LONG_STRING_SIZE);
#if TLSv1_3_SUPPORTED
strncat(turn_params.cipher_list, ":", TURN_LONG_STRING_SIZE - 1);
strncat(turn_params.cipher_list, DEFAULT_CIPHERSUITES, TURN_LONG_STRING_SIZE - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4f20d01?

Copy link
Collaborator

@eakraly eakraly left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@eakraly eakraly merged commit 902cb99 into coturn:master Dec 16, 2022
@eakraly
Copy link
Collaborator

eakraly commented Dec 16, 2022

@sysvinit I took the liberty to change the PR description slightly to reflect the change done (following the update you made).

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.

2 participants