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

Jump to bottom

Fix Async thread blocking on SqlConnection open for AAD modes #1213

Merged
merged 7 commits into from
Aug 23, 2021
Merged

Fix Async thread blocking on SqlConnection open for AAD modes #1213

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
48daafe
Fix Async thread blocking on SqlConnection open for AAD modes
cheenamalhotra Aug 16, 2021
9adeb8c
back to result
cheenamalhotra Aug 17, 2021
cf15d35
Fix AAD Tests
cheenamalhotra Aug 17, 2021
4d69e29
More changes
cheenamalhotra Aug 17, 2021
de5c963
Fix for Unix
cheenamalhotra Aug 17, 2021
75471d4
Wrap internal exceptions into SqlException
cheenamalhotra Aug 18, 2021
f25e4f7
Add comments
cheenamalhotra Aug 23, 2021
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
18 changes: 12 additions & 6 deletions ...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2352,7 +2352,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
}
else
{
_fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
_fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken;
}
break;
Expand All @@ -2368,7 +2368,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
else
{
authParamsBuilder.WithUserId(ConnectionOptions.UserID);
_fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
_fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken;
}
break;
Expand All @@ -2384,13 +2384,13 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
{
username = _credential.UserId;
authParamsBuilder.WithUserId(username).WithPassword(_credential.Password);
_fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
_fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
}
else
{
username = ConnectionOptions.UserID;
authParamsBuilder.WithUserId(username).WithPassword(ConnectionOptions.Password);
_fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
_fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
}
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken;
}
Expand Down Expand Up @@ -2444,8 +2444,9 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
|| _timeout.MillisecondsRemaining <= sleepInterval)
{
SqlClientEventSource.Log.TryTraceEvent("<sc.SqlInternalConnectionTds.GetFedAuthToken.MSALException error:> {0}", msalException.ErrorCode);

// Error[0]
SqlErrorCollection sqlErs = new SqlErrorCollection();
SqlErrorCollection sqlErs = new();
sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, ConnectionOptions.DataSource, StringsHelper.GetString(Strings.SQL_MSALFailure, username, ConnectionOptions.Authentication.ToString("G")), ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0));

// Error[1]
Expand All @@ -2467,6 +2468,11 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
Thread.Sleep(sleepInterval);
sleepInterval *= 2;
}
// All other exceptions from MSAL/Azure Identity APIs
catch (Exception e)
{
throw SqlException.CreateException(new() { new(0, (byte)0x00, (byte)TdsEnums.FATAL_ERROR_CLASS, ConnectionOptions.DataSource, e.Message, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0) }, "", this, e);
}
}

Debug.Assert(_fedAuthToken != null, "fedAuthToken should not be null.");
Expand Down Expand Up @@ -2632,7 +2638,7 @@ internal void OnFeatureExtAck(int featureId, byte[] data)
Debug.Assert(_tceVersionSupported <= TdsEnums.MAX_SUPPORTED_TCE_VERSION, "Client support TCE version 2");
_parser.IsColumnEncryptionSupported = true;
_parser.TceVersionSupported = _tceVersionSupported;
_parser.AreEnclaveRetriesSupported = _tceVersionSupported == 3;
_parser.AreEnclaveRetriesSupported = _tceVersionSupported == 3;

if (data.Length > 1)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,7 +1348,7 @@
<value>Failed to authenticate the user {0} in Active Directory (Authentication={1}).</value>
</data>
<data name="SQL_MSALInnerException" xml:space="preserve">
<value>Error code 0x{0}; state {1}</value>
<value>Error code 0x{0}</value>
</data>
<data name="SQL_ParsingErrorWithState" xml:space="preserve">
<value>Internal connection fatal error. Error state: {0}.</value>
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/DbConnectionStringCommon.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ internal static string ColumnEncryptionSettingToString(SqlConnectionColumnEncryp

internal static bool IsValidAuthenticationTypeValue(SqlAuthenticationMethod value)
{
Debug.Assert(Enum.GetNames(typeof(SqlAuthenticationMethod)).Length == 9, "SqlAuthenticationMethod enum has changed, update needed");
Debug.Assert(Enum.GetNames(typeof(SqlAuthenticationMethod)).Length == 10, "SqlAuthenticationMethod enum has changed, update needed");
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved
return value == SqlAuthenticationMethod.SqlPassword
|| value == SqlAuthenticationMethod.ActiveDirectoryPassword
|| value == SqlAuthenticationMethod.ActiveDirectoryIntegrated
Expand Down
13 changes: 9 additions & 4 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2790,7 +2790,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
}
else
{
fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken;
}
break;
Expand All @@ -2806,7 +2806,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
else
{
authParamsBuilder.WithUserId(ConnectionOptions.UserID);
fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken;
}
break;
Expand All @@ -2822,13 +2822,13 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
{
username = _credential.UserId;
authParamsBuilder.WithUserId(username).WithPassword(_credential.Password);
fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
}
else
{
username = ConnectionOptions.UserID;
authParamsBuilder.WithUserId(username).WithPassword(ConnectionOptions.Password);
fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
}
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken;
}
Expand Down Expand Up @@ -2906,6 +2906,11 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
Thread.Sleep(sleepInterval);
sleepInterval *= 2;
}
// All other exceptions from MSAL/Azure Identity APIs
catch (Exception e)
{
throw SqlException.CreateException(new() { new(0, (byte)0x00, (byte)TdsEnums.FATAL_ERROR_CLASS, ConnectionOptions.DataSource, e.Message, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0) }, "", this, e);
}
}

