-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rework locking in SslStream to support TLS1.3 #32925
Conversation
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
I made an update to change ValuerTask to Task, add missing ConfigureAwait(), named functions as suggested and fix few other issues. I will run another round of ssl-stress and tests in the loop but this should be ready for another round of review in the meantime. |
/azp run runtime-libraries outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries stress-ssl |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
await WriteSingleChunk(wAdapter, buff).ConfigureAwait(false); | ||
try | ||
{ | ||
await t.ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this logic appears to be duplicated with the caller. If that's true, since we expect this to be rare, could we just await a recursive call back to the caller? If there's any concern about stack diving from the recursion, we could queue the call. It'll end up being more expensive when it occurs, but it would seem to simplify things (feel free to tell me I'm wrong :) and this is supposed to be very rare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the logic as you suggested @stephentoub. While working on this I realized we don't have a good way how to test this. Getting one renegotiation is easy (either with TLS1.3 or with external server) but to do that again is not easy until we have some way how to trigger it in loop with concurrent writes,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we had an API in SslStream (server-side) to trigger a renegotiation. :) Perhaps it is time to add one.
And while we wait for an API approval, we could perhaps make this API 'internal' and use reflection in our tests to invoke it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we had an API in SslStream (server-side) to trigger a renegotiation. :) Perhaps it is time to add one. And while we wait for an API approval, we could perhaps make this API 'internal' and use reflection in our tests to invoke it.
I agree. However, it seems likely introducing that is going to require some re-work in the locking scheme, since the current scheme in this PR expects that renegotation can only be triggered by reads and relies on that for correctness. I think we should get this change in, in order to fix the known existing problems and unblock other work, and then introducing a new method (even if internal at first to help with testing) sounds like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is on my TODO list. I think I know how to do that at least for schannel and OpenSSL. Also note, that for TLS13 we go through this path even if renegotiation does not exist in that version. When I talked to experts, it seems like that particular status code is used anytime when something needs to be sent to lsass
for further processing.
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries outerloop |
/azp run runtime-libraries stress-ssl |
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
It seems like the execution of stress-ssl is broken again as it fails to build. |
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
/azp run runtime-libraries outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks, @wfurt! |
The locking code in SslStream was broken for quite a while without us noticing. We bump to it with OpenSSL 1.1.1 and TLS1.3 but the fix was to add locking to PAL: dotnet/corefx#37736. I run to the same problems when working on #1720. When MessageDecrypt decides to start renegotiation (there is no real renegotiation in TLS1.3 but on Windows, we still do get SEC_I_RENEGOTIATE as we need to pass data to
lsass
process.I started to fix it but after chatting with @stephentoub we decided to go with a simplistic approach: We grab synchronous lock around DecryptData/EncryptData and if renegotiation is happening we would use create TaskCompletionSource encrypt can wait on for completion.
With this, the code is pretty simple and old craft can be removed so as the change to OpenSSL PAL.
Performance seems to be about the same. On Windows, I get results bellow and it changes slightly between runs. On Linux, there was already lock and performance remains the same as well.
I did run SslStress for a while and I did not see any issues.
fixes #32920
contributes to #1720