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.

Sign up for GitHub

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

Jump to bottom

Check ALPN when receiving early data #8911

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Check ALPN when receiving early data #8911

merged 2 commits into from
Mar 15, 2024

Conversation

waleed-elmelegy-arm
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-arm commented Mar 7, 2024 •
edited
Loading

Description

Check if new session has the same ALPN as old session when receiving early data and if not reject early data.
fix #6355
Depends on #8858

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

@waleed-elmelegy-arm waleed-elmelegy-arm self-assigned this Mar 7, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm added DO-NOT-MERGE needs-ci Needs to pass CI tests labels Mar 7, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm changed the title Check alpn when receiving early data Check ALPN when receiving early data Mar 7, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the check-alpn-when-receiving-early-data branch 2 times, most recently from a500ef9 to 99c6492 Compare March 8, 2024 18:42
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, component-tls needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Mar 8, 2024
@ronald-cron-arm ronald-cron-arm added priority-very-high Highest priority - prioritise this over other review work and removed size-s Estimated task size: small (~2d) labels Mar 11, 2024
@gilles-peskine-arm
Copy link
Contributor

This has the "needs-preceding-pr" label but no mention of what it depends on. Also it has a merge conflict.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed 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 labels Mar 11, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the check-alpn-when-receiving-early-data branch from 99c6492 to 6b0ee52 Compare March 12, 2024 16:53
ronald-cron-arm
ronald-cron-arm requested changes Mar 13, 2024
View reviewed changes
library/ssl_tls.c Show resolved Hide resolved
library/ssl_tls13_server.c Outdated Show resolved Hide resolved
tests/include/test/ssl_helpers.h Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
tests/suites/test_suite_ssl.function Show resolved Hide resolved
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the check-alpn-when-receiving-early-data branch from 6b0ee52 to 22ac899 Compare March 14, 2024 01:51
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Mar 14, 2024
ronald-cron-arm
ronald-cron-arm requested changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

This looks almost good to me. The change log needs to be improved though (I've proposed something). Otherwise a few other suggestions for improvement.

library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls13_server.c Show resolved Hide resolved
tests/suites/test_suite_ssl.function Show resolved Hide resolved
ChangeLog.d/fix-alpn-negotiating-bug.txt Outdated Show resolved Hide resolved
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the check-alpn-when-receiving-early-data branch from 22ac899 to 9b51e8e Compare March 14, 2024 15:03
ronald-cron-arm
ronald-cron-arm previously approved these changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-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, thanks. We need to backport the commit "Fix bug in ALPN negotiating".

@github-merge-queue github-merge-queue bot dismissed ronald-cron-arm’s stale review March 15, 2024 10:59

The merge-base changed after approval.

tom-cosgrove-arm
tom-cosgrove-arm previously approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, one comment, not a blocker. And agreed, we need a backport of the bug fix

@waleed-elmelegy-arm waleed-elmelegy-arm dismissed tom-cosgrove-arm’s stale review March 15, 2024 12:03

The merge-base changed after approval.

@waleed-elmelegy-arm
Fix bug in ALPN negotiating
131b2ff 
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
@waleed-elmelegy-arm
Add ALPN checking when accepting early data
4dfb0e7 
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the check-alpn-when-receiving-early-data branch from 9b51e8e to 4dfb0e7 Compare March 15, 2024 12:12
ronald-cron-arm
ronald-cron-arm approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-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've done the rebase and ended up with the same tree.

@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 labels Mar 15, 2024
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Mar 15, 2024
View reviewed changes
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 - thanks

@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 15, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm mentioned this pull request Mar 15, 2024
Merged
3 tasks
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Mar 15, 2024
@ronald-cron-arm ronald-cron-arm removed the needs-ci Needs to pass CI tests label Mar 15, 2024
Merged via the queue into Mbed-TLS:development with commit a457633 Mar 15, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees

@waleed-elmelegy-arm waleed-elmelegy-arm

Labels
approved Design and code approved - may be waiting for CI or backports component-tls priority-very-high Highest priority - prioritise this over other review work
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TLS 1.3 server: Add ALPN check when accepting early data
4 participants
@waleed-elmelegy-arm @gilles-peskine-arm @ronald-cron-arm @tom-cosgrove-arm