From 5c470ab2b693593d23d43cd50484fef53d0a0499 Mon Sep 17 00:00:00 2001 From: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com> Date: Tue, 7 Jun 2022 10:10:46 -0700 Subject: [PATCH] Fix | Handle NRE on Azure federated authentication (#1625) # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs # src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx # src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs --- .../SqlClient/SqlInternalConnectionTds.cs | 29 +++++++--------- .../src/Microsoft/Data/SqlClient/SqlUtil.cs | 4 +++ .../netcore/src/Resources/Strings.Designer.cs | 13 +++++-- .../netcore/src/Resources/Strings.resx | 5 ++- .../SqlClient/SqlInternalConnectionTds.cs | 28 ++++++--------- .../src/Microsoft/Data/SqlClient/SqlUtil.cs | 5 +++ .../netfx/src/Resources/Strings.Designer.cs | 11 +++++- .../netfx/src/Resources/Strings.resx | 3 ++ .../src/Microsoft/Data/Common/AdapterUtil.cs | 34 +++++++++++++++++-- 9 files changed, 91 insertions(+), 41 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index 484e2233a9..ae988e34bf 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -103,6 +103,9 @@ public void AssertUnrecoverableStateCountIsCorrect() internal sealed class SqlInternalConnectionTds : SqlInternalConnection, IDisposable { + // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/retry-after#simple-retry-for-errors-with-http-error-codes-500-600 + internal const int MsalHttpRetryStatusCode = 429; + // CONNECTION AND STATE VARIABLES private readonly SqlConnectionPoolGroupProviderInfo _poolGroupProviderInfo; // will only be null when called for ChangePassword, or creating SSE User Instance private TdsParser _parser; @@ -2421,7 +2424,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) // Deal with Msal service exceptions first, retry if 429 received. catch (MsalServiceException serviceException) { - if (429 == serviceException.StatusCode) + if (serviceException.StatusCode == MsalHttpRetryStatusCode) { RetryConditionHeaderValue retryAfter = serviceException.Headers.RetryAfter; if (retryAfter.Delta.HasValue) @@ -2440,9 +2443,15 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) } else { - break; + SqlClientEventSource.Log.TryTraceEvent(" Timeout: {0}", serviceException.ErrorCode); + throw SQL.ActiveDirectoryTokenRetrievingTimeout(Enum.GetName(typeof(SqlAuthenticationMethod), ConnectionOptions.Authentication), serviceException.ErrorCode, serviceException); } } + else + { + SqlClientEventSource.Log.TryTraceEvent(" {0}", serviceException.ErrorCode); + throw ADP.CreateSqlException(serviceException, ConnectionOptions, this, username); + } } // Deal with normal MsalExceptions. catch (MsalException msalException) @@ -2453,21 +2462,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) { SqlClientEventSource.Log.TryTraceEvent(" {0}", msalException.ErrorCode); - // Error[0] - 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] - string errorMessage1 = StringsHelper.GetString(Strings.SQL_MSALInnerException, msalException.ErrorCode); - sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, ConnectionOptions.DataSource, errorMessage1, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); - - // Error[2] - if (!string.IsNullOrEmpty(msalException.Message)) - { - sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, ConnectionOptions.DataSource, msalException.Message, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); - } - SqlException exc = SqlException.CreateException(sqlErs, "", this); - throw exc; + throw ADP.CreateSqlException(msalException, ConnectionOptions, this, username); } SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0}, sleeping {1}[Milliseconds]", ObjectID, sleepInterval); diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs index 522092b988..bd6d6bbd6c 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs @@ -510,6 +510,10 @@ internal static Exception ActiveDirectoryDeviceFlowTimeout() return ADP.TimeoutException(Strings.SQL_Timeout_Active_Directory_DeviceFlow_Authentication); } + internal static Exception ActiveDirectoryTokenRetrievingTimeout(string authenticaton, string errorCode, Exception exception) + { + return ADP.TimeoutException(StringsHelper.GetString(Strings.AAD_Token_Retrieving_Timeout, authenticaton, errorCode, exception?.Message), exception); + } // // SQL.DataCommand diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs b/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs index f33751fb08..9f3d2fec91 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.Designer.cs @@ -19,7 +19,7 @@ namespace System { // class via a tool like ResGen or Visual Studio. // To add or remove a member, edit your .ResX file then rerun ResGen // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")] + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "17.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class Strings { @@ -60,6 +60,15 @@ internal Strings() { } } + /// + /// Looks up a localized string similar to Connection timed out while retrieving an access token using '{0}' authentication method. Last error: {1}: {2}. + /// + internal static string AAD_Token_Retrieving_Timeout { + get { + return ResourceManager.GetString("AAD_Token_Retrieving_Timeout", resourceCulture); + } + } + /// /// Looks up a localized string similar to Specified parameter name '{0}' is not valid.. /// @@ -941,7 +950,7 @@ internal static string Data_InvalidOffsetLength { return ResourceManager.GetString("Data_InvalidOffsetLength", resourceCulture); } } - + /// /// Looks up a localized string similar to Internal error occurred when retrying the download of the HGS root certificate after the initial request failed. Contact Customer Support Services.. /// diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx b/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx index bfe5559389..433c6a7684 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx +++ b/src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx @@ -1932,4 +1932,7 @@ Parameter '{0}' cannot have Direction Output or InputOutput when EnableOptimizedParameterBinding is enabled on the parent command. - \ No newline at end of file + + Connection timed out while retrieving an access token using '{0}' authentication method. Last error: {1}: {2} + + diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index 623ce80d93..520c624714 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -105,6 +105,8 @@ public void AssertUnrecoverableStateCountIsCorrect() sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposable { + // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/retry-after#simple-retry-for-errors-with-http-error-codes-500-600 + internal const int MsalHttpRetryStatusCode = 429; // Connection re-route limit internal const int _maxNumberOfRedirectRoute = 10; @@ -2859,7 +2861,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) // Deal with Msal service exceptions first, retry if 429 received. catch (MsalServiceException serviceException) { - if (429 == serviceException.StatusCode) + if (serviceException.StatusCode == MsalHttpRetryStatusCode) { RetryConditionHeaderValue retryAfter = serviceException.Headers.RetryAfter; if (retryAfter.Delta.HasValue) @@ -2878,9 +2880,15 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) } else { - break; + SqlClientEventSource.Log.TryTraceEvent(" Timeout: {0}", serviceException.ErrorCode); + throw SQL.ActiveDirectoryTokenRetrievingTimeout(Enum.GetName(typeof(SqlAuthenticationMethod), ConnectionOptions.Authentication), serviceException.ErrorCode, serviceException); } } + else + { + SqlClientEventSource.Log.TryTraceEvent(" {0}", serviceException.ErrorCode); + throw ADP.CreateSqlException(serviceException, ConnectionOptions, this, username); + } } // Deal with normal MsalExceptions. catch (MsalException msalException) @@ -2891,21 +2899,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) { SqlClientEventSource.Log.TryTraceEvent(" {0}", msalException.ErrorCode); - // Error[0] - SqlErrorCollection sqlErs = new SqlErrorCollection(); - 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] - string errorMessage1 = StringsHelper.GetString(Strings.SQL_MSALInnerException, msalException.ErrorCode); - sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, ConnectionOptions.DataSource, errorMessage1, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); - - // Error[2] - if (!string.IsNullOrEmpty(msalException.Message)) - { - sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, ConnectionOptions.DataSource, msalException.Message, ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); - } - SqlException exc = SqlException.CreateException(sqlErs, "", this); - throw exc; + throw ADP.CreateSqlException(msalException, ConnectionOptions, this, username); } SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0}, sleeping {1}[Milliseconds]", ObjectID, sleepInterval); diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs index 3bc5591b3e..ab1d3fe1f7 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs @@ -654,6 +654,11 @@ static internal Exception ActiveDirectoryDeviceFlowTimeout() return ADP.TimeoutException(Strings.SQL_Timeout_Active_Directory_DeviceFlow_Authentication); } + internal static Exception ActiveDirectoryTokenRetrievingTimeout(string authenticaton, string errorCode, Exception exception) + { + return ADP.TimeoutException(StringsHelper.GetString(Strings.AAD_Token_Retrieving_Timeout, authenticaton, errorCode, exception?.Message), exception); + } + // // SQL.DataCommand // diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.Designer.cs b/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.Designer.cs index bfaf7407be..f7fc54bd04 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.Designer.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.Designer.cs @@ -19,7 +19,7 @@ namespace System { // class via a tool like ResGen or Visual Studio. // To add or remove a member, edit your .ResX file then rerun ResGen // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")] + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "17.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class Strings { @@ -60,6 +60,15 @@ internal Strings() { } } + /// + /// Looks up a localized string similar to Connection timed out while retrieving an access token using '{0}' authentication method. Last error: {1}: {2}. + /// + internal static string AAD_Token_Retrieving_Timeout { + get { + return ResourceManager.GetString("AAD_Token_Retrieving_Timeout", resourceCulture); + } + } + /// /// Looks up a localized string similar to Data adapter mapping error.. /// diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.resx b/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.resx index acf6a6beff..ccc7fa15c3 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.resx +++ b/src/Microsoft.Data.SqlClient/netfx/src/Resources/Strings.resx @@ -4617,4 +4617,7 @@ Parameter '{0}' cannot have Direction Output or InputOutput when EnableOptimizedParameterBinding is enabled on the parent command. + + Connection timed out while retrieving an access token using '{0}' authentication method. Last error: {1}: {2} + diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs index f7e3715ccc..cb8c4cdecf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -24,6 +24,7 @@ using Microsoft.Data.SqlClient.Server; using Microsoft.Win32; using IsolationLevel = System.Data.IsolationLevel; +using Microsoft.Identity.Client; namespace Microsoft.Data.Common { @@ -188,9 +189,9 @@ internal static OverflowException Overflow(string error, Exception inner) return e; } - internal static TimeoutException TimeoutException(string error) + internal static TimeoutException TimeoutException(string error, Exception inner = null) { - TimeoutException e = new(error); + TimeoutException e = new(error, inner); TraceExceptionAsReturnValue(e); return e; } @@ -390,7 +391,34 @@ internal static ArgumentException InvalidArgumentLength(string argumentName, int => Argument(StringsHelper.GetString(Strings.ADP_InvalidArgumentLength, argumentName, limit)); internal static ArgumentException MustBeReadOnly(string argumentName) => Argument(StringsHelper.GetString(Strings.ADP_MustBeReadOnly, argumentName)); - #endregion + + internal static Exception CreateSqlException(MsalException msalException, SqlConnectionString connectionOptions, SqlInternalConnectionTds sender, string username) + { + // Error[0] + 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] + string errorMessage1 = StringsHelper.GetString(Strings.SQL_MSALInnerException, msalException.ErrorCode); + sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, + connectionOptions.DataSource, errorMessage1, + ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); + + // Error[2] + if (!string.IsNullOrEmpty(msalException.Message)) + { + sqlErs.Add(new SqlError(0, (byte)0x00, (byte)TdsEnums.MIN_ERROR_CLASS, + connectionOptions.DataSource, msalException.Message, + ActiveDirectoryAuthentication.MSALGetAccessTokenFunctionName, 0)); + } + return SqlException.CreateException(sqlErs, "", sender); + } + +#endregion #region CommandBuilder, Command, BulkCopy ///