{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/corefx/issues/10135","repository_url":"https://api.github.com/repos/dotnet/corefx","labels_url":"https://api.github.com/repos/dotnet/corefx/issues/10135/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/corefx/issues/10135/comments","events_url":"https://api.github.com/repos/dotnet/corefx/issues/10135/events","html_url":"https://github.com/dotnet/corefx/pull/10135","id":166224980,"number":10135,"title":"Change dispose logic of certificates in OpenSslX509ChainProcessor","user":{"login":"iamjasonp","id":1616847,"avatar_url":"https://avatars.githubusercontent.com/u/1616847?v=3","gravatar_id":"","url":"https://api.github.com/users/iamjasonp","html_url":"https://github.com/iamjasonp","followers_url":"https://api.github.com/users/iamjasonp/followers","following_url":"https://api.github.com/users/iamjasonp/following{/other_user}","gists_url":"https://api.github.com/users/iamjasonp/gists{/gist_id}","starred_url":"https://api.github.com/users/iamjasonp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/iamjasonp/subscriptions","organizations_url":"https://api.github.com/users/iamjasonp/orgs","repos_url":"https://api.github.com/users/iamjasonp/repos","events_url":"https://api.github.com/users/iamjasonp/events{/privacy}","received_events_url":"https://api.github.com/users/iamjasonp/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dotnet/corefx/labels/3%20-%20Ready%20For%20Review","name":"3 - Ready For Review","color":"CCCCCC"},{"url":"https://api.github.com/repos/dotnet/corefx/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2016-07-19T00:48:01Z","updated_at":"2016-07-20T03:23:45Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/corefx/pulls/10135","html_url":"https://github.com/dotnet/corefx/pull/10135","diff_url":"https://github.com/dotnet/corefx/pull/10135.diff","patch_url":"https://github.com/dotnet/corefx/pull/10135.patch"},"body":"Currently: certificates are loaded from both the System root store and also from a user root store. Once finished, OpenSslX509ChainProcessor.FindCandidates will go thorugh the certs and dispose of ones that are deemed not needed.\r\n\r\nThis particular approach caused an issue when the same certificate appears in both the System root certificate store AND the user root certificate store. The following chain of events would happen:\r\n\r\n1. Certs would be loaded from the system root cert store into `systemTrusted` (a HashSet scoped to the method)\r\n2. Certs would be loaded from the user root cert store into `systemTrusted` However, if the same cert was found, HashSet would not add it into `systemTrusted` and will fail silently\r\n3. FindIssuer happens to find the issuer that comes from the user's root store based on the ordering in `storesToCheck`\r\n4. The candidate list is populated with the end cert and a the root cert coming from the user store\r\n6. Eventually we create the `candidatesByReference` HashSet from the list of candidate certificates (which is the user root store one, not the system store one)\r\n5. The certificate coming from the system root cert store is Disposed (no references to it)\r\n6. However, since `systemTrusted` only contained the root cert from the system store and not the user store, and ultimately `OpenSslX509ChainProcessor.BuildChain` tries to build from the candidate (user root cert) but uses the `systemTrusted` HashSet (system root cert), the chain fails to build.\r\n\r\nThis fix would do a deep check into `systemTrusted`'s certificates to match using X509Certificate2's comparer rather than using a (likely faster) ref comparer; this prevents the issue of disposing a certificate that might ultimately be used.\r\n\r\nThere are a couple of alternatives that I milled over, but ultimately I thought that the other ways might cause more confusion/code complication - for example, it might be possible to always read in certs into collections with ReferenceEqualityComparers instead of the current hybrid model where we only use the ReferenceEqualityComparers upon cleanup time\r\n\r\ncc @bartonjs \r\n\r\nP.S. - this is blocking WCF testing scenarios from working"},"comment":{"url":"https://api.github.com/repos/dotnet/corefx/issues/comments/233828614","html_url":"https://github.com/dotnet/corefx/pull/10135#issuecomment-233828614","issue_url":"https://api.github.com/repos/dotnet/corefx/issues/10135","id":233828614,"user":{"login":"iamjasonp","id":1616847,"avatar_url":"https://avatars.githubusercontent.com/u/1616847?v=3","gravatar_id":"","url":"https://api.github.com/users/iamjasonp","html_url":"https://github.com/iamjasonp","followers_url":"https://api.github.com/users/iamjasonp/followers","following_url":"https://api.github.com/users/iamjasonp/following{/other_user}","gists_url":"https://api.github.com/users/iamjasonp/gists{/gist_id}","starred_url":"https://api.github.com/users/iamjasonp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/iamjasonp/subscriptions","organizations_url":"https://api.github.com/users/iamjasonp/orgs","repos_url":"https://api.github.com/users/iamjasonp/repos","events_url":"https://api.github.com/users/iamjasonp/events{/privacy}","received_events_url":"https://api.github.com/users/iamjasonp/received_events","type":"User","site_admin":false},"created_at":"2016-07-20T03:23:45Z","updated_at":"2016-07-20T03:23:45Z","body":"Verified that the [new test](http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/master/job/outerloop_ubuntu14.04_debug_prtest/22/testReport/System.Security.Cryptography.X509Certificates.Tests/ChainTests/) is passing in outerloops. Squashing now"}}
{"actor":{"display_login":"iamjasonp"}}