-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
[Android] Fix SslStreamCertificateContext empty custom trust store exception #104016
[Android] Fix SslStreamCertificateContext empty custom trust store exception #104016
Conversation
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the quick fix! It looks like there are still failures on arm Androids, however I don't think they are related. I created a new tracking issue for them #104030
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -54,17 +54,15 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce | |||
chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); | |||
} | |||
|
|||
if (trust != null) | |||
if (trust?._store?.Certificates.Count > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Certificates
property on X509Store reads the store live and instantiates all the certificates. Using it in this manner throws a bunch of data at the GC and finalizer queue. And has a ToC/ToU bug, as it could still end up as empty on line 60.
if (trust._trustList != null) | ||
{ | ||
chain.ChainPolicy.CustomTrustStore.AddRange(trust._trustList); | ||
} |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
The failing tests in runtime-extra-platforms are all unrelated |
…ception (dotnet#104016) * Check if certificate collections are not empty before changing trust mode to custom root trust * Enable SslStream_ClientCertificateContext_SendsChain test on Android * Apply suggestions from reviews * Avoid unnecessary allocations
…CertificateContext (#104541) Backport of #103372 and #104016 to release/8.0-staging ## Customer Impact - [X] Customer reported (#100602) - [ ] Found internally Customers developing Android apps are currently unable to use mutual TLS authentication in certain cases as the `SslStreamCertificateContext.Create(...)` method will fail to build an X509Chain instance if the certificate isn't trusted by the OS due to the limitations of the Android platform. ## Regression - [ ] Yes - [X] No ## Testing Unit tests and manual testing on Android emulator. ## Risk Low. The change is mostly limited to Android where this API doesn't currently work in many cases. --------- Co-authored-by: Tomas Weinfurt <[email protected]> Co-authored-by: Vitek Karas <[email protected]>
Closes #104010
Follow-up to #103372
When building the chain, we should not only check if the trust or additional certificate collections aren't null but also if they're not empty.
It is now also possible to enable one of the disabled Android tests (#68206).
/cc @matouskozak @wfurt