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.

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

Introduce option allowing TLS 1.3 server to prefer psk_ke over psk_dhe_ke #22794

Closed
wants to merge 3 commits into from

Conversation

minichma
Copy link
Contributor

@minichma minichma commented Nov 21, 2023

Introduce option SSL_OP_PREFER_NO_DHE_KEX, which allows configuring a TLS1.3 server to prefer session resumption using PSK-only key exchange over PSK with DHE, if both are available. The option is ignored unless PSK-only is explicitly allowed via SSL_OP_ALLOW_NO_DHE_KEX.

  • Config option name: PreferNoDHEKEX
  • Server flag: prefer_no_dhe_kex

This PR addresses #22783.

Checklist
  • documentation is added or updated
  • tests are added or updated
ToDo
  • add version of first release to the HISTORY section of SSL_CONF_cmd.pod.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Nov 21, 2023
@minichma minichma marked this pull request as ready for review November 22, 2023 08:07
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.

Good work. Could you please add CHANGES.md entry?

ssl/statem/extensions_srvr.c Outdated Show resolved Hide resolved
ssl/statem/extensions_srvr.c Outdated Show resolved Hide resolved
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Nov 22, 2023
…non-dhe psk key exchange over psk with dhe (config file option `PreferNoDHEKEX`, server option `prefer_no_dhe_kex`).
@minichma
Copy link
Contributor Author

@t8m Thank you! Improved as suggested. As the doc build complained about docs missing in openssl-s_(server|client).pod I added the options there as well (and in perlvars.pm too). However, as the option is ignored on the client (which could change in the future though), I'm not sure it is a good idea to add it to the client docs. What do you think? How would I remove it from the client docs without the linter complaining?

Btw, the CLA is on its way.

@t8m
Copy link
Member

t8m commented Nov 22, 2023

I would keep it at the client docs. However you also need to add the description for the new command line option in SSL_CONF_cmd.pod around line 98 and mention there that it is ignored on client.

@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 labels Nov 23, 2023
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

LGTM

@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Nov 23, 2023
@minichma minichma closed this Nov 23, 2023
@minichma minichma reopened this Nov 23, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Nov 23, 2023
@minichma
Copy link
Contributor Author

@t8m I closed and reopened the PR to get rid of the 'CLA missing' tag. I've added the changelog in anticipation of version 3.3. Anything else missing from my side?

@t8m t8m 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 Nov 23, 2023
@t8m
Copy link
Member

t8m commented Nov 23, 2023

@t8m I closed and reopened the PR to get rid of the 'CLA missing' tag. I've added the changelog in anticipation of version 3.3. Anything else missing from my side?

Nope, this is good as is.

@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.

@mattcaswell mattcaswell 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 Nov 24, 2023
@mattcaswell
Copy link
Member

Pushed. Thanks!

openssl-machine pushed a commit that referenced this pull request Nov 24, 2023
…non-dhe psk key exchange over psk with dhe (config file option `PreferNoDHEKEX`, server option `prefer_no_dhe_kex`).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22794)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22794)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2023
…L version 3.3.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22794)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 7, 2023
…non-dhe psk key exchange over psk with dhe (config file option `PreferNoDHEKEX`, server option `prefer_no_dhe_kex`).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22794)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 7, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22794)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 7, 2023
…L version 3.3.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22794)

Signed-off-by: fly2x <fly2x@hitls.org>
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
…non-dhe psk key exchange over psk with dhe (config file option `PreferNoDHEKEX`, server option `prefer_no_dhe_kex`).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#22794)
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#22794)
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
…L version 3.3.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#22794)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants