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

Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

smittals2
Copy link
Contributor

@smittals2 smittals2 commented Jan 2, 2025

Issues:

CryptoAlg-2559

Description of changes:

  1. SSL_CTX_new now configures ctx->tls13_cipher_suites with default TLS 1.3 ciphers (did not previously). The default preference order for TLS 1.3 ciphersuites differs based on whether AES Hardware acceleration is enabled in AWS-LC, we match this behavior in SSL_CTX_new.
  2. The SSL_CONFIG object holds it's own TLS 1.3 ciphersuites now
  3. Server side and client side cipher selection logic is modified to account for the new field in SSL_CONFIG
  4. Each time a cipher list is configured either on SSL_CTX or SSL_CONFIG, the main cipher_list is updated with values from its own tls13_cipher_suites via a new function update_cipher_list. This function also updates in_group_flags accordingly. This means, TLS 1.3 ciphersuites are stored seperately while the main cipher_list var holds ALL ciphersuites in both objects
  5. Outdated comments were updated

Call-outs:

Note SSL_CTX generally has a longer lifetime than an SSL object. SSL_CONFIG objects live on SSL objects. Both SSL_CTX and SSL_CONFIG have their own cipher_list and tls13_cipher_list fields. We support SSL_[CTX]_set_... APIs to set these fields.

In general, our TLS implementations pick cipher suites against the given connection constraints/parameters. Adding TLS 1.3 ciphersuites to ctx->cipher_list or config->cipher_list does not modify this behavior or introduce new problems.

Testing:

Added new testing to validate cipher selection behavior between SSL_CTX and SSL_CONFIG objects
Added change detection test for TLS 1.3 Client Hello
Updated old tests and to-do's regarding TLS 1.3
Testing to ensure SSL_get_ciphers returns ALL ciphersuites instead of TLS 1.2 and below.

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.

@smittals2 smittals2 changed the title [DRAFT] Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites Jan 2, 2025
@smittals2 smittals2 marked this pull request as ready for review January 2, 2025 23:55
@smittals2 smittals2 requested a review from a team as a code owner January 2, 2025 23:55
justsmth
justsmth previously approved these changes Jan 6, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.99%. Comparing base (695c3a0) to head (cc21d89).

Files with missing lines Patch % Lines
ssl/ssl_cipher.cc 88.09% 5 Missing ⚠️
ssl/ssl_lib.cc 93.93% 2 Missing ⚠️
ssl/ssl_test.cc 95.91% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2092      +/-   ##
==========================================
+ Coverage   78.96%   78.99%   +0.02%     
==========================================
  Files         611      611              
  Lines      105514   105603      +89     
  Branches    14941    14958      +17     
==========================================
+ Hits        83319    83420     +101     
+ Misses      21543    21531      -12     
  Partials      652      652              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@smittals2 smittals2 requested a review from justsmth January 6, 2025 20:48
include/openssl/ssl.h Outdated Show resolved Hide resolved
include/openssl/ssl.h Outdated Show resolved Hide resolved
ssl/ssl_lib.cc Show resolved Hide resolved
ssl/ssl_cipher.cc Outdated Show resolved Hide resolved
ssl/ssl_cipher.cc Show resolved Hide resolved
ssl/ssl_cipher.cc Outdated Show resolved Hide resolved
ssl/handshake_client.cc Outdated Show resolved Hide resolved
include/openssl/ssl.h Outdated Show resolved Hide resolved
ssl/ssl_cipher.cc Outdated Show resolved Hide resolved
@smittals2 smittals2 changed the title Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers Jan 23, 2025
include/openssl/ssl.h Show resolved Hide resolved
ssl/handshake_client.cc Show resolved Hide resolved
ssl/handshake_client.cc Show resolved Hide resolved
ssl/handshake_client.cc Show resolved Hide resolved
include/openssl/ssl.h Show resolved Hide resolved
ssl/ssl_cipher.cc Outdated Show resolved Hide resolved
ssl/ssl_cipher.cc Show resolved Hide resolved
ssl/ssl_cipher.cc Show resolved Hide resolved
ssl/ssl_lib.cc Show resolved Hide resolved
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.

4 participants