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

Add tests for Defragment incoming TLS handshake messages #9887

Open
mpg opened this issue Jan 9, 2025 · 2 comments · May be fixed by #9928
Open

Add tests for Defragment incoming TLS handshake messages #9887

mpg opened this issue Jan 9, 2025 · 2 comments · May be fixed by #9928
Assignees
Labels
size-s Estimated task size: small (~2d)

Comments

@mpg
Copy link
Contributor

mpg commented Jan 9, 2025

#9872 is currently in good shape except for the lack of automated tests (it was tested manually though and confirmed to resolve the issue). If the OP doesn't add automated tests, we'll add them ourselves.

Note: since we'll only have support for defragmenting incoming messages, not fragmenting outgoing, we'll need to use another implementation for testing - for example, openssl s_server -mtu <low_value> will fragment in TLS as well (see https://github.com/Mbed-TLS/mbedtls/pull/3817/files#diff-54a2261aca14ebb2491a1584cc3351a458487c23c25f90df08de2573cd705e32R9806 for example).

Note: if such tests are added 9872, then we can mark it as resolving this issue as well.

@mpg mpg added the size-s Estimated task size: small (~2d) label Jan 9, 2025
@mpg mpg moved this to 3.6.3 / 2.28.10 (final) release in Mbed TLS Epics Jan 9, 2025
@mpg mpg moved this from 3.6.3 / 2.28.10 (final) release to TLS 1.3 compatibility fix in Mbed TLS Epics Jan 9, 2025
@rojer
Copy link
Contributor

rojer commented Jan 9, 2025

some thoughts on testing: obviously this should be tested against widely used implementations such as openssl
but in my experience it's not sufficient. for example, the bug found while running older version of this code in the wild was against Amazon Greengrass, that for some reason decided to chop up messages in a weird way where record and message boundaries weren't lining up (to be clear, #9872 does not have this bug anymore).
so my idea of properly testing it involved writing a "record chopper" i/o proxy that would chop up messages into records of varying sizes, from 1 to N, or something like that.

@mpg
Copy link
Contributor Author

mpg commented Jan 13, 2025

Thanks for sharing your thoughts and experience! I think a "record chopper" would indeed be ideal for testing. However I'm afraid it is going to be more work than we can reasonably include in the upcoming 3.6.3 release.

In the short term, I think we'll probably have to be satisfied with testing against OpenSSL, but we can probably have a larger number of test cases than in #3817, with varying values of mtu - in particular, not just powers of 2 or multiples of 8 :)

@waleed-elmelegy-arm waleed-elmelegy-arm self-assigned this Jan 13, 2025
@waleed-elmelegy-arm waleed-elmelegy-arm linked a pull request Jan 24, 2025 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-s Estimated task size: small (~2d)
Projects
Status: TLS 1.3 compatibility fix
Development

Successfully merging a pull request may close this issue.

3 participants