diff --git a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml index 55b60b6120..fbe0f653fa 100644 --- a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml +++ b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml @@ -263,7 +263,13 @@ The following example creates a and a This property is mutually exclusive with the - property, among others. + and + + properties, among others. Setting this property when + + is already set throws + , because SSPI is an + alternative to token-based authentication. @@ -361,7 +367,13 @@ The following example creates a and a This property is mutually exclusive with the - property, among others. + and + + properties, among others. Setting this property when + + is already set throws + , because SSPI is an + alternative to token-based authentication. @@ -2221,10 +2233,27 @@ The following sample tries to open a connection to an invalid database to simula An instance. + + The + is set while the + or + + property is already set, or the connection is open. + The SspiContextProvider is a part of the connection pool key. Care should be taken when using this property to ensure the implementation returns a stable identity per resource. + + SSPI is an alternative to token-based authentication, so this property + is mutually exclusive with the + and + + properties. Setting this property when either of those is already set + throws , and setting + either of those while this property is set throws as well. Assigning + clears the property and never throws. + 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 b4a7eb54df..fe3b66ecfd 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -1487,6 +1487,12 @@ internal static Exception InvalidMixedUsageOfAccessTokenCallbackAndAuthenticatio internal static Exception InvalidMixedUsageOfAccessTokenCallbackAndIntegratedSecurity() => InvalidOperation(StringsHelper.GetString(Strings.ADP_InvalidMixedUsageOfAccessTokenCallbackAndIntegratedSecurity)); + + internal static Exception InvalidMixedUsageOfAccessTokenAndSspiContextProvider() + => InvalidOperation(StringsHelper.GetString(Strings.ADP_InvalidMixedUsageOfAccessTokenAndSspiContextProvider)); + + internal static Exception InvalidMixedUsageOfSspiContextProviderAndAccessToken() + => InvalidOperation(StringsHelper.GetString(Strings.ADP_InvalidMixedUsageOfSspiContextProviderAndAccessToken)); #endregion internal static readonly IntPtr s_ptrZero = IntPtr.Zero; 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..2ce129fdff 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,42 @@ 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 . + /// + /// + /// + /// Use this property wherever the only question is whether the caller supplied a token at + /// all, and the two paths are equivalent: the prelogin FEDAUTHREQUIRED handshake, server + /// certificate validation, and Transparent Network IP Resolution. Those all key off the + /// authentication mode, so treating the two paths differently would be a bug. + /// + /// + /// Do not use this property where the two paths diverge. Login feature-extension + /// negotiation must keep testing the fields individually because they select different + /// federated authentication library types: _accessTokenCallback requests + /// , while _accessTokenInBytes requests + /// and carries the token bytes. + /// + /// + internal bool IsAccessTokenProvided => + _accessTokenInBytes != null || _accessTokenCallback != null; + + #if NETFRAMEWORK + /// + /// The Transparent Network IP Resolution decision applied by the most recent + /// call, or if login has not run. + /// + /// + /// Exposed for tests. is pure and can be tested directly, + /// but that leaves the wiring, in particular that is + /// the value fed into it, unverified. This records the decision that was actually used. + /// + internal bool? TnirDisabledDuringLogin { get; private set; } + #endif + // @TODO: Should be private and accessed via internal property // @TODO: Rename to match naming conventions internal bool _cleanSQLDNSCaching = false; @@ -3115,7 +3151,10 @@ private void LoginNoFailover( #if NET bool isParallel = connectionOptions.MultiSubnetFailover; #else - bool disableTnir = ShouldDisableTnir(connectionOptions); + bool disableTnir = ShouldDisableTnir( + connectionOptions, + isAccessTokenProvided: IsAccessTokenProvided); + TnirDisabledDuringLogin = disableTnir; bool isParallel = connectionOptions.MultiSubnetFailover || (connectionOptions.TransparentNetworkIPResolution && !disableTnir); #endif @@ -3911,12 +3950,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..b8b693132e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -262,6 +262,13 @@ private SqlConnection(SqlConnection connection) _accessToken = connection._accessToken; _accessTokenCallback = connection._accessTokenCallback; + + // CopyFrom retains the source PoolGroup, and therefore the source ConnectionPoolKey. + // The provider must be copied along with it, otherwise the clone would authenticate + // with a provider that its public property does not report, and would let the caller + // set AccessToken/AccessTokenCallback without tripping the mutual-exclusivity checks. + _sspiContextProvider = connection._sspiContextProvider; + CacheConnectionStringProperties(); } @@ -763,8 +770,16 @@ 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. + // AccessTokenCallback and SspiContextProvider are mutually exclusive with AccessToken + // (validated above), so they are always null here when a token is supplied. Passing the + // current values through matters only when the token is being cleared. + ConnectionString_Set(new ConnectionPoolKey( + _connectionString, + credential: _credential, + accessToken: value, + accessTokenCallback: _accessTokenCallback, + sspiContextProvider: _sspiContextProvider)); _accessToken = value; } } @@ -787,7 +802,12 @@ public Func 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 8a4186e299..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,10 +1179,10 @@ 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._accessTokenInBytes != null || _connHandler._accessTokenCallback != null) + || _connHandler.IsAccessTokenProvided) { fedAuthRequired = payload[payloadOffset] == 0x01 ? true : false; } @@ -1219,7 +1219,7 @@ private PreLoginHandshakeStatus ConsumePreLoginHandshake( // Validate Certificate if Trust Server Certificate=false and Encryption forced (EncryptionOptions.ON) from Server. bool shouldValidateServerCert = (_encryptionOption == EncryptionOptions.ON && !trustServerCert) || - ((_connHandler._accessTokenInBytes != null || _connHandler._accessTokenCallback != null) && !trustServerCert); + (_connHandler.IsAccessTokenProvided && !trustServerCert); uint info = (shouldValidateServerCert ? TdsEnums.SNI_SSL_VALIDATE_CERTIFICATE : 0) | TdsEnums.SNI_SSL_USE_SCHANNEL_CACHE; diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs index 75ee7d7b82..f15a43158c 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs @@ -483,6 +483,24 @@ internal static string ADP_InvalidMixedUsageOfAccessTokenCallbackAndIntegratedSe } } + /// + /// Looks up a localized string similar to Cannot set the AccessToken or AccessTokenCallback property if the SspiContextProvider property has been set.. + /// + internal static string ADP_InvalidMixedUsageOfAccessTokenAndSspiContextProvider { + get { + return ResourceManager.GetString("ADP_InvalidMixedUsageOfAccessTokenAndSspiContextProvider", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Cannot set the SspiContextProvider property if the AccessToken or AccessTokenCallback property has been set.. + /// + internal static string ADP_InvalidMixedUsageOfSspiContextProviderAndAccessToken { + get { + return ResourceManager.GetString("ADP_InvalidMixedUsageOfSspiContextProviderAndAccessToken", resourceCulture); + } + } + /// /// Looks up a localized string similar to Cannot set the AccessTokenCallback property if 'Authentication=Active Directory Default' has been specified in the connection string.. /// diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx index 4d65d00a71..dbecc55f11 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx @@ -2079,6 +2079,12 @@ Cannot set the AccessTokenCallback property if the 'Integrated Security' connection string keyword has been set to 'true' or 'SSPI'. + + Cannot set the AccessToken or AccessTokenCallback property if the SspiContextProvider property has been set. + + + Cannot set the SspiContextProvider property if the AccessToken or AccessTokenCallback property has been set. + Cannot set the AccessTokenCallback property if 'Authentication=Active Directory Default' has been specified in the connection string. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs index 8b0dddae45..11b4ed8637 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs @@ -5,6 +5,9 @@ using System; using Microsoft.Data.SqlClient.Tests.Common; using Xunit; +#if NETFRAMEWORK +using SqlConnectionInternal = global::Microsoft.Data.SqlClient.Connection.SqlConnectionInternal; +#endif namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient { @@ -63,6 +66,48 @@ public void TestDefaultTnir(string dataSource, bool? tnirEnabledInConnString, bo // Assert Assert.Equal(expectedValue, connectionString.TransparentNetworkIPResolution); } + + /// + /// 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, no token: TNIR stays enabled. + [InlineData("my.test.server", false, null, false)] + // Non-Azure endpoint, no explicit TNIR keyword, token supplied: TNIR is disabled. + [InlineData("my.test.server", true, null, true)] + // Azure endpoint always disables TNIR when the keyword is absent. + [InlineData("test.database.windows.net", false, null, true)] + [InlineData("test.database.windows.net", true, null, true)] + // An explicit TNIR keyword always wins, regardless of access token or endpoint. Note that + // this method only decides whether the default is overridden; an explicit false is honoured + // by the caller through SqlConnectionOptions.TransparentNetworkIPResolution itself. + [InlineData("my.test.server", true, true, false)] + [InlineData("test.database.windows.net", true, true, false)] + [InlineData("test.database.windows.net", false, true, false)] + [InlineData("my.test.server", true, false, false)] + [InlineData("my.test.server", false, false, false)] + [InlineData("test.database.windows.net", true, false, false)] + [InlineData("test.database.windows.net", false, false, false)] + public void TestShouldDisableTnirWithCallerSuppliedToken( + string dataSource, + bool isAccessTokenProvided, + bool? tnirInConnString, + bool expectedValue) + { + SqlConnectionStringBuilder builder = new() { DataSource = dataSource }; + if (tnirInConnString.HasValue) + { + builder.TransparentNetworkIPResolution = tnirInConnString.Value; + } + + 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..f797797758 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Buffers; using System.Data; using System.Data.Common; using System.Diagnostics; @@ -764,6 +765,241 @@ public void ConnectionTestAccessTokenCallbackCombinations() } } + private static Func> CreateStubCallback() => + (ctx, token) => Task.FromResult(new SqlAuthenticationToken("invalid", DateTimeOffset.MaxValue)); + + private static async Task OpenConnection(SqlConnection connection, bool openAsync) + { + if (openAsync) + { + await connection.OpenAsync(); + } + else + { + connection.Open(); + } + } + + /// + /// When the server signals FEDAUTHREQUIRED in its pre-login response, a caller-supplied + /// token must cause the client to honour that response and echo it back in the Login7 + /// federated authentication feature extension. The simulated server rejects a mismatched + /// echo, so this fails if SqlConnectionInternal.IsAccessTokenProvided stops + /// accounting for . + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task AccessTokenCallbackHonorsPreLoginFedAuthRequired(bool openAsync) + { + using TdsServer server = new(new TdsServerArguments() + { + FedAuthRequiredPreLoginOption = TdsPreLoginFedAuthRequiredOption.FedAuthRequired, + }); + server.Start(); + + string connectionString = new SqlConnectionStringBuilder() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false, + }.ConnectionString; + + using SqlConnection connection = new(connectionString) + { + AccessTokenCallback = CreateStubCallback(), + }; + + await OpenConnection(connection, openAsync); + + Assert.Equal(ConnectionState.Open, connection.State); + +#if NETFRAMEWORK + // Transparent Network IP Resolution is disabled by default whenever the caller supplies + // a token. Asserting on the decision LoginNoFailover actually applied covers the wiring + // that a direct test of ShouldDisableTnir cannot. + Assert.True(GetTnirDisabledDuringLogin(connection)); + + using SqlConnection baseline = new(connectionString); + await OpenConnection(baseline, openAsync); + Assert.False(GetTnirDisabledDuringLogin(baseline)); + + static bool? GetTnirDisabledDuringLogin(SqlConnection connection) => + ((global::Microsoft.Data.SqlClient.Connection.SqlConnectionInternal)connection.InnerConnection) + .TnirDisabledDuringLogin; +#endif + } + + /// + /// Minimal concrete so tests can assign a non-null value. + /// Never used to authenticate, so is not exercised. + /// + private sealed class TestSspiContextProvider : SspiContextProvider + { + protected override bool GenerateContext( + ReadOnlySpan incomingBlob, + IBufferWriter outgoingBlobWriter, + SspiAuthenticationParameters authParams) + => throw new NotSupportedException(); + } + + /// + /// retains the source connection's pool group, and therefore + /// its pool key. It must copy too, otherwise + /// the clone reports no provider while its pool key still carries one, and accepts an access + /// token that the mutual-exclusivity validation would have rejected. + /// + [Fact] + public void CloneCopiesSspiContextProvider() + { + using SqlConnection source = new("Data Source=localhost"); + SspiContextProvider provider = new TestSspiContextProvider(); + source.SspiContextProvider = provider; + + using SqlConnection clone = (SqlConnection)((ICloneable)source).Clone(); + + Assert.Same(provider, clone.SspiContextProvider); + Assert.Same(provider, clone.PoolGroup.PoolKey.SspiContextProvider); + Assert.Throws(() => clone.AccessToken = "token"); + } + + /// + /// The setter also rebuilds the pool key, so it must + /// preserve rather than dropping it and + /// leaving the property and the pool key disagreeing. + /// + [Fact] + public void CredentialSetterPreservesSspiContextProviderInPoolKey() + { + SecureString password = new(); + password.MakeReadOnly(); + + using SqlConnection conn = new("Data Source=localhost"); + SspiContextProvider provider = new TestSspiContextProvider(); + conn.SspiContextProvider = provider; + + conn.Credential = new SqlCredential("user", password); + + Assert.Same(provider, conn.SspiContextProvider); + Assert.Same(provider, conn.PoolGroup.PoolKey.SspiContextProvider); + } + + /// + /// SSPI is an alternative to token-based authentication, so + /// is mutually exclusive with both and + /// , in either assignment order. + /// + [Fact] + public void SspiContextProviderAndAccessTokenStateAreMutuallyExclusive() + { + Func> callback = + (ctx, token) => Task.FromResult(new SqlAuthenticationToken("invalid", DateTimeOffset.MaxValue)); + + // Token first, then provider. + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessToken = "token"; + Assert.Throws( + () => conn.SspiContextProvider = new TestSspiContextProvider()); + } + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessTokenCallback = callback; + Assert.Throws( + () => conn.SspiContextProvider = new TestSspiContextProvider()); + } + + // Provider first, then token. + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.SspiContextProvider = new TestSspiContextProvider(); + Assert.Throws(() => conn.AccessToken = "token"); + } + + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.SspiContextProvider = new TestSspiContextProvider(); + Assert.Throws(() => conn.AccessTokenCallback = callback); + } + } + + /// + /// Clearing one authentication property must not drop the others from the connection pool key. + /// The setters rebuild the key on every assignment, and previously hard-coded the sibling + /// values to null, so clearing one property silently discarded another that was still set. + /// + [Fact] + public void ClearingOneAuthPropertyPreservesTheOthersInPoolKey() + { + Func> callback = + (ctx, token) => Task.FromResult(new SqlAuthenticationToken("invalid", DateTimeOffset.MaxValue)); + + // Clearing the provider must not discard an access token. + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessToken = "token"; + conn.SspiContextProvider = null; + + Assert.Equal("token", conn.AccessToken); + Assert.Equal("token", conn.PoolGroup.PoolKey.AccessToken); + } + + // Clearing the provider must not discard an access token callback. + using (SqlConnection conn = new("Data Source=localhost")) + { + conn.AccessTokenCallback = callback; + conn.SspiContextProvider = null; + + Assert.Same(callback, conn.AccessTokenCallback); + Assert.Same(callback, conn.PoolGroup.PoolKey.AccessTokenCallback); + } + + // Clearing token state must not discard a context provider. + using (SqlConnection conn = new("Data Source=localhost")) + { + SspiContextProvider provider = new TestSspiContextProvider(); + conn.SspiContextProvider = provider; + conn.AccessToken = null; + + Assert.Same(provider, conn.SspiContextProvider); + Assert.Same(provider, conn.PoolGroup.PoolKey.SspiContextProvider); + } + + using (SqlConnection conn = new("Data Source=localhost")) + { + SspiContextProvider provider = new TestSspiContextProvider(); + conn.SspiContextProvider = provider; + conn.AccessTokenCallback = null; + + Assert.Same(provider, conn.SspiContextProvider); + Assert.Same(provider, conn.PoolGroup.PoolKey.SspiContextProvider); + } + } + + /// + /// 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