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 ssl_ciphersuites option for TLSv1.3 ciphers #3359

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

willayton
Copy link
Contributor

@willayton willayton commented Apr 2, 2024

Description

Closes #3343

As described in the linked issue, it's desirable to be able to restrict the ciphersuites used for TLSv1.3 connections for security reasons, but Puma doesn't yet support that. https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_cipher_list.html explains that OpenSSL has a different setting for TLSv1.3 ciphers than for TLSv1.2 and below.

This pull request adds a new option to pass in TLSv1.3 ciphersuites config - ssl_ciphersuites. I've explained its usage in the README. This is intentionally similar to the existing ssl_cipher_filter option, which does the same job for TLSv1.2 and below.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • [n/a] If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg dentarg added ssl waiting-for-review Waiting on review from anyone labels Apr 3, 2024
@dentarg
Copy link
Member

dentarg commented Apr 3, 2024

Looks good to me but @MSP-Greg should take a look

Can you show how to verify this externally? Like, can I test these changes with curl perhaps?

@MSP-Greg
Copy link
Member

MSP-Greg commented Apr 3, 2024

Can you show how to verify this externally?

We may be able to check it in the test suite, not sure. I'll try to look at it a bit later. Using an SSLSocket for a client, this setting should change the SSLSocket#cipher value when the connection is established.

@MSP-Greg
Copy link
Member

MSP-Greg commented Apr 3, 2024

@willayton

I've got two commits at the top of https://github.com/MSP-Greg/puma/commits/pr-3359/ . I can add them to your branch, thought another pair of eyes would be good...

The first adds some 'guards' around the code, as Windows Ruby 2.4 tests with OpenSSL 1.0.2. I don't recommend that people use it, but as long as we can maintain compatibility without too much code cluttering things up, we'll continue to do so.

Second commit adds a test which verifies the code thru to OpenSSL.

@willayton
Copy link
Contributor Author

willayton commented Apr 4, 2024

Thanks @MSP-Greg, I've cherry-picked in your commits, they look good to me. It seems they've broken two workflow checks for ubuntu-20.04, but I'm failing to see how your changes relate to those failures. Any ideas? [Edit: Now the checks have run in this branch since I cherry-picked, those ubuntu checks seem to be passing. Just a spurious failure?]

@dentarg, I previously tested this by adding the new option in test/config/ssl_config.rb and running bundle exec bin/puma -C test/config/ssl_config.rb. I could then use openssl s_client -connect localhost:9292 to dump SSL session info, including the agreed cipher. (I'm working in an ubuntu dev environment.)
MSP-Greg's new test obviously adds regression testing which this approach lacks.

@MSP-Greg
Copy link
Member

MSP-Greg commented Apr 4, 2024

@willayton

Re the tests, we seem to have intermittent failures in TestIntegrationPumactl#test_refork_cluster in both macOS & Ubuntu (no fork on Windows) , and they're using Ruby 3.3 or 'head'.

I haven't had a chance to look at them. The current test suite has some blocking operations that may be causing them. I've got a rewrite of the test suite that fixes many of the blocking problems, I need to find the time to update it (with recent test changes) and see if it's still stable.

Or, the failures are not related to this PR...

I'll give this one more look over a bit later and hopefully merge it.

@willayton
Copy link
Contributor Author

Hey @MSP-Greg, anything left to do here before merge?

@MSP-Greg MSP-Greg merged commit 73b79ac into puma:master Apr 11, 2024
73 of 74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ssl waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to restrict cipher suites for TLS1.3
3 participants