Convert the TLSv1.3 crypto code and KTLS to use the new write record layer #19343
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
severity: fips change
We also clean up some of the KTLS code while we are doing it now that all users of KTLS have been moved to the new write record layer.
Most of this was unnecessary anyway since DTLS isn't using these codepaths.
This field was used to track whether a cipher ctx was valid for writing or not, and also whether we should write out plaintext alerts. With the new record layer design we no longer need to track whether a cipher ctx is valid since the whole record layer will be aborted if it is not. Also we have a different mechanism for tracking whether we should write out plaintext alerts. Therefore this field is removed from the SSL object.
…code We move some protocol specific code for write buffer and WPACKET allocation and initialisation out of tls_common.c and into the protocol specific files.
Remove TLSv1.3 specific processing of the record type out of tls_common.c and into tls13_meth.c
We introduce a new function to prepare the record header. KTLS has its own version since this is done by the kernel.
The KTLS cipher function is a no-op so it doesn't matter if we call it. We shouldn't special case KTLS in tls_common.c
Only tls13_meth.c needs to handle adding record padding. All other *_meth.c files can ignore it.
This applies any mac that might be necessary, ensures that we have enough space in the WPACKET to perform the encryption and sets up the SSL3_RECORD ready for that encryption.
For example in this we add the MAC if we are doing encrypt-then-mac.
The KTLS code no longer calls this function so this is not necessary.
This removes some KTLS specific code from tls_retry_write_records().
mattcaswell
force-pushed
the
tls13-write-rec
branch
from
7acee5d
to
45d330f
Compare
|
I've now taken this PR out of draft. It has been rebased to pull in all the commits from #19217 which have now been merged and all CI runs are passing (except 1 which appears to be due to a temporary network glitch and not related to this PR). Ping for review! |
mattcaswell
added
approval: review pending
hlandau
requested changes
Oct 6, 2022
|
@hlandau - fixup applied following your feedback. Please take another look. |
hlandau
approved these changes
Oct 6, 2022
hlandau
removed
the
approval: otc review pending
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
This field was used to track whether a cipher ctx was valid for writing or not, and also whether we should write out plaintext alerts. With the new record layer design we no longer need to track whether a cipher ctx is valid since the whole record layer will be aborted if it is not. Also we have a different mechanism for tracking whether we should write out plaintext alerts. Therefore this field is removed from the SSL object. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
…code We move some protocol specific code for write buffer and WPACKET allocation and initialisation out of tls_common.c and into the protocol specific files. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Remove TLSv1.3 specific processing of the record type out of tls_common.c and into tls13_meth.c Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
We introduce a new function to prepare the record header. KTLS has its own version since this is done by the kernel. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
The KTLS cipher function is a no-op so it doesn't matter if we call it. We shouldn't special case KTLS in tls_common.c Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Only tls13_meth.c needs to handle adding record padding. All other *_meth.c files can ignore it. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
This applies any mac that might be necessary, ensures that we have enough space in the WPACKET to perform the encryption and sets up the SSL3_RECORD ready for that encryption. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
For example in this we add the MAC if we are doing encrypt-then-mac. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
The KTLS code no longer calls this function so this is not necessary. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
This removes some KTLS specific code from tls_retry_write_records(). Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
openssl-machine
pushed a commit
that referenced
this pull request
Oct 12, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
We also clean up some of the KTLS code while we are doing it now that all users of KTLS have been moved to the new write record layer. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Most of this was unnecessary anyway since DTLS isn't using these codepaths. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
This field was used to track whether a cipher ctx was valid for writing or not, and also whether we should write out plaintext alerts. With the new record layer design we no longer need to track whether a cipher ctx is valid since the whole record layer will be aborted if it is not. Also we have a different mechanism for tracking whether we should write out plaintext alerts. Therefore this field is removed from the SSL object. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
…code We move some protocol specific code for write buffer and WPACKET allocation and initialisation out of tls_common.c and into the protocol specific files. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Remove TLSv1.3 specific processing of the record type out of tls_common.c and into tls13_meth.c Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
We introduce a new function to prepare the record header. KTLS has its own version since this is done by the kernel. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
The KTLS cipher function is a no-op so it doesn't matter if we call it. We shouldn't special case KTLS in tls_common.c Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Only tls13_meth.c needs to handle adding record padding. All other *_meth.c files can ignore it. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
This applies any mac that might be necessary, ensures that we have enough space in the WPACKET to perform the encryption and sets up the SSL3_RECORD ready for that encryption. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
For example in this we add the MAC if we are doing encrypt-then-mac. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
The KTLS code no longer calls this function so this is not necessary. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
This removes some KTLS specific code from tls_retry_write_records(). Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
beldmit
pushed a commit
to beldmit/openssl
that referenced
this pull request
Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19343)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is built on top of #19217 and includes those commits. This converts the TLSv1.3 code and all remaining KTLS code to use the new write record layer.