From a21760502437073a21c233c3da3c956da05f29f4 Mon Sep 17 00:00:00 2001 From: David Shulman Date: Thu, 6 Jun 2019 14:19:07 -0700 Subject: [PATCH] Rework Linux/Kerberos native interop layer The latest changes to the System.Net.Security.Native shim layer fixed a lot of important bugs for Linux Kerberos usage. But this created a new problem since SqlClient ships in out-of-band NuGet packages separate from the .NET Core runtime. SqlClient builds out of the CoreFx repo and uses the common source includes for Kerberos authentication. This created an unexpected dependency on the System.Net.Security.Native shim layer. The recent changes to these API signatures caused problems with different combinations of SqlClient NuGet packages and .NET Core 2.x versus .NET Core 3.0. After discussion with the SqlClient team, we decided to rework the changes to these native APIs so that they would remain compatible across all .NET Core versions. Long-term, the plan is to implement #36896 to expose a Kerberos API in .NET Core which could be used by SqlClient and other consumers. Closes #37183 Closes #25205 --- .../Interop.NetSecurityNative.cs | 18 ++++- .../Win32/SafeHandles/GssSafeHandles.cs | 2 +- .../Net/Security/NegotiateStreamPal.Unix.cs | 62 ++++++++++------- .../Security/Unix/SafeDeleteNegoContext.cs | 5 +- .../System.Net.Security.Native/pal_gssapi.c | 69 +++++++++++++++++-- .../System.Net.Security.Native/pal_gssapi.h | 18 ++++- .../src/System.Data.SqlClient.csproj | 2 - .../Data/SqlClient/SNI/SNIProxy.Unix.cs | 16 ----- .../Data/SqlClient/SNI/SNIProxy.Windows.cs | 16 ----- .../src/System/Data/SqlClient/SNI/SNIProxy.cs | 4 +- 10 files changed, 134 insertions(+), 78 deletions(-) delete mode 100644 src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Unix.cs delete mode 100644 src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Windows.cs diff --git a/src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs b/src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs index b3add795f2ef..5c0a73ff94e0 100644 --- a/src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs +++ b/src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs @@ -36,8 +36,8 @@ internal static extern Status ImportUserName( int inputNameByteCount, out SafeGssNameHandle outputName); - [DllImport(Interop.Libraries.NetSecurityNative, EntryPoint="NetSecurityNative_ImportTargetName")] - internal static extern Status ImportTargetName( + [DllImport(Interop.Libraries.NetSecurityNative, EntryPoint="NetSecurityNative_ImportPrincipalName")] + internal static extern Status ImportPrincipalName( out Status minorStatus, string inputName, int inputNameByteCount, @@ -69,6 +69,20 @@ internal static extern Status ReleaseCred( ref IntPtr credHandle); [DllImport(Interop.Libraries.NetSecurityNative, EntryPoint="NetSecurityNative_InitSecContext")] + internal static extern Status InitSecContext( + out Status minorStatus, + SafeGssCredHandle initiatorCredHandle, + ref SafeGssContextHandle contextHandle, + bool isNtlmOnly, + SafeGssNameHandle targetName, + uint reqFlags, + byte[] inputBytes, + int inputLength, + ref GssBuffer token, + out uint retFlags, + out bool isNtlmUsed); + + [DllImport(Interop.Libraries.NetSecurityNative, EntryPoint="NetSecurityNative_InitSecContextEx")] internal static extern Status InitSecContext( out Status minorStatus, SafeGssCredHandle initiatorCredHandle, diff --git a/src/Common/src/Microsoft/Win32/SafeHandles/GssSafeHandles.cs b/src/Common/src/Microsoft/Win32/SafeHandles/GssSafeHandles.cs index ef676261e363..aa3727c1cd70 100644 --- a/src/Common/src/Microsoft/Win32/SafeHandles/GssSafeHandles.cs +++ b/src/Common/src/Microsoft/Win32/SafeHandles/GssSafeHandles.cs @@ -36,7 +36,7 @@ public static SafeGssNameHandle CreateTarget(string name) Debug.Assert(!string.IsNullOrEmpty(name), "Invalid target name passed to SafeGssNameHandle create"); SafeGssNameHandle retHandle; Interop.NetSecurityNative.Status minorStatus; - Interop.NetSecurityNative.Status status = Interop.NetSecurityNative.ImportTargetName( + Interop.NetSecurityNative.Status status = Interop.NetSecurityNative.ImportPrincipalName( out minorStatus, name, Encoding.UTF8.GetByteCount(name), out retHandle); if (status != Interop.NetSecurityNative.Status.GSS_S_COMPLETE) diff --git a/src/Common/src/System/Net/Security/NegotiateStreamPal.Unix.cs b/src/Common/src/System/Net/Security/NegotiateStreamPal.Unix.cs index 1b17c6f15cc6..7a37ee3d9d77 100644 --- a/src/Common/src/System/Net/Security/NegotiateStreamPal.Unix.cs +++ b/src/Common/src/System/Net/Security/NegotiateStreamPal.Unix.cs @@ -107,18 +107,6 @@ private static bool GssInitSecurityContext( out uint outFlags, out bool isNtlmUsed) { - // If a TLS channel binding token (cbt) is available then get the pointer - // to the application specific data. - IntPtr cbtAppData = IntPtr.Zero; - int cbtAppDataSize = 0; - if (channelBinding != null) - { - int appDataOffset = Marshal.SizeOf(); - Debug.Assert(appDataOffset < channelBinding.Size); - cbtAppData = channelBinding.DangerousGetHandle() + appDataOffset; - cbtAppDataSize = channelBinding.Size - appDataOffset; - } - outputBuffer = null; outFlags = 0; @@ -138,19 +126,43 @@ private static bool GssInitSecurityContext( try { Interop.NetSecurityNative.Status minorStatus; - status = Interop.NetSecurityNative.InitSecContext(out minorStatus, - credential, - ref context, - isNtlm, - cbtAppData, - cbtAppDataSize, - targetName, - (uint)inFlags, - buffer, - (buffer == null) ? 0 : buffer.Length, - ref token, - out outFlags, - out isNtlmUsed); + + if (channelBinding != null) + { + // If a TLS channel binding token (cbt) is available then get the pointer + // to the application specific data. + int appDataOffset = Marshal.SizeOf(); + Debug.Assert(appDataOffset < channelBinding.Size); + IntPtr cbtAppData = channelBinding.DangerousGetHandle() + appDataOffset; + int cbtAppDataSize = channelBinding.Size - appDataOffset; + status = Interop.NetSecurityNative.InitSecContext(out minorStatus, + credential, + ref context, + isNtlm, + cbtAppData, + cbtAppDataSize, + targetName, + (uint)inFlags, + buffer, + (buffer == null) ? 0 : buffer.Length, + ref token, + out outFlags, + out isNtlmUsed); + } + else + { + status = Interop.NetSecurityNative.InitSecContext(out minorStatus, + credential, + ref context, + isNtlm, + targetName, + (uint)inFlags, + buffer, + (buffer == null) ? 0 : buffer.Length, + ref token, + out outFlags, + out isNtlmUsed); + } if ((status != Interop.NetSecurityNative.Status.GSS_S_COMPLETE) && (status != Interop.NetSecurityNative.Status.GSS_S_CONTINUE_NEEDED)) diff --git a/src/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs b/src/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs index 935ca3786413..07a768b36e98 100644 --- a/src/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs +++ b/src/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs @@ -43,10 +43,7 @@ public SafeDeleteNegoContext(SafeFreeNegoCredentials credential, string targetNa { try { - // Convert any "SERVICE/HOST" style of targetName to use "SERVICE@HOST" style. - // This is because the System.Net.Security.Native GSS-API layer uses - // GSS_C_NT_HOSTBASED_SERVICE format for targetName. - _targetName = SafeGssNameHandle.CreateTarget(targetName.Replace('/', '@')); + _targetName = SafeGssNameHandle.CreateTarget(targetName); } catch { diff --git a/src/Native/Unix/System.Net.Security.Native/pal_gssapi.c b/src/Native/Unix/System.Net.Security.Native/pal_gssapi.c index 926cd4e64f23..041a23b450c5 100644 --- a/src/Native/Unix/System.Net.Security.Native/pal_gssapi.c +++ b/src/Native/Unix/System.Net.Security.Native/pal_gssapi.c @@ -144,26 +144,52 @@ NetSecurityNative_ImportUserName(uint32_t* minorStatus, char* inputName, uint32_ return gss_import_name(minorStatus, &inputNameBuffer, GSS_C_NT_USER_NAME, outputName); } -uint32_t NetSecurityNative_ImportTargetName(uint32_t* minorStatus, - char* inputName, - uint32_t inputNameLen, - GssName** outputName) +uint32_t NetSecurityNative_ImportPrincipalName(uint32_t* minorStatus, + char* inputName, + uint32_t inputNameLen, + GssName** outputName) { assert(minorStatus != NULL); assert(inputName != NULL); assert(outputName != NULL); assert(*outputName == NULL); + // Principal name will usually be in the form SERVICE/HOST. But SPNEGO protocol prefers + // GSS_C_NT_HOSTBASED_SERVICE format. That format uses '@' separator instead of '/' between + // service name and host name. So convert input string into that format. + char* ptrSlash = memchr(inputName, '/', inputNameLen); + char* inputNameCopy = NULL; + if (ptrSlash != NULL) + { + inputNameCopy = (char*) malloc(inputNameLen); + if (inputNameCopy != NULL) + { + memcpy(inputNameCopy, inputName, inputNameLen); + inputNameCopy[ptrSlash - inputName] = '@'; + inputName = inputNameCopy; + } + else + { + *minorStatus = 0; + return GSS_S_BAD_NAME; + } + } + GssBuffer inputNameBuffer = {.length = inputNameLen, .value = inputName}; - return gss_import_name(minorStatus, &inputNameBuffer, GSS_C_NT_HOSTBASED_SERVICE, outputName); + uint32_t result = gss_import_name(minorStatus, &inputNameBuffer, GSS_C_NT_HOSTBASED_SERVICE, outputName); + + if (inputNameCopy != NULL) + { + free(inputNameCopy); + } + + return result; } uint32_t NetSecurityNative_InitSecContext(uint32_t* minorStatus, GssCredId* claimantCredHandle, GssCtxId** contextHandle, uint32_t isNtlm, - void* cbt, - int32_t cbtSize, GssName* targetName, uint32_t reqFlags, uint8_t* inputBytes, @@ -171,6 +197,35 @@ uint32_t NetSecurityNative_InitSecContext(uint32_t* minorStatus, PAL_GssBuffer* outBuffer, uint32_t* retFlags, int32_t* isNtlmUsed) +{ + return NetSecurityNative_InitSecContextEx(minorStatus, + claimantCredHandle, + contextHandle, + isNtlm, + NULL, + 0, + targetName, + reqFlags, + inputBytes, + inputLength, + outBuffer, + retFlags, + isNtlmUsed); +} + +uint32_t NetSecurityNative_InitSecContextEx(uint32_t* minorStatus, + GssCredId* claimantCredHandle, + GssCtxId** contextHandle, + uint32_t isNtlm, + void* cbt, + int32_t cbtSize, + GssName* targetName, + uint32_t reqFlags, + uint8_t* inputBytes, + uint32_t inputLength, + PAL_GssBuffer* outBuffer, + uint32_t* retFlags, + int32_t* isNtlmUsed) { assert(minorStatus != NULL); assert(contextHandle != NULL); diff --git a/src/Native/Unix/System.Net.Security.Native/pal_gssapi.h b/src/Native/Unix/System.Net.Security.Native/pal_gssapi.h index e3cf72fb4cc3..58a6662644b3 100644 --- a/src/Native/Unix/System.Net.Security.Native/pal_gssapi.h +++ b/src/Native/Unix/System.Net.Security.Native/pal_gssapi.h @@ -79,7 +79,7 @@ NetSecurityNative_ImportUserName(uint32_t* minorStatus, char* inputName, uint32_ /* Shims the gss_import_name method with nametype = GSS_C_NT_USER_NAME. */ -DLLEXPORT uint32_t NetSecurityNative_ImportTargetName(uint32_t* minorStatus, +DLLEXPORT uint32_t NetSecurityNative_ImportPrincipalName(uint32_t* minorStatus, char* inputName, uint32_t inputNameLen, GssName** outputName); @@ -107,8 +107,6 @@ DLLEXPORT uint32_t NetSecurityNative_InitSecContext(uint32_t* minorStatus, GssCredId* claimantCredHandle, GssCtxId** contextHandle, uint32_t isNtlm, - void* cbt, - int32_t cbtSize, GssName* targetName, uint32_t reqFlags, uint8_t* inputBytes, @@ -117,6 +115,20 @@ DLLEXPORT uint32_t NetSecurityNative_InitSecContext(uint32_t* minorStatus, uint32_t* retFlags, int32_t* isNtlmUsed); +DLLEXPORT uint32_t NetSecurityNative_InitSecContextEx(uint32_t* minorStatus, + GssCredId* claimantCredHandle, + GssCtxId** contextHandle, + uint32_t isNtlm, + void* cbt, + int32_t cbtSize, + GssName* targetName, + uint32_t reqFlags, + uint8_t* inputBytes, + uint32_t inputLength, + PAL_GssBuffer* outBuffer, + uint32_t* retFlags, + int32_t* isNtlmUsed); + /* Shims the gss_accept_sec_context method. */ diff --git a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj index 44c8cb23c4ee..c86f734e15e8 100644 --- a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj +++ b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj @@ -211,8 +211,6 @@ - - diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Unix.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Unix.cs deleted file mode 100644 index b587dedee32a..000000000000 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Unix.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -namespace System.Data.SqlClient.SNI -{ - /// - /// Managed SNI proxy implementation. Contains many SNI entry points used by SqlClient. - /// - internal partial class SNIProxy - { - // On Unix/Linux the format for the SPN is SERVICE@HOST. This is because the System.Net.Security.Native - // GSS-API layer uses GSS_C_NT_HOSTBASED_SERVICE format for the SPN. - private const string SpnServiceHostSeparator = "@"; - } -} diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Windows.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Windows.cs deleted file mode 100644 index 581056643151..000000000000 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.Windows.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -namespace System.Data.SqlClient.SNI -{ - /// - /// Managed SNI proxy implementation. Contains many SNI entry points used by SqlClient. - /// - internal partial class SNIProxy - { - // On Windows the format for the SPN is SERVICE/HOST. So the separator character between the - // SERVICE and HOST components is "/". - private const string SpnServiceHostSeparator = "/"; - } -} diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs index 8f1873756e0a..e99cb24ddff8 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs @@ -22,7 +22,7 @@ internal partial class SNIProxy { private const int DefaultSqlServerPort = 1433; private const int DefaultSqlServerDacPort = 1434; - private const string SqlServerSpnHeader = "MSSQLSvc"; + private const string SqlServerSpnHeader = "MSSQLSvc/"; public static readonly SNIProxy Singleton = new SNIProxy(); @@ -341,7 +341,7 @@ private static byte[] GetSqlServerSPN(string hostNameOrAddress, string portOrIns // If the DNS lookup failed, then resort to using the user provided hostname to construct the SPN. fullyQualifiedDomainName = hostEntry?.HostName ?? hostNameOrAddress; } - string serverSpn = SqlServerSpnHeader + SpnServiceHostSeparator + fullyQualifiedDomainName; + string serverSpn = SqlServerSpnHeader + fullyQualifiedDomainName; if (!string.IsNullOrWhiteSpace(portOrInstanceName)) { serverSpn += ":" + portOrInstanceName;