Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick · 2024-04-08T22:19:44Z

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this:

An internal implementation of ArrayBufferWriter is added for platforms that do not support it
SqlObjectPool is imbued with the ability to create/reset pooled objects
TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs
Native methods are updated to take in Span/* for writeable byte[]
SSPIContextProvider signature is updated to take IBufferWriter

Part of #2253

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this: - An internal implementation of ArrayBufferWriter is added for platforms that do not support it - SqlObjectPool is imbued with the ability to create/reset pooled objects - TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs - Native methods are updated to take in Span/* for writeable byte[] - SSPIContextProvider signature is updated to take IBufferWriter

twsouthwick · 2024-05-07T21:20:34Z

@David-Engel can I get a review on this next step in the SSPI journey?

codecov · 2024-05-08T03:24:57Z

Codecov Report

Attention: Patch coverage is 75.89286% with 27 lines in your changes missing coverage. Please review.

Project coverage is 72.71%. Comparing base (f7ab115) to head (6bc8358).
Report is 17 commits behind head on main.

❗ Current head 6bc8358 differs from pull request most recent head 7c3335e

Please upload reports for the commit 7c3335e to get more accurate results.

Files	Patch %	Lines
.../src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs	56.25%	21 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs	72.22%	5 Missing ⚠️
...t/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2452      +/-   ##
==========================================
- Coverage   72.81%   72.71%   -0.11%     
==========================================
  Files         311      314       +3     
  Lines       61694    61787      +93     
==========================================
+ Hits        44922    44926       +4     
- Misses      16772    16861      +89

Flag	Coverage Δ
addons	`92.88% <ø> (ø)`
netcore	`77.02% <94.59%> (-0.16%)`	⬇️
netfx	`70.42% <73.52%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twsouthwick · 2024-05-13T18:04:31Z

@David-Engel ping

David-Engel · 2024-05-13T23:39:20Z

I ran our internal Kerberos test pipeline against this PR and all tests passed.

twsouthwick · 2024-05-28T20:16:11Z

ping @JRahnama @DavoudEshtehari @arellegue

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs

twsouthwick · 2024-08-19T17:48:42Z

@David-Engel can you run this on the kerberos suite again?

twsouthwick · 2024-08-20T16:50:52Z

@David-Engel something is up with the merges so let me get those fixed before you do the kerberos runs

twsouthwick · 2024-08-20T18:24:10Z

@David-Engel I'm getting sporadic errors that don't seem to always repro: https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=96359&view=logs&j=b14281d2-3365-502e-c6c8-e9e46d660715&t=a67f9feb-8bad-50ac-92c7-d62292e3e6fb&l=63

Have you seen these or are they related to my pr?

DavoudEshtehari · 2024-08-21T00:26:44Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

@@ -519,6 +519,9 @@
    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlObjectPool.cs">
      <Link>Microsoft\Data\SqlClient\SqlObjectPool.cs</Link>
    </Compile>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ArrayBufferWriter.cs">


Just a minor adjustment here to keep the files in alphabetically order.

DavoudEshtehari · 2024-08-21T00:27:01Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj

+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlObjectPool.cs">
+      <Link>Microsoft\Data\SqlClient\SqlObjectPool.cs</Link>
+    </Compile>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ArrayBufferWriter.cs">


Same note here.

DavoudEshtehari · 2024-08-21T01:50:55Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#if NETFRAMEWORK || NETSTANDARD


There is no NetStandard anymore.

Suggested change

#if NETFRAMEWORK || NETSTANDARD

#if NETFRAMEWORK

DavoudEshtehari · 2024-08-21T02:00:06Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

+
+        private static void ThrowInvalidOperationException_AdvancedTooFar(int capacity)
+        {
+            throw new InvalidOperationException($"Buffer advanced to far: {capacity}");


The new message should be added in the Strings.res.

DavoudEshtehari · 2024-08-21T02:00:19Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

+
+        private static void ThrowOutOfMemoryException(uint capacity)
+        {
+            throw new OutOfMemoryException($"Buffer maximum size exceeded: {capacity}");


Same note here.

DavoudEshtehari · 2024-08-21T02:06:02Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs


-            if (rentedSSPIBuff != null)
+            if (sspiWriter is { })


I believe the is not null has more readability over this pattern in the cases like this.

DavoudEshtehari · 2024-08-21T02:12:09Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


No complain here only I'm curious about the constant 20 pooled objects here!
cc @David-Engel

David-Engel · 2024-08-21T22:37:44Z

@David-Engel I'm getting sporadic errors that don't seem to always repro: https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=96359&view=logs&j=b14281d2-3365-502e-c6c8-e9e46d660715&t=a67f9feb-8bad-50ac-92c7-d62292e3e6fb&l=63

Have you seen these or are they related to my pr?

I think they are. CI runs against main are clean. The failures point to integrated authentication when running against a named instance.

cheenamalhotra · 2024-10-18T20:39:45Z

@twsouthwick

Getting back to prioritizing these PRs, can you please address pending feedback and update branch? Thanks!

twsouthwick · 2024-10-21T03:33:35Z

Sure! It'll be about a week until I can look at it, but I'll get to it

This was referenced Apr 8, 2024

Add hook for custom SSPI context for SqlConnection instances #2253

Open

Expose SqlAuthenticationParameters on SSPIContextProvider #2454

Draft

twsouthwick force-pushed the sspi-writer branch from e52f1c3 to 1ec663f Compare May 7, 2024 16:28

twsouthwick marked this pull request as ready for review May 7, 2024 21:20

David-Engel approved these changes May 13, 2024

View reviewed changes

David-Engel requested review from JRahnama, DavoudEshtehari and arellegue May 13, 2024 23:38

David-Engel added this to the 6.0-preview1 milestone Jun 3, 2024

twsouthwick added 4 commits June 27, 2024 11:15

Merge branch 'main' into sspi-writer

7c3335e

Merge remote-tracking branch 'upstream/main' into sspi-writer

4c3c3f2

fix

d3bd2b9

switch to span

266cf7f

David-Engel reviewed Jul 31, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs Outdated Show resolved Hide resolved

add return

7d99053

twsouthwick mentioned this pull request Aug 19, 2024

Use string for SNI instead of byte[] #2790

Draft

twsouthwick added 3 commits August 19, 2024 11:17

Merge remote-tracking branch 'upstream/main' into sspi-writer

7e4d15f

revert

09db047

use return

a49abc3

inline

a1703d8

DavoudEshtehari reviewed Aug 21, 2024

View reviewed changes

DavoudEshtehari modified the milestones: 6.0-preview1, 6.0-preview2 Aug 26, 2024

cheenamalhotra removed this from the 6.0-preview2 milestone Oct 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick commented Apr 8, 2024 •

edited

Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 •

edited

Loading

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

DavoudEshtehari Aug 21, 2024

David-Engel commented Aug 21, 2024

cheenamalhotra commented Oct 18, 2024

twsouthwick commented Oct 21, 2024

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Are you sure you want to change the base?

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Conversation

twsouthwick commented Apr 8, 2024 • edited Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 • edited Loading

Codecov Report

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

David-Engel commented Aug 21, 2024

cheenamalhotra commented Oct 18, 2024

twsouthwick commented Oct 21, 2024

twsouthwick commented Apr 8, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading