Skip to content
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.

Sign up for GitHub

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 | SqlLocalDB instance pipename issue with Encryption #1433

Merged
merged 3 commits into from
Jan 14, 2022
Merged

Fix | SqlLocalDB instance pipename issue with Encryption #1433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
659ba22
address issue with localdb named pipe
Dec 6, 2021
6af0b8e
review comments
Jan 7, 2022
e745d71
Merge branch 'main' of https://github.com/dotnet/SqlClient into sqlLo…
Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/LocalDBAPI.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@ private static IntPtr UserInstanceDLLHandle
if (s_userInstanceDLLHandle == IntPtr.Zero)
{
SNINativeMethodWrapper.SNIQueryInfo(SNINativeMethodWrapper.QTypes.SNI_QUERY_LOCALDB_HMODULE, ref s_userInstanceDLLHandle);
if(s_userInstanceDLLHandle != IntPtr.Zero)
if (s_userInstanceDLLHandle != IntPtr.Zero)
{
SqlClientEventSource.Log.TryTraceEvent("LocalDBAPI.UserInstanceDLLHandle | LocalDB - handle obtained");
}
else
{
SNINativeMethodWrapper.SNI_Error sniError;
SNINativeMethodWrapper.SNIGetLastError(out sniError);
SNINativeMethodWrapper.SNIGetLastError(out SNINativeMethodWrapper.SNI_Error sniError);
throw CreateLocalDBException(errorMessage: StringsHelper.GetString("LocalDB_FailedGetDLLHandle"), sniError: (int)sniError.sniError);
}
}
Expand Down
34 changes: 23 additions & 11 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/LocalDBAPI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,37 @@ namespace Microsoft.Data
{
internal static partial class LocalDBAPI
{
private const string const_localDbPrefix = @"(localdb)\";
private const string LocalDbPrefix = @"(localdb)\";
private const string LocalDbPrefix_NP = @"np:\\.\pipe\LOCALDB#";


[UnmanagedFunctionPointer(CallingConvention.Cdecl, CharSet = CharSet.Unicode)]
private delegate int LocalDBFormatMessageDelegate(int hrLocalDB, uint dwFlags, uint dwLanguageId, StringBuilder buffer, ref uint buflen);

// check if name is in format (localdb)\<InstanceName - not empty> and return instance name if it is
// localDB can also have a format of np:\\.\pipe\LOCALDB#<some number>\tsql\query
internal static string GetLocalDbInstanceNameFromServerName(string serverName)
{
if (serverName == null)
return null;
serverName = serverName.TrimStart(); // it can start with spaces if specified in quotes
if (!serverName.StartsWith(const_localDbPrefix, StringComparison.OrdinalIgnoreCase))
return null;
string instanceName = serverName.Substring(const_localDbPrefix.Length).Trim();
if (instanceName.Length == 0)
return null;
else
return instanceName;
if (serverName is not null)
JRahnama marked this conversation as resolved.
Show resolved Hide resolved
{
// it can start with spaces if specified in quotes
// Memory allocation is reduced by using ReadOnlySpan
ReadOnlySpan<char> input = serverName.AsSpan().Trim();
if (input.StartsWith(LocalDbPrefix.AsSpan(), StringComparison.OrdinalIgnoreCase))
{
input = input.Slice(LocalDbPrefix.Length);
if (!input.IsEmpty)
{
return input.ToString();
johnnypham marked this conversation as resolved.
Show resolved Hide resolved
}
}
else if (input.StartsWith(LocalDbPrefix_NP.AsSpan(), StringComparison.OrdinalIgnoreCase))
{
return input.ToString();
Copy link
Contributor

@johnnypham johnnypham Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the input should also be checked if it's empty here. I'm getting different errors when using the prefixes as the server:

(localdb)\;
(provider: SNI_PN11, error: 51 - An instance name was not specified while connecting to a Local Database Runtime. Specify an instance name in the format (localdb)\instance_name.)

np:\\.\pipe\LOCALDB#/:
(provider: Named Pipes Provider, error: 40 - Could not open a connection to SQL Server)

Copy link
Contributor Author

@JRahnama JRahnama Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is user error. The error is pretty much explanatory and that is the behavior from before for (localdb).
1- (localdb)\.
2-(localdb)\Instance name
3- (localdb)\.\shared instance name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for named pipe:
the only part changing in np:\\.\pipe\LOCALDB#SH452F48\tsql\query is the number inside it

Copy link
Contributor Author

@JRahnama JRahnama Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use other methods such as RegEx, but that would be an overkill. The fact that connection string starts with np:\\.\pipe\LOCALDB# is enough to conclude that user is trying LocalDB, so the Encrypt=true would not be applied to it. If user wants to change the named pipe server will reject that.

Copy link
Contributor

@johnnypham johnnypham Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of (localdb)\ , we tell the user that the server name is not in the correct format, i.e. the instance name is missing. In the case of np:\\.\pipe\LOCALDB#/, shouldn't we also tell them that the instance name is missing?

Edit: I don't think you understand my original comment. There is no need for regex here and this has nothing to do with encrypt=true. All I'm saying is when the user tries to connect with np:\\.\pipe\LOCALDB#/, we should tell them they are missing the instance name, just like we already do if they try connecting with (localdb)\.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of (localdb)\ , we tell the user that the server name is not in the correct format, i.e. the instance name is missing. In the case of np:\\.\pipe\LOCALDB#/, shouldn't we also tell them that the instance name is missing?

I dont think so. Because that pattern is not created by user. It is in a folder in LocalDB inside Program File folder. So, you cant just create whatever you want. One can read the name from the folder using a process or copy paste it, whereas (localdb) could have some user mistakes. Plus are you testing in managed SNI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is the same in managed SNI.

The whole point of error handling is that we can't assume that the user knows what they're doing and got their named pipe server name from the correct source. If we assumed that the user does the right thing every time, there would be no need for error handling.

The error message when trying to connect to np:\\.\pipe\LOCALDB#/ states that a connection could not be opened because the server was not found or was inaccessible. We should be more specific and say that the connection failed because the provided server name was not even in the correct format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see merit in aligning the errors produced between tcp and np scenarios. However, I wouldn't block this PR since we are not changing the behavior here. The behavior can be addressed in the future so that we don't delay fixing this issue for the upcoming hotfix.

}

}
return null;
}
}
}
11 changes: 7 additions & 4 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,7 @@ internal SNIError GetLastError()
private static string GetLocalDBDataSource(string fullServerName, out bool error)
{
string localDBConnectionString = null;
bool isBadLocalDBDataSource;
string localDBInstance = DataSource.GetLocalDBInstance(fullServerName, out isBadLocalDBDataSource);
string localDBInstance = DataSource.GetLocalDBInstance(fullServerName, out bool isBadLocalDBDataSource);

if (isBadLocalDBDataSource)
{
Expand Down Expand Up @@ -381,6 +380,7 @@ internal class DataSource
private const string Slash = @"/";
private const string PipeToken = "pipe";
private const string LocalDbHost = "(localdb)";
private const string LocalDbHost_NP = @"np:\\.\pipe\LOCALDB#";
private const string NamedPipeInstanceNameHeader = "mssql$";
private const string DefaultPipeName = "sql\\query";

Expand Down Expand Up @@ -482,11 +482,9 @@ private void PopulateProtocol()
internal static string GetLocalDBInstance(string dataSource, out bool error)
{
string instanceName = null;

// ReadOnlySpan is not supported in netstandard 2.0, but installing System.Memory solves the issue
ReadOnlySpan<char> input = dataSource.AsSpan().TrimStart();
error = false;

// NetStandard 2.0 does not support passing a string to ReadOnlySpan<char>
if (input.StartsWith(LocalDbHost.AsSpan().Trim(), StringComparison.InvariantCultureIgnoreCase))
{
Expand All @@ -507,6 +505,11 @@ internal static string GetLocalDBInstance(string dataSource, out bool error)
error = true;
}
}
else if (input.StartsWith(LocalDbHost_NP.AsSpan().Trim(), StringComparison.InvariantCultureIgnoreCase))
{
instanceName = input.Trim().ToString();
}

return instanceName;
}

Expand Down
39 changes: 24 additions & 15 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

namespace Microsoft.Data
{

internal static class LocalDBAPI
{
const string const_localDbPrefix = @"(localdb)\";
const string const_partialTrustFlagKey = "ALLOW_LOCALDB_IN_PARTIAL_TRUST";
private const string LocalDbPrefix = @"(localdb)\";
private const string LocalDbPrefix_NP = @"np:\\.\pipe\LOCALDB#";
const string Const_partialTrustFlagKey = "ALLOW_LOCALDB_IN_PARTIAL_TRUST";

static PermissionSet _fullTrust = null;
static bool _partialTrustFlagChecked = false;
Expand All @@ -30,18 +30,27 @@ internal static class LocalDBAPI
// check if name is in format (localdb)\<InstanceName - not empty> and return instance name if it is
internal static string GetLocalDbInstanceNameFromServerName(string serverName)
{
if (serverName == null)
return null;
serverName = serverName.TrimStart(); // it can start with spaces if specified in quotes
if (!serverName.StartsWith(const_localDbPrefix, StringComparison.OrdinalIgnoreCase))
return null;
string instanceName = serverName.Substring(const_localDbPrefix.Length).Trim();
if (instanceName.Length == 0)
return null;
else
return instanceName;
}
if (serverName is not null)
{
// it can start with spaces if specified in quotes
// Memory allocation is reduced by using ReadOnlySpan
ReadOnlySpan<char> input = serverName.AsSpan().Trim();
if (input.StartsWith(LocalDbPrefix.AsSpan(), StringComparison.OrdinalIgnoreCase))
{
input = input.Slice(LocalDbPrefix.Length);
if (!input.IsEmpty)
{
return input.ToString();
}
}
else if (input.StartsWith(LocalDbPrefix_NP.AsSpan(), StringComparison.OrdinalIgnoreCase))
{
return input.ToString();
}

}
return null;
}

internal static void ReleaseDLLHandles()
{
Expand Down Expand Up @@ -261,7 +270,7 @@ internal static void DemandLocalDBPermissions()
{
if (!_partialTrustFlagChecked)
{
object partialTrustFlagValue = AppDomain.CurrentDomain.GetData(const_partialTrustFlagKey);
object partialTrustFlagValue = AppDomain.CurrentDomain.GetData(Const_partialTrustFlagKey);
if (partialTrustFlagValue != null && partialTrustFlagValue is bool)
{
_partialTrustAllowed = (bool)partialTrustFlagValue;
Expand Down
46 changes: 44 additions & 2 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// 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.
using System.Collections.Generic;
using System;
using System.Diagnostics;
using System.Threading;
using Xunit;

namespace Microsoft.Data.SqlClient.ManualTesting.Tests
Expand All @@ -13,14 +15,18 @@ public static class LocalDBTest
private static readonly string s_localDbConnectionString = @$"server=(localdb)\{DataTestUtility.LocalDbAppName}";
private static readonly string[] s_sharedLocalDbInstances = new string[] { @$"server=(localdb)\.\{DataTestUtility.LocalDbSharedInstanceName}", @$"server=(localdb)\." };
private static readonly string s_badConnectionString = $@"server=(localdb)\{DataTestUtility.LocalDbAppName};Database=DOES_NOT_EXIST;Pooling=false;";
private static readonly string s_commandPrompt = "cmd.exe";
private static readonly string s_sqlLocalDbInfo = @$"/c SqlLocalDb info {DataTestUtility.LocalDbAppName}";
private static readonly string s_startLocalDbCommand = @$"/c SqlLocalDb start {DataTestUtility.LocalDbAppName}";
private static readonly string s_localDbNamedPipeConnectionString = @$"server={GetLocalDbNamedPipe()}";
JRahnama marked this conversation as resolved.
Show resolved Hide resolved

static string LocalDbName = DataTestUtility.LocalDbAppName;
#region LocalDbTests
[SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] // No Registry support on UAP
[ConditionalFact(nameof(IsLocalDBEnvironmentSet))]
public static void SqlLocalDbConnectionTest()
{
ConnectionTest(s_localDbConnectionString);
ConnectionTest(s_localDbNamedPipeConnectionString);
}

[SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] // No Registry support on UAP
Expand All @@ -30,13 +36,15 @@ public static void LocalDBEncryptionNotSupportedTest()
// Encryption is not supported by SQL Local DB.
// But connection should succeed as encryption is disabled by driver.
ConnectionWithEncryptionTest(s_localDbConnectionString);
ConnectionWithEncryptionTest(s_localDbNamedPipeConnectionString);
}

[SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] // No Registry support on UAP
[ConditionalFact(nameof(IsLocalDBEnvironmentSet))]
public static void LocalDBMarsTest()
{
ConnectionWithMarsTest(s_localDbConnectionString);
ConnectionWithMarsTest(s_localDbNamedPipeConnectionString);
}

[SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] // No Registry support on UAP
Expand Down Expand Up @@ -123,5 +131,39 @@ private static void OpenConnection(string connString)
var result = command.ExecuteScalar();
Assert.NotNull(result);
}

private static string GetLocalDbNamedPipe()
{
string state = ExecuteLocalDBCommandProcess(s_commandPrompt, s_sqlLocalDbInfo, "state");
while (state.Equals("stopped", StringComparison.InvariantCultureIgnoreCase))
{
state = ExecuteLocalDBCommandProcess(s_commandPrompt, s_startLocalDbCommand, "state");
Thread.Sleep(2000);
}
return ExecuteLocalDBCommandProcess(s_commandPrompt, s_sqlLocalDbInfo, "pipeName");
}

private static string ExecuteLocalDBCommandProcess(string filename, string arguments, string infoType)
{
ProcessStartInfo sInfo = new()
{
FileName = filename,
Arguments = arguments,
UseShellExecute = false,
CreateNoWindow = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
};
string[] lines = Process.Start(sInfo).StandardOutput.ReadToEnd().Split(new string[] { Environment.NewLine }, StringSplitOptions.None);
if (infoType.Equals("state"))
{
return lines[5].Split(':')[1].Trim();
}
else if (infoType.Equals("pipeName"))
{
return lines[7].Split(new string[] { "Instance pipe name:" }, StringSplitOptions.None)[1].Trim();
}
return null;
}
}
}
1 change: 1 addition & 0 deletions ...oft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/config.default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"AzureKeyVaultClientSecret": "",
"SupportsIntegratedSecurity": true,
"LocalDbAppName": "",
"LocalDbSharedInstanceName":"",
"SupportsFileStream": false,
"FileStreamDirectory": "",
"UseManagedSNIOnWindows": false,
Expand Down