allow empty lists in SSL_CTX_set_ciphersuites #1511
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
==========================================
+ Coverage 76.95% 76.99% +0.03%
==========================================
Files 425 425
Lines 71546 71547 +1
==========================================
+ Hits 55056 55085 +29
+ Misses 16490 16462 -28 ☔ View full report in Codecov by Sentry. |
samuel40791765
force-pushed
the
ssl-string-empty
branch
from
c09da16
to
2e961c2
Compare
| // Configuring the empty cipher list fails. | ||
| EXPECT_FALSE(SSL_CTX_set_ciphersuites(ctx.get(), "")); |
There was a problem hiding this comment.
Instead of deleting this check for failure… should it be replaced with an explicit check for success of setting the (TLSv1.3) ciphersuite list to an empty string?
There was a problem hiding this comment.
Explicit check for success for SSL[_CTX]_set_cipher_list is on L5758-5763 in the new change :)
But I just realized that I forgot about SSL[_CTX]_set_ciphersuites...
| // Configuring the empty cipher list with |SSL_CTX_set_ciphersuites| | ||
| // also succeeds. | ||
| EXPECT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), "")); | ||
| EXPECT_EQ(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get()))); |
There was a problem hiding this comment.
Thanks, this is good.
Although this doesn't fully verify the pattern used by MySQL+MariaDB with OpenSSL, which is as follows…
- Call both:
SSL_CTX_set_cipher_list("NON-EMPTY list for TLSv1.2")- and
SSL_CTX_set_ciphersuites("") // empty list for TLSv1.3
- Expect that there will be a non-zero number of TLSv1.2 (or earlier) ciphers available for new connections, and zero TLSv1.3 ciphersuites available 😅
As described in openssl/openssl#13704 (comment), it seems like it's quite complicated to get a list of only the TLSv1.3 ciphersuites or only the TLSv1.2 ciphers
There was a problem hiding this comment.
We have an integration build with mysql that runs against the tests that consume this behavior. We're in the process of updating to 8.3
| // Configuring the empty cipher list with |SSL_CTX_set_ciphersuites| | ||
| // also succeeds. | ||
| EXPECT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), "")); | ||
| EXPECT_EQ(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get()))); |
There was a problem hiding this comment.
Thanks, this is good.
Although this doesn't fully verify the pattern used by MySQL+MariaDB with OpenSSL, which is as follows…
- Call both:
SSL_CTX_set_cipher_list("NON-EMPTY list for TLSv1.2")- and
SSL_CTX_set_ciphersuites("") // empty list for TLSv1.3
- Expect that there will be a non-zero number of TLSv1.2 (or earlier) ciphers available for new connections, and zero TLSv1.3 ciphersuites available 😅
As described in openssl/openssl#13704 (comment), it seems like it's quite complicated to get a list of only the TLSv1.3 ciphersuites or only the TLSv1.2 ciphers
Description of changes:
MySQL deprecated the usage of weaker ciphers in their test suite: mysql/mysql-server@bc14366 This brought an additional gap to light, where OpenSSL allows the setting of empty strings in the Ciphersuite string configuration. This is known behavior since 1.1.1 and still exists today.
I've reconfigured the behavior for
SSL_[CTX]_set_cipher_listwhich is our OpenSSL compat layer, but kept the same behavior forSSL_[CTX]_set_strict_cipher_list.Call-outs:
N/A
Testing:
Test tweaking
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.