Skip to content

Commit

Permalink
Fix Async thread blocking on SqlConnection open for AAD modes (#1213)
Browse files Browse the repository at this point in the history
  • Loading branch information
@cheenamalhotra
cheenamalhotra committed Aug 23, 2021
1 parent 896c4fc commit 5f6478a
Show file tree
Hide file tree
Showing 9 changed files with 75 additions and 23 deletions.
20 changes: 14 additions & 6 deletions ...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2358,7 +2358,9 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
}
else
{
_fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
// We use Task.Run here in all places to execute task synchronously in the same context.
// Fixes block-over-async deadlock possibilities https://github.com/dotnet/SqlClient/issues/1209
_fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken;
}
break;
Expand All @@ -2374,7 +2376,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 @@ -2390,13 +2392,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 @@ -2450,8 +2452,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 @@ -2473,6 +2476,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 @@ -2638,7 +2646,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

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
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
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");
return value == SqlAuthenticationMethod.SqlPassword
|| value == SqlAuthenticationMethod.ActiveDirectoryPassword
|| value == SqlAuthenticationMethod.ActiveDirectoryIntegrated
Expand Down
15 changes: 11 additions & 4 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2796,7 +2796,9 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
}
else
{
fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken();
// We use Task.Run here in all places to execute task synchronously in the same context.
// Fixes block-over-async deadlock possibilities https://github.com/dotnet/SqlClient/issues/1209
fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken;
}
break;
Expand All @@ -2812,7 +2814,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 @@ -2828,13 +2830,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 @@ -2912,6 +2914,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

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,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
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));

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

0 comments on commit 5f6478a

Please sign in to comment.