search

AlanBarber / NLog.Targets.Splunk

A NLog target for Splunk Http Event Collector (HEC) Sender
Apache License 2.0
9 stars 24 forks source link

Introduced IgnoreSslErrors property #15

Closed snakefoot closed 6 years ago

snakefoot commented 6 years ago

@initech-asynchrony @bruceharrison1984 Since you have shown to compile and make local changes, then you try and test if this PR ensures error-logging is working and that ssl-certificate-errors can be ignored (when IgnoreSslErrors = true)

snakefoot commented 6 years ago

Since IgnoreSslErrors is a very special for you only. Then one might consider not having the extra dependency on System.Net.Http.WebRequest.

Instead just provide the ability to assign a delegate Func<HttpMessageHandler>. Could be a static-member on the NLog-target. When assigned it will create a HttpClient with the returned HttpMessageHandler. An even better solution would be to have a it as a non-static property on the NLog-target.

Then the custom code for IgnoreSslErrors could be moved into your solution, and not clutter NLog.Targets.Splunk (Removing IgnoreSslErrors competely).

bruceharrison1984 commented 6 years ago

This PR fixes our problems, but I would like to re-iterate that ignoreSSL has a number of valid use cases beyond my particular situation.

snakefoot commented 6 years ago

@AlanBarber I guess it is your decision whether IgnoreSslErrors should be official part of the API, or if it should be custom extensions.

I guess if we accepted Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> as custom parameter (Instead of using IgnoreSslErrors). Then it would be possible for NetCoreApps to provide a direct reference to the static HttpClientHandler.DangerousAcceptAnyServerCertificateValidator, thus this ReferenceEquals would work (Also for MacOSx):

https://github.com/dotnet/corefx/blob/fe63206856998c223adb0240a1e57201b6cfd467/src/System.Net.Http/src/System/Net/Http/CurlHandler/CurlHandler.EasyRequest.cs#L773

AlanBarber commented 6 years ago

I'm fine adding an IgnoreSslErrors option to the API... I actually originally implemented a version of it for my log4net provider but when I converted to .NET Standard I had to remove it because there was no support at the time.

snakefoot commented 6 years ago

The always ignoreSslErrors = true is just me opening for the ability to have other future reasons to call BuildHttpMessageHandler . Ex maybe want to restrict to only use Tsl12

AlanBarber commented 6 years ago

@snakefoot Gotcha sounds good... i'm going to get an RC pushed up to nuget now