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

Don't attempt to set provider params on an ENGINE based cipher #22864

Closed
wants to merge 3 commits into from
Closed

Don't attempt to set provider params on an ENGINE based cipher #22864

wants to merge 3 commits into from
+56 −59

Conversation

mattcaswell
Copy link
Member

If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

We also add a test for this. While working on this issue I spotted some dead code resulting from the record layer refactor that is also removed.

@mattcaswell
Add a test for late loading of an ENGINE in TLS
a9c97da 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).
@mattcaswell
Don't attempt to set provider params on an ENGINE based cipher
1cfa998 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.
@mattcaswell
Remove some redundant code
4647a48 
We remove a function that was left behind and is no longer called after the
record layer refactor
@mattcaswell mattcaswell added branch: master Merge to master branch 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: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present branch: 3.2 Merge to openssl-3.2 labels Nov 29, 2023
@mattcaswell mattcaswell self-assigned this Nov 29, 2023
@mattcaswell mattcaswell mentioned this pull request Nov 29, 2023
Closed
@mattcaswell
Copy link
Member Author

This is the master/3.2 version. Backport to 3.1/3.0 in #22865

@mattcaswell mattcaswell requested a review from t8m November 29, 2023 12:43
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 29, 2023
@t8m t8m requested review from levitte and hlandau November 29, 2023 12:58
tmshort
tmshort reviewed Nov 29, 2023
View reviewed changes
ssl/ssl_local.h
Comment on lines -2930 to -2933
int tls_provider_set_tls_params(SSL_CONNECTION *s, EVP_CIPHER_CTX *ctx,
const EVP_CIPHER *ciph,
const EVP_MD *md);

Copy link
Contributor

@tmshort tmshort Nov 29, 2023 •
edited

Choose a reason for hiding this comment

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

Redundant with ossl_set_tls_provider_parameter()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The code was moved into the record layer as part of the record layer refactor....but the original got left behind.

tmshort
tmshort approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

Just one question, which doesn't block approval.

@tmshort tmshort 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 29, 2023
@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 Nov 30, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Add a test for late loading of an ENGINE in TLS
7765d25 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Don't attempt to set provider params on an ENGINE based cipher
afcc12c 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Remove some redundant code
e46a6b1 
We remove a function that was left behind and is no longer called after the
record layer refactor

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Add a test for late loading of an ENGINE in TLS
8b96444 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)

(cherry picked from commit 7765d25)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Don't attempt to set provider params on an ENGINE based cipher
11d7870 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)

(cherry picked from commit afcc12c)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
@mattcaswell
Remove some redundant code
fbf7bd0 
We remove a function that was left behind and is no longer called after the
record layer refactor

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22864)

(cherry picked from commit e46a6b1)
@mattcaswell
Copy link
Member Author

Pushed to master/3.2. Thanks.

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Add a test for late loading of an ENGINE in TLS
c1ea73f 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Don't attempt to set provider params on an ENGINE based cipher
4bb57ff 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Remove some redundant code
77608ac 
We remove a function that was left behind and is no longer called after the
record layer refactor

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Add a test for late loading of an ENGINE in TLS
7c74f7a 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

(cherry picked from commit 7765d25ffe4f2a60b2082d469dec3b40f3418024)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Don't attempt to set provider params on an ENGINE based cipher
4b6b35b 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

(cherry picked from commit afcc12c41ad82c5b63194502592de015604dbd47)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@mattcaswell
Remove some redundant code
24b6a64 
We remove a function that was left behind and is no longer called after the
record layer refactor

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22864)

(cherry picked from commit e46a6b1a5de0759023c5c9c2143ead4621f20d20)
Signed-off-by: fly2x <fly2x@hitls.org>
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
@mattcaswell @wbeck10p
Add a test for late loading of an ENGINE in TLS
729aa97 
Confirm that using an ENGINE works as expected with TLS even if it is
loaded late (after construction of the SSL_CTX).

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22864)
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
@mattcaswell @wbeck10p
Don't attempt to set provider params on an ENGINE based cipher
417bcc1 
If an ENGINE has been loaded after the SSL_CTX has been created then
the cipher we have cached might be provider based, but the cipher we
actually end up using might not be. Don't try to set provider params on
a cipher that is actually ENGINE based.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22864)
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
@mattcaswell @wbeck10p
Remove some redundant code
ebecb3b 
We remove a function that was left behind and is no longer called after the
record layer refactor

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22864)
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

@tmshort tmshort tmshort approved these changes

@levitte levitte Awaiting requested review from levitte

@hlandau hlandau Awaiting requested review from hlandau

Assignees

@mattcaswell mattcaswell

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 tests: present The PR has suitable tests present 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.

None yet
4 participants
@mattcaswell @openssl-machine @t8m @tmshort