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

Support Finished-based APIs for TLS 1.3 #1952

Merged
merged 3 commits into from
Nov 8, 2024

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented Oct 26, 2024

Issues:

Addresses CryptoAlg-2699

Description of changes:

Another Ruby 3.1 test is depending on certain SSL APIs to retain information when using TLS1.3. This time it's the SSL_get(_peer)_finished APIs that are involved. OpenSSL treats the fields identically in 1.2 and 1.3 and uses the same API to update both state machines.
We already have our own individual ssl_get/send_finished for TLS 1.2 and tls13_add/process_finished for TLS 1.3. I've added the field updates within the corresponding TLS 1.3 functions so that these return the correct value within a 1.3 setting.

Call-outs:

The field buffer for previous_server_finished and
previous_client_finished has to grow past 12 since it's also used for
1.3 now. This breaks the original SSL Transfer assumption and we'll have
to bump the version while adding corresponding logic to account for the
updated size. I've regenerated the SSL Transfer bytes for the round trip
tests and also added a test for that as well. We're not bumping the
version number here since it's a pretty minuscule change. This doesn't
break compatibility with us parsing older versions of AWS-LC SSL
Transfers, but older versions of AWS-LC won't be able to parse the new
version.

Testing:

WIP

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.

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.76%. Comparing base (0cfd3ed) to head (f3b8dec).

Files with missing lines Patch % Lines
ssl/tls13_both.cc 77.77% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1952      +/-   ##
==========================================
- Coverage   78.77%   78.76%   -0.02%     
==========================================
  Files         590      590              
  Lines      101482   101506      +24     
  Branches    14396    14401       +5     
==========================================
+ Hits        79945    79950       +5     
- Misses      20902    20920      +18     
- Partials      635      636       +1     

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

@samuel40791765 samuel40791765 force-pushed the ssl-finished-tls13 branch 2 times, most recently from d96578a to f7d267e Compare October 28, 2024 19:29
@samuel40791765 samuel40791765 marked this pull request as ready for review October 28, 2024 20:57
@samuel40791765 samuel40791765 requested a review from a team as a code owner October 28, 2024 20:57
@smittals2 smittals2 self-requested a review October 29, 2024 01:26
ssl/ssl_transfer_asn1.cc Outdated Show resolved Hide resolved
ssl/ssl_transfer_asn1.cc Outdated Show resolved Hide resolved
ssl/ssl_transfer_asn1.cc Outdated Show resolved Hide resolved
smittals2
smittals2 previously approved these changes Nov 6, 2024
@samuel40791765 samuel40791765 merged commit fa1c6c0 into aws:main Nov 8, 2024
115 of 116 checks passed
@samuel40791765 samuel40791765 deleted the ssl-finished-tls13 branch November 8, 2024 19:49
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