NSS Cros multiprofile: trust roots added by a profile shouldn't apply to other profiles.

NSS Cros multiprofile: trust roots added by a profile shouldn't apply to other profiles.

BUG=218627
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249928

[mattm, 2014-01-22 00:21:39]

This just addresses the simple case which should be "good enough" for
multiprofiles to launch.

Questions:
Could there be any issue with the system request's certverifyproc not allowing
user-trusted roots?

Was I correct that the tpm slot isn't necessary for the CertVerifyProcChromeOS?

[mattm, 2014-01-22 00:31:41]

On 2014/01/22 00:21:39, mattm wrote:
> This just addresses the simple case which should be "good enough" for
> multiprofiles to launch.
> 
> Questions:
> Could there be any issue with the system request's certverifyproc not allowing
> user-trusted roots?
> 
> Was I correct that the tpm slot isn't necessary for the
CertVerifyProcChromeOS?

Also in my testing I found that explicitly trusted server certs don't end up
calling the chainVerifyCallback, is that expected?

[Ryan Sleevi, 2014-01-22 00:41:34]

On 2014/01/22 00:31:41, mattm wrote:
> On 2014/01/22 00:21:39, mattm wrote:
> > This just addresses the simple case which should be "good enough" for
> > multiprofiles to launch.
> > 
> > Questions:
> > Could there be any issue with the system request's certverifyproc not
allowing
> > user-trusted roots?
> > 
> > Was I correct that the tpm slot isn't necessary for the
> CertVerifyProcChromeOS?
> 
> Also in my testing I found that explicitly trusted server certs don't end up
> calling the chainVerifyCallback, is that expected?

Yes.

[Ryan Sleevi, 2014-01-22 00:43:47]

I cannot convince myself this is correct with respect to NSS's internal caches
of chain validation, and I don't know that I'll have the few days needed to dig
deep into libpkix to convince myself of this being safe/correct/without
side-effects.

This is why when discussing this feature, this was listed as a "nice to have",
but not a "must have". Has this changed?

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.h:42: void
InitializeOnIOThread(net::CertVerifyProc* verify_proc);
At the risk of extra typing, can you pass this as

"const scoped_refptr<net::CertVerifyProc>& verify_proc"

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:47:
cert_verifier_->InitializeOnIOThread(net::CertVerifyProc::CreateDefault());
This is potentially dangerous as written, since if cert_verifier_ (or more
aptly, MultiThreadedCertVerifier) fails to grab a ref, this is leaked. Hence the
change request.

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.cc:39:
PR_LIST_TAIL(&current_chain->list))->cert;
NACK on using PR_LIST_TAIL for this type.

1) File a bug in NSS to add CERT_LIST_TAIL
2) Add an #ifdef guard ~line 6 to handle when this is being added

#ifndef CERT_LIST_TAIL
#define CERT_LIST_TAIL(l) ((CERTCertListNode *)PR_LIST_TAIL(&l->list))
#endif

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.h:22: // cert but with different trust
values).
The whole discussion of "profiles" is certainly something that doesn't seem like
it belongs in net/ at all. Much like the policy cert verifier, this seems like
it belongs in chrome/ (where the notion of profiles exists).

[Ryan Sleevi, 2014-01-22 00:46:58]

Also, few requests for liberal documentation. Easier to err on too much
documentation than too little.

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_nss.h:19: explicit
CertVerifyProcNSS(CERTChainVerifyCallback* chain_verify_callback);
Documentation.

