Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.6k
Improve allocations in NegotiateStreamPal#71280
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
1dd747d
Reduce buffer allocations during NTLM/Negotiate authentication
filipnavara 49c1ed0
Update ReadWriteAdapter.WriteAsync prototype to use Memory<byte> inst…
filipnavara 2fe4491
Spanify NTAuthentication.Decrypt and avoid couple of offset/count checks
filipnavara 3b4aff6
Spanify NegotiateStreamPal.VerifySignature/MakeSignature.
filipnavara 46e05a4
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
filipnavara File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
19 changes: 8 additions & 11 deletions
19 ...libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2 src/libraries/Common/src/Interop/Windows/SspiCli/ISSPIInterface.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2 src/libraries/Common/src/Interop/Windows/SspiCli/SSPIAuthType.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2 src/libraries/Common/src/Interop/Windows/SspiCli/SSPISecureChannelType.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
143 changes: 1 addition & 142 deletions
143 src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 14 additions & 13 deletions
27 src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 21 additions & 12 deletions
33 src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -20,6 +20,7 @@ internal sealed partial class NTAuthentication | ||
| private string? _spn; | ||
| private int _tokenSize; | ||
| private byte[]? _tokenBuffer; | ||
| private ContextFlagsPal _requestedContextFlags; | ||
| private ContextFlagsPal _contextFlags; | ||
| @@ -158,14 +159,14 @@ internal void CloseContext() | ||
| _isCompleted = false; | ||
| } | ||
| internal int VerifySignature(byte[] buffer, int offset, int count) | ||
| internal int VerifySignature(ReadOnlySpan<byte> buffer) | ||
| { | ||
| return NegotiateStreamPal.VerifySignature(_securityContext!, buffer, offset, count); | ||
| return NegotiateStreamPal.VerifySignature(_securityContext!, buffer); | ||
| } | ||
| internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref byte[] output) | ||
| internal int MakeSignature(ReadOnlySpan<byte> buffer, [AllowNull] ref byte[] output) | ||
| { | ||
| return NegotiateStreamPal.MakeSignature(_securityContext!, buffer, offset, count, ref output); | ||
| return NegotiateStreamPal.MakeSignature(_securityContext!, buffer, ref output); | ||
| } | ||
| internal string? GetOutgoingBlob(string? incomingBlob) | ||
| @@ -221,9 +222,10 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref | ||
| internal byte[]? GetOutgoingBlob(ReadOnlySpan<byte> incomingBlob, bool throwOnError, out SecurityStatusPal statusCode) | ||
| { | ||
| byte[]? result = new byte[_tokenSize]; | ||
| _tokenBuffer ??= _tokenSize == 0 ? Array.Empty<byte>() : new byte[_tokenSize]; | ||
stephentoub marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| bool firstTime = _securityContext == null; | ||
| int resultBlobLength; | ||
| try | ||
| { | ||
| if (!_isServer) | ||
| @@ -236,18 +238,19 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref | ||
| _requestedContextFlags, | ||
| incomingBlob, | ||
| _channelBinding, | ||
| ref result, | ||
| ref _tokenBuffer, | ||
| out resultBlobLength, | ||
| ref _contextFlags); | ||
| if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"SSPIWrapper.InitializeSecurityContext() returns statusCode:0x{((int)statusCode.ErrorCode):x8} ({statusCode})"); | ||
| if (statusCode.ErrorCode == SecurityStatusPalErrorCode.CompleteNeeded) | ||
| { | ||
| statusCode = NegotiateStreamPal.CompleteAuthToken(ref _securityContext, result); | ||
| statusCode = NegotiateStreamPal.CompleteAuthToken(ref _securityContext, _tokenBuffer.AsSpan(0, resultBlobLength)); | ||
| if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"SSPIWrapper.CompleteAuthToken() returns statusCode:0x{((int)statusCode.ErrorCode):x8} ({statusCode})"); | ||
| result = null; | ||
| resultBlobLength = 0; | ||
| } | ||
| } | ||
| else | ||
| @@ -259,7 +262,8 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref | ||
| _requestedContextFlags, | ||
| incomingBlob, | ||
| _channelBinding, | ||
| ref result, | ||
| ref _tokenBuffer, | ||
| out resultBlobLength, | ||
| ref _contextFlags); | ||
| if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"SSPIWrapper.AcceptSecurityContext() returns statusCode:0x{((int)statusCode.ErrorCode):x8} ({statusCode})"); | ||
| @@ -284,6 +288,7 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref | ||
| { | ||
| CloseContext(); | ||
| _isCompleted = true; | ||
| _tokenBuffer = null; | ||
| if (throwOnError) | ||
| { | ||
| throw NegotiateStreamPal.CreateExceptionFromError(statusCode); | ||
| @@ -297,12 +302,18 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref | ||
| SSPIHandleCache.CacheCredential(_credentialsHandle); | ||
| } | ||
| byte[]? result = | ||
| resultBlobLength == 0 || _tokenBuffer == null ? null : | ||
| _tokenBuffer.Length == resultBlobLength ? _tokenBuffer : | ||
| _tokenBuffer[0..resultBlobLength]; | ||
| // The return value will tell us correctly if the handshake is over or not | ||
| if (statusCode.ErrorCode == SecurityStatusPalErrorCode.OK | ||
| || (_isServer && statusCode.ErrorCode == SecurityStatusPalErrorCode.CompleteNeeded)) | ||
| { | ||
| // Success. | ||
| _isCompleted = true; | ||
| _tokenBuffer = null; | ||
| } | ||
| else | ||
| { | ||
| @@ -335,13 +346,11 @@ internal int Encrypt(ReadOnlySpan<byte> buffer, [NotNull] ref byte[]? output, ui | ||
| sequenceNumber); | ||
| } | ||
| internal int Decrypt(byte[] payload, int offset, int count, out int newOffset, uint expectedSeqNumber) | ||
| internal int Decrypt(Span<byte> payload, out int newOffset, uint expectedSeqNumber) | ||
| { | ||
| return NegotiateStreamPal.Decrypt( | ||
| _securityContext!, | ||
| payload, | ||
| offset, | ||
| count, | ||
| (_contextFlags & ContextFlagsPal.Confidentiality) != 0, | ||
| IsNTLM, | ||
| out newOffset, | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Do we need offset any more? It feels like if we pass pointer we can just do count.
Uh oh!
There was an error while loading. Please reload this page.
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.
We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
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.
Wouldn't the pointer change by itself be breaking for them?
Uh oh!
There was an error while loading. Please reload this page.
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 doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
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.
cc: @JRahnama just in case.
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 cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the
offsetparameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.