ServerCertificateSelector callback needs to be async, otherwise any async work in callback may cause deadlock with threadpool (eg. logging failure to disk).

[rickyburrell]

We have a kestrel based production load balancer edge hosted on large site.

When malicious clients open many https connections based on invalid host names, no cert is available and we async log this failure to W3C log file.

As the callback method is not async, we end up blocking most of the thread pool threads waiting for async logging to be scheduled and starvation occurs.

Surely it is easy enough to make this callback async?

[analogrelay]

Yep, this is something we’re actively looking at. The callback itself actually lives in the ru time though. @karelz @wfurt is there a bug tracking that work on the runtime?

For your use case though (it sounds like you’re logging?), I’d suggest something like a fire-and-forget async pattern or using a queue and background worker rather than blocking the callback inline. Even with an async callback, you wouldn’t want to keep the connection open while you’re logging. If the callback is async and you put your logging code inline in there, Kestrel won’t be able to release the connection until the callback completes fully.

A fire-and-forget pattern means using something like Task.Run, or calling an async method without awaiting the task. The critical part is not blocking the current thread on the result. If you do that, it’s important to make sure the method you fire-and-forget has exception handling and logging built in because if it throws an exception, it won’t be logged by Kestrel since it happened outside the request/connection scope. This could still cause some thread-pool pressure in a scenario with a lot of malicious requests though.

A background queue worker approach would mean synchronously adding a message to a queue in the callback and having a background task using IHostedService come along to periodically write out all the queued log messages. The trade-off here is that you have to buffer all these messages in memory until the background task can come along. It might be best to have a limit to the number of queued messages and just drop messages if the buffer is full (since you probably don’t need every single log message if you’re being stormed by malicious clients). You could also just track a count of bad requests (perhaps by host name), which would take a lot less memory usage.

[wfurt]

I did not find any issue just describing the async version. As @anurse mentioned it is on 5.0 TODO list and will be probably done as part of dotnet/runtime#31097. If it helps we can create separate issue just for the Async version.

[analogrelay]

I’m not worried about how it’s tracked in the runtime end @wfurt . I think we should keep this issue though since there’s a small amount of work Kestrel will need to do to expose the new callback.

[rickyburrell]

Hi Andrew, thanks for the feedback. We have already implemented a workaround as suggested, but the ideal solution would require that the error is logged: at the same time; and as a part of the connection termination process; and that back-pressure is exerted. We would appreciate any work done here - looking forward to this feature in a future version.

[analogrelay]

Triage: Blocked by the runtime change to add this to SslStream. Once we have that, we'll need to do a small amount of work to expose it.