CertificateFailedValidation event is not logging the ChainErrors as expected.

[jupacaza]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs#L155

In CertificateAuthenticationHandler line 155 you log event:
Logger.CertificateFailedValidation(clientCertificate.Subject, chainErrors);

but the chainErrors logs this:
System.Collections.Generic.List1[System.String]`

You need to log the values of the chainErrors. Otherwise this is not a helpful message.

Expected Behavior

Log the chain errors in the list.

Steps To Reproduce

Call a service using certificate authentication handler with a certificate that has a root that is not installed in the server. This should give chain errors. Check the logs and you'll see the chain errors are not logged properly.

From our service we see this event:
name: Microsoft.AspNetCore.Authentication.Certificate.CertificateAuthenticationHandler
body: Certificate validation failed, subject was {Subject}. {ChainErrors}
subject: CN=scrapped.net
chain errors: System.Collections.Generic.List`1[System.String]

Exceptions (if any)

No response

.NET Version

6.0.301

Anything else?

No response

[blowdart]

@HaoK A ToString() is needed here, or whatever it takes to expand the error list.

[HaoK]

Interested in submitting a PR for this fix @jupacaza ? Should be pretty straightforward, instead of building a list of errors, the code should just be appending to a string builder for the log message

[jupacaza]

@HaoK does this look good? #44503

[martincostello]

I think you could make it work by using List<string> or string[] instead of the enumerable, and then you'd keep the benefit of the array for structured logging.

I've certainly had it behave better with the concrete types when I've come across this before, but I'm not sure if it also works with IList<string>/ICollection<string>.

[HaoK]

What's the difference for structured logging with List/string[] does it automatically display differently than when its a string?

[martincostello]

We use Serilog plugged into ILogger to output to Kibana via LogStash and for string arrays we get fields like this in our log entries:

foo: [ “a”, “b”, “c” ]

With the proposed fix we’d get something like this instead:

foo: a, b, c

[HaoK]

Got it, so you do lose a little bit of nuance here, so the theory is we can just change it to List<string> and we'd keep the behavior you hvae today. That sounds reasonable to me @jupacaza lets go with List<string> since we already have that

[jupacaza]

@martincostello I leave it up to you but are you sure making the logging extension use List as parameter will log the information? Won't it just log something like: System.Collections.Generic.List`1[System.String] which is happening now? The logger is doing list.ToString() implicitly and that's why you get the "System.Collections..." line.