Support custom CA chain validation #1851

rido-min · 2023-09-29T23:56:56Z

Summary

Most MQTT clients from Paho (python, Java, GO) and even mosquitto-clients, allow to specify a CaFile to connect to TLS endpoints protected with certificates issued by a private CA.

A good example is test.mosquitto.org:8883 that requires https://test.mosquitto.org/ssl/mosquitto.org.crt to validate the server connection

Details

Adds a CertificationAuthoritiesFile option to TlsOptions
Adds a WithCertificationAuthoritiesFile(string pemFile) to TlsOptionsBuilder
Configures SslStream with CustomTrust (.NET 7+ only)
Adds a new sample, including the test.mosquitto.org (this might require to be updated when it gets expired in 2030 :) )

Overseeds #1848

rido-min · 2023-10-02T13:59:30Z

How can I fix the build error?

D:\a\MQTTnet\MQTTnet\certificate.snk' -- Invalid public key

krwq · 2023-10-04T06:16:17Z

Source/MQTTnet/Client/Options/MqttClientTlsOptions.cs

 #else
        public SslProtocols SslProtocol { get; set; } = SslProtocols.Tls12 | (SslProtocols)0x00003000 /*Tls13*/;
 #endif
+
+#if NET7_0_OR_GREATER
+        public string CertificationAuthoritiesFile { get; set; }


I'd recommend to make this API always visible but throw on the setter for <net7

Other properties in this project are usually not available because they are excluded by the compiler. I would like to stick to this approach and avoid changing the strategy here.

krwq · 2023-10-04T06:17:28Z

Source/MQTTnet/Implementations/MqttTcpChannel.cs

                            TargetHost = targetHost,
                            CipherSuitesPolicy = _tcpOptions.TlsOptions.CipherSuitesPolicy,
                            EncryptionPolicy = _tcpOptions.TlsOptions.EncryptionPolicy,
                            AllowRenegotiation = _tcpOptions.TlsOptions.AllowRenegotiation
                        };
+#if NET7_0_OR_GREATER
+                        if (!string.IsNullOrEmpty(_tcpOptions.TlsOptions.CertificationAuthoritiesFile))


