From b9518e36840ce5be8e29d82e3f53bbaf0109b9e8 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 10 May 2017 15:29:43 -0700 Subject: [PATCH] Invert the dependency between connection adapters and Frame (#1822) * Invert the dependency between connection adapters and Frame - Removed PrepareRequest from IAdaptedConnection and instead added a feature collection to the ConnectionAdapterContext. This allows features to be set once by the adapter instead of per request. It's the Frame's job to copy features from the connection level feature collection into the per request feature collection. - Set the scheme to "https" based on the presence of ITlsConnectionFeature. - Always set ITlsConnection feature if the HttpsAdaptedConnection doesn't throw during the handshake --- .../Internal/ConnectionAdapterContext.cs | 6 ++++- .../Adapter/Internal/IAdaptedConnection.cs | 3 --- .../Internal/LoggingConnectionAdapter.cs | 4 --- .../Internal/FrameConnection.cs | 26 ++++++++++--------- .../Internal/Http/Frame.cs | 19 +++++++------- .../HttpsConnectionAdapter.cs | 21 +++++---------- .../ConnectionAdapterTests.cs | 5 ---- .../HttpsConnectionAdapterTests.cs | 4 ++- test/shared/PassThroughConnectionAdapter.cs | 4 --- 9 files changed, 37 insertions(+), 55 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/ConnectionAdapterContext.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/ConnectionAdapterContext.cs index 6d074b3a7c..1f87c8311e 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/ConnectionAdapterContext.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/ConnectionAdapterContext.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.IO; +using Microsoft.AspNetCore.Http.Features; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal { @@ -9,11 +10,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal // we want to add more connection metadata later. public class ConnectionAdapterContext { - internal ConnectionAdapterContext(Stream connectionStream) + internal ConnectionAdapterContext(IFeatureCollection features, Stream connectionStream) { + Features = features; ConnectionStream = connectionStream; } + public IFeatureCollection Features { get; } + public Stream ConnectionStream { get; } } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/IAdaptedConnection.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/IAdaptedConnection.cs index 8129cdbb35..5960490e2b 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/IAdaptedConnection.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/IAdaptedConnection.cs @@ -3,14 +3,11 @@ using System; using System.IO; -using Microsoft.AspNetCore.Http.Features; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal { public interface IAdaptedConnection : IDisposable { Stream ConnectionStream { get; } - - void PrepareRequest(IFeatureCollection requestFeatures); } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/LoggingConnectionAdapter.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/LoggingConnectionAdapter.cs index 570075ecb7..10d370df56 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/LoggingConnectionAdapter.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Adapter/Internal/LoggingConnectionAdapter.cs @@ -40,10 +40,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal public Stream ConnectionStream { get; } - public void PrepareRequest(IFeatureCollection requestFeatures) - { - } - public void Dispose() { } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs index 8ca4c36372..b9d7bcfc55 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs @@ -7,6 +7,7 @@ using System.Diagnostics; using System.IO; using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; @@ -20,7 +21,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal { private readonly FrameConnectionContext _context; private readonly Frame _frame; - private readonly List _connectionAdapters; + private List _adaptedConnections; private readonly TaskCompletionSource _socketClosedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); private long _lastTimestamp; @@ -33,7 +34,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal { _context = context; _frame = context.Frame; - _connectionAdapters = context.ConnectionAdapters; } public string ConnectionId => _context.ConnectionId; @@ -78,7 +78,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal var input = _context.Input.Reader; var output = _context.Output; - if (_connectionAdapters.Count > 0) + if (_context.ConnectionAdapters.Count > 0) { adaptedPipeline = new AdaptedPipeline(input, output, @@ -159,17 +159,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal private async Task ApplyConnectionAdaptersAsync() { + var features = new FeatureCollection(); + var connectionAdapters = _context.ConnectionAdapters; var stream = new RawStream(_context.Input.Reader, _context.Output.Writer); - var adapterContext = new ConnectionAdapterContext(stream); - var adaptedConnections = new IAdaptedConnection[_connectionAdapters.Count]; + var adapterContext = new ConnectionAdapterContext(features, stream); + _adaptedConnections = new List(connectionAdapters.Count); try { - for (var i = 0; i < _connectionAdapters.Count; i++) + for (var i = 0; i < connectionAdapters.Count; i++) { - var adaptedConnection = await _connectionAdapters[i].OnConnectionAsync(adapterContext); - adaptedConnections[i] = adaptedConnection; - adapterContext = new ConnectionAdapterContext(adaptedConnection.ConnectionStream); + var adaptedConnection = await connectionAdapters[i].OnConnectionAsync(adapterContext); + _adaptedConnections.Add(adaptedConnection); + adapterContext = new ConnectionAdapterContext(features, adaptedConnection.ConnectionStream); } } catch (Exception ex) @@ -180,7 +182,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal } finally { - _frame.AdaptedConnections = adaptedConnections; + _frame.ConnectionFeatures = features; } return adapterContext.ConnectionStream; @@ -188,10 +190,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal private void DisposeAdaptedConnections() { - var adaptedConnections = _frame.AdaptedConnections; + var adaptedConnections = _adaptedConnections; if (adaptedConnections != null) { - for (int i = adaptedConnections.Length - 1; i >= 0; i--) + for (int i = adaptedConnections.Count - 1; i >= 0; i--) { adaptedConnections[i].Dispose(); } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs index 6a85d64757..52ea21fb96 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs @@ -12,7 +12,6 @@ using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Internal.System; using Microsoft.AspNetCore.Server.Kestrel.Internal.System.IO.Pipelines; @@ -21,6 +20,7 @@ using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; +using Microsoft.AspNetCore.Http.Features; // ReSharper disable AccessToModifiedClosure @@ -103,7 +103,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http public IPipeReader Input { get; set; } public OutputProducer Output { get; set; } - public IAdaptedConnection[] AdaptedConnections { get; set; } + public IFeatureCollection ConnectionFeatures { get; set; } public ITimeoutControl TimeoutControl { get; set; } protected IKestrelTrace Log => ServiceContext.Log; @@ -349,18 +349,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http RequestHeaders = FrameRequestHeaders; ResponseHeaders = FrameResponseHeaders; - if (AdaptedConnections != null) + if (ConnectionFeatures != null) { - try + foreach (var feature in ConnectionFeatures) { - foreach (var adaptedConnection in AdaptedConnections) + // Set the scheme to https if there's an ITlsConnectionFeature + if (feature.Key == typeof(ITlsConnectionFeature)) { - adaptedConnection.PrepareRequest(this); + Scheme = "https"; } - } - catch (Exception ex) - { - Log.LogError(0, ex, $"Uncaught exception from the {nameof(IAdaptedConnection.PrepareRequest)} method of an {nameof(IAdaptedConnection)}."); + + FastFeatureSet(feature.Key, feature.Value); } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionAdapter.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionAdapter.cs index 7c3bca81a3..328f4263aa 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionAdapter.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionAdapter.cs @@ -109,6 +109,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https return _closedAdaptedConnection; } + // Always set the feature even though the cert might be null + context.Features.Set(new TlsConnectionFeature + { + ClientCertificate = (X509Certificate2)sslStream.RemoteCertificate + }); + return new HttpsAdaptedConnection(sslStream); } @@ -123,17 +129,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https public Stream ConnectionStream => _sslStream; - public void PrepareRequest(IFeatureCollection requestFeatures) - { - var clientCertificate = (X509Certificate2)_sslStream.RemoteCertificate; - if (clientCertificate != null) - { - requestFeatures.Set(new TlsConnectionFeature { ClientCertificate = clientCertificate }); - } - - requestFeatures.Get().Scheme = "https"; - } - public void Dispose() { _sslStream.Dispose(); @@ -144,10 +139,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Https { public Stream ConnectionStream { get; } = new ClosedStream(); - public void PrepareRequest(IFeatureCollection requestFeatures) - { - } - public void Dispose() { } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConnectionAdapterTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConnectionAdapterTests.cs index eb5a857358..7c85fe99c5 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConnectionAdapterTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConnectionAdapterTests.cs @@ -7,7 +7,6 @@ using System.Net; using System.Net.Sockets; using System.Threading; using System.Threading.Tasks; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal; using Microsoft.AspNetCore.Testing; @@ -218,10 +217,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests public Stream ConnectionStream { get; } - public void PrepareRequest(IFeatureCollection requestFeatures) - { - } - public void Dispose() { } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsConnectionAdapterTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsConnectionAdapterTests.cs index e4d6e0eb22..eb4b7976d1 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsConnectionAdapterTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsConnectionAdapterTests.cs @@ -94,7 +94,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests using (var server = new TestServer(context => { - Assert.Equal(context.Features.Get(), null); + var tlsFeature = context.Features.Get(); + Assert.NotNull(tlsFeature); + Assert.Null(tlsFeature.ClientCertificate); return context.Response.WriteAsync("hello world"); }, serviceContext, listenOptions)) diff --git a/test/shared/PassThroughConnectionAdapter.cs b/test/shared/PassThroughConnectionAdapter.cs index 7d6fdd8222..c018e49ad7 100644 --- a/test/shared/PassThroughConnectionAdapter.cs +++ b/test/shared/PassThroughConnectionAdapter.cs @@ -27,10 +27,6 @@ namespace Microsoft.AspNetCore.Testing public Stream ConnectionStream { get; } - public void PrepareRequest(IFeatureCollection requestFeatures) - { - } - public void Dispose() { }