Bug in MqttTcpChannel (.NET Framework/NetStandard1.3)

[rydergillen-compacSort]

I ran into a issue when trying to connect via SSL with a Self-Signed Cert. I needed to override the validation function to always return true for self-signed certs. In previous versions of .NET I could use

ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, errors) => { return true; };

In dotnet CORE this is ignored. My only remaining option is to call the SslStream ctor overload that accepts a validation function

public SslStream(Stream innerStream, bool leaveInnerStreamOpen, RemoteCertificateValidationCallback userCertificateValidationCallback)

I implemented a custom MqttClientFactory that in turn called public MqttTcpChannel(Socket socket, SslStream sslStream)

This allowed me to pass in the custom validation logic. However when I ran the code the certificate was still being flagged as invalid. Upon digging into the code I noticed

public async Task ConnectAsync(MqttClientOptions options)

Ignores this value... by always overwriting it with _sslStream = new SslStream(new NetworkStream(_socket, true));

if (options.TlsOptions.UseTls) { _sslStream = new SslStream(new NetworkStream(_socket, true)); await _sslStream.AuthenticateAsClientAsync(options.Server, LoadCertificates(options), SslProtocols.Tls12, options.TlsOptions.CheckCertificateRevocation); }

This does not appear to be 100% correct, I see that one overload is intended for SERVER opposed to CLIENT channel. However I don't think having ConnectAsync stomp on the value set in the ctor is correct. Its also not intuitive that only a specific overload must be used depending on which end of the channel is communicating.

I would be happy to submit a pull request. I'm thinking that Client/Server need separate implementations of IMqttCommunicationChannel OR marking one as internal (this may not work without the ugly InternalVisibleToAttribute)

Another approach is to just expose the validation delegate as a property on MqttClientOptions. This seems hacky and does nothing to address the greater issue (specific ctor for Client vs. Server)

Thoughts?

[chkr1011]

Hi,
yes I know that global solution from regular .NET framework.
In general there is a dedicated object for Tls options for this in the MQTT client.
In my opinion there should be a new boolean value which allows ignoring self signed certificates. Something similar is already there for revoked certificates. Maybe this can be reused or already works for your case!?
Then the implementations should inspect the options and add a handler in the SslStream etc. so that there is no new factory required and the user can set it with a simple option.

What do you think about this solution?

Best regards
Christian

[rydergillen-compacSort]

I don't think a simple Boolean property would be sufficient. There isn't a specific property on a Self-Signed cert identifying it as such. A Func<,,,> matching the signature of the delegate could be exposed as a property. This would allow the developer full control over what certificates to accept/reject.

I think something alone these lines would work for my specific use case. I guess what I was also trying to resolve is the need to always have a property for each setting that should be mapped back. At some point if the needed functionality deviates from the standard design the developer would be better off constructing the SSL stream externally.

The public MqttTcpChannel(Socket socket, SslStream sslStream) signature indicates this is possible, alas the SslStream property is always overwritten by an internal call during ConnectAsync(), making it unsuitable for Client communications.

[chkr1011]

I agree but the Func you mentioned must be sufficient for all frameworks. I saw that core has different properties than the classic .NET stuff. If you provide such a function the implementation must map this function to the used stream which might be not possible because UWP don't has this function. It only has several enum which are describing what certificate status can be ignored. So a Func will not make sense in my opinion. A boolean is easier for that. What do you think about this?

Best regards
Christian

[chkr1011]

Please have a look at the develop branch. I added several TLS options regarding certificate validation. Please let me know if this fits your needs.

[rydergillen-compacSort]

Looks like it should work fine for my needs. I will take the updated package a run a few tests.

[chkr1011]

I also added a callback mechanism for dealing with complex certificate validations. It is described in the wiki. I will close this ticket. Please let me know it you need more features to deal with certificates.

Best regards
Christian