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

Continues processing cookieless client hellos for dtls1.3 #22400

Closed
wants to merge 4 commits into from
Closed

Continues processing cookieless client hellos for dtls1.3 #22400

wants to merge 4 commits into from

Conversation

fwh-dc
Copy link
Contributor

@fwh-dc fwh-dc commented Oct 16, 2023

No description provided.

@fwh-dc fwh-dc mentioned this pull request Oct 17, 2023
Closed
@t8m t8m added this to the DTLS-1.3 milestone Oct 18, 2023
@fwh-dc fwh-dc changed the base branch from master to feature/dtls-1.3 March 21, 2024 12:53
@fwh-dc fwh-dc closed this Mar 21, 2024
@fwh-dc fwh-dc reopened this Mar 21, 2024
@fwh-dc
Copy link
Contributor Author

fwh-dc commented Mar 22, 2024

CI failure looks irrelevant

@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: feature The issue/pr requests/adds a feature tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) labels Mar 27, 2024
t8m
t8m reviewed Mar 27, 2024
View reviewed changes
ssl/statem/statem_srvr.c
@@ -1618,17 +1618,6 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL_CONNECTION *s, PACKET *pkt)
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
goto err;
}
/*
Copy link
Member

Choose a reason for hiding this comment

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

Should this be conditional on DTLS-1.3 being enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "being enabled"?

I cannot check for connection versions at this point because the server has not yet called ssl_choose_server_version().

But maybe I can move the check to a place after the extensions has been read and then check for a cookie then?

Copy link
Member

Choose a reason for hiding this comment

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

I meant DTLS-1.3 not being enabled on the SSL_CONNECTION object (i.e. what ssl_get_min_max_version() returns).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check my latest update and let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

CI seems relevant.

@fwh-dc fwh-dc force-pushed the continue-processing-cookieless-client-hellos branch from b3eebc0 to 41bd984 Compare April 3, 2024 13:29
@nhorman nhorman mentioned this pull request Apr 3, 2024
Open
@openssl-machine openssl-machine force-pushed the feature/dtls-1.3 branch 2 times, most recently from fbea037 to 553fcfb Compare April 23, 2024 11:08
@mattcaswell
Copy link
Member

Needs rebase

@fwh-dc fwh-dc marked this pull request as draft May 13, 2024 17:44
@fwh-dc fwh-dc force-pushed the continue-processing-cookieless-client-hellos branch from 41bd984 to 6211f32 Compare May 14, 2024 12:41
@fwh-dc fwh-dc force-pushed the continue-processing-cookieless-client-hellos branch from 6211f32 to f990397 Compare May 14, 2024 13:28
@fwh-dc fwh-dc marked this pull request as ready for review May 15, 2024 09:05
@fwh-dc
Copy link
Contributor Author

fwh-dc commented May 15, 2024

@mattcaswell @t8m

Ready for review. I've implemented your suggestion @t8m.

t8m
t8m reviewed May 15, 2024
View reviewed changes
ssl/statem/statem_srvr.c Outdated Show resolved Hide resolved
@fwh-dc fwh-dc requested a review from t8m May 15, 2024 11:16
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label May 15, 2024
@t8m t8m requested a review from mattcaswell May 15, 2024 13:05
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 15, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 16, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented May 16, 2024

Merged to the feature branch. Thank you for your contribution.

@t8m t8m closed this May 16, 2024
openssl-machine pushed a commit that referenced this pull request May 16, 2024
@fwh-dc @t8m
Continue processing cookieless client hellos for dtls1.3
bf5269e 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22400)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
DTLS-1.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fwh-dc @mattcaswell @openssl-machine @t8m