process more TLS frames at one when available #50815

wfurt · 2021-04-06T21:52:26Z

This is fragment of original #49743
This change will process more frames if already read from transport returning bigger chunks of data to caller.
Aside from the big loop change this adds more spanification and tracing for EOF as we may reach it while already decrypted some data.

AuxRecord tests are removed as they depend on particular read side.

fixes #49000

ghost · 2021-04-06T21:52:30Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This is fragment of original #49743
This change will process more frames if already read from transport returning bigger chunks of data to caller.
Aside from the big loop change this adds more spanification and tracing for EOF as we may reach it while already decrypted some data.

AuxRecord tests are removed as they depend on particular read side.

fixes #49000

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Security`
Milestone:	-

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

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

davidfowl · 2021-04-23T14:32:39Z

Preview5?

…fers2

wfurt · 2021-04-30T19:48:09Z

I did another pass towards direction we discussed @geoffkizer.
While this is not 100% complete it would be great if you can take another look and provide feedback.
To make that more manageable I moved all the IO to getFullFrameIfNeed() and I also added haveFullTlsFrame() helper function to make it more obvious.

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

geoffkizer · 2021-05-03T17:33:03Z

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

+            // _internalOffset is 0 after ResetReadBuffer and we use _internalBufferCount to determined where to read.
+            while (_internalBufferCount < frameSize)
+            {
+                // We don't have enough bytes buffered, so issue an initial read to try to get enough.  This is


This comment isn't really correct anymore

What do you think is incorrect? I'm reading it again and it seems to describe the intent.

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

geoffkizer · 2021-05-13T06:42:52Z

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

+
+        private SecurityStatusPal DecryptData(int frameSize, out int decryptedCount, out int decryptedOffset)
+        {
+            // Set _decrytpedBytesOffset/Count to the current frame we have (including header)


Seems like we should assert that _decryptedBytesCount is 0 here, right? Otherwise we could be overwriting unconsumed decrypted data.

yes, the asset is good idea.

Suggested change

// Set _decrytpedBytesOffset/Count to the current frame we have (including header)

// Set _decryptedBytesOffset/Count to the current frame we have (including header)

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

geoffkizer · 2021-05-13T06:53:40Z

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

+                    TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header);
+                    if (_lastFrame.Header.Type != TlsContentType.AppData)
+                    {
+                        // Alerts, handshake and anything else will be processed separately.


Why? Seems like it's better to process these sooner rather than later. And there could be more data in subsequent frames as well, right?

I'm worried about compatibility. Goal of this change is efficiency, not behavior changes. While I don't have specific example (besides the AUX tests) currently all the additional service frames would be processed after handing out data received prior.

Ok, seems reasonable. Can we at least add a comment that this probably isn't strictly necessary?

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

…fers2

wfurt · 2021-06-14T12:24:43Z

This should be ready for another review round @geoffkizer @stephentoub. With exception of the Async debate all the feedback should be addressed.

karelz · 2021-06-16T10:08:38Z

@geoffkizer can you please finish code review here?

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

geoffkizer

A couple nits above. Generally LGTM

…fers1

…fers2

davidfowl · 2021-06-24T15:33:42Z

👍🏾

…bugger2 * origin/main: (107 commits) Disable MacCatalyst arm64 PR test runs on staging pipeline (dotnet#54678) [WASM] Fix async/await in config loading (dotnet#54652) Fix for heap_use_after_free flagged by sanitizer (dotnet#54679) [wasm] Bump emscripten to 2.0.23 (dotnet#53603) Fix compiler references when building inside VS (dotnet#54614) process more TLS frames at one when available (dotnet#50815) Add PeriodicTimer (dotnet#53899) UdpClient with span support (dotnet#53429) exclude fragile tests (dotnet#54671) get last error before calling a method that might fail as well (dotnet#54667) [FileStream] add tests for device and UNC paths (dotnet#54545) Fix sporadic double fd close (dotnet#54660) Remove Version.Clone from AssemblyName.Clone (dotnet#54621) [wasm] Enable fixed libraries tests (dotnet#54641) [wasm] Fix blazor/aot builds (dotnet#54651) [mono][wasm] Fix compilation error on wasm (dotnet#54659) Fix telemetry for Socket connects to Dns endpoints (dotnet#54071) [wasm] Build static components; include hot_reload in runtime (dotnet#54568) [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300) Put Crossgen2 in sync with dotnet#54235 (dotnet#54438) ...

This reverts commit 2ac023c.

wfurt added 3 commits March 31, 2021 17:19

process more TLS frames when available

eca0f25

add SslStream.Implementation.cs

a1954cc

remove extra comment

8d191b6

wfurt added the area-System.Net.Security label Apr 6, 2021

wfurt requested review from davidfowl, stephentoub and geoffkizer April 6, 2021 21:52

wfurt self-assigned this Apr 6, 2021

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Show resolved Hide resolved

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 7, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 8, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 8, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Apr 8, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Show resolved Hide resolved

wfurt added 3 commits April 29, 2021 00:05

Merge branch 'main' of https://github.com/dotnet/runtime into ssl_buf…

045ee03

…fers2

add back DefaultRequestHeaders_SentUnparsed

3eb022c

feedback from review

cb216c8

wfurt added 2 commits April 30, 2021 14:42

fix winhttp

d88a170

fix linker test

bed839e

geoffkizer reviewed May 3, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 3, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Show resolved Hide resolved

geoffkizer reviewed May 3, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 3, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 13, 2021

View reviewed changes

scalablecory approved these changes May 13, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

wfurt mentioned this pull request May 13, 2021

add NegotiateClientCertificateAsync support on Windows #51905

Merged

wfurt added 3 commits June 12, 2021 20:26

Merge branch 'main' of https://github.com/dotnet/runtime into ssl_buf…

6b2c583

…fers2

feedback from review

2c32bfd

feedback from review

65ac2d1

make EnsureFullTlsFrame async

c98dba8

karelz assigned geoffkizer Jun 16, 2021

geoffkizer reviewed Jun 22, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jun 22, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

geoffkizer approved these changes Jun 22, 2021

View reviewed changes

wfurt added 3 commits June 23, 2021 13:05

feedback from review

ac9d288

Merge branch 'main' of https://github.com/dotnet/runtime into ssl_buf…

1164aa0

…fers1

Merge branch 'main' of https://github.com/dotnet/runtime into ssl_buf…

3be81a4

…fers2

wfurt merged commit 2ac023c into dotnet:main Jun 24, 2021

MihaZupan added a commit to MihaZupan/runtime that referenced this pull request Jun 30, 2021

Revert "process more TLS frames at one when available (dotnet#50815)"

ee68bfd

This reverts commit 2ac023c.

wfurt mentioned this pull request Jul 1, 2021

SslStream should avoid copying read buffer data when it already has another frame to process #49086

Closed

karelz added this to the 6.0.0 milestone Jul 15, 2021

karelz mentioned this pull request Jul 28, 2021

System.Net.Security test run killed on Mono Linux #56478

Closed

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process more TLS frames at one when available #50815

process more TLS frames at one when available #50815

wfurt commented Apr 6, 2021

ghost commented Apr 6, 2021

davidfowl commented Apr 23, 2021

wfurt commented Apr 30, 2021

geoffkizer May 3, 2021

wfurt Jun 14, 2021 •

edited

Loading

geoffkizer May 13, 2021

wfurt Jun 14, 2021

geoffkizer Jun 22, 2021

geoffkizer May 13, 2021

wfurt Jun 14, 2021

geoffkizer Jun 22, 2021

wfurt commented Jun 14, 2021

karelz commented Jun 16, 2021

geoffkizer left a comment

davidfowl commented Jun 24, 2021

	// Set _decrytpedBytesOffset/Count to the current frame we have (including header)
	// Set _decryptedBytesOffset/Count to the current frame we have (including header)

process more TLS frames at one when available #50815

process more TLS frames at one when available #50815

Conversation

wfurt commented Apr 6, 2021

ghost commented Apr 6, 2021

davidfowl commented Apr 23, 2021

wfurt commented Apr 30, 2021

geoffkizer May 3, 2021

Choose a reason for hiding this comment

wfurt Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

geoffkizer May 13, 2021

Choose a reason for hiding this comment

wfurt Jun 14, 2021

Choose a reason for hiding this comment

geoffkizer Jun 22, 2021

Choose a reason for hiding this comment

geoffkizer May 13, 2021

Choose a reason for hiding this comment

wfurt Jun 14, 2021

Choose a reason for hiding this comment

geoffkizer Jun 22, 2021

Choose a reason for hiding this comment

wfurt commented Jun 14, 2021

karelz commented Jun 16, 2021

geoffkizer left a comment

Choose a reason for hiding this comment

davidfowl commented Jun 24, 2021

wfurt Jun 14, 2021 •

edited

Loading