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

test: expand s2n_record_read testing to both TLS1.3 and TLS1.2 #4903

Merged
merged 7 commits into from
Nov 21, 2024

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Nov 18, 2024

#4765

Description of changes:

A test in s2n_record_read_test.c passes for TLS1.2 but fails when TLS1.3 protocol is negotiated. This is unexpected since record reading behavior should be protocol independent.

The test "corrupts" a payload by modifying the ContentType (first byte) and expects a decryption error. However, this doesn't work for TLS1.2 since ConentType is not used in decryption in TLS1.3.

The fix in this PR is to instead corrupt the 6th byte (the encrypted data), which should result in an error for both TLS1.2 and TLS1.3.

Testing:

Expand test to both the TLS1.2 andTLS1.3 protocol.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Nov 18, 2024
@toidiu toidiu changed the title test: expand record read testing to both tls13 and tls12 test: expand s2n_record_read testing to both TLS.13 and TLS.12 Nov 18, 2024
tests/unit/s2n_record_read_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_record_read_test.c Outdated Show resolved Hide resolved
Comment on lines 209 to 211
/* Offset and corrupt the 6th byte, which is used by both TLS 1.2 and TLS 1.3
* during decryption
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the last byte be simpler / more obvious?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping the last byte does work but I prefer skipping the header len amount since that is the reason why this currently doesnt work.

// skipping last byte tested via

int last_byte = s2n_stuffer_data_available(&invalidation_stuffer) - 1;    
...
EXPECT_SUCCESS(s2n_stuffer_skip_read(&invalidation_stuffer, last_byte)); 

@toidiu toidiu disabled auto-merge November 18, 2024 22:20
@toidiu toidiu changed the title test: expand s2n_record_read testing to both TLS.13 and TLS.12 test: expand s2n_record_read testing to both TLS1.3 and TLS1.2 Nov 20, 2024
@toidiu toidiu enabled auto-merge (squash) November 21, 2024 17:08
@toidiu toidiu merged commit ff4c487 into aws:main Nov 21, 2024
38 checks passed
@toidiu toidiu deleted the ak-testRecordRead branch November 21, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants