From 37e6ad7e127b24bd4a10ee9cb3d9c52661c38158 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 10 Jun 2019 17:32:00 +0100 Subject: [PATCH] Improve HostFiltering perf (#9359) --- src/Http/Http.Abstractions/src/HostString.cs | 3 +- src/Http/Http/src/HeaderDictionary.cs | 2 +- .../src/HostFilteringMiddleware.cs | 78 ++++++++++++------- .../HostFiltering/src/LoggerExtensions.cs | 40 ++++++++++ .../test/HostFilteringMiddlewareTests.cs | 4 +- src/Mvc/Mvc.Core/src/ConsumesAttribute.cs | 4 +- .../src/Formatters/TextInputFormatter.cs | 2 +- 7 files changed, 100 insertions(+), 33 deletions(-) create mode 100644 src/Middleware/HostFiltering/src/LoggerExtensions.cs diff --git a/src/Http/Http.Abstractions/src/HostString.cs b/src/Http/Http.Abstractions/src/HostString.cs index 5b83251ca3..15e0071a27 100644 --- a/src/Http/Http.Abstractions/src/HostString.cs +++ b/src/Http/Http.Abstractions/src/HostString.cs @@ -247,7 +247,8 @@ namespace Microsoft.AspNetCore.Http } } - for (int i = 0; i < patterns.Count; i++) + var count = patterns.Count; + for (int i = 0; i < count; i++) { var pattern = patterns[i]; diff --git a/src/Http/Http/src/HeaderDictionary.cs b/src/Http/Http/src/HeaderDictionary.cs index 2113204823..4383020254 100644 --- a/src/Http/Http/src/HeaderDictionary.cs +++ b/src/Http/Http/src/HeaderDictionary.cs @@ -74,7 +74,7 @@ namespace Microsoft.AspNetCore.Http } ThrowIfReadOnly(); - if (StringValues.IsNullOrEmpty(value)) + if (value.Count == 0) { Store?.Remove(key); } diff --git a/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs b/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs index 0404d38538..d791b5c929 100644 --- a/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs +++ b/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; using System.Text; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; @@ -68,19 +69,13 @@ namespace Microsoft.AspNetCore.HostFiltering if (!CheckHost(context, allowedHosts)) { - context.Response.StatusCode = 400; - if (_options.IncludeFailureMessage) - { - context.Response.ContentLength = DefaultResponse.Length; - context.Response.ContentType = "text/html"; - return context.Response.Body.WriteAsync(DefaultResponse, 0, DefaultResponse.Length); - } - return Task.CompletedTask; + return HostValidationFailed(context); } return _next(context); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private IList EnsureConfigured() { if (_allowAnyNonEmptyHost == true || _allowedHosts?.Count > 0) @@ -88,10 +83,28 @@ namespace Microsoft.AspNetCore.HostFiltering return _allowedHosts; } + return Configure(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private Task HostValidationFailed(HttpContext context) + { + context.Response.StatusCode = 400; + if (_options.IncludeFailureMessage) + { + context.Response.ContentLength = DefaultResponse.Length; + context.Response.ContentType = "text/html"; + return context.Response.Body.WriteAsync(DefaultResponse, 0, DefaultResponse.Length); + } + return Task.CompletedTask; + } + + private IList Configure() + { var allowedHosts = new List(); if (_options.AllowedHosts?.Count > 0 && !TryProcessHosts(_options.AllowedHosts, allowedHosts)) { - _logger.LogDebug("Wildcard detected, all requests with hosts will be allowed."); + _logger.WildcardDetected(); _allowedHosts = allowedHosts; _allowAnyNonEmptyHost = true; return _allowedHosts; @@ -104,7 +117,7 @@ namespace Microsoft.AspNetCore.HostFiltering if (_logger.IsEnabled(LogLevel.Debug)) { - _logger.LogDebug("Allowed hosts: {Hosts}", string.Join("; ", allowedHosts)); + _logger.AllowedHosts(string.Join("; ", allowedHosts)); } _allowedHosts = allowedHosts; @@ -142,37 +155,50 @@ namespace Microsoft.AspNetCore.HostFiltering } // This does not duplicate format validations that are expected to be performed by the host. + [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool CheckHost(HttpContext context, IList allowedHosts) { - var host = new StringSegment(context.Request.Headers[HeaderNames.Host].ToString()).Trim(); + var host = context.Request.Headers[HeaderNames.Host].ToString(); - if (StringSegment.IsNullOrEmpty(host)) + if (host.Length == 0) { - // Http/1.0 does not require the host header. - // Http/1.1 requires the header but the value may be empty. - if (!_options.AllowEmptyHosts) - { - _logger.LogInformation("{Protocol} request rejected due to missing or empty host header.", context.Request.Protocol); - return false; - } - _logger.LogDebug("{Protocol} request allowed with missing or empty host header.", context.Request.Protocol); - return true; + return IsEmptyHostAllowed(context); } if (_allowAnyNonEmptyHost == true) { - _logger.LogTrace("All hosts are allowed."); + _logger.AllHostsAllowed(); return true; } - if (HostString.MatchesAny(host, allowedHosts)) + return CheckHostInAllowList(allowedHosts, host); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private bool CheckHostInAllowList(IList allowedHosts, string host) + { + if (HostString.MatchesAny(new StringSegment(host), allowedHosts)) { - _logger.LogTrace("The host '{Host}' matches an allowed host.", host); + _logger.AllowedHostMatched(host); return true; } - - _logger.LogInformation("The host '{Host}' does not match an allowed host.", host); + + _logger.NoAllowedHostMatched(host); return false; } + + [MethodImpl(MethodImplOptions.NoInlining)] + private bool IsEmptyHostAllowed(HttpContext context) + { + // Http/1.0 does not require the host header. + // Http/1.1 requires the header but the value may be empty. + if (!_options.AllowEmptyHosts) + { + _logger.RequestRejectedMissingHost(context.Request.Protocol); + return false; + } + _logger.RequestAllowedMissingHost(context.Request.Protocol); + return true; + } } } diff --git a/src/Middleware/HostFiltering/src/LoggerExtensions.cs b/src/Middleware/HostFiltering/src/LoggerExtensions.cs new file mode 100644 index 0000000000..34321555d0 --- /dev/null +++ b/src/Middleware/HostFiltering/src/LoggerExtensions.cs @@ -0,0 +1,40 @@ +// 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 Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.HostFiltering +{ + internal static class LoggerExtensions + { + private static readonly Action _wildcardDetected = + LoggerMessage.Define(LogLevel.Debug, new EventId(0, nameof(WildcardDetected)), "Wildcard detected, all requests with hosts will be allowed."); + + private static readonly Action _allowedHosts = + LoggerMessage.Define(LogLevel.Debug, new EventId(1, nameof(AllowedHosts)), "Allowed hosts: {Hosts}"); + + private static readonly Action _allHostsAllowed = + LoggerMessage.Define(LogLevel.Trace, new EventId(2, nameof(AllHostsAllowed)), "All hosts are allowed."); + + private static readonly Action _requestRejectedMissingHost = + LoggerMessage.Define(LogLevel.Information, new EventId(3, nameof(RequestRejectedMissingHost)), "{Protocol} request rejected due to missing or empty host header."); + + private static readonly Action _requestAllowedMissingHost = + LoggerMessage.Define(LogLevel.Debug, new EventId(4, nameof(RequestAllowedMissingHost)), "{Protocol} request allowed with missing or empty host header."); + + private static readonly Action _allowedHostMatched = + LoggerMessage.Define(LogLevel.Trace, new EventId(5, nameof(AllowedHostMatched)), "The host '{Host}' matches an allowed host."); + + private static readonly Action _noAllowedHostMatched = + LoggerMessage.Define(LogLevel.Information, new EventId(6, nameof(NoAllowedHostMatched)), "The host '{Host}' does not match an allowed host."); + + public static void WildcardDetected(this ILogger logger) => _wildcardDetected(logger, null); + public static void AllowedHosts(this ILogger logger, string allowedHosts) => _allowedHosts(logger, allowedHosts, null); + public static void AllHostsAllowed(this ILogger logger) => _allHostsAllowed(logger, null); + public static void RequestRejectedMissingHost(this ILogger logger, string protocol) => _requestRejectedMissingHost(logger, protocol, null); + public static void RequestAllowedMissingHost(this ILogger logger, string protocol) => _requestAllowedMissingHost(logger, protocol, null); + public static void AllowedHostMatched(this ILogger logger, string host) => _allowedHostMatched(logger, host, null); + public static void NoAllowedHostMatched(this ILogger logger, string host) => _noAllowedHostMatched(logger, host, null); + } +} diff --git a/src/Middleware/HostFiltering/test/HostFilteringMiddlewareTests.cs b/src/Middleware/HostFiltering/test/HostFilteringMiddlewareTests.cs index ab45bc9554..921202dbd3 100644 --- a/src/Middleware/HostFiltering/test/HostFilteringMiddlewareTests.cs +++ b/src/Middleware/HostFiltering/test/HostFilteringMiddlewareTests.cs @@ -80,14 +80,14 @@ namespace Microsoft.AspNetCore.HostFiltering { app.Use((ctx, next) => { - ctx.Request.Headers[HeaderNames.Host] = " "; + ctx.Request.Headers[HeaderNames.Host] = ""; return next(); }); app.UseHostFiltering(); app.Run(c => { Assert.True(c.Request.Headers.TryGetValue(HeaderNames.Host, out var host)); - Assert.True(StringValues.Equals(" ", host)); + Assert.True(StringValues.Equals("", host)); return Task.CompletedTask; }); app.Run(c => Task.CompletedTask); diff --git a/src/Mvc/Mvc.Core/src/ConsumesAttribute.cs b/src/Mvc/Mvc.Core/src/ConsumesAttribute.cs index d66cf981d1..12afb98c32 100644 --- a/src/Mvc/Mvc.Core/src/ConsumesAttribute.cs +++ b/src/Mvc/Mvc.Core/src/ConsumesAttribute.cs @@ -79,7 +79,7 @@ namespace Microsoft.AspNetCore.Mvc // // Requests without a content type do not return a 415. It is a common pattern to place [Consumes] on // a controller and have GET actions - if (requestContentType != null && !IsSubsetOfAnyContentType(requestContentType)) + if (!string.IsNullOrEmpty(requestContentType) && !IsSubsetOfAnyContentType(requestContentType)) { context.Result = new UnsupportedMediaTypeResult(); } @@ -127,7 +127,7 @@ namespace Microsoft.AspNetCore.Mvc // In case there is a single candidate with a constraint it should be selected. // If there are multiple actions with consumes action constraints this should result in ambiguous exception // unless there is another action without a consumes constraint. - if (requestContentType == null) + if (string.IsNullOrEmpty(requestContentType)) { var isActionWithoutConsumeConstraintPresent = context.Candidates.Any( candidate => candidate.Constraints == null || diff --git a/src/Mvc/Mvc.Core/src/Formatters/TextInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/TextInputFormatter.cs index cf104bacbc..e30c5850db 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/TextInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/TextInputFormatter.cs @@ -91,7 +91,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } var requestContentType = context.HttpContext.Request.ContentType; - var requestMediaType = requestContentType == null ? default(MediaType) : new MediaType(requestContentType); + var requestMediaType = string.IsNullOrEmpty(requestContentType) ? default : new MediaType(requestContentType); if (requestMediaType.Charset.HasValue) { // Create Encoding based on requestMediaType.Charset to support charset aliases and custom Encoding