Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix handling of client cert on macOS #73574

Merged
merged 12 commits into from
Aug 16, 2022
Merged

fix handling of client cert on macOS #73574

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
47dcc51
fix handling of client cert on macOS
wfurt Aug 8, 2022
80c5d06
update test
wfurt Aug 8, 2022
40da568
fix test check
wfurt Aug 8, 2022
e92c1cb
add debug for building chain
wfurt Aug 8, 2022
fa356a6
increase retry
wfurt Aug 8, 2022
65afb2f
more debug
wfurt Aug 9, 2022
2b72fa3
more debug
wfurt Aug 9, 2022
729c430
more debug
wfurt Aug 9, 2022
09bb3ca
log ChainStatus
wfurt Aug 9, 2022
1f66fea
update test
wfurt Aug 13, 2022
b9f86c0
Merge branch 'main' of https://github.com/dotnet/runtime into macCCert
wfurt Aug 13, 2022
d710819
add comment
wfurt Aug 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,15 @@ internal static SslPolicyErrors VerifyCertificateProperties(
{
long chainSize = Interop.AppleCrypto.X509ChainGetChainSize(chainHandle);

if (retrieveChainCertificates)
if (retrieveChainCertificates && chainSize > 1)
{
chain ??= new X509Chain();
if (chainPolicy != null)
{
chain.ChainPolicy = chainPolicy;
}

for (int i = 0; i < chainSize; i++)
for (int i = 1; i < chainSize; i++)
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
IntPtr certHandle = Interop.AppleCrypto.X509ChainGetCertificateAtIndex(chainHandle, i);
chain.ChainPolicy.ExtraStore.Add(new X509Certificate2(certHandle));
Expand Down
6 changes: 1 addition & 5 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[]
Trust = trust;
}

internal static SslStreamCertificateContext Create(X509Certificate2 target)
{
// On OSX we do not need to build chain unless we are asked for it.
return new SslStreamCertificateContext(target, Array.Empty<X509Certificate2>(), null);
}
internal static SslStreamCertificateContext Create(X509Certificate2 target) => Create(target, null, offline: false, trust: null, noOcspFetch: true);
}
}
79 changes: 65 additions & 14 deletions src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,35 +859,84 @@ public async Task SslStream_UntrustedCaWithCustomCallback_Throws(bool customCall
}
}

