Use user specific NSSDatabase in CertLoader.

Use user specific NSSDatabase in CertLoader.

CertLoader is still global object, but it now loads only primary
user's certificates. Loading only primary user's certificates
is ok, since shill only uses primary user's network profile
(and currently only network stack is interested in certificates from CertLoader).

Added some tests for CertLoader and NetworkConnectionHandler.

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

[tbarzic, 2014-01-22 23:07:14]

Please take a look.
mattm: for NSSDB related stuff (mostly in cert_loader.cc and unittests).
nkostylev: owner for c/b/cros/login

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:598: if
(CertLoader::IsInitialized()) {
Do you have any suggestions for a better place to put this?

[mattm, 2014-01-23 01:45:55]

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:600:
CertLoader::Get()->StartWithUser(user->username_hash());
Should use GetNSSCertDatabaseForResourceContext from
chrome/browser/net/nss_context.h instead of having the CertLoader create a new
NSSCertDatabaseChromeOS. There might be some issue with CertLoader lifetime vs
ResourceContext lifetime to work out. But we have moved from having multiple
instances of the certdatabase objects in the past to a single shared one (which
should now be one per profile).

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
File chrome/browser/chromeos/options/cert_library.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
chrome/browser/chromeos/options/cert_library.cc:177: return
CertLoader::Get()->IsCertificateInPrivateSlot(*cert);
You could replace both is_hardware_backed and IsCertificateInPrivateSlot here
with the NSSCertDatabase::IsHardwareBacked function, unless you need explicitly
to tell that it is in that particular slot. (Which may actually be wrong with
the TPM system slot being added.)

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:238: base::Owned(new net::NSSCertDatabaseChromeOS(
hmm.. I think it would be preferable to have the NSSCertDatabase expose
asynchronous methods.

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader_un...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader_un...
chromeos/cert_loader_unittest.cc:101: database.reset(new
net::NSSCertDatabaseChromeOS(
(Creating database objects for the unittest is fine though.)

[tbarzic, 2014-01-23 04:45:27]

PTAL

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:600:
CertLoader::Get()->StartWithUser(user->username_hash());
On 2014/01/23 01:45:55, mattm wrote:
> Should use GetNSSCertDatabaseForResourceContext from
> chrome/browser/net/nss_context.h instead of having the CertLoader create a new
> NSSCertDatabaseChromeOS. There might be some issue with CertLoader lifetime vs
> ResourceContext lifetime to work out. But we have moved from having multiple
> instances of the certdatabase objects in the past to a single shared one
(which
> should now be one per profile).

Makes sense. Moved Db "creation" for the CertLoader to chrome/browser/.

As far as I understand, there should not be an issue with CertLoader lifetime vs
ResourceContext lifetime.
CertLoader is accessed only on UI thread, which stops running when profiles go
away.
An NSSCertDatabase is owned by ResourceContext, which lives on IO thread, and
it's destruction task is posted to IO thread on profile destruction. So, as long
as CertLoader only uses its NSSDB reference on IO thread, and the profile
destruction itself doesn't induce certificate loading in CertLoader, all tasks
issued by CertLoader to IO thread should be in message queue before the
ResourceContext deletion task.
Am I missing something here?

Off course, this means CertLoader cannot use WorkerPool for calling
database->ListCerts.

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
File chrome/browser/chromeos/options/cert_library.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chrome/browser/chromeos...
chrome/browser/chromeos/options/cert_library.cc:177: return
CertLoader::Get()->IsCertificateInPrivateSlot(*cert);
On 2014/01/23 01:45:55, mattm wrote:
> You could replace both is_hardware_backed and IsCertificateInPrivateSlot here
> with the NSSCertDatabase::IsHardwareBacked function, unless you need
explicitly
> to tell that it is in that particular slot. (Which may actually be wrong with
> the TPM system slot being added.)

Done.

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:238: base::Owned(new net::NSSCertDatabaseChromeOS(
On 2014/01/23 01:45:55, mattm wrote:
> hmm.. I think it would be preferable to have the NSSCertDatabase expose
> asynchronous methods.

I'm not sure that by itself would help here. Unless the NSSCertDatabase ensures
ListCerts is done on the right thread.

And if we want to avoid creating new NSSCertDatabase here, we'd have to post
ListCerts on IO thread (instead of worker thread). Is that OK?

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader_un...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader_un...
chromeos/cert_loader_unittest.cc:101: database.reset(new
net::NSSCertDatabaseChromeOS(
On 2014/01/23 01:45:55, mattm wrote:
> (Creating database objects for the unittest is fine though.)

ack

[Nikita (slow), 2014-01-23 17:15:37]

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:602: if
(CertLoader::IsInitialized())
When CertLoader is initialized?

What happens if it is not initialized at this point?

[tbarzic, 2014-01-23 17:47:55]

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:602: if
(CertLoader::IsInitialized())
On 2014/01/23 17:15:38, Nikita Kostylev wrote:
> When CertLoader is initialized?

It's initialized together with DBusServices on startup (during
ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart;
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
)

> 
> What happens if it is not initialized at this point?

It should be initialized at this point, unless it's not going to be initialized
at all (e.g. in unit tests).

[Nikita (slow), 2014-01-23 17:56:03]

chromeos/login lgtm

[stevenjb, 2014-01-23 18:17:41]

+pneubeck@

Looks good, mostly nits and questions.

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:664: return;
This is testing in case the callback occurs during shutdown? We should add a
comment to that effect, otherwise it seems redundant.

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/net/nss_cert_database_factory.h (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/net/nss_cert_database_factory.h:21: // It makes sure
that the NSSCertDatabase is fetched from the right tread
thread

https://codereview.chromium.org/135193007/diff/740031/chromeos/cert_loader_un...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/cert_loader_un...
chromeos/cert_loader_unittest.cc:38: << "though the private slot had been
initialized.";
nit: align

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:438: if
(TPMTokenLoader::IsInitialized())
This should always be true, shouldn't it?
CertLoader::Get()->is_hardware_backed() would be false otherwise, right? So
ClientCertificatesLoaded() should always be false and we shouldn't get here?

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:196: // will get reset at the
beginning of |ConnectToNetwork|.
Ugh. Subtle. Good catch, thanks.

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:452: if
(TPMTokenLoader::IsInitialized())
Again, can a cert be hardware backed and TPMTokenLoader not be initialized? If
this is only a problem in some tests, maybe we should have the tests create a
dummy TPMTokenLoader?

[tbarzic, 2014-01-23 19:18:36]

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:664: return;
On 2014/01/23 18:17:42, stevenjb wrote:
> This is testing in case the callback occurs during shutdown? We should add a
> comment to that effect, otherwise it seems redundant.

I was just about to remove this.

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
File chrome/browser/chromeos/net/nss_cert_database_factory.h (right):

https://codereview.chromium.org/135193007/diff/740031/chrome/browser/chromeos...
chrome/browser/chromeos/net/nss_cert_database_factory.h:21: // It makes sure
that the NSSCertDatabase is fetched from the right tread
On 2014/01/23 18:17:42, stevenjb wrote:
> thread

Done.

https://codereview.chromium.org/135193007/diff/740031/chromeos/cert_loader_un...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/cert_loader_un...
chromeos/cert_loader_unittest.cc:38: << "though the private slot had been
initialized.";
On 2014/01/23 18:17:42, stevenjb wrote:
> nit: align

Done.

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:438: if
(TPMTokenLoader::IsInitialized())
On 2014/01/23 18:17:42, stevenjb wrote:
> This should always be true, shouldn't it?
> CertLoader::Get()->is_hardware_backed() would be false otherwise, right? So
> ClientCertificatesLoaded() should always be false and we shouldn't get here?

It could have happened in tests (but started using dummy version)

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/135193007/diff/740031/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:452: if
(TPMTokenLoader::IsInitialized())
On 2014/01/23 18:17:42, stevenjb wrote:
> Again, can a cert be hardware backed and TPMTokenLoader not be initialized? If
> this is only a problem in some tests, maybe we should have the tests create a
> dummy TPMTokenLoader?

Done.

[stevenjb, 2014-01-23 19:32:22]

lgtm, but I would like pneubeck@ to take a look since he's more familiar with
the network cert stuff.

[mattm, 2014-01-23 19:46:51]

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:238: base::Owned(new net::NSSCertDatabaseChromeOS(
On 2014/01/23 04:45:28, tbarzic wrote:
> On 2014/01/23 01:45:55, mattm wrote:
> > hmm.. I think it would be preferable to have the NSSCertDatabase expose
> > asynchronous methods.
> 
> I'm not sure that by itself would help here. Unless the NSSCertDatabase
ensures
> ListCerts is done on the right thread.

Yeah, what I meant was NSSCertDatabase would internally handle safely doing the
posting to a worker thread to implement the asynchronous call. It could take the
task runner as a constructor arg so that tests don't need to use workers.
 
> And if we want to avoid creating new NSSCertDatabase here, we'd have to post
> ListCerts on IO thread (instead of worker thread). Is that OK?

Keeping this on a workerthread would be best.

[tbarzic, 2014-01-23 19:52:02]

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/210003/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:238: base::Owned(new net::NSSCertDatabaseChromeOS(
On 2014/01/23 19:46:52, mattm wrote:
> On 2014/01/23 04:45:28, tbarzic wrote:
> > On 2014/01/23 01:45:55, mattm wrote:
> > > hmm.. I think it would be preferable to have the NSSCertDatabase expose
> > > asynchronous methods.
> > 
> > I'm not sure that by itself would help here. Unless the NSSCertDatabase
> ensures
> > ListCerts is done on the right thread.
> 
> Yeah, what I meant was NSSCertDatabase would internally handle safely doing
the
> posting to a worker thread to implement the asynchronous call. It could take
the
> task runner as a constructor arg so that tests don't need to use workers.
>  
> > And if we want to avoid creating new NSSCertDatabase here, we'd have to post
> > ListCerts on IO thread (instead of worker thread). Is that OK?
> 
> Keeping this on a workerthread would be best.

OK, then I'll keep creating new instance NSSCertDatabaseChromeOS instance for
now and make NSSCertDatabase use Worker thread in a follow up. Sounds good?

Or would you prefer to reverse the order of patches :)

[pneubeck (no reviews), 2014-01-24 13:18:01]

Good work! lgtm
I have mostly minor comments.
Could you check whether this affects crbug.com/272596 ?

Also I wonder, if at some time we will have to make TPMTokenLoader also user
aware and provide one instance of TPMTokenLoader and CertLoader each per user?

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/nss_cert_database_factory.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.cc:26: // Gets
NSSCertDatabase for the resource context. It must be called on IO
Instead of the "It must be called on..." add a
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); ?

the same dcheck could be added to the function above.

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/nss_cert_database_factory.h (right):

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:18: namespace chromeos {
Is this really chromeos only?
If not, could we move it to nss_context.h, so that people find it more easily?

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:22: // (IO thread) and
run callback on the calling thread. The callback will always
s/run callback/the callback is run/ ?

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:24: void
GetNSSCertDatabaseForProfile(
It might be helpful to add a comment to GetNSSCertDatabaseForResourceContext in
nss_context.h how to get an NSSCertDatabase instance. I.e. explain that for
profiles they should use this factory function.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:103: void CertLoader::SetSlowTaskRunnerForTest(
nit:
If you already reorder the definitions, could you match it with the declaration
order?
Although it would be preferable to save all reordering for a separate CL.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:118: if (!database_ || !cert)
DCHECK(cert) (or let it crash in IsHardwareBacked below) instead of return false
?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:128: if (certificates_requested_)
this shouldn't be called more than once anymore, right?
then it should return silently and instead only CHECK(!database_);

all other occurrences of certificates_requested_ can be replaced by database_
and the former be removed.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:132: CHECK(database);
nit: not necessary, will crash anyways below.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:136: tpm_token_slot_id_ =
Has to be cached? Why not calculate every time?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:139: is_hardware_backed_ = hardware_backed_for_test_ ||
hardware_backed_for_test_ can only switch is_hardware_backed_ from false to
true.
Maybe rename it to "force_hardware_backed_for_test_" ?

Does this value even have to be cached? Why not calculate every time?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:168: // worker pool.
if you plan to change this, please add a TODO

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:10: #include "base/basictypes.h"
missing #include "base/compiler_specific.h"

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:24: class X509Certificate;
missing forward declaration of net::CertificateList

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:66: // Sets the task runner that any slow calls will be
made from, e.g. calls
nit: indentation

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:30: cert->os_cert_handle()))
nit: missing {}

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:36: void
NotReachedPrivateSlotCallback(crypto::ScopedPK11Slot slot) {
Is "NotReached.." the right prefix here?
Maybe Fail... ?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:37: ASSERT_FALSE(true) <<
"GetPrivateSlotForChromeOSUser callback called even "
EXPECT..., as it doesn't prevent a crash

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:54:
crypto::GetPublicSlotForChromeOSUser(primary_user_.username_hash()));
why only checking the public slot and not the private slot?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:72: new_certificates_loaded_events_count_++;
could EXPECT_TRUE(new_certificates_loaded_events_count == 0 || !initial_load)

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:77: bool
NewCertificatesLoadedEventsCountEquals(size_t value) {
for better debugging should return the count. then EXPECT_EQ( , ) can be used.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:88: if (!user->constructed_successfully())
what happens in the early return cases? shouldn't they instead be ASSERTed ?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:110: bool ImportCACerts(const std::string&
cert_file,
only used to import a single CA cert, thus could
ASSERT_EQ(imported_certs->size(),1) at the end

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:113: if (!database || !imported_certs)
this function can have return type void
and instead do all the ASSERTs itself (instead of at the calling site).
The error messages will be more helpful and the |success| variable below not
necessary.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:124: *imported_certs,
net::NSSCertDatabase::TRUSTED_SSL, &failed);
why not use the default trust bits? CertLoader shouldn't depend on the actual
trust.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:135: std::string pkcs12_data;
same as for ImportCACerts. could be void(...) and check for errors directly.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:169: ASSERT_TRUE(primary_db_);
could be moved into FinishUserInitAndGetDatabase

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:196:
ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
already tested in "Basic". same for the other tests.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:200:
ASSERT_TRUE(cert_loader_->certificates_loaded());
these two are covered in "Basic" as well.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:242:
EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
please comment why Importing a cert to the second db triggers an update of
CertLoader that is only initialized with the first db.
As this behavior doesn't seem to be relevant for the CertLoader's interface,
maybe just don't check the number of events here.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:256:
ASSERT_FALSE(cert_loader_->cert_list().empty());
I think it's unlucky that these initial part (lines 248-256) are just a
repetition of the basic test. Could you put these into a init function which is
called in each test case or derive a TestFixture that does just that?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:297:
EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
same comments as for CertLoaderNoUpdateOnSecondaryDbChanges

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:334: 
please consider adding one more test for trust change of a CA cert.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
chromeos/network/client_cert_resolver_unittest.cc:19: #include
"chromeos/login/login_state.h"
unused?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
chromeos/network/client_cert_resolver_unittest.cc:59: slot_ =
crypto::GetPrivateSlotForChromeOSUser(
nit: maybe rename slot_ to private_slot_

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_cert_migrator_unittest.cc:15: #include
"chromeos/login/login_state.h"
unused?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_cert_migrator_unittest.cc:67:
cert_loader_->set_hardware_backed_for_test();
shouldn't be necessary for these tests.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:434: cert_loader_->cert_list());
CertificateIsConfigured is private, why not move "cert_loader_->cert_list()"
there?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:456:
TPMTokenLoader::Get()->tpm_user_pin(),
will this change in the future too? so that there are multiple loaded
tpmtokens/one per user ?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:66:
CertLoader::Initialize();
can then the
 if (CertLoader::IsInitialized())

be removed from NetworkConnectionHandler::Init ?

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:276: ASSERT_EQ(1U,
certs.size());
could be moved to the import function

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:286:
TEST_F(NetworkConnectionHandlerTest, ConnectCertificateFromSecondaryUserFails) {
I think this is sufficiently tested at the CertLoader.
NetworkConnectionHandler only cares about what CerLoader->cert_list() returns.

[tbarzic, 2014-01-25 00:26:26]

there still is a leek from crbug.com/272596
I'll fix that separately

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/nss_cert_database_factory.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.cc:26: // Gets
NSSCertDatabase for the resource context. It must be called on IO
On 2014/01/24 13:18:02, pneubeck wrote:
> Instead of the "It must be called on..." add a
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); ?
> 
> the same dcheck could be added to the function above.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/nss_cert_database_factory.h (right):

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:18: namespace chromeos {
On 2014/01/24 13:18:02, pneubeck wrote:
> Is this really chromeos only?
> If not, could we move it to nss_context.h, so that people find it more easily?

hm, not really, it's more nss specific :)
moving it to nss_context.

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:22: // (IO thread) and
run callback on the calling thread. The callback will always
On 2014/01/24 13:18:02, pneubeck wrote:
> s/run callback/the callback is run/ ?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chrome/browser/chromeo...
chrome/browser/chromeos/net/nss_cert_database_factory.h:24: void
GetNSSCertDatabaseForProfile(
On 2014/01/24 13:18:02, pneubeck wrote:
> It might be helpful to add a comment to GetNSSCertDatabaseForResourceContext
in
> nss_context.h how to get an NSSCertDatabase instance. I.e. explain that for
> profiles they should use this factory function.

moved the method there

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:103: void CertLoader::SetSlowTaskRunnerForTest(
On 2014/01/24 13:18:02, pneubeck wrote:
> nit:
> If you already reorder the definitions, could you match it with the
declaration
> order?
> Although it would be preferable to save all reordering for a separate CL.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:118: if (!database_ || !cert)
On 2014/01/24 13:18:02, pneubeck wrote:
> DCHECK(cert) (or let it crash in IsHardwareBacked below) instead of return
false
> ?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:128: if (certificates_requested_)
On 2014/01/24 13:18:02, pneubeck wrote:
> this shouldn't be called more than once anymore, right?
> then it should return silently and instead only CHECK(!database_);
> 
> all other occurrences of certificates_requested_ can be replaced by database_
> and the former be removed.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:132: CHECK(database);
On 2014/01/24 13:18:02, pneubeck wrote:
> nit: not necessary, will crash anyways below.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:136: tpm_token_slot_id_ =
On 2014/01/24 13:18:02, pneubeck wrote:
> Has to be cached? Why not calculate every time?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:139: is_hardware_backed_ = hardware_backed_for_test_ ||
On 2014/01/24 13:18:02, pneubeck wrote:
> hardware_backed_for_test_ can only switch is_hardware_backed_ from false to
> true.
> Maybe rename it to "force_hardware_backed_for_test_" ?
> 
> Does this value even have to be cached? Why not calculate every time?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:168: // worker pool.
On 2014/01/24 13:18:02, pneubeck wrote:
> if you plan to change this, please add a TODO

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:10: #include "base/basictypes.h"
On 2014/01/24 13:18:02, pneubeck wrote:
> missing #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:24: class X509Certificate;
On 2014/01/24 13:18:02, pneubeck wrote:
> missing forward declaration of net::CertificateList

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader.h...
chromeos/cert_loader.h:66: // Sets the task runner that any slow calls will be
made from, e.g. calls
On 2014/01/24 13:18:02, pneubeck wrote:
> nit: indentation

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:30: cert->os_cert_handle()))
On 2014/01/24 13:18:02, pneubeck wrote:
> nit: missing {}

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:36: void
NotReachedPrivateSlotCallback(crypto::ScopedPK11Slot slot) {
On 2014/01/24 13:18:02, pneubeck wrote:
> Is "NotReached.." the right prefix here?
> Maybe Fail... ?

yeah, FailOn... sounds betterthan NotReached

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:37: ASSERT_FALSE(true) <<
"GetPrivateSlotForChromeOSUser callback called even "
On 2014/01/24 13:18:02, pneubeck wrote:
> EXPECT..., as it doesn't prevent a crash

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:54:
crypto::GetPublicSlotForChromeOSUser(primary_user_.username_hash()));
On 2014/01/24 13:18:02, pneubeck wrote:
> why only checking the public slot and not the private slot?

the private slot does not have to be set at this point (not until
FinishInitialization).

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:72: new_certificates_loaded_events_count_++;
On 2014/01/24 13:18:02, pneubeck wrote:
> could EXPECT_TRUE(new_certificates_loaded_events_count == 0 || !initial_load)

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:77: bool
NewCertificatesLoadedEventsCountEquals(size_t value) {
On 2014/01/24 13:18:02, pneubeck wrote:
> for better debugging should return the count. then EXPECT_EQ( , ) can be used.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:88: if (!user->constructed_successfully())
On 2014/01/24 13:18:02, pneubeck wrote:
> what happens in the early return cases? shouldn't they instead be ASSERTed ?

made it void and added ASSERTS

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:110: bool ImportCACerts(const std::string&
cert_file,
On 2014/01/24 13:18:02, pneubeck wrote:
> only used to import a single CA cert, thus could
> ASSERT_EQ(imported_certs->size(),1) at the end 

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:113: if (!database || !imported_certs)
On 2014/01/24 13:18:02, pneubeck wrote:
> this function can have return type void
> and instead do all the ASSERTs itself (instead of at the calling site).
> The error messages will be more helpful and the |success| variable below not
> necessary.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:124: *imported_certs,
net::NSSCertDatabase::TRUSTED_SSL, &failed);
On 2014/01/24 13:18:02, pneubeck wrote:
> why not use the default trust bits? CertLoader shouldn't depend on the actual
> trust.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:135: std::string pkcs12_data;
On 2014/01/24 13:18:02, pneubeck wrote:
> same as for ImportCACerts. could be void(...) and check for errors directly.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:169: ASSERT_TRUE(primary_db_);
On 2014/01/24 13:18:02, pneubeck wrote:
> could be moved into FinishUserInitAndGetDatabase

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:196:
ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
On 2014/01/24 13:18:02, pneubeck wrote:
> already tested in "Basic". same for the other tests.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:200:
ASSERT_TRUE(cert_loader_->certificates_loaded());
On 2014/01/24 13:18:02, pneubeck wrote:
> these two are covered in "Basic" as well.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:242:
EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
On 2014/01/24 13:18:02, pneubeck wrote:
> please comment why Importing a cert to the second db triggers an update of
> CertLoader that is only initialized with the first db.
> As this behavior doesn't seem to be relevant for the CertLoader's interface,
> maybe just don't check the number of events here.

Removed.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:256:
ASSERT_FALSE(cert_loader_->cert_list().empty());
On 2014/01/24 13:18:02, pneubeck wrote:
> I think it's unlucky that these initial part (lines 248-256) are just a
> repetition of the basic test. Could you put these into a init function which
is
> called in each test case or derive a TestFixture that does just that?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:297:
EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
On 2014/01/24 13:18:02, pneubeck wrote:
> same comments as for CertLoaderNoUpdateOnSecondaryDbChanges

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/cert_loader_u...
chromeos/cert_loader_unittest.cc:334: 
On 2014/01/24 13:18:02, pneubeck wrote:
> please consider adding one more test for trust change of a CA cert.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
chromeos/network/client_cert_resolver_unittest.cc:19: #include
"chromeos/login/login_state.h"
On 2014/01/24 13:18:02, pneubeck wrote:
> unused?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/clien...
chromeos/network/client_cert_resolver_unittest.cc:59: slot_ =
crypto::GetPrivateSlotForChromeOSUser(
On 2014/01/24 13:18:02, pneubeck wrote:
> nit: maybe rename slot_ to private_slot_

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_cert_migrator_unittest.cc:15: #include
"chromeos/login/login_state.h"
On 2014/01/24 13:18:02, pneubeck wrote:
> unused?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_cert_migrator_unittest.cc:67:
cert_loader_->set_hardware_backed_for_test();
On 2014/01/24 13:18:02, pneubeck wrote:
> shouldn't be necessary for these tests.

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:434: cert_loader_->cert_list());
On 2014/01/24 13:18:02, pneubeck wrote:
> CertificateIsConfigured is private, why not move "cert_loader_->cert_list()"
> there?

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:456:
TPMTokenLoader::Get()->tpm_user_pin(),
On 2014/01/24 13:18:02, pneubeck wrote:
> will this change in the future too? so that there are multiple loaded
> tpmtokens/one per user ?

Probably not.
We need TPMTokenLoader to load chaps module and initialize singleton
NSSCertDatabase (which should be gone soon). It's not really user specific :)

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:66:
CertLoader::Initialize();
On 2014/01/24 13:18:02, pneubeck wrote:
> can then the
>  if (CertLoader::IsInitialized())
> 
> be removed from NetworkConnectionHandler::Init ?

I think so, I'll add it back if trybots start complaining..

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:276: ASSERT_EQ(1U,
certs.size());
On 2014/01/24 13:18:02, pneubeck wrote:
> could be moved to the import function

Done.

https://codereview.chromium.org/135193007/diff/1230001/chromeos/network/netwo...
chromeos/network/network_connection_handler_unittest.cc:286:
TEST_F(NetworkConnectionHandlerTest, ConnectCertificateFromSecondaryUserFails) {
On 2014/01/24 13:18:02, pneubeck wrote:
> I think this is sufficiently tested at the CertLoader.
> NetworkConnectionHandler only cares about what CerLoader->cert_list() returns.

OK, removed the test.

[pneubeck (no reviews), 2014-01-27 08:23:23]

apart from the one more comment, the changes lgtm

https://codereview.chromium.org/135193007/diff/850002/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/850002/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:76: net::CertDatabase::GetInstance()->AddObserver(this);
oh. shouldn't this be
  database->AddObserver(this);
?
If not now but in the future, please add a TODO, as this is likely to be
overlooked.

[tbarzic, 2014-01-27 20:26:11]

https://codereview.chromium.org/135193007/diff/850002/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/135193007/diff/850002/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:76: net::CertDatabase::GetInstance()->AddObserver(this);
On 2014/01/27 08:23:24, pneubeck wrote:
> oh. shouldn't this be
>   database->AddObserver(this);
> ?
> If not now but in the future, please add a TODO, as this is likely to be
> overlooked.

added a comment about this.

[mattm, 2014-01-27 23:36:46]

lgtm

https://codereview.chromium.org/135193007/diff/1680001/chrome/browser/net/nss...
File chrome/browser/net/nss_context.cc (right):

https://codereview.chromium.org/135193007/diff/1680001/chrome/browser/net/nss...
chrome/browser/net/nss_context.cc:45: 
nit: only one blank line needed here

https://codereview.chromium.org/135193007/diff/1680001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/135193007/diff/1680001/chromeos/cert_loader.h...
chromeos/cert_loader.h:127: // Set during tests if |IsHArdwareBacked()| should
always return true.
s/IsHA/IsHa/

[tbarzic, 2014-01-28 02:46:50]

https://codereview.chromium.org/135193007/diff/1680001/chrome/browser/net/nss...
File chrome/browser/net/nss_context.cc (right):

https://codereview.chromium.org/135193007/diff/1680001/chrome/browser/net/nss...
chrome/browser/net/nss_context.cc:45: 
On 2014/01/27 23:36:47, mattm wrote:
> nit: only one blank line needed here

Done.

https://codereview.chromium.org/135193007/diff/1680001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/135193007/diff/1680001/chromeos/cert_loader.h...
chromeos/cert_loader.h:127: // Set during tests if |IsHArdwareBacked()| should
always return true.
On 2014/01/27 23:36:47, mattm wrote:
> s/IsHA/IsHa/

Done.

[commit-bot: I haz the power, 2014-01-28 02:53:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/135193007/1720001

[commit-bot: I haz the power, 2014-01-28 03:34:21]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-28 04:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/135193007/1720001

[commit-bot: I haz the power, 2014-01-28 06:17:02]

Change committed as 247414