Issue 2101303005: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning loop.

mattm

mattm@chromium.org changed reviewers: + rsleevi@chromium.org

4 years, 5 months ago (2016-06-30 01:18:07 UTC) #1

Ryan Sleevi

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_mac.cc File net/cert/cert_verify_proc_mac.cc (right): https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_mac.cc#newcode644 net/cert/cert_verify_proc_mac.cc:644: &keychain); Can we not just use the last keychain? ...

4 years, 5 months ago (2016-06-30 01:49:16 UTC) #3

Ryan Sleevi

https://codereview.chromium.org/2101303005/diff/1/net/cert/test_keychain_search_list_mac.h File net/cert/test_keychain_search_list_mac.h (right): https://codereview.chromium.org/2101303005/diff/1/net/cert/test_keychain_search_list_mac.h#newcode16 net/cert/test_keychain_search_list_mac.h:16: class NET_EXPORT TestKeychainSearchList { For this, do we need ...

4 years, 5 months ago (2016-06-30 01:50:00 UTC) #4

mattm

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_mac.cc File net/cert/cert_verify_proc_mac.cc (right): https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_mac.cc#newcode644 net/cert/cert_verify_proc_mac.cc:644: &keychain); On 2016/06/30 01:49:16, Ryan Sleevi wrote: > Can ...

4 years, 5 months ago (2016-06-30 03:20:53 UTC) #6

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_m...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_m...
net/cert/cert_verify_proc_mac.cc:644: &keychain);
On 2016/06/30 01:49:16, Ryan Sleevi wrote:
> Can we not just use the last keychain? Or the name?
> 
> I ask because Apple's changed this path 3x now, so I'm a bit nervous on the
> gross-hack side :)

Unfortunately, SystemRootCertificates is not part of the list that
SecKeychainCopySearchList returns.

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_m...
net/cert/cert_verify_proc_mac.cc:655: }
On 2016/06/30 01:49:16, Ryan Sleevi wrote:
> Suggestion: Re-order this before the conditional, since it's duplicated
> 
> if (try_reordered_keychain || TestKeychainSearchList::HasInstance()) {
>   status = ...
>   scoped_alternate_keychain_search_list = ;
> }
> if (try_reordered_keychain) {
>   if (!TKSL::HasInstance()) {
>    status = SecKeychainCopySearchList();
>    scoped_alternate_keychain_search_list = ...;
>   }
>   // Reverse the order?
> }

Done.

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_m...
net/cert/cert_verify_proc_mac.cc:661: // chain building.
On 2016/06/30 01:49:16, Ryan Sleevi wrote:
> I actually think we can nuke all of this (657-661) because all of that's been
> moved into BuildAndEvaluateSecTrustRef

Done.

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_u...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2101303005/diff/1/net/cert/cert_verify_proc_u...
net/cert/cert_verify_proc_unittest.cc:152: #if defined(OS_WIN) ||
defined(USE_NSS_CERTS)
On 2016/06/30 01:49:16, Ryan Sleevi wrote:
> Can this be set for OS_MACOSX now? If not, should we update the comment
> explaining why? Is there a way to fold in the new code to the existing
> unittests?

It still fails, since adding CRLSet checking to the chopping loop only helps in
cases where chopping things off the list of intermediates allows it to build a
better path. 
It could work if the test was changed to only give the bad path and see if it
can fetch the correct path via AIA from a test server, though that's more work
to set up. I could look into that if you think it's worthwhile.

> 
> I'm trying to think how we can make the tests cross-platform, since we totally
> desire the results to be cross-platform.

https://codereview.chromium.org/2101303005/diff/1/net/cert/test_keychain_sear...
File net/cert/test_keychain_search_list_mac.h (right):