https://codereview.chromium.org/137553004/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:19: class NET_EXPORT
NSSProfileFilterChromeOS {
Follow up request: Document.

[mattm, 2014-01-24 04:47:30]

I expanded the test some (and moved it to
net/cert/cert_verify_proc_chromeos_unittest.cc).

I did run into a problem, sometime the verify callback is getting passed a
trusted root cert that was deleted or was part of a previous test. The cert
object passed to the verify callback is a temporary one, so it's allowed by the
first check in NSSProfileFilterChromeOS::IsCertAllowed.

If I change that check to return false instead of true all these tests pass, but
other tests fail (ones which depend on ScopedTestCert or similar). I noticed
when I do that change, the stale cert is only passed to the callback on the
first call, once we reject it it seems to get released from some cache and
doesn't even come up in further calls to the verify callback. OTOH if the verify
callback allows it, it will continue being used on subsequent calls.


Places where I see these old temp certs getting passed to the callback:
In CertVerifyProcChromeOSTest.TestChainVerify
1. The tests after deleting the D root work correctly, but the tests after
deleting the E root fail.

2. When running multiple tests in a row, the later tests will fail because with
the D root or E root cert getting passed to the callback.   

If I run the tests with --test-launcher-batch-limit=1 so that each gets run in
its own process, the tests that failed due to running multiple in a row pass.

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.h:42: void
InitializeOnIOThread(net::CertVerifyProc* verify_proc);
On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> At the risk of extra typing, can you pass this as
> 
> "const scoped_refptr<net::CertVerifyProc>& verify_proc"

Done.

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/137553004/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:47:
cert_verifier_->InitializeOnIOThread(net::CertVerifyProc::CreateDefault());
On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> This is potentially dangerous as written, since if cert_verifier_ (or more
> aptly, MultiThreadedCertVerifier) fails to grab a ref, this is leaked. Hence
the
> change request.

Done.

(Also had to change it to specify a slot for one of the tests.)

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.cc:39:
PR_LIST_TAIL(&current_chain->list))->cert;
On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> NACK on using PR_LIST_TAIL for this type.
> 
> 1) File a bug in NSS to add CERT_LIST_TAIL
> 2) Add an #ifdef guard ~line 6 to handle when this is being added
> 
> #ifndef CERT_LIST_TAIL
> #define CERT_LIST_TAIL(l) ((CERTCertListNode *)PR_LIST_TAIL(&l->list))
> #endif

Done.

https://bugzilla.mozilla.org/show_bug.cgi?id=962413

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.h:22: // cert but with different trust
values).
On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> The whole discussion of "profiles" is certainly something that doesn't seem
like
> it belongs in net/ at all. Much like the policy cert verifier, this seems like
> it belongs in chrome/ (where the notion of profiles exists).

If it's defined in chrome/, we couldn't have CertVerifyProc::CreateDefault
return one.

I've rewritten the text to remove references to profiles.

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_nss.h:19: explicit
CertVerifyProcNSS(CERTChainVerifyCallback* chain_verify_callback);
On 2014/01/22 00:46:59, Ryan Sleevi wrote:
> Documentation.

Done.

https://codereview.chromium.org/137553004/diff/30001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:19: class NET_EXPORT
NSSProfileFilterChromeOS {
On 2014/01/22 00:46:59, Ryan Sleevi wrote:
> Follow up request: Document.

Done.

[Ryan Sleevi, 2014-01-25 01:50:16]

On 2014/01/24 04:47:30, mattm wrote:
> I expanded the test some (and moved it to
> net/cert/cert_verify_proc_chromeos_unittest.cc).
> 
> I did run into a problem, sometime the verify callback is getting passed a
> trusted root cert that was deleted or was part of a previous test. The cert
> object passed to the verify callback is a temporary one, so it's allowed by
the
> first check in NSSProfileFilterChromeOS::IsCertAllowed.
> 
> If I change that check to return false instead of true all these tests pass,
but
> other tests fail (ones which depend on ScopedTestCert or similar). I noticed
> when I do that change, the stale cert is only passed to the callback on the
> first call, once we reject it it seems to get released from some cache and
> doesn't even come up in further calls to the verify callback. OTOH if the
verify
> callback allows it, it will continue being used on subsequent calls.

In "normal" operation (eg: when not using ScopedTestRoot), I would expect that
any trusted roots must belong to at least one token.

The reason that net/cert/test_root_certs.h isn't in test-only code is to allow
implementations to determine if they're running under test, and in a way that
the compiler can optimize out under WPO.


if (TestRootCerts::HasInstance()) {
  TestRootCerts* root_certs = TestRootCerts::GetInstance();
  if (root_certs->Contains(cert))
    return true;
  return false;
}

Note: This would go under CertVerifyProc, not in the filter.

> 
> 
> Places where I see these old temp certs getting passed to the callback:
> In CertVerifyProcChromeOSTest.TestChainVerify
> 1. The tests after deleting the D root work correctly, but the tests after
> deleting the E root fail.
> 
> 2. When running multiple tests in a row, the later tests will fail because
with
> the D root or E root cert getting passed to the callback.   
> 
> If I run the tests with --test-launcher-batch-limit=1 so that each gets run in
> its own process, the tests that failed due to running multiple in a row pass.
>

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.h:22: // cert but with different trust
values).
On 2014/01/24 04:47:31, mattm wrote:
> On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> > The whole discussion of "profiles" is certainly something that doesn't seem
> like
> > it belongs in net/ at all. Much like the policy cert verifier, this seems
like
> > it belongs in chrome/ (where the notion of profiles exists).
> 
> If it's defined in chrome/, we couldn't have CertVerifyProc::CreateDefault
> return one.
> 
> I've rewritten the text to remove references to profiles.

