Send CA names on Linux and OSX #65195

rzikm · 2022-02-11T16:25:12Z

Fixes Issue #55802

This PR adds support for SslCertificateTrust on Linux and OSX, both platforms are implemented by enumerating the certificates in question (either from provided collection, or enumerating provided certificate store), and passing them to respective platform APIs:

SSL_add_client_CA on Linux
SSLSetCertificateAuthorities on OSX

on OSX, there seems to some additional validation on the passed certificates. When I tried to pass certificates normally used in tests, the call failed with secErrParam (One or more parameters passed to the function are not valid.). However, I have not been able to find out what is the problem, so I leave it currently as is, since there is probably not much we can do about it anyway. Certs taken out from the machine Root store sem to work okay.

ghost · 2022-02-11T16:25:20Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes Issue #55802

Author:	rzikm
Assignees:	rzikm
Labels:	`area-System.Net.Security`
Milestone:	-

rzikm · 2022-02-12T12:30:30Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+            if (frameSize < int.MaxValue)
+            {
+                _buffer.EnsureAvailableSpace(frameSize - _buffer.EncryptedLength);
+            }
+


When supplying store-based SslCertificateTrust, the ServerHello can grow up substantially (well over 8 KB buffer used until now, so we may need to resize the buffer.

I'm not sure if this is needed. If the hello is bigger than maximum frame size it should arrive in two (or more) frames. And that should be OK IMHO. If it is not, we should figure out what is happening.
This would be problem already, right? e.g. this is unrelated to sending the list since it is in receiving path.

There is a way for the code to deterministically throw exception without this change in case Root trust cert names are sent in ServerHello (around 120 names):

Default buffer size during handshake is 8kB

(one of) the incoming frames is larger than 8kB, the size is known before entering the while loop below the added code

first read fills in rest of the buffer, part of the frame remains in the inner stream

since the frame was known, the call to _buffer.EnsureAvailableSpace is skipped

on second iteration, 0 bytes are read since there is no available space to read into

throw new IOException(SR.net_io_eof)

Is there chance this will mess up our buffer logic? I briefly look at the EnsureAvailableSpace implementation and it can for example change _activeStart e.g. move the data within the buffer. I'm wondering if we should call EnsureAvailableSpace immediately when we determine frame size...?

I don't think we can mess things up here. The compacting/moving of the data was happening even before we merged the _internalBuffer and _handshakeBuffer

I'm wondering if we should call EnsureAvailableSpace immediately when we determine frame size...?

That is what is happening here, unless I misunderstand you. The frame size is read in the HaveFullTlsFrame call above and (if the size wasnt' known at that time) at the end of the while loop below.

The alternative place to call EnsureAvailableSpace would be right after we finish processing the previous frame and that does not feel right.

wfurt

generally looks good.

wfurt · 2022-02-13T05:31:53Z

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs

+                        X509Certificate2Collection certList = (trust._trustList ?? trust._store!.Certificates);
+
+                        Debug.Assert(certList != null, "certList != null");
+                        foreach (X509Certificate2 cert in certList)


This may be fine. But I'm wondering if we could add all at once via some array or stackalloc.

Is it just so we don't traverse the language boundary for each item? We can do that I guess.

wfurt · 2022-02-13T05:32:37Z

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs

+                        {
+                            if (!Ssl.SslAddClientCA(sslHandle, cert.Handle))
+                            {
+                                Debug.Fail("Failed to add issuer to trusted CA list.");


I would probably also add trace message so we have some evidence for release builds.

This should only happen when more than max. number of X509_NAME are pushed to the stack, that is something around int.MaxValue. I'd wager we are safe here.

I wanted to add a comment there but did not push that yet.

wfurt · 2022-02-13T05:39:03Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+            if (frameSize < int.MaxValue)
+            {
+                _buffer.EnsureAvailableSpace(frameSize - _buffer.EncryptedLength);
+            }
+


I'm not sure if this is needed. If the hello is bigger than maximum frame size it should arrive in two (or more) frames. And that should be OK IMHO. If it is not, we should figure out what is happening.
This would be problem already, right? e.g. this is unrelated to sending the list since it is in receiving path.

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs

wfurt · 2022-02-13T05:42:44Z

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs

+                        }
+
+                        acceptableIssuers = issuers;
+                        return clientCertificate;


This probably does not matter but I'm wondering if we can turn this into Theory and have bool to send the client cert conditionally.

I think this does not matter for this test, we are just checking that issuers are populated based on the SslCertificateTrust. There are other tests that exercise the cert-returning behavior

rzikm · 2022-02-14T13:34:23Z

Should be ready for another round of reviews

wfurt · 2022-02-15T04:45:11Z

For the macOS you can try log stream -p "pidof securityd". It is likely that the certificate is refused if it has client EKU and/or does not have the CA bit set.
https://support.apple.com/en-us/HT210176 may be relevant.

We have test code to generate certificates and CA on demand. You can experiment with that. However, I had to disable some of the tests as there seems to be some race condition between when the certificate is generate to when it can be actually used.

wfurt · 2022-02-15T04:52:40Z

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs

+        {
+            fixed (IntPtr* pHandles = &MemoryMarshal.GetReference(x509handles))
+            {
+                return SslAddClientCAs(ssl, pHandles, x509handles.Length);


I'm wondering if we should throw here on failure to be consistent with SslSetCertificateAuthorities on macOS.

this can fail only when maximum number of STACK_OF(X509_NAME) items is reached (something around MAX_INT), so we would not be likely to throw anyway, I added the propagation of the return code for consistency.

wfurt · 2022-02-15T04:55:52Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+                    X509Certificate2Collection certList = (trust._trustList ?? trust._store!.Certificates);
+
+                    Debug.Assert(certList != null, "certList != null");
+                    IntPtr[] handles = new IntPtr[certList.Count];


can you use same stackalloc optimization as in Linux?

I will try, I was trying to avoid changing CFArrayCreate to accept Span<IntPtr> instead of IntPtr[] used to construct the handle passed to native code accepts array, and I

wfurt · 2022-02-15T04:57:45Z

src/libraries/System.Net.Security/src/System/Net/Security/SslCertificateTrust.cs

@@ -21,7 +21,7 @@ public static SslCertificateTrust CreateForX509Store(X509Store store, bool sendT
                throw new PlatformNotSupportedException(SR.net_ssl_trust_store);
            }
 #else
-            if (sendTrustInHandshake)
+            if (sendTrustInHandshake && !System.OperatingSystem.IsLinux() && !System.OperatingSystem.IsMacOS())
            {
                // to be removed when implemented.
                throw new PlatformNotSupportedException("Not supported yet.");


I think we should update the message and put it to resource file. This looks like omission from time when we expected to finish everything before 6.0 release

src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.c

rzikm · 2022-02-15T14:26:52Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-02-15T14:27:10Z

Azure Pipelines successfully started running 1 pipeline(s).

wfurt · 2022-02-16T23:52:48Z

BTW this should work for Linux & macOS @rzikm

diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs
index eb5947be183..6ae6eec6c71 100644
--- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs
+++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs
@@ -20,19 +20,15 @@ public class SslStreamCertificateTrustTest
         [PlatformSpecific(TestPlatforms.Linux | TestPlatforms.OSX)]
         public async Task SslStream_SendCertificateTrust_CertificateCollection()
         {
-            using X509Store store = new X509Store("Root", StoreLocation.LocalMachine);
-            store.Open(OpenFlags.ReadOnly | OpenFlags.OpenExistingOnly);
-            X509Certificate2[] certList = store.Certificates.DistinctBy(c => c.Subject).Take(2).ToArray();
+            (X509Certificate2 certificate, X509Certificate2Collection caCerts) = TestHelper.GenerateCertificates("foo");

             SslCertificateTrust trust = SslCertificateTrust.CreateForX509Collection(
-                new X509Certificate2Collection(certList),
+                caCerts,
                 sendTrustInHandshake: true);

             string[] acceptableIssuers = await ConnectAndGatherAcceptableIssuers(trust);

-            Assert.Equal(2, acceptableIssuers.Length);
-            Assert.Contains(certList[0].Subject, acceptableIssuers);
-            Assert.Contains(certList[1].Subject, acceptableIssuers);
+            Assert.Equal(caCerts.Count, acceptableIssuers.Length);
         }

passing CA certificates makes macOS happy. e.g. it seems to depend on constraint CA:true
I would rather do that to make sure there is some weird and unexpected dependency on certificate store.

I'm wondering if we should check the certificate and throw consistent on all platforms. There is only DN on the wire but it does not make sense IMHO to advertise something that is not CA.
cc: @bartonjs for extra opinion.

If we decide not to go that path I think we should write test with "disliked" certificate to make sure we handle that and that we surface some reasonable exception.

And perhaps test that purposely break the 1 TLS frame boundary so we have test coverage for that.

bartonjs · 2022-02-17T00:18:01Z

I'm wondering if we should check the certificate and throw consistent on all platforms.

It depends on what you think the callers are doing. If they do something like open a cert store and pass store.Certificates (whether it be LM\Root or they're just using the store as a way to talk about a list that's shared across processes) then the nicer thing would be to skip over anything that isn't both self-issued and has the CA:true basic constraint set.

If you expect the caller instead to have a reference to like 3 certs and make the collection manually, then throwing seems more appropriate.

wfurt · 2022-02-17T01:14:02Z

I just look at I see this in my "ROOT" Keychain

        Issuer: CN=Microsoft Internal Corporate Root
        Subject: CN=MSIT CA W2

Since this is combined with the SslCertificateTrust I would expect custom trust would be common. But it seems like we are at point where macOS cannot handle arbitrary certificates like Linux. The choice is to live (and document?) platform differences or try to put in (somewhat) arbitrary restrictions.
I'm not sure what Windows would do if we put cert without CA flag to root store.

bartonjs · 2022-02-17T02:02:35Z

I'm not sure what Windows would do if we put cert without CA flag to root store.

It doesn't care. Probably doesn't care that it's self-issued, either. But the chain/trust evaluator will care at the end.

or try to put in (somewhat) arbitrary restrictions.

Skipping over non-CA certificates in managed code, for all OSes, seems reasonable to me. I previously said non-self-issued... but I'm now not recalling if this list is supposed to be only root authorities (self-issued) or if it's the subject name of any acceptable CA (including intermediates). So don't add that filter without checking somewhere authoritative 😄

rzikm · 2022-02-17T13:13:17Z

Skipping over non-CA certificates in managed code, for all OSes, seems reasonable to me.

Same here, it's what the RFC says anyway

4.2.4. Certificate Authorities
The "certificate_authorities" extension is used to indicate the
certificate authorities (CAs) which an endpoint supports and which
SHOULD be used by the receiving endpoint to guide certificate
selection.
....
authorities: A list of the distinguished names [X501] of acceptable
certificate authorities, represented in DER-encoded [X690] format.
These distinguished names specify a desired distinguished name for
a trust anchor or subordinate CA; thus, this message can be used
to describe known trust anchors as well as a desired authorization
space.

rzikm · 2022-02-17T16:43:48Z

There seem to be consistent test failures on Windows, so I will for now disable the test: #65515

wfurt · 2022-02-17T19:07:46Z

While the intent is clear, this is more about technicalities IMHO. We have seen in past where for example people setup self-signed certs or self-hosted CA were are all the flags or best practices are not followed.

After thinking about it for a while I'm inclined to defer the decision to OS. We already have platform differences in certificate validation so it seems we should follow that here as well.

rzikm · 2022-02-17T20:59:25Z

After thinking about it for a while I'm inclined to defer the decision to OS. We already have platform differences in certificate validation so it seems we should follow that here as well.

It is possible to put non-CA cert in a store on OSX, right? If so, we still need to filter these certs out, otherwise user would not be able to use SslCertificateTrust from that particular store.

wfurt · 2022-02-17T21:22:06Z

It is possible to put non-CA cert in a store on OSX, right? If so, we still need to filter these certs out, otherwise user would not be able to use SslCertificateTrust from that particular store.

I was thinking about it and I think that is OK as long as we surface reasonable exception. I feel that it is better than silent failure. In case of macOS is is somewhat strict in what it considers as valid (e.g. trusted) CA. Putting certificates that do not meet the expectation to root store will not make them trusted for SSL. If somebody really need to deal with this they can always give us collection. And I think sending the CA list is going be very rare.

rzikm · 2022-02-22T19:25:30Z

don't know why, but the remoteCertificate was null in the callback, and thus we did not actually retrieve the issuers list, I changed the condition in the callback to count the number of invocations and ignore the first one, that seems to do the trick.

Still don't know whats different between the VM at Helix and my WSL instance, both use OpenSSL 1.1.1.

wfurt · 2022-02-23T03:48:49Z

Still don't know whats different between the VM at Helix and my WSL instance, both use OpenSSL 1.1.1.

Could be openssl.conf. If you still have Helix repro, could you try to run just that single test to see if that still fails?
While this is unrelated to sending the CA list I think we should understand what the client did not get server certificate. It would be also OK to open issue for tracking and investigate later.

This reverts commit 7d864a185a7150cd384f16bba5efd616f27759ca.

rzikm · 2022-02-23T11:56:16Z

The issue was openssl/openssl#7384. I fixed it by forcing TLS 1.2, as we do in other test (like CertificateValidationClientServer)

wfurt

LGTM.

wfurt · 2022-02-23T17:56:03Z

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamCertificateTrustTests.cs

+        [PlatformSpecific(TestPlatforms.Linux | TestPlatforms.OSX)]
+        public async Task SslStream_SendCertificateTrust_CertificateCollection()
+        {
+            (X509Certificate2 certificate, X509Certificate2Collection caCerts) = TestHelper.GenerateCertificates("foo");


nit: you can use nameof(SslStream_SendCertificateTrust_CertificateCollection) -> using test names sometimes helps with debugging if tests are running in parallel.

rzikm · 2022-02-24T12:53:35Z

CI failures are unrelated (transient failure when running System.IO.Tests)

Changes implemented in dotnet/runtime#65195

ghost assigned rzikm Feb 11, 2022

dotnet-issue-labeler bot added the area-System.Net.Security label Feb 11, 2022

rzikm commented Feb 12, 2022

View reviewed changes

wfurt reviewed Feb 13, 2022

View reviewed changes

rzikm force-pushed the 55802-Add-ability-to-send-SslCertificateTrust-in-TLS-handshake-on-Linux-and-macOS branch from eab38fe to 262f567 Compare February 14, 2022 09:49

rzikm changed the title ~~Send CA names on Linux~~ Send CA names on Linux and OSX Feb 14, 2022

rzikm force-pushed the 55802-Add-ability-to-send-SslCertificateTrust-in-TLS-handshake-on-Linux-and-macOS branch from 67274ae to b076d8a Compare February 14, 2022 13:33

rzikm marked this pull request as ready for review February 14, 2022 13:34

wfurt reviewed Feb 15, 2022

View reviewed changes

src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.c Outdated Show resolved Hide resolved

rzikm force-pushed the 55802-Add-ability-to-send-SslCertificateTrust-in-TLS-handshake-on-Linux-and-macOS branch from b076d8a to c2ce0d8 Compare February 15, 2022 14:15

rzikm requested a review from wfurt February 15, 2022 14:26

rzikm mentioned this pull request Feb 17, 2022

SslCertificateTrust is not sent on Windows #65515

Closed

rzikm force-pushed the 55802-Add-ability-to-send-SslCertificateTrust-in-TLS-handshake-on-Linux-and-macOS branch from 9975564 to 045a3fc Compare February 17, 2022 16:42

rzikm and others added 14 commits February 23, 2022 12:52

Send CA names on Linux

a0d76d8

Optimize setting certname list in OpenSSL

23cfd08

First shot at OSX implementation

717e866

Fix tests on OSX

7247db0

Optimize setting certificates on OSX

09e01eb

Improve message in PNSE

9ee8520

Fix compilation

17b573c

Use only CA certificates in test

5cad62d

Remove unwanted formatting changes

33fda73

Skip non-CA certificates

6432d87

Fix crash on OSX

27fd404

Disable new tests on Windows

e064a9d

Revert "Skip non-CA certificates"

15cc494

This reverts commit 7d864a185a7150cd384f16bba5efd616f27759ca.

Fix tests on Ubuntu 18.04

fbba399

rzikm force-pushed the 55802-Add-ability-to-send-SslCertificateTrust-in-TLS-handshake-on-Linux-and-macOS branch from 541957b to fbba399 Compare February 23, 2022 11:53

wfurt approved these changes Feb 23, 2022

View reviewed changes

runfoapp bot mentioned this pull request Feb 23, 2022

System.IO.Tests work item failing with SIGKILL #65791

Closed

Change cert name

e40e1b0

rzikm merged commit 9212310 into dotnet:main Feb 24, 2022

rzikm mentioned this pull request Mar 3, 2022

Add ability to send SslCertificateTrust in TLS handshake on Linux and macOS #55802

Closed

rzikm added a commit to dotnet/dotnet-api-docs that referenced this pull request Mar 16, 2022

Update docs for SslCertificateTrust with changes in .NET 7

98666df

Changes implemented in dotnet/runtime#65195

rzikm mentioned this pull request Mar 16, 2022

Update docs for SslCertificateTrust with changes in .NET 7 dotnet/dotnet-api-docs#7832

Merged

ghost locked as resolved and limited conversation to collaborators Mar 26, 2022

karelz added this to the 7.0.0 milestone Apr 8, 2022

bartonjs added cryptographic-docs-impact needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Aug 20, 2022

Send CA names on Linux and OSX #65195

Send CA names on Linux and OSX #65195

Conversation

rzikm commented Feb 11, 2022 • edited Loading

ghost commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzikm Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt 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

rzikm Feb 14, 2022 • edited Loading

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

rzikm commented Feb 14, 2022

wfurt commented Feb 15, 2022

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

rzikm commented Feb 15, 2022

azure-pipelines bot commented Feb 15, 2022

wfurt commented Feb 16, 2022

bartonjs commented Feb 17, 2022

wfurt commented Feb 17, 2022

bartonjs commented Feb 17, 2022

rzikm commented Feb 17, 2022

rzikm commented Feb 17, 2022

wfurt commented Feb 17, 2022

rzikm commented Feb 17, 2022

wfurt commented Feb 17, 2022

rzikm commented Feb 22, 2022

wfurt commented Feb 23, 2022

rzikm commented Feb 23, 2022

wfurt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzikm commented Feb 24, 2022

rzikm commented Feb 11, 2022 •

edited

Loading

rzikm Feb 14, 2022 •

edited

Loading

rzikm Feb 14, 2022 •

edited

Loading