Skip to content

Conversation

@ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Feb 23, 2024

Description

Fix #8611
On top of #8760, only the last 4 commits belong to this PR.

PR checklist

  • changelog not required, will add one for TLS 1.3 early data feature as a whole
  • backport not required, new feature
  • tests provided

@ronald-cron-arm ronald-cron-arm added enhancement needs-ci Needs to pass CI tests component-tls13 priority-high High priority - will be reviewed soon needs-preceding-pr Requires another PR to be merged first needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Feb 23, 2024
@mpg mpg self-requested a review February 26, 2024 07:12
mpg
mpg previously requested changes Feb 26, 2024
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.

Mostly minor points, the only important one is about handling errors from ssl_write_real().

@mpg mpg requested review from waleed-elmelegy-arm and removed request for waleed-elmelegy-arm February 27, 2024 16:43
@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Feb 27, 2024
@ronald-cron-arm ronald-cron-arm force-pushed the tls13-cli-max-early-data-size branch from 2cefacf to 3328d8a Compare March 1, 2024 15:46
@ronald-cron-arm
Copy link
Contributor Author

Following the merge of #8760 (dependency of this PR), I've rebased this PR on top of development and then addressed Manuel's comments (four last commits). The previous head was 2cefacf.

@ronald-cron-arm ronald-cron-arm added needs-ci Needs to pass CI tests and removed needs-preceding-pr Requires another PR to be merged first needs-review Every commit must be reviewed by at least two team members, labels Mar 1, 2024
Allocate the buffer to write/read early data. That
way in ASan builds. buffer overwrite/overread can
be detected.

Signed-off-by: Ronald Cron <[email protected]>
@ronald-cron-arm ronald-cron-arm force-pushed the tls13-cli-max-early-data-size branch from 3328d8a to 7c07aab Compare March 1, 2024 18:23
@ronald-cron-arm ronald-cron-arm removed the needs-ci Needs to pass CI tests label Mar 4, 2024
@ronald-cron-arm ronald-cron-arm added the needs-review Every commit must be reviewed by at least two team members, label Mar 4, 2024
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

I have reviewed the rebase (0ee2e4aae-2cefacfdd became 5c4fc9156-aad852376) and the commits de9b03dcb - 7c07aab72 and I am happy with them. Together with @mpg's review this constitutes one approval for this PR.

Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-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

@tom-cosgrove-arm tom-cosgrove-arm dismissed mpg’s stale review March 8, 2024 19:18

Manuel's requested changes have been made

@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Mar 9, 2024
@ronald-cron-arm ronald-cron-arm removed this pull request from the merge queue due to a manual request Mar 9, 2024
@ronald-cron-arm ronald-cron-arm added the needs-ci Needs to pass CI tests label Mar 10, 2024
@ronald-cron-arm ronald-cron-arm removed the needs-ci Needs to pass CI tests label Mar 11, 2024
@ronald-cron-arm
Copy link
Contributor Author

@tom-cosgrove-arm @waleed-elmelegy-arm following the merge of #8854, this PR had conflicts with development head: test_suite_ssl test cases and test function added at the end of respectively test_suite_ssl.data and test_suite_ssl.function in both PRs. For once I opted for the merge of the development head instead of the rebase to resolve the conflicts. I've done both and resolving the conflicts were easier with the merge.

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@ronald-cron-arm ronald-cron-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Mar 11, 2024
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-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

@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Mar 12, 2024
@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 12, 2024
Merged via the queue into Mbed-TLS:development with commit ec4ed8e Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports component-tls13 enhancement priority-very-high Highest priority - prioritise this over other review work

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

TLS1.3 EarlyData: client: Check max_early_data_size

4 participants