I don't understand why this is an issue. It still belongs in Chrome proper, much
like the imported trust anchors. CertVerifier is created in src/chrome, so it
can always supply a custom CertVerifyProc.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_chromeos.h:32: crypto::ScopedPK11Slot private_slot);
Why take a private slot? This doesn't make sense for the CrOS case, does it?

Also, the comment isn't clear, in that it refers to slot singular.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_chromeos_unittest.cc:156: EXPECT_EQ(ERR_CERT_REVOKED,
wrong error code?? revoked isnt right, should be untrusted like on 126

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:119: // chain_verify_callback.
wrong mapping

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:8: #include <certt.h>
forward declare, you're only using pointer types

[mattm, 2014-01-28 04:36:43]

So there are still two problem cases I found:

1) Passing additional trust roots to CertVerifyProc. I can probably fix this
with some refactoring so that the IsChainValidFunc can check if the cert is one
of the additional trust roots.. I'll look into that next.

2) Some client cert store tests also fail. Maybe
net/ssl/client_cert_store_chromeos_unittest.cc just shouldn't instantiate the
ClientCertStoreTest typed test cases?

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.h:22: // cert but with different trust
values).
On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> On 2014/01/24 04:47:31, mattm wrote:
> > On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> > > The whole discussion of "profiles" is certainly something that doesn't
seem
> > like
> > > it belongs in net/ at all. Much like the policy cert verifier, this seems
> like
> > > it belongs in chrome/ (where the notion of profiles exists).
> > 
> > If it's defined in chrome/, we couldn't have CertVerifyProc::CreateDefault
> > return one.
> > 
> > I've rewritten the text to remove references to profiles.
> 
> I don't understand why this is an issue. It still belongs in Chrome proper,
much
> like the imported trust anchors. CertVerifier is created in src/chrome, so it
> can always supply a custom CertVerifyProc.

Done.
guess I was worried about the number of places calling
CertVerifier::CreateDefault, but after looking at them I guess most of them
don't need this.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_chromeos.h:32: crypto::ScopedPK11Slot private_slot);
On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> Why take a private slot? This doesn't make sense for the CrOS case, does it?
> 
> Also, the comment isn't clear, in that it refers to slot singular.

Oops, yeah. Didn't finish an earlier change to only pass the public_slot.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_chromeos_unittest.cc:156: EXPECT_EQ(ERR_CERT_REVOKED,
On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> wrong error code?? revoked isnt right, should be untrusted like on 126

Done.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:119: // chain_verify_callback.
On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> wrong mapping

Done.

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/380001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:8: #include <certt.h>
On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> forward declare, you're only using pointer types

CERTChainVerifyCallback is an typedef of an unnamed struct, can't forward
declare.

[mattm, 2014-01-28 06:02:32]

On 2014/01/28 04:36:43, mattm wrote:
> So there are still two problem cases I found:
> 
> 1) Passing additional trust roots to CertVerifyProc. I can probably fix this
> with some refactoring so that the IsChainValidFunc can check if the cert is
one
> of the additional trust roots.. I'll look into that next.
> 
> 2) Some client cert store tests also fail. Maybe
> net/ssl/client_cert_store_chromeos_unittest.cc just shouldn't instantiate the
> ClientCertStoreTest typed test cases?


