Don't signal SSL_CB_HANDSHAKE_START for TLSv1.3 post-handshake messages#8096
Don't signal SSL_CB_HANDSHAKE_START for TLSv1.3 post-handshake messages#8096mattcaswell wants to merge 1 commit intoopenssl:masterfrom
Conversation
mattcaswell
added
branch: master
|
Since this is a breaking change targeted at a letter release, I think it should have an OMC vote to back it up. |
|
I'll leave this open for a bit for any comments before starting a vote. |
|
Typo |
Haha! I accidentally invented a new word. I quite like it. :-) |
|
I would consider a backfit of this as we could classify it as fixing a bug using the flexible definition of bug in this context. |
|
Is this safe to cherry-pick back to the 1.1.1a branch on its own? I'm not sure if master is intended to be 1.1.1b, or if it also has longer term or unstable changes. Currently I'm testing directly against this master PR, because I was concerned this might have some implicit deps on other previous changes, even though it cherry-picks clean. |
|
Master is 3.0.0. |
|
OK, I cherry-picked this to the 1.1.1a tag, and then onto my Node.js working branch. I think it works as code written against 1.1.0 would expect. I've run into a problem with connections not being SSL_shutdown, almost certainly a local problem, that prevents me running the Node.js test suite and confirming that this PR is working across all tests. I'm working to sort that out now. |
mattcaswell
force-pushed
the
change-info-cb
branch
from
04eccdf to
445db37
Compare
|
I fixed the typos in the commit message and in the docs. |
Code destined for 1.1.1b goes into the OpenSSL_1_1_1-stable branch. |
|
This PR is working for Node.js in my tests. @agl does it address the issues you found with nginx and haproxy? |
Yes, this would resolve the issue. For what it's worth, both NGinx & HAProxy have also been updated to not run this logic for TLS 1.3 KeyUpdate messages. See https://trac.nginx.org/nginx/changeset/e3ba4026c02d2c1810fd6f2cecf499fc39dde5ee/nginx/src/event/ngx_event_openssl.c for the NGinx change and http://git.haproxy.org/?p=haproxy.git;a=commit;h=526894ff3925d272c13e57926aa6b5d9d8ed5ee3 for the HAProxy change. There's a good chance there's other web server implementations out there though that have similar issues. |
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal start/end of a post-handshake message exchange in TLSv1.3. Unfortunately experience has shown that this confuses some applications who mistake it for a TLSv1.2 renegotiation. This means that KeyUpdate messages are not handled properly. This commit removes the use of SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal the start/end of a post-handshake message exchange. Individual post-handshake messages are still signalled in the normal way. This is a potentially breaking change if there are any applications already written that expect to see these TLSv1.3 events. However, without it, KeyUpdate is not currently usable for many applications. Fixes openssl#8069
mattcaswell
force-pushed
the
change-info-cb
branch
from
445db37 to
93a9fc8
Compare
|
I updated this based on the discussion on the openssl-project list. This version does not signal the start and end of post-handshake message exchanges at all (although the individual messages are still signalled in the normal way). |
|
Needs review now that the vote on this has passed. |
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal start/end of a post-handshake message exchange in TLSv1.3. Unfortunately experience has shown that this confuses some applications who mistake it for a TLSv1.2 renegotiation. This means that KeyUpdate messages are not handled properly. This commit removes the use of SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal the start/end of a post-handshake message exchange. Individual post-handshake messages are still signalled in the normal way. This is a potentially breaking change if there are any applications already written that expect to see these TLSv1.3 events. However, without it, KeyUpdate is not currently usable for many applications. Fixes #8069 Reviewed-by: Richard Levitte <[email protected]> (Merged from #8096) (cherry picked from commit 4af5836)
|
Pushed to master and 1.1.1. |
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal start/end of a post-handshake message exchange in TLSv1.3. Unfortunately experience has shown that this confuses some applications who mistake it for a TLSv1.2 renegotiation. This means that KeyUpdate messages are not handled properly. This commit removes the use of SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE to signal the start/end of a post-handshake message exchange. Individual post-handshake messages are still signalled in the normal way. This is a potentially breaking change if there are any applications already written that expect to see these TLSv1.3 events. However, without it, KeyUpdate is not currently usable for many applications. Fixes #8069 Reviewed-by: Richard Levitte <[email protected]> (Merged from #8096)
Cherry-pick: openssl/openssl@4af5836 Original-Pr: openssl/openssl#8096
Cherry-pick: openssl/openssl@4af5836 Original-Pr: openssl/openssl#8096
6 participants
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and
SSL_CB_HANDSHAKE_END to signal start/end of a post-handshake message
exchange in TLSv1.3. Unfortunately experience has shown that this confuses
some applications who mistake it for a TLSv1.2 reneogitation. This means
that KeyUpdate messages are not handled properly.
This introduces new events SSL_CB_POST_HANDSHAKE_START and
SSL_CB_POST_HANDSHAKE_END to signal the start/end of a post-handshake
message exchange.
This is a potentially breaking change if there are any applications already
written that expect to see these TLSv1.3 events. However, without it,
KeyUpdate is not currently usable for many applications.
Fixes #8069
Checklist