From 9ee4bced2601c943fbf26e244fcace833cba5025 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 30 Jun 2020 15:30:35 -0700 Subject: [PATCH 1/5] Fix pooled connection access token expiry --- .../Data/ProviderBase/DbConnectionInternal.cs | 8 ++ .../Data/ProviderBase/DbConnectionPool.cs | 7 ++ .../Data/SqlClient/SqlConnectionFactory.cs | 4 +- .../SqlClient/SqlInternalConnectionTds.cs | 84 +++++++++++-------- .../Data/ProviderBase/DbConnectionInternal.cs | 7 ++ .../Data/ProviderBase/DbConnectionPool.cs | 7 ++ .../SqlClient/SqlInternalConnectionTds.cs | 49 ++++++----- 7 files changed, 109 insertions(+), 57 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index c500da0c23..d7ce59b676 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -203,6 +203,14 @@ virtual protected bool ReadyToPrepareTransaction } } + virtual internal bool IsAccessTokenExpired + { + get + { + return false; + } + } + abstract protected void Activate(Transaction transaction); internal void ActivateConnection(Transaction transaction) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs index 682a42339f..90ada93ff3 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs @@ -1285,6 +1285,13 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj _waitHandles.CreationSemaphore.Release(1); } } + + // Do not use this pooled connection if access token is about to expire soon before we can connect. + if(null != obj && obj.IsAccessTokenExpired) + { + DestroyObject(obj); + obj = null; + } } while (null == obj); } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs index 03fdf79d0b..3c9f2116d2 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs @@ -37,7 +37,6 @@ override protected DbConnectionInternal CreateConnection(DbConnectionOptions opt { SqlConnectionString opt = (SqlConnectionString)options; SqlConnectionPoolKey key = (SqlConnectionPoolKey)poolKey; - SqlInternalConnection result = null; SessionData recoverySessionData = null; SqlConnection sqlOwningConnection = (SqlConnection)owningConnection; @@ -131,8 +130,7 @@ override protected DbConnectionInternal CreateConnection(DbConnectionOptions opt opt = new SqlConnectionString(opt, instanceName, userInstance: false, setEnlistValue: null); poolGroupProviderInfo = null; // null so we do not pass to constructor below... } - result = new SqlInternalConnectionTds(identity, opt, key.Credential, poolGroupProviderInfo, "", null, redirectedUserInstance, userOpt, recoverySessionData, applyTransientFaultHandling: applyTransientFaultHandling, key.AccessToken, pool); - return result; + return new SqlInternalConnectionTds(identity, opt, key.Credential, poolGroupProviderInfo, "", null, redirectedUserInstance, userOpt, recoverySessionData, applyTransientFaultHandling: applyTransientFaultHandling, key.AccessToken, pool); } protected override DbConnectionOptions CreateConnectionOptions(string connectionString, DbConnectionOptions previous) 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 4525d4c75d..7b1ef942e6 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 @@ -123,6 +123,9 @@ sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposa internal bool _federatedAuthenticationAcknowledged; internal bool _federatedAuthenticationInfoRequested; // Keep this distinct from _federatedAuthenticationRequested, since some fedauth library types may not need more info internal bool _federatedAuthenticationInfoReceived; + + // The Federated Authentication returned by TryGetFedAuthTokenLocked or GetFedAuthToken. + SqlFedAuthToken _fedAuthToken = null; internal byte[] _accessTokenInBytes; private readonly ActiveDirectoryAuthenticationTimeoutRetryHelper _activeDirectoryAuthTimeoutRetryHelper; @@ -181,7 +184,7 @@ internal bool IsDNSCachingBeforeRedirectSupported } } - internal SQLDNSInfo pendingSQLDNSObject = null; + internal SQLDNSInfo pendingSQLDNSObject = null; // TCE flags internal byte _tceVersionSupported; @@ -684,6 +687,16 @@ protected override bool UnbindOnTransactionCompletion } } + /// + /// Validates if federated authentication is used, Access Token used by this connection is active for the next 10 minutes. + /// + internal override bool IsAccessTokenExpired + { + get + { + return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddMinutes(10d); + } + } //////////////////////////////////////////////////////////////////////////////////////// // GENERAL METHODS @@ -2091,8 +2104,6 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // We want to refresh the token, if taking the lock on the authentication context is successful. bool attemptRefreshTokenLocked = false; - // The Federated Authentication returned by TryGetFedAuthTokenLocked or GetFedAuthToken. - SqlFedAuthToken fedAuthToken = null; if (_dbConnectionPool != null) { @@ -2127,7 +2138,7 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) } else if (_forceExpiryLocked) { - attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out fedAuthToken); + attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out _fedAuthToken); } #endif @@ -2141,11 +2152,11 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // Call the function which tries to acquire a lock over the authentication context before trying to update. // If the lock could not be obtained, it will return false, without attempting to fetch a new token. - attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out fedAuthToken); + attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out _fedAuthToken); // If TryGetFedAuthTokenLocked returns true, it means lock was obtained and fedAuthToken should not be null. // If there was an exception in retrieving the new token, TryGetFedAuthTokenLocked should have thrown, so we won't be here. - Debug.Assert(!attemptRefreshTokenLocked || fedAuthToken != null, "Either Lock should not have been obtained or fedAuthToken should not be null."); + Debug.Assert(!attemptRefreshTokenLocked || _fedAuthToken != null, "Either Lock should not have been obtained or fedAuthToken should not be null."); Debug.Assert(!attemptRefreshTokenLocked || _newDbConnectionPoolAuthenticationContext != null, "Either Lock should not have been obtained or _newDbConnectionPoolAuthenticationContext should not be null."); // Indicate in EventSource Trace that we are successful with the update. @@ -2162,8 +2173,8 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) if (dbConnectionPoolAuthenticationContext == null || attemptRefreshTokenUnLocked) { // Get the Federated Authentication Token. - fedAuthToken = GetFedAuthToken(fedAuthInfo); - Debug.Assert(fedAuthToken != null, "fedAuthToken should not be null."); + _fedAuthToken = GetFedAuthToken(fedAuthInfo); + Debug.Assert(_fedAuthToken != null, "fedAuthToken should not be null."); if (_dbConnectionPool != null) { @@ -2174,18 +2185,18 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) else if (!attemptRefreshTokenLocked) { Debug.Assert(dbConnectionPoolAuthenticationContext != null, "dbConnectionPoolAuthenticationContext should not be null."); - Debug.Assert(fedAuthToken == null, "fedAuthToken should be null in this case."); + Debug.Assert(_fedAuthToken == null, "fedAuthToken should be null in this case."); Debug.Assert(_newDbConnectionPoolAuthenticationContext == null, "_newDbConnectionPoolAuthenticationContext should be null."); - fedAuthToken = new SqlFedAuthToken(); + _fedAuthToken = new SqlFedAuthToken(); // If the code flow is here, then we are re-using the context from the cache for this connection attempt and not // generating a new access token on this thread. - fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; + _fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; } - Debug.Assert(fedAuthToken != null && fedAuthToken.accessToken != null, "fedAuthToken and fedAuthToken.accessToken cannot be null."); - _parser.SendFedAuthToken(fedAuthToken); + Debug.Assert(_fedAuthToken != null && _fedAuthToken.accessToken != null, "fedAuthToken and fedAuthToken.accessToken cannot be null."); + _parser.SendFedAuthToken(_fedAuthToken); } /// @@ -2260,7 +2271,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) int numberOfAttempts = 0; // Object that will be returned to the caller, containing all required data about the token. - SqlFedAuthToken fedAuthToken = new SqlFedAuthToken(); + _fedAuthToken = new SqlFedAuthToken(); // Username to use in error messages. string username = null; @@ -2300,31 +2311,31 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) if (_activeDirectoryAuthTimeoutRetryHelper.State == ActiveDirectoryAuthenticationTimeoutRetryState.Retrying) { - fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; + _fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; } else { - Task.Run(() => fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); - _activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken; + Task.Run(() => _fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); + _activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken; } break; case SqlAuthenticationMethod.ActiveDirectoryInteractive: if (_activeDirectoryAuthTimeoutRetryHelper.State == ActiveDirectoryAuthenticationTimeoutRetryState.Retrying) { - fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; + _fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; } else { authParamsBuilder.WithUserId(ConnectionOptions.UserID); - Task.Run(() => fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); - _activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken; + Task.Run(() => _fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); + _activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken; } break; case SqlAuthenticationMethod.ActiveDirectoryPassword: case SqlAuthenticationMethod.ActiveDirectoryServicePrincipal: if (_activeDirectoryAuthTimeoutRetryHelper.State == ActiveDirectoryAuthenticationTimeoutRetryState.Retrying) { - fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; + _fedAuthToken = _activeDirectoryAuthTimeoutRetryHelper.CachedToken; } else { @@ -2332,22 +2343,22 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) { username = _credential.UserId; authParamsBuilder.WithUserId(username).WithPassword(_credential.Password); - Task.Run(() => fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); + Task.Run(() => _fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); } else { username = ConnectionOptions.UserID; authParamsBuilder.WithUserId(username).WithPassword(ConnectionOptions.Password); - Task.Run(() => fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); + Task.Run(() => _fedAuthToken = authProvider.AcquireTokenAsync(authParamsBuilder).Result.ToSqlFedAuthToken()).Wait(); } - _activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken; + _activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken; } break; default: throw SQL.UnsupportedAuthenticationSpecified(ConnectionOptions.Authentication); } - Debug.Assert(fedAuthToken.accessToken != null, "AccessToken should not be null."); + Debug.Assert(_fedAuthToken.accessToken != null, "AccessToken should not be null."); #if DEBUG if (_forceMsalRetry) { @@ -2417,28 +2428,29 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo) } } - Debug.Assert(fedAuthToken != null, "fedAuthToken should not be null."); - Debug.Assert(fedAuthToken.accessToken != null && fedAuthToken.accessToken.Length > 0, "fedAuthToken.accessToken should not be null or empty."); + Debug.Assert(_fedAuthToken != null, "fedAuthToken should not be null."); + Debug.Assert(_fedAuthToken.accessToken != null && _fedAuthToken.accessToken.Length > 0, "fedAuthToken.accessToken should not be null or empty."); // Store the newly generated token in _newDbConnectionPoolAuthenticationContext, only if using pooling. if (_dbConnectionPool != null) { - DateTime expirationTime = DateTime.FromFileTimeUtc(fedAuthToken.expirationFileTime); - _newDbConnectionPoolAuthenticationContext = new DbConnectionPoolAuthenticationContext(fedAuthToken.accessToken, expirationTime); + DateTime expirationTime = DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime); + _newDbConnectionPoolAuthenticationContext = new DbConnectionPoolAuthenticationContext(_fedAuthToken.accessToken, expirationTime); } SqlClientEventSource.Log.TraceEvent(" {0}, Finished generating federated authentication token.", ObjectID); - return fedAuthToken; + return _fedAuthToken; } internal void OnFeatureExtAck(int featureId, byte[] data) { if (RoutingInfo != null) { - if (TdsEnums.FEATUREEXT_SQLDNSCACHING != featureId) { + if (TdsEnums.FEATUREEXT_SQLDNSCACHING != featureId) + { return; } } - + switch (featureId) { case TdsEnums.FEATUREEXT_SRECOVERY: @@ -2636,16 +2648,18 @@ internal void OnFeatureExtAck(int featureId, byte[] data) throw SQL.ParsingError(ParsingErrorState.CorruptedTdsStream); } - if (1 == data[0]) { + if (1 == data[0]) + { IsSQLDNSCachingSupported = true; _cleanSQLDNSCaching = false; - + if (RoutingInfo != null) { IsDNSCachingBeforeRedirectSupported = true; } } - else { + else + { // we receive the IsSupported whose value is 0 IsSQLDNSCachingSupported = false; _cleanSQLDNSCaching = true; diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index d7c1409b1f..751344db97 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -381,6 +381,13 @@ public ConnectionState State return _state; } } + virtual internal bool IsAccessTokenExpired + { + get + { + return false; + } + } abstract protected void Activate(SysTx.Transaction transaction); diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs index 9505351d3b..214c080622 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs @@ -1528,6 +1528,13 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj { Marshal.ThrowExceptionForHR(releaseSemaphoreResult); // will only throw if (hresult < 0) } + + // Do not use this pooled connection if access token is about to expire soon before we can connect. + if (null != obj && obj.IsAccessTokenExpired) + { + DestroyObject(obj); + obj = null; + } } while (null == obj); } 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 0da06ba54d..06289809b4 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 @@ -131,13 +131,14 @@ sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposa internal bool _federatedAuthenticationInfoRequested; // Keep this distinct from _federatedAuthenticationRequested, since some fedauth library types may not need more info internal bool _federatedAuthenticationInfoReceived; + // The Federated Authentication returned by TryGetFedAuthTokenLocked or GetFedAuthToken. + SqlFedAuthToken _fedAuthToken = null; internal byte[] _accessTokenInBytes; private readonly ActiveDirectoryAuthenticationTimeoutRetryHelper _activeDirectoryAuthTimeoutRetryHelper; private readonly SqlAuthenticationProviderManager _sqlAuthenticationProviderManager; // Certificate auth calbacks. - // ServerCertificateValidationCallback _serverCallback; ClientCertificateRetrievalCallback _clientCallback; SqlClientOriginalNetworkAddressInfo _originalNetworkAddressInfo; @@ -808,6 +809,16 @@ protected override bool UnbindOnTransactionCompletion } } + /// + /// Validates if federated authentication is used, Access Token used by this connection is active for the next 10 minutes. + /// + internal override bool IsAccessTokenExpired + { + get + { + return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddMinutes(10d); + } + } //////////////////////////////////////////////////////////////////////////////////////// // GENERAL METHODS @@ -2538,9 +2549,6 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // We want to refresh the token, if taking the lock on the authentication context is successful. bool attemptRefreshTokenLocked = false; - // The Federated Authentication returned by TryGetFedAuthTokenLocked or GetFedAuthToken. - SqlFedAuthToken fedAuthToken = null; - if (_dbConnectionPool != null) { Debug.Assert(_dbConnectionPool.AuthenticationContexts != null); @@ -2575,7 +2583,7 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) } else if (_forceExpiryLocked) { - attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out fedAuthToken); + attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out _fedAuthToken); } #endif @@ -2589,11 +2597,11 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // Call the function which tries to acquire a lock over the authentication context before trying to update. // If the lock could not be obtained, it will return false, without attempting to fetch a new token. - attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out fedAuthToken); + attemptRefreshTokenLocked = TryGetFedAuthTokenLocked(fedAuthInfo, dbConnectionPoolAuthenticationContext, out _fedAuthToken); - // If TryGetFedAuthTokenLocked returns true, it means lock was obtained and fedAuthToken should not be null. + // If TryGetFedAuthTokenLocked returns true, it means lock was obtained and _fedAuthToken should not be null. // If there was an exception in retrieving the new token, TryGetFedAuthTokenLocked should have thrown, so we won't be here. - Debug.Assert(!attemptRefreshTokenLocked || fedAuthToken != null, "Either Lock should not have been obtained or fedAuthToken should not be null."); + Debug.Assert(!attemptRefreshTokenLocked || _fedAuthToken != null, "Either Lock should not have been obtained or _fedAuthToken should not be null."); Debug.Assert(!attemptRefreshTokenLocked || _newDbConnectionPoolAuthenticationContext != null, "Either Lock should not have been obtained or _newDbConnectionPoolAuthenticationContext should not be null."); // Indicate in Bid Trace that we are successful with the update. @@ -2610,8 +2618,8 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) if (dbConnectionPoolAuthenticationContext == null || attemptRefreshTokenUnLocked) { // Get the Federated Authentication Token. - fedAuthToken = GetFedAuthToken(fedAuthInfo); - Debug.Assert(fedAuthToken != null, "fedAuthToken should not be null."); + _fedAuthToken = GetFedAuthToken(fedAuthInfo); + Debug.Assert(_fedAuthToken != null, "_fedAuthToken should not be null."); if (_dbConnectionPool != null) { @@ -2622,18 +2630,18 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) else if (!attemptRefreshTokenLocked) { Debug.Assert(dbConnectionPoolAuthenticationContext != null, "dbConnectionPoolAuthenticationContext should not be null."); - Debug.Assert(fedAuthToken == null, "fedAuthToken should be null in this case."); + Debug.Assert(_fedAuthToken == null, "_fedAuthToken should be null in this case."); Debug.Assert(_newDbConnectionPoolAuthenticationContext == null, "_newDbConnectionPoolAuthenticationContext should be null."); - fedAuthToken = new SqlFedAuthToken(); + _fedAuthToken = new SqlFedAuthToken(); // If the code flow is here, then we are re-using the context from the cache for this connection attempt and not // generating a new access token on this thread. - fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; + _fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; } - Debug.Assert(fedAuthToken != null && fedAuthToken.accessToken != null, "fedAuthToken and fedAuthToken.accessToken cannot be null."); - _parser.SendFedAuthToken(fedAuthToken); + Debug.Assert(_fedAuthToken != null && _fedAuthToken.accessToken != null, "_fedAuthToken and _fedAuthToken.accessToken cannot be null."); + _parser.SendFedAuthToken(_fedAuthToken); } /// @@ -2873,7 +2881,8 @@ internal void OnFeatureExtAck(int featureId, byte[] data) { if (_routingInfo != null) { - if (TdsEnums.FEATUREEXT_SQLDNSCACHING != featureId) { + if (TdsEnums.FEATUREEXT_SQLDNSCACHING != featureId) + { return; } } @@ -3101,16 +3110,18 @@ internal void OnFeatureExtAck(int featureId, byte[] data) throw SQL.ParsingError(ParsingErrorState.CorruptedTdsStream); } - if (1 == data[0]) { + if (1 == data[0]) + { IsSQLDNSCachingSupported = true; _cleanSQLDNSCaching = false; - + if (_routingInfo != null) { IsDNSCachingBeforeRedirectSupported = true; } } - else { + else + { // we receive the IsSupported whose value is 0 IsSQLDNSCachingSupported = false; _cleanSQLDNSCaching = true; From f89590041967b63b7eadaaea115a89067402e886 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Mon, 6 Jul 2020 09:34:18 -0700 Subject: [PATCH 2/5] Fix expiration time not cached issue --- .../src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | 1 + .../src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | 1 + 2 files changed, 2 insertions(+) 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 7b1ef942e6..c394228397 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 @@ -2193,6 +2193,7 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // If the code flow is here, then we are re-using the context from the cache for this connection attempt and not // generating a new access token on this thread. _fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; + _fedAuthToken.expirationFileTime = dbConnectionPoolAuthenticationContext.ExpirationTime.ToFileTime(); } Debug.Assert(_fedAuthToken != null && _fedAuthToken.accessToken != null, "fedAuthToken and fedAuthToken.accessToken cannot be null."); 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 06289809b4..39338c0a1d 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 @@ -2638,6 +2638,7 @@ internal void OnFedAuthInfo(SqlFedAuthInfo fedAuthInfo) // If the code flow is here, then we are re-using the context from the cache for this connection attempt and not // generating a new access token on this thread. _fedAuthToken.accessToken = dbConnectionPoolAuthenticationContext.AccessToken; + _fedAuthToken.expirationFileTime = dbConnectionPoolAuthenticationContext.ExpirationTime.ToFileTime(); } Debug.Assert(_fedAuthToken != null && _fedAuthToken.accessToken != null, "_fedAuthToken and _fedAuthToken.accessToken cannot be null."); From 08668644152059b5e9423ecf37ab0afba9cd4e02 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 14 Jul 2020 14:45:08 -0700 Subject: [PATCH 3/5] Update Buffer time to use connection timeout and max out at 600 seconds. --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 3 +++ .../SqlClient/SqlInternalConnectionTds.cs | 20 +++++++++++++---- .../src/Microsoft/Data/Common/AdapterUtil.cs | 2 ++ .../SqlClient/SqlInternalConnectionTds.cs | 22 ++++++++++++++----- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs index 7f5335c7d3..d98dc914d3 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs @@ -29,7 +29,10 @@ internal static partial class ADP internal static Task FalseTask => _falseTask ?? (_falseTask = Task.FromResult(false)); internal const CompareOptions DefaultCompareOptions = CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth | CompareOptions.IgnoreCase; + internal const int DefaultConnectionTimeout = DbConnectionStringDefaults.ConnectTimeout; + internal const int InfiniteConnectionTimeout = 0; // infinite connection timeout identifier in seconds + internal const int MaxBufferAccessTokenExpiry = 600; // max duration for buffer in seconds static private void TraceException(string trace, Exception e) { 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 c394228397..5f6682331b 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 @@ -135,6 +135,18 @@ sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposa private bool _serverSupportsDNSCaching = false; + /// + /// Returns buffer time allowed before access token expiry to continue using the access token. + /// + private int accessTokenExpirationBufferTime + { + get + { + return (ConnectionOptions.ConnectTimeout == ADP.InfiniteConnectionTimeout || ConnectionOptions.ConnectTimeout >= ADP.MaxBufferAccessTokenExpiry) + ? ADP.MaxBufferAccessTokenExpiry : ConnectionOptions.ConnectTimeout; + } + } + /// /// Get or set if SQLDNSCaching is supported by the server. /// @@ -688,13 +700,13 @@ protected override bool UnbindOnTransactionCompletion } /// - /// Validates if federated authentication is used, Access Token used by this connection is active for the next 10 minutes. + /// Validates if federated authentication is used, Access Token used by this connection is active for the value of 'accessTokenExpirationBufferTime'. /// internal override bool IsAccessTokenExpired { get { - return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddMinutes(10d); + return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); } } @@ -1069,10 +1081,10 @@ bool isDelegateControlRequest ThreadHasParserLockForClose = false; _parserLock.Release(); releaseConnectionLock = false; - }, 0); + }, ADP.InfiniteConnectionTimeout); if (reconnectTask != null) { - AsyncHelper.WaitForCompletion(reconnectTask, 0); // there is no specific timeout for BeginTransaction, uses ConnectTimeout + AsyncHelper.WaitForCompletion(reconnectTask, ADP.InfiniteConnectionTimeout); // there is no specific timeout for BeginTransaction, uses ConnectTimeout internalTransaction.ConnectionHasBeenRestored = true; return; } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs index eb97aff1aa..659c4f31be 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs @@ -2180,6 +2180,8 @@ static internal Exception InvalidArgumentValue(string methodName) internal const int DecimalMaxPrecision28 = 28; // there are some cases in Odbc where we need that ... internal const int DefaultCommandTimeout = 30; internal const int DefaultConnectionTimeout = DbConnectionStringDefaults.ConnectTimeout; + internal const int InfiniteConnectionTimeout = 0; // infinite connection timeout identifier in seconds + internal const int MaxBufferAccessTokenExpiry = 600; // max duration for buffer in seconds internal const float FailoverTimeoutStep = 0.08F; // fraction of timeout to use for fast failover connections internal const float FailoverTimeoutStepForTnir = 0.125F; // Fraction of timeout to use in case of Transparent Network IP resolution. internal const int MinimumTimeoutForTnirMs = 500; // The first login attempt in Transparent network IP Resolution 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 39338c0a1d..324056ae5e 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 @@ -119,7 +119,7 @@ sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposa // Connection Resiliency private bool _sessionRecoveryRequested; internal bool _sessionRecoveryAcknowledged; - internal SessionData _currentSessionData; // internal for use from TdsParser only, otehr should use CurrentSessionData property that will fix database and language + internal SessionData _currentSessionData; // internal for use from TdsParser only, other should use CurrentSessionData property that will fix database and language private SessionData _recoverySessionData; // Federated Authentication @@ -147,6 +147,18 @@ sealed internal class SqlInternalConnectionTds : SqlInternalConnection, IDisposa private bool _serverSupportsDNSCaching = false; + /// + /// Returns buffer time allowed before access token expiry to continue using the access token. + /// + private int accessTokenExpirationBufferTime + { + get + { + return (ConnectionOptions.ConnectTimeout == ADP.InfiniteConnectionTimeout || ConnectionOptions.ConnectTimeout >= ADP.MaxBufferAccessTokenExpiry) + ? ADP.MaxBufferAccessTokenExpiry : ConnectionOptions.ConnectTimeout; + } + } + /// /// Get or set if SQLDNSCaching FeatureExtAck is supported by the server. /// @@ -810,13 +822,13 @@ protected override bool UnbindOnTransactionCompletion } /// - /// Validates if federated authentication is used, Access Token used by this connection is active for the next 10 minutes. + /// Validates if federated authentication is used, Access Token used by this connection is active for the value of 'accessTokenExpirationBufferTime'. /// internal override bool IsAccessTokenExpired { get { - return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddMinutes(10d); + return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); } } @@ -1332,10 +1344,10 @@ internal void ExecuteTransactionYukon( ThreadHasParserLockForClose = false; _parserLock.Release(); releaseConnectionLock = false; - }, 0); + }, ADP.InfiniteConnectionTimeout); if (reconnectTask != null) { - AsyncHelper.WaitForCompletion(reconnectTask, 0); // there is no specific timeout for BeginTransaction, uses ConnectTimeout + AsyncHelper.WaitForCompletion(reconnectTask, ADP.InfiniteConnectionTimeout); // there is no specific timeout for BeginTransaction, uses ConnectTimeout internalTransaction.ConnectionHasBeenRestored = true; return; } From 311d85d76aee2c4295bf3a1ef1d72f557cbab122 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 14 Jul 2020 20:59:15 -0700 Subject: [PATCH 4/5] Touch-ups --- .../Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 9 +-------- .../Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | 8 +------- .../Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 8 +------- .../Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | 8 +------- 4 files changed, 4 insertions(+), 29 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index d7ce59b676..601890f73e 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -203,14 +203,7 @@ virtual protected bool ReadyToPrepareTransaction } } - virtual internal bool IsAccessTokenExpired - { - get - { - return false; - } - } - + virtual internal bool IsAccessTokenExpired => false; abstract protected void Activate(Transaction transaction); internal void ActivateConnection(Transaction transaction) 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 5f6682331b..432089972e 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 @@ -702,13 +702,7 @@ protected override bool UnbindOnTransactionCompletion /// /// Validates if federated authentication is used, Access Token used by this connection is active for the value of 'accessTokenExpirationBufferTime'. /// - internal override bool IsAccessTokenExpired - { - get - { - return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); - } - } + internal override bool IsAccessTokenExpired => _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); //////////////////////////////////////////////////////////////////////////////////////// // GENERAL METHODS diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 751344db97..aba0bd2f09 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -381,13 +381,7 @@ public ConnectionState State return _state; } } - virtual internal bool IsAccessTokenExpired - { - get - { - return false; - } - } + virtual internal bool IsAccessTokenExpired => false; abstract protected void Activate(SysTx.Transaction transaction); 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 324056ae5e..0dd2ba82e5 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 @@ -824,13 +824,7 @@ protected override bool UnbindOnTransactionCompletion /// /// Validates if federated authentication is used, Access Token used by this connection is active for the value of 'accessTokenExpirationBufferTime'. /// - internal override bool IsAccessTokenExpired - { - get - { - return _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); - } - } + internal override bool IsAccessTokenExpired => _federatedAuthenticationInfoRequested && DateTime.FromFileTimeUtc(_fedAuthToken.expirationFileTime) < DateTime.UtcNow.AddSeconds(accessTokenExpirationBufferTime); //////////////////////////////////////////////////////////////////////////////////////// // GENERAL METHODS From 2bb851bf0f5c67fcf987649450026e26668e1281 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 15 Jul 2020 11:00:13 -0700 Subject: [PATCH 5/5] Nit changes --- .../src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 3 ++- .../src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 601890f73e..7f959eb1c9 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -203,7 +203,8 @@ virtual protected bool ReadyToPrepareTransaction } } - virtual internal bool IsAccessTokenExpired => false; + internal virtual bool IsAccessTokenExpired => false; + abstract protected void Activate(Transaction transaction); internal void ActivateConnection(Transaction transaction) diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index aba0bd2f09..25ef54f6f7 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -381,7 +381,8 @@ public ConnectionState State return _state; } } - virtual internal bool IsAccessTokenExpired => false; + + internal virtual bool IsAccessTokenExpired => false; abstract protected void Activate(SysTx.Transaction transaction);