rework locking in SslStream to support TLS1.3 #32925

wfurt · 2020-02-27T19:23:07Z

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.

Method	Toolchain	Mean	Error	StdDev	Median	Min	Max	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
DefaultHandshakeIPv4Async	master\CoreRun.exe	780.6 us	18.41 us	21.20 us	780.2 us	750.4 us	821.4 us	1.00	0.03	3.1250	-	-	13.62 KB
DefaultHandshakeIPv4Async	newLocking\CoreRun.exe	781.1 us	17.16 us	19.76 us	777.3 us	743.8 us	811.4 us	1.00	0.00	-	-	-	13.68 KB

DefaultHandshakeIPv6Async	master\CoreRun.exe	767.2 us	14.24 us	13.98 us	770.5 us	726.9 us	783.2 us	0.99	0.04	2.8409	-	-	13.61 KB
DefaultHandshakeIPv6Async	newLocking\CoreRun.exe	769.2 us	20.27 us	23.34 us	773.5 us	733.6 us	800.2 us	1.00	0.00	2.9762	-	-	13.68 KB

DefaultHandshakePipeAsync	master\CoreRun.exe	763.1 us	18.29 us	21.06 us	760.5 us	731.8 us	804.2 us	1.02	0.04	2.9762	-	-	15.8 KB
DefaultHandshakePipeAsync	newLocking\CoreRun.exe	749.4 us	16.23 us	18.69 us	742.2 us	724.0 us	783.1 us	1.00	0.00	3.1250	-	-	15.86 KB

TLS12HandshakeECDSA256CertAsync	master\CoreRun.exe	4,279.7 us	53.66 us	50.19 us	4,267.0 us	4,210.0 us	4,398.6 us	1.00	0.02	-	-	-	18.8 KB
TLS12HandshakeECDSA256CertAsync	newLocking\CoreRun.exe	4,289.8 us	85.58 us	80.06 us	4,266.8 us	4,187.0 us	4,488.5 us	1.00	0.00	-	-	-	18.79 KB

TLS12HandshakeRSA1024CertAsync	master\CoreRun.exe	3,713.6 us	33.65 us	29.83 us	3,714.3 us	3,635.7 us	3,747.2 us	1.00	0.02	-	-	-	18.98 KB
TLS12HandshakeRSA1024CertAsync	newLocking\CoreRun.exe	3,722.3 us	68.32 us	60.56 us	3,723.6 us	3,621.7 us	3,827.1 us	1.00	0.00	-	-	-	19.11 KB

TLS12HandshakeRSA2048CertAsync	master\CoreRun.exe	4,624.0 us	45.38 us	42.45 us	4,636.7 us	4,524.7 us	4,697.6 us	1.00	0.01	-	-	-	19.37 KB
TLS12HandshakeRSA2048CertAsync	newLocking\CoreRun.exe	4,604.8 us	47.08 us	44.03 us	4,602.3 us	4,542.0 us	4,691.7 us	1.00	0.00	-	-	-	19.42 KB

TLS12HandshakeRSA4096CertAsync	master\CoreRun.exe	10,789.4 us	187.44 us	175.33 us	10,735.4 us	10,540.6 us	11,047.1 us	0.97	0.02	-	-	-	20.16 KB
TLS12HandshakeRSA4096CertAsync	newLocking\CoreRun.exe	11,072.6 us	186.93 us	165.71 us	11,052.7 us	10,839.1 us	11,502.9 us	1.00	0.00	-	-	-	20.17 KB

I did run SslStress for a while and I did not see any issues.

fixes #32920
contributes to #1720

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

wfurt · 2020-02-28T20:13:00Z

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.

wfurt · 2020-02-28T22:48:13Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-02-28T22:48:25Z

Azure Pipelines successfully started running 1 pipeline(s).

wfurt · 2020-02-28T22:48:48Z

/azp run runtime-libraries stress-ssl

azure-pipelines · 2020-02-28T22:48:57Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

stephentoub · 2020-02-29T13:04:46Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

-                await WriteSingleChunk(wAdapter, buff).ConfigureAwait(false);
+                try
+                {
+                    await t.ConfigureAwait(false);


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.

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,

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 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.

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

wfurt · 2020-03-10T19:53:49Z

/azp run runtime-libraries outerloop

wfurt · 2020-03-10T19:54:01Z

/azp run runtime-libraries stress-ssl

azure-pipelines · 2020-03-10T19:54:02Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2020-03-10T19:54:11Z

Azure Pipelines successfully started running 1 pipeline(s).

wfurt · 2020-03-12T18:31:31Z

It seems like the execution of stress-ssl is broken again as it fails to build.
cc: @ViktorHofer

ViktorHofer · 2020-03-12T19:01:20Z

cc @Anipik @ericstj

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

wfurt · 2020-03-20T05:43:49Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-03-20T05:44:02Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub · 2020-03-20T13:31:42Z

Thanks, @wfurt!

wfurt added 3 commits February 26, 2020 17:27

initial locking

b5d293a

Merge branch 'master' of https://github.com/dotnet/runtime into sslLock2

785cd8c

Merge branch 'master' of https://github.com/dotnet/runtime into sslLock2

4240071

wfurt added the area-System.Net.Security label Feb 27, 2020

wfurt added this to the 5.0 milestone Feb 27, 2020

wfurt requested review from stephentoub and a team February 27, 2020 19:23

wfurt self-assigned this Feb 27, 2020

wfurt mentioned this pull request Feb 27, 2020

Annotate System.Net.Security for nullable reference types #32541

Merged