Add 1RTT secrets into TlsSecrets for openssl#5222
Conversation
When doing the initial authoring, I neglected to add the 1RTT secrets into the TlsSecrets struct, meaning the SSLKEYLOG file wasn't populated with those values. Fix that up Fixes microsoft#5218
|
I just tested this branch and the buffers are still empty. Seems like you're not setting msquic/src/platform/tls_quictls.c Line 487 in 2623c07 |
|
Addressed |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5222 +/- ##
==========================================
- Coverage 86.74% 84.71% -2.03%
==========================================
Files 59 59
Lines 18330 18331 +1
==========================================
- Hits 15901 15530 -371
- Misses 2429 2801 +372 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ManickaP
left a comment
ManickaP
left a comment
There was a problem hiding this comment.
Thanks, re-tested, works for me.
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
Minor nitpick because of the strange comment style of this codebase, looks good to me otherwise
Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>
|
There are multiple test failures in the linux test pass that seems relevant. The Windows kernel failure can be ignored (known issue). |
|
I agree that I also think that Isn't relevant, as its using quictls, rather than openssl. https://github.com/microsoft/msquic/actions/runs/16298098148/job/46026351373?pr=5222 |
|
Eww, ok, I see the failure (I think, we're getting use after free conditions on this stack): That is a dereference of the TlsSecrets pointer in QuicTlsYieldSecret, after its been freed here: It appears the QuicTestConnectAndPing() function allocates a TlsSecrets array, starts a connection, waits for the ClientStats and ServerStats completion events, checks to make sure that the underlying TLS backend set EarlyTrafficSecrets, and then exits. I'm guessing that, because openssl yields read and write secrets independently, The completion events are triggered before the server side read secret is yielded, and because we only check after that for early traffic secrets, the test exits, allowing the UniquePtrs that hold the ClientSecrets and ServerSecrets to be deallocated, when there is still a pending Callback to QuicTlsYieldSecret comming. The TLS backend gets that callback, and dereferences the TlsSecrets pointer which is now deallocated, and...boom. I'd say the easy fix would be to convert the UniquePtrs to SharedPtrs, but I'm pretty sure the shared_ptr semantics dont work in C code. The alternate solution I think is to shutdown the connection before returning, which I think will guarantee that no further calls will be made to QuicTlsYieldSecret() |
|
fixed |
|
The unique_ptr are implementation rolled out by hand (no STL because our test code runs in kernel mode too). I'll have to look more into details in the test logic to understand how it is meant to be synchronized so we fix it. Ideally, we should avoid spining on it and have an event that let us know when we are done. |
|
An event to indicate handshake completion would be ideal, I agree, just not sure how to implement that, so I went with a spin solution for now to get through the issue. Happy to implement something better though if you can think of a way to do it. |
|
Sorry for taking a while to get back to this. Even if it isn't documented clearly, it is generally considered that the secrets will have been set by the time the connection is started (QUIC_CONNECTION_EVENT_CONNECTED). The completion event in the tests should fire when all the streams off all the connections are completed, which would necessarily be after the Is the read key set by OpenSSL after the |
|
@guhetier I'm not sure. I can say with confidence that the read key on a server is installed only when the handshake done message is sent, which will be after the write key is installed, as to weather or not that occurs after the QUIC_CONNECTION_EVENT_CONNECTED is sent, I'm unsure. |
|
@nhorman From our doc:
Does it sound like the secrets could be set later than this from what you know? I am not familiar enough to know if when this PR set the secret, we could be past that point (or I'll need to really dive in that code). If it is expected that they could be provided later, we will need to find an event (existing or not) we can use as the point when the keys have been set. |
|
@guhetier I believe the answer to the question is yes. The write secret will be installed as soon as the encrypted extensions are delivered by the client to the server, but on the server, the read secret may not be installed until the server sends the handshake done message, which sounds to me like it could be after the QUIC_CONNECTION_EVENT_CONNECTED is sent. |
|
@nhorman Got it. I'll create a task to follow up on it. For the test itself, I think that as long as we ensure the secret buffer outlive the registration, we will be safe. (apologies for the delay once again) |
|
@guhetier no apologies needed, thank you for getting back on this. To be clear, I think what you're suggesting is this type of change: Correct? Unfortunately, that doesn't seem to help. I backed out my spin change from commit ece1f10 locally and applied the above patch, but I get the same asan failure. I'll keep looking into it |
|
actually scrap that last comment, you're suggestion seems to work quite well, problem was that I forgot (as I often do) that the redhat config on my fedora system uses settings that upstream openssl can't parse, so all tests fail if I don't back them out. I'll push changes shortly |
We were failing one of the DataTests due to the Client/Server secrets getting destroyed before the underlying TLS structure was done with them. This was occuring because in QuicTestConnectAndPing, the test function returns before the openssl TLS layer had completing registering the server side application read key, which isn't needed for the reporting of an established connection. The function returned, cleaned the secrets buffer, after which the TLS layer tried to access them, and boom. Thanks to @guhetier, who noted that the destruction of the MsQuicRegistration forces shutdown of the connection, and blocks on its completion, so if we move the declaration of the UniquePtr buffers to before the allocation of said registration, the fact that C++ calls destructors in reverse order ensures that the Secrets memory remains valid until the connection is shut down.
|
updated with @guhetier suggested fix for the asan error |
4 participants
When doing the initial authoring, I neglected to add the 1RTT secrets into the TlsSecrets struct, meaning the SSLKEYLOG file wasn't populated with those values. Fix that up
Fixes #5218