Updated to do those things. WDYT?

[Ryan Sleevi, 2014-01-30 05:27:39]

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/30001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_chromeos.h:22: // cert but with different trust
values).
On 2014/01/28 04:36:43, mattm wrote:
> On 2014/01/25 01:50:17, Ryan Sleevi wrote:
> > On 2014/01/24 04:47:31, mattm wrote:
> > > On 2014/01/22 00:43:48, Ryan Sleevi wrote:
> > > > The whole discussion of "profiles" is certainly something that doesn't
> seem
> > > like
> > > > it belongs in net/ at all. Much like the policy cert verifier, this
seems
> > like
> > > > it belongs in chrome/ (where the notion of profiles exists).
> > > 
> > > If it's defined in chrome/, we couldn't have CertVerifyProc::CreateDefault
> > > return one.
> > > 
> > > I've rewritten the text to remove references to profiles.
> > 
> > I don't understand why this is an issue. It still belongs in Chrome proper,
> much
> > like the imported trust anchors. CertVerifier is created in src/chrome, so
it
> > can always supply a custom CertVerifyProc.
> 
> Done.
> guess I was worried about the number of places calling
> CertVerifier::CreateDefault, but after looking at them I guess most of them
> don't need this.

Nothing outside of unit tests, the net::URLRequestContextBuilder, and "process
hosts" (eg: chrome, remoting) should be using CreateDefault. Definitely needs
fixing.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:70: if
(root_certs->Contains(cert)) {
At least here, you can short change with
net::TestRootCerts::GetInstance()->Contains(cert) since you only use
|root_certs| once

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:71: *chain_ok =
PR_TRUE;
Add a comment explaining why this check and short-circuit.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:81: *chain_ok =
PR_TRUE;
Ditto for adding comment explaining why

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:24: class NET_EXPORT
CertVerifyProcChromeOS : public net::CertVerifyProcNSS {
Update comment, remove net_export (and related include)

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:37:
CERTChainVerifyCallback* InitializeCERTChainVerifyCallback();
No longer used

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:47:
crypto::GetPublicSlotForChromeOSUser(user_2_.username_hash()));
cache public slots as part of init? (Note lines 37 and 32)

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:95:
*root_subject_name = root->subjectName;
root_subject_name->assign() ?

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:97:
*root_subject_name = "";
root_subject_name->clear()

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:124: // XXX
XXX - Document :)

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:192:
TEST_F(CertVerifyProcChromeOSTest, TestAdditionalTrustAnchors) {
Document?

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:213: // XXX
why doesn't this work with D Root CA?
Yes. Why doesn't it? :)

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:226:
EXPECT_EQ("CN=B CA", verify_root);
Test that User 2 is *not* succeeding (when an empty additional_trust_anchors)

Goal is to test cache effects

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:347:
ASSERT_TRUE(false);
s/ASSERT_TRUE(false)/FAIL()/ (or GTEST_FAIL if needed)

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:557:
globals_->cert_verifier.reset(net::CertVerifier::CreateDefault());
I think you should update this to use MultiThreadedCertVerifier as well,
otherwise you're coupling lines 554/555 to the implementation details of
CreateDefault, which is non-ideal.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:977: new
net::MultiThreadedCertVerifier(verify_proc.get()));
Why are you not using the io_thread_globals here?

https://codereview.chromium.org/137553004/diff/490001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:28: int VerifyInternalNSS(X509Certificate* cert,
VerifyInternalImpl ?

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_certs.h
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs.h:87: bool Contains(X509Certificate::OSCertHandle cert)
const;
Not a terrible fan of passing the OSCertHandle, although I understand why.

