From 48085ae3143aba004f065680344d3299a09b6dea Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 7 Aug 2026 22:45:45 -0700 Subject: [PATCH 1/4] Treat AccessTokenCallback like AccessToken for TNIR and pool keys On .NET Framework the driver disables Transparent Network IP Resolution by default whenever federated authentication is in use, unless the caller explicitly specified the TransparentNetworkIPResolution keyword. However, ShouldDisableTnir only tested _accessTokenInBytes (SqlConnection.AccessToken) and ignored _accessTokenCallback (SqlConnection.AccessTokenCallback), so the two token-supplying APIs behaved differently. Raised in review discussion on #4493. Changes: * Add SqlConnectionInternal.IsAccessTokenProvided, a single source of truth for "the caller supplied a token, either literally or via a callback", and use it in all three places that previously inlined the field checks (ShouldDisableTnir plus two spots in TdsParser.ConsumePreLoginHandshake). The duplicated, hand-written expression is what allowed the two paths to drift apart. * Fix the AccessToken, AccessTokenCallback and SspiContextProvider setters, which each rebuilt the ConnectionPoolKey with the sibling authentication values hard-coded to null. Setting SspiContextProvider silently dropped a previously assigned access token or callback from the pool key, so it never reached the internal connection even though the public property still reported it as set. These now preserve sibling state, matching the ConnectionString setter. (AccessToken and AccessTokenCallback are already mutually exclusive, so that pairing was benign; SspiContextProvider is not.) * Expose ShouldDisableTnir as internal static so it can be unit tested, and add coverage for the TNIR decision matrix and for pool-key preservation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5368f578-219b-40a6-92a9-4742b56edbe6 --- .../Connection/SqlConnectionInternal.cs | 38 +++++++++++- .../Microsoft/Data/SqlClient/SqlConnection.cs | 25 ++++++-- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 4 +- .../SqlClient/SqlConnectionOptionsTest.cs | 38 ++++++++++++ .../SimulatedServerTests/ConnectionTests.cs | 59 +++++++++++++++++++ 5 files changed, 155 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index fd8817b9aa..53a034b3a5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -145,6 +145,19 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable // @TODO: Probably a good idea to introduce a delegate type internal readonly Func> _accessTokenCallback; + /// + /// True when the caller supplied a federated authentication access token directly, either + /// as a literal token via or as a token provider + /// via . + /// + /// + /// Both paths represent the same "caller-supplied token" authentication mode, so they must + /// always be treated identically. Use this property rather than testing the underlying + /// fields individually. + /// + internal bool IsAccessTokenProvided => + _accessTokenInBytes != null || _accessTokenCallback != null; + // @TODO: Should be private and accessed via internal property // @TODO: Rename to match naming conventions internal bool _cleanSQLDNSCaching = false; @@ -3115,7 +3128,9 @@ private void LoginNoFailover( #if NET bool isParallel = connectionOptions.MultiSubnetFailover; #else - bool disableTnir = ShouldDisableTnir(connectionOptions); + bool disableTnir = ShouldDisableTnir( + connectionOptions, + isAccessTokenProvided: IsAccessTokenProvided); bool isParallel = connectionOptions.MultiSubnetFailover || (connectionOptions.TransparentNetworkIPResolution && !disableTnir); #endif @@ -3911,12 +3926,29 @@ private void ResolveExtendedServerName(ServerInfo serverInfo, bool aliasLookup, } #if NETFRAMEWORK - private bool ShouldDisableTnir(SqlConnectionOptions connectionOptions) + /// + /// Determines whether Transparent Network IP Resolution (TNIR) should be disabled for this + /// connection attempt. + /// + /// The parsed connection options. + /// + /// True when the caller supplied a federated authentication access token directly, either + /// via or + /// . + /// + /// + /// True when TNIR should be disabled. TNIR is disabled by default for Azure SQL endpoints + /// and for federated authentication, but an explicit + /// TransparentNetworkIPResolution keyword always takes precedence. + /// + internal static bool ShouldDisableTnir( + SqlConnectionOptions connectionOptions, + bool isAccessTokenProvided) { bool isAzureEndPoint = ADP.IsAzureSqlServerEndpoint(connectionOptions.DataSource); // @TODO: Turn into a HashSet and just check the list instead of this MESS. - bool isFedAuthEnabled = _accessTokenInBytes != null || + bool isFedAuthEnabled = isAccessTokenProvided || #pragma warning disable 0618 // Type or member is obsolete connectionOptions.Authentication == SqlAuthenticationMethod.ActiveDirectoryPassword || #pragma warning restore 0618 // Type or member is obsolete diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index c86b30525a..280898fd7a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -763,8 +763,15 @@ public string AccessToken CheckAndThrowOnInvalidCombinationOfConnectionOptionAndAccessToken(ConnectionOptions); } - // Need to call ConnectionString_Set to do proper pool group check - ConnectionString_Set(new ConnectionPoolKey(_connectionString, credential: _credential, accessToken: value, accessTokenCallback: null, sspiContextProvider: null)); + // Need to call ConnectionString_Set to do proper pool group check. + // Preserve the other authentication state so it isn't dropped from the pool key + // (see the ConnectionString setter, which is the reference for this pattern). + ConnectionString_Set(new ConnectionPoolKey( + _connectionString, + credential: _credential, + accessToken: value, + accessTokenCallback: _accessTokenCallback, + sspiContextProvider: _sspiContextProvider)); _accessToken = value; } } @@ -787,7 +794,12 @@ public Func + /// TNIR is disabled by default whenever federated authentication is in play, including when + /// the token is supplied directly through AccessToken or AccessTokenCallback, + /// unless the user explicitly specified the TNIR keyword. + /// + [Theory] + // Non-Azure endpoint, no explicit TNIR keyword: access token (or callback) disables TNIR. + [InlineData("my.test.server", false, false, false)] + [InlineData("my.test.server", true, false, true)] + // Azure endpoint always disables TNIR when the keyword is absent. + [InlineData("test.database.windows.net", false, false, true)] + [InlineData("test.database.windows.net", true, false, true)] + // An explicit TNIR keyword always wins, regardless of access token or endpoint. + [InlineData("my.test.server", true, true, false)] + [InlineData("test.database.windows.net", true, true, false)] + [InlineData("test.database.windows.net", false, true, false)] + public void TestShouldDisableTnirWithAccessToken( + string dataSource, + bool isAccessTokenProvided, + bool tnirExplicitlySpecified, + bool expectedValue) + { + SqlConnectionStringBuilder builder = new() { DataSource = dataSource }; + if (tnirExplicitlySpecified) + { + builder.TransparentNetworkIPResolution = true; + } + + SqlConnectionOptions connectionOptions = new(builder.ConnectionString); + + Assert.Equal( + expectedValue, + SqlConnectionInternal.ShouldDisableTnir(connectionOptions, isAccessTokenProvided)); + } #endif /// /// Test MSF values when set through connection string and through app context switch. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs index a082c8c7e6..f16844ea83 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs @@ -764,6 +764,65 @@ public void ConnectionTestAccessTokenCallbackCombinations() } } + /// + /// Setting one authentication-related property must not silently drop the others from the + /// connection pool key. Previously, assigning + /// rebuilt the pool key with a null (and a + /// null ), so the token was never handed to the + /// internal connection even though the public property still reported it as set. + /// + [Fact] + public void AccessTokenStateIsPreservedInPoolKeyWhenSspiContextProviderIsSet() + { + Func> callback = + (ctx, token) => Task.FromResult(new SqlAuthenticationToken("invalid", DateTimeOffset.MaxValue)); + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessTokenCallback = callback; + Assert.Same(callback, conn.PoolGroup.PoolKey.AccessTokenCallback); + + conn.SspiContextProvider = null; + + Assert.Same(callback, conn.AccessTokenCallback); + Assert.Same(callback, conn.PoolGroup.PoolKey.AccessTokenCallback); + } + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessToken = "token"; + Assert.Equal("token", conn.PoolGroup.PoolKey.AccessToken); + + conn.SspiContextProvider = null; + + Assert.Equal("token", conn.AccessToken); + Assert.Equal("token", conn.PoolGroup.PoolKey.AccessToken); + } + } + + /// + /// and + /// are mutually exclusive, so neither setter can ever clobber a live value of the other. + /// + [Fact] + public void AccessTokenAndAccessTokenCallbackAreMutuallyExclusive() + { + Func> callback = + (ctx, token) => Task.FromResult(new SqlAuthenticationToken("invalid", DateTimeOffset.MaxValue)); + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessTokenCallback = callback; + Assert.Throws(() => conn.AccessToken = "token"); + } + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessToken = "token"; + Assert.Throws(() => conn.AccessTokenCallback = callback); + } + } + [Theory] [InlineData(9, 0, 2047)] // SQL Server 2005 [InlineData(10, 0, 2531)] // SQL Server 2008 From dbc53bf5693c81186f14e6bc75b71c9128cbf745 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:13:14 -0700 Subject: [PATCH 2/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index d3639a11e2..d8c904f1e0 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -1179,7 +1179,7 @@ private PreLoginHandshakeStatus ConsumePreLoginHandshake( // We must NOT use the response for the FEDAUTHREQUIRED PreLogin option, if the connection string option // was not using the new Authentication keyword or in other words, if Authentication=NotSpecified - // Or AccessToken is not null, mean token based authentication is used. + // Or an access token was supplied (AccessToken/AccessTokenCallback), which means token-based authentication is used. if ((_connHandler.ConnectionOptions != null && _connHandler.ConnectionOptions.Authentication != SqlAuthenticationMethod.NotSpecified) || _connHandler.IsAccessTokenProvided) From 964a428f2bd0870d90b45a83932fbf060a26f7fb Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:20:02 -0700 Subject: [PATCH 3/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../Data/SqlClient/Connection/SqlConnectionInternal.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 53a034b3a5..9abd6936cf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -147,9 +147,8 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable /// /// True when the caller supplied a federated authentication access token directly, either - /// as a literal token via or as a token provider - /// via . - /// + /// as a literal token via or as a token provider + /// via . /// /// Both paths represent the same "caller-supplied token" authentication mode, so they must /// always be treated identically. Use this property rather than testing the underlying From 9c92fda29b213ea965cd8f4e174812f09216eea6 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:25:04 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 9abd6936cf..97bd473568 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -149,6 +149,7 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable /// True when the caller supplied a federated authentication access token directly, either /// as a literal token via or as a token provider /// via . + /// /// /// Both paths represent the same "caller-supplied token" authentication mode, so they must /// always be treated identically. Use this property rather than testing the underlying