Add client certificate chain support and SSL context generation #454

jmcrumb · 2024-03-25T22:16:15Z

Context

Current implementation of client certificate loading does not allow for use of chained client certificates, either from the base options of filepath or the LoadClientCertificate hook in NatsTlsOpts. The current behavior results in the first certificate in the file to be read, and any subsequent certificates contained are ignored. For clients which use certificate chaining, this forces server allowlisting of lowest-level intermediate certificates or failure to connect.

Resolution

Change the SDK to use certificate collections rather than individual certificates as well as expose a hook for custom loading of said collection. Some of the functionality of this fix, specifically use of SslStreamCertificateContext during construction of SslClientAuthenticationOptions, is only available in the latest stable .NET major version (.NET 8), so this feature will only have support that the .NET 8 build of the NATS SDK.

Changes

[BREAKING] Change NatsTlsOpts.LoadCertificate(X509Certificate2) to NatsTlsOpts.LoadCertificates(X509Certificate2Collection)
Add support for Certificate Chains
Add helper functions for loading certificates from string or file
Add SslStreamCertificateContext and X509ChainPolicy creation for use during SSL handshake

Additional Notes

During the RCA for this issue another bug was identified.
When attempting to connect from a Windows 10 Enterprise (19045.4046) machine, SSL connection was failing when NatsTlsOpts.InsecureSkipVerify = false. The same code ran without error on dockerized Linux. I suspect this is because of windows certificate handling (the same logic prompted special handling here, and may need to be applied to SslStreamConnection.RcsCbCaCertChain.

In addition, during the failure above, a null pointer exception would occur at SetupReaderWriterAsync since _currentConnectUri was never set due to a caught error during SSL connection and authentication.

mtmk · 2024-03-26T05:23:45Z

Please note that this PR is meant to highlight the implementation, and as such tests have been omitted as they used credentials which cannot be checked into this repository.

can we create test versions of these with a shell script?

jmcrumb · 2024-03-26T17:56:41Z

@mtmk Updated generator script with the openssl generation of the chained certs and added test cases for a multi-PEM file input

mtmk · 2024-03-27T08:45:30Z

checks seems to be failing. having a look.

edit: Tests were failing with certificate is not authorized. Just altered the script for intermediate certs, test should be fixed now.

edit2: on Linux with .NET 6 when using chain certs NATS.Client.Core.Tests.TlsCertsTest.Client_connect test was failing. To fix it, added a skip for chained certs when .NET 6.

mtmk · 2024-03-27T13:06:55Z

@jmcrumb could you sign your commits please?

mtmk · 2024-03-27T13:54:29Z

Please note that this PR is meant to highlight the implementation, and as such tests have been omitted as they used credentials which cannot be checked into this repository.

is this still true?

jmcrumb · 2024-03-27T17:19:01Z

is this still true?

No. Just revised the description.
And thanks for addressing the test issue!

mtmk · 2024-03-28T13:25:37Z

@jmcrumb, I updated the docs a little. if you can please check the last commit and let me know if you're happy with it then we can merge this PR I think.

* #451 * #433 * #454

jmcrumb

@mtmk Looks good to me!

src/NATS.Client.Core/Internal/SslStreamConnection.cs

caleblloyd · 2024-03-28T16:27:53Z

src/NATS.Client.Core/NatsTlsOpts.cs

@@ -62,7 +63,7 @@ public sealed record NatsTlsOpts
    /// <summary>
    /// Callback that loads Client Certificate
    /// </summary>
-    public Func<ValueTask<X509Certificate2>>? LoadClientCert { get; init; }
+    public Func<ValueTask<X509Certificate2Collection>>? LoadClientCerts { get; init; }


I don't think we should necessarily pluralize LoadClientCerts - it is still just 1 cert plus intermediates. Plural to me indicates the list of Client Certs will be tried based off some property of the cert such as common name or thumbprint. Should expand the doc here to explain why it's X509Certificate2Collection

Also - should this callback return a SslStreamCertificateContext instead of an X509Certificate2Collection, since it appears that is the correct API for building a client certificate chain with intermediates

what about X509ChainPolicy it needs that as well

SslClientAuthenticationOptions.CertificateChainPolicy applies to remote certificate validation, I think that could be implemented similar to NatsTlsOpts.CertificateRevocationCheckMode and is not needed in this callback?

Hard for me to weigh in on this with my limited vision of customers beyond my organization (and what they need). The most extreme and simple would be to expose the hook for SslClientAuthenticationOptions or for a Func<SslClientAuthenticationOptions,SslClientAuthenticationOptions> which allows for customizable overrides of the default built in SslStreamConnection. Between that and the current state just seems like different levels of tradeoff between flexibility and consistency.

ok, trying the tests without CertificateChainPolicy doesn't seem to make any difference. Also looks like it might mess up the OCPS (I might've remembered the acronym wrong) feature:

If not null, CertificateRevocationCheckMode and SslCertificateTrust are ignored.

Also - should this callback return a SslStreamCertificateContext instead of an X509Certificate2Collection, since it appears that is the correct API for building a client certificate chain with intermediates

the only problem is that ClientCertificateContext option (which gets assigned) is not available in net6.0

Yes, that API popped up in net8, there is a mega thread about it here: dotnet/runtime#26323

caleblloyd · 2024-04-02T14:10:48Z

@mtmk can we merge this to a staging branch?

I think I'd like to undo the breaking change, and split it to:

    /// NET6 and NET8, set Obsolete after NET6 EOL
    /// <summary>
    /// Callback that loads Client Certificate
    /// </summary>
    public Func<ValueTask<X509Certificate2>>? LoadClientCert { get; init; }
    
    // NET8 ONLY
    /// <summary>
    /// Callback that loads Client Certificate
    /// </summary>
    public Func<ValueTask<SslStreamCertificateContext>>? LoadClientCertContext { get; init; }

