CertificateFailedValidation event is not logging the ChainErrors as expected.

[jupacaza]

Is there an existing issue for this?

• [X] 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? https://github.com/dotnet/aspnetcore/pull/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.

[martincostello]

You’d have to test it to be honest to confirm which types it works for. I’m 99% sure it works with arrays, but I’m not sure as sure of List, and less sure of the interfaces.

Basically I think there’s special-casing for certain types, but enumerable isn’t special-cased as it might be lazy and/or enumerate multiple times if it tried to get the elements to log, which is why it gets the ToString() default.

[jupacaza]

I tried to repro the issue in my own service: created a logger extension with an IEnumerable parameter and log a mock list of errors. Did not repo, it shows in the desired way: the IEnumerable is a list { "Error1", "Error2" } Certificate validation failed for subject 'CN=SCRAPPED'. Error1, Error2.

My logging framework might be different, though.

I will pass an IList<> but can't be sure that works. I'm 100% sure string.Join will work.

[HaoK]

Thanks again for the fix @jupacaza and your input @martincostello

[jupacaza]

Thank @HaoK . If would be great if someone could reproduce the issue and check the fix works, though.

[HaoK]

Just to clarify, @jupacaza you did verified that your fix works for your scenario right? Just not the structured logging scenario via alternate loggers?