another option: File.Exists (error when doesn't exist would be nice but possibly just letting it error later is fine as well, follow existing conventions)

rido-min · 2023-10-06T01:23:09Z

Source/MQTTnet/Client/Options/MqttClientTlsOptionsBuilder.cs

@@ -132,5 +132,13 @@ public MqttClientTlsOptionsBuilder WithCipherSuitesPolicy(EncryptionPolicy encry
            return this;
        }
 #endif
+
+#if NET7_0_OR_GREATER
+        public MqttClientTlsOptionsBuilder WithCertificationAuthoritiesFile(string pemFile)


another option will be WithCaFile is shorter -not as descriptive as the other methods in this class - but is aligned with other mqtt tooling.

The other methods and properties in this project usually take the "long" version or adopt the same name when it is simply mapped to a property in the .NET framework.

this comment is outdated, the last commit changed the signature to: public MqttClientTlsOptionsBuilder WithTrustChain(X509Certificate2Collection chain)

To align with .NET api this could be .WithCustomTrustChain

logicaloud · 2023-10-14T19:50:56Z

I think it would be good to support handling of certificate authorities directly within the MQTTnet library.

Some observations and questions:

Naming should probably be ...CertificateAuthorities... instead of ...CertificationAuthorities...?
Should the target conditional be NET5_0_OR_GREATER? Or are there features used that are not available since .NET 5? I'm aware that .NET 5 is out of support, but this would be in keeping with other conditionals used elsewhere supporting older .NET versions (and .NET 6 is still in support).
My preference would be to pass the certificate authorities as a list or collection of X509Certificate2 to keep things in memory. Certificates may originate from a configuration file, database, of otherwise different format not readily available as a .crt file on disk. If the certificate authority certificates are indeed already available as a suitable file, then it is a small burden for the MQTTnet library user to load the file into a list or collection of X509Certificate2 certificates for handover to WithCertificateAuthorities. If the the certificates are not already in a suitable file, then passing a filename could become a pain point, because the MQTTnet library user needs to convert the format into the .crt file format and ensure that a there is a writable file system location available just for handover.

rido-min · 2023-10-15T20:11:40Z

thanks @logicaloud for you comments:

The name was suggested by @CIPop, maybe we could use WithTrustChain ?
IIRC The API we are using to use a custom trust store is available in .NET7+ only, but let me double check if this could also work in .NET5+
Good point, I'm updating the PR to accept a X509CertCollection

rido-min · 2023-10-16T06:04:31Z

@logicaloud with this 0b5dff5 I've addressed your comments:

Renamed to public MqttClientTlsOptionsBuilder WithTrustChain(X509Certificate2Collection chain)
Unfortunately SslClientAuthenticationOptions.CertificateChainPolicy requires .NET7
Agree, the user should provide the caChain as a cert collection without imposing any crt/pem format in a file

logicaloud · 2023-10-16T19:13:44Z

Thank you for looking into this, @rido-min. Regarding naming, WithTrustChain seems more technical, WithCertificateAuthorities may need less explanation in the context of MQTT. Either way, it is not up to me to accept pull requests, so best to wait now until @chkr1011 has time to review and comment.

rido-min · 2023-10-17T01:02:28Z

WithTrustChain seems more technical,

Well, keep in mind this is on top of MqttTlsOptions, so I guess the TrustChain concept will be easier to get.

rido-min · 2023-10-26T23:40:31Z

apologies for adding another commit cc1ed2c

Now the chain validation uses X509VerificationFlags.IgnoreEndRevocationUnknown so users are not forced to completely disable revocation checks when using custom CAs.

@krwq do you mind taking another look?

Add CaFile based on SslStream CustomTrust

8b22637

krwq reviewed Oct 4, 2023

View reviewed changes

krwq approved these changes Oct 4, 2023

View reviewed changes

rido-min commented Oct 6, 2023

View reviewed changes

This was referenced Oct 10, 2023

Review CustomChain validation Azure-Samples/MqttApplicationSamples#69

Merged

Support Custom CA chain in TLS validation #1857

Closed

change to cert collection

0b5dff5

do not require to disable revocation checks

cc1ed2c

chkr1011 approved these changes Nov 1, 2023

View reviewed changes

chkr1011 merged commit 01c90ba into dotnet:master Nov 1, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom CA chain validation #1851

Support custom CA chain validation #1851

rido-min commented Sep 29, 2023

rido-min commented Oct 2, 2023

krwq Oct 4, 2023

chkr1011 Oct 17, 2023

krwq Oct 4, 2023 •

edited

rido-min Oct 6, 2023

chkr1011 Oct 17, 2023

rido-min Oct 17, 2023

logicaloud commented Oct 14, 2023

rido-min commented Oct 15, 2023

rido-min commented Oct 16, 2023

logicaloud commented Oct 16, 2023

rido-min commented Oct 17, 2023

rido-min commented Oct 26, 2023

Support custom CA chain validation #1851

Support custom CA chain validation #1851

Conversation

rido-min commented Sep 29, 2023

Summary

Details

rido-min commented Oct 2, 2023

krwq Oct 4, 2023

Choose a reason for hiding this comment

chkr1011 Oct 17, 2023

Choose a reason for hiding this comment

krwq Oct 4, 2023 • edited

Choose a reason for hiding this comment

rido-min Oct 6, 2023

Choose a reason for hiding this comment

chkr1011 Oct 17, 2023

Choose a reason for hiding this comment

rido-min Oct 17, 2023

Choose a reason for hiding this comment

logicaloud commented Oct 14, 2023

rido-min commented Oct 15, 2023

rido-min commented Oct 16, 2023

logicaloud commented Oct 16, 2023

rido-min commented Oct 17, 2023

rido-min commented Oct 26, 2023

krwq Oct 4, 2023 •

edited