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

Improve backwards compatability for the TLS transfer parser #1337

Conversation

torben-hansen
Copy link
Contributor

@torben-hansen torben-hansen commented Dec 4, 2023

Description of changes:

Allows the parser to deserialise TLS transfer state as long as the state version is as old or older than the parser.

#1322 added a few more (optional) fields to the TLS transfer format. We want this to impose a version bump. But the parser currently doesn't allow inputting state serialised through an older parser. Improve backwards compatibility to allow future version bumps.

This makes deploying to a large fleet with a long delta of software versions being out-of-sync much easier.

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.

@torben-hansen torben-hansen requested a review from a team as a code owner December 4, 2023 21:33
@skmcgrail
Copy link
Member

skmcgrail commented Dec 4, 2023

Still need to bump SSL_SERIAL_VERSION to version 2 right? Maybe I didn't understand our discussion outcome earlier, but I thought that was the intent of the follow-up was to bump and handle the backwards compatibility with the older version?

@torben-hansen
Copy link
Contributor Author

Still need to bump SSL_SERIAL_VERSION to version 2 right? Maybe I didn't understand our discussion outcome earlier, but I thought that was the intent of the follow-up was to bump and handle the backwards compatibility with the older version?

I could bump it or just leave it as-is. There is no functional difference. I left it out in this PR with no real arguments for why.

@torben-hansen
Copy link
Contributor Author

Still need to bump SSL_SERIAL_VERSION to version 2 right? Maybe I didn't understand our discussion outcome earlier, but I thought that was the intent of the follow-up was to bump and handle the backwards compatibility with the older version?

bumped config parser version: ed630ee

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f3e49b0) 76.47% compared to head (4348543) 76.47%.

Files Patch % Lines
ssl/ssl_transfer_asn1.cc 63.63% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1337   +/-   ##
=======================================
  Coverage   76.47%   76.47%           
=======================================
  Files         421      421           
  Lines       71016    71021    +5     
=======================================
+ Hits        54308    54312    +4     
- Misses      16708    16709    +1     

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

@torben-hansen torben-hansen merged commit bdcc3ff into aws:main Dec 5, 2023
17 checks passed
skmcgrail pushed a commit to skmcgrail/aws-lc that referenced this pull request Aug 13, 2024
Allows the parser to deserialise TLS transfer state as long as the state version is as old or older than the parser.
skmcgrail added a commit that referenced this pull request Aug 14, 2024
### Issues:
Addresses P147941517

### Description of changes: 
Backports #1322 and
#1337 to the fips-2022-11-02 branch.

This changes does not modify the FIPS module boundary.

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.

---------

Co-authored-by: Will Childs-Klein <childw@amazon.com>
Co-authored-by: torben-hansen <50673096+torben-hansen@users.noreply.github.com>
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