serilog-contrib / Serilog.Enrichers.Sensitive

A Serilog LogEvent enricher that masks sensitive data
MIT License
111 stars 23 forks source link

Masking does not work for HTTP messages logged from app.UseHttpLogging() in .NET 6 #28

Closed BainsDev closed 1 year ago

BainsDev commented 1 year ago

When using the .NET middleware to log HTTP messages in an ASP.NET Core Web API project, the Serilog masking settings are ignored for the logged messages.

Messages originating from Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware all ignore the masking settings.

Messages logged with Serilog explicitly using the Serilog logger are masked correctly.

Example to reproduce with:

using Serilog;
using Serilog.Enrichers.Sensitive;
using System.Xml.Linq;

Log.Logger = new LoggerConfiguration()
    .Enrich.WithSensitiveDataMasking(options => options.MaskProperties.Add("name"))
    .WriteTo.Console()
    .CreateLogger();

try
{
    Log.Information("Starting web application");

    var builder = WebApplication.CreateBuilder(args);

    builder.Host.UseSerilog();
    builder.Services.AddHttpLogging(settings =>
    {
        settings.LoggingFields = Microsoft.AspNetCore.HttpLogging.HttpLoggingFields.All;
    }
    );
    var app = builder.Build();

    app.UseHttpLogging();

    Log.Information("Test {@test}", new { name = "test" }); // gets masked

    app.MapGet("/", () => new {name = "test"}); // doesn't get masked

    app.Run();
}
catch (Exception ex)
{
    Log.Fatal(ex, "Application terminated unexpectedly");
}
finally
{
    Log.CloseAndFlush();
}
sandermvanvliet commented 1 year ago

Thank you for reporting this, I’ll have a look to see what’s going on

sandermvanvliet commented 1 year ago

I've just run a repro of this and it's actually expected that this doesn't work. The property name is on the body, not the log message. That means that there is no trigger to mask the payload being logged.

In the screenshot below you can see the captured log event and this shows the properties of that log event does not contain a name property: image

In order to mask data in the payload body you'd need to create a separate masking operator that knows how this particular case.

A very inefficient approach would be:

public class PayloadMaskingOperator : IMaskingOperator
{
    public MaskingResult Mask(string input, string mask)
    {
        var trimmedInput = input.Trim();

        var didMask = false;

        // Check if the input is a serialized JSON object
        if (trimmedInput.StartsWith("{") && trimmedInput.EndsWith("}"))
        {
            JObject jsonObject;
            try
            {
                jsonObject = JObject.Parse(input);
            }
            catch (JsonReaderException)
            {
                // Not JSON
                return MaskingResult.NoMatch;
            }

            didMask = MaskNode(mask, jsonObject, didMask);

            if (!didMask)
            {
                return MaskingResult.NoMatch;
            }

            return new MaskingResult
            {
                Match = true,
                Result = jsonObject.ToString()
            };
        }

        return MaskingResult.NoMatch;
    }

    private static bool MaskNode(string mask, JObject jsonObject, bool didMask)
    {
        foreach (var node in jsonObject)
        {
            if (node.Key == "name")
            {
                jsonObject[node.Key] = mask;
                didMask = true;
            }
            else if (node.Value.Children().OfType<JObject>().Any())
            {
                foreach (var child in node.Value.Children().OfType<JObject>())
                {
                    MaskNode(mask, child, didMask);
                }
            }
        }

        return didMask;
    }
}

which results in: image

So it is possible, but it'll take some work to do properly.

Also, I would typically not suggest to log payloads at all given that they can be fairly large.

BainsDev commented 1 year ago

Ah, thanks for the detailed explanation and the code sample. Will close this for now as it's not an issue :).