I have something staged but I can't push to this PR because it's on a fork

# Conflicts: # src/NATS.Client.Core/NatsTlsOpts.cs

mtmk · 2024-04-02T14:54:02Z

thanks @jmcrumb! we're staging now. we should be ready to merge to main then release very soon.

jmcrumb · 2024-04-02T16:15:17Z

Great to hear, thank you!

* Add client certificate chain support and SSL context generation (#454) * Add client certificate chain support and SSL context generation * Update certificate generation with certificate chain * Cert fix for tests * Test fix for .net6.0 * Minor doc updates * CA cert optional fix * CA cert optional fix * removed CA option from client context * Rename and docs * rename * rename * merge fixes --------- Co-authored-by: Maxon Crumb <mcrumb@starbucks.com> Co-authored-by: Ziya Suzen <ziya@suzen.net> * add NatsTlsOpts.LoadClientCertContext Signed-off-by: Caleb Lloyd <caleb@synadia.com> * remove LocalCertificateSelectionCallback on net8.0 Signed-off-by: Caleb Lloyd <caleb@synadia.com> * introduce ConfigureClientAuthentication Signed-off-by: Caleb Lloyd <caleb@synadia.com> * pragma Signed-off-by: Caleb Lloyd <caleb@synadia.com> * make SslClientAuthenticationOptionsExtensions internal Signed-off-by: Caleb Lloyd <caleb@synadia.com> --------- Signed-off-by: Caleb Lloyd <caleb@synadia.com> Co-authored-by: Maxon Crumb <37674616+jmcrumb@users.noreply.github.com> Co-authored-by: Maxon Crumb <mcrumb@starbucks.com> Co-authored-by: Ziya Suzen <ziya@suzen.net>

* #451 Dispose fixes * #433 Update NATS.Client.Hosting package as NATS.Extensions.Microsoft.DependencyInjection * (staged) #454 Add client certificate chain support and SSL context generation * #463 add NatsTlsOpts.LoadClientCertContext * #459 Object store metadata fix * #455 Deserialize empty payloads * Release 2.2.0 [nats:update-docs]

Add client certificate chain support and SSL context generation

11a178f

Update certificate generation with certificate chain

43e5c82

mtmk added the enhancement New feature or request label Mar 27, 2024

mtmk added this to the 2.1.5 milestone Mar 27, 2024

mtmk self-assigned this Mar 27, 2024

Cert fix for tests

2db37bd

Test fix for .net6.0

0e91569

Minor doc updates

b6ed851

mtmk added a commit that referenced this pull request Mar 28, 2024

Release 2.1.5

acdb605

* #451 * #433 * #454

mtmk mentioned this pull request Mar 28, 2024

Release 2.2.0 #456

Merged

jmcrumb commented Mar 28, 2024

View reviewed changes

caleblloyd reviewed Mar 28, 2024

View reviewed changes

mtmk added 6 commits March 28, 2024 17:20

CA cert optional fix

d0b47f1

CA cert optional fix

d5961c7

removed CA option from client context

75e7040

Rename and docs

e664883

rename

6818656

rename

f785f0e

mtmk changed the base branch from main to stage-pem-chain-mtls April 2, 2024 14:40

mtmk added 2 commits April 2, 2024 15:42

Merge remote-tracking branch 'origin/main' into pem-chain-mtls

6ac8e8f

# Conflicts: # src/NATS.Client.Core/NatsTlsOpts.cs

merge fixes

5ce3fb5

mtmk merged commit 6bdf41c into nats-io:stage-pem-chain-mtls Apr 2, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client certificate chain support and SSL context generation #454

Add client certificate chain support and SSL context generation #454

jmcrumb commented Mar 25, 2024 •

edited

Loading

mtmk commented Mar 26, 2024

jmcrumb commented Mar 26, 2024

mtmk commented Mar 27, 2024 •

edited

Loading

mtmk commented Mar 27, 2024

mtmk commented Mar 27, 2024

jmcrumb commented Mar 27, 2024 •

edited

Loading

mtmk commented Mar 28, 2024

jmcrumb left a comment

caleblloyd Mar 28, 2024

caleblloyd Mar 28, 2024

mtmk Mar 28, 2024

caleblloyd Mar 28, 2024

jmcrumb Mar 28, 2024

mtmk Mar 28, 2024

mtmk Mar 28, 2024

caleblloyd Mar 29, 2024

caleblloyd commented Apr 2, 2024

mtmk commented Apr 2, 2024

jmcrumb commented Apr 2, 2024

Add client certificate chain support and SSL context generation #454

Add client certificate chain support and SSL context generation #454

Conversation

jmcrumb commented Mar 25, 2024 • edited Loading

Context

Resolution

Changes

Additional Notes

mtmk commented Mar 26, 2024

jmcrumb commented Mar 26, 2024

mtmk commented Mar 27, 2024 • edited Loading

mtmk commented Mar 27, 2024

mtmk commented Mar 27, 2024

jmcrumb commented Mar 27, 2024 • edited Loading

mtmk commented Mar 28, 2024

jmcrumb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleblloyd commented Apr 2, 2024

mtmk commented Apr 2, 2024

jmcrumb commented Apr 2, 2024

jmcrumb commented Mar 25, 2024 •

edited

Loading

mtmk commented Mar 27, 2024 •

edited

Loading

jmcrumb commented Mar 27, 2024 •

edited

Loading