Debug.Assert(fedAuthToken != null, "fedAuthToken should not be null.");
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2563,7 +2563,7 @@
<value>Failed to authenticate the user {0} in Active Directory (Authentication={1}).</value>
</data>
<data name="SQL_MSALInnerException" xml:space="preserve">
<value>Error code 0x{0}; state {1}</value>
<value>Error code 0x{0}</value>
</data>
<data name="SQL_ChangePasswordRequiresYukon" xml:space="preserve">
<value>ChangePassword requires SQL Server 9.0 or later.</value>
Expand Down
23 changes: 23 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,29 @@ public static string GetValueString(object paramValue)
return paramValue.ToString();
}

public static string FetchKeyInConnStr(string connStr, string[] keys)
{
// tokenize connection string and find matching key
if (connStr != null && keys != null)
{
string[] connProps = connStr.Split(';');
foreach (string cp in connProps)
{
if (!string.IsNullOrEmpty(cp.Trim()))
{
foreach (var key in keys)
{
if (cp.Trim().ToLower().StartsWith(key.Trim().ToLower()))
{
return cp.Substring(cp.IndexOf('=') + 1);
}
}
}
}
}
return null;
}

public static string RemoveKeysInConnStr(string connStr, string[] keysToRemove)
{
// tokenize connection string and remove input keys.
Expand Down
30 changes: 22 additions & 8 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,10 @@ public static void AADPasswordWithWrongPassword()
string[] credKeys = { "Password", "PWD" };
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, credKeys) + "Password=TestPassword;";

AggregateException e = Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStr));
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStr));
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved

string expectedMessage = "ID3242: The security token could not be authenticated or authorized.";
Assert.Contains(expectedMessage, e.InnerException.InnerException.Message);
Assert.Contains(expectedMessage, e.Message);
}


Expand Down Expand Up @@ -241,7 +241,11 @@ public static void EmptyPasswordInConnStrAADPassword()
// connection fails with expected error message.
string[] pwdKey = { "Password", "PWD" };
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, pwdKey) + "Password=;";
Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStr));
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStr));

string user = DataTestUtility.FetchKeyInConnStr(DataTestUtility.AADPasswordConnectionString, new string[] { "User Id", "UID" });
string expectedMessage = string.Format("Failed to authenticate the user {0} in Active Directory (Authentication=ActiveDirectoryPassword).", user);
Assert.Contains(expectedMessage, e.Message);
}

[PlatformSpecific(TestPlatforms.Windows)]
Expand All @@ -251,7 +255,10 @@ public static void EmptyCredInConnStrAADPassword()
// connection fails with expected error message.
string[] removeKeys = { "User ID", "Password", "UID", "PWD" };
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, removeKeys) + "User ID=; Password=;";
Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStr));
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStr));

string expectedMessage = "Failed to authenticate the user in Active Directory (Authentication=ActiveDirectoryPassword).";
Assert.Contains(expectedMessage, e.Message);
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
Expand All @@ -261,16 +268,23 @@ public static void EmptyCredInConnStrAADPasswordAnyUnix()
// connection fails with expected error message.
string[] removeKeys = { "User ID", "Password", "UID", "PWD" };
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, removeKeys) + "User ID=; Password=;";
Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStr));
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStr));

string expectedMessage = "MSAL cannot determine the username (UPN) of the currently logged in user.For Integrated Windows Authentication and Username/Password flows, please use .WithUsername() before calling ExecuteAsync().";
Assert.Contains(expectedMessage, e.Message);
}

[ConditionalFact(nameof(IsAADConnStringsSetup))]
public static void AADPasswordWithInvalidUser()
{
// connection fails with expected error message.
string[] removeKeys = { "User ID", "UID" };
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, removeKeys) + "User ID=testdotnet@microsoft.com";
Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStr));
string user = "testdotnet@domain.com";
string connStr = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, removeKeys) + $"User ID={user}";
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStr));

string expectedMessage = string.Format("Failed to authenticate the user {0} in Active Directory (Authentication=ActiveDirectoryPassword).", user);
Assert.Contains(expectedMessage, e.Message);
}

[ConditionalFact(nameof(IsAADConnStringsSetup))]
Expand Down Expand Up @@ -386,7 +400,7 @@ public static void ActiveDirectoryManagedIdentityWithInvalidUserIdMustFail(strin
string connStrWithNoCred = DataTestUtility.RemoveKeysInConnStr(DataTestUtility.AADPasswordConnectionString, credKeys) +
$"Authentication=Active Directory Managed Identity; User Id={userId}";

AggregateException e = Assert.Throws<AggregateException>(() => ConnectAndDisconnect(connStrWithNoCred));
SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStrWithNoCred));

string expectedMessage = "ManagedIdentityCredential authentication unavailable";
Assert.Contains(expectedMessage, e.GetBaseException().Message);
Expand Down