NSSCertDatabaseChromeOS

Add NSSCertDatabaseChromeOS which uses the correct slots for each user.

Add GetNSSCertDatabaseForResourceContext to nss_context to get one.

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

[Ryan Sleevi, 2013-12-13 19:16:59]

I'm still trying to wrap my head around the design goal here for the
CertDatabase and NSSCertDatabase. It's not clear from reading this change what
direction you're going in, or where it stands after this change - which suggests
there's some more documentation needed, for sure, but I was hoping you can
expand a bit on things.

The design I've been trying to steer CertDatabase towards is making it a
per-profile concept; when Android implemented those bits, I tried to discourage
the CertDatabase::GetInstance() dependency in as many places, instead passing
the CertDatabase through.

I want to make sure we have as quick a solution as possible for the CrOS use
case, but I also want to make sure we're going towards that design - of passing
in the CertDatabase and NSSCertDatabase through.

As a design question, would the relationship between NSSCertDatabase and
CertDatabase be simpler if we focused first on threading it through the embedder
layers (eg: so it comes from the IO thread / ProfileIOData)? Seems like that may
avoid some of the Singleton/LazyInstance-ness.

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context.h (right):

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:40: // the lifetime of the process.
Process? Or IO thread?

It seems like the long-term goal is validity tied to the ResourceContext, which
your API should (artificially) restrict to here.

That is, I see no reason to commit to process-lifetime if the end-goal is
ResourceContext-lifetime.

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:45: WARN_UNUSED_RESULT;
Unnecessary?

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/resource...
File chrome/browser/resources/options/certificate_manager.js (right):

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/resource...
chrome/browser/resources/options/certificate_manager.js:217:
$('caCertsTab-import').disabled = false;
Seems like this change can/should be split into a follow-up CL. It doesn't seem
strictly related.

