-
Notifications
You must be signed in to change notification settings - Fork 5.2k
fix certificate ctx on windows and macOS #39818
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
Changes from all commits
1a9673f
eed8a7a
b85ff7c
9a60428
fe706b9
d7fc52e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,10 +7,102 @@ namespace System.Net.Security | |
| { | ||
| public partial class SslStreamCertificateContext | ||
| { | ||
| // No leaf, include root. | ||
| private const bool TrimRootCertificate = false; | ||
|
|
||
| internal static SslStreamCertificateContext Create(X509Certificate2 target) | ||
| { | ||
| // On Windows we do not need to build chain unless we are asked for it. | ||
| return new SslStreamCertificateContext(target, Array.Empty<X509Certificate2>()); | ||
| } | ||
|
|
||
| private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates) | ||
| { | ||
| if (intermediates.Length > 0) | ||
| { | ||
| using (X509Chain chain = new X509Chain()) | ||
| { | ||
| chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; | ||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; | ||
| chain.ChainPolicy.DisableCertificateDownloads = true; | ||
| bool osCanBuildChain = chain.Build(target); | ||
|
|
||
| int count = 0; | ||
| foreach (X509ChainStatus status in chain.ChainStatus) | ||
| { | ||
| if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid)) | ||
| { | ||
| osCanBuildChain = false; | ||
| break; | ||
| } | ||
|
|
||
| count++; | ||
| } | ||
|
|
||
| // OS failed to build the chain but we have at least some intermediates. | ||
| // We will try to add them to "Intermediate Certification Authorities" store. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really want to add all intermediates? Or just the ones that the OS ends up using in a chain? e.g. Add all of intermediates to chain.ChainPolicy.ExtraStore and build again; then add anything relevant (preferably using a diffing algorithm so that it doesn't potentially reset custom store property overrides). Using a second chain object (you could copy the policy object over) would make the diff easy, just walk each position and compare that cert.RawData values SequenceEqual. Ideally, after each add you'd rerun the offline/no-extra build to see if you can avoid invalidating existing higher-scoped entries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (While a diffing algorithm and a while loop sound potentially expensive, most chains are three items long, so there aren't many iterations) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was considering to add some more complicated logic. But as you said if most chains are 3 long, it does not matter that much. I assumed that adding intermediate once to store is cheap enough so I decided not to care. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's one thing to check:
If the property resets, we need to do complex stuff here. If it doesn't, we're good. (I think we do "merge properties", which should (generally) be safe, but it's good to test that before we run the risk of altering machine state) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that assumes same CA set between runs, right? (right now we generate unique set for each test run and I had situation where it would build up before adding cleanup to the tests) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It assumes that we ever get something in the
And intermed1 is already in the store, but intermed2 wasn't; we'll end up calling store.Add(intermed1) which will cause the merge logic to apply. Since it changes system persisted state we need to be careful to ensure it's only additive, not destructive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here is what I did:
so the conclusion is that adding existing certificate is not destructive. |
||
| if (!osCanBuildChain) | ||
| { | ||
| X509Store? store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine); | ||
|
|
||
| try | ||
| { | ||
| store.Open(OpenFlags.ReadWrite); | ||
| } | ||
| catch | ||
| { | ||
| // If using system store fails, try to fall-back to user store. | ||
| store.Dispose(); | ||
| store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser); | ||
| try | ||
| { | ||
| store.Open(OpenFlags.ReadWrite); | ||
| } | ||
| catch | ||
| { | ||
| store.Dispose(); | ||
| store = null; | ||
| if (NetEventSource.IsEnabled) | ||
| { | ||
| NetEventSource.Error(this, $"Failed to open certificate store for intermediates."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (store != null) | ||
| { | ||
| using (store) | ||
| { | ||
| // Add everything except the root | ||
| for (int index = count; index < intermediates.Length - 1; index++) | ||
| { | ||
| store.Add(intermediates[index]); | ||
bartonjs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| osCanBuildChain = chain.Build(target); | ||
| foreach (X509ChainStatus status in chain.ChainStatus) | ||
| { | ||
| if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid)) | ||
| { | ||
| osCanBuildChain = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!osCanBuildChain) | ||
| { | ||
| // Add also root to Intermediate CA store so OS can complete building chain. | ||
| // (This does not make it trusted. | ||
| store.Add(intermediates[intermediates.Length - 1]); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Certificate = target; | ||
| IntermediateCertificates = intermediates; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.