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

Added 'ciphersuites=' method to allow setting of TLSv1.3 cipher suites along with some unit tests #493

Merged
merged 8 commits into from Feb 1, 2022

Conversation

kmdz1
Copy link
Contributor

@kmdz1 kmdz1 commented Jan 18, 2022

Added an instance method, 'ciphersuites=', to OpenSSL::SSL::SSLContext which allows users to set the TLSv1.3 cipher suites. Also, added unit tests for this method and improved the test coverage for the existing 'ciphers=' method.

Resolves #476

@kmdz1 kmdz1 changed the title Added 'ciphersuites=' method to allow setting of TLSv1.3 cipher suites along with some unit tests Resolves #476 Added 'ciphersuites=' method to allow setting of TLSv1.3 cipher suites along with some unit tests Jan 18, 2022
@kmdz1 kmdz1 changed the title Resolves #476 Added 'ciphersuites=' method to allow setting of TLSv1.3 cipher suites along with some unit tests Resolves #476 Jan 18, 2022
@kmdz1 kmdz1 changed the title Resolves #476 resolves #476 Jan 18, 2022
@kmdz1 kmdz1 changed the title resolves #476 Added 'ciphersuites=' method to allow setting of TLSv1.3 cipher suites along with some unit tests Jan 18, 2022
@ioquatix
Copy link
Member

How is this different to just straight ciphers?

@ioquatix ioquatix requested a review from rhenium January 19, 2022 09:18
@eviljoel
Copy link

I work with twkmd12 and can vouch that we need this change.

To answer @ioquatix's question, TLSv1.3 almost completely ignores the 'ciphers' property. (If I remember right, I think the only thing you can do via the 'ciphers' property that affects TLSv1.3 is to disable all TLSv1.3 ciphers [along with all other ciphers for all other SSL/TLS versions].) The only way to get fine grained control of the ciphers used by TLSv1.3 is via the ciphersuites property.

One reason we want this feature is to get access to the CCM TLSv1.3 ciphers which are disabled by default.

Please let me know if you need more details.

@kmdz1
Copy link
Contributor Author

kmdz1 commented Jan 19, 2022

How is this different to just straight ciphers?

'ciphers=' sets cipher suites for TLSv1.2 and below, while 'ciphersuites=' sets them for TLSv1.3.

@ioquatix
Copy link
Member

I understand the need for this functionality but I'm just wondering:

  1. Whether we can't just reuse ciphers= for it (sounds like no?). If semantically it's different it makes sense different accessors, I'm just concerned it introduces more complexity for users.
  2. Whether we need to communicate clearly the use cases where one would use ciphers and ciphersuites. It sounds like we would eventually (i.e. in 5-10 years) deprecate ciphers.

@kmdz1
Copy link
Contributor Author

kmdz1 commented Jan 20, 2022

@ioquatix The behavior of the two OpenSSL functions does differ, so the setters should be separated. For example, the 'str' argument provided to SSL_CTX_set_cipher_list, which only sets TLSv1.2 and below cipher suites, is much more complex (e.g. there's support for logical operators) and there are many more cipher suites involved. Whereas with SSL_CTX_set_ciphersuites, which only sets TLSv1.3 cipher suites, there are at most five cipher suites; usually three, because the two CCM AEAD mode cipher suites are often disabled and the 'str' argument is a colon delimited list of these cipher suites in order of preference. Also, it looks like TLSv1.3 cipher suites are given a higher preference than others, so using one setter would break preference ordering.

Note: OpenSSL's s_client and s_server utilities have different command line options for setting TLSv1.2 and below cipher suites, and TLSv1.3 cipher suites (-ciphers and -ciphersuites respectively).

Seems to me like your option 2 is the best course. It looks like there's a push away from the prior complexity and eventually 'ciphers=' will need to be deprecated.

@ioquatix
Copy link
Member

Thanks for explaining this. I understand now exactly what is being proposed.

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

LGTM.

ext/openssl/ossl_ssl.c Outdated Show resolved Hide resolved
Copy link
Member

@rhenium rhenium 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 working on this!

I left some change requests within the test code. The implementation itself looks good!

test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
ext/openssl/ossl_ssl.c Outdated Show resolved Hide resolved
@kmdz1
Copy link
Contributor Author

kmdz1 commented Jan 20, 2022

@rhenium Thank you for the suggestions!

@kmdz1
Copy link
Contributor Author

kmdz1 commented Jan 21, 2022

Will get to the bottom of why the Windows tests are failing.

@ioquatix
Copy link
Member

Just ping me again here if you need to run more tests :)

@kmdz1
Copy link
Contributor Author

kmdz1 commented Jan 31, 2022

@ioquatix @rhenium Can we get this merged?

@rhenium
Copy link
Member

rhenium commented Feb 1, 2022

All GitHub Actions checks pass now and everything looks ready for merge. Thanks for your work!

@rhenium rhenium merged commit 12250c7 into ruby:master Feb 1, 2022
nobu pushed a commit to nobu/ruby that referenced this pull request Jul 8, 2022
… cipher suites along with some unit tests (ruby/openssl#493)

Add OpenSSL::SSL::SSLContext#ciphersuites= method along with unit tests.

ruby/openssl@12250c7cef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

New feature request: the ability for the user to set TLS 1.3 ciphersuites
4 participants