Repository navigation
Fix 0-RTT enforcement for resume-only servers - #6475
Gaurav Singh (gaurav2699) wants to merge 2 commits into
Conversation
8cd6e78 to
c83dd7c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6475 +/- ##
==========================================
- Coverage 88.57% 86.75% -1.82%
==========================================
Files 60 60
Lines 19139 19146 +7
==========================================
- Hits 16953 16611 -342
- Misses 2186 2535 +349 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Server unidirectional 0-RTT may shut down before CONNECTED, so exact resumption validation belongs on the client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd711df3-c521-401a-8832-a4e2cffcc1bf
Guillaume Hetier (guhetier)
left a comment
Guillaume Hetier (guhetier)
left a comment
There was a problem hiding this comment.
I need to have another look at the product logic change itself, but I think that test improvements are needed.
This test is hard to work with already, but we cannot just add more to it.
| typedef struct CXPLAT_TLS_CONFIG { | ||
|
|
||
| BOOLEAN IsServer; | ||
| BOOLEAN ServerEarlyDataEnabled; |
There was a problem hiding this comment.
nit: rename to EarlyDataEnabled, and add a comment (the comment can mention server-only)
| Status = QUIC_STATUS_TLS_ERROR; | ||
| goto Exit; | ||
| } | ||
| } else if (!(Config->SecConfig->TlsFlags & CXPLAT_TLS_CREDENTIAL_FLAG_DISABLE_RESUMPTION)) { |
There was a problem hiding this comment.
Add a comment and/or assertion to clarify that the code that follows in for client only.
Consider helper functions to split the logic in named blocks.
| bool ResumeOnly; | ||
| bool NoResume; |
There was a problem hiding this comment.
Those two boolean overlap, it allows for a confusing configuration (ResumeOnly = true, NoResume = true).
Consider using ResumeEnabled / EarlyDataEnabled instead, or a single enum to represent 3 states.
| false, // ClientRebind | ||
| true, // ClientZeroRtt | ||
| true, // ServerRejectZeroRtt | ||
| Params.NoResume ? QUIC_SERVER_NO_RESUME : |
There was a problem hiding this comment.
Instead of parametrizing this test, which is hard to understand, consider having more tests.
The pattern here (test helper with way too many parameters) is a bad one we haven't been able to move away from yet, it leads to very hard to follow tests with large amount of code that is not needed for each individual scenario.
Parameters are better used for variants of the exact same scenario (IPv4 vs IPv6, ...), not for data that affect the validation logic. Said differently, if you test validate you can send data in many configuration, params for the configuration is fine. If your test try to validate that every configuration does what it is specifically meant to do (not just that data was sent/received), then a different test is needed for the specific scenario.
Otherwise, it becomes near impossible to understand which scenario is tested.
Here, we are in the Reject0Rtt test, but sometimes, we test that actually, the server does not reject 0Rtt?
|
|
||
| PingStats ServerStats(Length, ConnectionCount, TotalStreamCount, FifoScheduling, UnidirectionalStreams, ServerInitiatedStreams, ClientZeroRtt && !ServerRejectZeroRtt, false, QUIC_STATUS_SUCCESS); | ||
| PingStats ClientStats(Length, ConnectionCount, TotalStreamCount, FifoScheduling, UnidirectionalStreams, ServerInitiatedStreams, ClientZeroRtt && !ServerRejectZeroRtt); | ||
| const bool ExpectedResumed = |
There was a problem hiding this comment.
Lets make this name clearer. The function is about everything in a connection. ExpectedResumed outside of the context of the PR is not that clear.
| ServerConfiguration, | ||
| ClientConfiguration, | ||
| &ClientStats.ResumptionTicket); | ||
| if (ServerResumptionLevel == QUIC_SERVER_RESUME_ONLY) { |
There was a problem hiding this comment.
This likely should be in QuicTestPrimeResumption.
Why does the server need different settings when priming in this scenario? This is confusing.
Description
Separate TLS session resumption from server 0-RTT permission.
QUIC_SERVER_RESUME_AND_ZERORTT.QUIC_SERVER_NO_RESUME.RESUME_ONLYand per-connectionNO_RESUMEoverrides.ICM 31000000771364
Testing
Passed Windows Debug x64 builds:
pwsh .\scripts\build.ps1 -Config Debug -Arch x64 -Tls opensslpwsh .\scripts\build.ps1 -Config Debug -Arch x64 -Tls quictlsFor both providers, the following focused cases passed:
RESUME_ONLYearly-data rejection with successful resumptionNO_RESUMErejection12 focused tests passed with no failures or skips.
Documentation
No documentation changes are required. The implementation now matches the documented server resumption-level behavior.