From 8e910baf04366d901bc939d578e527fd840c123c Mon Sep 17 00:00:00 2001 From: Master T Date: Mon, 16 Nov 2015 22:04:27 +0100 Subject: [PATCH] Improve based on pull request feedback --- .../ClientCertificateValidationCallback.cs | 11 --- .../HttpsConnectionFilter.cs | 91 ++++++++++--------- .../HttpsConnectionFilterOptions.cs | 5 +- .../Filter/ConnectionFilterContext.cs | 2 + .../Filter/IConnectionFilter.cs | 1 - .../Filter/NoOpConnectionFilter.cs | 3 - .../Http/Connection.cs | 2 +- .../Http/Frame.cs | 8 +- .../ConnectionFilterTests.cs | 6 -- .../HttpsConnectionFilterTests.cs | 14 +-- 10 files changed, 61 insertions(+), 82 deletions(-) delete mode 100644 src/Microsoft.AspNet.Server.Kestrel.Https/ClientCertificateValidationCallback.cs diff --git a/src/Microsoft.AspNet.Server.Kestrel.Https/ClientCertificateValidationCallback.cs b/src/Microsoft.AspNet.Server.Kestrel.Https/ClientCertificateValidationCallback.cs deleted file mode 100644 index 12616900cc..0000000000 --- a/src/Microsoft.AspNet.Server.Kestrel.Https/ClientCertificateValidationCallback.cs +++ /dev/null @@ -1,11 +0,0 @@ -// 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.Net.Security; -using System.Security.Cryptography.X509Certificates; - -namespace Microsoft.AspNet.Server.Kestrel.Https -{ - public delegate bool ClientCertificateValidationCallback( - X509Certificate2 certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors); -} diff --git a/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilter.cs b/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilter.cs index 39149cc0bc..236f4147d1 100644 --- a/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilter.cs +++ b/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilter.cs @@ -14,28 +14,25 @@ namespace Microsoft.AspNet.Server.Kestrel.Https { public class HttpsConnectionFilter : IConnectionFilter { - private readonly X509Certificate2 _serverCert; - private readonly ClientCertificateMode _clientCertMode; - private readonly ClientCertificateValidationCallback _clientValidationCallback; + private readonly HttpsConnectionFilterOptions _options; private readonly IConnectionFilter _previous; - private readonly SslProtocols _sslProtocols; - private X509Certificate2 _clientCert; public HttpsConnectionFilter(HttpsConnectionFilterOptions options, IConnectionFilter previous) { - if (options.ServerCertificate == null) + if (options == null) { - throw new ArgumentNullException(nameof(options.ServerCertificate)); + throw new ArgumentNullException(nameof(options)); } if (previous == null) { throw new ArgumentNullException(nameof(previous)); } + if (options.ServerCertificate == null) + { + throw new ArgumentException("The server certificate parameter is required."); + } - _serverCert = options.ServerCertificate; - _clientCertMode = options.ClientCertificateMode; - _clientValidationCallback = options.ClientCertificateValidation; - _sslProtocols = options.SslProtocols; + _options = options; _previous = previous; } @@ -45,68 +42,76 @@ namespace Microsoft.AspNet.Server.Kestrel.Https if (string.Equals(context.Address.Scheme, "https", StringComparison.OrdinalIgnoreCase)) { + X509Certificate2 clientCertificate = null; SslStream sslStream; - if (_clientCertMode == ClientCertificateMode.NoCertificate) + if (_options.ClientCertificateMode == ClientCertificateMode.NoCertificate) { sslStream = new SslStream(context.Connection); - await sslStream.AuthenticateAsServerAsync(_serverCert, clientCertificateRequired: false, - enabledSslProtocols: _sslProtocols, checkCertificateRevocation: false); + await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: false, + enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation); } else { sslStream = new SslStream(context.Connection, leaveInnerStreamOpen: false, userCertificateValidationCallback: (sender, certificate, chain, sslPolicyErrors) => { - if (sslPolicyErrors.HasFlag(SslPolicyErrors.RemoteCertificateNotAvailable)) + Console.WriteLine("callback type: " + (certificate is X509Certificate2)); + if (certificate == null) { - return _clientCertMode != ClientCertificateMode.RequireCertificate; + return _options.ClientCertificateMode != ClientCertificateMode.RequireCertificate; } - - if (_clientValidationCallback == null) + if (_options.ClientCertificateValidation == null) { if (sslPolicyErrors != SslPolicyErrors.None) { return false; } } -#if DOTNET5_4 - // conversion X509Certificate to X509Certificate2 not supported - // https://github.com/dotnet/corefx/issues/4510 - X509Certificate2 certificate2 = null; - return false; -#else - X509Certificate2 certificate2 = certificate as X509Certificate2 ?? - new X509Certificate2(certificate); -#endif - if (_clientValidationCallback != null) + X509Certificate2 certificate2 = certificate as X509Certificate2; + if (certificate2 == null) { - if (!_clientValidationCallback(certificate2, chain, sslPolicyErrors)) +#if DOTNET5_4 + // conversion X509Certificate to X509Certificate2 not supported + // https://github.com/dotnet/corefx/issues/4510 + return false; +#else + certificate2 = new X509Certificate2(certificate); +#endif + } + + if (_options.ClientCertificateValidation != null) + { + if (!_options.ClientCertificateValidation(certificate2, chain, sslPolicyErrors)) { return false; } } - _clientCert = certificate2; + clientCertificate = certificate2; return true; }); - await sslStream.AuthenticateAsServerAsync(_serverCert, clientCertificateRequired: true, - enabledSslProtocols: _sslProtocols, checkCertificateRevocation: false); + await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: true, + enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation); + Console.WriteLine("remote type: " + (sslStream.RemoteCertificate is X509Certificate2)); } + + var previousPrepareRequest = context.PrepareRequest; + context.PrepareRequest = features => + { + previousPrepareRequest?.Invoke(features); + + if (clientCertificate != null) + { + features.Set( + new TlsConnectionFeature {ClientCertificate = clientCertificate}); + } + + features.Get().Scheme = "https"; + }; context.Connection = sslStream; } } - - public void PrepareRequest(IFeatureCollection features) - { - _previous.PrepareRequest(features); - - if (_clientCert != null) - { - features.Set( - new TlsConnectionFeature { ClientCertificate = _clientCert }); - } - } } } diff --git a/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilterOptions.cs b/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilterOptions.cs index b8d65482c7..e066c17f3c 100644 --- a/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilterOptions.cs +++ b/src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilterOptions.cs @@ -1,6 +1,8 @@ // 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.Security; using System.Security.Authentication; using System.Security.Cryptography.X509Certificates; @@ -16,7 +18,8 @@ namespace Microsoft.AspNet.Server.Kestrel.Https public X509Certificate2 ServerCertificate { get; set; } public ClientCertificateMode ClientCertificateMode { get; set; } - public ClientCertificateValidationCallback ClientCertificateValidation { get; set; } + public Func ClientCertificateValidation { get; set; } public SslProtocols SslProtocols { get; set; } + public bool CheckCertificateRevocation { get; set; } } } diff --git a/src/Microsoft.AspNet.Server.Kestrel/Filter/ConnectionFilterContext.cs b/src/Microsoft.AspNet.Server.Kestrel/Filter/ConnectionFilterContext.cs index 278ca58254..fc4f0384c6 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Filter/ConnectionFilterContext.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Filter/ConnectionFilterContext.cs @@ -1,6 +1,7 @@ // 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 Microsoft.AspNet.Http.Features; @@ -10,5 +11,6 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter { public ServerAddress Address { get; set; } public Stream Connection { get; set; } + public Action PrepareRequest { get; set; } } } diff --git a/src/Microsoft.AspNet.Server.Kestrel/Filter/IConnectionFilter.cs b/src/Microsoft.AspNet.Server.Kestrel/Filter/IConnectionFilter.cs index 8c4f827be9..bc08d959ff 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Filter/IConnectionFilter.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Filter/IConnectionFilter.cs @@ -9,6 +9,5 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter public interface IConnectionFilter { Task OnConnection(ConnectionFilterContext context); - void PrepareRequest(IFeatureCollection features); } } diff --git a/src/Microsoft.AspNet.Server.Kestrel/Filter/NoOpConnectionFilter.cs b/src/Microsoft.AspNet.Server.Kestrel/Filter/NoOpConnectionFilter.cs index aac76ec34e..95ecc7a50a 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Filter/NoOpConnectionFilter.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Filter/NoOpConnectionFilter.cs @@ -13,8 +13,5 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter { return TaskUtilities.CompletedTask; } - - public void PrepareRequest(IFeatureCollection features) - {} } } diff --git a/src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs b/src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs index 42d8aeb30a..c2ff59be01 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs @@ -177,7 +177,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Http private Frame CreateFrame() { - return new Frame(this, _remoteEndPoint, _localEndPoint, ConnectionFilter); + return new Frame(this, _remoteEndPoint, _localEndPoint, _filterContext?.PrepareRequest); } void IConnectionControl.Pause() diff --git a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs index 0edaeb3fee..7f1c9767fb 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs @@ -56,22 +56,22 @@ namespace Microsoft.AspNet.Server.Kestrel.Http private readonly IPEndPoint _localEndPoint; private readonly IPEndPoint _remoteEndPoint; - private readonly IConnectionFilter _connectionFilter; + private readonly Action _prepareRequest; public Frame(ConnectionContext context) - : this(context, remoteEndPoint: null, localEndPoint: null, connectionFilter: null) + : this(context, remoteEndPoint: null, localEndPoint: null, prepareRequest: null) { } public Frame(ConnectionContext context, IPEndPoint remoteEndPoint, IPEndPoint localEndPoint, - IConnectionFilter connectionFilter) + Action prepareRequest) : base(context) { _remoteEndPoint = remoteEndPoint; _localEndPoint = localEndPoint; - _connectionFilter = connectionFilter; + _prepareRequest = prepareRequest; FrameControl = this; Reset(); diff --git a/test/Microsoft.AspNet.Server.KestrelTests/ConnectionFilterTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/ConnectionFilterTests.cs index 0af98798cb..5842e1d7c0 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/ConnectionFilterTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/ConnectionFilterTests.cs @@ -95,9 +95,6 @@ namespace Microsoft.AspNet.Server.KestrelTests return _empty; } - public void PrepareRequest(IFeatureCollection frame) - {} - public int BytesRead => _rewritingStream.BytesRead; } @@ -113,9 +110,6 @@ namespace Microsoft.AspNet.Server.KestrelTests context.Connection = new RewritingStream(oldConnection); } - - public void PrepareRequest(IFeatureCollection frame) - {} } private class RewritingStream : Stream diff --git a/test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs index b738d97131..8b766fba8e 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs @@ -122,7 +122,7 @@ namespace Microsoft.AspNet.Server.KestrelTests using (var server = new TestServer(App, serviceContext, serverAddress)) { - using (var client = new HttpClient()) + using (var client = new HttpClient(handler)) { await Assert.ThrowsAnyAsync( () => client.GetAsync(serverAddress)); @@ -137,10 +137,6 @@ namespace Microsoft.AspNet.Server.KestrelTests } } - // https://github.com/dotnet/corefx/issues/4512 - // WinHttpHandler throws an Exception (ERROR_INTERNET_SECURE_FAILURE) -#if DNX451 - // https://github.com/aspnet/KestrelHttpServer/issues/240 // This test currently fails on mono because of an issue with SslStream. [ConditionalFact] @@ -181,7 +177,7 @@ namespace Microsoft.AspNet.Server.KestrelTests using (var server = new TestServer(app, serviceContext, serverAddress)) { - using (var client = new HttpClient()) + using (var client = new HttpClient(handler)) { var result = await client.GetAsync(serverAddress); @@ -196,11 +192,6 @@ namespace Microsoft.AspNet.Server.KestrelTests #endif } } -#endif - - // https://github.com/dotnet/corefx/issues/4510 - // Can't convert X509Certificate to X509Certificate2 in HttpsConnectionFilter -#if DNX451 // https://github.com/aspnet/KestrelHttpServer/issues/240 // This test currently fails on mono because of an issue with SslStream. @@ -269,7 +260,6 @@ namespace Microsoft.AspNet.Server.KestrelTests #endif } } -#endif } }