-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsPorts dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037 SummaryWhen working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes. Customer ImpactThis will prevent users from upgrading to Net6 while using Kerberos authentication in Unix. TestingAdding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.
|
@@ -16,6 +16,9 @@ internal static partial class NetSecurityNative | |||
IntPtr bufferPtr, | |||
ulong length); | |||
|
|||
[DllImport(Interop.Libraries.NetSecurityNative, EntryPoint = "NetSecurityNative_EnsureGssInitialized")] |
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.
You probably want to use 'GeneratedDllImport' like the others in the file.
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.
@JRahnama for context, this is new in 7.0. So you will need to use the new form in this change, and the back port will use the form you have here.
@wfurt you will remember this one. It was fixed in the Microsoft.SqlClient package but is hitting System.Data.SqlClient users trying to update to .NET 6. Can you confirm this if the right change? We will need to service it. |
I've not followed what's broken and why, and there are no tests in the PR. What is SqlClient using that is now broken in. NET 6? |
As well as the issue above the original discussion was was in #60906 |
yes, I can look @danmoseley. In essence SqlClient uses our internals directly @stephentopub and some changes we did in 6.0 broke some assumptions. In general prior to 6.0 OpenSSL was always initialized and ready to be used but that is no longer true in 6.0. (we had other similar issues around that) I think this is primarily for historical reasons from time when the code lived in corefx/runtime. |
// dotnet runtime issue https://github.com/dotnet/runtime/pull/55037 | ||
static NetSecurityNative() | ||
{ | ||
if (Environment.Version.Major >= 6) |
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.
This file is only included in .NET 7+ binaries in this repo, so this check is unnecessary.
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.
And the EnsureGssInitialized calls are already being done in the main branch:
Lines 17 to 20 in 3806e15
static NetSecurityNative() | |
{ | |
GssInitializer.Initialize(); | |
} |
Perhaps you meant to create a PR against one of the servicing branches?
Thanks, that's what I thought the answer was going to be. The fix is for SqlClient to stop doing that, not for runtime to maintain its private implementation details as callable surface area. This has been known for years: |
When I looked at the issue it wasn't clear to me what the fix actually is. If taking a copy of the used files into it's own repo isn't sufficient what is needed to decouple the two? It's also worth noting that there isn't going to be enough person power available to do anything even if it is high on the list of priorities (see #51836 (comment) ) so even if well understood the timeframe for doing this is years away. |
it needs to stop P/Invoking into runtime's native code: |
Well I tried to stop it doing that as well if you read the issue I referred to and that won't happen either. As for this PR. I suspect the right thing to do is to find the original PR that fixes it in this repo and request a backport to the 6.0 release branch. Does that sound right? |
It does not sound right. The PR that introduced GssInitializer is part of release/6.0. I do not think you actually need any fixes in the core runtime bits for this. If there is an actual problem to be solved, you should be able to keep hacking around it in SqlClient using more private reflection. (And of course, the proper way to fix this once for all is to delete the dependency on runtime impl details from SqlClient.) |
If that's the case then it would go in release/3.1 where this package ships from - and @JRahnama had it originally.. |
agreed. This is why I would like to add public surface for application to use Kerberos. (not the low-level) |
Thanks every one for taking time and looking into this. We are investigating native interop removal from managed SNI in Microsoft.Data.SqlClient and it is in our to-do list, but this issue is still remaining in System.Data.SqlClient users with Net 6. Our original intent by making this PR and the other one in corefx was to unblock users from upgrading to Net6 at first step. I am not sure if any fix from M.D.SqlClient in this matter could be back ported to S.D.SqlClient. What would be the solution for that? As @danmoseley mentioned should we reopen the mentioned PR in release/3.1 in corefx? |
Perhaps we should chat off-line @JRahnama. As @jkotas mentioned the runtime changes are only in .NET 6 .e.g there should be no need to touch 5.x tor 3.x ... unless you want SqlClient from there to work with 6.0 runtime. Prior to 6.0 OpenSsl was initialized automatically during library load. That was changed in 6.0 to explicit call to support some particular scenarios. |
I believe this is the goal. |
I assume that you meant dotnet/corefx#43133. The delta in this PR affects both the core runtime assemblies and System.Data.SqlClient. The change in core runtime assemblies is unnecessary - it comes with unnecessary risk. If you go with adding the workaround to release/3.1, it should be authored to affect System.Data.SqlClient only. |
Closing this PR and reopening the PR in corefx 3.1/release seems like the best option. |
Ports dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037
Tracked by issue dotnet/SqlClient#1390
Summary
When working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes.
Customer Impact
This will prevent users from upgrading to Net6 while using Kerberos authentication in Unix.
Testing
Adding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.