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

[Android] Fix SslStreamCertificateContext empty custom trust store exception #104016

Merged
merged 5 commits into from
Jun 28, 2024
Merged

[Android] Fix SslStreamCertificateContext empty custom trust store exception #104016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e18dd22
Check if certificate collections are not empty before changing trust …
simonrozsival Jun 26, 2024
7b8a000
Enable SslStream_ClientCertificateContext_SendsChain test on Android
simonrozsival Jun 26, 2024
ea7ad9a
Apply suggestions from reviews
simonrozsival Jun 27, 2024
6d9d1bb
Avoid unnecessary allocations
simonrozsival Jun 27, 2024
ab0f8e7
Merge branch 'main' of github.com:dotnet/runtime into fix-sslstreamce…
simonrozsival Jun 27, 2024
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
9 changes: 7 additions & 2 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,20 @@ internal static SslStreamCertificateContext Create(

if (trust != null)
{
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
if (trust._store != null)
{
chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates);
}

if (trust._trustList != null)
{
chain.ChainPolicy.CustomTrustStore.AddRange(trust._trustList);
}
Copy link
Member

Choose a reason for hiding this comment

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

I recommend returning to the previous pattern of just calling AddRange.

Then, you can decide to only change the custom trust mode if chain.ChainPolicy.CustomTrustStore is not empty:

if (chain.ChainPolicy.CustomTrustStore.Count > 0)
{
    chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
}

Copy link
Member

@bartonjs bartonjs Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not intuitive to me, though, that we'd let "I said empty custom trust" mean "so use system trust" on Android, but mean "so nothing is trusted" on other platforms.

The trust being specified as empty should really manifest as an error here, or somewhere else, not mean "just do something different".

Nevermind. I see that this chain build is just to get the issuer/issuee relationships, and that there're no revocation checks and all verification errors are suppressed.


if (chain.ChainPolicy.CustomTrustStore.Count > 0)
{
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
}
}

chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags;
Expand All @@ -77,7 +82,7 @@ internal static SslStreamCertificateContext Create(
NetEventSource.Error(null, $"Failed to build chain for {target.Subject}");
}

if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates != null)
if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates?.Count > 0)
{
// Some platforms like Android may not be able to build the chain unless the chain root is trusted.
// We can try to rebuild the chain with making all extra certificates trused.
Expand Down
1 change: 0 additions & 1 deletion 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 @@ -917,7 +917,6 @@ public async Task SslStream_ClientCertificate_SendsChain()
[Theory]
[InlineData(true)]
[InlineData(false)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)]
public async Task SslStream_ClientCertificateContext_SendsChain(bool useTrust)
{
(X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = Configuration.Certificates.GenerateCertificates(nameof(SslStream_ClientCertificateContext_SendsChain), serverCertificate: false);
Expand Down
Loading