fix(sspi): TLS 1.3 support in TSSSP module #536
Conversation
Fix TLS packet header validation: TLS 1.3 uses the TLS 1.2 version number in the packet header.
| let tls_version: u16 = if tls_version == ProtocolVersion::TLSv1_3 { | ||
| // TLS 1.3 uses the same version number as TLS 1.2 in the record layer. | ||
| ProtocolVersion::TLSv1_2 | ||
| } else { | ||
| tls_version | ||
| } | ||
| .into(); |
There was a problem hiding this comment.
style (non-blocking): avoid this kind of into(). I recommend u16::from in a readable way.
There was a problem hiding this comment.
Pull Request Overview
This PR adds TLS 1.3 support to the CredSSP TLS connection handling by properly handling TLS 1.3 version numbers in record layer headers and adding support for the TLS 1.3 AES-256-GCM cipher suite.
Key changes:
- TLS 1.3 record layer version handling (maps TLS 1.3 to TLS 1.2 version number per RFC 8446)
- Enhanced error messages with actual vs expected packet header debugging information
- Added TLS13_AES_256_GCM_SHA384 cipher suite support
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/credssp/sspi_cred_ssp/tls_connection.rs | Adds TLS 1.3 version handling logic that maps TLS 1.3 to TLS 1.2 version in record headers, and improves error messages with debugging information |
| src/credssp/sspi_cred_ssp/cipher_block_size.rs | Adds TLS13_AES_256_GCM_SHA384 cipher suite to the supported block cipher list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &payload[0..3], | ||
| tls_packet_start |
There was a problem hiding this comment.
The order of arguments in the error message is reversed compared to the other similar error message in the same file. At line 207-209, the format shows 'expected tls_packet_start but got payload[0..3]', but here at line 266-267 it's reversed. This inconsistency could confuse debugging. Both error messages should have the same order: 'expected {expected_value} but got {actual_value}'.
| &payload[0..3], | |
| tls_packet_start | |
| tls_packet_start, | |
| &payload[0..3] |
|
question: Is this a new feature? Should probably be |
I wouldn't call it that. I expected TLS 1.3 to work correctly before 😅 There was no reason why it wouldn't work. I only fixed it. But if you still wish to change the PR title, then I don't mind changing it :) |
CBenoit
changed the title
|
Makes sense to stick to |
CBenoit
changed the title
Hi,
I fixed TLS 1.3 support in TSSSP implementation:
CipherSuite::TLS13_AES_256_GCM_SHA384support.