Skip to content

Conversation

@minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Feb 27, 2025

Description

Backport of #9989

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.

waleed-elmelegy-arm and others added 30 commits February 27, 2025 15:20
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]>
… HS deframentation tests.

Signed-off-by: Minos Galanakis <[email protected]>
Signed-off-by: Minos Galanakis <[email protected]>
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.

4834826 is a faithful backport of #9989 at d01ac30, with a spurious dependency. Otherwise LGTM.

-C "waiting for more fragments"

requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3
requires_certificate_authentication
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no dependency on ECDSA here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented on the same quesiton on the development #9989 (comment)

In short its not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

#9989 (comment) doesn't apply to 3.6, which has RSA-DHE.

So what in our test coverage makes something needed in development but not in 3.6?

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.

That's not really clear to me either. I can see that it passes but I don't really understand why it's needed in development but not 3.6.

Considering we really want to merge this soon, and the thing we don't understand is just the details of a dependency declaration (and possibly some corner case in our complex and underdocumented autodetection logic), I'm OK approving without understanding this specific point, as an exception.

But I'd really like us to get to the bottom of this at some point.

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, component-tls priority-high High priority - will be reviewed soon labels Feb 27, 2025
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

@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 b55fd70 into Mbed-TLS:features/tls-defragmentation/3.6 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component-tls needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon

Development

Successfully merging this pull request may close these issues.

4 participants