I'd be OK with Contains(CERTCertificate*) and Contains(X509*) in the
platform-specific bits (much like 61, 66, and 76 all directly reference native
types)

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
File net/cert/test_root_certs_nss.cc (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs_nss.cc:119: it != trust_cache_.end();
++it on this line

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
File net/cert/test_root_certs_openssl.cc (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs_openssl.cc:49: ++it) {
ditto on ++it

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-foo-test-chains.sh (right):

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-foo-test-chains.sh:7: # XXX This script generates
two chains of test certificates:
XXX ?

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-foo-test-chains.sh:197: > foo-chain1.pem
naming: better name than "foo"

[mattm, 2014-02-04 05:31:20]

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:70: if
(root_certs->Contains(cert)) {
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> At least here, you can short change with
> net::TestRootCerts::GetInstance()->Contains(cert) since you only use
> |root_certs| once

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:71: *chain_ok =
PR_TRUE;
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Add a comment explaining why this check and short-circuit.

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:81: *chain_ok =
PR_TRUE;
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Ditto for adding comment explaining why

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:24: class NET_EXPORT
CertVerifyProcChromeOS : public net::CertVerifyProcNSS {
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Update comment, remove net_export (and related include)

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:37:
CERTChainVerifyCallback* InitializeCERTChainVerifyCallback();
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> No longer used

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:47:
crypto::GetPublicSlotForChromeOSUser(user_2_.username_hash()));
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> cache public slots as part of init? (Note lines 37 and 32)

That ended up being messier looking, but there is an alternative of using the
NSSCertDatabase's GetPublicSlot() method, so I did that.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:95:
*root_subject_name = root->subjectName;
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> root_subject_name->assign() ?

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:97:
*root_subject_name = "";
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> root_subject_name->clear() 

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:124: // XXX
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> XXX - Document :)

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:192:
TEST_F(CertVerifyProcChromeOSTest, TestAdditionalTrustAnchors) {
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Document?

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:213: // XXX
why doesn't this work with D Root CA?
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Yes. Why doesn't it? :)

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:226:
EXPECT_EQ("CN=B CA", verify_root);
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Test that User 2 is *not* succeeding (when an empty additional_trust_anchors)
> 
> Goal is to test cache effects

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:347:
ASSERT_TRUE(false);
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> s/ASSERT_TRUE(false)/FAIL()/ (or GTEST_FAIL if needed)

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:557:
globals_->cert_verifier.reset(net::CertVerifier::CreateDefault());
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> I think you should update this to use MultiThreadedCertVerifier as well,
> otherwise you're coupling lines 554/555 to the implementation details of
> CreateDefault, which is non-ideal.

Done.

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/137553004/diff/490001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:977: new
net::MultiThreadedCertVerifier(verify_proc.get()));
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Why are you not using the io_thread_globals here?

Even if it's not using a PolicyCertVerifier for some reason, it should create a
verifier that's using the CertVerifyProcChromeOS for this profile instead of the
global one.

https://codereview.chromium.org/137553004/diff/490001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:28: int VerifyInternalNSS(X509Certificate* cert,
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> VerifyInternalImpl ?

Done.

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_certs.h
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs.h:87: bool Contains(X509Certificate::OSCertHandle cert)
const;
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> Not a terrible fan of passing the OSCertHandle, although I understand why.
> 
> I'd be OK with Contains(CERTCertificate*) and Contains(X509*) in the
> platform-specific bits (much like 61, 66, and 76 all directly reference native
> types)

Done.

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
File net/cert/test_root_certs_nss.cc (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs_nss.cc:119: it != trust_cache_.end();
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> ++it on this line

Done.

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
File net/cert/test_root_certs_openssl.cc (right):

https://codereview.chromium.org/137553004/diff/490001/net/cert/test_root_cert...
net/cert/test_root_certs_openssl.cc:49: ++it) {
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> ditto on ++it

Done.

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-foo-test-chains.sh (right):

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-foo-test-chains.sh:7: # XXX This script generates
two chains of test certificates:
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> XXX ?

Done.

https://codereview.chromium.org/137553004/diff/490001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-foo-test-chains.sh:197: > foo-chain1.pem
On 2014/01/30 05:27:40, Ryan Sleevi wrote:
> naming: better name than "foo"

Done.

[Ryan Sleevi, 2014-02-07 01:38:28]

LGTM, mod nits.

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:42: static SECStatus
IsChainValidFunc(void* is_chain_valid_arg,
Document

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:293: const
std::string test_order = std::tr1::get<2>(param);
nit: You should be able to write get<0>() and let Koenig lookup handle this,
which is future-friendly with GTEST_HAS_TR1_TUPLE and friends

https://codereview.chromium.org/137553004/diff/1410001/net/cert/test_root_cer...
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/137553004/diff/1410001/net/cert/test_root_cer...
net/cert/test_root_certs.h:79: bool Contains(X509* cert) const;
|X509| is not defined here. You need to forward declare it.

It only works because line 12, which we want to remove, and re-instate lines
32/33.

[mattm, 2014-02-07 03:38:03]

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:42: static SECStatus
IsChainValidFunc(void* is_chain_valid_arg,
On 2014/02/07 01:38:28, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc (right):

https://codereview.chromium.org/137553004/diff/1410001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc:293: const
std::string test_order = std::tr1::get<2>(param);
On 2014/02/07 01:38:28, Ryan Sleevi wrote:
> nit: You should be able to write get<0>() and let Koenig lookup handle this,
> which is future-friendly with GTEST_HAS_TR1_TUPLE and friends

This seems not to work because the compiler doesn't know to parse get<0> as a
templated function.
...
It looks like you can make it work by adding an unused forward declaration like:
template<int>
void get();

which causes the compiler to be able to parse it and then it is able to do
Koenig lookup, but that doesn't seem worth the trouble.

https://codereview.chromium.org/137553004/diff/1410001/net/cert/test_root_cer...
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/137553004/diff/1410001/net/cert/test_root_cer...
net/cert/test_root_certs.h:79: bool Contains(X509* cert) const;
On 2014/02/07 01:38:28, Ryan Sleevi wrote:
> |X509| is not defined here. You need to forward declare it.
> 
> It only works because line 12, which we want to remove, and re-instate lines
> 32/33.

Oops, done.

[mattm, 2014-02-07 03:57:50]

adding OWNERS:

derat: chrome/browser/chromeos/net/
pneubeck: chrome/browser/chromeos/policy/
mmenke: chrome/browser/profiles/
thestig: chrome/browser/io_thread.cc

[Lei Zhang, 2014-02-07 05:35:02]

c/b/io_thread.cc lgtm

[pneubeck (no reviews), 2014-02-07 09:33:50]

chrome/browser/chromeos/policy/
and profile_io_data.cc
lgtm

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:980:
cert_verifier_->InitializeOnIOThread(verify_proc.get());
why .get() ? InitializeOnIOThread accepts a const scoped_refptr&

[Daniel Erat, 2014-02-07 16:29:27]

lgtm as a chrome/browser/chromeos/net OWNER

(seeing that rsleevi has already reviewed the cert logic)

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:34: virtual int
VerifyInternal(
nit: mind adding a

  // net::CertVerifyProcNSS implementation:

comment above this so it's clear where it comes from?

[mattm, 2014-02-07 22:13:53]

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.h (right):

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.h:34: virtual int
VerifyInternal(
On 2014/02/07 16:29:28, Daniel Erat wrote:
> nit: mind adding a
> 
>   // net::CertVerifyProcNSS implementation:
> 
> comment above this so it's clear where it comes from?

Done.

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/137553004/diff/1650001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:980:
cert_verifier_->InitializeOnIOThread(verify_proc.get());
On 2014/02/07 09:33:51, pneubeck wrote:
> why .get() ? InitializeOnIOThread accepts a const scoped_refptr&

Good catch. Removed .get().

[mmenke, 2014-02-07 22:31:51]

profiles/ LGTM

[mattm, 2014-02-07 22:51:21]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-02-07 22:52:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/137553004/1870001

[commit-bot: I haz the power, 2014-02-07 23:43:11]

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

[commit-bot: I haz the power, 2014-02-07 23:43:11]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[mattm, 2014-02-07 23:55:28]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-02-07 23:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/137553004/2200001

[commit-bot: I haz the power, 2014-02-08 04:00:44]

Change committed as 249928