W3CLogger Custom Fields

[dustinsoftware]

Background and Motivation

W3CLogger supports logging many pieces of the processed requests, including some HTTP headers. We'd like the ability to pass in a list of additional headers to be logged, which will be logged after all of the LoggingFields have been written.

Example:

#Fields: date time c-ip cs-username s-ip s-port cs-method cs-uri-stem cs-uri-query sc-status time-taken cs-host cs(User-Agent) cs(Referer) cs(x-forwarded-for) cs(cf-connecting-ip) cs(x-client-ssl-protocol)

2022-05-13 15:28:37 1.1.1.1 - 1.1.1.2 8888 POST /api/foo/28F83FE6-14F8-480B-A82B-C10DAFE4B7AF/ - 200 25.2435 foo.example.com k6/0.29.0+(https://k6.io/) - 1.1.1.3,+1.1.1.4 1.1.1.5 TLSv1.2

Proposed API

diff --git a/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt b/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt
index 7dc5c58110..f2aee72e7b 100644
--- a/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt
+++ b/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt
@@ -1 +1,3 @@
 #nullable enable
+Microsoft.AspNetCore.HttpLogging.W3CLoggerOptions.AdditionalRequestHeaders.get -> System.Collections.Generic.IReadOnlyList<string!>?
+Microsoft.AspNetCore.HttpLogging.W3CLoggerOptions.AdditionalRequestHeaders.set -> void

diff --git a/src/Middleware/HttpLogging/src/W3CLoggerOptions.cs b/src/Middleware/HttpLogging/src/W3CLoggerOptions.cs
index bb9406eb73..790a5abd59 100644
--- a/src/Middleware/HttpLogging/src/W3CLoggerOptions.cs
+++ b/src/Middleware/HttpLogging/src/W3CLoggerOptions.cs
@@ -1,6 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics.CodeAnalysis;
+
 namespace Microsoft.AspNetCore.HttpLogging;
 
 /// <summary>
@@ -106,6 +108,12 @@ public sealed class W3CLoggerOptions
         }
     }
 
+    /// <summary>
+    /// List of additional header values to log.
+    /// </summary>
+    [AllowNull]
+    public IReadOnlyList<string>? AdditionalRequestHeaders { get; set; }
+
     /// <summary>
     /// Fields to log. Defaults to logging request and response properties and headers,
     /// plus date/time info and server name.

Usage Examples

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddW3CLogging(logging =>
        {
            // Log all W3C fields
            logging.LoggingFields = W3CLoggingFields.All;
            logging.AdditionalRequestHeaders = new[] {
                "x-forwarded-for",
                "cf-connecting-ip",
                "x-client-ssl-protocol",
            };
        });
    }

Alternative Designs

Some of this functionality is supported in IIS logging:
https://docs.microsoft.com/en-us/iis/configuration/system.applicationhost/sites/site/logfile/customfields/

<add logFieldName="X-Forwarded-For" sourceName="X_FORWARDED_FOR" sourceType="RequestHeader" />

An earlier iteration of this proposal had support for custom field names:

        services.AddW3CLogging(logging =>
        {
            // Log all W3C fields
            logging.LoggingFields = W3CLoggingFields.All;
            logging.AdditionalRequestHeaders = new[] {
                ("x-forwarded-for", "X_FORWARDED_FOR"),
                ("cf-connecting-ip", "CF_CONNECTING_IP"),
                ("x-client-ssl-protocol", "X_CLIENT_SSL_PROTOCOL"),
            };
        });

Risks

It should be fine if future additions to W3CLoggingFields are made, as long as the custom fields are always rendered afterwards. Consumers of this library will need to opt-in to custom logging anyway.

It should probably be discouraged to use W3CLoggingFields.All in combination with custom headers, since a package update could add an additional field and disrupt the order of parsing custom fields (based on string position) by downstream tools.

[adityamandaleeka]

This seems like a reasonable request.

@blowdart Any concerns?

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[blowdart]

Off by default, and with dire warnings in documentations please

[dustinsoftware]

PR opened in #41753

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review notes:

1. If we add response headers later, could we distinguish?
   Yes because of the cs/sc field name prefixes.
2. Can we make this option more generic to support more field types like response headers?
   Maybe if we parsed prefixes like cs and sc, but that seems complicated. We could add a ResponseHeaders option in the future if we want.
3. Settable IReadOnlyList<string>??
   Even though it's an IReadOnlyList, we'd still have to clone the headers for safety. Let's just make it a get-only ISet<string>. That way we can throw after the server has started logging if someone tries to mutate the now read-only ISet.
4. Should we do the alternative proposal that lets you control the column name for a given field like IIS.
   It seems like the spect want you to use (cs){lowercase request header name} specifically. We don't let you control other column names, so we so need to offer that capability for request headers.

namespace Microsoft.AspNetCore.HttpLogging;

public sealed class W3CLoggerOptions
{
+    /// <summary>
+    /// Barry approved summary mentioning security implications
+    /// </summary>
+    public ISet<string> AdditionalRequestHeaders { get; }

This isn't API, but we feel AdditionalRequestHeaders should be merged with any request headers specified via W3CLoggingFields. It should not be an error to specify "Cookies" in both places for example.

[dustinsoftware]

Thanks. The API review notes make sense to me, I'll proceed with updating the PR using public ISet<string> AdditionalRequestHeaders { get; }

This isn't API, but we feel AdditionalRequestHeaders should be merged with any request headers specified via W3CLoggingFields. It should not be an error to specify "Cookies" in both places for example.

That works, I see there are currently these flags we check for: HeaderNames.Host, HeaderNames.Referer, HeaderNames.UserAgent, HeaderNames.Cookie. If LoggingFields.HasFlag is true for any of those, and the header was specified as an additional header, it will be ignored as an additional header (so the order of Cookie would always appear before any AdditionalRequestHeaders, for example)

[dustinsoftware]

A follow up proposal has been added for response headers in #42208