Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,6 @@ internal bool IsDNSCachingBeforeRedirectSupported
// Json Support Flag
internal bool IsJsonSupportEnabled = false;

// User Agent Flag
internal bool IsUserAgentEnabled = true;

// Vector Support Flag
internal bool IsVectorSupportEnabled = false;

Expand Down Expand Up @@ -1417,10 +1414,7 @@ private void Login(ServerInfo server, TimeoutTimer timeout, string newPassword,
requestedFeatures |= TdsEnums.FeatureExtension.SQLDNSCaching;
requestedFeatures |= TdsEnums.FeatureExtension.JsonSupport;
requestedFeatures |= TdsEnums.FeatureExtension.VectorSupport;

#if DEBUG
requestedFeatures |= TdsEnums.FeatureExtension.UserAgent;
#endif

_parser.TdsLogin(login, requestedFeatures, _recoverySessionData, _fedAuthFeatureExtensionData, encrypt);
}
Expand Down Expand Up @@ -3000,6 +2994,12 @@ internal void OnFeatureExtAck(int featureId, byte[] data)
IsVectorSupportEnabled = true;
break;
}
case TdsEnums.FEATUREEXT_USERAGENT:
{
// Unexpected ack from server but we ignore it entirely
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.OnFeatureExtAck|ADV> {0}, Received feature extension acknowledgement for USERAGENTSUPPORT (ignored)", ObjectID);
break;
}

default:
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
using Microsoft.Data.SqlClient.DataClassification;
using Microsoft.Data.SqlClient.LocalDb;
using Microsoft.Data.SqlClient.Server;
using Microsoft.Data.SqlClient.UserAgent;

#if NETFRAMEWORK
using Microsoft.Data.SqlTypes;
#endif
Expand Down Expand Up @@ -9053,7 +9055,15 @@ private void WriteLoginData(SqlLogin rec,
}
}

ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length, true);
ApplyFeatureExData(
requestedFeatures,
recoverySessionData,
fedAuthFeatureExtensionData,
UserAgentInfo.UserAgentCachedJsonPayload.ToArray(),
useFeatureExt,
length,
true
);
}
catch (Exception e)
{
Expand All @@ -9072,6 +9082,7 @@ private void WriteLoginData(SqlLogin rec,
private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures,
SessionData recoverySessionData,
FederatedAuthenticationFeatureExtensionData fedAuthFeatureExtensionData,
byte[] userAgentJsonPayload,
bool useFeatureExt,
int length,
bool write = false)
Expand Down Expand Up @@ -9124,6 +9135,11 @@ private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures,
length += WriteVectorSupportFeatureRequest(write);
}

if ((requestedFeatures & TdsEnums.FeatureExtension.UserAgent) != 0)
{
length += WriteUserAgentFeatureRequest(userAgentJsonPayload, write);
}

length++; // for terminator
if (write)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,6 @@ internal bool IsDNSCachingBeforeRedirectSupported
// Vector Support Flag
internal bool IsVectorSupportEnabled = false;

// User Agent Flag
internal bool IsUserAgentEnabled = true;

// TCE flags
internal byte _tceVersionSupported;

Expand Down Expand Up @@ -3038,7 +3035,18 @@ internal void OnFeatureExtAck(int featureId, byte[] data)
IsVectorSupportEnabled = true;
break;
}
case TdsEnums.FEATUREEXT_USERAGENT:
{
// TODO: Verify that the server sends an acknowledgment (Ack)
// using this log message in the future.

// This Ack from the server is unexpected and is ignored completely.
// According to the TDS specification, an Ack is not defined/expected
// for this scenario. We handle it only for completeness
// and to support testing.
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.OnFeatureExtAck|ADV> {0}, Received feature extension acknowledgement for USERAGENTSUPPORT (ignored)", ObjectID);
break;
}
default:
{
// Unknown feature ack
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
using Microsoft.Data.SqlClient.DataClassification;
using Microsoft.Data.SqlClient.LocalDb;
using Microsoft.Data.SqlClient.Server;
using Microsoft.Data.SqlClient.UserAgent;

#if NETFRAMEWORK
using Microsoft.Data.SqlTypes;
#endif
Expand Down Expand Up @@ -9119,7 +9121,15 @@ private void WriteLoginData(SqlLogin rec,
}
}

ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length, true);
ApplyFeatureExData(
requestedFeatures,
recoverySessionData,
fedAuthFeatureExtensionData,
UserAgentInfo.UserAgentCachedJsonPayload.ToArray(),
useFeatureExt,
length,
true
);
}
catch (Exception e)
{
Expand All @@ -9138,6 +9148,7 @@ private void WriteLoginData(SqlLogin rec,
private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures,
SessionData recoverySessionData,
FederatedAuthenticationFeatureExtensionData fedAuthFeatureExtensionData,
byte[] userAgentJsonPayload,
bool useFeatureExt,
int length,
bool write = false)
Expand Down Expand Up @@ -9192,6 +9203,11 @@ private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures,
length += WriteVectorSupportFeatureRequest(write);
}

