Skip to content

Add basic handshake defragmentation tests in ssl-opt #9989

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

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Feb 17, 2025

Description

Implements a portion of the testing needed for #9872. This pr extends #9928 and resolves #9887

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

@minosgalanakis minosgalanakis changed the base branch from development to features/tls-defragmentation/development February 17, 2025 14:59
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

(Posting partial early review.)

@minosgalanakis minosgalanakis force-pushed the issue9887_add_basic_defragmentation_tests branch from 6e261a1 to aeba05a Compare February 18, 2025 20:59
@minosgalanakis minosgalanakis force-pushed the issue9887_add_basic_defragmentation_tests branch 2 times, most recently from 5bf1ff3 to 5c8cbb2 Compare February 20, 2025 09:44
@minosgalanakis minosgalanakis force-pushed the issue9887_add_basic_defragmentation_tests branch 5 times, most recently from aee03e9 to 765755a Compare February 23, 2025 23:43
@minosgalanakis
Copy link
Contributor Author

Quick update:

I have addressed all the comments, cleaned up the commit history to make it easier to review, and increased coverage.

I will be sharing a table with what is covered for design review purposes.

@mpg
Copy link
Contributor

mpg commented Feb 24, 2025

Thanks! Now that #9872 is merged, can you rebase on top of it?

Also, can you add a commit disabling (by prepending skip_next_test) the failings tests (TLS 1.2 with < 16 fragments) temporarily, just to make it easier to check that those are the only failures (ie, the CI should come back green with those disabled).

Once we've agreed on a solution to the underlying problem, we'll re-enable those tests in some form (probably just assert failure when using 1.2 and with fragment length < 16, but we can't do that right now, as the handshake succeeds with ChachaPoly).

waleed-elmelegy-arm and others added 13 commits February 24, 2025 09:16
Tests uses openssl s_server with a mix of max_send_frag
and split_send_frag options.

Signed-off-by: Waleed Elmelegy <[email protected]>
* Add tests for the server side.
* Remove restriction for TLS 1.2 so that we can test TLS 1.2 & 1.3.
* Use latest version of openSSL to make sure -max_send_frag &
  -split_send_frag flags are supported.

Signed-off-by: Waleed Elmelegy <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added needs-work needs-backports Backports are missing or are pending review and approval. component-tls size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Feb 27, 2025
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

There's still one spurious dependency on requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3: #9989 (comment) . Other than that LGTM

tests/ssl-opt.sh Outdated

requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2
requires_certificate_authentication
requires_config_enabled MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

I had missed this spurious dependency, good call for removing it from the backport: the negative test for handshake too small doesn't require ECDSA support. In fact it doesn't need certificate authentication at all. We can fix this later.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 27, 2025
@minosgalanakis
Copy link
Contributor Author

One test failedin all_u16-test_tls13_only_psk, because I removed the certifcate requirement for the negative testing. Re-introduced that and should be green.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -14220,12 +14217,12 @@ run_test "Handshake defragmentation on server: len=128, TLS 1.3" \
-s "handshake fragment: 0 \\.\\. 128 of [0-9]\\+ msglen 128" \
-s "waiting for more fragments (128"

# Server-side ClientHello degfragmentation is only supported for MBEDTLS_SSL_PROTO_TLS1_3. For TLS 1.2 testing
# Server-side ClientHello defragmentationis only supported for MBEDTLS_SSL_PROTO_TLS1_3. For TLS 1.2 testing
Copy link
Contributor

@mpg mpg Feb 28, 2025

Choose a reason for hiding this comment

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

Introducing a typo: missing space defragmentationis (no need to fix if there's nothing else).

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

We'll need to have some tests around fragmentation that run with PSK, but ok for now.

@github-project-automation github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Feb 28, 2025
@gilles-peskine-arm gilles-peskine-arm merged commit 6eabe58 into Mbed-TLS:features/tls-defragmentation/development Feb 28, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Feb 28, 2025
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Mar 5, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Development

Successfully merging this pull request may close these issues.

Basic ssl-opt testing for TLS HS defragmentation
5 participants