Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

smittals2 · 2025-01-02T23:34:41Z

Issues:

CryptoAlg-2559

Description of changes:

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.
Each time any cipher list is created or modified (either for TLS 1.3 or otherwise), ctx->cipher_list is updated with values from ctx->tls13_cipher_suites via a new function update_cipher_list. This function also updates in_group_flags accordingly.
Outdated comments were updated
Updates to existing tests to account for TLS 1.3 ciphersuites being configured by default

Call-outs:

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

Testing:

Added tests to ensure TLS 1.3 ciphersuites are returned as configured when calling SSL_get_ciphers.

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.

… tls 1.3

codecov-commenter · 2025-01-06T18:18:38Z

Codecov Report

Attention: Patch coverage is 92.30769% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.75%. Comparing base (6aa30a9) to head (5fcdd4a).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
ssl/ssl_cipher.cc	88.63%	5 Missing ⚠️
ssl/ssl_lib.cc	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2092   +/-   ##
=======================================
  Coverage   78.75%   78.75%           
=======================================
  Files         598      598           
  Lines      103653   103726   +73     
  Branches    14720    14735   +15     
=======================================
+ Hits        81634    81694   +60     
- Misses      21365    21379   +14     
+ Partials      654      653    -1

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

andrewhop · 2025-01-06T23:11:13Z

include/openssl/ssl.h

-// 1.3, and functions to query the cipher list do not include TLS 1.3
-// ciphers.
+// Note: TLS 1.3 ciphersuites are only configurable via
+// |SSL_[CTX]_set_ciphersuites|. Other setter functions have no impact on


In the event we publish the docs in the future please spell out the two methods so they link correctly.

andrewhop · 2025-01-07T01:24:48Z

include/openssl/ssl.h

+// TLS13_DEFAULT_CIPHER_LIST_AES_HW is the default TLS 1.3 cipher suite
+// configuration when AES hardware acceleration is enabled.
+#define TLS13_DEFAULT_CIPHER_LIST_AES_HW "TLS_AES_128_GCM_SHA256:" \
+                                          "TLS_AES_256_GCM_SHA384:" \
+                                          "TLS_CHACHA20_POLY1305_SHA256"
+
+// TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW is the default TLS 1.3 cipher suite
+// configuration when no AES hardware acceleration is enabled.
+#define TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW "TLS_CHACHA20_POLY1305_SHA256:" \
+                                            "TLS_AES_128_GCM_SHA256:" \
+                                            "TLS_AES_256_GCM_SHA384"


Do these need to be in the public header? Do we expect customer to pass these back to us? Also how did you come up with these options?

andrewhop · 2025-01-07T01:29:28Z

ssl/ssl_lib.cc

+  const bool has_aes_hw = EVP_has_aes_hardware();
+  const char *cipher_rule;
+  if (has_aes_hw) {
+    cipher_rule = TLS13_DEFAULT_CIPHER_LIST_AES_HW;
+  } else {
+    cipher_rule = TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW;
+  }
+
+  if (!SSL_CTX_set_ciphersuites(ret.get(), cipher_rule) ||
+      !SSL_CTX_set_strict_cipher_list(ret.get(), SSL_DEFAULT_CIPHER_LIST) ||


Is this changing our behavior? From the PR description:

SSL_CTX_new now configures ctx->tls13_cipher_suites with default TLS 1.3 ciphers (did not previously).

What did we do before if you attempted to connect to a TLS 1.3 enabled endpoint?

andrewhop · 2025-01-07T01:31:25Z

ssl/ssl_cipher.cc

+// This function maintains the ordering of ciphersuites and places TLS 1.3
+// ciphersuites at the front of the list.


Does the order of TLS 1.3 and the < 1.3 ciphersuites affect what gets negotiated?

andrewhop · 2025-01-07T01:33:10Z

ssl/ssl_cipher.cc

+    return 0;
+  }
+
+  // Delete any existing TLSv1.3 ciphersuites. These will be first in the list


Is this enforced by AWS-LC or could a customer pass in a list with the wrong order?

andrewhop · 2025-01-07T01:36:05Z

ssl/ssl_cipher.cc

@@ -1234,7 +1234,88 @@ static bool is_known_default_alias_keyword_filter_rule(const char *rule,
  return false;
 }

-bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
+// update_cipher_list updates |ctx->cipher_list| by:


This comment explains what this function does, but why do we need to remove everything and then create a new list?

smittals2 and others added 13 commits December 26, 2024 15:10

combine the two cipher lists

bc7b951

defining default TLS 1.3 ciphersuites

e80cb6e

configure TLS 1.3 ciphersuites by default

c49f32d

remove default TLS 13 ciphersuites macro

bfb369f

fix update function for ctx->cipher_list

ef0f455

remove default tls13_cipher_list coonfiguirating in SSL_CTX

aacb978

testing for new SSL_Get_ciphers functionality

b6f297b

configure TLS 1.3 ciphersuites by default

749513a

modified existing tests to work with TLS 1.3 configured by default

1e818d5

documentation update

425e9a5

fixed logic for configuring in_group_flags

656b710

changed TLS 1.3 ciphersuite ordering

2622e1e

Merge branch 'main' into get_ciphers

fe430f4

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 marked this pull request as ready for review January 2, 2025 23:55

smittals2 requested a review from a team as a code owner January 2, 2025 23:55

add differentiation for AES HW acceleration when ordering ciphers for…

72603cc

… tls 1.3

justsmth previously approved these changes Jan 6, 2025

View reviewed changes

use const

eb28574

smittals2 dismissed justsmth’s stale review via eb28574 January 6, 2025 17:52

remove old include

5fcdd4a

clean up comments

d4e5047

smittals2 requested a review from justsmth January 6, 2025 20:48

andrewhop reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

smittals2 commented Jan 2, 2025 •

edited

Loading

codecov-commenter commented Jan 6, 2025

andrewhop Jan 6, 2025

andrewhop Jan 7, 2025

andrewhop Jan 7, 2025

andrewhop Jan 7, 2025

andrewhop Jan 7, 2025

andrewhop Jan 7, 2025

		// This function maintains the ordering of ciphersuites and places TLS 1.3
		// ciphersuites at the front of the list.

Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

Are you sure you want to change the base?

Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

Conversation

smittals2 commented Jan 2, 2025 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Jan 6, 2025

Codecov Report

andrewhop Jan 6, 2025

Choose a reason for hiding this comment

andrewhop Jan 7, 2025

Choose a reason for hiding this comment

andrewhop Jan 7, 2025

Choose a reason for hiding this comment

andrewhop Jan 7, 2025

Choose a reason for hiding this comment

andrewhop Jan 7, 2025

Choose a reason for hiding this comment

andrewhop Jan 7, 2025

Choose a reason for hiding this comment

smittals2 commented Jan 2, 2025 •

edited

Loading