https://codereview.chromium.org/2101303005/diff/1/net/cert/test_keychain_sear...
net/cert/test_keychain_search_list_mac.h:16: class NET_EXPORT
TestKeychainSearchList {
On 2016/06/30 01:50:00, Ryan Sleevi wrote:
> For this, do we need a dedicated class? Or can this logic be part of
> FixupSecTrustRef on the TestRootCerts?

The keychain reordering hack in CertVerifyProcMac needs to see the test keychain
list, so I don't see a way to do it. If it wasn't for that it would work.

Ryan Sleevi

https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn#newcode1474 net/BUILD.gn:1474: libs = [ "Security.framework" ] I thought we also ...

4 years, 4 months ago (2016-08-12 19:50:17 UTC) #7

mattm

https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn#newcode1474 net/BUILD.gn:1474: libs = [ "Security.framework" ] On 2016/08/12 19:50:17, Ryan ...

4 years, 4 months ago (2016-08-16 01:41:34 UTC) #9

https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2101303005/diff/40001/net/BUILD.gn#newcode1474
net/BUILD.gn:1474: libs = [ "Security.framework" ]
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> I thought we also needed to add Foundation.framework (because of CFArray)?

I think this is okay since the cert_verify_proc_unittest.cc doesn't use CFArray,
only SecKeychainRef.

test_keychain_search_list_mac and cert_verify_proc_mac use CFArray, but those
are in net (rather than net_unittests) and net already has both
Foundation.framework and Security.framework.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:627: }
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> Could you add some additional documentation here explaining the relationships
> between keychain_search_list, try_reordered_keychain, and
> scoped_alternate_keychain_search_list? I struggled a bit to make sense of this
> all when coming back to it.
> 
> if (TestKeychainSearchList::HasInstance()) {
>   // Unit tests need to be able to hermetically simulate situations
>   // where a user has an undesirable certificate in a per-user keychain.
>   // Adding/Removing a Keychain using [API here] has global side effects,
>   // which would break other tests and processes running on the same
>   // machine, so instead ... [explain some logic or point to the other
>   // explainer].
> }

Done.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:649: &keychain);
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> DESIGN: Is it worth adding a unit test for this call as a sentinel for future
OS
> uprevs (e.g. if this fails, it'll take down some tests with it, but we could
> pinpoint it if a specific unittest was failing and was documented as such)

Done.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:660: while (CFArrayGetCount(cert_array) > 0) {
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> I'm a little nervous that this is moved far away from the giant wall of text
> documentation, in that now I have to scroll an additional screen's worth up in
> order to match implementation and execution.
> 
> Perhaps it's suitable just to include a line or two that links this part of
the
> algorithm with the above implementation documentation, like
> 
> // Beginning with the certificate chain as supplied by the server,
> // attempt to verify the chain. If a failure is encountered, trim
> // a certificate from the end (so long as one remains) and retry, in
> // the hope of forcing OS X to find a better path.

Done.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:674: (crl_set &&
!CheckRevocationWithCRLSet(temp_chain, crl_set));
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> I suspect we should do with some documentation here about some of the nuances
of
> CRLSets. If you're not comfortable writing it up, LMK and I'll try to do
> something.

That would be great since I'm not sure what the nuances are myself.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1445: GetTestCertsDirectory(),
"tripadvisor-verisign-chain.pem",
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> Can we generate a test cert that demonstrates the problem, now that you've
made
> sure the 'real' cert thing is fixed ;)

I didn't like including this, but I think to do that we would need to be able to
generate a test cert that chains to a root in SystemRootCertificates.keychain.

https://codereview.chromium.org/2101303005/diff/40001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/2101303005/diff/40001/net/data/ssl/certificat...
net/data/ssl/certificates/README:69: certificates "multi-root-B-by-F.pem" and
"multi-root-F-by-E.pem".
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> This is in the wrong section; it's not a 'real' cert.

Done.

https://codereview.chromium.org/2101303005/diff/40001/net/data/ssl/certificat...
File net/data/ssl/certificates/tripadvisor-verisign-chain.pem (right):

https://codereview.chromium.org/2101303005/diff/40001/net/data/ssl/certificat...
net/data/ssl/certificates/tripadvisor-verisign-chain.pem:1: -----BEGIN
CERTIFICATE-----
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> Can you include the "openssl x509 -text" output for both these certs in the
> chain? See some of the other files for examples.

Done.

https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (left):

