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

Make version check in s_server DTLS aware #16709

Closed
wants to merge 1 commit into from
Closed

Make version check in s_server DTLS aware #16709

wants to merge 1 commit into from

Conversation

Sean-Der
Copy link

Resolves #16707

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Sep 29, 2021
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Sep 29, 2021
@t8m t8m added approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug labels Sep 30, 2021
t8m
t8m approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

LGTM. Good for all branches. I am also OK with CLA: trivial.

paulidale
paulidale reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

LGTM. Fine for all branched and agreed trivial.
@mattcaswell any thoughts?

@paulidale paulidale 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 Sep 30, 2021
kaduk
kaduk reviewed Sep 30, 2021
View reviewed changes
apps/s_server.c Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Oct 1, 2021

@paulidale Would you please formally approve?

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m
Copy link
Member

t8m commented Oct 5, 2021

@paulidale ping

@t8m t8m mentioned this pull request Oct 15, 2021
Closed
mattcaswell
mattcaswell reviewed Oct 15, 2021
View reviewed changes
apps/s_server.c
@@ -131,7 +131,7 @@ static unsigned int psk_server_cb(SSL *ssl, const char *identity,
if (s_debug)
BIO_printf(bio_s_out, "psk_server_cb\n");

if (SSL_version(ssl) >= TLS1_3_VERSION) {
if (SSL_version(ssl) >= TLS1_3_VERSION && SSL_version(ssl) < DTLS1_2_VERSION) {
Copy link
Member

Choose a reason for hiding this comment

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

This should instead be checking SSL_is_dtls()

@Sean-Der Sean-Der closed this Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaduk kaduk kaduk left review comments

@mattcaswell mattcaswell mattcaswell left review comments

@paulidale paulidale paulidale left review comments

@t8m t8m t8m approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

psk_server_cb in s_server returns 0 for DTLS because of invalid SSL version detection.
6 participants
@Sean-Der @t8m @openssl-machine @kaduk @mattcaswell @paulidale