Refactor: Disentangle Client_Hello implementations#5293
Refactor: Disentangle Client_Hello implementations#5293reneme merged 2 commits intorandombit:masterfrom
Conversation
7c95243 to
7c83ec6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7c83ec6 to
3d9a3d2
Compare
|
@copilot Please review again. Put an emphasis on finding semantic differences in the refactoring. Most code was merely moved into different modules but didn't actually change at all. The PR description points out one exception regarding a 15 years old workaround in the handling of the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d9a3d2 to
a06de2f
Compare
|
@randombit if you don't mind looking into it: this PR and the next one in the stack (#5303) are essentially just moving stuff around like the ones that came before and are both ready to go. Then, the actual feature (TLS 1.3 w/o 1.2 in #5318) would be free-standing and ready for review. |
|
Too bad copilot was not willing to cooperate on this. But looks good to me. |
This separates implementations of Client_Hello_12 and Client_Hello_13 into their respective modules tls12 and tls13. Since the TLS 1.3 code must have a rudimentary understanding of the legacy Client Hello a new Client_Hello_12_Shim is introduced. This facilitates protocol downgrades from 1.3 to 1.2 when needed.
a06de2f to
9e05d7f
Compare
|
Rebased once more because f9b4566 |
See here for an overview of the related pull requests to disentangle TLS 1.2 and 1.3.
This moves the implementation of
Client_Hello_12into the tls12 module andClient_Hello_13to tls13 likewise. To support downgrading the protocol version, the TLS 1.3 implementation needs to have a rudimentary understanding of the TLS 1.2 Client Hello. This is now implemented by theClient_Hello_12_Shimbase class.The only semantically relevant change is pulled into an independent commit. There is a 15 years old workaround (?) in the parsing of TLS 1.2 Client Hello messages that introduces a "fake extension" at parsing time when the "TLS_EMPTY_RENEGOTIATION_INFO_SCSV" ciphersuite is offered but no
Renegotiation_Extensionwas found in the original Client Hello. This code moved out of theClient_Hello_12_Shimand into the full-blownClient_Hello_12class. Hence, the TLS 1.3 messages parsing tests now won't see this "fake extension" anymore, because they only use theClient_Hello_12_Shimto parse. The "workaround' isn't visible in TLS 1.3 any longer.@copilot Most of this pull request is just moving code around without changing the implementations of the individual methods. Sometimes, small adjustments had to be done when certain aspects needed a different scope. Please create a detailed report on implementations and logic that didn't just move to make it easier for a human reviewer to follow what actually changes in this PR. Also: this PR contains two comments. The first is from #5292, please don't review this one one here.
Pull Request Dependencies