https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9378: // Doesn't pass on OS X yet for
reasons that need to be investigated.
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> I think this may start causing flaky failures, because of OS X's OCSP cache
> (which is global and we don't flush)

Isn't that a bigger problem then? Wouldn't this test running at all have a
chance of causing future flakes in any of the other tests that use the same ocsp
test root (such as HTTPSOCSPTest.Valid)?

https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9402:
EXPECT_EQ(ExpectedCertStatusForFailedOnlineRevocationCheck(),
On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> Looking at the definition of this, shouldn't you just update the function to
> support OS X as well?

Done. I had to fix HTTPSEVCRLSetTest.MissingCRLSetAndInvalidOCSP, so you should
check that my fix there makes sense.

mattm

https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_request_unittest.cc File net/url_request/url_request_unittest.cc (left): https://codereview.chromium.org/2101303005/diff/40001/net/url_request/url_request_unittest.cc#oldcode9378 net/url_request/url_request_unittest.cc:9378: // Doesn't pass on OS X yet for reasons ...

4 years, 4 months ago (2016-08-16 02:14:31 UTC) #10

Ryan Sleevi

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_proc_mac.cc File net/cert/cert_verify_proc_mac.cc (right): https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_proc_mac.cc#newcode674 net/cert/cert_verify_proc_mac.cc:674: (crl_set && !CheckRevocationWithCRLSet(temp_chain, crl_set)); On 2016/08/16 01:41:34, mattm wrote: ...

4 years, 4 months ago (2016-08-17 02:28:13 UTC) #11

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:674: (crl_set &&
!CheckRevocationWithCRLSet(temp_chain, crl_set));
On 2016/08/16 01:41:34, mattm wrote:
> That would be great since I'm not sure what the nuances are myself.

// Check to see if the path |temp_chain| has been revoked. This is
// less than ideal to perform after path building, rather than during,
// because there may be multiple paths to trust anchors, and only some
// of them are revoked. Ideally, CRLSets would be part of path
// building, which they are when using NSS (Linux) or CryptoAPI (Windows).
//
// The CRLSet checking is performed inside the loop in the hope that
// if a path is revoked, it's an older path, and the only reason it
// was built is because the server forced it (by supplying an older
// or less desirable intermediate) or because the user had installed
// a certificate in their Keychain forcing this path. However, this
// means its still possible for a CRLSet block of an intermediate to
// prevent access, even when there is a 'good' chain. To fully
// remedy this, a solution might be to have CRLSets contain enough
// knowledge about what the 'desired' path might be, but for the
// time being, the implementation is kept as 'simple' as it can be.

Sorry, I should have elaborated more on what I meant by "nuances" here. Mostly,
I was thinking about the risk of why CRLSets post-chain is less than ideal. I'm
not sure if we should reference the unittests here, but I think I could live
with it, and we could say something like "See
//net/data/ssl/scripts/generate-multi-root-test-chains.sh to better understand
how the PKI graph can look."

*shrug*

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1445: GetTestCertsDirectory(),
"tripadvisor-verisign-chain.pem",
On 2016/08/16 01:41:34, mattm wrote:
> On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> > Can we generate a test cert that demonstrates the problem, now that you've
> made
> > sure the 'real' cert thing is fixed ;)
> 
> I didn't like including this, but I think to do that we would need to be able
to
> generate a test cert that chains to a root in SystemRootCertificates.keychain.

Can we get away without hardcoding SytemRootCertificates.keychain? Such as by
simply rotating the keychain list (so last becomes first, first becomes second,
etc)?

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-17 03:29:06 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/120001

4 years, 4 months ago (2016-08-17 03:29:16 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-17 03:31:39 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/239551) linux_chromium_asan_rel_ng on ...

4 years, 4 months ago (2016-08-17 03:31:40 UTC) #15

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-17 03:50:45 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/160001

4 years, 4 months ago (2016-08-17 03:50:55 UTC) #18

mattm

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_proc_mac.cc File net/cert/cert_verify_proc_mac.cc (right): https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_proc_mac.cc#newcode674 net/cert/cert_verify_proc_mac.cc:674: (crl_set && !CheckRevocationWithCRLSet(temp_chain, crl_set)); On 2016/08/17 02:28:12, Ryan Sleevi ...

4 years, 4 months ago (2016-08-17 03:50:57 UTC) #19

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:674: (crl_set &&
!CheckRevocationWithCRLSet(temp_chain, crl_set));
On 2016/08/17 02:28:12, Ryan Sleevi (slow) wrote:
> On 2016/08/16 01:41:34, mattm wrote:
> > That would be great since I'm not sure what the nuances are myself.
> 
> // Check to see if the path |temp_chain| has been revoked. This is
> // less than ideal to perform after path building, rather than during,
> // because there may be multiple paths to trust anchors, and only some
> // of them are revoked. Ideally, CRLSets would be part of path
> // building, which they are when using NSS (Linux) or CryptoAPI (Windows).
> //
> // The CRLSet checking is performed inside the loop in the hope that
> // if a path is revoked, it's an older path, and the only reason it
> // was built is because the server forced it (by supplying an older
> // or less desirable intermediate) or because the user had installed
> // a certificate in their Keychain forcing this path. However, this
> // means its still possible for a CRLSet block of an intermediate to
> // prevent access, even when there is a 'good' chain. To fully
> // remedy this, a solution might be to have CRLSets contain enough
> // knowledge about what the 'desired' path might be, but for the
> // time being, the implementation is kept as 'simple' as it can be.
> 
> 
> 
> Sorry, I should have elaborated more on what I meant by "nuances" here.
Mostly,
> I was thinking about the risk of why CRLSets post-chain is less than ideal. 

Ah, that. Done.

> I'm
> not sure if we should reference the unittests here, but I think I could live
> with it, and we could say something like "See
> //net/data/ssl/scripts/generate-multi-root-test-chains.sh to better understand
> how the PKI graph can look."
> 
> *shrug*

eh, comment is pretty long already :) I can add it if you want though.

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2101303005/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1445: GetTestCertsDirectory(),
"tripadvisor-verisign-chain.pem",
On 2016/08/17 02:28:12, Ryan Sleevi (slow) wrote:
> On 2016/08/16 01:41:34, mattm wrote:
> > On 2016/08/12 19:50:17, Ryan Sleevi (slow) wrote:
> > > Can we generate a test cert that demonstrates the problem, now that you've
> > made
> > > sure the 'real' cert thing is fixed ;)
> > 
> > I didn't like including this, but I think to do that we would need to be
able
> to
> > generate a test cert that chains to a root in
SystemRootCertificates.keychain.
> 
> Can we get away without hardcoding SytemRootCertificates.keychain? Such as by
> simply rotating the keychain list (so last becomes first, first becomes
second,
> etc)?