Then we can also discuss as to whether there should be a spinner/status when the
TPM is loading.

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database.cc
File net/cert/cert_database.cc (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database....
net/cert/cert_database.cc:14: return Singleton<CertDatabase,
LeakySingletonTraits<CertDatabase> >::get();
While a good change, it seems like this should be split into a small CL with
it's own note.

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database....
net/cert/cert_database.h:93: void AddSource(NSSCertDatabase* source);
Naming wise, I think this is fairly ambiguous.

Perhaps

ObserveNSSCertDatabase() ?

That said, I'm having trouble understanding why there isn't a 1:1 relationship
between NSSCertDatabase and CertDatabase here. When would one have a CertDB
observing multiple (distinct) NSS CertDBs?

https://codereview.chromium.org/111273002/diff/280001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:112: certs->begin(),
is this clang-format'd? I would assume it would be 

certs->erase(std::remove_if(
    certs->begin(),
    certs->end(),
    NSSProfile...()
));

[mattm, 2013-12-14 00:41:08]

On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> I'm still trying to wrap my head around the design goal here for the
> CertDatabase and NSSCertDatabase. It's not clear from reading this change what
> direction you're going in, or where it stands after this change - which
suggests
> there's some more documentation needed, for sure, but I was hoping you can
> expand a bit on things.

My goal was just to create a NSSCertDatabase for each chromeos user which (tries
to) do the right thing for each. I wasn't trying to clean up any interfaces or
ownership at the same time. 

> The design I've been trying to steer CertDatabase towards is making it a
> per-profile concept; when Android implemented those bits, I tried to
discourage
> the CertDatabase::GetInstance() dependency in as many places, instead passing
> the CertDatabase through.
> 
> I want to make sure we have as quick a solution as possible for the CrOS use
> case, but I also want to make sure we're going towards that design - of
passing
> in the CertDatabase and NSSCertDatabase through.
> 
> As a design question, would the relationship between NSSCertDatabase and
> CertDatabase be simpler if we focused first on threading it through the
embedder
> layers (eg: so it comes from the IO thread / ProfileIOData)? Seems like that
may
> avoid some of the Singleton/LazyInstance-ness.

Well, in this CL it does come through the ProfileIOData/ResourceContext (via
nss_context), it's just not the only way to access it or what owns it.

There are a lot of things under chromeos/ that use NSSCertDatabase::GetInstance,
so I wanted to provide a migration path for those. I don't know all the details
of how they all get created and what owns them and what their lifetimes are and
whether it's feasible to pass in the handle(s) from chrome/ instead. I guess we
could make the DB owned by the profile and make updating chromeos code to use it
also require updating that code to get it through nss_context.

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database....
net/cert/cert_database.h:93: void AddSource(NSSCertDatabase* source);
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> Naming wise, I think this is fairly ambiguous.
> 
> Perhaps
> 
> ObserveNSSCertDatabase() ?
> 
> That said, I'm having trouble understanding why there isn't a 1:1 relationship
> between NSSCertDatabase and CertDatabase here. When would one have a CertDB
> observing multiple (distinct) NSS CertDBs?

Since NSSCertDatabaseChromeOS subclasses NSSCertDatabase, and there can be
multiple NSSCertDatabaseChromeOS.  (Also there still exists the instance of the
non-subclassed NSSCertDatabase for all the existing users of
NSSCertDatabase::GetInstance() which still need to be updated.)

[mattm, 2013-12-18 04:00:50]

Split up and refactored.

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context.h (right):

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:40: // the lifetime of the process.
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> Process? Or IO thread?
> 
> It seems like the long-term goal is validity tied to the ResourceContext,
which
> your API should (artificially) restrict to here.
> 
> That is, I see no reason to commit to process-lifetime if the end-goal is
> ResourceContext-lifetime.

Switched to ResourceContext-lifetime.

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:45: WARN_UNUSED_RESULT;
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> Unnecessary?

Think it's worthwhile for the same reason as the others: If you don't check the
return value, you can't know whether you need to wait for the callback or not.

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/resource...
File chrome/browser/resources/options/certificate_manager.js (right):

https://codereview.chromium.org/111273002/diff/280001/chrome/browser/resource...
chrome/browser/resources/options/certificate_manager.js:217:
$('caCertsTab-import').disabled = false;
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> Seems like this change can/should be split into a follow-up CL. It doesn't
seem
> strictly related.
> 
> Then we can also discuss as to whether there should be a spinner/status when
the
> TPM is loading.

ok.

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database.cc
File net/cert/cert_database.cc (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database....
net/cert/cert_database.cc:14: return Singleton<CertDatabase,
LeakySingletonTraits<CertDatabase> >::get();
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> While a good change, it seems like this should be split into a small CL with
> it's own note.

Done.

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/cert_database....
net/cert/cert_database.h:93: void AddSource(NSSCertDatabase* source);
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> Naming wise, I think this is fairly ambiguous.
> 
> Perhaps
> 
> ObserveNSSCertDatabase() ?

done.

> That said, I'm having trouble understanding why there isn't a 1:1 relationship
> between NSSCertDatabase and CertDatabase here. When would one have a CertDB
> observing multiple (distinct) NSS CertDBs?

https://codereview.chromium.org/111273002/diff/280001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/111273002/diff/280001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:112: certs->begin(),
On 2013/12/13 19:16:59, Ryan Sleevi wrote:
> is this clang-format'd? I would assume it would be 
> 
> certs->erase(std::remove_if(
>     certs->begin(),
>     certs->end(),
>     NSSProfile...()
> ));

Yeah, this is clang-formatted.

[Ryan Sleevi, 2013-12-18 21:28:31]

It's progressing. I still think that there's a lot of missing comments to this
code that makes it difficult to review, let alone maintain, so I hope you can
take a good pass at it and liberally comment here.

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context_chromeos.cc (right):

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos.cc:58:
(*i).Run(nss_cert_database_.get());
Is there risk here of any of these callbacks causing the dtor to be run, thus
nuking nss_cert_database_?

The .swap() suggests "maybe", which is a little scary.

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos.cc:66: ReadyCallbackList;
Should be before the function - per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context_chromeos_browsertest.cc (right):

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos_browsertest.cc:24: class DBTester {
Please add comments throughout here to document, even if it's as simple as
explaining why you're using this helper DBTester class

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database....
net/cert/cert_database.h:93: void ObserveNSSCertDatabase(NSSCertDatabase*
source);
Per our discussion, this is temporary, right?

If so, please open a tracking bug (to fix the CertDatabase::GetInstance()
callers) and let's fix this.

Also, should it be public, or should it be private + friend class to
NSSCertDatabase? Since this feels like a "Temporary while refactoring", I'd
prefer the latter.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:57: << "NSSCertDatabaseChromeOS instead.";
comment nit: Make this a stronger warning.

"NSSCertDatabase::GetInstance() is deprecated. See http://crbug.com/foo"

I would suggest this at LOG(INFO) or LOG(ERROR) (similar to NOTIMPLEMENTED()) to
provide clear guidance to the code paths being hit that shouldn't be using this.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos_unittest.cc (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:26: if ((*it)->os_cert_handle()
== cert->os_cert_handle())
BUG? Are you sure comparing OS cert handle equality is what you want?

There are cases where it can be the same (logical, under the hood, PKCS#11
cert), but different CERTCertificate*s

Use X509Certificate::IsSameOSCert because of this

you could also:

return find_if(cert_list.begin(), cert_list.end(),
ptr_fun(X509Certificate::IsSameOSCert)) != cert_list.end()

if you like the STL functional approach :)

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:58:
CertDatabase::GetInstance()->RemoveObserver(this);
If either user_1_.constructed_successfully() or
user_2_.constructed_successfully() is false, you will attempt to remove an
invalid observer (since ASSERT_TRUE will short-circuit return)

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:74:
std::vector<CERTCertificate*> added_;
comments for |added_| and |added_ca_|

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:81:
TEST_F(NSSCertDatabaseChromeOSTest, ListModules) {
Please add a comment before each test describing what you're testing.

Please also feel free to liberally comment what each "logical block" within the
tests are doing.

Ideally, without 'reading code', one can quickly scan the comments to
understand:

What is being tested
 How each case is tested

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:120:
EXPECT_FALSE(certs_2[0]->os_cert_handle()->isperm);
I don't understand why you're doing this check. Same with 113.

[mattm, 2013-12-19 22:35:00]

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context_chromeos.cc (right):

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos.cc:58:
(*i).Run(nss_cert_database_.get());
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> Is there risk here of any of these callbacks causing the dtor to be run, thus
> nuking nss_cert_database_?
> 
> The .swap() suggests "maybe", which is a little scary.

After thinking about it more, I believe that can't actually happen. The manager
is owned by the ResourceContext/ProfileIOData. The ProfileIOData is owned by the
Profile.  When the Profile is shutting down, it will post a task to the IO
thread to delete the ProfileIOData. Since this is already running on the IO
thread, it can't be deleted since the delete task will still be waiting in the
message loop queue.

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos.cc:66: ReadyCallbackList;
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> Should be before the function - per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context_chromeos_browsertest.cc (right):

https://codereview.chromium.org/111273002/diff/320001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_chromeos_browsertest.cc:24: class DBTester {
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> Please add comments throughout here to document, even if it's as simple as
> explaining why you're using this helper DBTester class

Done.

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database....
net/cert/cert_database.h:93: void ObserveNSSCertDatabase(NSSCertDatabase*
source);
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> Per our discussion, this is temporary, right?
> 
> If so, please open a tracking bug (to fix the CertDatabase::GetInstance()
> callers) and let's fix this.

Hm, even after making CertDatabase a per-profile object, CertDatabase and
NSSCertDatabase would still be separate, I still think we need something like
this to combine the events from both. I might be forgetting something we
discussed.

> Also, should it be public, or should it be private + friend class to
> NSSCertDatabase? Since this feels like a "Temporary while refactoring", I'd
> prefer the latter.

If we friend the NSSCertDatabase, this could actually all be much simpler.
NSSCertDatabase could just directly call the CertDatabase->NotifyObservers
methods and we could drop the Notifier helper and the observer lists on
NSSCertDatabase, etc.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:57: << "NSSCertDatabaseChromeOS instead.";
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> comment nit: Make this a stronger warning.
> 
> "NSSCertDatabase::GetInstance() is deprecated. See http://crbug.com/foo%22
> 
> I would suggest this at LOG(INFO) or LOG(ERROR) (similar to NOTIMPLEMENTED())
to
> provide clear guidance to the code paths being hit that shouldn't be using
this.

Done.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos_unittest.cc (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:26: if ((*it)->os_cert_handle()
== cert->os_cert_handle())
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> BUG? Are you sure comparing OS cert handle equality is what you want?
> 
> There are cases where it can be the same (logical, under the hood, PKCS#11
> cert), but different CERTCertificate*s
> 
> Use X509Certificate::IsSameOSCert because of this

Done.
 
> you could also:
> 
> return find_if(cert_list.begin(), cert_list.end(),
> ptr_fun(X509Certificate::IsSameOSCert)) != cert_list.end()
> 
> if you like the STL functional approach :)

Would need a bit of extra magic to get the os_cert_handle, and to bind one arg
to the one we are looking for ..

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:58:
CertDatabase::GetInstance()->RemoveObserver(this);
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> If either user_1_.constructed_successfully() or
> user_2_.constructed_successfully() is false, you will attempt to remove an
> invalid observer (since ASSERT_TRUE will short-circuit return)

The observer list seems to be fine with trying to remove something not in it,
but I'll add a check anyway to be safe.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:74:
std::vector<CERTCertificate*> added_;
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> comments for |added_| and |added_ca_|

Done.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:81:
TEST_F(NSSCertDatabaseChromeOSTest, ListModules) {
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> Please add a comment before each test describing what you're testing.
> 
> Please also feel free to liberally comment what each "logical block" within
the
> tests are doing.
> 
> Ideally, without 'reading code', one can quickly scan the comments to
> understand:
> 
> What is being tested
>  How each case is tested

Done.

https://codereview.chromium.org/111273002/diff/320001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos_unittest.cc:120:
EXPECT_FALSE(certs_2[0]->os_cert_handle()->isperm);
On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> I don't understand why you're doing this check. Same with 113.

Yeah, it's not necessary here. copy-pasta from the other test, where it was
useful to diagnose test failures if the home-dir nss DB happened to have some of
the test certs in it for whatever reason. But that shouldn't affect these tests
since they don't open the home-dir db.

[Ryan Sleevi, 2013-12-20 01:35:19]

lgtm

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/111273002/diff/320001/net/cert/cert_database....
net/cert/cert_database.h:93: void ObserveNSSCertDatabase(NSSCertDatabase*
source);
On 2013/12/19 22:35:00, mattm wrote:
> On 2013/12/18 21:28:32, Ryan Sleevi wrote:
> > Per our discussion, this is temporary, right?
> > 
> > If so, please open a tracking bug (to fix the CertDatabase::GetInstance()
> > callers) and let's fix this.
> 
> Hm, even after making CertDatabase a per-profile object, CertDatabase and
> NSSCertDatabase would still be separate, I still think we need something like
> this to combine the events from both. I might be forgetting something we
> discussed.

Both CertDatabase and NSSCertDatabase are objects that should be
threaded-through per-profile.

When the IOThread initializes its CertDatabase, for the NSS case, it can
construct a CertDatabaseNSS and pass the NSSCertDatabase in (or we can fold
NSSCertDatabase back into CertDatabaseNSS if-and-only-if the only accesses to
the "NSS-specific" stuff is done at chrome/ or higher).

IIRC, the NSS-specific bits are used by chromeos/ as well, but beyond that, it's
all at chrome/?

Anyways, we can address this in a follow-up, but I think we've got a good
transition away from this dependency graph, and the "friend" is just to keep the
dependencies from growing.

> 
> > Also, should it be public, or should it be private + friend class to
> > NSSCertDatabase? Since this feels like a "Temporary while refactoring", I'd
> > prefer the latter.
> 
> If we friend the NSSCertDatabase, this could actually all be much simpler.
> NSSCertDatabase could just directly call the CertDatabase->NotifyObservers
> methods and we could drop the Notifier helper and the observer lists on
> NSSCertDatabase, etc.

[commit-bot: I haz the power, 2013-12-20 01:35:38]

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

[commit-bot: I haz the power, 2013-12-20 08:22:30]

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

[commit-bot: I haz the power, 2013-12-20 17:11:29]

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

[commit-bot: I haz the power, 2013-12-20 19:00:59]

Change committed as 242131