if ((requestedFeatures & TdsEnums.FeatureExtension.UserAgent) != 0)
{
length += WriteUserAgentFeatureRequest(userAgentJsonPayload, write);
}

length++; // for terminator
if (write)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public enum EnvChangeType : byte
public const byte FEATUREEXT_JSONSUPPORT = 0x0D;
public const byte FEATUREEXT_VECTORSUPPORT = 0x0E;
// TODO: re-verify if this byte competes with another feature
public const byte FEATUREEXT_USERAGENT = 0x0F;
public const byte FEATUREEXT_USERAGENT = 0x10;

[Flags]
public enum FeatureExtension : uint
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Buffers;
using System.Diagnostics;
using System.Text;
using Microsoft.Data.SqlClient.UserAgent;
using Microsoft.Data.SqlClient.Utilities;

#nullable enable
Expand Down Expand Up @@ -191,8 +192,21 @@ internal void TdsLogin(
}

int feOffset = length;

// NOTE: This approach of pre-calculating the packet length is inefficient.
// We're making 2 passes over the data to be written.
// Instead, we should be writing everything to the buffer once,
// leaving a hole where the header length goes.

// calculate and reserve the required bytes for the featureEx
length = ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length);
length = ApplyFeatureExData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an observation - nothing for you to address here right now.

This approach of pre-calculating the packet length is inefficient. We're making 2 passes over the data to be written. Instead, we should be writing everything to the buffer once, leaving a hole where the header length goes, calculating the length as we write the data, and then going back at the end to fill in the header's length field.

I wonder if we're similarly inefficient for the other TDS packets/messages we write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave a comment here so that we can follow up and investigate a bit later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been addressed?

requestedFeatures,
recoverySessionData,
fedAuthFeatureExtensionData,
UserAgentInfo.UserAgentCachedJsonPayload.ToArray(),
useFeatureExt,
length
);

WriteLoginData(rec,
requestedFeatures,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Security;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.SqlServer.TDS;
using Microsoft.SqlServer.TDS.FeatureExtAck;
using Microsoft.SqlServer.TDS.Login7;
using Microsoft.SqlServer.TDS.PreLogin;
using Microsoft.SqlServer.TDS.Servers;
using Xunit;

Expand Down Expand Up @@ -620,5 +618,78 @@ public void TestConnWithVectorFeatExtVersionNegotiation(bool expectedConnectionR
Assert.Throws<InvalidOperationException>(() => connection.Open());
}
}