Unfortunately, SystemRootCertificates is not in the list returned by
SecKeychainCopySearchList. Expanded the comment about that in
cert_verify_proc_mac.cc.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-17 03:53:25 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/239559) mac_chromium_compile_dbg_ng on ...

4 years, 4 months ago (2016-08-17 03:53:26 UTC) #21

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-09 23:02:10 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/200001

4 years, 3 months ago (2016-09-09 23:02:37 UTC) #23

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-10 00:29:29 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/294031)

4 years, 3 months ago (2016-09-10 00:29:30 UTC) #25

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-13 01:27:40 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/260001

4 years, 3 months ago (2016-09-13 01:28:00 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-13 03:59:41 UTC) #30

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-13 03:59:42 UTC) #31

Ryan Sleevi

LGTM w/ one design question (below) compared to TestRootCerts (and other Singleton-y things) Also, should ...

4 years, 3 months ago (2016-09-13 22:46:54 UTC) #33

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-14 02:29:10 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/280001

4 years, 3 months ago (2016-09-14 02:29:32 UTC) #35

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-14 02:32:39 UTC) #36

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/226056) linux_chromium_rel_ng on ...

4 years, 3 months ago (2016-09-14 02:32:42 UTC) #37

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-14 02:49:12 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/280001

4 years, 3 months ago (2016-09-14 02:49:29 UTC) #39

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-14 02:52:36 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/277842) linux_chromium_compile_dbg_ng on ...

4 years, 3 months ago (2016-09-14 02:52:37 UTC) #41

mattm

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-14 03:03:49 UTC) #42

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/300001

