From dfe12223b89916b0e16800f02e4d120afc1c500c Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2016 15:00:56 -0700 Subject: [PATCH] Ensure HTTPS handshake errors aren't logged on socket close --- .../HttpsConnectionFilter.cs | 11 +- .../Internal/ClosedStream.cs | 66 ++++++++ .../HttpsTests.cs | 157 ++++++++++++++++++ .../HttpsConnectionFilterTests.cs | 52 ------ 4 files changed, 232 insertions(+), 54 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Server.Kestrel.Https/Internal/ClosedStream.cs create mode 100644 test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionFilter.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionFilter.cs index 270d0e7297..30a34cfb91 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionFilter.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionFilter.cs @@ -8,12 +8,15 @@ using System.Security.Cryptography.X509Certificates; using System.Threading.Tasks; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Filter; +using Microsoft.AspNetCore.Server.Kestrel.Https.Internal; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Server.Kestrel.Https { public class HttpsConnectionFilter : IConnectionFilter { + private static readonly ClosedStream _closedStream = new ClosedStream(); + private readonly HttpsConnectionFilterOptions _options; private readonly IConnectionFilter _previous; private readonly ILogger _logger; @@ -95,8 +98,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https certificateRequired = true; } - context.Connection = sslStream; - try { await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, certificateRequired, @@ -105,6 +106,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https catch (IOException ex) { _logger?.LogInformation(1, ex, "Failed to authenticate HTTPS connection."); + + sslStream.Dispose(); + context.Connection = _closedStream; + return; } @@ -121,6 +126,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https features.Get().Scheme = "https"; }; + + context.Connection = sslStream; } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Https/Internal/ClosedStream.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Https/Internal/ClosedStream.cs new file mode 100644 index 0000000000..c9d030cd87 --- /dev/null +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Https/Internal/ClosedStream.cs @@ -0,0 +1,66 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace Microsoft.AspNetCore.Server.Kestrel.Https.Internal +{ + internal class ClosedStream : Stream + { + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + + public override long Length + { + get + { + throw new NotSupportedException(); + } + } + + public override long Position + { + get + { + throw new NotSupportedException(); + } + set + { + throw new NotSupportedException(); + } + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) + { + throw new NotSupportedException(); + } + + public override void SetLength(long value) + { + throw new NotSupportedException(); + } + + public override int Read(byte[] buffer, int offset, int count) + { + return 0; + } + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + return Task.FromResult(0); + } + + public override void Write(byte[] buffer, int offset, int count) + { + throw new NotSupportedException(); + } + } +} diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs new file mode 100644 index 0000000000..2fa1838195 --- /dev/null +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs @@ -0,0 +1,157 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Net.Sockets; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Server.Kestrel.Https; +using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests +{ + public class HttpsTests + { + [Fact] + public async Task EmptyRequestLoggedAsInformation() + { + var loggerFactory = new HandshakeErrorLoggerFactory(); + + var hostBuilder = new WebHostBuilder() + .UseKestrel(options => + { + options.UseHttps(@"TestResources/testCert.pfx", "testPassword"); + }) + .UseUrls("https://127.0.0.1:0/") + .UseLoggerFactory(loggerFactory) + .Configure(app => { }); + + using (var host = hostBuilder.Build()) + { + host.Start(); + + using (await HttpClientSlim.GetSocket(new Uri($"http://127.0.0.1:{host.GetPort()}/"))) + { + // Close socket immediately + } + } + + await loggerFactory.FilterLogger.LogTcs.Task; + + Assert.Equal(1, loggerFactory.FilterLogger.LastEventId.Id); + Assert.Equal(LogLevel.Information, loggerFactory.FilterLogger.LastLogLevel); + Assert.Equal(0, loggerFactory.ErrorLogger.TotalErrorsLogged); + } + + + [Fact] + public async Task ClientHandshakeFailureLoggedAsInformation() + { + var loggerFactory = new HandshakeErrorLoggerFactory(); + + var hostBuilder = new WebHostBuilder() + .UseKestrel(options => + { + options.UseHttps(@"TestResources/testCert.pfx", "testPassword"); + }) + .UseUrls("https://127.0.0.1:0/") + .UseLoggerFactory(loggerFactory) + .Configure(app => { }); + + using (var host = hostBuilder.Build()) + { + host.Start(); + + using (var socket = await HttpClientSlim.GetSocket(new Uri($"https://127.0.0.1:{host.GetPort()}/"))) + using (var stream = new NetworkStream(socket)) + { + // Send null bytes and close socket + await stream.WriteAsync(new byte[10], 0, 10); + } + } + + await loggerFactory.FilterLogger.LogTcs.Task; + + Assert.Equal(1, loggerFactory.FilterLogger.LastEventId.Id); + Assert.Equal(LogLevel.Information, loggerFactory.FilterLogger.LastLogLevel); + Assert.Equal(0, loggerFactory.ErrorLogger.TotalErrorsLogged); + } + + private class HandshakeErrorLoggerFactory : ILoggerFactory + { + public HttpsConnectionFilterLogger FilterLogger { get; } = new HttpsConnectionFilterLogger(); + public ApplicationErrorLogger ErrorLogger { get; } = new ApplicationErrorLogger(); + + public ILogger CreateLogger(string categoryName) + { + if (categoryName == nameof(HttpsConnectionFilter)) + { + return FilterLogger; + } + else + { + return ErrorLogger; + } + + } + + public void AddProvider(ILoggerProvider provider) + { + throw new NotImplementedException(); + } + + public void Dispose() + { + } + } + + private class HttpsConnectionFilterLogger : ILogger + { + public LogLevel LastLogLevel { get; set; } + public EventId LastEventId { get; set; } + public TaskCompletionSource LogTcs { get; } = new TaskCompletionSource(); + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) + { + LastLogLevel = logLevel; + LastEventId = eventId; + LogTcs.SetResult(null); + } + + public bool IsEnabled(LogLevel logLevel) + { + throw new NotImplementedException(); + } + + public IDisposable BeginScope(TState state) + { + throw new NotImplementedException(); + } + } + + private class ApplicationErrorLogger : ILogger + { + public int TotalErrorsLogged { get; set; } + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) + { + if (logLevel == LogLevel.Error) + { + TotalErrorsLogged++; + } + } + + public bool IsEnabled(LogLevel logLevel) + { + return true; + } + + public IDisposable BeginScope(TState state) + { + throw new NotImplementedException(); + } + } + } +} diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/HttpsConnectionFilterTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/HttpsConnectionFilterTests.cs index 5212b709c1..2e5ad48707 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/HttpsConnectionFilterTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/HttpsConnectionFilterTests.cs @@ -331,34 +331,6 @@ namespace Microsoft.AspNetCore.Server.KestrelTests } } - [Fact] - public async Task ClientHandshakeFailureLoggedAsInformation() - { - var logger = new HandshakerErrorLogger(); - var mockLoggerFactory = new Mock(MockBehavior.Strict); - mockLoggerFactory.Setup(m => m.CreateLogger(nameof(HttpsConnectionFilter))).Returns(logger); - - var serviceContext = new TestServiceContext(new HttpsConnectionFilter( - new HttpsConnectionFilterOptions { ServerCertificate = _x509Certificate2 }, - new NoOpConnectionFilter(), - mockLoggerFactory.Object) - ); - - using (var server = new TestServer(App, serviceContext, _serverAddress)) - { - using (await HttpClientSlim.GetSocket(new Uri($"https://localhost:{server.Port}/"))) - { - // Close socket immediately - } - - await logger.LogTcs.Task; - - Assert.Equal(1, logger.LastEventId.Id); - Assert.Equal(LogLevel.Information, logger.LastLogLevel); - mockLoggerFactory.VerifyAll(); - } - } - private static async Task App(HttpContext httpContext) { var request = httpContext.Request; @@ -405,29 +377,5 @@ namespace Microsoft.AspNetCore.Server.KestrelTests Assert.Null(line); } } - - private class HandshakerErrorLogger : ILogger - { - public LogLevel LastLogLevel { get; set; } - public EventId LastEventId { get; set; } - public TaskCompletionSource LogTcs { get; } = new TaskCompletionSource(); - - public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) - { - LastLogLevel = logLevel; - LastEventId = eventId; - LogTcs.SetResult(null); - } - - public bool IsEnabled(LogLevel logLevel) - { - throw new NotImplementedException(); - } - - public IDisposable BeginScope(TState state) - { - throw new NotImplementedException(); - } - } } }