// Test to verify that the client sends a UserAgent version
// and driver behaves correctly even if server sent an Ack
[Theory]
[InlineData(false)] // We do not force test server to send an Ack
[InlineData(true)] // Server is forced to send an Ack
public void TestConnWithUserAgentFeatureExtension(bool forceAck)
{
using var server = TestTdsServer.StartTestServer();

// Configure the server to support UserAgent version 0x01
server.ServerSupportedUserAgentFeatureExtVersion = 0x01;

// Opt in to forced ACK for UserAgentSupport (no negotiation)
server.EmitUserAgentFeatureExtAck = forceAck;

bool loginFound = false;

// Captured from LOGIN7 as parsed by the test server
byte observedVersion = 0;
byte[] observedJsonBytes = Array.Empty<byte>();

// Inspect what the client sends in the LOGIN7 packet
server.OnLogin7Validated = loginToken =>
{
var token = loginToken.FeatureExt
.OfType<TDSLogin7GenericOptionToken>()
.FirstOrDefault(t => t.FeatureID == TDSFeatureID.UserAgentSupport);


// Test should fail if no UserAgent FE token is found
Assert.NotNull(token);

Assert.Equal((byte)TDSFeatureID.UserAgentSupport, (byte)token.FeatureID);

// Layout: [0] = version byte, rest = UTF-8 JSON blob
Assert.True(token.Data.Length >= 2, "UserAgent token is too short");

observedVersion = token.Data[0];
Assert.Equal(0x1, observedVersion);

observedJsonBytes = token.Data.AsSpan(1).ToArray();
loginFound = true;
};

// TODO: Confirm the server sent an Ack by reading log message from SqlInternalConnectionTds
using var connection = new SqlConnection(server.ConnectionString);
connection.Open();

// Verify client did offer UserAgent
Assert.True(loginFound, "Expected UserAgent extension in LOGIN7");

// Verify the connection itself succeeded
Assert.Equal(ConnectionState.Open, connection.State);

// Note: Accessing UserAgentInfo via Reflection.
// We cannot use InternalsVisibleTo here because making internals visible to FunctionalTests
// causes the *.TestHarness.cs stubs to clash with the real internal types in SqlClient.
var asm = typeof(SqlConnection).Assembly;
var userAgentInfoType =
asm.GetTypes().FirstOrDefault(t => string.Equals(t.Name, "UserAgentInfo", StringComparison.Ordinal)) ??
asm.GetTypes().FirstOrDefault(t => t.FullName?.EndsWith(".UserAgentInfo", StringComparison.Ordinal) == true);

Assert.NotNull(userAgentInfoType);

// Try to get the property
var prop = userAgentInfoType.GetProperty("UserAgentCachedJsonPayload",
BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic);
Assert.NotNull(prop);

ReadOnlyMemory<byte> cachedPayload = (ReadOnlyMemory<byte>)prop.GetValue(null)!;
Assert.Equal(cachedPayload.ToArray(), observedJsonBytes.ToArray());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,26 @@ public delegate void OnAuthenticationCompletedDelegate(
/// </summary>
public byte ServerSupportedVectorFeatureExtVersion { get; set; } = DefaultSupportedVectorFeatureExtVersion;

/// <summary>
/// Property for setting server version for user agent feature extension.
/// </summary>
public byte ServerSupportedUserAgentFeatureExtVersion { get; set; } = DefaultSupportedUserAgentFeatureExtVersion;

/// <summary>
/// Client version for vector FeatureExtension.
/// </summary>
private byte _clientSupportedVectorFeatureExtVersion = 0;

/// <summary>
/// Server will ACK UserAgentSupport in the login response when this property is set to true.
/// </summary>
public bool EmitUserAgentFeatureExtAck { get; set; } = false;

/// <summary>
/// Default feature extension version supported on the server for user agent.
/// </summary>
public const byte DefaultSupportedUserAgentFeatureExtVersion = 0x01;

/// <summary>
/// Session counter
/// </summary>
Expand Down Expand Up @@ -654,6 +669,30 @@ protected virtual TDSMessageCollection OnAuthenticationCompleted(ITDSServerSessi
}
}

// If tests request it, force an ACK for UserAgentSupport with no negotiation
if (EmitUserAgentFeatureExtAck)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rewrite this large method to split these conditional blocks into individual methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you not be reading session instance value here as done above for other feature ext tokens?

{
byte ackVersion = ServerSupportedUserAgentFeatureExtVersion;

var data = new byte[] { ackVersion };
var uaAck = new TDSFeatureExtAckGenericOption(
TDSFeatureID.UserAgentSupport,
(uint)data.Length,
data);

// Reuse an existing FeatureExtAck token if present, otherwise add a new one
var featureExtAckToken = responseMessage.OfType<TDSFeatureExtAckToken>().FirstOrDefault();
if (featureExtAckToken == null)
{
featureExtAckToken = new TDSFeatureExtAckToken(uaAck);
responseMessage.Add(featureExtAckToken);
}
else
{
featureExtAckToken.Options.Add(uaAck);
}
}

// Create DONE token
TDSDoneToken doneToken = new TDSDoneToken(TDSDoneTokenStatusType.Final);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ public enum TDSFeatureID : byte
/// </summary>
VectorSupport = 0x0E,

/// <summary>
/// User Agent Support
/// </summary>
UserAgentSupport = 0x10,

/// <summary>
/// End of the list
/// </summary>
Expand Down
Loading