From 98082dcac9d612732b56842ca3c5a5ad338595d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= <11718369+ManickaP@users.noreply.github.com> Date: Wed, 17 Jul 2024 12:23:09 +0200 Subject: [PATCH] Redact query from HttpTelemetry (#104970) * Redact query from HttpTelemetry * Feedback * Handle redirect as well + feedback --- .../src/System/Net/Http/HttpTelemetry.cs | 28 ++++ .../SocketsHttpHandler/RedirectHandler.cs | 2 +- .../tests/FunctionalTests/TelemetryTest.cs | 149 +++++++++++++++++- 3 files changed, 176 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs index 3dfc789919b33..03485b8ef2830 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Diagnostics.Tracing; +using System.Text; using System.Threading; namespace System.Net.Http @@ -37,6 +38,14 @@ public static class Keywords private void RequestStart(string scheme, string host, int port, string pathAndQuery, byte versionMajor, byte versionMinor, HttpVersionPolicy versionPolicy) { Interlocked.Increment(ref _startedRequests); + if (!GlobalHttpSettings.DiagnosticsHandler.DisableUriRedaction) + { + int queryIndex = pathAndQuery.IndexOf('?'); + if (queryIndex >= 0 && queryIndex < (pathAndQuery.Length - 1)) + { + pathAndQuery = $"{pathAndQuery.AsSpan(0, queryIndex + 1)}*"; + } + } WriteEvent(eventId: 1, scheme, host, port, pathAndQuery, versionMajor, versionMinor, versionPolicy); } @@ -175,6 +184,25 @@ public void Redirect(string redirectUri) WriteEvent(eventId: 16, redirectUri); } + [NonEvent] + public void Redirect(Uri redirectUri) + { + if (!GlobalHttpSettings.DiagnosticsHandler.DisableUriRedaction) + { + string pathAndQuery = redirectUri.PathAndQuery; + int queryIndex = pathAndQuery.IndexOf('?'); + if (queryIndex >= 0 && queryIndex < (pathAndQuery.Length - 1)) + { + UriBuilder uriBuilder = new UriBuilder(redirectUri) + { + Query = "*" + }; + redirectUri = uriBuilder.Uri; + } + } + Redirect(redirectUri.AbsoluteUri); + } + [NonEvent] public void Http11ConnectionEstablished(long connectionId, string scheme, string host, int port, IPEndPoint? remoteEndPoint) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs index 961f6ff614892..a1256da552f63 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs @@ -55,7 +55,7 @@ internal override async ValueTask SendAsync(HttpRequestMess if (HttpTelemetry.Log.IsEnabled()) { - HttpTelemetry.Log.Redirect(redirectUri.AbsoluteUri); + HttpTelemetry.Log.Redirect(redirectUri); } if (NetEventSource.Log.IsEnabled()) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs index 081c09ae69336..e6409e389f794 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs @@ -3,6 +3,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.Tracing; using System.IO; using System.Linq; @@ -48,6 +49,19 @@ public static IEnumerable TestMethods_MemberData() yield return new object[] { "InvokerSend" }; } + public static IEnumerable Redaction_MemberData() + { + string[] uriTails = new string[] { "/test/path?q1=a&q2=b", "/test/path", "?q1=a&q2=b", "" }; + foreach (string uriTail in new[] { "/test/path?q1=a&q2=b", "/test/path", "?q1=a&q2=b", "" }) + { + foreach (string fragment in new[] { "", "#frag" }) + { + yield return new object[] { uriTail + fragment, true }; + yield return new object[] { uriTail + fragment, false }; + } + } + } + [OuterLoop] [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [MemberData(nameof(TestMethods_MemberData))] @@ -399,7 +413,74 @@ await server.AcceptConnectionAsync(async connection => }, UseVersion.ToString(), testMethod).DisposeAsync(); } - private static void ValidateStartFailedStopEvents(ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)> events, Version version, bool shouldHaveFailures = false, int count = 1) + [OuterLoop] + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [MemberData(nameof(Redaction_MemberData))] + public async Task EventSource_SendingRequest_PathAndQueryRedaction_LogsStartStop(string uriTail, bool disableRedaction) + { + var psi = new ProcessStartInfo(); + psi.Environment.Add("DOTNET_SYSTEM_NET_HTTP_DISABLEURIREDACTION", disableRedaction.ToString()); + var fragIndex = uriTail.IndexOf('#'); + var expectedPathAndQuery = uriTail.Substring(0, fragIndex >= 0 ? fragIndex : uriTail.Length); + if (!disableRedaction) + { + var queryIndex = expectedPathAndQuery.IndexOf('?'); + expectedPathAndQuery = expectedPathAndQuery.Substring(0, queryIndex >= 0 ? queryIndex + 1 : expectedPathAndQuery.Length); + expectedPathAndQuery = queryIndex >= 0 ? expectedPathAndQuery + '*' : expectedPathAndQuery; + } + expectedPathAndQuery = expectedPathAndQuery.StartsWith('/') ? expectedPathAndQuery : '/' + expectedPathAndQuery; + + await RemoteExecutor.Invoke(static async (useVersionString, uriTail, expectedPathAndQuery) => + { + Version version = Version.Parse(useVersionString); + using var listener = new TestEventListener("System.Net.Http", EventLevel.Verbose, eventCounterInterval: 0.1d); + listener.AddActivityTracking(); + + var events = new ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)>(); + Uri expectedUri = null; + await listener.RunWithCallbackAsync(e => events.Enqueue((e, e.ActivityId)), async () => + { + await GetFactoryForVersion(version).CreateClientAndServerAsync( + async uri => + { + expectedUri = uri; + using HttpClientHandler handler = CreateHttpClientHandler(version); + using HttpClient client = CreateHttpClient(handler, useVersionString); + client.BaseAddress = uri; + using var invoker = new HttpMessageInvoker(handler); + + var request = new HttpRequestMessage(HttpMethod.Get, uri) + { + Version = version + }; + + await client.GetAsync(uriTail); + }, + async server => + { + await server.AcceptConnectionAsync(async connection => + { + await connection.ReadRequestDataAsync(); + await WaitForEventCountersAsync(events); + await connection.SendResponseAsync(); + }); + }); + + await WaitForEventCountersAsync(events); + }); + Assert.DoesNotContain(events, e => e.Event.EventId == 0); // errors from the EventSource itself + + ValidateStartFailedStopEvents(events, version, pathAndQuery: expectedPathAndQuery); + + ValidateConnectionEstablishedClosed(events, version, expectedUri); + + ValidateRequestResponseStartStopEvents(events, null, 0, count: 1); + + ValidateEventCounters(events, requestCount: 1, shouldHaveFailures: false, versionMajor: version.Major); + }, UseVersion.ToString(), uriTail, expectedPathAndQuery, new RemoteInvokeOptions { StartInfo = psi }).DisposeAsync(); + } + + private static void ValidateStartFailedStopEvents(ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)> events, Version version, bool shouldHaveFailures = false, int count = 1, string? pathAndQuery = null) { (EventWrittenEventArgs Event, Guid ActivityId)[] starts = events.Where(e => e.Event.EventName == "RequestStart").ToArray(); foreach (EventWrittenEventArgs startEvent in starts.Select(e => e.Event)) @@ -409,6 +490,10 @@ private static void ValidateStartFailedStopEvents(ConcurrentQueue<(EventWrittenE Assert.NotEmpty((string)startEvent.Payload[1]); // host Assert.True(startEvent.Payload[2] is int port && port >= 0 && port <= 65535); Assert.NotEmpty((string)startEvent.Payload[3]); // pathAndQuery + if (pathAndQuery is not null) + { + Assert.Equal(pathAndQuery, (string)startEvent.Payload[3]); // pathAndQuery + } byte versionMajor = Assert.IsType(startEvent.Payload[4]); Assert.Equal(version.Major, versionMajor); byte versionMinor = Assert.IsType(startEvent.Payload[5]); @@ -455,7 +540,7 @@ private static void ValidateStartFailedStopEvents(ConcurrentQueue<(EventWrittenE } } - // The validation asssumes that the connection id's are in range 0..(connectionCount-1) + // The validation assumes that the connection id's are in range 0..(connectionCount-1) protected static void ValidateConnectionEstablishedClosed(ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)> events, Version version, Uri uri, int connectionCount = 1) { EventWrittenEventArgs[] connectionsEstablished = events.Select(e => e.Event).Where(e => e.EventName == "ConnectionEstablished").ToArray(); @@ -824,6 +909,66 @@ await GetFactoryForVersion(version).CreateServerAsync((originalServer, originalU }, UseVersion.ToString()).DisposeAsync(); } + [OuterLoop] + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [MemberData(nameof(Redaction_MemberData))] + public async Task EventSource_Redirect_PathAndQueryRedaction_LogsRedirect(string uriTail, bool disableRedaction) + { + var psi = new ProcessStartInfo(); + psi.Environment.Add("DOTNET_SYSTEM_NET_HTTP_DISABLEURIREDACTION", disableRedaction.ToString()); + var fragIndex = uriTail.IndexOf('#'); + var expectedUriTail = uriTail.Substring(0, fragIndex >= 0 ? fragIndex : uriTail.Length); + if (!disableRedaction) + { + var queryIndex = expectedUriTail.IndexOf('?'); + expectedUriTail = expectedUriTail.Substring(0, queryIndex >= 0 ? queryIndex + 1 : expectedUriTail.Length); + expectedUriTail = queryIndex >= 0 ? expectedUriTail + '*' : expectedUriTail; + } + expectedUriTail = fragIndex >= 0 ? expectedUriTail + uriTail.Substring(fragIndex) : expectedUriTail; + + await RemoteExecutor.Invoke(static async (useVersionString, uriTail, expectedUriTail) => + { + Version version = Version.Parse(useVersionString); + + using var listener = new TestEventListener("System.Net.Http", EventLevel.Verbose, eventCounterInterval: 0.1d); + listener.AddActivityTracking(); + var events = new ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)>(); + Uri expectedUri = null; + + await listener.RunWithCallbackAsync(e => events.Enqueue((e, e.ActivityId)), async () => + { + await GetFactoryForVersion(version).CreateServerAsync((originalServer, originalUri) => + { + return GetFactoryForVersion(version).CreateServerAsync(async (redirectServer, redirectUri) => + { + expectedUri = redirectUri; + using HttpClient client = CreateHttpClient(useVersionString); + + using HttpRequestMessage request = new(HttpMethod.Get, originalUri) { Version = version }; + + Task clientTask = client.SendAsync(request); + Task serverTask = originalServer.HandleRequestAsync(HttpStatusCode.Redirect, new[] { new HttpHeaderData("Location", redirectUri.AbsoluteUri + uriTail) }); + + await Task.WhenAny(clientTask, serverTask); + Assert.False(clientTask.IsCompleted, $"{clientTask.Status}: {clientTask.Exception}"); + await serverTask; + + serverTask = redirectServer.HandleRequestAsync(); + await TestHelper.WhenAllCompletedOrAnyFailed(clientTask, serverTask); + await clientTask; + }); + }); + + await WaitForEventCountersAsync(events); + }); + + EventWrittenEventArgs redirectEvent = events.Where(e => e.Event.EventName == "Redirect").Single().Event; + Assert.Equal(1, redirectEvent.Payload.Count); + Assert.Equal(expectedUri.ToString() + expectedUriTail, (string)redirectEvent.Payload[0]); + Assert.Equal("redirectUri", redirectEvent.PayloadNames[0]); + }, UseVersion.ToString(), uriTail, expectedUriTail, new RemoteInvokeOptions { StartInfo = psi }).DisposeAsync(); + } + public static bool SupportsRemoteExecutorAndAlpn = RemoteExecutor.IsSupported && PlatformDetection.SupportsAlpn; [OuterLoop]