refactor(harness): Extend handshake logic to support TLS 1.2#5614
refactor(harness): Extend handshake logic to support TLS 1.2#5614kaukabrizvi merged 10 commits intoaws:mainfrom
Conversation
| let mut progress = true; | ||
|
|
||
| // Keep looping while handshake not complete or progress is still being made | ||
| while !self.handshake_completed() || progress { |
There was a problem hiding this comment.
Why is the progress field necessary? In my mind, it feels like just !self.handshake_completed() should be sufficient?
There was a problem hiding this comment.
I thought the same initially, but on TLS 1.2 the round trip in dynamic record sizing test hit:
S2N_ERR_BAD_MESSAGE: Bad message encountered (/tls/s2n_post_handshake.c:104)
That happened because one side still had handshake bytes in flight even after both reported completion. The progress check drains those remaining bytes before starting application data - after adding it, the round trip worked as intended. The alternative would be a separate helper for tests, but I preferred centralizing it here. Let me know what you think.
There was a problem hiding this comment.
Quick update - I’ve since removed the progress-tracking logic. I updated handshake_completed() to rely on the result of poll_negotiate() rather than the handshake type string, so now the loop exits correctly without needing to track progress. TLS 1.2 no longer leaves stray handshake bytes, and the dynamic record sizing test works as expected.
|
Is there going to be a followup PR that adds the actual TLS1.2 test after this PR is merged, if so should we just include that change in this PR? How do I know that the TLS 1.2 handshake tests are going to work? |
@jouho The current dynamic record sizing test already exercises both TLS 1.2 and TLS 1.3 by performing a handshake agnostic of TLS 1.2 vs. TLS 1.3. As a result, the test currently fails for OpenSSL 1.0.2 (which negotiates TLS 1.2); you can see the failure here. With the changes in this PR, the test passes across all libcryptos - including OpenSSL 1.0.2 - indicating that the TLS 1.2 and TLS 1.3 handshake succeeds. |
jmayclin
left a comment
There was a problem hiding this comment.
Approved after comment added.
Co-authored-by: James Mayclin <maycj@amazon.com>
Goal
Add TLS 1.2 support to the in-memory handshake driver by replacing the fixed two round trips logic with a single, version-agnostic loop that drives both peers until completion and exits only when no further progress is observed.
Why
The previous approach assumed TLS 1.3 timing (2 RTT) and could fail with TLS 1.2/HRR patterns. We would prefer one robust path that works for both TLS 1.2 and TLS 1.3 without special-casing. Currently, the dynamic record sizing test only supports TLS 1.3, causing it to fail on OpenSSL 1.0.2, which uses TLS 1.2.
How
Replace fixed 2-RTT assumption with while !handshake_completed() driving client/server.
Callouts
A top-level while !handshake_completed() alone isn’t sufficient: one peer can still have post-handshake bytes in flight (TLS 1.2) when sending application data, leading to S2N_ERR_BAD_MESSAGE.
Waiting for empty transcripts can hang in TLS 1.3 because servers may queue post-handshake messages (e.g., NewSessionTicket) that are consumed during app reads, not another handshake tick.
Using a progress-based exit avoids both issues: we don’t require emptiness (no hang), and we don’t stop early while bytes are still moving.
Testing
I ran the integration tests in a nix-based OpenSSL 1.0.2 shell (negotiating TLS 1.2) and OpenSSL 3.0 shell (negotiating TLS 1.3). Both tests pass.
Related
This PR is a follow-up to #5608, which should test dynamic record sizing across protocol versions but currently only supports TLS 1.3 handshakes. See comment here.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.