Server certificate validation #194
Conversation
|
|
| /// Disable any certificate validation. Do not use this in production. | ||
| /// </summary> | ||
| /// <param name="config"></param> | ||
| [Obsolete("This will open the client to Man-in-Middle attacks. It should never be used in production.")] |
There was a problem hiding this comment.
I'm not sure this should be obsolete, I think this still needs to be the default behavior and we update the configuration guide on how to better handle this.
There was a problem hiding this comment.
To be honest, I will not change this. I just don't want my name next to an insecure default. If someone needs to troubleshoot the integration, there is this default implementation for things like testing or just getting it to run locally, but this must not be used in production without even thinking about the consequences.
If some user really has the requirement and a good justification he may use this method and supress the warning, or just implement his own version of I don't care.
I know this will be a breaking change and should be mentioned clearly in the release notes.
There was a problem hiding this comment.
We literally DO NOT have a choice in the matter. We can't use this in the default client because the cert will eventually expire and all deployed clients will all of a sudden not be able to submit events. It is not an option for us to go and tell users to update all of their software that makes use of the Exceptionless client whenever our SSL cert expires.
There was a problem hiding this comment.
All clients using exceptionless.io have no problem as long as exceptionless.io has a valid certificate (which it should).
All of these extension methods are more or less dirty workarounds for users who a hosting their own exceptionless without a valid (or otherwise trusted) certificate.
Since exceptions may always contain sensitive and security critical data, I think running your own exceptionless instance without a proper sense of security should not be supported in any way.
There was a problem hiding this comment.
Ahh. I guess I was thinking about the thumbprint setting. The default would do normal trusted root certificate validation and this is just for people to use a self-signed cert. Sorry for the confusion.
| //handler.ServerCertificateCustomValidationCallback = delegate { return true; }; | ||
| var callback = config.ServerCertificateValidationCallback; | ||
| if (callback != null) { | ||
| #if NET45 |
There was a problem hiding this comment.
Can we make this into dedicated methods. I know local functions are compiler sugar but it can cause runtime issues on say unmatched windows 7 machines (at least that's what I've found with other c# new features).
| return callback(certData); | ||
| } | ||
|
|
||
| handler.ServerCertificateCustomValidationCallback = Validate; |
There was a problem hiding this comment.
We have to be very careful here. Can you see if the supported flag was added here (https://github.com/dotnet/corefx/issues/9728). Please see this thread for more information on why this has been dangerous on non full framework.
There was a problem hiding this comment.
This should be fixed with 2.1 as curl was replaced by SocketsHttpHandler (https://blogs.msdn.microsoft.com/dotnet/2018/04/11/announcing-net-core-2-1-preview-2/#user-content-sockets-performance-and-socketshttphandler). I'm unsure how to detect this.
There was a problem hiding this comment.
Our clients are compiled for .net Standard 1.2 and above, we can't guarantee that our clients are running 2.1 and need to be safe.
There was a problem hiding this comment.
The handler will only be installed if the user used one of the extension methods or implemented his own callback. Since this is currently also not supported for netcore version relying on curl this will not break anything.
I guess it should be mentioned in the release notes that there may be a problem with mac or linux on older netcore versions, but I think there is nothing you or I can do about this bug.
The previous implementation also required valid certificates on curl based netcore, so the behaviour is the same.
| @@ -167,6 +157,20 @@ public class DefaultSubmissionClient : ISubmissionClient, IDisposable { | |||
| return client; | |||
| } | |||
|
|
|||
| #if !PORTABLE && !NETSTANDARD1_2 | |||
| #if NET45 | |||
There was a problem hiding this comment.
Let's just do this for net45 and NETSTANDARD 2_0 as <2.0 might be on core 1.1.
There was a problem hiding this comment.
Can you tell me why? What is so special about core 1.1, that this feature should not be available?
The API is available from netstandard 1.3, supported since core 1.0 and hasn't changed since.
There was a problem hiding this comment.
Because it's part of the api in .net core 1.1 but broken in the runtime and this will cause all kinds of issues on non windows machines. We need to only include this on full framework or net standard 2.0 builds. If you could make this change that would be greatly appreciated and we could continue with merging this change in. I'm starting to work on testing for the next release and this could still make it in.
|
Looks good, lets only include these changes on NET45 and .NET Standard 2.0 |
|
Thanks for the PR |
|
were going to update our requirements on .net core 1.0 on plat and accept this change as is. Thanks for the pr and work for us. v4.3.1 is going to roll out shortly. |
fixes #192