[ConditionalFact]
[Fact]
[SkipOnPlatform(TestPlatforms.Android, "Self-signed certificates are rejected by Android before the .NET validation is reached")]
public async Task SslStream_ClientCertificate_SendsChain()
{
// macOS ignores CertificateAuthority
// https://github.com/dotnet/runtime/issues/48207
StoreName storeName = OperatingSystem.IsMacOS() ? StoreName.My : StoreName.CertificateAuthority;
List<SslStream> streams = new List<SslStream>();
TestHelper.CleanupCertificates();
(X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = TestHelper.GenerateCertificates("SslStream_ClinetCertificate_SendsChain", serverCertificate: false);
TestHelper.CleanupCertificates(nameof(SslStream_ClientCertificate_SendsChain), storeName);
(X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = TestHelper.GenerateCertificates(nameof(SslStream_ClientCertificate_SendsChain), serverCertificate: false);

using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser))
using (X509Store store = new X509Store(storeName, StoreLocation.CurrentUser))
{
// add chain certificate so we can construct chain since there is no way how to pass intermediates directly.
store.Open(OpenFlags.ReadWrite);
store.AddRange(clientChain);
store.Close();
}

using (var chain = new X509Chain())
_output.WriteLine("Certificates added to {0}", storeName);

// make sure we can build chain. There may be some race conditions after certs being added to the store.
int retries = 10;
int delay = 100;
while (retries > 0)
{
using (var chain = new X509Chain())
{
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.DisableCertificateDownloads = true;
bool chainStatus = chain.Build(clientCertificate);
_output.WriteLine("round {0}: chain.Build: finished as {1} with {2} certificates {3}", retries, chainStatus, chain.ChainElements.Count, DateTime.Now);
if (chainStatus && chain.ChainElements.Count >= clientChain.Count)
{
break;
}
}

Thread.Sleep(delay);
delay *= 2;
retries--;
}

if (retries == 0)
{
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.DisableCertificateDownloads = false;
bool chainStatus = chain.Build(clientCertificate);
// Verify we can construct full chain
if (chain.ChainElements.Count < clientChain.Count)
using (var chain = new X509Chain())
{
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.Build(clientCertificate);
foreach (X509ChainElement element in chain.ChainElements)
{
_output.WriteLine($"{element.Certificate.Subject} length {element.ChainElementStatus.Length}");
foreach (X509ChainStatus status in element.ChainElementStatus)
{
_output.WriteLine($" Status: {status.Status}: {status.StatusInformation}");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging items from chain.ChainStatus may also be useful, in case there's an overall code it presented that didn't map to any individual element

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

        CN=SslStream_ClientCertificate_SendsChain, O=SslStream_ClientCertificate_SendsChain length 1
          Status:  PartialChain: One or more certificates required to validate this certificate cannot be found.
          ChainStatus: PartialChain One or more certificates required to validate this certificate cannot be found..

it seems like it cannot find the part certs.
That are in store dump AFAIK

        CN=A Revocation Test CA 0, O=SslStream_ClientCertificate_SendsChain
        CN=A Revocation Test Root, O=SslStream_ClientCertificate_SendsChain

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. I certainly would have expected the default keychain to be involved in a chain build; but I guess not. We probably don't have tests like this in the X509 suite because we don't like messing with system state.

Since there's not a flow where the client can provide their own chain context, you might just need to have this test leave the responder running, so the intermediate can be fetched from AIA. (Overload GenerateCertificates to out the responder instead of disposing it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did many runs on my local machines and it works fine. But there is something unusual about the CI machine ;(
I was thinking about making the test optional again but we lost coverage before and it hid valid issue.

And I can try the AIA. I did not like it for the servers as it make some of the test less predicable but I can try it for client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did many runs on my local machines and it works fine. But there is something unusual about the CI machine ;(

Now that sounds familiar :). IIRC it doesn't have a UI session, just an SSH session, and that makes some of the keychain systems behave oddly.


foreach (X509ChainStatus status in chain.ChainStatus)
{
_output.WriteLine($" ChainStatus: {status.Status} {status.StatusInformation}.");
}
}

using (X509Store store = new X509Store(storeName, StoreLocation.CurrentUser))
{
throw new SkipTestException($"chain cannot be built {chain.ChainElements.Count}");
store.Open(OpenFlags.ReadWrite);
foreach (X509Certificate2 cert in store.Certificates)
{
_output.WriteLine(cert.Subject);
}
store.Close();
}
}

Assert.NotEqual(0, retries);

var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost" };
clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true;
clientOptions.LocalCertificateSelectionCallback = (sender, target, certificates, remoteCertificate, issuers) => clientCertificate;
Expand All @@ -905,7 +954,9 @@ public async Task SslStream_ClientCertificate_SendsChain()
_output.WriteLine("received {0}", c.Subject);
}

Assert.Equal(clientChain.Count - 1, chain.ChainPolicy.ExtraStore.Count);
// We may get completed chain from OS even if client sent less.
// As minimum, it should send more than the leaf cert
Assert.True(chain.ChainPolicy.ExtraStore.Count >= clientChain.Count - 1);
Assert.Contains(clientChain[0], chain.ChainPolicy.ExtraStore);
return true;
};
Expand All @@ -926,7 +977,7 @@ public async Task SslStream_ClientCertificate_SendsChain()
streams.Add(server);
}

TestHelper.CleanupCertificates();
TestHelper.CleanupCertificates(nameof(SslStream_ClientCertificate_SendsChain), storeName);
clientCertificate.Dispose();
foreach (X509Certificate c in clientChain)
{
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ internal static (NetworkStream ClientStream, NetworkStream ServerStream) GetConn
}
}

internal static void CleanupCertificates([CallerMemberName] string? testName = null)
internal static void CleanupCertificates([CallerMemberName] string? testName = null, StoreName storeName = StoreName.CertificateAuthority)
{
string caName = $"O={testName}";
try
{
using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine))
using (X509Store store = new X509Store(storeName, StoreLocation.LocalMachine))
{
store.Open(OpenFlags.ReadWrite);
foreach (X509Certificate2 cert in store.Certificates)
Expand All @@ -127,7 +127,7 @@ internal static void CleanupCertificates([CallerMemberName] string? testName = n

try
{
using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser))
using (X509Store store = new X509Store(storeName, StoreLocation.CurrentUser))
{
store.Open(OpenFlags.ReadWrite);
foreach (X509Certificate2 cert in store.Certificates)
Expand Down