diff --git a/src/Elastic.Apm/DiagnosticListeners/HttpDiagnosticListenerFullFrameworkImpl.cs b/src/Elastic.Apm/DiagnosticListeners/HttpDiagnosticListenerFullFrameworkImpl.cs index c855632ed..a6c3abb03 100644 --- a/src/Elastic.Apm/DiagnosticListeners/HttpDiagnosticListenerFullFrameworkImpl.cs +++ b/src/Elastic.Apm/DiagnosticListeners/HttpDiagnosticListenerFullFrameworkImpl.cs @@ -76,6 +76,19 @@ protected override void ProcessExceptionEvent(object eventValue, Uri requestUrl) { if (statusCodeObject is HttpStatusCode statusCode) { + // HttpHandlerDiagnosticListener fires Ex.Stop for every non-redirect response, including + // intermediate 401 challenges during NTLM/Negotiate authentication. Detect these by checking + // the WWW-Authenticate header (also present in the Ex.Stop payload) and keep the span alive + // so the subsequent Stop event can record the final status code instead. + if (statusCode == HttpStatusCode.Unauthorized && IsNtlmOrNegotiateChallenge(eventValue)) + { + Logger.Trace()?.Log("NTLM/Negotiate 401 challenge detected - keeping span open for final response. Request URL: {RequestUrl}", requestUrl); + if (ProcessingRequests.TryAdd(request, span)) + return; + Logger.Error()?.Log("Failed to re-add span after NTLM challenge - ending span to avoid leak. Request URL: {RequestUrl}", requestUrl); + // Fall through to record the 401 and end the span + } + span.Context.Http.StatusCode = (int)statusCode; if (span.Context.Http.StatusCode >= 300) @@ -96,5 +109,37 @@ protected override void ProcessExceptionEvent(object eventValue, Uri requestUrl) span.End(); } + + // Exposed as internal so tests can exercise header parsing directly without going through reflection. + internal static bool IsNtlmOrNegotiateChallenge(WebHeaderCollection headers) + { + var wwwAuthenticate = headers?[HttpResponseHeader.WwwAuthenticate]; + if (string.IsNullOrEmpty(wwwAuthenticate)) + return false; + + // Parse the scheme token from each comma-delimited challenge. Taking the first whitespace-delimited + // token per challenge avoids false-positives such as Basic realm="Negotiate" where "Negotiate" is a + // realm value, not a scheme name. + foreach (var part in wwwAuthenticate.Split(',')) + { + var challenge = part.Trim(); + if (string.IsNullOrEmpty(challenge)) + continue; + + var spaceIndex = challenge.IndexOf(' '); + var scheme = spaceIndex < 0 ? challenge : challenge.Substring(0, spaceIndex); + + if (scheme.Equals("NTLM", StringComparison.OrdinalIgnoreCase) || + scheme.Equals("Negotiate", StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } + + private static bool IsNtlmOrNegotiateChallenge(object eventValue) + { + var headersObject = eventValue.GetType().GetTypeInfo().GetDeclaredProperty("Headers")?.GetValue(eventValue); + return headersObject is WebHeaderCollection headers && IsNtlmOrNegotiateChallenge(headers); + } } } diff --git a/test/Elastic.Apm.Tests/HttpDiagnosticListenerTests.cs b/test/Elastic.Apm.Tests/HttpDiagnosticListenerTests.cs index c62366cbc..c0c97756d 100644 --- a/test/Elastic.Apm.Tests/HttpDiagnosticListenerTests.cs +++ b/test/Elastic.Apm.Tests/HttpDiagnosticListenerTests.cs @@ -393,6 +393,285 @@ public void CaptureErrorOnFailingHttpCall_DirectCall() } } + // ----------------------------------------------------------------------- + // Issue #2466 — NTLM/Negotiate authentication + // + // Root cause: HttpHandlerDiagnosticListener.IsLastResponse returns true for + // any non-redirect status code, so it fires Ex.Stop for every intermediate + // 401 NTLM challenge *before* the final HttpWebResponse is constructed. + // The agent was treating that Ex.Stop as the terminal event, ending the span + // with status 401 and capturing an error. The subsequent Stop event (fired + // when the real 200 arrives) then found nothing in ProcessingRequests and + // was silently dropped. + // ----------------------------------------------------------------------- + +#pragma warning disable SYSLIB0014 // WebRequest.Create is the only way to get an HttpWebRequest in tests + private static (HttpDiagnosticListenerFullFrameworkImpl Listener, HttpWebRequest Request) + CreateFullFrameworkListenerAndRequest(ApmAgent agent) + { + agent.HttpTraceConfiguration.CaptureSpan = true; + var listener = new HttpDiagnosticListenerFullFrameworkImpl(agent); + var request = (HttpWebRequest)WebRequest.Create("http://elastic.co"); + return (listener, request); + } +#pragma warning restore SYSLIB0014 + + private static void FireExStop(HttpDiagnosticListenerFullFrameworkImpl listener, HttpWebRequest request, + HttpStatusCode statusCode, WebHeaderCollection headers) => + listener.OnNext(new KeyValuePair( + "System.Net.Http.Desktop.HttpRequestOut.Ex.Stop", + new { Request = request, StatusCode = statusCode, Headers = headers })); + + private static void FireStart(HttpDiagnosticListenerFullFrameworkImpl listener, HttpWebRequest request) => + listener.OnNext(new KeyValuePair( + "System.Net.Http.Desktop.HttpRequestOut.Start", + new { Request = request })); + + private static void FireStop(HttpDiagnosticListenerFullFrameworkImpl listener, HttpWebRequest request) => + listener.OnNext(new KeyValuePair( + "System.Net.Http.Desktop.HttpRequestOut.Stop", + new { Request = request, Response = (HttpWebResponse)null })); + + /// + /// Reproduces https://github.com/elastic/apm-agent-dotnet/issues/2466 — single NTLM round. + /// Proves the failure: without the fix, ProcessExceptionEvent calls CaptureError + span.End(), + /// so payloadSender.Errors would be non-empty and the span would carry StatusCode 401. + /// Proves the fix: the intermediate Ex.Stop is ignored; the span is only ended by the final + /// Stop event, leaving Errors empty. + /// + [Fact] + public void NtlmChallenge_SingleRound_SpanCapturedWithoutError() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + // 1. Request starts + FireStart(listener, request); + listener.ProcessingRequests.Should().ContainKey(request); + + // 2. Server returns 401 + NTLM challenge (intermediate handshake step). + // HttpHandlerDiagnosticListener fires Ex.Stop because IsLastResponse(401) == true. + // Before the fix: CaptureError + span.End() would be called here → span status 401. + var ntlmHeaders = new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "NTLM" }; + FireExStop(listener, request, HttpStatusCode.Unauthorized, ntlmHeaders); + + // Span must survive the challenge — still tracked, not ended with 401 + listener.ProcessingRequests.Should().ContainKey(request, + "span must not be prematurely ended on the intermediate NTLM 401 challenge"); + + // 3. NTLM handshake succeeds; final Stop fires. + // (HttpWebResponse has no public ctor so Response is null — the span still ends cleanly.) + FireStop(listener, request); + listener.ProcessingRequests.Should().NotContainKey(request, "span must be ended by the final Stop"); + + // Core proof of the fix: without it, CaptureError would have been called for the 401 challenge + payloadSender.WaitForSpans(count: 1); + using (new AssertionScope()) + { + payloadSender.Errors.Should().BeEmpty("the NTLM challenge must not be recorded as an APM error"); + payloadSender.Spans.Should().HaveCount(1, "exactly one span should be sent for the whole request"); + payloadSender.FirstSpan.Context.Http.StatusCode.Should().NotBe(401, + "the span must not carry the intermediate challenge status code"); + } + } + + /// + /// Full two-round NTLM handshake: the runtime fires Ex.Stop twice (type-1 and type-2 challenges) + /// before the final Stop. Both intermediate events must be ignored. + /// + [Fact] + public void NtlmChallenge_TwoRounds_BothChallengesIgnoredSpanCapturedOnce() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + + // Round 1: bare NTLM challenge + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "NTLM" }); + listener.ProcessingRequests.Should().ContainKey(request, "span must survive round-1 NTLM challenge"); + + // Round 2: NTLM type-2 message with server's challenge token + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "NTLM dummybase64token==" }); + listener.ProcessingRequests.Should().ContainKey(request, "span must survive round-2 NTLM challenge"); + + FireStop(listener, request); + + payloadSender.WaitForSpans(count: 1); + using (new AssertionScope()) + { + payloadSender.Errors.Should().BeEmpty(); + payloadSender.Spans.Should().HaveCount(1); + } + } + + /// + /// Negotiate (SPNEGO/Kerberos) challenges use the same code path as NTLM. + /// + [Fact] + public void NegotiateChallenge_SingleRound_SpanCapturedWithoutError() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "Negotiate" }); + listener.ProcessingRequests.Should().ContainKey(request, "span must survive a Negotiate challenge"); + + FireStop(listener, request); + listener.ProcessingRequests.Should().NotContainKey(request); + + payloadSender.WaitForSpans(count: 1); + using (new AssertionScope()) + { + payloadSender.Errors.Should().BeEmpty("Negotiate challenge must not be recorded as an APM error"); + payloadSender.Spans.Should().HaveCount(1); + payloadSender.FirstSpan.Context.Http.StatusCode.Should().NotBe(401); + } + } + + /// + /// A genuine 401 (Basic auth, no NTLM/Negotiate header) must still be recorded as a failure. + /// Regression guard: the fix must not suppress real auth failures. + /// + [Fact] + public void Genuine401_ExStopWithBasicAuthHeader_SpanEndedWithFailure() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "Basic realm=\"example\"" }); + + listener.ProcessingRequests.Should().NotContainKey(request, "genuine 401 must end the span immediately"); + payloadSender.WaitForErrors(); + using (new AssertionScope()) + { + payloadSender.Errors.Should().NotBeEmpty("genuine 401 must be reported as a failure"); + payloadSender.WaitForSpans(count: 1); + payloadSender.FirstSpan.Context.Http.StatusCode.Should().Be(401); + } + } + + /// + /// Regression guard: Ex.Stop with a null Headers payload (edge case) must still end the span as a failure. + /// + [Fact] + public void ExStop_With401AndNullHeaders_SpanEndedWithFailure() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + // Headers property present but null — must not crash, must still record the 401 + FireExStop(listener, request, HttpStatusCode.Unauthorized, headers: null); + + listener.ProcessingRequests.Should().NotContainKey(request); + payloadSender.WaitForErrors(); + payloadSender.Errors.Should().NotBeEmpty("a 401 with null headers is still a failure"); + } + + /// + /// Regression guard: Ex.Stop with a non-401 status code (500) must still end the span as a failure. + /// The NTLM check is gated on 401 Unauthorized; all other codes go through the original path. + /// + [Fact] + public void ExStop_WithNon401StatusCode_SpanEndedWithFailure() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + FireExStop(listener, request, HttpStatusCode.InternalServerError, new WebHeaderCollection()); + + listener.ProcessingRequests.Should().NotContainKey(request, "500 must end the span immediately"); + payloadSender.WaitForErrors(); + using (new AssertionScope()) + { + payloadSender.Errors.Should().NotBeEmpty("500 must be reported as a failure"); + payloadSender.WaitForSpans(count: 1); + payloadSender.FirstSpan.Context.Http.StatusCode.Should().Be(500); + } + } + + /// + /// Regression guard: WWW-Authenticate: Basic realm="Negotiate" must NOT be treated as NTLM/Negotiate. + /// "Negotiate" appears in the realm value, not as the authentication scheme token. + /// + [Fact] + public void Genuine401_BasicRealmContainingNegotiate_SpanEndedWithFailure() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "Basic realm=\"Negotiate\"" }); + + listener.ProcessingRequests.Should().NotContainKey(request, + "Basic realm=\"Negotiate\" is not an NTLM/Negotiate challenge and must end the span immediately"); + payloadSender.WaitForErrors(); + using (new AssertionScope()) + { + payloadSender.Errors.Should().NotBeEmpty("a Basic 401 with Negotiate in the realm must be recorded as a failure"); + payloadSender.WaitForSpans(count: 1); + payloadSender.FirstSpan.Context.Http.StatusCode.Should().Be(401); + } + } + + /// + /// When NTLM authentication ultimately fails (wrong credentials), the server returns a terminal 401 + /// with WWW-Authenticate: NTLM. The .NET Framework runtime fires Ex.Stop for the terminal challenge + /// and then Stop when the request completes; the span must survive the second Ex.Stop and be ended by Stop. + /// + [Fact] + public void NtlmChallenge_TerminalAuthFailure_SpanEndedByFinalStop() + { + var payloadSender = new MockPayloadSender(); + var agent = new ApmAgent(new TestAgentComponents(payloadSender: payloadSender)); + StartTransaction(agent); + var (listener, request) = CreateFullFrameworkListenerAndRequest(agent); + + FireStart(listener, request); + + // Intermediate NTLM type-1 challenge + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "NTLM" }); + listener.ProcessingRequests.Should().ContainKey(request, "span must survive intermediate NTLM challenge"); + + // Terminal 401: credentials rejected; server re-issues NTLM challenge — Ex.Stop fires again + FireExStop(listener, request, HttpStatusCode.Unauthorized, + new WebHeaderCollection { [HttpResponseHeader.WwwAuthenticate] = "NTLM" }); + listener.ProcessingRequests.Should().ContainKey(request, + "span must survive terminal NTLM 401 until the final Stop arrives"); + + // Stop fires when the request completes — this must end the span + FireStop(listener, request); + listener.ProcessingRequests.Should().NotContainKey(request, "span must be ended by the final Stop"); + + payloadSender.WaitForSpans(count: 1); + payloadSender.Spans.Should().HaveCount(1, "exactly one span must be sent for the whole request"); + } + /// /// Makes sure we set the correct type and subtype for external, http spans ///