4 years, 3 months ago (2016-09-14 03:04:05 UTC) #43

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-14 04:09:30 UTC) #44

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-14 04:09:30 UTC) #45

mattm

On 2016/09/13 22:46:54, Ryan Sleevi (slow) wrote: > LGTM w/ one design question (below) compared ...

4 years, 3 months ago (2016-09-14 05:52:57 UTC) #46

On 2016/09/13 22:46:54, Ryan Sleevi (slow) wrote:
> LGTM w/ one design question (below) compared to TestRootCerts (and other
> Singleton-y things)
> 
> Also, should we add some "create a keychain" script ala
>
https://opensource.apple.com/source/security_certificates/security_certificat...
> for generating the hand-keychains? Or not bother?

done.

https://codereview.chromium.org/2101303005/diff/260001/net/cert/test_keychain...
File net/cert/test_keychain_search_list_mac.h (right):

https://codereview.chromium.org/2101303005/diff/260001/net/cert/test_keychain...
net/cert/test_keychain_search_list_mac.h:24: static
std::unique_ptr<TestKeychainSearchList> Create();
On 2016/09/13 22:46:54, Ryan Sleevi (slow) wrote:
> I'm a little curious why the pseudo-singletonness.
> 
> That is, why expose the public dtor and the Create() method, but then expose
> HasInstance()/GetInstance()?

It is a little different than TestRootCerts since there can only be one search
list, whereas even though TestRootCerts is a singleton you can add multiple
certs to it. 

I kinda like this better than the TestRootCerts way, since it returns a scoped
ptr so it makes you clean up automatically rather than needing to call Clear in
a TearDown method. 

I guess TestKeychainSearchList could actually be a Singleton, I guess it could
then have some method that would return a ScopedKeychainSearchList or something.
But that just seems like adding an extra layer without a benefit.

https://codereview.chromium.org/2101303005/diff/280001/net/data/ssl/certifica...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/2101303005/diff/280001/net/data/ssl/certifica...
net/data/ssl/certificates/README:68:
scripts/generate-verisign_class3_g5_crosssigned-trusted-keychain.sh)
I left this in this section since although the keychain file is generated by a
script, it's based on a real-world cert.

https://codereview.chromium.org/2101303005/diff/280001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-keychain.sh (right):

https://codereview.chromium.org/2101303005/diff/280001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-keychain.sh:26: # (or does it?)
On my machine it didn't seem to. But I left the search list saving/restoring in
anyway just to be safe, I guess?

mattm

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the ...

4 years, 3 months ago (2016-09-15 00:46:57 UTC) #48

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101303005/300001

4 years, 3 months ago (2016-09-15 00:47:45 UTC) #49

commit-bot: I haz the power

Description was changed from ========== CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning ...

4 years, 3 months ago (2016-09-15 00:56:01 UTC) #51

commit-bot: I haz the power

Patchset 11 (id:??) landed as https://crrev.com/9cedf75377d817c6b32a01f1d30fbe10663b8bb8 Cr-Commit-Position: refs/heads/master@{#418732}

4 years, 3 months ago (2016-09-15 00:56:03 UTC) #52

davidben

4 years, 3 months ago (2016-09-16 15:21:47 UTC) #53

Message was sent while issue was closed.

A revert of this CL (patchset #11 id:300001) has been created in
https://codereview.chromium.org/2347893002/ by davidben@chromium.org.

The reason for reverting is: This breaks verification on OS X 10.12 and probably
needs some further investigation.

BUG=647241.

Issue 2101303005: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning loop. (Closed)

Description

Patch Set 1 #

Patch Set 2 : review changes and url_request_unittest fixes #

Patch Set 3 : rebase #

Patch Set 4 : updates for comment #7 #

Patch Set 5 : updates for comment #11 #

Patch Set 6 : rebase #

Patch Set 7 : rebase #

Patch Set 8 : fix pem "text" that was breaking reitveld patch #

Patch Set 9 : fix other tests that broke on the ExpectedCertStatusForFailedOnlineRevocationCheck change #

Patch Set 10 : Scripts for keychain generation #

Patch Set 11 : rebase #

Messages